[go] cmd/compile: recursively insert bounds checks for zero-sized arrays
Cuong Manh Le (Gerrit)
Cuong Manh Le has uploaded the change for review![]( "Open in Gerrit")
Commit message
cmd/compile: recursively insert bounds checks for zero-sized arrays
CL 774120 attempted to fix missing bounds checks for zero-sized arrays,
but the implementation was incomplete. It failed to account for nested
zero-sized types, where an index operation might still require a check
despite the inner element size.
This change generalizes the fix by recursively walking the IR nodes
and ensuring a bounds check is inserted for every array index
operation encountered, regardless of nesting depth.
Updates #79197
Fixes #79236
Change diff
diff --git a/src/cmd/compile/internal/ssagen/ssa.go b/src/cmd/compile/internal/ssagen/ssa.go
index d049631..5c57c31 100644
--- a/src/cmd/compile/internal/ssagen/ssa.go
+++ b/src/cmd/compile/internal/ssagen/ssa.go
@@ -4546,11 +4546,11 @@
// they are somewhere inside an outer [0].
// We can ignore the actual assignment, it is dynamically
// unreachable. See issue 77635.
+ s.boundsCheckArrayIndex(left)
return
}
if t.Size() == 0 {
- len := s.constInt(types.Types[types.TINT], n)
- s.boundsCheck(i, len, ssa.BoundsIndex, false)
+ s.boundsCheckArrayIndex(left)
return
}
@@ -5262,6 +5262,7 @@
// &x[i], which will always panic when evaluated.
// We just return something reasonable in this case.
// It will be dynamically unreachable. See issue 77635.
+ s.boundsCheckArrayIndex(n)
return s.newValue1A(ssa.OpAddr, n.Type().PtrTo(), ir.Syms.Zerobase, s.sb)
}
@@ -5444,6 +5445,25 @@
return s.newValue2(ssa.OpNilCheck, ptr.Type, ptr, s.mem())
}
+// boundsCheckArrayIndex performs bounds checking recursively for indexed array expressions.
+func (s *state) boundsCheckArrayIndex(n ir.Node) {
+ for {
+ nn := n
+ if nn.Op() == ir.OINDEX {
+ nn := nn.(*ir.IndexExpr)
+ typ := nn.X.Type()
+ if typ.IsArray() {
+ idx := s.expr(nn.Index)
+ len := s.constInt(types.Types[types.TINT], typ.NumElem())
+ s.boundsCheck(idx, len, ssa.BoundsIndex, nn.Bounded())
+ n = nn.X
+ continue
+ }
+ }
+ break
+ }
+}
+
// boundsCheck generates bounds checking code. Checks if 0 <= idx <[=] len, branches to exit if not.
// Starts a new block on return.
// On input, len must be converted to full int width and be nonnegative.
diff --git a/test/fixedbugs/issue79236.go b/test/fixedbugs/issue79236.go
new file mode 100644
index 0000000..f09c937
--- /dev/null
+++ b/test/fixedbugs/issue79236.go
@@ -0,0 +1,41 @@
+// run
+
+// Copyright 2026 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 main
+
+//go:noinline
+func a1(i int) {
+ var a [2][0]int
+ a[i] = [0]int{}
+}
+
+//go:noinline
+func a2(i int) int {
+ var a [0][2]int
+ return a[i][0]
+}
+
+//go:noinline
+func a3(i int) {
+ var a [0][2]int
+ a[i][0] = 1
+}
+
+func wantPanic(name string, f func()) {
+ defer func() {
+ if r := recover(); r != nil {
+ return
+ }
+ panic(name + ": no panic (bug)")
+ }()
+ f()
+}
+
+func main() {
+ wantPanic("a1", func() { a1(5) })
+ wantPanic("a2", func() { _ = a2(5) })
+ wantPanic("a3", func() { a3(5) })
+}
Change information
- M src/cmd/compile/internal/ssagen/ssa.go
- A test/fixedbugs/issue79236.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
- David Chase
- Josh Bleecher Snyder
- 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 Commit-Queue+1![]( "Open in Gerrit")
David Chase (Gerrit)
David Chase voted Code-Review+2![]( "Open in Gerrit")
- Cuong Manh Le
- Josh Bleecher Snyder
- 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 2 comments![]( "Open in Gerrit")
s.assign(left.X, v, false, 0)Seems reasonable, but I think the underlying problem here is we don't evaluate `left.X` in either of the cases above (like we do in this line). Could there be other things with side effects in there, other than index panics?
Related details
- Cuong Manh Le
- Josh Bleecher Snyder
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 golang...@luci-project-accounts.iam.gserviceaccount.com
Related details
- Cuong Manh Le
- Josh Bleecher Snyder
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 2 comments![]( "Open in Gerrit")
Votes added by Cuong Manh Le
| Auto-Submit | +1 |
| Commit-Queue | +1 |
2 comments
s.assign(left.X, v, false, 0)Seems reasonable, but I think the underlying problem here is we don't evaluate `left.X` in either of the cases above (like we do in this line). Could there be other things with side effects in there, other than index panics?
Seems reasonable, but I think the underlying problem here is we don't evaluate left.X in either of the cases above (like we do in this line)
Ah right, then I think we can simplify `s.boundsCheckArrayIndex` by evaluating `n.X`.
Could there be other things with side effects in there, other than index panics?
I could not think off-hand, but just evaluating left.X like you suggested above is safer.
nn := nCuong Manh LeThis `nn` seems unnecessary.
Acknowledged
Related details
- Josh Bleecher Snyder
- 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")
- Cuong Manh Le
- Josh Bleecher Snyder
- 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")
- Josh Bleecher Snyder
- 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")
Related details
- Josh Bleecher Snyder
- 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 3 comments![]( "Open in Gerrit")
// unreachable. See issue 77635.Maybe just `_ = s.expr(left.X)` would be enough here?
We don't actually need the outermost bounds check. It is guaranteed not to even reach it.
len := s.constInt(types.Types[types.TINT], n)We would need the `expr` call here also.
(But maybe `expr` isn't right if the value isn't ssa-able? Then maybe we need `.addr` instead. That could use a `evalForSideEffects` or some such function that picks between the two.)
idx := s.expr(nn.Index)I think we already evaluated this in the caller? We would have to pass in the already-computed *ssa.Value, I think. Don't want to evaluate the index twice.
Related details
- Cuong Manh Le
- Josh Bleecher Snyder
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
David Chase (Gerrit)
David Chase voted Code-Review+0![]( "Open in Gerrit")
| Code-Review | +0 |
Related details
- Cuong Manh Le
- Josh Bleecher Snyder
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 golang...@luci-project-accounts.iam.gserviceaccount.com
Related details
- Cuong Manh Le
- Josh Bleecher Snyder
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 4 comments![]( "Open in Gerrit")
Votes added by Cuong Manh Le
| Auto-Submit | +1 |
| Commit-Queue | +1 |
4 comments
// unreachable. See issue 77635.Maybe just `_ = s.expr(left.X)` would be enough here?
We don't actually need the outermost bounds check. It is guaranteed not to even reach it.
Nope, in case of `a1` function:
```
//go:noinline
func a1(i int) {
var a [2][0]int
a[i] = [0]int{}
}
```
`left.X` will be `a`, thus calling `s.expr(left.X)` leads to missing bound checks.
len := s.constInt(types.Types[types.TINT], n)We would need the `expr` call here also.
(But maybe `expr` isn't right if the value isn't ssa-able? Then maybe we need `.addr` instead. That could use a `evalForSideEffects` or some such function that picks between the two.)
Done
s.assign(left.X, v, false, 0)Cuong Manh LeSeems reasonable, but I think the underlying problem here is we don't evaluate `left.X` in either of the cases above (like we do in this line). Could there be other things with side effects in there, other than index panics?
Seems reasonable, but I think the underlying problem here is we don't evaluate left.X in either of the cases above (like we do in this line)
Ah right, then I think we can simplify `s.boundsCheckArrayIndex` by evaluating `n.X`.
Could there be other things with side effects in there, other than index panics?
I could not think off-hand, but just evaluating left.X like you suggested above is safer.
Done
I think we already evaluated this in the caller? We would have to pass in the already-computed *ssa.Value, I think. Don't want to evaluate the index twice.
- Josh Bleecher Snyder
- 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")
// unreachable. See issue 77635.Cuong Manh LeMaybe just `_ = s.expr(left.X)` would be enough here?
We don't actually need the outermost bounds check. It is guaranteed not to even reach it.
Nope, in case of `a1` function:
```
//go:noinline
func a1(i int) {
var a [2][0]int
a[i] = [0]int{}
}
````left.X` will be `a`, thus calling `s.expr(left.X)` leads to missing bound checks.
Hey @khr@golang.oỏg, a gentle ping 😊
Related details
- Josh Bleecher Snyder
- 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")
// unreachable. See issue 77635.Cuong Manh LeMaybe just `_ = s.expr(left.X)` would be enough here?
We don't actually need the outermost bounds check. It is guaranteed not to even reach it.
Cuong Manh LeNope, in case of `a1` function:
```
//go:noinline
func a1(i int) {
var a [2][0]int
a[i] = [0]int{}
}
````left.X` will be `a`, thus calling `s.expr(left.X)` leads to missing bound checks.
Hey @khr@golang.oỏg, a gentle ping 😊
Ops, my typo.
Keith Randall (Gerrit)
Keith Randall added 1 comment![]( "Open in Gerrit")
// unreachable. See issue 77635.Cuong Manh LeMaybe just `_ = s.expr(left.X)` would be enough here?
We don't actually need the outermost bounds check. It is guaranteed not to even reach it.
Cuong Manh LeNope, in case of `a1` function:
```
//go:noinline
func a1(i int) {
var a [2][0]int
a[i] = [0]int{}
}
````left.X` will be `a`, thus calling `s.expr(left.X)` leads to missing bound checks.
Cuong Manh LeHey @khr@golang.oỏg, a gentle ping 😊
Ops, my typo.
Ah, ok.
Then this comment needs updating. There are cases here where it is not always-panic. Those are always 0-size assignment cases?
Maybe it would be clearer to do the `t.Size()==0` case before the `n != 1` case?
Related details
- Cuong Manh Le
- Josh Bleecher Snyder
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 golang...@luci-project-accounts.iam.gserviceaccount.com
Related details
- Cuong Manh Le
- Josh Bleecher Snyder
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
// unreachable. See issue 77635.Cuong Manh LeMaybe just `_ = s.expr(left.X)` would be enough here?
We don't actually need the outermost bounds check. It is guaranteed not to even reach it.
Cuong Manh LeNope, in case of `a1` function:
```
//go:noinline
func a1(i int) {
var a [2][0]int
a[i] = [0]int{}
}
````left.X` will be `a`, thus calling `s.expr(left.X)` leads to missing bound checks.
Cuong Manh LeHey @khr@golang.oỏg, a gentle ping 😊
Keith RandallOps, my typo.
Ah, ok.
Then this comment needs updating. There are cases here where it is not always-panic. Those are always 0-size assignment cases?
Maybe it would be clearer to do the `t.Size()==0` case before the `n != 1` case?
Maybe it would be clearer to do the t.Size()==0 case before the n != 1 case?
- Josh Bleecher Snyder
- 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 3 comments![]( "Open in Gerrit")
s.boundsCheck(z, z, ssa.BoundsIndex, false)Does this need a special case also?
```
func f() [0]int { ... }
f()[i] = 3
```
We still do want to evaluate `f()`.
s.boundsCheckArrayIndex(left, i)This needs to be left.X, to avoid re-evaluating the index.
s.boundsCheckArrayIndex(left.X, nil)This is weird, because we know nothing about left.X. I see no reason why it is necessarily an array index. I think we just want a "eval this expr and throw away the result" evaluator.
Related details
- Cuong Manh Le
- Josh Bleecher Snyder
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 golang...@luci-project-accounts.iam.gserviceaccount.com
Related details
- Cuong Manh Le
- Josh Bleecher Snyder
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 3 comments![]( "Open in Gerrit")
Votes added by Cuong Manh Le
| Auto-Submit | +1 |
| Commit-Queue | +1 |
3 comments
s.boundsCheck(z, z, ssa.BoundsIndex, false)Does this need a special case also?
```
func f() [0]int { ... }
f()[i] = 3
```
We still do want to evaluate `f()`.
Hmm, `f()[i]` is not addressable, so it even does not pass typechecker.
Further, even in case it passes, I believe order pass ensures `f()` was evaluated before we reach here, by re-written in roughly to:
```
tmp = f()
tmp[i] = 3
```
s.boundsCheckArrayIndex(left, i)This needs to be left.X, to avoid re-evaluating the index.
Wait, the index belongs to `left`, not `left.X`. For example, with `a[i] = [0]int{}` case above, we have `left` is `a[i]`, `left.X` is `a`, and the index is `i`. So we do want to call `s.boundsCheckArrayIndex(a[i], i)`, so we won't re-evaluate `i`.
This is weird, because we know nothing about left.X. I see no reason why it is necessarily an array index. I think we just want a "eval this expr and throw away the result" evaluator.
- Josh Bleecher Snyder
- 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 2 comments![]( "Open in Gerrit")
s.boundsCheck(z, z, ssa.BoundsIndex, false)Cuong Manh LeDoes this need a special case also?
```
func f() [0]int { ... }
f()[i] = 3
```
We still do want to evaluate `f()`.
Hmm, `f()[i]` is not addressable, so it even does not pass typechecker.
Further, even in case it passes, I believe order pass ensures `f()` was evaluated before we reach here, by re-written in roughly to:
```
tmp = f()
tmp[i] = 3
```
I guess I meant something generically side-effecting in place of `f()`. For example,
var x *[0]int
(*x)[i] = 3
should nil panic, not bounds panic?
s.boundsCheckArrayIndex(left, i)Cuong Manh LeThis needs to be left.X, to avoid re-evaluating the index.
Wait, the index belongs to `left`, not `left.X`. For example, with `a[i] = [0]int{}` case above, we have `left` is `a[i]`, `left.X` is `a`, and the index is `i`. So we do want to call `s.boundsCheckArrayIndex(a[i], i)`, so we won't re-evaluate `i`.
Good point. `boundsCheckArrayIndex` is has this strange maybe-evaluates-part-of-it semantics.
I'd really like to do this somehow without such a strange function.
Related details
- Cuong Manh Le
- Josh Bleecher Snyder
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 golang...@luci-project-accounts.iam.gserviceaccount.com
Related details
- Cuong Manh Le
- Josh Bleecher Snyder
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 2 comments![]( "Open in Gerrit")
Votes added by Cuong Manh Le
| Auto-Submit | +1 |
| Commit-Queue | +1 |
2 comments
s.boundsCheck(z, z, ssa.BoundsIndex, false)Cuong Manh LeDoes this need a special case also?
```
func f() [0]int { ... }
f()[i] = 3
```
We still do want to evaluate `f()`.
Keith RandallHmm, `f()[i]` is not addressable, so it even does not pass typechecker.
Further, even in case it passes, I believe order pass ensures `f()` was evaluated before we reach here, by re-written in roughly to:
```
tmp = f()
tmp[i] = 3
```
I guess I meant something generically side-effecting in place of `f()`. For example,
var x *[0]int
(*x)[i] = 3should nil panic, not bounds panic?
This code does nil panic, since it won't reach here. The condition of this block explicitly requires the `left.X` type is array:
```
left.Op() == ir.OINDEX && left.(*ir.IndexExpr).X.Type().IsArray()
```
In this case, it's a pointer to array.
s.boundsCheckArrayIndex(left, i)Cuong Manh LeThis needs to be left.X, to avoid re-evaluating the index.
Keith RandallWait, the index belongs to `left`, not `left.X`. For example, with `a[i] = [0]int{}` case above, we have `left` is `a[i]`, `left.X` is `a`, and the index is `i`. So we do want to call `s.boundsCheckArrayIndex(a[i], i)`, so we won't re-evaluate `i`.
Good point. `boundsCheckArrayIndex` is has this strange maybe-evaluates-part-of-it semantics.
I'd really like to do this somehow without such a strange function.
Oh, then I think we only need to call `boundsCheckArrayIndex` in case of `s.addr`, so we don't need the maybe-evaluates-part-of-it semantics anymore.
Related details
- Josh Bleecher Snyder
- 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 2 comments![]( "Open in Gerrit")
s.boundsCheck(z, z, ssa.BoundsIndex, false)Cuong Manh LeDoes this need a special case also?
```
func f() [0]int { ... }
f()[i] = 3
```
We still do want to evaluate `f()`.
Keith RandallHmm, `f()[i]` is not addressable, so it even does not pass typechecker.
Further, even in case it passes, I believe order pass ensures `f()` was evaluated before we reach here, by re-written in roughly to:
```
tmp = f()
tmp[i] = 3
```
Cuong Manh LeI guess I meant something generically side-effecting in place of `f()`. For example,
var x *[0]int
(*x)[i] = 3should nil panic, not bounds panic?
This code does nil panic, since it won't reach here. The condition of this block explicitly requires the `left.X` type is array:
```
left.Op() == ir.OINDEX && left.(*ir.IndexExpr).X.Type().IsArray()
```In this case, it's a pointer to array.
Kindly ping @k...@golang.org?
s.boundsCheckArrayIndex(left, i)Cuong Manh LeThis needs to be left.X, to avoid re-evaluating the index.
Keith RandallWait, the index belongs to `left`, not `left.X`. For example, with `a[i] = [0]int{}` case above, we have `left` is `a[i]`, `left.X` is `a`, and the index is `i`. So we do want to call `s.boundsCheckArrayIndex(a[i], i)`, so we won't re-evaluate `i`.
Cuong Manh LeGood point. `boundsCheckArrayIndex` is has this strange maybe-evaluates-part-of-it semantics.
I'd really like to do this somehow without such a strange function.
Oh, then I think we only need to call `boundsCheckArrayIndex` in case of `s.addr`, so we don't need the maybe-evaluates-part-of-it semantics anymore.
Done
Related details
- Josh Bleecher Snyder
- 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. |