[go] cmd/compile: fix ICE when compiling global a, b = f()
Cuong Manh Le (Gerrit)
Cuong Manh Le has uploaded the change for review![]( "Open in Gerrit")
Commit message
cmd/compile: fix ICE when compiling global a, b = f()
CL 327651 rewrites a, b = f() to use temporaries when types are not
identical. That would leave OAS2 node appears in body of init function
for global variables initialization. The staticinit pass is not updated
to handle OAS2 node, causing ICE when compiling global variables.
To fix this, handle OAS2 nodes like other OAS2*, since they mostly
necessitate dynamic execution anyway.
Fixes #68264
Change diff
diff --git a/src/cmd/compile/internal/staticinit/sched.go b/src/cmd/compile/internal/staticinit/sched.go
index 7317ed1..bf4bb57 100644
--- a/src/cmd/compile/internal/staticinit/sched.go
+++ b/src/cmd/compile/internal/staticinit/sched.go
@@ -107,7 +107,7 @@
case ir.OAS:
n := n.(*ir.AssignStmt)
lhs, rhs = []ir.Node{n.X}, n.Y
- case ir.OAS2DOTTYPE, ir.OAS2FUNC, ir.OAS2MAPR, ir.OAS2RECV:
+ case ir.OAS2DOTTYPE, ir.OAS2FUNC, ir.OAS2MAPR, ir.OAS2RECV, ir.OAS2:
n := n.(*ir.AssignListStmt)
if len(n.Lhs) < 2 || len(n.Rhs) != 1 {
base.FatalfAt(n.Pos(), "unexpected shape for %v: %v", n.Op(), n)
diff --git a/test/fixedbugs/issue68264.go b/test/fixedbugs/issue68264.go
new file mode 100644
index 0000000..7d67e55
--- /dev/null
+++ b/test/fixedbugs/issue68264.go
@@ -0,0 +1,15 @@
+// compile
+
+// Copyright 2024 The Go Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style
+// license that can be found in the LICENSE file.
+
+package p
+
+type nat []int
+
+var a, b nat = y()
+
+func y() (nat, []int) {
+ return nat{0}, nat{1}
+}
Change information
- M src/cmd/compile/internal/staticinit/sched.go
- A test/fixedbugs/issue68264.go
Related details
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Cuong Manh Le (Gerrit)
Cuong Manh Le voted Commit-Queue+1![]( "Open in Gerrit")
| Commit-Queue | +1 |
Related details
- Cherry Mui
- Keith Randall
- Matthew Dempsky
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Cuong Manh Le (Gerrit)
Cuong Manh Le uploaded new patchset![]( "Open in Gerrit")
- TryBots-Pass: LUCI-TryBot-Result-1 by Go LUCI
Related details
- Cherry Mui
- Cuong Manh Le
- Keith Randall
- Matthew Dempsky
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Cuong Manh Le (Gerrit)
Cuong Manh Le voted![]( "Open in Gerrit")
- Cherry Mui
- Keith Randall
- Matthew Dempsky
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Cherry Mui (Gerrit)
Cherry Mui added 1 comment![]( "Open in Gerrit")
case ir.OAS2:
return false // rewriting of a, b = f()Can we treat OAS2 the same way as OAS2FUNC? Is there any reason we can't do that?
The comment is also confusing as it looks like an OAS2FUNC.
Related details
- Cuong Manh Le
- Keith Randall
- Matthew Dempsky
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Cuong Manh Le (Gerrit)
Cuong Manh Le added 1 comment![]( "Open in Gerrit")
case ir.OAS2:
return false // rewriting of a, b = f()Can we treat OAS2 the same way as OAS2FUNC? Is there any reason we can't do that?
The comment is also confusing as it looks like an OAS2FUNC.
Can we treat OAS2 the same way as OAS2FUNC? Is there any reason we can't do that?
Nope, because the check at L115 is only applied for OAS2* ops, where we have the len(rhs) == 1. For OAS2, we always have len(rhs) > 1.
The comment is also confusing as it looks like an OAS2FUNC.
Hmm, to be clear, I mean a, b = f() is rewrittent to:
tmp1, tmp2, = f()
a, b = tmp1, tmp2 // The OAS2 is here.
Maybe we should change it to: "The result of rewriting of a, b = f()"?
Related details
- Cherry Mui
- Keith Randall
- Matthew Dempsky
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Cherry Mui (Gerrit)
Cherry Mui voted and added 1 comment![]( "Open in Gerrit")
Votes added by Cherry Mui
| Code-Review | +2 |
1 comment
case ir.OAS2:
return false // rewriting of a, b = f()Cuong Manh LeCan we treat OAS2 the same way as OAS2FUNC? Is there any reason we can't do that?
The comment is also confusing as it looks like an OAS2FUNC.
Can we treat OAS2 the same way as OAS2FUNC? Is there any reason we can't do that?
Nope, because the check at L115 is only applied for OAS2* ops, where we have the len(rhs) == 1. For OAS2, we always have len(rhs) > 1.
The comment is also confusing as it looks like an OAS2FUNC.
Hmm, to be clear, I mean a, b = f() is rewrittent to:
tmp1, tmp2, = f()
a, b = tmp1, tmp2 // The OAS2 is here.Maybe we should change it to: "The result of rewriting of a, b = f()"?
Okay, thanks.
For the comment, for completeness, I think we want to be sure that things like
```
var a, b = S{...}, T{...} // static values
```
are actually handled as static. Have they been rewritten to separate OASes at this point? If so mention that in the comments.
Maybe something like,
// Usually OAS2 has been rewritten to separate OASes at <reference of the code>.
// What's left here is "var a, b = tmp1, tmp2" as a result from rewriting
// "var a, b = f()" that needs type conversion, which is not static.
Related details
- Cuong Manh Le
- Keith Randall
- Matthew Dempsky
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Cuong Manh Le (Gerrit)
Cuong Manh Le uploaded new patchset![]( "Open in Gerrit")
- TryBots-Pass: LUCI-TryBot-Result+1 by Go LUCI
Related details
- Cuong Manh Le
- Keith Randall
- Matthew Dempsky
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Cuong Manh Le (Gerrit)
Cuong Manh Le voted and added 1 comment![]( "Open in Gerrit")
Votes added by Cuong Manh Le
| Auto-Submit | +1 |
| Commit-Queue | +1 |
return false // rewriting of a, b = f()Cuong Manh LeCan we treat OAS2 the same way as OAS2FUNC? Is there any reason we can't do that?
The comment is also confusing as it looks like an OAS2FUNC.
Cherry MuiCan we treat OAS2 the same way as OAS2FUNC? Is there any reason we can't do that?
Nope, because the check at L115 is only applied for OAS2* ops, where we have the len(rhs) == 1. For OAS2, we always have len(rhs) > 1.
The comment is also confusing as it looks like an OAS2FUNC.
Hmm, to be clear, I mean a, b = f() is rewrittent to:
tmp1, tmp2, = f()
a, b = tmp1, tmp2 // The OAS2 is here.Maybe we should change it to: "The result of rewriting of a, b = f()"?
Okay, thanks.
For the comment, for completeness, I think we want to be sure that things like
```
var a, b = S{...}, T{...} // static values
```
are actually handled as static. Have they been rewritten to separate OASes at this point? If so mention that in the comments.Maybe something like,
// Usually OAS2 has been rewritten to separate OASes at <reference of the code>.
// What's left here is "var a, b = tmp1, tmp2" as a result from rewriting
// "var a, b = f()" that needs type conversion, which is not static.
are actually handled as static. Have they been rewritten to separate OASes at this point? If so mention that in the comments.
- Keith Randall
- Matthew Dempsky
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Than McIntosh (Gerrit)
Than McIntosh added 1 comment![]( "Open in Gerrit")
s.seenMutation = mayModifyPkgVar(rhs)Returning early means we won't invoke mayModifyPkgVar on any of the RHS expressions -- is this OK do you think?
Related details
- Cuong Manh Le
- Keith Randall
- Matthew Dempsky
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Cherry Mui (Gerrit)
Cherry Mui voted and added 1 comment![]( "Open in Gerrit")
Votes added by Cherry Mui
| Code-Review | +2 |
1 comment
case ir.OAS2:
return false // rewriting of a, b = f()Cuong Manh LeCan we treat OAS2 the same way as OAS2FUNC? Is there any reason we can't do that?
The comment is also confusing as it looks like an OAS2FUNC.
Cherry MuiCan we treat OAS2 the same way as OAS2FUNC? Is there any reason we can't do that?
Nope, because the check at L115 is only applied for OAS2* ops, where we have the len(rhs) == 1. For OAS2, we always have len(rhs) > 1.
The comment is also confusing as it looks like an OAS2FUNC.
Hmm, to be clear, I mean a, b = f() is rewrittent to:
tmp1, tmp2, = f()
a, b = tmp1, tmp2 // The OAS2 is here.Maybe we should change it to: "The result of rewriting of a, b = f()"?
Cuong Manh LeOkay, thanks.
For the comment, for completeness, I think we want to be sure that things like
```
var a, b = S{...}, T{...} // static values
```
are actually handled as static. Have they been rewritten to separate OASes at this point? If so mention that in the comments.Maybe something like,
// Usually OAS2 has been rewritten to separate OASes at <reference of the code>.
// What's left here is "var a, b = tmp1, tmp2" as a result from rewriting
// "var a, b = f()" that needs type conversion, which is not static.
are actually handled as static. Have they been rewritten to separate OASes at this point? If so mention that in the comments.
Yes, they are emitted as separated OASes by types2.
Another question is, why "var a, b = tmp1, tmp2" is not rewritten to two separate OASes? Could we do that instead?
Cuong Manh Le (Gerrit)
Cuong Manh Le added 2 comments![]( "Open in Gerrit")
Because technically, there's no reason to rewrite the assignment like that.
types2 does the rewritting to ensure variables are initialized in the correct order. That's why there's no OAS2 during staticinit before.
s.seenMutation = mayModifyPkgVar(rhs)Returning early means we won't invoke mayModifyPkgVar on any of the RHS expressions -- is this OK do you think?
Yes, because the only OAS2 nodes that could be existed at this time is the result of rewritting `a, b = f()` by us. All user codes that contains OAS2 are rewritten by types2 to ensure global variables are initialized in the correct order.
Related details
- Cherry Mui
- Keith Randall
- Matthew Dempsky
- Than McIntosh
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Cherry Mui (Gerrit)
Cherry Mui added 1 comment![]( "Open in Gerrit")
I'm not sure we want to rely on the fact that OAS2 means "a, b = f()". When other part, seemingly unrelated code changes, this property may change, and it is hard to enforce this. (E.g. what if later we rewrite another OAS2XXX to OAS2, or stop rewriting some static OAS2?) This may be okay for now, but I'd hope we have a fix that is closer to where the rewrite occurs.
Related details
- Cuong Manh Le
- Keith Randall
- Than McIntosh
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Cuong Manh Le (Gerrit)
Cuong Manh Le added 1 comment![]( "Open in Gerrit")
I think this works for any OAS2XX, not only OAS2UFUNC `a, b = f()`. Any static OAS2 nodes are rewritten by types2 to separated OAS nodes, to ensure the initialization order. Any OAS2 nodes after types2 finished the init order are meaningless for the staticinit.
Related details
- Cherry Mui
- Keith Randall
- Than McIntosh
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Cherry Mui (Gerrit)
Cherry Mui added 1 comment![]( "Open in Gerrit")
Okay. Would it be hard to check the RHS of OAS2 is indeed not static? Or, since it is only temporaries for now, we can return false if the RHS are temporaries, otherwise panic, so we know we won't miss anything.
Related details
- Cuong Manh Le
- Keith Randall
- Than McIntosh
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Cuong Manh Le (Gerrit)
Cuong Manh Le uploaded new patchset![]( "Open in Gerrit")
- TryBots-Pass: LUCI-TryBot-Result+1 by Go LUCI
Related details
- Cuong Manh Le
- Keith Randall
- Than McIntosh
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Cuong Manh Le (Gerrit)
Cuong Manh Le voted and added 1 comment![]( "Open in Gerrit")
Votes added by Cuong Manh Le
| Auto-Submit | +1 |
| Commit-Queue | +1 |
1 comment
case ir.OAS2:
return false // rewriting of a, b = f()- Keith Randall
- Than McIntosh
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Cherry Mui (Gerrit)
Cherry Mui voted and added 1 comment![]( "Open in Gerrit")
Votes added by Cherry Mui
| Code-Review | +2 |
1 comment
- Cuong Manh Le
- Keith Randall
- Than McIntosh
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |