-
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
runtime: missing deferreturn on linux/ppc64le #39049
Comments
@4a6f656c thanks for the repro case! In trying to reproduce this on a linux-ppc64le-buildlet gomote (after pushing the go source tree, building it with make.sh, and sshing in via 'gomote ssh', I got this error: ~# /workdir/go/bin/go run gen.go _/root/crash/huge1crash/huge1/a.s:1: expected '(', found C2 _/root/crash/huge2crash/huge2/a.s:1: expected '(', found C2 Any suggestions on why the as files are not assembling properly? Do you think I'll be able to repro on a gomote buildlet? Thanks! |
@danscales - ugh, when I've copied and pasted into play.golang.org, the unicode dot (·) got replaced with
Correcting that should fix the problem. You should be able to repro on a gomote buildlet as long as it's got enough resources. (I tried attaching Edit: I've just updated the play.golang.org links to a version that should have this fixed. |
@4a6f656c Thanks for the fixed gen.go file! I wasn't able to reproduce the problem on Gomote type linux-ppc64le-buildlet. I get the correct output 'panic: blah' and no 'missing deferreturn' message. Do you think it would repro better on some other buildlet (maybe linux-ppc64le-power9osu -- I'll try that next)? Is there anything unusual about your ppc64le configuration? |
Oh, also, I tried both on current tip and on the Go 1.14.2 release, but didn't repro on either. |
@danscales - I don't think there is anything particularly unusual about these machines, but I'll take a closer look later today. You may want to bump `n' up to a larger value (say 8000), as it may be dependent on the system stack allocation. |
@danscales - I just tested on a clean machine and noticed that I'd left |
OK, thanks to the repro case from Joel, I was able to figure out that this was due to the use of trampolines on PPC64 for calling deferreturn for programs with very large text sizes. The current method for finding/marking the deferreturn stub in a function doesn't work with these trampolines. These trampolines currently are possibly used only for arm and ppc64. I will check out the best way to identify these trampolines for deferreturn. |
Trampolines are very simple functions and don't have deferreturn in them. I wonder why they are special. I'll take a look. Let me know if there is anything I could help. |
To clarify, the problem is that a call to deferreturn in a normal function (which has defers) is being turned into a trampoline, and therefore the code to recognize the deferreturn stub (for open-coded defers) based on a call to deferreturn is not working. The code for recognizing the deferreturn call is in pcln.go:computeDeferReturn(). We just need to decide if some extra pattern matching (for trampoline calls) is OK, or if we should do some more complicated passing of the relative position of the deferreturn call within the function from the compiler. |
Thanks, @danscales . I think I understand the issue now. I can try to write a CL, if that's helpful. |
Change https://golang.org/cl/234105 mentions this issue: |
@cherrymui Oh, that was quick -- thanks for writing a CL. I'll take a look! |
Is there any chance of this being backported to 1.14 ? Thanks! |
This is a critical bug for kubernetes project, can someone help us cherry-picking this to 1.14 release? @danscales @aclements @4a6f656c @thanm @danscales @jeremyfaller |
@gopherbot please open a backport to 1.14 |
Backport issue(s) opened: #39991 (for 1.14). Remember to create the cherry-pick CL(s) as soon as the patch is submitted to master, according to https://golang.org/wiki/MinorReleases. |
@ianlancetaylor Thanks for a quick response, may I know when 1.14.5 bits will be available? |
We normally do minor releases around the start of each month, which in this case due to the U.S. Independence Day holiday means the beginning of next week. But someone will need to backport the change. |
I'll do the backport. I'll note that this won't be a clean cherry-pick, as the fix here is applied to the new linker, while Go 1.14 still uses the old linker. The logic is easy to backport, though. |
Thanks, @cherrymui . Let me know if I can help in any way. |
Change https://golang.org/cl/240917 mentions this issue: |
Change https://golang.org/cl/241087 mentions this issue: |
This CL ports CL 234105 and CL 240621 to the old linker, which fix critical bugs (runtime crashes). Updates #39049. Updates #39927. Change-Id: I47afc84349119e320d2e60d64b7188a410835d2b Reviewed-on: https://go-review.googlesource.com/c/go/+/241087 Run-TryBot: Cherry Zhang <[email protected]> TryBot-Result: Gobot Gobot <[email protected]> Reviewed-by: Than McIntosh <[email protected]> Reviewed-by: Jeremy Faller <[email protected]>
This is a backport of CL 234105. This is not a clean cherry-pick, as CL 234105 is for the new linker, whereas we still use the old linker here. This CL backports the logic. The runtime needs to find the PC of the deferreturn call in a few places. So for functions that have defer, we record the PC of deferreturn call in its funcdata. For very large binaries, the deferreturn call could be made through a trampoline. The current code of finding deferreturn PC fails in this case. This CL handles the trampoline as well. Fixes #39991. Updates #39049. Change-Id: I929be54d6ae436f5294013793217dc2a35f080d4 Reviewed-on: https://go-review.googlesource.com/c/go/+/234105 Run-TryBot: Cherry Zhang <[email protected]> TryBot-Result: Gobot Gobot <[email protected]> Reviewed-by: Jeremy Faller <[email protected]> Reviewed-by: Than McIntosh <[email protected]> Reviewed-on: https://go-review.googlesource.com/c/go/+/240917 Run-TryBot: Dmitri Shuralyov <[email protected]> Reviewed-by: Austin Clements <[email protected]> Reviewed-by: Joel Sing <[email protected]>
I also run into the similar error with kubernetes kubelet tooling which I report upstream at kubernetes/kubelet#13 For my case the app runs correctly with 1.13.x and regressed with 1.14.x. The 1.14.5 does not address this issue unfortunately. Any guidance is greatly appreciated. cc @cherrymui |
@runlevel5 , Go 1.14.5 was a security release, so it did not include the fix for this. This should be fixed in the next non-security release (which is very likely to be 1.14.6 and I think should be out soon). |
@aclements thanks for clarification |
Finally 1.14.6 has resolved the issue :) |
What version of Go are you using (
go version
)?Does this issue reproduce with the latest release?
Yes, the same issue exists on tip.
This is a regression between Go 1.13 and Go 1.14, presumably either due to the introduction of open coded defers, or due to a bug that is now being triggered.
What operating system and processor architecture are you using (
go env
)?go env
OutputWhat did you do?
This was initially observed when trying to run tests in a large code base on linux/ppc64le. In order to reproduce the issue, a
panic()
anddefer()
needs to be run at a high PC - I've written a Go program (https://play.golang.org/p/CmKwSyteWhX) that produces a Go program that triggers this issue.Save https://play.golang.org/p/CmKwSyteWhX as
gen.go
then run:What did you expect to see?
What did you see instead?
Changing
n
from 4149 to 4148 ingen.go
will reduce the number of instructions prior to thedefer()
and results in the test succeeding.The text was updated successfully, but these errors were encountered: