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

More debugging information in Cmm terms #2308

Merged
merged 4 commits into from Mar 13, 2019

Conversation

@mshinwell
Copy link
Contributor

commented Mar 11, 2019

Following on from GPR#851 and GPR#873, this pull request further enhances debugging information in Cmm terms. This was driven both by manually examining the debugger's behaviour and also by a report received from a user regarding substandard DWARF location information.

Some parts of this pull request will hook into future pull requests, in particular:

  • It relies on some debugging information coming from Clambda which isn't yet there.

  • It relies on the Switch module providing an interface that includes the transmission of more debugging information; see GPR#2294.

  • It features stub "placeholder debugging information" functions which will be filled in later. Using a trick, the gdb support provides the ability to debug at the Cmm level as well as the OCaml source level, in the initial instance for terms that had no OCaml source representation. This is useful for examining module initialisation functions, currying and partial application wrappers, etc. (In fact, when I demonstrated this work to some people at POPL, one well-known researcher said that this arguably seemed the most interesting feature.) The way the trick works is that the placeholder functions generate dummy Debuginfo.t values that are distinct from each other. The Cmm code of the startup file is formatted, with formatter hooks being used to retrieve the source location of the various subterms within the file. The Debuginfo.t values are then rewritten to use these locations.

There was a missing copyright header on the AFL instrumentation interface file which I took the liberty of adding here along with the other changes.

@chambart or @lthls : perhaps one of you could look at this, since @lthls has done a lot of work in the Cmm area recently and @chambart did GPR#873.

I strongly recommend using patdiff to review these changes. This will turn a big and scary diff into a relatively small and straightforward one---and, in particular, it provides obvious highlighting when there's an actual "real" change (which might indicate an error).

@stedolan

This comment has been minimized.

Copy link
Contributor

commented Mar 11, 2019

It looks like some changes to asmcomp/i386/selection.ml need to be added to this PR.

Apart from that, this looks good to me. It's an almost entirely mechanical change.

Is it definitely OK to change all of the temporary variable names from "foo" to "*foo*"? Is there any possibility that some of these names might leak into asm symbols and confuse a downstream assembler?

For the record: as the author of afl_instrument.mli (and the person who should have put a copyright notice there in the first place), I'm happy with what the notice added here says.

@mshinwell

This comment has been minimized.

Copy link
Contributor Author

commented Mar 12, 2019

Ah, you are right, I forgot to do the other architectures. Will do that this morning.

Regarding the identifiers: I should have pointed out this change specifically in the comment above, my apologies. Adding the asterisks follows an existing convention in the compiler and ensures that internally-generated variables can be identified. (I would like to have a proper flag on Ident, Variable and Backend_var, but that is going to have to wait.) The reason it is helpful to identify these variables for the DWARF work is that we can then assert that every non-internal variable (i.e. one that corresponds to a variable in the OCaml source code, although it may not actually be the same variable, due to inlining) must be marked with the identifier in the .cmt file whose type is to be used for printing values of such variable.

I don't think any of the Cmm variable names with asterisks will be lifted to symbols, but presumably names with asterisks from the front end might be. In this case I don't think there will be a problem because we escape symbols before emitting them into assembly files.

@mshinwell mshinwell force-pushed the mshinwell:cmm_debug branch from 0924085 to 0fda761 Mar 12, 2019

@stedolan
Copy link
Contributor

left a comment

(I was surprised that this change makes the code ~300 lines longer, but most of these new lines seem to be caused by dbg parameters pushing lines over the 80-char limit and causing new linebreaks)

@mshinwell mshinwell force-pushed the mshinwell:cmm_debug branch from 0fda761 to a927d1a Mar 13, 2019

@mshinwell

This comment has been minimized.

Copy link
Contributor Author

commented Mar 13, 2019

I've added a Changes entry; this is ready to merge once CI passes.

@stedolan stedolan merged commit 618e5db into ocaml:trunk Mar 13, 2019

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.