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

Jump to bottom

cmd/compile: add line numbers for function exit paths? #31199

Open
josharian opened this issue Apr 2, 2019 · 5 comments
Open

cmd/compile: add line numbers for function exit paths? #31199

josharian opened this issue Apr 2, 2019 · 5 comments
Labels
compiler/runtime Issues related to the Go compiler and/or runtime. Debugging NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Milestone
Unplanned

Comments

@josharian
Copy link
Contributor

Noticed while looking at #31193.

func count(x uint) {
	if x != 0 {
		count(x - 1)
	}
}

compiles to:

"".count STEXT size=70 args=0x8 locals=0x10
	0x0000 00000 (count_test.go:8)	TEXT	"".count(SB), ABIInternal, $16-8
	0x0000 00000 (count_test.go:8)	MOVQ	(TLS), CX
	0x0009 00009 (count_test.go:8)	CMPQ	SP, 16(CX)
	0x000d 00013 (count_test.go:8)	JLS	63
	0x000f 00015 (count_test.go:8)	SUBQ	$16, SP
	0x0013 00019 (count_test.go:8)	MOVQ	BP, 8(SP)
	0x0018 00024 (count_test.go:8)	LEAQ	8(SP), BP
	0x001d 00029 (count_test.go:9)	MOVQ	"".x+24(SP), AX
	0x0022 00034 (count_test.go:9)	TESTQ	AX, AX
	0x0025 00037 (count_test.go:9)	JNE	49
	0x0027 00039 (<unknown line number>)	MOVQ	8(SP), BP
	0x002c 00044 (<unknown line number>)	ADDQ	$16, SP
	0x0030 00048 (<unknown line number>)	RET
	0x0031 00049 (count_test.go:10)	DECQ	AX
	0x0034 00052 (count_test.go:10)	MOVQ	AX, (SP)
	0x0038 00056 (count_test.go:10)	CALL	"".count(SB)
	0x003d 00061 (count_test.go:10)	JMP	39
	0x003f 00063 (count_test.go:8)	CALL	runtime.morestack_noctxt(SB)
	0x0044 00068 (count_test.go:8)	JMP	0

Note the <unknown line number> entries. It seems to me that those could be attributed to the closing brace of the function: It is code executed as the function exits.

I don't know whether adding line numbers here would be good or bad for debugging, or good or bad for binary size. I just thought I'd mention it in case @dr2chase sees an opportunity here. If not, we can close.
@josharian josharian added NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. Debugging labels Apr 2, 2019
@aarzilli
Copy link
Contributor

aarzilli commented Apr 2, 2019

In debug_line those are all assigned to the if x != 0 { line (which is fine in this case). I think that's reflecting pclntab, I don't know if it matters in this case.

@josharian
Copy link
Contributor Author

Ah, yes, the -S output reflects pclntab. Giving pclntab nearby line numbers here might shrink the binary a bit, since they will encode to be smaller, but that's perhaps not worth the effort.

I wonder whether it'd be useful to write a tool that compares pclntab file/line vs dwarf file/line for every instruction and flags mismatches, as a means of finding opportunities to improve.

@dr2chase
Copy link
Contributor

dr2chase commented Apr 2, 2019

Depends on what you mean by improve. The things we're trying to optimize are (roughly in order) debugging experience, profiling accuracy, and binary size. Changing to use the is_stmt flags was intended to get better debugging experience while accurately attributing anything that was not a statement for profiling purposes. But it increases binary size.

@josharian
Copy link
Contributor Author

So what you're saying is that there are intentional mismatches between pclntab and dwarf? Then, yes, a tool that compares them would not be helpful at all. :)

@dr2chase
Copy link
Contributor

dr2chase commented Apr 2, 2019

I actually think they're supposed to match, but I'm not sure it's required. pclntab lacks the is_stmt markers, is all.

@gopherbot gopherbot added the compiler/runtime Issues related to the Go compiler and/or runtime. label Jul 13, 2022
@seankhliao seankhliao added this to the Unplanned milestone Aug 20, 2022
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. Debugging NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Projects
Go Compiler / Runtime
Status: Triage Backlog
Milestone
Unplanned
Development

No branches or pull requests
5 participants
@josharian @aarzilli @dr2chase @gopherbot @seankhliao