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

cmd/compile: make optimized functions' parameter stack layout available to debuggers #27039

Closed
heschik opened this issue Aug 16, 2018 · 14 comments

Comments

Projects
None yet
6 participants
@heschik
Copy link
Contributor

commented Aug 16, 2018

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; putPrunedScopes reorders 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

@heschik heschik added the Debugging label Aug 16, 2018

@aarzilli

This comment has been minimized.

Copy link
Contributor

commented Aug 16, 2018

and the location lists aren't reliable enough to use to form the stack frame.

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.

@heschik

This comment has been minimized.

Copy link
Contributor Author

commented Aug 16, 2018

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.

@cherrymui

This comment has been minimized.

Copy link
Contributor

commented Aug 16, 2018

Couldn't the parameter stack layout be derived from the type of the function?

@heschik

This comment has been minimized.

Copy link
Contributor Author

commented Aug 16, 2018

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.

@cherrymui

This comment has been minimized.

Copy link
Contributor

commented Aug 17, 2018

I was just thinking what gdb's ptype uses. It seems it is the formal parameter DIEs, and as you mentioned, the order is sometimes incorrect. Maybe it makes sense to make the the formal parameter DIEs to have the correct order, so ptype would also work. (I don't know how difficult it is, though.)

@aarzilli

This comment has been minimized.

Copy link
Contributor

commented Aug 17, 2018

For the record, I'd be ok with using runtime._type, I didn't think of that.

@heschik

This comment has been minimized.

Copy link
Contributor Author

commented Aug 17, 2018

@cherrymui's point led me to do what I really should've done before I filed this bug -- read the spec.

3.3.4 Declarations Owned by Subroutines and Entry Points

The declarations enclosed by a subroutine or entry point are represented by debugging information entries that are owned by the subroutine or entry point entry. Entries representing the formal parameters of the subroutine or entry point appear in the same order as the corresponding declarations in the source program.

So we are actually in violation of the spec.

@derekparker

This comment has been minimized.

Copy link
Contributor

commented Aug 17, 2018

@heschik can we get input from @thanm on why the parameters are being sorted in an optimized function.

@heschik

This comment has been minimized.

Copy link
Contributor Author

commented Aug 17, 2018

He's OOO for a couple days, I'm sure we'll get it sorted out (rimshot) soon though.

@thanm

This comment has been minimized.

Copy link
Member

commented Aug 27, 2018

[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.

@gopherbot

This comment has been minimized.

Copy link

commented Aug 28, 2018

Change https://golang.org/cl/131895 mentions this issue: cmd/compile: remove var sorting from DWARF inline generation

@derekparker

This comment has been minimized.

Copy link
Contributor

commented Aug 28, 2018

Thanks @thanm.

gopherbot pushed a commit that referenced this issue Aug 29, 2018

cmd/compile: remove var sorting from DWARF inline generation
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>
@heschik

This comment has been minimized.

Copy link
Contributor Author

commented Oct 12, 2018

I may have forgotten some context, but the change above is sufficient, right?

@thanm

This comment has been minimized.

Copy link
Member

commented Oct 12, 2018

@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.

@thanm thanm closed this Oct 12, 2018

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.