cmd/compile: make optimized functions' parameter stack layout available to debuggers #27039
Comments
IMHO this is worth fixing regardless of call injection: the option of debugging optimized binaries has been ventilated on multiple occasions and this is a big downside of doing that. |
|
I agree, but for reasons like the unused argument issue I mentioned above, I expect it will be difficult to get the location lists completely reliable. (Practically, there are also quite a few accuracy issues with them too, so it would be a long time coming no matter what.) But special handling for parameters shouldn't be too hard. I'm leaning slightly in that direction. |
|
Couldn't the parameter stack layout be derived from the type of the function? |
|
Do you mean the runtime._type? I think Delve does understand those to some extent, but I think we'd rather keep that to a minimum. For example, I implemented DW_AT_go_runtime_type to make it possible to resolve interface types without reading runtime.moduledata. |
|
I was just thinking what gdb's |
|
For the record, I'd be ok with using runtime._type, I didn't think of that. |
|
@cherrymui's point led me to do what I really should've done before I filed this bug -- read the spec.
So we are actually in violation of the spec. |
|
@heschik can we get input from @thanm on why the parameters are being sorted in an optimized function. |
|
He's OOO for a couple days, I'm sure we'll get it sorted out (rimshot) soon though. |
|
[Back from vacation, now catching up on email] Sorting: I added this to make things easier for generation of DWARF inline info DIEs. At the point where DWARF is generated we're seeing a post-optimization version of the function (or to be more accurate, several combined functions due to inlining), each inlined body has to be matched up to pre-optimization decl list (not as easy as you might thing, since vars can be deleted, split into pieces, renamed, etc). The original version of the code just looked at the source position info for the variables to determine declaration order, but that proved to be problematic (front end was sometimes assigning the same location to two different vars). I could certain make an attempt at refactoring things to get rid of the sorting if that would be helpful. |
|
Change https://golang.org/cl/131895 mentions this issue: |
|
Thanks @thanm. |
When generation DWARF inline info records, the current implementation includes a sorting pass that reorders a subprogram's child variable DIEs based on class (param/auto) and name. This sorting is no longer needed, and can cause problems for a debugger (if we want to use the DWARF info for creating a call to an optimized function); this patch removes it. Ordering of DWARF subprogram variable/parameter DIEs is still deterministic with this change, since it is keyed off the order in which vars appear in the pre-inlining function "Dcl" list. Updates #27039 Change-Id: I3b91290d11bb3b9b36fb61271d80b801841401ee Reviewed-on: https://go-review.googlesource.com/131895 Reviewed-by: Heschi Kreinick <heschi@google.com>
|
I may have forgotten some context, but the change above is sufficient, right? |
|
@heschik yes I think we should be good to go. Can't quite recall why I didn't close out the bug earlier, but I will do that now. |
To call a function that takes arguments (and to read the result) debuggers need to be able to form a stack frame for the function. With knowledge of the argument types and their order, this is pretty straightforward. In an unoptimized program, our DWARF exposes that information in two ways: first, the formal_parameter DIEs are in stack order, so you can infer the layout, and second, they have their home stack slot offsets as their locations.
In an optimized function, neither of these is usable;
putPrunedScopesreorders the parameter DIEs, and the location lists aren't reliable enough to use to form the stack frame.I'm not sure what the right way forward is. We could fix one or both of the above, or introduce a new custom attribute that defines the "canonical" stack slot for a parameter.
Retain DIE order: I don't have a clear sense of why the vars need to be sorted; @thanm will probably know more here. It's a little bit unsatisfying to rely on DIE ordering though.
Improve location lists: This will be very hard to get right without simply giving arguments special treatment. For example, the current location list generation logic will not notice unused arguments at all. It should be straightforward to generate location entries for each parameter at function entry, though, and we could just do that.
New custom attribute: easy, but really feels like giving up. I'd personally rather leave this as a last resort.
cc @dr2chase, @aarzilli, @derekparker
The text was updated successfully, but these errors were encountered: