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

Add labels around call sites (+ some background on DWARF call site support) #2316

Open
wants to merge 2 commits into
base: trunk
from

Conversation

@mshinwell
Copy link
Contributor

commented Mar 12, 2019

This pull request generalises the existing support in the Mach language for "labels immediately after call sites" to "labels immediately before and after call sites". Such labels are required to emit DWARF information that describes the position of call sites. (This is a DWARF-4 GNU extension adopted as standard in DWARF-5.)

Emitting comprehensive DWARF information for all call sites enables many more variables to be available and/or printed properly in the debugger than would otherwise be the case. This behaviour relies on the debugger being able to answer the following question. Suppose we are in some function that has associated with it a stack frame (known as the "current frame"). There is a previous (older) frame immediately above us on the stack which can be discovered by unwinding. The question is whether that previous frame corresponds to the caller of the function. (It might not because there may have been one or more tail calls in the middle.) gdb can provide various answers to this question, one of which is "definitely yes", if the DWARF information is comprehensive enough. This requires the compilier to describe all call sites that might be involved in such stack frame relations and to say whether or not they are tail calls. The OCaml compiler, after the DWARF patches, emits sufficient information for gdb to be able to do this.

This has two implications that are very useful in practice:

  • Known arguments (e.g. the constant None) at call sites, which will be inferrable by an enhanced version of Available_regs to be presented shortly, can be described. This means that if the parameter becomes unavailable in the callee, it can often still be recovered, as gdb will look at the DWARF information associated with the call site if the answer to the above question is "definitely yes". (Support for this particular kind of argument recovery was what the DWARF call site support was originally intended for. It is associated with the term "entry values" in gdb terminology.)

  • We can sometimes, in a callee, find the OCaml types of its arguments even if the types of the parameters of the callee are polymorphic. (The OCaml types are required to print values properly.)
    This is done by examining the DWARF information for the relevant call site, on the condition that the answer to the above question is "definitely yes". The idea is that at the call site of a polymorphic function, the type(s) of the function's polymorphic argument(s) are more likely to be monomorphic. This support is rudimentary at present (and could probably be confused by polymorphic recursion), but it is sufficient for a fair number of invocations of polymorphic functions such as List.map to have their arguments printed correctly. Improving the support for type recovery would likely only involve changes in libmonda, not the compiler.

@stedolan Could you take a first pass of review please? patdiff is probably helpful for this one. There is some duplication for the "maybe_..." functions as you will see, but that will go away in the future when Asm_directives arrives. Likewise, the condition as to whether certain of the labels should be enabled cannot yet be wired through, as the pull request that adds new command-line flags for debugging information output control has not yet been presented.

I added a couple of begin and end pairs in one-line conditionals to improve code clarity. I've made mistakes in the emitters in the past due to confusion relating to these.

Call sites produced in the emitter (e.g. caml_call_gc) will not currently have the necessary labels nor the necessary data structures associated with them to obtain a list of all such call sites that have been inserted during emission. This will be dealt with in a future pull request.

@lthls

This comment has been minimized.

Copy link
Contributor

commented Mar 19, 2019

I'm looking at it and will post a review soon.

@lthls

lthls approved these changes Mar 19, 2019

Copy link
Contributor

left a comment

I've reviewed the patch and it is correct.

But, I'm afraid I find it very unpleasant that the patch contains a lot of code that is duplicated, and relies on an invariant that is neither dynamically checked nor statically prevented.

I'm still approving this, because there's a lot of work on the DWARF information generation that still needs to be done and I don't want to hold it up more than necessary, but when time permits I would like to see some improvements made on at least two issues:

  • Add some logic to prevent double emission of the after label, and refactor the maybe_add_label and record_frame logic around calls.

  • Move as many helper functions as possible out of the emit.mlp files. The supports_dwarf_call_sites function could go in Arch or Proc, and some of the common logic involved in wrapping calls could be lifted to Emitaux.

@nojb

This comment has been minimized.

Copy link
Contributor

commented Mar 19, 2019

I'm still approving this, because there's a lot of work on the DWARF information generation that still needs to be done and I don't want to hold it up more than necessary, but when time permits I would like to see some improvements made on at least two issues:

I don't know anything about this particular PR, but in my opinion this reasoning is problematic. @gasche made a similar observation recently and I also feel that merging suboptimal code with the hope that it will be cleaned up later is not the way to go.

@mshinwell

This comment has been minimized.

Copy link
Contributor Author

commented Mar 20, 2019

Yeah, I think we should work on this some more -- as above, I was only after a first pass of review. I doubt I will have time this week, but I'm going to see @lthls next week, and we can try to find a solution then.

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.