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.

Sign up for GitHub

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

Jump to bottom

cmd/compile: nonsensical DWARF location lists produced for function argument #61700

Open
andreimatei opened this issue Aug 1, 2023 · 2 comments · May be fixed by #64944
Open

cmd/compile: nonsensical DWARF location lists produced for function argument #61700

andreimatei opened this issue Aug 1, 2023 · 2 comments · May be fixed by #64944
Assignees
@thanm
Labels
compiler/runtime Issues related to the Go compiler and/or runtime. NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Milestone
Backlog

Comments

@andreimatei
Copy link
Contributor

andreimatei commented Aug 1, 2023 •
edited

What version of Go are you using (go version)?

$ go version
go version go1.21rc3 linux/amd64

Does this issue reproduce with the latest release?

Yes.
It also reproduces with 1.20, and maybe older versions too.

What operating system and processor architecture are you using (go env)?

go env Output
$ go env
GO111MODULE=''
GOARCH='amd64'
GOBIN=''
GOCACHE='/home/andrei/.cache/go-build'
GOENV='/home/andrei/.config/go/env'
GOEXE=''
GOEXPERIMENT=''
GOFLAGS=''
GOHOSTARCH='amd64'
GOHOSTOS='linux'
GOINSECURE=''
GOMODCACHE='/home/andrei/go/pkg/mod'
GONOPROXY=''
GONOSUMDB=''
GOOS='linux'
GOPATH='/home/andrei/go'
GOPRIVATE=''
GOPROXY='https://proxy.golang.org,direct'
GOROOT='/home/andrei/sdk/go1.21rc3'
GOSUMDB='sum.golang.org'
GOTMPDIR=''
GOTOOLCHAIN='auto'
GOTOOLDIR='/home/andrei/sdk/go1.21rc3/pkg/tool/linux_amd64'
GOVCS=''
GOVERSION='go1.21rc3'
GCCGO='gccgo'
GOAMD64='v1'
AR='ar'
CC='gcc'
CXX='g++'
CGO_ENABLED='1'
GOMOD='/dev/null'
GOWORK=''
CGO_CFLAGS='-O2 -g'
CGO_CPPFLAGS=''
CGO_CXXFLAGS='-O2 -g'
CGO_FFLAGS='-O2 -g'
CGO_LDFLAGS='-O2 -g'
PKG_CONFIG='pkg-config'
GOGCCFLAGS='-fPIC -m64 -pthread -Wl,--no-gc-sections -fmessage-length=0 -ffile-prefix-map=/tmp/go-build1118470848=/tmp/go-build -gno-record-gcc-switches'

What did you do?

In the CockroachDB (optimized) binary, I very commonly see broken location lists that contain duplicate pieces - so, for example, a 128-bit interface is described as a 256-bit struct, where the two halves are repeated. Playing by hand with small repros, I could also produce a case where the pieces are not even repeated, making the debug information even more confusing / nonsensical - which leads me to believe that the bug is not simply a "mechanical" one. Here's an example that shows all these problems:
https://goplay.tools/snippet/UfS5J4cYGy5

Consider this function in the snippet:

28:func myfunc(ctx context.Context) {
29:	dummy()
30:	foo(ctx)
31:	if gx == 0 {
32:		ctx = makeCtx1()
33:		dummy()
34:	} else {
35:		ctx = makeCtx2()
36:	}
37:	foo(ctx)
38:}

The debug info for the ctx context.Context function argument is the following:

0x000021c9:     DW_TAG_formal_parameter
                  DW_AT_name    ("ctx")
                  DW_AT_variable_parameter      (0x00)
                  DW_AT_decl_line       (28)
                  DW_AT_type    (0x0000000000019374 "context.Context")
                  DW_AT_location        (0x000016ff: 
                     [0x0000000000463da0, 0x0000000000463dbd): DW_OP_reg0 RAX, DW_OP_piece 0x8, DW_OP_reg0 RAX, DW_OP_piece 0x8, DW_OP_reg3 RBX, DW_OP_piece 0x8, DW_OP_reg3 RBX, DW_OP_piece 0x8
                     [0x0000000000463dbd, 0x0000000000463de0): DW_OP_call_frame_cfa, DW_OP_piece 0x8, DW_OP_call_frame_cfa, DW_OP_piece 0x8, DW_OP_fbreg +8, DW_OP_piece 0x8, DW_OP_fbreg +8, DW_OP_piece 0x8
                     [0x0000000000463de0, 0x0000000000463de5): DW_OP_fbreg -32, DW_OP_piece 0x8, DW_OP_call_frame_cfa, DW_OP_piece 0x8, DW_OP_fbreg +8, DW_OP_piece 0x8, DW_OP_fbreg +8, DW_OP_piece 0x8
                     [0x0000000000463de5, 0x0000000000463dea): DW_OP_fbreg -32, DW_OP_piece 0x8, DW_OP_call_frame_cfa, DW_OP_piece 0x8, DW_OP_fbreg -24, DW_OP_piece 0x8, DW_OP_fbreg +8, DW_OP_piece 0x8
                     [0x0000000000463dea, 0x0000000000463df6): DW_OP_fbreg -32, DW_OP_piece 0x8, DW_OP_call_frame_cfa, DW_OP_piece 0x8, DW_OP_fbreg -24, DW_OP_piece 0x8, DW_OP_fbreg +8, DW_OP_piece 0x8
                     [0x0000000000463df6, 0x0000000000463e00): DW_OP_call_frame_cfa, DW_OP_piece 0x8, DW_OP_call_frame_cfa, DW_OP_piece 0x8, DW_OP_fbreg +8, DW_OP_piece 0x8, DW_OP_fbreg +8, DW_OP_piece 0x8
                     [0x0000000000463e00, 0x0000000000463e05): DW_OP_reg0 RAX, DW_OP_piece 0x8, DW_OP_call_frame_cfa, DW_OP_piece 0x8, DW_OP_reg3 RBX, DW_OP_piece 0x8, DW_OP_fbreg +8, DW_OP_piece 0x8
                     [0x0000000000463e05, 0x0000000000463e29): DW_OP_piece 0x8, DW_OP_call_frame_cfa, DW_OP_piece 0x8, DW_OP_piece 0x8, DW_OP_fbreg +8, DW_OP_piece 0x8)

