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: incorrect DWARF location for runtime.throw's in-param #46425

Closed
thanm opened this issue May 27, 2021 · 14 comments
Closed

cmd/compile: incorrect DWARF location for runtime.throw's in-param #46425

thanm opened this issue May 27, 2021 · 14 comments
Labels
NeedsInvestigation
Milestone

Comments

@thanm
Copy link
Contributor

@thanm thanm commented May 27, 2021

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

$ go version
go version devel go1.17-15a374d5c1 Wed May 19 01:09:20 2021 +0000 linux/amd64

Does this issue reproduce with the latest release?

Yes. This is only on tip.

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

linux/amd64

What did you do?

Compile a small program like

https://play.golang.org/p/1wRyGFu3wca

then run it in the debugger. Set a breakpoint in runtime.throw, then run. print "s"

What did you expect to see?

(gdb) p s
$1 = 0x47bfb6 "all goroutines are asleep - deadlock!"

What did you see instead?

(gdb) p s
$2 = 0x0 <error: Cannot access memory at address 0x0>

This is a bug in the Go compiler's debug location analysis phase, being triggered by the register ABI. Things seem to work when the register ABI is disabled, but I think mainly because the incoming locations for "s" are never killed, e.g. the stack location is essentially always available. When "s" is passed in RAX/RAB, it correctly tracks this fact, but then doesn't record the correct entry location when those registers are overwritten. I am not entirely sure what is going wrong, more analysis needed.

@thanm thanm added the NeedsInvestigation label May 27, 2021
@thanm thanm added this to the Go1.17 milestone May 27, 2021
@thanm thanm self-assigned this May 27, 2021
@hyangah
Copy link
Contributor

@hyangah hyangah commented May 28, 2021

