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: bad inlining tree emitted for function literal #46234

Closed
thanm opened this issue May 18, 2021 · 7 comments
Closed

cmd/compile: bad inlining tree emitted for function literal #46234

thanm opened this issue May 18, 2021 · 7 comments
Assignees
Milestone

Comments

@thanm
Copy link
Contributor

@thanm thanm commented May 18, 2021

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

$ go version
go version devel go1.17-8b0901fd32 Tue May 18 07:50:25 2021 +0000 linux/amd64

Does this issue reproduce with the latest release?

Only on 1.17 / tip -- this problem is not present in 1.16.

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

linux/amd64

What did you do?

Run this program: https://play.golang.org/p/VZWvwkvYUGx

What did you expect to see?

In 1.16 you get a crash on a nil pointer, e.g.

$ go run repro.go
begin main
panic: runtime error: invalid memory address or nil pointer dereference
[signal SIGSEGV: segmentation violation code=0x1 addr=0x0 pc=0x4a2245]

goroutine 1 [running]:
main.WEA.func1(0x0)
	/tmp/repro.go:17 +0x5
main.(*CM).NewA(0xc0000eef40, 0x0, 0x0, 0x4c761f, 0x3, 0x0, 0xc0000eeed0, 0x2, 0x2, 0xb, ...)
	/tmp/repro.go:30 +0x42
main.(*R).CA(0xc0000eef48, 0x0, 0x0, 0x4c761f, 0x3, 0x0, 0x0, 0xb, 0x0)
	/tmp/repro.go:37 +0xd1
main.main()
	/tmp/repro.go:52 +0xc5
exit status 2
thanm@cetina:/tmp$ 

What did you see instead?

Program gets an infinite loop in traceback:

begin main
panic: runtime error: invalid memory address or nil pointer dereference
[signal SIGSEGV: segmentation violation code=0x1 addr=0x0 pc=0x487f00]

goroutine 1 [running]:
main.WEA(...)
        /tmp/repro.go:17
main.WEA(...)
        /tmp/repro.go:17
main.WEA(...)
        /tmp/repro.go:17
main.WEA(...)
        /tmp/repro.go:17
<repeats>

The problem sees to be in the runtime's traceback handling of inlining -- at the point of the fault we are at

Dump of assembler code for function main.(*R).CA.func1:
=> 0x0000000000487f00 <+0>:	movb   $0x1,(%rax)
   0x0000000000487f03 <+3>:	retq   

The inlining tree for this function (from go build -gcflags=-d=pctab=pctoinline) looks like

-- inlining tree for "".(*R).CA.func1:
0 | -1 | "".WEA (/tmp/repro.go:37:39) pc=0

The problem here is that the traceback code is trying to "back up" an instruction when landing on 0x0000000000487f00 to recover the parent PC (runtime code here), but this winds up hitting the same instruction, hence we never get out of the loop.

I'm tentatively marking this as a compiler bug, can be retitled if it turns out there are other components involved.

@thanm thanm added this to the Go1.17 milestone May 18, 2021
@thanm thanm self-assigned this May 18, 2021
@cherrymui
Copy link
Contributor

@cherrymui cherrymui commented May 18, 2021

I think the problem here is that the function literal main.(*R).CA.func1 itself is not actually an inlined function (its containing function main.WEA is). So it should not have an inlining tree at all.

Loading

@thanm
Copy link
Contributor Author

@thanm thanm commented May 18, 2021

Agree-- we don't want an inl tree for main.(*R).CA.func1. Now working on trying to understand how it is we get the inltree.

Loading

@gopherbot
Copy link

@gopherbot gopherbot commented May 18, 2021

Change https://golang.org/cl/320913 mentions this issue: cmd/compile: don't emit inltree for closure within body of inlined func

Loading

@mdempsky
Copy link
Member

@mdempsky mdempsky commented May 18, 2021

When we clone a function literal because of inlining, do we have a way for users/debuggers to distinguish stepping through a cloned function literal vs the original? It seems like we should somehow maintain that information, along with position details about where it was cloned into.

(Of course, for release purposes, I think it's more critical to have traceback working reliably than to have perfect debugging info.)

Loading

@gopherbot gopherbot closed this in 6d2ef2e May 18, 2021
@thanm
Copy link
Contributor Author

@thanm thanm commented May 18, 2021

When we clone a function literal because of inlining, do we have a way for users/debuggers to distinguish stepping through a cloned function literal vs the original? It seems like we should somehow maintain that information, along with position details about where it was cloned into.

This is an interesting question. I guess my question would be "does the user care"? If the line table info and variable locations are correct (in the sense that they show the expected line numbers, and the debugger can print the right values), then I wonder whether it matters that you're in a clone or not.

Loading

@mdempsky
Copy link
Member

@mdempsky mdempsky commented May 18, 2021

That's fair. I think most users shouldn't need to worry about this. But I'm wondering about power users (particularly us) trying to diagnose suspected miscompilation that affects inlined function literals that were optimized differently than the original.

What about the position information for newfn and newfn.Nname? Could we update their positions at least to maintain inlining stack info? Can we use inlined positions for PEXTERN/PFUNC declarations, or are they limited to PAUTO?

I think if we could at least do that, it would address my theoretical power user concern.

Loading

@gopherbot
Copy link

@gopherbot gopherbot commented Nov 23, 2021

Change https://golang.org/cl/366494 mentions this issue: cmd/compile/internal/inline: revise closure inl position fix

Loading

gopherbot pushed a commit that referenced this issue Nov 24, 2021
This patch revises the fix for issue 46234, fixing a bug that was
accidentally introduced by CL 320913. When inlining a chunk of code
with a closure expression, we want to avoid updating the source
positions in the function being closed over, but we do want to update
the position for the ClosureExpr itself (since it is part of the
function we are inlining). CL 320913 unintentionally did away with the
closure expr source position update; here we restore it again.

Updates #46234.
Fixes #49171.

Change-Id: Iaa51bc498e374b9e5a46fa0acd7db520edbbbfca
Reviewed-on: https://go-review.googlesource.com/c/go/+/366494
Trust: Than McIntosh <thanm@google.com>
Trust: Dan Scales <danscales@google.com>
Reviewed-by: Dan Scales <danscales@google.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
4 participants