Notice that all the location lists contain 4 8-byte pieces, whereas they should contain 2 8-byte pieces (context.Context is a 16-byte interface).
Some of these location lists contain duplicate information - e.g. the first one:

DW_OP_reg0 RAX, DW_OP_piece 0x8, DW_OP_reg0 RAX, DW_OP_piece 0x8, DW_OP_reg3 RBX, DW_OP_piece 0x8, DW_OP_reg3 RBX, DW_OP_piece 0x8

Notice that the RAX and RBX pieces are each repeated.

But then it gets even more funky. Notice, for example, the 3rd location list:

[0x0000000000463de0, 0x0000000000463de5): DW_OP_fbreg -32, DW_OP_piece 0x8, DW_OP_call_frame_cfa, DW_OP_piece 0x8, DW_OP_fbreg +8, DW_OP_piece 0x8, DW_OP_fbreg +8, DW_OP_piece 0x8

Here, the first piece is no longer repeated.

FWIW, what happens around the 463de0 PC location is the following:

; /home/andrei/src/github.com/andreimatei/stackviz/test_progs/ctx.go:32
  463dd6: e8 45 ff ff ff               	callq	0x463d20 <main.makeCtx1>
; /home/andrei/src/github.com/andreimatei/stackviz/test_progs/ctx.go:37
  463ddb: 48 89 44 24 10               	movq	%rax, 0x10(%rsp)
  463de0: 48 89 5c 24 18               	movq	%rbx, 0x18(%rsp)
; /home/andrei/src/github.com/andreimatei/stackviz/test_progs/ctx.go:33
  463de5: e8 76 ff ff ff               	callq	0x463d60 <main.dummy>

At location 463ddb, the first word of the ctx variable is written to rsp + 16, which corresponds to FBREG - 32. So that explains the update to the first piece. But the 2nd piece (which should not have existed in the first place) is left un-updated.

What did you expect to see?

I expected the location lists for the ctx variable to add up to 128 bits, not 256.

If someone is kindly willing to give me some clues and point me in the right direction, I might be inclined to work on some fix.

cc @dr2chase @randall77 @thanm
@gopherbot gopherbot added the compiler/runtime Issues related to the Go compiler and/or runtime. label Aug 1, 2023
@andreimatei andreimatei mentioned this issue Aug 1, 2023
Open
@mknyszek mknyszek added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Aug 2, 2023
@mknyszek mknyszek added this to the Backlog milestone Aug 2, 2023
@ajwerner
Copy link

ajwerner commented Jan 3, 2024

I think this has something to do with the canonicalization of slots (or lack thereof). When I dump the debug logging for the building of debug information for the above function I see this:

at v17: ctx[uintptr] now in AX                                                       
at v17: ctx[unsafe.Pointer] now in AX                                                
at v13: ctx+8[*uint8] now in BX       
at v13: ctx+8[unsafe.Pointer] now in BX                                              
at v8: ctx[uintptr] clobbered out of AX                                              
at v8: ctx[unsafe.Pointer] clobbered out of AX           
at v8: ctx+8[*uint8] clobbered out of BX
at v8: ctx+8[unsafe.Pointer] clobbered out of BX

...

location lists:
Add entry for ctx:      b2v26-b2v-30000 =       ctx[uintptr]@<nil> ctx[unsafe.Pointer]@@+0 ctx+8[*uint8]@<nil> ctx+8[unsafe.Pointer]@@+8

The code and elsewhere in the cache seems to think that there will only be one entry for a part of a variable.

go/src/cmd/compile/internal/ssa/debug.go

Lines 656 to 662 in b25f555

