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: stack load line numbers wrong in non-regabi backend #60673

Closed
rsc opened this issue Jun 8, 2023 · 1 comment
Closed

cmd/compile: stack load line numbers wrong in non-regabi backend #60673

rsc opened this issue Jun 8, 2023 · 1 comment
Labels
compiler/runtime Issues related to the Go compiler and/or runtime. NeedsFix The path to resolution is known, but the work has not been done.
Milestone
Go1.22

Comments

@rsc
Copy link
Contributor

rsc commented Jun 8, 2023

Here's a little program:

$ cat /tmp/x.go
package main

type T struct { x int }

func do(t *T) {do(t) }

func alloc() *T { return &T{42} }

func f() {
	x := alloc()
	do(x)
	do(x)
}

GOARCH=amd64 go tool compile -S on amd64 produces:

main.f STEXT size=71 args=0x0 locals=0x18 funcid=0x0 align=0x0
	0x0000 00000 (/tmp/x.go:9)	TEXT	main.f(SB), ABIInternal, $24-0
	0x0000 00000 (/tmp/x.go:9)	CMPQ	SP, 16(R14)
	0x0004 00004 (/tmp/x.go:9)	PCDATA	$0, $-2
	0x0004 00004 (/tmp/x.go:9)	JLS	59
	0x0006 00006 (/tmp/x.go:9)	PCDATA	$0, $-1
	0x0006 00006 (/tmp/x.go:9)	PUSHQ	BP
	0x0007 00007 (/tmp/x.go:9)	MOVQ	SP, BP
	0x000a 00010 (/tmp/x.go:9)	SUBQ	$16, SP
	0x000e 00014 (/tmp/x.go:9)	FUNCDATA	$0, gclocals·g2BeySu+wFnoycgXfElmcg==(SB)
	0x000e 00014 (/tmp/x.go:9)	FUNCDATA	$1, gclocals·g2BeySu+wFnoycgXfElmcg==(SB)
	0x000e 00014 (/tmp/x.go:10)	XCHGL	AX, AX
	0x000f 00015 (/tmp/x.go:7)	MOVQ	$0, main..autotmp_3+8(SP)
	0x0018 00024 (/tmp/x.go:7)	MOVQ	$42, main..autotmp_3+8(SP)
	0x0021 00033 (/tmp/x.go:11)	LEAQ	main..autotmp_3+8(SP), AX
	0x0026 00038 (/tmp/x.go:11)	PCDATA	$1, $0
	0x0026 00038 (/tmp/x.go:11)	CALL	main.do(SB)
	0x002b 00043 (/tmp/x.go:12)	LEAQ	main..autotmp_3+8(SP), AX
	0x0030 00048 (/tmp/x.go:12)	CALL	main.do(SB)
	0x0035 00053 (/tmp/x.go:13)	ADDQ	$16, SP
	0x0039 00057 (/tmp/x.go:13)	POPQ	BP
	0x003a 00058 (/tmp/x.go:13)	RET
	0x003b 00059 (/tmp/x.go:13)	NOP
	0x003b 00059 (/tmp/x.go:9)	PCDATA	$1, $-1
	0x003b 00059 (/tmp/x.go:9)	PCDATA	$0, $-2
	0x003b 00059 (/tmp/x.go:9)	NOP
	0x0040 00064 (/tmp/x.go:9)	CALL	runtime.morestack_noctxt(SB)
	0x0045 00069 (/tmp/x.go:9)	PCDATA	$0, $-1
	0x0045 00069 (/tmp/x.go:9)	JMP	0

Note the inlined call to alloc at 0x000f (line 7) with the inlining mark (XCHGL AX, AX) above it.
No other inlining here.

Note in particular that between the two CALL main.do at 0x002b there is a reload of x from main..autotmp_3 and it is attributed to line 12, because it is the setup for the next call on line 12.

GOARCH=386 go tool compile -S produces:

