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: function argument debug_info locations always wrong without GOEXPERIMENT=noregabi #45720

Closed
aarzilli opened this issue Apr 23, 2021 · 11 comments
Assignees
Labels
Milestone

Comments

@aarzilli
Copy link
Contributor

@aarzilli aarzilli commented Apr 23, 2021

$ go version
go version devel go1.17-5963f0a332 Fri Apr 23 05:38:47 2021 +0000 linux/amd64

For example with https://github.com/go-delve/delve/blob/master/_fixtures/issue573.go, the DIE for main.foo contains this:

    <639c2>   DW_AT_name        : x
    <639c4>   DW_AT_variable_parameter: 0
    <639c5>   DW_AT_decl_line   : 17
    <639c6>   DW_AT_type        : <0x359e0>
    <639ca>   DW_AT_location    : 1 byte block: 9c      (DW_OP_call_frame_cfa)

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:

	issue573.go:17	0x4965a0	4c8d6424e0		lea r12, ptr [rsp-0x20]
	issue573.go:17	0x4965a5	4d3b6610		cmp r12, qword ptr [r14+0x10]
	issue573.go:17	0x4965a9	0f867b010000		jbe 0x49672a
=>	issue573.go:17	0x4965af*	4881eca0000000		sub rsp, 0xa0
	issue573.go:17	0x4965b6	4889ac2498000000	mov qword ptr [rsp+0x98], rbp
	issue573.go:17	0x4965be	488dac2498000000	lea rbp, ptr [rsp+0x98]
	issue573.go:17	0x4965c6	48898424a8000000	mov qword ptr [rsp+0xa8], rax
	issue573.go:17	0x4965ce	48899c24b0000000	mov qword ptr [rsp+0xb0], rbx
	issue573.go:17	0x4965d6	48c744242800000000	mov qword ptr [rsp+0x28], 0x0
	issue573.go:18	0x4965df	488d4c2468		lea rcx, ptr [rsp+0x68]

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:

  1. move the prologue_end marker way down, until after the arguments have been saved. This would mean that call injection is still broken and would have to be disabled but users wouldn't generally see junk inside function arguments.
  2. have callers do the saving to the stack after copying the arguments to registers if -N is specified, I believe this is what C compilers do when optimizations are turned off.
  3. actually have a correct argument list for function arguments where function arguments have a register as location until they are saved to the stack
  4. Have -N imply GOEXPERIMENT=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

@thanm
Copy link
Contributor

@thanm thanm commented Apr 23, 2021

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

/home/thanm/issue573.go:17
  47f660:       lea    -0x20(%rsp),%r12
  47f665:       cmp    0x10(%r14),%r12
  47f669:       jbe    47f7ea <main.foo+0x18a>
  47f66f:       sub    $0xa0,%rsp
  47f676:       mov    %rbp,0x98(%rsp)
  47f67e:       lea    0x98(%rsp),%rbp
  47f686:       mov    %rax,0xa8(%rsp)
  47f68e:       mov    %rbx,0xb0(%rsp)
  47f696:       movq   $0x0,0x28(%rsp)

So the plan is to have the location expression for "x" look something like:

    00087e80 ffffffffffffffff 000000000047f660 (base address)
    00087e90 000000000047f660 000000000047f686 (DW_OP_reg0 (rax))
    00087ea3 000000000047f68e 000000000047f6b9 (DW_OP_fbreg: -168)

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.

Loading

@aarzilli
Copy link
Contributor Author

@aarzilli aarzilli commented Apr 23, 2021

Ok, sounds good, thank you.

Loading

@cherrymui cherrymui added this to the Go1.17 milestone Apr 26, 2021
@thanm thanm self-assigned this Apr 30, 2021
@gopherbot
Copy link

@gopherbot gopherbot commented Apr 30, 2021

Change https://golang.org/cl/314431 mentions this issue: cmd/compile: regabi support for DWARF location expressions

Loading

@gopherbot
Copy link

@gopherbot gopherbot commented Apr 30, 2021

Change https://golang.org/cl/314930 mentions this issue: cmd/compile: revise block/func end sentinels in debug analysis

Loading

@gopherbot
Copy link

@gopherbot gopherbot commented Apr 30, 2021

Change https://golang.org/cl/315071 mentions this issue: cmd/compile: handle field padding for register-passed structs

Loading

@gopherbot
Copy link

@gopherbot gopherbot commented Apr 30, 2021

Change https://golang.org/cl/315610 mentions this issue: cmd/compile: fix abbrev selection for output params

Loading

gopherbot pushed a commit that referenced this issue Apr 30, 2021
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>
gopherbot pushed a commit that referenced this issue Apr 30, 2021
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>
gopherbot pushed a commit that referenced this issue Apr 30, 2021
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>
gopherbot pushed a commit that referenced this issue Apr 30, 2021
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>
@thanm
Copy link
Contributor

@thanm thanm commented May 1, 2021

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

Loading

@aarzilli
Copy link
Contributor Author

@aarzilli aarzilli commented May 1, 2021

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

sort.Stable(varsByScopeAndOffset{dwarfVars, varScopes})

this isn't always the case:

  1. return arguments are always listed before all input arguments
  2. if the compiler runs out of registers arguments passed on the stack will appear first

Loading

@thanm
Copy link
Contributor

@thanm thanm commented May 1, 2021

OK, thanks. This is not surprising given the various changes in frame offset for registerized outputs, etc. I'll look into sending a patch.

Loading

@aarzilli
Copy link
Contributor Author

@aarzilli aarzilli commented May 1, 2021

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

Loading

@gopherbot
Copy link

@gopherbot gopherbot commented May 1, 2021

Change https://golang.org/cl/315280 mentions this issue: cmd/compile: preserve argument order

Loading

@gopherbot gopherbot closed this in 169155d May 3, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
4 participants