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

Debugging-related changes in the Mach language and passes #2303

Open
wants to merge 5 commits into
base: trunk
from

Conversation

@mshinwell
Copy link
Contributor

commented Mar 8, 2019

This pull request contains the changes necessary to the Mach language and its associated passes for gdb support. I'm fairly pleased with these changes insofar as they have, in the end, worked out to be fairly modest in scale.

The main thing here is the introduction of a new type, Insn_debuginfo.t, which is used all the way through to the emitters as a replacement for Debuginfo.t on instructions. It encapsulates the existing Debuginfo.t structure together with various register and variable availability sets. The introduction of this type means that the Mach.instruction type goes back to looking very nearly how it used to, save that the dbg field is mutable.

The interface for creating Mach instructions has been reworked a little. I think the new version provides an improvement in readability. It also incorporates the new types; and, crucially, stops creating instructions that have empty debugging information fields. (The forthcoming patch to Selectgen will contain a few changes to ensure this property holds when we first create Mach code.)

In the new world, we need every instruction to have non-empty debugging information, as far as is reasonably possible. The reason is that we track an approximation to lexical scopes right through from the Lambda code. The scope information is contained within an enhanced Debuginfo.t type (to be presented in due course) and is used to assign instructions to DWARF lexical blocks, which are nested possibly-discontiguous ranges of code. In turn, these lexical blocks affect the visibility of local variables in the debugger.

This visibility control is important since, in OCaml programs, the entirety of a function often contains a large number of variables. If they all appear at all program points within such function in the debugger, with of course many of them showing as unavailable, it becomes unmanageable.

If a given instruction lacks debugging information, it will be assigned to the toplevel lexical block of the enclosing function, which means that when the user lands on that instruction in the debugger local variables may suddenly become invisible.

The changes to the Mach passes themselves are straightforward; with one exception, these are just coping with the interface change.

The exception relates to the insertion of spill instructions. As noted in the code, we need to ensure that spill instructions do not get positioned in the wrong place relative to the Iname_for_debugger pseudooperation. It is straightforward to ensure this.

Apart from that, there is the introduction of a field to carry phantom let information on function declarations; correction of one spelling mistake; and I think that's all. There are probably some "dangling" references to things not yet merged in some of the comments, but those will resolve themselves once the remainder of the gdb support is in.

@chambart @lthls Could you take a look at this in the first instance?

@mshinwell mshinwell requested a review from chambart Mar 8, 2019

@mshinwell

This comment has been minimized.

Copy link
Contributor Author

commented Mar 8, 2019

I forgot one thing: we also need to generalise the "label_after" fields in Mach operations, so that we have the necessary labels in place for emitting DWARF call site information. This change is easy and can be done as a separate GPR.

Edit: This is now #2316 .

@mshinwell mshinwell force-pushed the mshinwell:insn_debuginfo branch from 26577ab to 604a3ae Mar 8, 2019

@lthls

lthls approved these changes Mar 15, 2019

Copy link
Contributor

left a comment

Apart from one small question, the PR looks fine (it is mostly mechanical changes guided by the changes in types).

Reg.Set.fold
(fun r i -> instr_cons (Iop Ispill) [|r|] [|spill_reg r|] i)
regset i
let regset = Reg.Set.elements regset in

This comment has been minimized.

Copy link
@lthls

lthls Mar 15, 2019

Contributor

Is there a reason why you chose to call Reg.Set.elements then List.fold_left instead of keeping the original Reg.Set.fold ?

This comment has been minimized.

Copy link
@mshinwell

mshinwell Mar 19, 2019

Author Contributor

Unsure, this was probably a left-over from a previous version. Fixed.

@mshinwell mshinwell force-pushed the mshinwell:insn_debuginfo branch from 604a3ae to 4f3dc20 Mar 19, 2019

@mshinwell

This comment has been minimized.

Copy link
Contributor Author

commented Mar 19, 2019

@lthls Please check the new changesets and approve, thanks.

@mshinwell

This comment has been minimized.

Copy link
Contributor Author

commented Mar 19, 2019

I have also sent this to precheck (#227).

@mshinwell

This comment has been minimized.

Copy link
Contributor Author

commented Mar 27, 2019

Precheck seemed to go wrong for this one, so I've tried again (precheck#233).

@lthls

lthls approved these changes Apr 3, 2019

Copy link
Contributor

left a comment

I've re-reviewed, and didn't notice anything more suspicious than last time, so my approval still stands.

@mshinwell mshinwell force-pushed the mshinwell:insn_debuginfo branch from 4f3dc20 to 87319a0 Apr 4, 2019

@mshinwell

This comment has been minimized.

Copy link
Contributor Author

commented Apr 4, 2019

Thanks -- I've rebased this, so it can be merged if the CI passes.

@mshinwell mshinwell force-pushed the mshinwell:insn_debuginfo branch from 3802a2f to 09acc46 Apr 8, 2019

@mshinwell

This comment has been minimized.

Copy link
Contributor Author

commented Apr 8, 2019

@xavierleroy Without asking you to read the whole patch, could you confirm whether you are happy with the changes to the Mach interface, and the principle of collating the various debug-related fields into the new Insn_debuginfo type? These are the main principles behind this patch.

@xavierleroy

This comment has been minimized.

Copy link
Contributor

commented Apr 9, 2019

I'm missing a global view on all your "debugging-prerequisite" PRs. Could we wait for the next developers meeting to see where all that fits in our future roadmap?

@mshinwell

This comment has been minimized.

Copy link
Contributor Author

commented Apr 9, 2019

@xavierleroy Will move to caml-devel.

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