main.f STEXT size=73 args=0x0 locals=0x8 funcid=0x0 align=0x0
	0x0000 00000 (/tmp/x.go:9)	TEXT	main.f(SB), NOFRAME|ABIInternal, $8-0
	0x0000 00000 (/tmp/x.go:9)	MOVL	TLS, CX
	0x0007 00007 (/tmp/x.go:9)	PCDATA	$0, $-2
	0x0007 00007 (/tmp/x.go:9)	MOVL	(CX)(TLS*2), CX
	0x000d 00013 (/tmp/x.go:9)	PCDATA	$0, $-1
	0x000d 00013 (/tmp/x.go:9)	CMPL	SP, 8(CX)
	0x0010 00016 (/tmp/x.go:9)	PCDATA	$0, $-2
	0x0010 00016 (/tmp/x.go:9)	JLS	66
	0x0012 00018 (/tmp/x.go:9)	PCDATA	$0, $-1
	0x0012 00018 (/tmp/x.go:9)	SUBL	$8, SP
	0x0015 00021 (/tmp/x.go:9)	FUNCDATA	$0, gclocals·g2BeySu+wFnoycgXfElmcg==(SB)
	0x0015 00021 (/tmp/x.go:9)	FUNCDATA	$1, gclocals·g2BeySu+wFnoycgXfElmcg==(SB)
	0x0015 00021 (/tmp/x.go:10)	XCHGL	AX, AX
	0x0016 00022 (/tmp/x.go:7)	MOVL	$0, main..autotmp_3+4(SP)
	0x001e 00030 (/tmp/x.go:7)	MOVL	$42, main..autotmp_3+4(SP)
	0x0026 00038 (/tmp/x.go:7)	LEAL	main..autotmp_3+4(SP), AX
	0x002a 00042 (/tmp/x.go:7)	MOVL	AX, (SP)
	0x002d 00045 (/tmp/x.go:11)	PCDATA	$1, $0
	0x002d 00045 (/tmp/x.go:11)	CALL	main.do(SB)
	0x0032 00050 (/tmp/x.go:7)	LEAL	main..autotmp_3+4(SP), AX
	0x0036 00054 (/tmp/x.go:7)	MOVL	AX, (SP)
	0x0039 00057 (/tmp/x.go:12)	CALL	main.do(SB)
	0x003e 00062 (/tmp/x.go:13)	ADDL	$8, SP
	0x0041 00065 (/tmp/x.go:13)	RET
	0x0042 00066 (/tmp/x.go:13)	NOP
	0x0042 00066 (/tmp/x.go:9)	PCDATA	$1, $-1
	0x0042 00066 (/tmp/x.go:9)	PCDATA	$0, $-2
	0x0042 00066 (/tmp/x.go:9)	CALL	runtime.morestack_noctxt(SB)
	0x0047 00071 (/tmp/x.go:9)	PCDATA	$0, $-1
	0x0047 00071 (/tmp/x.go:9)	JMP	0

Note the same inlining at 0x0016. But then note the LEAL/MOVL reloading x at 0x0032. It is attributed to line 7, the implementation of alloc, instead of to the setup for the next call. It says x.go:7 instead of x.go:12 like the GOARCH=amd64 one.

This inconsistency is part of a problem I'm seeing with runtime.CallersFrames applied to the result of runtime.Callers.

It's not the most direct problem, though: using runtime.CallersFrames on runtime.Callers should not care about the line number of that instruction at all!
@rsc rsc added the NeedsFix The path to resolution is known, but the work has not been done. label Jun 8, 2023
@rsc rsc added this to the Go1.22 milestone Jun 8, 2023
@gopherbot gopherbot added the compiler/runtime Issues related to the Go compiler and/or runtime. label Jun 8, 2023
gopherbot pushed a commit to golang/telemetry that referenced this issue Jun 12, 2023
@pjweinbgo
x/telemetry/counter: fix stack computation
d63a359 
The line number computation in stack counters was wrong
(thank you Russ), and has been changed. There is no longer
a special case for 386s.

Part of the issue for 386s is
golang/go#60673
but it is not relevant for this.

Change-Id: Iccc9a43ac05d94df2aa7915bfc3d4d74e4d2ff6a
Reviewed-on: https://go-review.googlesource.com/c/telemetry/+/501596
Run-TryBot: Peter Weinberger <pjw@google.com>
Reviewed-by: Jamal Carvalho <jamal@golang.org>
TryBot-Result: Gopher Robot <gobot@golang.org>
@gopherbot
Copy link

Change https://go.dev/cl/502656 mentions this issue: cmd/compile: use callsite as line number for argument marshaling

@randall77 randall77 changed the title cmd/compile: stack load line numbers wrong in non-ssa backend cmd/compile: stack load line numbers wrong in non-regabi backend Jun 12, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
compiler/runtime Issues related to the Go compiler and/or runtime. NeedsFix The path to resolution is known, but the work has not been done.
Projects
None yet
Milestone
Go1.22
Development

No branches or pull requests
2 participants
@rsc @gopherbot