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

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

Comments

@randall77
Copy link
Contributor

@randall77 randall77 commented Aug 12, 2020

@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
@aclements
Copy link
Member

@aclements aclements commented Aug 14, 2020

@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

@randall77 randall77 commented Aug 14, 2020

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

@gopherbot gopherbot commented Aug 14, 2020

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

@dmitshur dmitshur removed this from the Go1.15.1 milestone Sep 1, 2020
@dmitshur dmitshur added this to the Go1.15.2 milestone Sep 1, 2020
@dmitshur dmitshur removed this from the Go1.15.2 milestone Sep 9, 2020
@dmitshur dmitshur added this to the Go1.15.3 milestone Sep 9, 2020
@randall77
Copy link
Contributor Author

@randall77 randall77 commented Sep 10, 2020

@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

@dmitshur dmitshur commented Sep 10, 2020

@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

@randall77 randall77 commented Sep 10, 2020

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

@toothrot
Copy link
Contributor

@toothrot toothrot commented Sep 11, 2020

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

@gopherbot
Copy link

@gopherbot gopherbot commented Oct 5, 2020

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

@gopherbot gopherbot closed this Oct 5, 2020
gopherbot pushed a commit that referenced this issue Oct 5, 2020
…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 added a commit to claucece/go that referenced this issue Oct 22, 2020
…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.
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
5 participants