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] Include more useful information in ocamldebug "bt" #6267

Closed
vicuna opened this Issue Dec 12, 2013 · 8 comments

Comments

Projects
None yet
1 participant
@vicuna
Copy link
Collaborator

commented Dec 12, 2013

Original bug ID: 6267
Reporter: jwatzmanfb
Assigned to: @xclerc
Status: closed (set by @xavierleroy on 2015-12-11T18:25:39Z)
Resolution: fixed
Priority: normal
Severity: minor
Fixed in version: 4.01.1+dev
Category: tools (ocaml{lex,yacc,dep,debug,...})
Tags: patch
Monitored by: @gasche @yakobowski

Bug description

Backtraces in ocamldebug currently include only the program counter, module name, and character offset in the file of the frame. While perhaps useful for tools, this isn't terribly useful for humans, who typically think in terms of at least line numbers if not file names.

This patch adds the filename and line number into the backtrace information, for the benefit of human users. I, at least, find this format much more useful when debugging.

(PS: This is my first time filing a Mantis ticket or sending a patch for ocaml. 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

This comment has been minimized.

Copy link
Collaborator Author

commented Jan 13, 2014

Comment author: @xclerc

Seems good; I am only afraid of external tools
relying on the current formatting. Would it be
OK to trigger the proposed behavior through a
command-line option?

@vicuna

This comment has been minimized.

Copy link
Collaborator Author

commented Jan 13, 2014

Comment author: @gasche

If/when we decide we want this, could we add (one of the) column number(s) as well?

@vicuna

This comment has been minimized.

Copy link
Collaborator Author

commented Jan 13, 2014

Comment author: jwatzmanfb

Seems good; I am only afraid of external tools relying on the current formatting. Would it be OK to trigger the proposed behavior through a command-line option?

Sure, though it seems unfortunate to me to have reasonable behavior need to be explicitly turned on via an option instead of the default :) On the other hand, the option might also be able to turn off the pc and char, which aren't so useful for human operators. I'll muse on it (and let me know if you feel strongly).

EDIT: forgot to add -- do you think this is a serious danger? I deliberately appended the data so any tools using a regex on the front of the output wouldn't be affected. (Though that doesn't save us if they insist on matching the whole line.)

If/when we decide we want this, could we add (one of the) column number(s) as well?

Easy enough to do if there's demand? I personally wouldn't use it and it would just clutter things, but particularly if this becomes a "human-readable backtrace" mode then the clutter is much much less of an issue.

@vicuna

This comment has been minimized.

Copy link
Collaborator Author

commented Jan 13, 2014

Comment author: @gasche

I think disabling this with an optional command switch (--no-readable-trace?) would be fine. Re. column numbers: if the information is there, we can teach Emacs/vi to parse it jump to the right position in the file, which is substantially more helpful than going there by hand. "sourcefile:lineno:column" is (part of) a standardized format for GNU tools error location reporting ( http://www.gnu.org/prep/standards/standards.html#Errors ).

@vicuna

This comment has been minimized.

Copy link
Collaborator Author

commented Jan 13, 2014

Comment author: jwatzmanfb

Sold. I'll fix this up when I get some free time.

@vicuna

This comment has been minimized.

Copy link
Collaborator Author

commented Jan 20, 2014

Comment author: jwatzmanfb

New version of patch updated. I went with "-machine-readable" to allow it to potentially apply to other things in the future as well, and made "-emacs" imply it. Let me know what you think, and feel free to bikeshead about the name, I just picked something and don't feel strongly ;)

@vicuna

This comment has been minimized.

Copy link
Collaborator Author

commented Jan 23, 2014

Comment author: @xclerc

Patch applied in both trunk (revision 14416) and 4.01 (revision 14417) branches.

@vicuna

This comment has been minimized.

Copy link
Collaborator Author

commented Jan 23, 2014

Comment author: jwatzmanfb

Patch applied in both trunk (revision 14416) and 4.01 (revision 14417) branches.

Yay! Thanks so much. Interested in taking a look at #6270 as well? ;)

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.