runtime: pcdata is -2 and 12 locals stack map entries error on nil pointer #40629
Comments
ianlancetaylor
changed the title
ianlancetaylor
added
the
NeedsInvestigation
|
I can reproduce. Here's what's happening:
I'm not really sure why it can't find a good stack map at that instruction. I've tried to hack up a reproducer for this, but I can't get step 4 to fail. I will investigate more. |
|
This appears to be a bug in open-coded defers. When I turn them off, the problem goes away. The underlying problem is that the call to Looks like the tracking of I think we need to compute the |
|
This patch: 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 please open a backport issue for 1.14. 1.13 is ok, as open-coded defers were released for 1.14. |
|
Backport issue(s) opened: #40646 (for 1.13), #40647 (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. |
|
Here's a simple-ish reproducer: |
|
I think there's something fundamentally kinda broken with using the last defer as the point to grab the live variables for the deferreturn. With code like: There's only one return in this code, and it only has p in its live args, not q. But we need q live in case I was thinking that we could look harder for the last return sequence, but I don't think that actually works in all cases. I think we need to mark the defer args slots live across the whole function (and zero them at entry). |
|
Yes, the setting of the stack map for the deferreturn stub is tricky, because it is not otherwise connected to the rest of the function. And, as you show, it is especially tricky when we have parts of the function that can never return (have an infinite loop or panic). Note there is already some extra code to deal with a related case - making sure that the defer args are kept alive in code that never returns but could have a panic: https://go-review.googlesource.com/c/go/+/204802 . This code does some forcing of liveness of defer arg slots in the blocks that never return. It could be useful to adapt for the method of fixing that you suggest. Note that we already zero any defer args in the entry block that have pointers in them. It would be nice if we could just not do open-coded defers if a function has any code that cannot exit because of an infinite loop. Note the change above also determines which blocks cannot reach a return or an exit (panic) (hence must be because of an infinite loop). But that code is only in SSA after the genssa pass that creates the open-coded defers. I don't think it would be easy to write anything equivalent before generating SSA, would it? |
|
Since I am just heading out for vacation starting tomorrow for the next two weeks, and will have only partial connectivity (and mostly in the evenings), Keith was nice enough to say that he would work on the fix for this. I will join in as I can, and definitely plan to review the code for the fix. |
|
I think the OP's example did not use infinite loops. It just had a return that ended up being removed by deadcode (I haven't checked, but probably prove decided that some branch was impossible). If that happened to be the last return in program order, then the live map for the deferreturn is wrong. It's easier to repro with an infinite loop, but probably not necessary. |
|
Change https://golang.org/cl/247522 mentions this issue: |
|
@gopherbot, please open a backport issue for 1.15. |
|
@gopherbot is lazy. Did it myself. |
|
Change https://golang.org/cl/248621 mentions this issue: |
|
Change https://golang.org/cl/248622 mentions this issue: |
dmitshur
added
NeedsFix
…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>
…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 #40647 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/+/248622 TryBot-Result: Go Bot <gobot@golang.org> Trust: Dmitri Shuralyov <dmitshur@golang.org>
…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>
What version of Go are you using (
go version)?Does this issue reproduce with the latest release?
Not with 1.15rc1 (or 1.13.15)
What operating system and processor architecture are you using (
go env)?go envOutputWhat did you do?
Run
$ ./matterircd -debugMatterircd will spawn an irc-server on 127.0.0.1:6667 by default, connect with an irc client, do
/msg slack login 1 2 3It crashes because i'm not checking for a nil pointer on
u.credentialshere: https://github.com/42wim/matterircd/blob/685c4a3ed67d78cee4027d2e735586f824dd70c7/mm-go-irckit/mmservice.go#L51I'm reporting it because on go1.15rc1 and go1.13.15 it panics "the normal way" :)
What did you expect to see?
Normal panic
What did you see instead?
The text was updated successfully, but these errors were encountered: