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

[PATCH] Remove need for -I directives to ocamldebug in common case #6270

Closed
vicuna opened this issue Dec 13, 2013 · 11 comments
Closed

[PATCH] Remove need for -I directives to ocamldebug in common case #6270

vicuna opened this issue Dec 13, 2013 · 11 comments

Comments

@vicuna
Copy link

vicuna commented Dec 13, 2013

Original bug ID: 6270
Reporter: jwatzmanfb
Assigned to: @xclerc
Status: closed (set by @xavierleroy on 2016-12-07T10:47:34Z)
Resolution: fixed
Priority: normal
Severity: minor
Category: tools (ocaml{lex,yacc,dep,debug,...})
Tags: patch
Related to: #6841
Monitored by: @gasche @yakobowski

Bug description

The need for a long list of -I directives makes interactively using ocamldebug a pain in the butt. Many folks have solved this with various find invocations or even Python wrappers, but those lead to other problems when it might include files you weren't expecting (or miss things you were). But all of this is really annoying since the tooling should be able to figure out itself, even heuristically, where your source files are -- gdb gets this right, why can't we?

This patch implements one of the more important heuristics from gdb: you typically debug on the same machine you built on, so looking for the source files and built artifacts in the absolute paths where they were during compilation is a good first try. We write out absolute paths into the debug events, and then automatically append the dirname from each such path into the load path. We also update source_of_module to use an absolute path if passed one, falling back on looking for the file's basename in the usual directory search if that absolute path isn't found. (Ideally these two mechanisms would be the same, but that's a problem for another day. :))

The load path appending behavior combined with the absolute path basename fallback mean that if you happen to be debugging on a machine where the original source and build artifacts are not available in their original absolute locations, things will work as before, using the standard load path mechanism. You can also explicitly use -I to prepend directories to the load path and override the defaults located by this new mechanism.

I personally find this makes using ocamldebug much more pleasant :)

(PS: This is my second time filing a Mantis ticket and sending a patch for ocaml, and my first hasn't been responded to yet (#6267). I wasn't able to find any documentation on your preferred format for receiving patches, so I hope this is acceptable. If you prefer something else, please let me know and I'd be happy to oblige!)

File attachments

@vicuna
Copy link
Author

vicuna commented Jan 13, 2014

Comment author: @yallop

The patch works as advertised for me, and seems like a significant usability improvement.

@vicuna
Copy link
Author

vicuna commented Jan 23, 2014

Comment author: @alainfrisch

(Disclaimer: comments below are only based on a quick read of the patch. I haven't tried it.)

Always turning filename into absolute paths in debug event locations can drawbacks:

  • Location displayed in backtraces shows filenames meaningful only on the development machine.

  • The size of the generated bytecode or .cds file could grow significantly, esp. because sharing is lost between all the pos_fname strings (always re-created from absolute_path). This could be mitigated by memoizing the absolute_path function.

@vicuna
Copy link
Author

vicuna commented Jan 23, 2014

Comment author: jwatzmanfb

  • Location displayed in backtraces shows filenames meaningful only on the development machine.

Ah, yeah, good point. Do you think it would be better to add a new field next to ev_loc, like ev_abs_path, that is used only for this purpose? I think this would work well for add_dirs_from_symbols but source_of_module would need some plumbing (which I'm willing to do, just checking first :)).

  • The size of the generated bytecode or .cds file could grow significantly, esp. because sharing is lost between all the pos_fname strings (always re-created from absolute_path). This could be mitigated by memoizing the absolute_path function.

Didn't realize the details of this, thanks for letting me know. I'll make sure this is fixed when I fix the above issue.

@vicuna
Copy link
Author

vicuna commented Jan 24, 2014

Comment author: @xclerc

I basically agree with Alain, and am wondering:
would it be acceptable to emit absolute paths in
debug events iff the "-absname" switch is passed
to the compiler? Of course, even if use of absolute
paths is triggered by a switch, sharing paths
between events is still useful.

@vicuna
Copy link
Author

vicuna commented Jan 24, 2014

Comment author: @alainfrisch

I basically agree with Alain, and am wondering:
would it be acceptable to emit absolute paths in
debug events iff the "-absname" switch is passed
to the compiler?

We use -absname for all our code (to facilitate jumping to the location of errors), but would not like to increase the size of bytecode/.cds file nor to include full paths in backtrace. (Internally we use bytecode only for internal development -- so we would not really mind about backtraces containing full paths. But some of our clients might recompile and use bytecode in production on platforms which don't have a native backend.)

@vicuna
Copy link
Author

vicuna commented Jan 24, 2014

Comment author: @alainfrisch

Do you think it would be better to add a new field next to ev_loc, like ev_abs_path, that is used only for this purpose?

Do we really want to include absolute path information directly in debug events? What about using an extra (optional) section in the .cds or bytecode files?

@vicuna
Copy link
Author

vicuna commented Jan 25, 2014

Comment author: jwatzmanfb

Do we really want to include absolute path information directly in debug events?

No, we probably don't need to, it was just the only way that I knew how to do it :)

What about using an extra (optional) section in the .cds or bytecode files?

That sounds like a better idea, I will look into this. Do you have a pointer to where the sections to the cds and bytecode files are defined/serialized, or documentation about it? If not, no worries, I'm sure I can dig in and figure it out eventually.

@vicuna
Copy link
Author

vicuna commented Feb 18, 2014

Comment author: jwatzmanfb

Given that you are now accepting pull requests on github, I just sent #14 as an update to this ticket.

Sorry to take so log to respond -- this is a side project for me and I just found time to work on it again earlier today :)

@vicuna
Copy link
Author

vicuna commented Mar 23, 2014

Comment author: jwatzmanfb

Ping again :) I'm told that one of xclerc or frisch needs to sign off on the patch getting incorporated into SVN -- would one of you mind to take a look and either commit the patch or provide feedback that I can use to make another revision?

The previous version has been sitting for about a month, and I just made another minor tweak to it which makes it even more non-intrusive. Would appreciate the review and am happy to keep revising it if it's still not acceptable, just let me know here or on GitHub.

Thanks!

@vicuna
Copy link
Author

vicuna commented Mar 31, 2014

Comment author: @xclerc

The patch, as reviewed/commented/amended on github,
has been committed in trunk (revision 14507).

@vicuna
Copy link
Author

vicuna commented Jun 27, 2014

Comment author: @lpw25

Any chance we could merge #58 into 4.02 as well, since it is basically a bugfix for this patch?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

1 participant