Skip to content

Commit 2c9689a

Browse files
thepuddsgopherbot
authored andcommitted
cmd/compile/internal/escape: add hash for bisecting stack allocation of variable-sized makeslice
CL 653856 enabled stack allocation of variable-sized makeslice results. This CL adds debug hashing of that change, plus a debug flag to control the byte threshold used. The debug hashing machinery means we also now have a way to disable just the CL 653856 optimization by doing -gcflags='all=-d=variablemakehash=n' or similar, though the stderr output will then typically have many lines of debug hash output. Using this CL plus the bisect command, I was able to retroactively find one of the lines of code responsible for #73199: $ bisect -compile=variablemake go test -skip TestListWireGuardDrivers [...] bisect: FOUND failing change set --- change set #1 (enabling changes causes failure) ./security_windows.go:1321:38 (variablemake) ./security_windows.go:1321:38 (variablemake) --- Previously, I had tracked down those lines by diffing '-gcflags=-m=1' output and brief code inspection, but seeing the bisect was very nice. This CL also adds a compiler debug flag to control the threshold for stack allocation of variably sized make results. This can help us identify more code that is relying on certain stack allocations. This might be a temporary flag that we delete prior to Go 1.25 (given we would not want people to rely on it), or maybe it might make sense to keep it for some period of time beyond the release of Go 1.25 to help the ecosystem shake out other bugs. Using these two flags together (and picking a threshold of 64 rather than the default of 32), it looks for example like this x/sys/windows code might be relying on stack allocation of a byte slice: $ bisect -compile=variablemake go test -gcflags=-d=variablemakethreshold=64 -skip TestListWireGuardDrivers [...] bisect: FOUND failing change set --- change set #1 (enabling changes causes failure) ./syscall_windows_test.go:1178:16 (variablemake) Updates #73199 Fixes #73253 Change-Id: I160179a0e3c148c3ea86be5c9b6cea8a52c3e5b7 Reviewed-on: https://go-review.googlesource.com/c/go/+/663795 Auto-Submit: Keith Randall <[email protected]> Reviewed-by: Dmitri Shuralyov <[email protected]> Reviewed-by: Keith Randall <[email protected]> Reviewed-by: Keith Randall <[email protected]> LUCI-TryBot-Result: Go LUCI <[email protected]>
1 parent 30aca06 commit 2c9689a

File tree

4 files changed

+15
-7
lines changed

4 files changed

+15
-7
lines changed

src/cmd/compile/internal/base/debug.go

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -72,6 +72,8 @@ type DebugFlags struct {
7272
PGOInlineBudget int `help:"inline budget for hot functions" concurrent:"ok"`
7373
PGODevirtualize int `help:"enable profile-guided devirtualization; 0 to disable, 1 to enable interface devirtualization, 2 to enable function devirtualization" concurrent:"ok"`
7474
RangeFuncCheck int `help:"insert code to check behavior of range iterator functions" concurrent:"ok"`
75+
VariableMakeHash string `help:"hash value for debugging stack allocation of variable-sized make results" concurrent:"ok"`
76+
VariableMakeThreshold int `help:"threshold in bytes for possible stack allocation of variable-sized make results" concurrent:"ok"`
7577
WrapGlobalMapDbg int `help:"debug trace output for global map init wrapping"`
7678
WrapGlobalMapCtl int `help:"global map init wrap control (0 => default, 1 => off, 2 => stress mode, no size cutoff)"`
7779
ZeroCopy int `help:"enable zero-copy string->[]byte conversions" concurrent:"ok"`

src/cmd/compile/internal/base/flag.go

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -183,7 +183,8 @@ func ParseFlags() {
183183
Debug.InlStaticInit = 1
184184
Debug.PGOInline = 1
185185
Debug.PGODevirtualize = 2
186-
Debug.SyncFrames = -1 // disable sync markers by default
186+
Debug.SyncFrames = -1 // disable sync markers by default
187+
Debug.VariableMakeThreshold = 32 // 32 byte default for stack allocated make results
187188
Debug.ZeroCopy = 1
188189
Debug.RangeFuncCheck = 1
189190
Debug.MergeLocals = 1
@@ -270,6 +271,9 @@ func ParseFlags() {
270271
if Debug.MergeLocalsHash != "" {
271272
MergeLocalsHash = NewHashDebug("mergelocals", Debug.MergeLocalsHash, nil)
272273
}
274+
if Debug.VariableMakeHash != "" {
275+
VariableMakeHash = NewHashDebug("variablemake", Debug.VariableMakeHash, nil)
276+
}
273277

274278
if Flag.MSan && !platform.MSanSupported(buildcfg.GOOS, buildcfg.GOARCH) {
275279
log.Fatalf("%s/%s does not support -msan", buildcfg.GOOS, buildcfg.GOARCH)

src/cmd/compile/internal/base/hashdebug.go

Lines changed: 5 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -53,10 +53,11 @@ func (d *HashDebug) SetInlineSuffixOnly(b bool) *HashDebug {
5353
// The default compiler-debugging HashDebug, for "-d=gossahash=..."
5454
var hashDebug *HashDebug
5555

56-
var FmaHash *HashDebug // for debugging fused-multiply-add floating point changes
57-
var LoopVarHash *HashDebug // for debugging shared/private loop variable changes
58-
var PGOHash *HashDebug // for debugging PGO optimization decisions
59-
var MergeLocalsHash *HashDebug // for debugging local stack slot merging changes
56+
var FmaHash *HashDebug // for debugging fused-multiply-add floating point changes
57+
var LoopVarHash *HashDebug // for debugging shared/private loop variable changes
58+
var PGOHash *HashDebug // for debugging PGO optimization decisions
59+
var MergeLocalsHash *HashDebug // for debugging local stack slot merging changes
60+
var VariableMakeHash *HashDebug // for debugging variable-sized make optimizations
6061

6162
// DebugHashMatchPkgFunc reports whether debug variable Gossahash
6263
//

src/cmd/compile/internal/walk/builtin.go

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -568,7 +568,8 @@ func walkMakeSlice(n *ir.MakeExpr, init *ir.Nodes) ir.Node {
568568
// The conv is necessary in case n.Type is named.
569569
return walkExpr(typecheck.Expr(typecheck.Conv(s, n.Type())), init)
570570
}
571-
tryStack = base.Flag.N == 0
571+
// Check that this optimization is enabled in general and for this node.
572+
tryStack = base.Flag.N == 0 && base.VariableMakeHash.MatchPos(n.Pos(), nil)
572573
}
573574

574575
// The final result is assigned to this variable.
@@ -582,7 +583,7 @@ func walkMakeSlice(n *ir.MakeExpr, init *ir.Nodes) ir.Node {
582583
// } else {
583584
// slice = makeslice(elemType, len, cap)
584585
// }
585-
const maxStackSize = 32
586+
maxStackSize := int64(base.Debug.VariableMakeThreshold)
586587
K := maxStackSize / t.Elem().Size() // rounds down
587588
if K > 0 { // skip if elem size is too big.
588589
nif := ir.NewIfStmt(base.Pos, ir.NewBinaryExpr(base.Pos, ir.OLE, typecheck.Conv(cap, types.Types[types.TUINT64]), ir.NewInt(base.Pos, K)), nil, nil)

0 commit comments

Comments
 (0)