@cherrymui @suzmue reported this problem in go1.16 too (I am not sure if it's the same problem)

@thanm
Copy link
Contributor Author

@thanm thanm commented May 28, 2021

@cherrymui @suzmue reported this problem in go1.16 too (I am not sure if it's the same problem)

I looked at the DWARF produced by the 1.16 compiler. It is indeed broken, but it looks like in a different way (two different bugs).

derekparker pushed a commit to go-delve/delve that referenced this issue Jun 28, 2021
We can get the throw reason by looking at the argument "s" in runtime.throw. This is not currently working in Go 1.16 or Go 1.17 (see golang/go#46425), but does work in Go 1.15 and Go 1.14
@gopherbot
Copy link

@gopherbot gopherbot commented Jul 1, 2021

Change https://golang.org/cl/332269 mentions this issue: cmd/compile: fix bug in dwarf-gen var location generation

@suzmue
Copy link
Contributor

@suzmue suzmue commented Jul 22, 2021

With Go 1.17rc1, I am seeing that the location is correct within the systemstack call, but it is optimized out in the body of runtime.throw. Delve sets a breakpoint at runtime.fatalthrow for users to inspect the program state, so this is the point that it would be helpful to have access to the value of s.

suzmue@cloudshell:~/delve$ gdb fatalerror 
...
(gdb) b runtime.throw
Breakpoint 1 at 0x433820: file /usr/local/go/src/runtime/panic.go, line 1107.
(gdb) b runtime.fatalthrow
Breakpoint 2 at 0x433a20: file /usr/local/go/src/runtime/panic.go, line 1163.
(gdb) b /usr/local/go/src/runtime/panic.go:1111
Breakpoint 3 at 0x460eaf: file /usr/local/go/src/runtime/panic.go, line 1111.
(gdb) run
Starting program: /home/suzmue/delve/fatalerror 
...
Thread 1 "fatalerror" hit Breakpoint 1, runtime.throw (s=...) at /usr/local/go/src/runtime/panic.go:1107
1107    func throw(s string) {
(gdb) p s
$1 = <optimized out>
(gdb) c
Continuing.

Thread 1 "fatalerror" hit Breakpoint 3, runtime.throw.func1 () at /usr/local/go/src/runtime/panic.go:1111
1111                    print("fatal error: ", s, "\n")
(gdb) p s
$2 = 0x480a79 "go of nil func value"
(gdb) c
Continuing.
fatal error: go of nil func value

Thread 1 "fatalerror" hit Breakpoint 2, runtime.fatalthrow () at /usr/local/go/src/runtime/panic.go:1163
1163    func fatalthrow() {
(gdb) frame 1
#1  0x0000000000433892 in runtime.throw (s=...) at /usr/local/go/src/runtime/panic.go:1117
1117            fatalthrow()
(gdb) p s
$3 = <optimized out>

If this would be more appropriate as a new issue to open, since it seems that the incorrect location seems to be fixed I can do so.

@suzmue suzmue reopened this Jul 22, 2021
@cherrymui
Copy link
Member

@cherrymui cherrymui commented Jul 22, 2021

Interesting. At the point of fatalthow call s is no longer live. Maybe we could keep it live in throw.

Why does it set a breakpoint in fatalthrow instead of throw?

@suzmue
Copy link
Contributor

@suzmue suzmue commented Jul 22, 2021

I think its historical from when fatalpanic and fatalthrow split, but I'm not sure @derekparker @aarzilli would know better.

In both Go 1.16 and Go 1.17 the location info appears to only be available in the print function in runtime.throw:

systemstack(func() {
	print("fatal error: ", s, "\n")
})

It seems that s should also be live at the start of runtime.throw since it is not used print("fatal error: ", s, "\n"). Would it be possible to get the location information available then at least?

@cherrymui
Copy link
Member

@cherrymui cherrymui commented Jul 22, 2021

It seems that s should also be live at the start of runtime.throw

With @thanm 's fix I believe it should work for 1.17. I just tried at tip and it does seem to work

(gdb) b runtime.throw
Breakpoint 1 at 0x42b7e0: file /usr/local/google/home/cherryyz/src/go/src/runtime/panic.go, line 1188.
(gdb) run
Starting program: /tmp/d 
[New LWP 267089]
[New LWP 267090]
[New LWP 267091]
[New LWP 267092]
[Switching to LWP 267092]

Thread 5 "d" hit Breakpoint 1, runtime.throw (s="all goroutines are asleep - deadlock!") at /usr/local/google/home/cherryyz/src/go/src/runtime/panic.go:1188
1188	func throw(s string) {
(gdb) p s
$1 = "all goroutines are asleep - deadlock!"

I know it doesn't work with Go 1.16 due to some bug. @thanm would it be possible to fix 1.16 with something simple?

@suzmue
Copy link
Contributor

@suzmue suzmue commented Jul 22, 2021

Thanks it does seem to be working at runtime.throw. Sorry about that those logs must have been using a binary built with 1.16.

@derekparker @aarzilli What do you think about moving the fatal throw breakpoint to runtime.throw instead of runtime.fatalthrow? This seems like it would be a more reliable place to get the information about the throw message.

@thanm
Copy link
Contributor Author

@thanm thanm commented Jul 22, 2021

Echoing what Cherry said, the crux of the problem is that "s" is no longer live at the call to fatalthrow; in general it isn't possible to generate a good location unless the lifetime is extended somehow.

The location at the start of runtime.throw should be accurate now in 1.17. There are still many other issues with debug locations, but that one at least is working.

@thanm
Copy link
Contributor Author

@thanm thanm commented Jul 22, 2021

accurate now in 1.16
Sorry, I meant 1.17 (I edited the comment)

@derekparker
Copy link
Contributor

@derekparker derekparker commented Jul 22, 2021

@derekparker @aarzilli What do you think about moving the fatal throw breakpoint to runtime.throw instead of runtime.fatalthrow? This seems like it would be a more reliable place to get the information about the throw message.

I think that should be ok, but I haven't dug into the ramifications of the change. I'll take a closer look tomorrow, but it does seem like a more reliable spot.

On initial digging it seems nothing directly calls runtime.fatalthrow that I can see, so we wouldn't miss anything by setting the breakpoint on runtime.throw instead.

@thanm
Copy link
Contributor Author

@thanm thanm commented Aug 9, 2021

@suzmue ok if I close this issue out?

@suzmue
Copy link
Contributor

@suzmue suzmue commented Aug 9, 2021

@thanm Yes definitely! Thanks for your help with this issue.

Delve has also switched to using runtime.throw in PR go-delve/delve#2616, so this information will be surfaced to users.

@thanm
Copy link
Contributor Author

@thanm thanm commented Aug 9, 2021

Cool, thanks.

@thanm thanm closed this as completed Aug 9, 2021
@rsc rsc unassigned thanm Jun 23, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
NeedsInvestigation
Projects
None yet
Development

No branches or pull requests

6 participants