-
Notifications
You must be signed in to change notification settings - Fork 17.7k
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: closure miscompilation in gopher-lua benchmark #59680
Comments
Revert: https://go.dev/cl/485498 |
…closures" This reverts commit f8162a0. Reason for revert: #59680 Change-Id: I91821c691a2d019ff0ad5b69509e32f3d56b8f67 Reviewed-on: https://go-review.googlesource.com/c/go/+/485498 Reviewed-by: Russ Cox <rsc@golang.org> TryBot-Result: Gopher Robot <gobot@golang.org> Run-TryBot: Michael Knyszek <mknyszek@google.com> Reviewed-by: Matthew Dempsky <mdempsky@google.com> Auto-Submit: Michael Knyszek <mknyszek@google.com>
I'll take a look tomorrow. |
One thing that isn't immediately clear -- we've already had two attempts for the original CL to enable more inlining of functions with closures (e.g. CL 479095. Did this particular problem not manifest the first time around? Or it was there but we just didn't pick up on it? It would be helpful to understand that get a sense for the scope of the situation. |
I spent some tim debugging this. The problem appears to relate to how the closure is being handled, or at least that's what it appears. The offending function (in which crash is taking place) is
which corresponds to a source function at line 48:
In the assembly above, register $rdx is the closure pointer; the code loads up the value of "b" and then uses that to pick out I stopped in the debugger and dumped the closure pointer:
The first word is ok (points to the correct function) but the value of "b" is garbage -- it's printing as 0xc00018ac98 instead of the correct value of 0xc0001a62c0... thus the actual fault comes at the point where the code loads up "b.pid". Unsure as to how/why the closure is going bad. Maybe an escape analysis thing? |
For reference, here's line 48, the closure that's crashing. I think you're right this is an escape analysis issue. At 94850c6 (just before the latest revert of this ill-fated CL), the function that constructs the closure looks like:
The closure itself is being heap allocated (+0x001a). But, if I'm reading this right, we're putting a stack address ( This only happens in the version of
Here we can see that we copied If we pull in the revert (ce10e9d), |
cc @mdempsky and @cherrymui since this involves escape analysis. |
Here is a single-file reproducer: https://go.dev/play/p/6IkaUaWvYwD My CL stack that rolls forward the closure inlining change: http://go.dev/cl/c/go/+/492017 With this test case and the CL stack above, the crashing stack trace in GDB is:
I did a build of the testcase with "gcflags=-m=3 -W=3 -S". What is surprising about the output there is that the offending function definitely turns up in walk/order and then the assembly output:
however it never seems to be touched by escape analysis. I added some debugging code here and from what I can tell it never actually gets visited, which might explain the problem. |
OK, I think I see the issue here. It's a pkginit problem. Before we run
but when I think what's needed here is something in pkginit that will detect this case and mark the closures as no longer hidden. I'll work on a CL for that. |
Change https://go.dev/cl/492135 mentions this issue: |
If the function referenced by a closure expression is incorporated into a static init, be sure to mark it as non-hidden, since otherwise it will be live but no longer reachable from the init func, hence it will be skipped during escape analysis, which can lead to miscompilations. Fixes #59680. Change-Id: Ib858aee296efcc0b7655d25c23ab8a6a8dbdc5f9 Reviewed-on: https://go-review.googlesource.com/c/go/+/492135 TryBot-Result: Gopher Robot <gobot@golang.org> Reviewed-by: Cherry Mui <cherryyz@google.com> Reviewed-by: Matthew Dempsky <mdempsky@google.com> Run-TryBot: Than McIntosh <thanm@google.com> Reviewed-by: Cuong Manh Le <cuong.manhle.vn@gmail.com>
CL 484860 broke the x/benchmarks builder: https://build.golang.org/?repo=golang.org%2fx%2fbenchmarks
Failure: https://build.golang.org/log/11803116ada45552b52ed62dd86f4255cdba5d87
What's happening is this call produces a closure that appears to be inlined into a compiler-generated init function. The closure it produces later gets installed here.
The end result is this function ends up dereferencing a nil pointer.
Reproducer instructions:
k-nucleotide.txt
input.txt
The text was updated successfully, but these errors were encountered: