Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Envaux.env_from_summary: guard against failure to find module #1987

Merged
merged 2 commits into from Aug 21, 2018

Conversation

Projects
None yet
2 participants
@nojb
Copy link
Contributor

commented Aug 11, 2018

See MPR#7830. There it is reported that ocamldebug will crash when trying to print any value if the .cmi of any opened module is not found.

To reproduce, make an empty file empty.ml, and put the following in main.ml:

open Empty
let f () = ()
let () = ()

Then compile with

ocamlc -g empty.ml main.ml
rm empty.cmi

and run the resulting program under ocamldebug with

ocamldebug a.out
(ocd) break @ Main 1
(ocd) r
(ocd) print foo
Uncaught exception: Not_found

I traced the error to the function Envaux.env_from_summary (this function is only used in the debugger). There the case Env_open will raise Not_found if the .cmi file in question is not found (the exception is raised in Env.find_pers_struct).

This part of the code was last touched in 3d03736 (cc: @alainfrisch), where an exception handler for Not_found was removed. Based on that change, this PR restores a handler for Not_found, but I am not sure it is the right fix (it gives a sensible error message in the case above though).

Some questions/remarks:

  • I couldn't quite figure out how the Not_found exception raised by find_pers_struct is handled in the rest of the compiler.
  • Even if this fix is correct, the debugger should not error out if a .cmi file is not found, especially if it is not required for anything. Even if it is, at most it should make some types abstract, like in the toplevel.
@gasche

This comment has been minimized.

Copy link
Member

commented Aug 15, 2018

This would need a Changes entry. Maybe we could consider having this in the 4.07 maintenance branch as well?

@nojb

This comment has been minimized.

Copy link
Contributor Author

commented Aug 16, 2018

I would still appreciate a second opinion on whether this is the right fix here.

@gasche

This comment has been minimized.

Copy link
Member

commented Aug 16, 2018

I reviewed the change and I believe from looking at Alain's patch that the change that you are doing is correct, not a regression. That said, I don't know much about this part of the codebase so I can't comment on whether this is "the right" fix.

@nojb nojb force-pushed the nojb:fix_env_of_summary_open branch from 556c524 to 3be3f57 Aug 21, 2018

@nojb

This comment has been minimized.

Copy link
Contributor Author

commented Aug 21, 2018

Planning to merge this one-line change unless someone objects.

@nojb nojb merged commit 6aab769 into ocaml:trunk Aug 21, 2018

1 of 2 checks passed

continuous-integration/appveyor/pr AppVeyor build failed
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@gasche

This comment has been minimized.

Copy link
Member

commented Aug 21, 2018

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.