-
Notifications
You must be signed in to change notification settings - Fork 18k
cmd/compile: function argument debug_info locations always wrong without GOEXPERIMENT=noregabi #45720
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
Comments
Hi, thanks for the heads up. This is a problem we're aware of and actively working on; please stay tuned for developments. At the moment with -N the compiler is emitting location expressions that describe where parameters would be if the register ABI were not turned on, which is clearly broken. The tentative plan is to emit a 2-piece location for each register param, the first part in the ABI register and then the stack home for the remainder of the function. For the program you mentioned, the "-l -N" prolog looks like
So the plan is to have the location expression for "x" look something like:
or something to this effect. Debug call injection is a separate issue; on that front we will have to ask for changes from Delve, since the debug call injection interface provided by the runtime has had to change with the regabi. |
Ok, sounds good, thank you. |
Change https://golang.org/cl/314431 mentions this issue: |
Change https://golang.org/cl/314930 mentions this issue: |
Change https://golang.org/cl/315071 mentions this issue: |
Change https://golang.org/cl/315610 mentions this issue: |
The SSA code for debug variable location analysis (for DWARF) has two special 'sentinel' values that it uses to handshake with the debugInfo.GetPC callback when capturing the PC values of debug variable ranges after prog generatoin: "BlockStart" and "BlockEnd". "BlockStart" has the expected semantics: it means "the PC value of the first instruction of block B", but "BlockEnd" does not mean "PC value of the last instruction of block B", but rather it is implemented as "the PC value of the last instruction of the function". This causes confusion when reading the code, and seems to to result in implementation flaws in the past, leading to incorrect ranges in some cases. To help with this, add a new sentinel "FuncEnd" (which has the "last inst in the function" semantics) and change the implementation of "BlockEnd" to actually mean what its name implies (last inst in block). Updates #45720. Change-Id: Ic3497fb60413e898d2bfe27805c3db56483d12a2 Reviewed-on: https://go-review.googlesource.com/c/go/+/314930 Trust: Than McIntosh <thanm@google.com> Run-TryBot: Than McIntosh <thanm@google.com> TryBot-Result: Go Bot <gobot@golang.org> Reviewed-by: Cherry Zhang <cherryyz@google.com> Reviewed-by: David Chase <drchase@google.com>
Revise the code that generates DWARF location expressions for input parameters to get it to work properly with the new register ABI when optimization is turned off. The previously implementation assumed stack locations for all input+output parameters when -N (disable optimization) was in effect. In the new implementation, a register-resident input parameter is given a 2-element location list, the first list element pointing to the ABI register(s) containing the param, and the second element pointing to the stack home once it has been spilled. NB, this change fixes a bunch of the Delve pkg/proc unit tests (maybe about half of the outstanding failures). Still a good number that need to be investigated, however. Updates #40724. Updates #45720. Change-Id: I743bbb9af187bcdebeb8e690fdd6db58094ca415 Reviewed-on: https://go-review.googlesource.com/c/go/+/314431 Trust: Than McIntosh <thanm@google.com> Trust: Cherry Zhang <cherryyz@google.com> Run-TryBot: Than McIntosh <thanm@google.com> TryBot-Result: Go Bot <gobot@golang.org> Reviewed-by: Cherry Zhang <cherryyz@google.com> Reviewed-by: David Chase <drchase@google.com>
When constructing multi-piece DWARF location expressions for struct-typed parameters using the register ABI, make sure that the location expressions generated properly reflect padding between elements (this is required by debuggers). Example: type small struct { x uint16 ; y uint8 ; z int32 } func ABC(p1 int, p2 small, f1 float32) { ... In the DWARF location expression for "p2" on entry to the routine, we need pieces for each field, but for debuggers (such as GDB) to work properly, we also need to describe the padding between elements. Thus instead of <rbx> DW_OP_piece 2 <rcx> DW_OP_piece 1 <rdi> DW_OP_piece 4 we need to emit <rbx> DW_OP_piece 2 <rcx> DW_OP_piece 1 DW_OP_piece 1 <rdi> DW_OP_piece 4 This patch adds a new helper routine in abiutils to compute the correct padding amounts for a struct type, a unit test for the helper, and updates the debug generation code to call the helper and insert apadding "piece" ops in the right spots. Updates #40724. Updates #45720. Change-Id: Ie208bee25776b9eb70642041869e65e4fa65a005 Reviewed-on: https://go-review.googlesource.com/c/go/+/315071 Trust: Than McIntosh <thanm@google.com> Run-TryBot: Than McIntosh <thanm@google.com> TryBot-Result: Go Bot <gobot@golang.org> Reviewed-by: David Chase <drchase@google.com>
In Cl 302071 we changed the compiler to use a different recipe for selecting the DWARF frame offset for output parameters, to reflect the fact that registerized output params don't have a stack memory location on entry to the function. In the process, however, we switched from using an abbrev pf DW_ABRV_PARAM to an abbrev of DW_ABRV_AUTO, which means that Delve can't recognize them correctly. To fix the problem, switch back to picking the correct abbrev entry, while leaving the new offset recipe intact. Updates #40724. Updates #45720. Change-Id: If721c9255bcd030177806576cde3450563f7a235 Reviewed-on: https://go-review.googlesource.com/c/go/+/315610 Trust: Than McIntosh <thanm@google.com> Run-TryBot: Than McIntosh <thanm@google.com> TryBot-Result: Go Bot <gobot@golang.org> Reviewed-by: Cherry Zhang <cherryyz@google.com>
@aarzilli I've submitted my CLs, so in theory this issue should be fixed now. Let me know if it is ok to close, or if you want to hold it open pending additional testing, that's fine too. Thanks. |
There's one last problem that I've found. Formal parameters need to appear inside a function's DIE in the same order as they appear in the argument list. For the most part this does happen however because of $GOROOT/src/cmd/compile/internal/dwarfgen/scope.go:48
this isn't always the case:
|
OK, thanks. This is not surprising given the various changes in frame offset for registerized outputs, etc. I'll look into sending a patch. |
I tried just disabling the sort and it seems to work, but I do not know if the input order can be trusted in all cases (I think it used to be wrong). |
Change https://golang.org/cl/315280 mentions this issue: |
For example with https://github.com/go-delve/delve/blob/master/_fixtures/issue573.go, the DIE for main.foo contains this:
Giving the location of argument x as DW_OP_call_frame_cfa, however this is not true until main.foo actually copies the argument (which is stored in a register) there:
the argument values aren't correct until 0x4965df and users trying to inspect them will see junk before that point. Delve also uses this information to implement call injection. I can see a few solutions to this:
-N
is specified, I believe this is what C compilers do when optimizations are turned off.-N
implyGOEXPERIMENT=noregabi
IMHO the preferred solution would be (2) or (3) because it makes call injection work and lets users debug something that still has regabi enabled, but I don't know if either is possible given the timeline.
Regarding (4): delve could easily specify GOEXPERIMENT itself when compiling however I think it should be implied by
-N
: we've been telling users that-N -l
is what produces a binary that's easy to debug and with this it would be no longer true. I also don't think-N
would be useful to anyone, without GOEXPERIMENT=noregabi.cc @dr2chase
The text was updated successfully, but these errors were encountered: