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

Jump to bottom

runtime: pcdata is -2 and 12 locals stack map entries error on nil pointer [1.15 backport] #40742

Closed
randall77 opened this issue Aug 12, 2020 · 8 comments
Closed

runtime: pcdata is -2 and 12 locals stack map entries error on nil pointer [1.15 backport] #40742

randall77 opened this issue Aug 12, 2020 · 8 comments
Labels
CherryPickApproved Used during the release process for point releases FrozenDueToAge release-blocker
Milestone
Go1.15.3

Comments

@randall77
Copy link
Contributor

randall77 commented Aug 12, 2020 •
edited by dmitshur

@randall77 requested issue #40629 to be considered for backport to the next 1.15 minor release.

This patch:

+++ b/src/cmd/compile/internal/ssa/func.go
@@ -274,6 +274,9 @@ func (f *Func) freeValue(v *Value) {
        if len(v.Args) != 0 {
                f.Fatalf("value %s still has %d args", v, len(v.Args))
        }
+       if v == f.LastDeferExit {
+               println("FREEING THE LASTDEFEREXIT")
+       }
        // Clear everything but ID (which we reuse).
        id := v.ID

triggers a bunch of times during make.bash. Both 1.14.2 and tip. Looks like we need to fix this for the release - I think we're just getting lucky that we don't stack copy or gc trace such cases normally (stack copy is only likely to happen with an unrecovered panic?), or that the random other instruction's liveness map is correct (or good enough).
@gopherbot gopherbot added this to the Go1.15.1 milestone Aug 12, 2020
@randall77 randall77 added the CherryPickCandidate Used during the release process for point releases label Aug 12, 2020
@aclements
Copy link
Member

@randall77 , how do you feel about backporting this given the relatively high complexity of the CL? Is there a simpler fix that might be safer to backport?

@randall77
Copy link
Contributor Author

I don't think there's really anything simpler. I could revert a bunch of the code deletion, but since it isn't called anyway (and is buggy regardless) I'm not sure that would help. The actual code addition is small.
I'm open to suggestions though.

@gopherbot
Copy link

Change https://golang.org/cl/248621 mentions this issue: cmd/compile: fix live variable computation for deferreturn

@dmitshur dmitshur modified the milestones: Go1.15.1, Go1.15.2 Sep 1, 2020
@dmitshur dmitshur modified the milestones: Go1.15.2, Go1.15.3 Sep 9, 2020
@randall77
Copy link
Contributor Author

@dmitshur Why did this issue's fix not go out with 1.15.2? It looks like it didn't make it to CherryPickApproved, but why didn't it get to that state?

@dmitshur
Copy link
Contributor

@randall77 For a backport issue to get to CherryPickApproved state, someone on the release team needs to approve it. We've discussed this issue in past release meetings but a decision hasn't been made yet (though we are quite close by now). If there is a backport issue that you believe is critical and a minor release should not be made without that backport issue being considered (and either approved, or we find agreement not to block on it), the release-blocker label should be used to indicate that.

Also please see what I wrote here about the timing of backport issues and minor releases.

@randall77
Copy link
Contributor Author

Ok, just wanted to make sure it wasn't missed.

@toothrot toothrot added the CherryPickApproved Used during the release process for point releases label Sep 11, 2020
@toothrot
Copy link
Contributor

Approved. This is a serious issue with no workaround. As @dmitshur said, sorry for the delay.

@gopherbot gopherbot removed the CherryPickCandidate Used during the release process for point releases label Sep 11, 2020
@gopherbot
Copy link

Closed by merging 0893e6a to release-branch.go1.15.

gopherbot pushed a commit that referenced this issue Oct 5, 2020
@randall77 @dmitshur
[release-branch.go1.15] cmd/compile: fix live variable computation fo…
0893e6a 
…r deferreturn

Taking the live variable set from the last return point is problematic.
See #40629 for details, but there may not be a return point, or it may
be before the final defer.

Additionally, keeping track of the last call as a *Value doesn't quite
work. If it is dead-code eliminated, the storage for the Value is reused
for some other random instruction. Its live variable information,
if it is available at all, is wrong.

Instead, just mark all the open-defer argument slots as live
throughout the function. (They are already zero-initialized.)

Fixes #40742

Change-Id: Ie456c7db3082d0de57eaa5234a0f32525a1cce13
Reviewed-on: https://go-review.googlesource.com/c/go/+/247522
Run-TryBot: Keith Randall <khr@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Dan Scales <danscales@google.com>
(cherry picked from commit 32a84c9)
Reviewed-on: https://go-review.googlesource.com/c/go/+/248621
Trust: Dmitri Shuralyov <dmitshur@golang.org>
claucece pushed a commit to claucece/go that referenced this issue Oct 22, 2020
@randall77 @claucece
[release-branch.go1.15] cmd/compile: fix live variable computation fo…
59611c7 
…r deferreturn

Taking the live variable set from the last return point is problematic.
See golang#40629 for details, but there may not be a return point, or it may
be before the final defer.

Additionally, keeping track of the last call as a *Value doesn't quite
work. If it is dead-code eliminated, the storage for the Value is reused
for some other random instruction. Its live variable information,
if it is available at all, is wrong.

Instead, just mark all the open-defer argument slots as live
throughout the function. (They are already zero-initialized.)

Fixes golang#40742

Change-Id: Ie456c7db3082d0de57eaa5234a0f32525a1cce13
Reviewed-on: https://go-review.googlesource.com/c/go/+/247522
Run-TryBot: Keith Randall <khr@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Dan Scales <danscales@google.com>
(cherry picked from commit 32a84c9)
Reviewed-on: https://go-review.googlesource.com/c/go/+/248621
Trust: Dmitri Shuralyov <dmitshur@golang.org>
@golang golang locked and limited conversation to collaborators Oct 5, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
CherryPickApproved Used during the release process for point releases FrozenDueToAge release-blocker
Projects
None yet
Milestone
Go1.15.3
Development

No branches or pull requests
5 participants
@toothrot @dmitshur @aclements @randall77 @gopherbot