[go] cmd/compile: ensure bloop only kept alive addressable nodes
Cuong Manh Le (Gerrit)
Cuong Manh Le has uploaded the change for review![]( "Open in Gerrit")
Commit message
cmd/compile: ensure bloop only kept alive addressable nodes
Fixes #76636
Change diff
diff --git a/src/cmd/compile/internal/bloop/bloop.go b/src/cmd/compile/internal/bloop/bloop.go
index 761b07a..e8ab9b3 100644
--- a/src/cmd/compile/internal/bloop/bloop.go
+++ b/src/cmd/compile/internal/bloop/bloop.go
@@ -45,7 +45,7 @@
"cmd/internal/src"
)
-// getNameFromNode tries to iteratively peel down the node to
+// getAddressableNameFromNode tries to iteratively peel down the node to
// get the name.
func getNameFromNode(n ir.Node) *ir.Name {
// Tries to iteratively peel down the node to get the names.
@@ -73,6 +73,14 @@
return nil
}
+// getAddressableNameFromNode is like getNameFromNode but returns nil if the node is not addressable.
+func getAddressableNameFromNode(n ir.Node) *ir.Name {
+ if name := getNameFromNode(n); name != nil && ir.IsAddressable(name) {
+ return name
+ }
+ return nil
+}
+
// keepAliveAt returns a statement that is either curNode, or a
// block containing curNode followed by a call to runtime.KeepAlive for each
// node in ns. These calls ensure that nodes in ns will be live until
@@ -94,6 +102,9 @@
if n.Sym().IsBlank() {
continue
}
+ if !ir.IsAddressable(n) {
+ base.FatalfAt(n.Pos(), "keepAliveAt: node %v is not addressable", n)
+ }
arg := ir.NewConvExpr(pos, ir.OCONV, types.Types[types.TUNSAFEPTR], typecheck.NodAddr(n))
if !n.Type().IsInterface() {
srcRType0 := reflectdata.TypePtrAt(pos, n.Type())
@@ -129,7 +140,7 @@
switch n := stmt.(type) {
case *ir.AssignStmt:
// Peel down struct and slice indexing to get the names
- name := getNameFromNode(n.X)
+ name := getAddressableNameFromNode(n.X)
if name != nil {
debugName(name, n.Pos())
ret = keepAliveAt([]ir.Node{name}, n)
@@ -144,7 +155,7 @@
case *ir.AssignListStmt:
ns := []ir.Node{}
for _, lhs := range n.Lhs {
- name := getNameFromNode(lhs)
+ name := getAddressableNameFromNode(lhs)
if name != nil {
debugName(name, n.Pos())
ns = append(ns, name)
@@ -159,7 +170,7 @@
}
ret = keepAliveAt(ns, n)
case *ir.AssignOpStmt:
- name := getNameFromNode(n.X)
+ name := getAddressableNameFromNode(n.X)
if name != nil {
debugName(name, n.Pos())
ret = keepAliveAt([]ir.Node{name}, n)
@@ -206,7 +217,7 @@
argTmps := []ir.Node{}
names := []ir.Node{}
for i, a := range n.Args {
- if name := getNameFromNode(a); name != nil {
+ if name := getAddressableNameFromNode(a); name != nil {
// If they are name, keep them alive directly.
debugName(name, n.Pos())
names = append(names, name)
@@ -215,7 +226,7 @@
s := a.(*ir.CompLitExpr)
ns := []ir.Node{}
for i, elem := range s.List {
- if name := getNameFromNode(elem); name != nil {
+ if name := getAddressableNameFromNode(elem); name != nil {
debugName(name, n.Pos())
ns = append(ns, name)
} else {
diff --git a/test/bloop.go b/test/bloop.go
index fd22132..69ae8f3 100644
--- a/test/bloop.go
+++ b/test/bloop.go
@@ -25,6 +25,12 @@
something = x[0]
}
+func receiver(f func()) { // ERROR "can inline receiver" "f does not escape"
+ f()
+}
+
+func argument() {} // ERROR "can inline argument"
+
func test(b *testing.B, localsink, cond int) { // ERROR ".*"
for i := 0; i < b.N; i++ {
caninline(1) // ERROR "inlining call to caninline"
@@ -49,5 +55,7 @@
{
caninline(1) // ERROR "inlining call to caninline" "function result will be kept alive"
}
+
+ receiver(argument) // ERROR inlining call to receiver" "function arg will be kept alive"
}
}
Change information
- M src/cmd/compile/internal/bloop/bloop.go
- M test/bloop.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![]( "Open in Gerrit")
| Auto-Submit | +1 |
| Commit-Queue | +1 |
Related details
- Junyang Shao
- Keith Randall
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Junyang Shao (Gerrit)
Junyang Shao voted and added 1 comment![]( "Open in Gerrit")
Votes added by Junyang Shao
| Code-Review | +2 |
1 comment
Related details
- Cuong Manh Le
- Keith Randall
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Junyang Shao (Gerrit)
Junyang Shao added 1 comment![]( "Open in Gerrit")
For non-addressable names, could it be useful to be kept alive?
The specific case your comment in CL 725180 mentioned is a global, which I believe DSE won't be able to remove reference of it, so not keeping it alive is right.
But if there are cases that we want to keep them alive, maybe we can do a conversion to TINTER for them instead of TUNSAFEPTR to make it work?
Related details
- Cuong Manh Le
- Keith Randall
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
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")
- Keith Randall
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")
For non-addressable names, could it be useful to be kept alive?
The specific case your comment in CL 725180 mentioned is a global, which I believe DSE won't be able to remove reference of it, so not keeping it alive is right.
But if there are cases that we want to keep them alive, maybe we can do a conversion to TINTER for them instead of TUNSAFEPTR to make it work?
Hmm, isn't that copying it to a tmp, then keeping the tmp alive is enough?
Related details
- Junyang Shao
- Keith Randall
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Junyang Shao (Gerrit)
Junyang Shao voted and added 1 comment![]( "Open in Gerrit")
Votes added by Junyang Shao
| Code-Review | +2 |
Cuong Manh LeFor non-addressable names, could it be useful to be kept alive?
The specific case your comment in CL 725180 mentioned is a global, which I believe DSE won't be able to remove reference of it, so not keeping it alive is right.
But if there are cases that we want to keep them alive, maybe we can do a conversion to TINTER for them instead of TUNSAFEPTR to make it work?
Hmm, isn't that copying it to a tmp, then keeping the tmp alive is enough?
Ohh yes, I forgot that for the call arg case there is a tmp fallback for name not found.
I assume non-addressable names are impossible to be present in assignment lhs, but I am not 100% sure as I am not very familiar with the ir, feel free to close my comment.
Thanks :D
Related details
- Cuong Manh Le
- Keith Randall
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")
Cuong Manh LeFor non-addressable names, could it be useful to be kept alive?
The specific case your comment in CL 725180 mentioned is a global, which I believe DSE won't be able to remove reference of it, so not keeping it alive is right.
But if there are cases that we want to keep them alive, maybe we can do a conversion to TINTER for them instead of TUNSAFEPTR to make it work?
Junyang ShaoHmm, isn't that copying it to a tmp, then keeping the tmp alive is enough?
Ohh yes, I forgot that for the call arg case there is a tmp fallback for name not found.
I assume non-addressable names are impossible to be present in assignment lhs, but I am not 100% sure as I am not very familiar with the ir, feel free to close my comment.
Thanks :D
I assume non-addressable names are impossible to be present in assignment lhs
It's true.
Both types2 (*Checker.lhsVar) and typecheck (checkassign) ensure that only addressable names are present in the lhs of the assignment.
Related details
- Keith Randall
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Keith Randall (Gerrit)
Keith Randall added 1 comment![]( "Open in Gerrit")
return n.(*ir.Name)getNameFromNode isn't used any more.
Can we just leave the name as is, and put the additional condition in right here?
```
name := n.(*ir.Name)
if !ir.IsAddressable(name) {
name = nil
}
return name
```
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 added 1 comment![]( "Open in Gerrit")
return n.(*ir.Name)getNameFromNode isn't used any more.
Can we just leave the name as is, and put the additional condition in right here?```
name := n.(*ir.Name)
if !ir.IsAddressable(name) {
name = nil
}
return name
```
I also thought about this, but I’m afraid we wont catch all cases here, for example, all OCONV, OVONVNOP, OCONVIFACE below can return ONAME, too.
That’s why I ended up with new entrypoint to catch ONAME, I think it would make things easier when bisecting needed. We can cleanup then when we are sure that nothing broken.
Related details
- Keith Randall
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")
return n.(*ir.Name)Cuong Manh LegetNameFromNode isn't used any more.
Can we just leave the name as is, and put the additional condition in right here?```
name := n.(*ir.Name)
if !ir.IsAddressable(name) {
name = nil
}
return name
```
I also thought about this, but I’m afraid we wont catch all cases here, for example, all OCONV, OVONVNOP, OCONVIFACE below can return ONAME, too.
That’s why I ended up with new entrypoint to catch ONAME, I think it would make things easier when bisecting needed. We can cleanup then when we are sure that nothing broken.
Hmm, further brainstorming, in case of `f(T(arg))`, what do we want to keep alive? It's `T(arg)` or `arg`?
Related details
- Keith Randall
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Keith Randall (Gerrit)
Keith Randall added 1 comment![]( "Open in Gerrit")
return n.(*ir.Name)Cuong Manh LegetNameFromNode isn't used any more.
Can we just leave the name as is, and put the additional condition in right here?```
name := n.(*ir.Name)
if !ir.IsAddressable(name) {
name = nil
}
return name
```
Cuong Manh LeI also thought about this, but I’m afraid we wont catch all cases here, for example, all OCONV, OVONVNOP, OCONVIFACE below can return ONAME, too.
That’s why I ended up with new entrypoint to catch ONAME, I think it would make things easier when bisecting needed. We can cleanup then when we are sure that nothing broken.
Hmm, further brainstorming, in case of `f(T(arg))`, what do we want to keep alive? It's `T(arg)` or `arg`?
The spec for b.Loop says
arguments to and results from function calls within the loop are kept alive, preventing the compiler from fully optimizing away the loop body
That's not just outermost function calls, it is all calls. So both `T(arg)` and `arg` (and `f(T(arg))`) should be kept alive.
(But not from function calls that might appear after inlining. Just the ones that are syntactically there in the source code.)
Related details
- Cuong Manh Le
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")
return n.(*ir.Name)Cuong Manh LegetNameFromNode isn't used any more.
Can we just leave the name as is, and put the additional condition in right here?```
name := n.(*ir.Name)
if !ir.IsAddressable(name) {
name = nil
}
return name
```
Cuong Manh LeI also thought about this, but I’m afraid we wont catch all cases here, for example, all OCONV, OVONVNOP, OCONVIFACE below can return ONAME, too.
That’s why I ended up with new entrypoint to catch ONAME, I think it would make things easier when bisecting needed. We can cleanup then when we are sure that nothing broken.
Keith RandallHmm, further brainstorming, in case of `f(T(arg))`, what do we want to keep alive? It's `T(arg)` or `arg`?
The spec for b.Loop says
arguments to and results from function calls within the loop are kept alive, preventing the compiler from fully optimizing away the loop bodyThat's not just outermost function calls, it is all calls. So both `T(arg)` and `arg` (and `f(T(arg))`) should be kept alive.
(But not from function calls that might appear after inlining. Just the ones that are syntactically there in the source code.)
Hmm, then I think even before this CL, only arg will be kept alive when T(arg) is a argument of function call.
Should we let this CL in to fix the ICE, then followup CL will fix the semantics.
Related details
- Keith Randall
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Keith Randall (Gerrit)
Keith Randall voted and added 1 comment![]( "Open in Gerrit")
Votes added by Keith Randall
| Code-Review | +2 |
1 comment
return n.(*ir.Name)Cuong Manh LegetNameFromNode isn't used any more.
Can we just leave the name as is, and put the additional condition in right here?```
name := n.(*ir.Name)
if !ir.IsAddressable(name) {
name = nil
}
return name
```
Cuong Manh LeI also thought about this, but I’m afraid we wont catch all cases here, for example, all OCONV, OVONVNOP, OCONVIFACE below can return ONAME, too.
That’s why I ended up with new entrypoint to catch ONAME, I think it would make things easier when bisecting needed. We can cleanup then when we are sure that nothing broken.
Keith RandallHmm, further brainstorming, in case of `f(T(arg))`, what do we want to keep alive? It's `T(arg)` or `arg`?
Cuong Manh LeThe spec for b.Loop says
arguments to and results from function calls within the loop are kept alive, preventing the compiler from fully optimizing away the loop bodyThat's not just outermost function calls, it is all calls. So both `T(arg)` and `arg` (and `f(T(arg))`) should be kept alive.
(But not from function calls that might appear after inlining. Just the ones that are syntactically there in the source code.)
Hmm, then I think even before this CL, only arg will be kept alive when T(arg) is a argument of function call.
Should we let this CL in to fix the ICE, then followup CL will fix the semantics.
Yes, sounds like a plan. The followup can be for 1.27, I think.
Related details
- Cuong Manh Le
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Keith Randall (Gerrit)
Keith Randall voted Code-Review+1![]( "Open in Gerrit")
| Code-Review | +1 |
Related details
- Cuong Manh Le
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Keith Randall (Gerrit)
Keith Randall submitted the change![]( "Open in Gerrit")
Change information
cmd/compile: ensure bloop only kept alive addressable nodes
Fixes #76636
- M src/cmd/compile/internal/bloop/bloop.go
- M test/bloop.go
Code-Review: +2 by Keith Randall, +2 by Junyang Shao, +1 by Keith Randall
TryBots-Pass: LUCI-TryBot-Result+1 by Go LUCI
| 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")
return n.(*ir.Name)Cuong Manh LegetNameFromNode isn't used any more.
Can we just leave the name as is, and put the additional condition in right here?```
name := n.(*ir.Name)
if !ir.IsAddressable(name) {
name = nil
}
return name
```
Cuong Manh LeI also thought about this, but I’m afraid we wont catch all cases here, for example, all OCONV, OVONVNOP, OCONVIFACE below can return ONAME, too.
That’s why I ended up with new entrypoint to catch ONAME, I think it would make things easier when bisecting needed. We can cleanup then when we are sure that nothing broken.
Keith RandallHmm, further brainstorming, in case of `f(T(arg))`, what do we want to keep alive? It's `T(arg)` or `arg`?
Cuong Manh LeThe spec for b.Loop says
arguments to and results from function calls within the loop are kept alive, preventing the compiler from fully optimizing away the loop bodyThat's not just outermost function calls, it is all calls. So both `T(arg)` and `arg` (and `f(T(arg))`) should be kept alive.
(But not from function calls that might appear after inlining. Just the ones that are syntactically there in the source code.)
Keith RandallHmm, then I think even before this CL, only arg will be kept alive when T(arg) is a argument of function call.
Should we let this CL in to fix the ICE, then followup CL will fix the semantics.
Yes, sounds like a plan. The followup can be for 1.27, I think.
Yeah, agree.
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. |