parts := state.varParts[n]
state.varSlots[varID] = parts
for _, slotID := range parts {
state.slotVars[slotID] = VarID(varID)
}
*state.partsByVarOffset.(*partsByVarOffset) = partsByVarOffset{parts, state.slots}
sort.Sort(state.partsByVarOffset)

I guess the question I have is whether indeed there should be just one Value/slot per piece of a variable or not. My sense is no, that these different values of different types can be independently live and that more logic is needed to merge these differently typed slots together when constructing the location lists. Maybe there's something else clever I'm missing, so any advice on how to proceed would be appreciated.

ajwerner added a commit to ajwerner/go that referenced this issue Jan 3, 2024
@ajwerner
cmd/compile/internal/ssa: fix loc lists bug
20276e8 
Before this change there was a bug which can occur when an SSA'd
variable or parameter has multiple different corresponding to the same
piece of variable. Before this change, the locations of each type of
piece would be concatenated, resulting in non-sensical location
information with more pieces than there ought to be for the data type.

This patch rectifies the situation by associating the slots
corresponding to the same piece and computing the location of that piece
by coalescing the locations of the corresponding slots.

Fixes golang#61700
ajwerner added a commit to ajwerner/go that referenced this issue Jan 3, 2024
@ajwerner
cmd/compile/internal/ssa: fix location lists bug
56d91bf 
Before this change there was a bug which can occur when an SSA'd
variable has multiple different values (slots) corresponding to the same
piece of variable. Before this change, the locations of each type of
a piece would be concatenated, resulting in non-sensical location
information with more pieces than there ought to be for the data type.

This patch rectifies the situation by associating the slots
corresponding to the same piece and computing the location of that piece
by coalescing the locations of the corresponding slots.

Fixes golang#61700
ajwerner added a commit to ajwerner/go that referenced this issue Jan 3, 2024
@ajwerner
cmd/compile/internal/ssa: fix location lists bug
f701257 
Before this change there was a bug which can occur when an SSA'd
variable has multiple different values (slots) corresponding to the same
piece of variable. Before this change, the locations of each type of
a piece would be concatenated, resulting in non-sensical location
information with more pieces than there ought to be for the data type.

This patch rectifies the situation by associating the slots
corresponding to the same piece and computing the location of that piece
by coalescing the locations of the corresponding slots.

Fixes golang#61700
@ajwerner ajwerner linked a pull request Jan 3, 2024 that will close this issue
Open
@gopherbot
Copy link

Change https://go.dev/cl/553735 mentions this issue: cmd/compile/internal/ssa: fix location lists bug

ajwerner added a commit to ajwerner/go that referenced this issue Jan 3, 2024
@ajwerner
cmd/compile/internal/ssa: fix location lists bug
bafe4cd 
Before this change there was a bug which can occur when an SSA'd
variable has multiple different values (slots) corresponding to the same
piece of variable. Before this change, the locations of each type of
a piece would be concatenated, resulting in non-sensical location
information with more pieces than there ought to be for the data type.

This patch rectifies the situation by associating the slots
corresponding to the same piece and computing the location of that piece
by coalescing the locations of the corresponding slots.

Additionally, this change tries to clean up some of the terminology by
eliminating use of the word "part" which was ambiguious with regards to
slots and pieces.

Fixes golang#61700
ajwerner added a commit to ajwerner/go that referenced this issue Jan 3, 2024
@ajwerner
cmd/compile/internal/ssa: fix location lists bug
e8eb998 
Before this change there was a bug which can occur when an SSA'd
variable has multiple different values (slots) corresponding to the same
piece of variable. Before this change, the locations of each type of
a piece would be concatenated, resulting in non-sensical location
information with more pieces than there ought to be for the data type.

This patch rectifies the situation by associating the slots
corresponding to the same piece and computing the location of that piece
by coalescing the locations of the corresponding slots.

Additionally, this change tries to clean up some of the terminology by
eliminating use of the word "part" which was ambiguious with regards to
slots and pieces.

Fixes golang#61700
ajwerner added a commit to ajwerner/go that referenced this issue Jan 3, 2024
@ajwerner
cmd/compile/internal/ssa: fix location lists bug
ab3dfbb 
Before this change there was a bug which can occur when an SSA'd
variable has multiple different values (slots) corresponding to the same
piece of variable. Before this change, the locations of each type of
a piece would be concatenated, resulting in nonsensical location
information with more pieces than there ought to be for the data type.

This patch rectifies the situation by associating the slots
corresponding to the same piece and computing the location of that piece
by coalescing the locations of the corresponding slots.

Additionally, this change tries to clean up some of the terminology by
eliminating use of the word "part" which was ambiguious with regards to
slots and pieces.

Fixes golang#61700
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@thanm thanm

Labels
compiler/runtime Issues related to the Go compiler and/or runtime. NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Projects
Go Compiler / Runtime
Status: Todo
Milestone
Backlog
Development

Successfully merging a pull request may close this issue.

cmd/compile/internal/ssa: fix location lists bug ajwerner/go
5 participants
@andreimatei @mknyszek @ajwerner @gopherbot @thanm