cmd/compile: bad inlining tree emitted for function literal #46234
Comments
|
I think the problem here is that the function literal |
|
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. |
|
Change https://golang.org/cl/320913 mentions this issue: |
|
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.) |
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. |
|
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. |
|
Change https://golang.org/cl/366494 mentions this issue: |
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>
|
At tip with Is this an issue, or could it be one? The original bug report mentions this "inlining tree" output being wrong, and it's not present with For #54593, it's been mentioned that pprof sampling seems to correlate with the kubernetes apiserver hanging and getting restarted. I'm wondering if it's hitting a similar issue with infinite stack traces somewhere. |
|
Agree that if it's not being referenced by the instructions, it seems hard to imagine that it would be triggering the original bug. But it does seem a bit odd. |
|
Yeah, the presence of the inline tree looks suspicious. What do you mean by "it's not being referenced by the instructions"? Thanks. |
|
It shows up in the dump. For example in this code: the calls to WEA() get inlined. In the dump you can see this in the second column: Note the "0" for the instruction at 00059. At least that's how I'm reading it; hopefully I am not off base. |
In the instruction stream, the second column is always "-1" or blank. (Contrast @thanm's example of a different function where the entry is 0 at one point.) However, I have been able to construct alternative cases where the instructions for an inlined function literal do reference where the enclosing function was inlined to (i.e., the original bug here). I haven't successfully gotten any of them to crash with pprof yet though. |
|
Okay, thanks. You're right that that one is not referenced. It is odd that the unreferenced inline tree appears. Maybe something along the line of the CL above would be necessary for unified IR. |
|
I have this program: https://go.dev/play/p/OikFLBmKguY?v=gotip Which produces this And that output looks like it should be "wrong". Specifically, the inlining tree entry is actually used for loading the closure variable values. And the But pprof doesn't seem to be getting stuck there. |
|
Okay, manually instrumenting the runtime at Line 362 in 7f632f7 |
gopherbot
added
the
compiler/runtime
|
Actually, I'm going to file a new issue instead of reopening this one. I don't want to cause any weird issues for milestones. |
|
Yeah, that indeed looks like a cycle. Profiling collects at most 32 frames, so I guess the program doesn't fall into infinite loop, but the profile would looks like a bunch of recursive calls. |
|
@cherrymui I was able to confirm that the loop does go infinite. We only check the |
|
Oops... Good point. Thanks. |
|
Change https://go.dev/cl/425395 mentions this issue: |
When inlining function calls, we rewrite the position information on all of the nodes to keep track of the inlining context. This is necessary so that at runtime, we can synthesize additional stack frames so that the inlining is transparent to the user. However, for function literals, we *don't* want to apply this rewriting to the underlying function. Because within the function literal (when it's not itself inlined), the inlining context (if any) will have already be available at the caller PC instead. Unified IR was already getting this right in the case of user-written statements within the function literal, which is what the unit test for #46234 tested. However, it was still using inline-adjusted positions for the function declaration and its parameters, which occasionally end up getting used for generated code (e.g., loading captured values from the closure record). I've manually verified that this fixes the hang in https://go.dev/play/p/avQ0qgRzOgt, and spot-checked the -d=pctab=pctoinline output for kube-apiserver and kubelet and they seem better. However, I'm still working on a more robust test for this (hence "Updates" not "Fixes") and internal assertions to verify that we're emitting correct inline trees. In particular, there are still other cases (even in the non-unified frontend) where we're producing corrupt (but at least acyclic) inline trees. Updates #54625. Change-Id: Iacfd2e1eb06ae8dc299c0679f377461d3d46c15a Reviewed-on: https://go-review.googlesource.com/c/go/+/425395 Run-TryBot: Matthew Dempsky <mdempsky@google.com> Auto-Submit: Matthew Dempsky <mdempsky@google.com> Reviewed-by: Cuong Manh Le <cuong.manhle.vn@gmail.com> TryBot-Result: Gopher Robot <gobot@golang.org> Reviewed-by: David Chase <drchase@google.com>
When inlining function calls, we rewrite the position information on all of the nodes to keep track of the inlining context. This is necessary so that at runtime, we can synthesize additional stack frames so that the inlining is transparent to the user. However, for function literals, we *don't* want to apply this rewriting to the underlying function. Because within the function literal (when it's not itself inlined), the inlining context (if any) will have already be available at the caller PC instead. Unified IR was already getting this right in the case of user-written statements within the function literal, which is what the unit test for golang#46234 tested. However, it was still using inline-adjusted positions for the function declaration and its parameters, which occasionally end up getting used for generated code (e.g., loading captured values from the closure record). I've manually verified that this fixes the hang in https://go.dev/play/p/avQ0qgRzOgt, and spot-checked the -d=pctab=pctoinline output for kube-apiserver and kubelet and they seem better. However, I'm still working on a more robust test for this (hence "Updates" not "Fixes") and internal assertions to verify that we're emitting correct inline trees. In particular, there are still other cases (even in the non-unified frontend) where we're producing corrupt (but at least acyclic) inline trees. Updates golang#54625. Change-Id: Iacfd2e1eb06ae8dc299c0679f377461d3d46c15a Reviewed-on: https://go-review.googlesource.com/c/go/+/425395 Run-TryBot: Matthew Dempsky <mdempsky@google.com> Auto-Submit: Matthew Dempsky <mdempsky@google.com> Reviewed-by: Cuong Manh Le <cuong.manhle.vn@gmail.com> TryBot-Result: Gopher Robot <gobot@golang.org> Reviewed-by: David Chase <drchase@google.com>
What version of Go are you using (
go version)?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.
What did you see instead?
Program gets an infinite loop in traceback:
The problem sees to be in the runtime's traceback handling of inlining -- at the point of the fault we are at
The inlining tree for this function (from go build -gcflags=-d=pctab=pctoinline) looks like
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.
The text was updated successfully, but these errors were encountered: