From 5a6340ff28c87e099f33c941e3d73e50d715ddf7 Mon Sep 17 00:00:00 2001 From: Joel Sing Date: Thu, 25 Dec 2025 01:56:10 +1100 Subject: [PATCH 01/11] cmd/compile/internal/ssa: remove unnecessary masking with shifts on riscv64 Shift instructions on riscv64 only use the lower six bits (or the lower five bits for word based shift instructions) - this means that masking the lower bits is unnecessary and can be eliminated. Change-Id: Ic0804ebe12de1054d4d6993cfc267579ff4ae527 Reviewed-on: https://go-review.googlesource.com/c/go/+/748923 LUCI-TryBot-Result: golang-scoped@luci-project-accounts.iam.gserviceaccount.com Reviewed-by: Michael Pratt Reviewed-by: Julian Zhu Reviewed-by: David Chase --- .../compile/internal/ssa/_gen/RISCV64.rules | 8 +++ .../rewriteriscv64/rewriteRISCV64.go | 72 +++++++++++++++++++ test/codegen/bits.go | 20 +++--- 3 files changed, 90 insertions(+), 10 deletions(-) diff --git a/src/cmd/compile/internal/ssa/_gen/RISCV64.rules b/src/cmd/compile/internal/ssa/_gen/RISCV64.rules index 504c75a2ee8b8f..dca1bccde0b02c 100644 --- a/src/cmd/compile/internal/ssa/_gen/RISCV64.rules +++ b/src/cmd/compile/internal/ssa/_gen/RISCV64.rules @@ -721,6 +721,14 @@ (SRLI [x] (MOVHUreg y)) && x >= 16 => (MOVDconst [0]) (SRLI [x] (MOVWUreg y)) && x >= 32 => (MOVDconst [0]) +// Remove unnecessary masking with shifts. +(SLL x (ANDI [63] y)) => (SLL x y) +(SRL x (ANDI [63] y)) => (SRL x y) +(SRA x (ANDI [63] y)) => (SRA x y) +(SLLW x (ANDI [31] y)) => (SLLW x y) +(SRLW x (ANDI [31] y)) => (SRLW x y) +(SRAW x (ANDI [31] y)) => (SRAW x y) + // Fold constant into immediate instructions where possible. (ADD (MOVDconst [val]) x) && ssa.Is32Bit(val) && !t.IsPtr() => (ADDI [val] x) (AND (MOVDconst [val]) x) && ssa.Is32Bit(val) => (ANDI [val] x) diff --git a/src/cmd/compile/internal/ssarewrite/rewriteriscv64/rewriteRISCV64.go b/src/cmd/compile/internal/ssarewrite/rewriteriscv64/rewriteRISCV64.go index 7c6b165f51cda5..5353d78bc01702 100644 --- a/src/cmd/compile/internal/ssarewrite/rewriteriscv64/rewriteRISCV64.go +++ b/src/cmd/compile/internal/ssarewrite/rewriteriscv64/rewriteRISCV64.go @@ -8311,6 +8311,18 @@ func rewriteValue_OpRISCV64SEQZ(v *ssa.Value) bool { func rewriteValue_OpRISCV64SLL(v *ssa.Value) bool { v_1 := v.Args[1] v_0 := v.Args[0] + // match: (SLL x (ANDI [63] y)) + // result: (SLL x y) + for { + x := v_0 + if v_1.Op != ssaop.OpRISCV64ANDI || ssa.AuxIntToInt64(v_1.AuxInt) != 63 { + break + } + y := v_1.Args[0] + v.Reset(ssaop.OpRISCV64SLL) + v.AddArg2(x, y) + return true + } // match: (SLL x (MOVDconst [val])) // result: (SLLI [val&63] x) for { @@ -8384,6 +8396,18 @@ func rewriteValue_OpRISCV64SLLI(v *ssa.Value) bool { func rewriteValue_OpRISCV64SLLW(v *ssa.Value) bool { v_1 := v.Args[1] v_0 := v.Args[0] + // match: (SLLW x (ANDI [31] y)) + // result: (SLLW x y) + for { + x := v_0 + if v_1.Op != ssaop.OpRISCV64ANDI || ssa.AuxIntToInt64(v_1.AuxInt) != 31 { + break + } + y := v_1.Args[0] + v.Reset(ssaop.OpRISCV64SLLW) + v.AddArg2(x, y) + return true + } // match: (SLLW x (MOVDconst [val])) // result: (SLLIW [val&31] x) for { @@ -8637,6 +8661,18 @@ func rewriteValue_OpRISCV64SNEZ(v *ssa.Value) bool { func rewriteValue_OpRISCV64SRA(v *ssa.Value) bool { v_1 := v.Args[1] v_0 := v.Args[0] + // match: (SRA x (ANDI [63] y)) + // result: (SRA x y) + for { + x := v_0 + if v_1.Op != ssaop.OpRISCV64ANDI || ssa.AuxIntToInt64(v_1.AuxInt) != 63 { + break + } + y := v_1.Args[0] + v.Reset(ssaop.OpRISCV64SRA) + v.AddArg2(x, y) + return true + } // match: (SRA x (MOVDconst [val])) // result: (SRAI [val&63] x) for { @@ -8748,6 +8784,18 @@ func rewriteValue_OpRISCV64SRAI(v *ssa.Value) bool { func rewriteValue_OpRISCV64SRAW(v *ssa.Value) bool { v_1 := v.Args[1] v_0 := v.Args[0] + // match: (SRAW x (ANDI [31] y)) + // result: (SRAW x y) + for { + x := v_0 + if v_1.Op != ssaop.OpRISCV64ANDI || ssa.AuxIntToInt64(v_1.AuxInt) != 31 { + break + } + y := v_1.Args[0] + v.Reset(ssaop.OpRISCV64SRAW) + v.AddArg2(x, y) + return true + } // match: (SRAW x (MOVDconst [val])) // result: (SRAIW [val&31] x) for { @@ -8766,6 +8814,18 @@ func rewriteValue_OpRISCV64SRAW(v *ssa.Value) bool { func rewriteValue_OpRISCV64SRL(v *ssa.Value) bool { v_1 := v.Args[1] v_0 := v.Args[0] + // match: (SRL x (ANDI [63] y)) + // result: (SRL x y) + for { + x := v_0 + if v_1.Op != ssaop.OpRISCV64ANDI || ssa.AuxIntToInt64(v_1.AuxInt) != 63 { + break + } + y := v_1.Args[0] + v.Reset(ssaop.OpRISCV64SRL) + v.AddArg2(x, y) + return true + } // match: (SRL x (MOVDconst [val])) // result: (SRLI [val&63] x) for { @@ -8862,6 +8922,18 @@ func rewriteValue_OpRISCV64SRLI(v *ssa.Value) bool { func rewriteValue_OpRISCV64SRLW(v *ssa.Value) bool { v_1 := v.Args[1] v_0 := v.Args[0] + // match: (SRLW x (ANDI [31] y)) + // result: (SRLW x y) + for { + x := v_0 + if v_1.Op != ssaop.OpRISCV64ANDI || ssa.AuxIntToInt64(v_1.AuxInt) != 31 { + break + } + y := v_1.Args[0] + v.Reset(ssaop.OpRISCV64SRLW) + v.AddArg2(x, y) + return true + } // match: (SRLW x (MOVDconst [val])) // result: (SRLIW [val&31] x) for { diff --git a/test/codegen/bits.go b/test/codegen/bits.go index 80ccd57c005ef9..7d85c4bcd2233b 100644 --- a/test/codegen/bits.go +++ b/test/codegen/bits.go @@ -94,14 +94,14 @@ func bitsCheckVarU64(a, b uint64) (n int) { // amd64:"BTQ" // arm64:"MOVD [$]1," "LSL" "TST" // loong64:"MOVV [$]1," "SLLV R" "AND" "BNE" - // riscv64:"ANDI [$]63," "MOV [$]1," "SLL " "AND " + // riscv64:"MOV [$]1," "SLL " "AND " -"ANDI" if a&(1<<(b&63)) != 0 { return 1 } // amd64:"BTQ" -"BT. [$]0," // arm64:"LSR" "TBZ [$]0," // loong64:"SRLV" "AND [$]1," "BEQ" - // riscv64:"ANDI [$]63," "SRL " "ANDI [$]1," + // riscv64:"SRL " "ANDI [$]1," -"ANDI [$]63," if (b>>(a&63))&1 != 0 { return 1 } @@ -137,7 +137,7 @@ func bitsSetU64(a, b uint64) (n uint64) { // amd64:"BTSQ" // arm64:"MOVD [$]1," "LSL" "ORR" // loong64:"MOVV [$]1," "SLLV" "OR" - // riscv64:"ANDI [$]63," "MOV [$]1," "SLL " "OR " + // riscv64:"MOV [$]1," "SLL " "OR " -"ANDI" n += b | (1 << (a & 63)) // amd64:"BTSQ [$]63," @@ -165,7 +165,7 @@ func bitsClearU64(a, b uint64) (n uint64) { // amd64:"BTRQ" // arm64:"MOVD [$]1," "LSL" "BIC" // loong64:"MOVV [$]1," "SLLV" "ANDN" - // riscv64:"ANDI [$]63," "MOV [$]1," "SLL " "ANDN" + // riscv64:"MOV [$]1," "SLL " "ANDN" -"ANDI" n += b &^ (1 << (a & 63)) // amd64:"BTRQ [$]63," @@ -209,7 +209,7 @@ func bitsFlipU64(a, b uint64) (n uint64) { // amd64:"BTCQ" // arm64:"MOVD [$]1," "LSL" "EOR" // loong64:"MOVV [$]1," "SLLV" "XOR" - // riscv64:"ANDI [$]63," "MOV [$]1," "SLL " "XOR " + // riscv64:"MOV [$]1," "SLL " "XOR " -"ANDI" n += b ^ (1 << (a & 63)) // amd64:"BTCQ [$]63," @@ -319,14 +319,14 @@ func bitsCheckVarU32(a, b uint32) (n int) { // amd64:"BTL" // arm64:"AND [$]31," "MOVD [$]1," "LSL" "TSTW" // loong64:"MOVV [$]1," "SLL R" "AND R" "MOVWU" "BNE" - // riscv64:"ANDI [$]31," "MOV [$]1," "SLLW" "AND " + // riscv64:"MOV [$]1," "SLLW" "AND " -"ANDI" if a&(1<<(b&31)) != 0 { return 1 } // amd64:"BTL" -"BT. [$]0" // arm64:"AND [$]31," "LSR" "TBZ" // loong64:"SRL R" "AND [$]1," "BEQ" - // riscv64:"ANDI [$]31," "SRLW " "ANDI [$]1," + // riscv64:"SRLW " "ANDI [$]1," -"ANDI [$]31," if (b>>(a&31))&1 != 0 { return 1 } @@ -362,7 +362,7 @@ func bitsSetU32(a, b uint32) (n uint32) { // amd64:"BTSL" // arm64:"AND [$]31," "MOVD [$]1," "LSL" "ORR" // loong64:"MOVV [$]1," "SLL " "OR " - // riscv64:"ANDI [$]31," "MOV [$]1," "SLLW " "OR " + // riscv64:"MOV [$]1," "SLLW " "OR " -"ANDI" n += b | (1 << (a & 31)) // amd64:"ORL [$]-2147483648," @@ -390,7 +390,7 @@ func bitsClearU32(a, b uint32) (n uint32) { // amd64:"BTRL" // arm64:"AND [$]31," "MOVD [$]1," "LSL" "BIC" // loong64:"MOVV [$]1," "SLL R" "ANDN" - // riscv64:"ANDI [$]31," "MOV [$]1," "SLLW " "ANDN" + // riscv64:"MOV [$]1," "SLLW " "ANDN" -"ANDI" n += b &^ (1 << (a & 31)) // amd64:"ANDL [$]2147483647," @@ -418,7 +418,7 @@ func bitsFlipU32(a, b uint32) (n uint32) { // amd64:"BTCL" // arm64:"AND [$]31," "MOVD [$]1," "LSL" "EOR" // loong64:"MOVV [$]1," "SLL R" "XOR" - // riscv64:"ANDI [$]31," "MOV [$]1," "SLLW " "XOR " + // riscv64:"MOV [$]1," "SLLW " "XOR " -"ANDI" n += b ^ (1 << (a & 31)) // amd64:"XORL [$]-2147483648," From 98ea7651402e741f6df6a21209bd9f78a8af5760 Mon Sep 17 00:00:00 2001 From: David Chase Date: Mon, 31 Aug 2026 12:45:38 -0400 Subject: [PATCH 02/11] cmd/compile: adjust the dominance-frontier iterator This makes phi source locations more similar to how they were before CL 819540, which affects scheduling and debugging. It still changes the order in which children are visited, using the sibling links in sparse-dominators instead of the block order. (Sibling links are derived from reverse post order, which ought to be more stable in the face of minor changes, hence "better" from the POV of minimizing unintended changes/surprises in the future.) The original CL comes from Josh Bleecher Snyder and his silicon minions. Change-Id: If0d54ca26f1a0af55cbadbaaf638c6aaa5a8a914 Reviewed-on: https://go-review.googlesource.com/c/go/+/825184 Reviewed-by: Keith Randall Reviewed-by: Keith Randall LUCI-TryBot-Result: golang-scoped@luci-project-accounts.iam.gserviceaccount.com --- src/cmd/compile/internal/ssa/dfplus_iter.go | 17 ++++++------- .../compile/internal/ssa/dfplus_iter_test.go | 24 ++++++++++++++++++- src/cmd/compile/internal/ssagen/phi.go | 4 ++-- 3 files changed, 34 insertions(+), 11 deletions(-) diff --git a/src/cmd/compile/internal/ssa/dfplus_iter.go b/src/cmd/compile/internal/ssa/dfplus_iter.go index 025cce6347182e..8784d6bc253584 100644 --- a/src/cmd/compile/internal/ssa/dfplus_iter.go +++ b/src/cmd/compile/internal/ssa/dfplus_iter.go @@ -18,16 +18,17 @@ import ( // IterDomFrontierPlus iterates the DF+ of seeds: every block at which // a phi may need to be placed if a variable were defined in the seed -// blocks. Blocks are yielded at most once, in a deterministic order; -// an early break stops the walk. Seed blocks themselves are not -// yielded as such, but a seed that is also a merge point (e.g. a loop -// header) is. +// blocks. For each frontier block, it also yields the block whose +// outgoing edge discovered the frontier. Frontier blocks are yielded +// at most once, in a deterministic order; an early break stops the walk. +// Seed blocks themselves are not yielded as such, but a seed that is +// also a merge point (e.g. a loop header) is. // seeds iterator is consumed in full before the walk starts (the current // algorithm has to walk deeper roots first). // CFG must not change while iteration is in progress; inserting // values (like phis) is fine. -func (f *Func) IterDomFrontierPlus(seeds iter.Seq[*Block]) iter.Seq[*Block] { - return func(yield func(*Block) bool) { +func (f *Func) IterDomFrontierPlus(seeds iter.Seq[*Block]) iter.Seq2[*Block, *Block] { + return func(yield func(*Block, *Block) bool) { // Materialize the seeds into a pooled slice reused by walkDFPlus. s := f.Cache.AllocBlockSlice(f.NumBlocks())[:0] defer f.Cache.FreeBlockSlice(s[:cap(s)]) @@ -61,7 +62,7 @@ const ( // plus the frontier found, and memory is O(f.NumBlocks()). The walk reads // the CFG's edges and uses the cached dominator tree. // The seeds slice is reused in place by the PiggyBank. -func (f *Func) walkDFPlus(seeds []*Block, yield func(*Block) bool) { +func (f *Func) walkDFPlus(seeds []*Block, yield func(*Block, *Block) bool) { sdom := f.Sdom() // Roots to process, deepest first. @@ -120,7 +121,7 @@ func (f *Func) walkDFPlus(seeds []*Block, yield func(*Block) bool) { flags[c.ID] |= flagPiggyBanked heap.Push(&piggyBank, c) } - if !yield(c) { + if !yield(c, b) { return } } diff --git a/src/cmd/compile/internal/ssa/dfplus_iter_test.go b/src/cmd/compile/internal/ssa/dfplus_iter_test.go index 8189df088137f9..439551cb4493d0 100644 --- a/src/cmd/compile/internal/ssa/dfplus_iter_test.go +++ b/src/cmd/compile/internal/ssa/dfplus_iter_test.go @@ -118,7 +118,29 @@ func TestIterDomFrontierPlusSeedAtMerge(t *testing.T) { } } -func collectBlockIDs(seq iter.Seq[*Block]) []ID { +func TestIterDomFrontierPlusOrigin(t *testing.T) { + f := (&Config{}).NewFunc(nil, &Cache{}) + entry := f.NewBlock(block.BlockIf) + f.Entry = entry + left := f.NewBlock(block.BlockPlain) + right := f.NewBlock(block.BlockPlain) + merge := f.NewBlock(block.BlockExit) + + entry.AddEdgeTo(left) + entry.AddEdgeTo(right) + left.AddEdgeTo(merge) + right.AddEdgeTo(merge) + + var got [][2]*Block + for b, origin := range f.IterDomFrontierPlus(slices.Values([]*Block{left})) { + got = append(got, [2]*Block{b, origin}) + } + if want := [][2]*Block{{merge, left}}; !slices.Equal(got, want) { + t.Fatalf("got frontier and origin %v, want %v", got, want) + } +} + +func collectBlockIDs(seq iter.Seq2[*Block, *Block]) []ID { var ids []ID for b := range seq { ids = append(ids, b.ID) diff --git a/src/cmd/compile/internal/ssagen/phi.go b/src/cmd/compile/internal/ssagen/phi.go index 6721f7df7d82b9..45e42bd03a276b 100644 --- a/src/cmd/compile/internal/ssagen/phi.go +++ b/src/cmd/compile/internal/ssagen/phi.go @@ -145,9 +145,9 @@ func (s *phiState) insertPhis() { func (s *phiState) insertVarPhis(n int, var_ ir.Node, defs []*ssa.Block, typ *types.Type) { // Iterate DF+ of the defining blocks. - for c := range s.f.IterDomFrontierPlus(slices.Values(defs)) { + for c, b := range s.f.IterDomFrontierPlus(slices.Values(defs)) { // Add a phi to block c for variable n. - v := c.NewValue0I(s.s.blockStarts[c.ID], ssaop.OpPhi, typ, int64(n)) + v := c.NewValue0I(s.s.blockStarts[b.ID], ssaop.OpPhi, typ, int64(n)) // Note: we store the variable number in the phi's AuxInt field. Used temporarily by phi building. if var_.Op() == ir.ONAME { s.s.addNamedValue(var_.(*ir.Name), v) From 859c99e8cf4ef700c6cc2d3951f81aa304f9a81b Mon Sep 17 00:00:00 2001 From: Keith Randall Date: Mon, 3 Aug 2026 10:31:01 -0700 Subject: [PATCH 03/11] cmd/compile: add test for 80534 It was fixed by CL 804320, just adding the regression test. Fixes #80534 Change-Id: Id18bdd9eb84f91d0dc7db1545e913ab8284d6333 Reviewed-on: https://go-review.googlesource.com/c/go/+/809660 LUCI-TryBot-Result: golang-scoped@luci-project-accounts.iam.gserviceaccount.com Reviewed-by: Carlos Amedee Auto-Submit: Keith Randall Reviewed-by: Keith Randall --- test/fixedbugs/issue80534.go | 15 +++++++++++++++ 1 file changed, 15 insertions(+) create mode 100644 test/fixedbugs/issue80534.go diff --git a/test/fixedbugs/issue80534.go b/test/fixedbugs/issue80534.go new file mode 100644 index 00000000000000..a1fddf607e57c0 --- /dev/null +++ b/test/fixedbugs/issue80534.go @@ -0,0 +1,15 @@ +// compile + +// 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 + +var x [3][3]int + +func main() { + for i := range 3 { + x[i][i] = 0 + } +} From d8af7eb741df245a289f2b096d85e7417ed0b8b9 Mon Sep 17 00:00:00 2001 From: cuishuang Date: Fri, 14 Aug 2026 23:59:12 +0800 Subject: [PATCH 04/11] math/big: preserve divisor when Int.Divide remainder aliases it Int.Divide preserves y.abs for rounding adjustments, but currently copies it only when the quotient aliases the divisor. When the remainder aliases the divisor, nat.div may overwrite y.abs before the Floor, Ceil, or Round adjustment uses it. This can produce an incorrect remainder. Copy the divisor magnitude when either output aliases it, and add tests for remainder-divisor aliasing. Fixes #80882 Change-Id: Ifce2d3fef94b955d43d35bd015cd3e2e74645e35 Reviewed-on: https://go-review.googlesource.com/c/go/+/814621 LUCI-TryBot-Result: golang-scoped@luci-project-accounts.iam.gserviceaccount.com Reviewed-by: David Chase Reviewed-by: Robert Griesemer Auto-Submit: Robert Griesemer --- src/math/big/int.go | 2 +- src/math/big/int_test.go | 50 ++++++++++++++++++++++++++++++++++++++++ 2 files changed, 51 insertions(+), 1 deletion(-) diff --git a/src/math/big/int.go b/src/math/big/int.go index e2630c000deb84..68a178582f1221 100644 --- a/src/math/big/int.go +++ b/src/math/big/int.go @@ -403,7 +403,7 @@ func (z *Int) Divide(x, y, r *Int, mode RoundingMode) (*Int, *Int) { r_abs = r.abs } y_abs := y.abs // save y - if z == y || alias(z_abs, y.abs) { + if z == y || r == y || alias(z_abs, y.abs) || alias(r_abs, y.abs) { y_abs = nat(nil).set(y.abs) } neg := x.neg != y.neg diff --git a/src/math/big/int_test.go b/src/math/big/int_test.go index 4b5261c3de6401..6339336371f444 100644 --- a/src/math/big/int_test.go +++ b/src/math/big/int_test.go @@ -573,6 +573,56 @@ func TestIntDivide(t *testing.T) { } } +func TestIntDivideRemainderAliasingDivisor(t *testing.T) { + tests := []struct { + name string + x, y, q, r int64 + mode RoundingMode + }{ + {"trunc", 5, 3, 1, 2, Trunc}, + {"ceil", 5, 3, 2, -1, Ceil}, + {"floor", -5, 3, -2, 1, Floor}, + {"round", 2, 3, 1, -1, Round}, + } + + for _, test := range tests { + for _, scaleString := range []string{"1", "12345678901234567890"} { + t.Run(test.name+"/scale="+scaleString, func(t *testing.T) { + scale, ok := new(Int).SetString(scaleString, 10) + if !ok { + t.Fatal("invalid test scale") + } + x := new(Int).Mul(NewInt(test.x), scale) + y := new(Int).Mul(NewInt(test.y), scale) + wantQ := NewInt(test.q) + wantR := new(Int).Mul(NewInt(test.r), scale) + + gotQ, gotR := new(Int).Divide(x, y, y, test.mode) + if gotQ.Cmp(wantQ) != 0 || gotR.Cmp(wantR) != 0 { + t.Fatalf("Divide(%v, %v, y, %v) = (%v, %v); want (%v, %v)", x, test.y, test.mode, gotQ, gotR, wantQ, wantR) + } + + y.Mul(NewInt(test.y), scale) + _, gotR = (*Int)(nil).Divide(x, y, y, test.mode) + if gotR.Cmp(wantR) != 0 { + t.Fatalf("Divide(%v, %v, y, %v) with nil quotient returned remainder %v; want %v", x, test.y, test.mode, gotR, wantR) + } + }) + } + } + + t.Run("shared backing array", func(t *testing.T) { + x := NewInt(5) + y := NewInt(3) + r := new(Int).SetBits(y.Bits()) + + gotQ, gotR := new(Int).Divide(x, y, r, Ceil) + if gotQ.Cmp(NewInt(2)) != 0 || gotR.Cmp(NewInt(-1)) != 0 { + t.Fatalf("Divide(5, 3, r, Ceil) = (%v, %v); want (2, -1)", gotQ, gotR) + } + }) +} + var bitLenTests = []struct { in string out int From fbea197d327913903d2db36ede886e81d78277e6 Mon Sep 17 00:00:00 2001 From: Junyang Shao Date: Mon, 31 Aug 2026 17:09:42 -0400 Subject: [PATCH 05/11] cmd/compile: don't let no-code SIMD values imply CPU features The cpufeatures pass treated any SIMD-typed value in a block as implying the presence of the CPU feature its type needs ("a fault is a fault"). That reasoning is wrong for values that emit no machine code: in particular, a SIMD-typed zero is materialized as a reference to the fixed all-zeros register X15 and flows freely through code that must run on machines without AVX. Since rematerializable values are placed in the entry block, a single inlined SIMD zero could mark an entire function as AVX-capable. Under GOEXPERIMENT=simd this happened to the map fast paths: the inlined AES hash (guarded only by a runtime flag the pass cannot see) put a SIMD zero in the entry block, so every block of mapaccess claimed AVX, including the unconditionally executed legacy-SSE group-match code. This is latent until block features gain consumers that change instruction encodings. Skip no-code values (SIMD zeros, phis, copies, args, SelectN) when computing block feature effects. Functions whose signatures mention SIMD types still claim features for their entry block; that is a deliberate contract, not an inference from generated code. The loop-header expectation removed from test/simd.go relied on the old behavior: that block holds only rematerializable SIMD zeros and emits no vector instruction. For #80835 Change-Id: Iec0ab0cf2f1df6c31951b6c36d1f5ae64653531e Reviewed-on: https://go-review.googlesource.com/c/go/+/825185 Reviewed-by: David Chase LUCI-TryBot-Result: golang-scoped@luci-project-accounts.iam.gserviceaccount.com Reviewed-by: Junyang Shao Auto-Submit: Junyang Shao --- .../internal/ssacompile/cpufeatures.go | 20 ++++++++++++ test/simd.go | 32 ++++++++++++++++++- 2 files changed, 51 insertions(+), 1 deletion(-) diff --git a/src/cmd/compile/internal/ssacompile/cpufeatures.go b/src/cmd/compile/internal/ssacompile/cpufeatures.go index fd01675dadd92f..cccac88e6063bd 100644 --- a/src/cmd/compile/internal/ssacompile/cpufeatures.go +++ b/src/cmd/compile/internal/ssacompile/cpufeatures.go @@ -116,6 +116,17 @@ func ifEffect(b *ssa.Block) (features ssa.CPUfeatures, taken int) { return } +// noCodeValue reports whether v is known to emit no machine code, so +// that even a SIMD type does not imply the presence of any CPU feature. +func noCodeValue(v *ssa.Value) bool { + switch v.Op { + case ssaop.OpZeroSIMD, ssaop.OpPhi, ssaop.OpCopy, ssaop.OpSelectN, + ssaop.OpArg, ssaop.OpArgIntReg, ssaop.OpArgFloatReg: + return true + } + return false +} + func cpufeatures(f *ssa.Func) { arch := f.Config.Ctxt.Arch.Family // TODO there are other SIMD architectures @@ -183,6 +194,15 @@ func cpufeatures(f *ssa.Func) { // instruction that would fault if the feature (avx, avx512) // were not present, then assume that the feature is present // for all the instructions in the block, a fault is a fault. + if noCodeValue(v) { + // v emits no instruction, so its type implies + // nothing about the CPU. In particular a + // SIMD-typed zero is just a reference to the + // fixed all-zeros register and flows freely + // through code that must run on machines + // without AVX. + continue + } t := v.Type if t.IsResults() { for i := 0; i < t.NumFields(); i++ { diff --git a/test/simd.go b/test/simd.go index 05cc24f3a9c2a3..9d44055979190a 100644 --- a/test/simd.go +++ b/test/simd.go @@ -150,10 +150,40 @@ func ternTricky3(x, y, z archsimd.Int32x8) archsimd.Int32x8 { func vpternlogdPanic() { resultsMask := archsimd.Mask64x8{} - for { // ERROR "has features avx[+]avx2[+]avx512" + // The loop header holds only rematerializable SIMD zeros, which emit + // no code, so it claims no features. + for { resultsMask = archsimd.Mask64x8FromBits(0).Or( // ERROR "has features avx[+]avx2[+]avx512" archsimd.Float64x8{}.Less( archsimd.BroadcastFloat64x8(0))).Or(resultsMask) // ERROR "Rewriting.*ternInt" "Skipping rewrite" fmt.Print(resultsMask.And(resultsMask.And(archsimd.Mask64x8{}))) } } + +type notSIMD struct { + v archsimd.Int8x16 + n int +} + +var cond bool + +// A SIMD-typed zero emits no code (it is just a reference to the fixed +// all-zeros register), so it must imply no CPU features: this function +// must stay compilable for machines without AVX and produce no feature +// diagnostics at all. +func zeroOnly(p *notSIMD) { + p.v = archsimd.Int8x16{} + p.n++ +} + +// Only the block executing a real SIMD instruction claims features; +// they do not leak into the unconditionally executed parts of the +// function through the no-code zero or the merge. +func zeroMerge(p *notSIMD, s []int8) { + x := archsimd.Int8x16{} + if cond { + x = archsimd.LoadInt8x16(s) // ERROR "has features avx$" + } + p.v = x + p.n++ +} From 0b2fd4aa9c0884db7d90e2278762beb5c1cb6686 Mon Sep 17 00:00:00 2001 From: Junyang Shao Date: Mon, 31 Aug 2026 17:09:52 -0400 Subject: [PATCH 06/11] cmd/compile: give critical-edge split blocks the CPU features of their edge Blocks created by the critical pass start with no CPU features, so later consumers of block features (e.g. the encoding of regalloc-inserted shuffle copies placed in split blocks) had no facts to work with there. CPU features are execution-invariant facts. A split block executes only after its predecessor and unconditionally proceeds to its successor, so the features of both hold in it; a split block reused for several predecessor edges keeps only what all of them guarantee. The features given to split blocks are reported at -d=ssa/critical/debug=1, and test/simd_critical.go exercises all three paths through the pass: fresh split blocks for edges into single-phi and multi-phi merge blocks, and the reuse of a split block for several edges carrying the same phi argument. For #80835 Change-Id: Ibcf0a15bc47260abbff2b3b3e9fbcc24f0dc3bd1 Reviewed-on: https://go-review.googlesource.com/c/go/+/825186 Reviewed-by: David Chase Reviewed-by: Junyang Shao Auto-Submit: Junyang Shao LUCI-TryBot-Result: golang-scoped@luci-project-accounts.iam.gserviceaccount.com --- .../compile/internal/ssacompile/critical.go | 19 ++++++ test/simd_critical.go | 66 +++++++++++++++++++ 2 files changed, 85 insertions(+) create mode 100644 test/simd_critical.go diff --git a/src/cmd/compile/internal/ssacompile/critical.go b/src/cmd/compile/internal/ssacompile/critical.go index f4c3157e470fac..c700392d99c3ba 100644 --- a/src/cmd/compile/internal/ssacompile/critical.go +++ b/src/cmd/compile/internal/ssacompile/critical.go @@ -68,20 +68,39 @@ func critical(f *ssa.Func) { // the new blocks to be re-examined. d = f.NewBlock(block.BlockPlain) d.Pos = p.Pos + // CPU features are execution-invariant facts: d + // executes only after p and unconditionally jumps + // to b, so both blocks' features hold in d. + d.CPUfeatures = p.CPUfeatures | b.CPUfeatures blocks[argID] = d if f.Pass.Debug > 0 { f.Warnl(p.Pos, "split critical edge") + if d.CPUfeatures != ssa.CPUNone { + f.Warnl(p.Pos, "split-edge block b%d has features %v", d.ID, d.CPUfeatures) + } } } else { reusedBlock = true + // d gains another predecessor, so only the + // features common to all of its predecessors + // (plus b's) are still guaranteed. + d.CPUfeatures &= p.CPUfeatures | b.CPUfeatures + if f.Pass.Debug > 0 && d.CPUfeatures != ssa.CPUNone { + f.Warnl(p.Pos, "reused split-edge block b%d has features %v", d.ID, d.CPUfeatures) + } } } else { // no existing block, so allocate a new block // to place on the edge d = f.NewBlock(block.BlockPlain) d.Pos = p.Pos + // See above for why d inherits these features. + d.CPUfeatures = p.CPUfeatures | b.CPUfeatures if f.Pass.Debug > 0 { f.Warnl(p.Pos, "split critical edge") + if d.CPUfeatures != ssa.CPUNone { + f.Warnl(p.Pos, "split-edge block b%d has features %v", d.ID, d.CPUfeatures) + } } } diff --git a/test/simd_critical.go b/test/simd_critical.go new file mode 100644 index 00000000000000..5f1163f6351418 --- /dev/null +++ b/test/simd_critical.go @@ -0,0 +1,66 @@ +// errorcheck -0 -d=ssa/critical/debug=1 + +//go:build goexperiment.simd && amd64 + +// 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. + +// Test that blocks created by the critical pass to split critical +// edges inherit the CPU features of the edge they sit on, so that +// later consumers (e.g. regalloc-inserted shuffle copies) can use +// feature-dependent instruction encodings there. There are three +// paths through the pass, each exercised below: +// +// 1. a fresh split block for an edge into a block with a single phi +// 2. a fresh split block for an edge into a block with several phis +// (or none) +// 3. a split block reused for several predecessor edges carrying the +// same phi argument, which keeps only the features all of its +// predecessors guarantee + +package foo + +import "simd/archsimd" + +var cond bool + +// Case 1: the merge block has a single phi (x), so the split block for +// the critical edge is created on the single-phi path. It inherits +// avx from its predecessor and successor. +func singlePhi(a, b archsimd.Int64x4) archsimd.Int64x4 { + x := a.Add(b) + if cond { // ERROR "split critical edge" "split-edge block b[0-9]+ has features avx$" + x = x.Add(a) + } + return x +} + +// Case 2: the merge block has two phis (x and y), so the split block +// for the critical edge is created on the no-single-phi path. +func multiPhi(a, b archsimd.Int64x4) (archsimd.Int64x4, archsimd.Int64x4) { + x := a.Add(b) + y := b.Sub(a) + if cond { // ERROR "split critical edge" "split-edge block b[0-9]+ has features avx$" + x = x.Add(a) + y = y.Sub(b) + } + return x, y +} + +// Case 3: the short-circuit && evaluates each operand in its own +// block, and both false edges jump to the single-phi merge block with +// the same phi argument (the zero value of x). The split block is +// created for the edge from the second operand's block, whose features +// are avx+avx2+avx512 (it is dominated by the first operand's block +// and holds the 512-bit ops); when it is reused for the edge from the +// first operand's block, which only guarantees avx, its features must +// drop to the common subset rather than keep avx512. +func reuseIntersect(s []int64, s8 []int64) { + var x archsimd.Int64x4 + if archsimd.LoadInt64x4(s).IsZero() && // ERROR "split critical edge" "reused split-edge block b[0-9]+ has features avx$" "split-edge block b[0-9]+ has features avx$" + archsimd.LoadInt64x8(s8).Equal(archsimd.LoadInt64x8(s8)).ToBits() != 0 { // ERROR "split critical edge" "split-edge block b[0-9]+ has features avx[+]avx2[+]avx512$" + x = archsimd.LoadInt64x4(s) + } + x.Store(s) +} From 0d6e66c45503f559f4e09c4e9c5ff77fa020169e Mon Sep 17 00:00:00 2001 From: Brad Fitzpatrick Date: Tue, 1 Sep 2026 02:02:25 +0000 Subject: [PATCH 07/11] net/http/internal/http2: don't use a per-Server error channel pool The move of HTTP/2 into std made errChanPool a field on http2.Server so that pooled channels wouldn't be reused across synctest bubbles. But that regressed memory for servers using the ServeConn-per-conn pattern (HTTP/2 over hijacked or tunneled conns): every sync.Pool re-pins after each GC, allocating a GOMAXPROCS-sized poolLocal array (4 KB at GOMAXPROCS=32) per pool, and a per-conn pool provides no reuse anyway. On a production proxy with ~400k conns, the poolLocal arrays accounted for ~2 GB, ~10% of heap. Instead, go back to a single global pool and skip pooling entirely when the current goroutine is in a synctest bubble. Change-Id: I8a8df547507b185b7943ab8d227da8262acf8a49 Reviewed-on: https://go-review.googlesource.com/c/go/+/825424 LUCI-TryBot-Result: golang-scoped@luci-project-accounts.iam.gserviceaccount.com Reviewed-by: Damien Neil Reviewed-by: Nicholas Husin Reviewed-by: Nicholas Husin --- src/net/http/internal/http2/server.go | 44 +++++++++++++-------------- 1 file changed, 22 insertions(+), 22 deletions(-) diff --git a/src/net/http/internal/http2/server.go b/src/net/http/internal/http2/server.go index c6d27d96146cca..addfc1dd24ffa3 100644 --- a/src/net/http/internal/http2/server.go +++ b/src/net/http/internal/http2/server.go @@ -33,6 +33,7 @@ import ( "crypto/tls" "errors" "fmt" + "internal/synctest" "io" "log" "math" @@ -103,10 +104,6 @@ var ( type Server struct { mu sync.Mutex activeConns map[*serverConn]struct{} - - // Pool of error channels. This is per-Server rather than global - // because channels can't be reused across synctest bubbles. - errChanPool sync.Pool } func (s *Server) registerConn(sc *serverConn) { @@ -138,30 +135,33 @@ func (s *Server) startGracefulShutdown() { s.mu.Unlock() } -// Global error channel pool used for uninitialized Servers. -// We use a per-Server pool when possible to avoid using channels across synctest bubbles. +// errChanPool is a pool of reusable channels for reporting the result +// of a blocking frame write. +// +// The pool is not used inside synctest bubbles, since a channel created +// in one bubble can't be used from another bubble or from outside a +// bubble, and sync.Pool is not bubble-aware. var errChanPool = sync.Pool{ New: func() any { return make(chan error, 1) }, } -func (s *Server) getErrChan() chan error { - if s == nil { - return errChanPool.Get().(chan error) // Server used without calling ConfigureServer +func getErrChan() chan error { + if synctest.IsInBubble() { + // Channels can't be shared across synctest bubbles. + // Skip the pool; allocation cost is irrelevant in tests. + return make(chan error, 1) } - return s.errChanPool.Get().(chan error) + return errChanPool.Get().(chan error) } -func (s *Server) putErrChan(ch chan error) { - if s == nil { - errChanPool.Put(ch) // Server used without calling ConfigureServer - return +func putErrChan(ch chan error) { + if !synctest.IsInBubble() { + errChanPool.Put(ch) } - s.errChanPool.Put(ch) } func (s *Server) Configure(conf ServerConfig, tcfg *tls.Config) error { s.activeConns = make(map[*serverConn]struct{}) - s.errChanPool = sync.Pool{New: func() any { return make(chan error, 1) }} if tcfg.CipherSuites != nil && tcfg.MinVersion < tls.VersionTLS13 { // If they already provided a TLS 1.0–1.2 CipherSuite list, return an @@ -1009,7 +1009,7 @@ var writeDataPool = sync.Pool{ // writeDataFromHandler writes DATA response frames from a handler on // the given stream. func (sc *serverConn) writeDataFromHandler(stream *stream, data []byte, endStream bool) error { - ch := sc.srv.getErrChan() + ch := getErrChan() writeArg := writeDataPool.Get().(*writeData) *writeArg = writeData{stream.id, data, endStream} err := sc.writeFrameFromHandler(FrameWriteRequest{ @@ -1041,7 +1041,7 @@ func (sc *serverConn) writeDataFromHandler(stream *stream, data []byte, endStrea return errStreamClosed } } - sc.srv.putErrChan(ch) + putErrChan(ch) if frameWriteDone { writeDataPool.Put(writeArg) } @@ -2353,7 +2353,7 @@ func (sc *serverConn) writeHeaders(st *stream, headerData *writeResHeaders) erro // waiting for this frame to be written, so an http.Flush mid-handler // writes out the correct value of keys, before a handler later potentially // mutates it. - errc = sc.srv.getErrChan() + errc = getErrChan() } if err := sc.writeFrameFromHandler(FrameWriteRequest{ write: headerData, @@ -2365,7 +2365,7 @@ func (sc *serverConn) writeHeaders(st *stream, headerData *writeResHeaders) erro if errc != nil { select { case err := <-errc: - sc.srv.putErrChan(errc) + putErrChan(errc) return err case <-sc.doneServing: return errClientDisconnected @@ -3063,7 +3063,7 @@ func (w *responseWriter) Push(target, method string, header Header) error { method: method, url: u, header: cloneHeader(header), - done: sc.srv.getErrChan(), + done: getErrChan(), } select { @@ -3080,7 +3080,7 @@ func (w *responseWriter) Push(target, method string, header Header) error { case <-st.cw: return errStreamClosed case err := <-msg.done: - sc.srv.putErrChan(msg.done) + putErrChan(msg.done) return err } } From 50d7989e5a45c413ac82ab3dc8e22dae3cfded78 Mon Sep 17 00:00:00 2001 From: qmuntal Date: Tue, 1 Sep 2026 15:24:48 +0200 Subject: [PATCH 08/11] cmd/go: use configured C compiler in cgo_path_space_quote test The test selected clang whenever it was present on PATH, even when CC was configured to gcc. On Windows builders where clang lacked the required C headers, this caused go run to fail. Use the cc condition to select the configured compiler, keeping the test focused on handling quotes and spaces in CC. Change-Id: Ib4f7288f16966ce605f10e1326158a91ef27b8a1 Reviewed-on: https://go-review.googlesource.com/c/go/+/825644 Reviewed-by: Michael Matloob Reviewed-by: Dmitri Shuralyov LUCI-TryBot-Result: golang-scoped@luci-project-accounts.iam.gserviceaccount.com Reviewed-by: Michael Matloob --- src/cmd/go/testdata/script/cgo_path_space_quote.txt | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/cmd/go/testdata/script/cgo_path_space_quote.txt b/src/cmd/go/testdata/script/cgo_path_space_quote.txt index b3448e71f755f6..2dcafeeec1e527 100644 --- a/src/cmd/go/testdata/script/cgo_path_space_quote.txt +++ b/src/cmd/go/testdata/script/cgo_path_space_quote.txt @@ -4,7 +4,7 @@ # with single or double quotes. This is the same as -gcflags and similar # options. -[!exec:clang] [!exec:gcc] skip +[!cc:clang] [!cc:gcc] skip [!cgo] skip [GOARCH:ppc64le] skip 'broken on ppc64le: #81174' [GOARCH:ppc64] skip 'broken on ppc64: #81174' @@ -12,8 +12,8 @@ env GOENV=$WORK/go.env mkdir 'program files' go build -o 'program files' './which cc/which cc.go' -[exec:clang] env CC='"'$PWD${/}program' 'files${/}which' 'cc"' 'clang -[!exec:clang] env CC='"'$PWD${/}program' 'files${/}which' 'cc"' 'gcc +[cc:clang] env CC='"'$PWD${/}program' 'files${/}which' 'cc"' 'clang +[cc:gcc] env CC='"'$PWD${/}program' 'files${/}which' 'cc"' 'gcc go env CC stdout 'program files[/\\]which cc" (clang|gcc)$' go env -w CC=$CC From 91c531e00e2a3aaf3b61cb939e2a9662e1a811dd Mon Sep 17 00:00:00 2001 From: Russ Cox Date: Wed, 26 Aug 2026 14:32:46 -0400 Subject: [PATCH 09/11] runtime: tolerate transient skew in the not-in-go metric test /sched/goroutines/not-in-go is documented as an approximate count, and a single read of it can legitimately come up one short. While a third party is partway through taking a P away from a goroutine in a cgo call, that goroutine is briefly counted in neither half of the runtime's accounting: it is no longer reachable through its P, and sched.nGsyscallNoP has not been incremented for it yet. See go.dev/issue/78877 for the analysis. Read the metric until it settles instead of trusting one sample. The accounting bugs this program exists to catch, including the negative nGsyscallNoP of go.dev/issue/76435, make the count wrong and keep it wrong, so they never settle and still fail here. Also fix the failure reporting. failed was declared false and never assigned, so a bad reading exited 0 and the program only failed because the stray println perturbed the output runTestProg compares against. Measured on linux/amd64 against an unpatched runtime: 5 of 150000 runs of NotInGoMetricCgoCallAndCallback saw the short reading, and every one of them settled after a single 1ms retry. 250000 runs of NotInGoMetricCgoCallAndCallback and 150000 each of NotInGoMetricCgoCall and NotInGoMetricCgoCallback then passed with no failures, against a baseline of 5 and 2 failures respectively. Updates #78877. Change-Id: I8165f8731c6d04503e8c49b809b0cf696a6a6964 Reviewed-on: https://go-review.googlesource.com/c/go/+/822584 LUCI-TryBot-Result: golang-scoped@luci-project-accounts.iam.gserviceaccount.com Reviewed-by: David Chase Reviewed-by: Michael Pratt --- src/runtime/testdata/testprogcgo/notingo.go | 57 +++++++++++++-------- 1 file changed, 35 insertions(+), 22 deletions(-) diff --git a/src/runtime/testdata/testprogcgo/notingo.go b/src/runtime/testdata/testprogcgo/notingo.go index a385ae24d6fb34..7b68d686803ce8 100644 --- a/src/runtime/testdata/testprogcgo/notingo.go +++ b/src/runtime/testdata/testprogcgo/notingo.go @@ -64,8 +64,36 @@ import ( "runtime" "runtime/metrics" "sync/atomic" + "time" ) +// waitNotInGo waits for /sched/goroutines/not-in-go to read want, and reports +// whether it got there. +// +// A single read of the metric can legitimately disagree with want. It's +// documented as an approximate count, and while some other thread is partway +// through taking a P away from a goroutine in a cgo call, that goroutine is +// briefly counted in neither half of the runtime's accounting, so the reading +// comes up one short. The skew lasts only as long as the handoff, so reading +// again converges. See go.dev/issue/78877. +// +// The accounting bugs this program exists to catch make the count wrong and +// keep it wrong, which never converges and still fails here. +func waitNotInGo(what string, want uint64) bool { + s := []metrics.Sample{{Name: "/sched/goroutines/not-in-go:goroutines"}} + var n uint64 + for start := time.Now(); time.Since(start) < 5*time.Second; { + metrics.Read(s) + n = s[0].Value.Uint64() + if n == want { + return true + } + time.Sleep(time.Millisecond) + } + println(what, "expected", want, "not-in-go goroutines, found", n) + return false +} + func init() { register("NotInGoMetricCgoCall", NotInGoMetricCgoCall) register("NotInGoMetricCgoCallback", NotInGoMetricCgoCallback) @@ -88,12 +116,7 @@ func NotInGoMetricCgoCall() { } // Read not-in-go before taking the Ps back. - s := []metrics.Sample{{Name: "/sched/goroutines/not-in-go:goroutines"}} - failed := false - metrics.Read(s) - if n := s[0].Value.Uint64(); n != N { - println("pre-STW: expected", N, "not-in-go goroutines, found", n) - } + failed := !waitNotInGo("pre-STW:", N) // Do something that stops the world to take all the Ps back. // @@ -102,9 +125,8 @@ func NotInGoMetricCgoCall() { runtime.ReadMemStats(&m) // Read not-in-go. - metrics.Read(s) - if n := s[0].Value.Uint64(); n != N { - println("post-STW: expected", N, "not-in-go goroutines, found", n) + if !waitNotInGo("post-STW:", N) { + failed = true } // Fail if we get a bad reading. @@ -153,10 +175,7 @@ func NotInGoMetricCgoCallback() { } // Read not-in-go. - s := []metrics.Sample{{Name: "/sched/goroutines/not-in-go:goroutines"}} - metrics.Read(s) - if n := s[0].Value.Uint64(); n != 0 { - println("expected 0 not-in-go goroutines, found", n) + if !waitNotInGo("after-callbacks:", 0) { os.Exit(2) } println("OK") @@ -202,12 +221,7 @@ func NotInGoMetricCgoCallAndCallback() { } // Read not-in-go before taking the Ps back. - s := []metrics.Sample{{Name: "/sched/goroutines/not-in-go:goroutines"}} - failed := false - metrics.Read(s) - if n := s[0].Value.Uint64(); n != N { - println("pre-STW: expected", N, "not-in-go goroutines, found", n) - } + failed := !waitNotInGo("pre-STW:", N) // Do something that stops the world to take all the Ps back. // @@ -216,9 +230,8 @@ func NotInGoMetricCgoCallAndCallback() { runtime.ReadMemStats(&m) // Read not-in-go. - metrics.Read(s) - if n := s[0].Value.Uint64(); n != N { - println("post-STW: expected", N, "not-in-go goroutines, found", n) + if !waitNotInGo("post-STW:", N) { + failed = true } // Fail if we get a bad reading. From 4397bae26b852bb78e80d8b0712631865414d65f Mon Sep 17 00:00:00 2001 From: qmuntal Date: Tue, 1 Sep 2026 16:31:24 +0200 Subject: [PATCH 10/11] cmd/go: isolate goauth_git from system credential helpers TestScript/goauth_git sets up a file-backed credential helper for the test, but Git treats credential.helper as multi-valued. As a result, helpers from system Git configuration can still be invoked. This showed up in one test run as a 349-second timeout in the authenticated go get case. At the deadline, one git credential fill and two asynchronous git credential approve commands were still blocked. Clear the inherited helper list before adding the test helper. Besides making the test self-contained, this keeps its credentials out of system keychains. On a Windows host with a system credential manager, the test also dropped from 15.38 seconds to 2.74 seconds. Change-Id: I488beaf09c4df7d4a67fe5c2b3307acb29ec088c Reviewed-on: https://go-review.googlesource.com/c/go/+/825664 Reviewed-by: Michael Matloob LUCI-TryBot-Result: golang-scoped@luci-project-accounts.iam.gserviceaccount.com Reviewed-by: Dmitri Shuralyov Reviewed-by: Michael Matloob Reviewed-by: Dmitri Shuralyov --- src/cmd/go/testdata/script/goauth_git.txt | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/cmd/go/testdata/script/goauth_git.txt b/src/cmd/go/testdata/script/goauth_git.txt index 94b31b4543e45b..8ec66ec1428486 100644 --- a/src/cmd/go/testdata/script/goauth_git.txt +++ b/src/cmd/go/testdata/script/goauth_git.txt @@ -8,7 +8,9 @@ env GOSUMDB=off # Disable 'git credential fill' interactive prompts. env GIT_TERMINAL_PROMPT=0 exec git init -exec git config credential.helper 'store --file=.git-credentials' +# Ignore credential helpers inherited from system configuration. +exec git config credential.helper '' +exec git config --add credential.helper 'store --file=.git-credentials' cp go.mod.orig go.mod # Set GOAUTH to git without a working directory. From 1dacc626582f0d5daf85b6d5149150a5d3f07815 Mon Sep 17 00:00:00 2001 From: Michael Matloob Date: Wed, 19 Aug 2026 10:40:11 -0400 Subject: [PATCH 11/11] cmd/go/internal/work: separate export and text content ids in buildids This change hashes the export and object data separately, so that we can more granularly determine whether a build action (only needing the export data) or a link action (depending on the object data) should be run. Fixes #15752 Change-Id: Ie61dcb96cc7efb53653ffb392dee60f96a6a6964 Reviewed-on: https://go-review.googlesource.com/c/go/+/818461 Reviewed-by: Michael Matloob Reviewed-by: Austin Clements LUCI-TryBot-Result: golang-scoped@luci-project-accounts.iam.gserviceaccount.com Auto-Submit: Michael Matloob --- src/cmd/go/go_test.go | 14 +-- src/cmd/go/internal/work/action.go | 4 +- src/cmd/go/internal/work/buildid.go | 123 +++++++++++++++------- src/cmd/go/internal/work/exec.go | 22 ++-- src/cmd/go/internal/work/gc.go | 6 +- src/cmd/go/testdata/script/cover_list.txt | 22 ++-- 6 files changed, 126 insertions(+), 65 deletions(-) diff --git a/src/cmd/go/go_test.go b/src/cmd/go/go_test.go index 5632b224d801f5..4caea619a35a73 100644 --- a/src/cmd/go/go_test.go +++ b/src/cmd/go/go_test.go @@ -2228,19 +2228,19 @@ func TestTestCache(t *testing.T) { // Changing the actual package should have limited effects. tg.tempFile("src/p1/p1.go", "package p1\nvar X = 02\n") tg.run("test", "-p=1", "-x", "-v", "-short", "t/...") - - // p2 should have been rebuilt. - tg.grepStderr(`([\\/]compile|gccgo).*p2.go`, "did not recompile p2") + // p2 should not have been rebuilt. + tg.grepStderrNot(`([\\/]compile|gccgo).*p2.go`, "incorrectly recompiled p2") // t1 does not import anything, should not have been rebuilt. tg.grepStderrNot(`([\\/]compile|gccgo).*t1_test.go`, "incorrectly recompiled t1") tg.grepStderrNot(`([\\/]link|gccgo).*t1_test`, "incorrectly relinked t1_test") tg.grepStdout(`ok \tt/t1\t\(cached\)`, "did not cache t/t1") - // t2 imports p1 and must be rebuilt and relinked, - // but the change should not have any effect on the test binary, + // t2 imports p1 and it must not be rebuilt because p1's export data + // didn't change but must be relinked because p1's object data did. + // The change should not have any effect on the test binary, // so the test should not have been rerun. - tg.grepStderr(`([\\/]compile|gccgo).*t2_test.go`, "did not recompile t2") + tg.grepStderrNot(`([\\/]compile|gccgo).*t2_test.go`, "incorrectly recompiled t2") tg.grepStderr(`([\\/]link|gccgo).*t2\.test`, "did not relink t2_test") // This check does not currently work with gccgo, as garbage // collection of unused variables is not turned on by default. @@ -2249,7 +2249,7 @@ func TestTestCache(t *testing.T) { } // t3 imports p1, and changing X changes t3's test binary. - tg.grepStderr(`([\\/]compile|gccgo).*t3_test.go`, "did not recompile t3") + tg.grepStderrNot(`([\\/]compile|gccgo).*t3_test.go`, "incorrectly recompiled t3") tg.grepStderr(`([\\/]link|gccgo).*t3\.test`, "did not relink t3_test") tg.grepStderr(`t3\.test.*-test.short`, "did not rerun t3_test") tg.grepStdoutNot(`ok \tt/t3\t\(cached\)`, "reported cached t3_test result") diff --git a/src/cmd/go/internal/work/action.go b/src/cmd/go/internal/work/action.go index 65e4d324d3a4d4..9c7dacb76e7659 100644 --- a/src/cmd/go/internal/work/action.go +++ b/src/cmd/go/internal/work/action.go @@ -128,10 +128,10 @@ type Action struct { } // BuildActionID returns the action ID section of a's build ID. -func (a *Action) BuildActionID() string { return actionID(a.buildID) } +func (a *Action) BuildActionID() string { return buildActionID(a.buildID) } // BuildContentID returns the content ID section of a's build ID. -func (a *Action) BuildContentID() string { return contentID(a.buildID) } +func (a *Action) BuildContentID() string { return buildObjectID(a.buildID) } // BuildID returns a's build ID. func (a *Action) BuildID() string { return a.buildID } diff --git a/src/cmd/go/internal/work/buildid.go b/src/cmd/go/internal/work/buildid.go index 573a6da38867fa..8c517703c27fc4 100644 --- a/src/cmd/go/internal/work/buildid.go +++ b/src/cmd/go/internal/work/buildid.go @@ -6,7 +6,9 @@ package work import ( "bytes" + "cmd/internal/archive" "fmt" + "io" "os" "os/exec" "strings" @@ -27,37 +29,41 @@ import ( // // Go packages and binaries are stamped with build IDs that record both // the action ID, which is a hash of the inputs to the action that produced -// the packages or binary, and the content ID, which is a hash of the action -// output, namely the archive or binary itself. The hash is the same one +// the packages or binary, and their content IDs which are hashes of the +// action outputs. The content IDs are hashes of the export and object files +// in the case of gc package builds, or of the entire archive or binary, in the case +// of gccgo builds or link actions. These hashes are the same // used by the build artifact cache (see cmd/go/internal/cache), but // truncated when stored in packages and binaries, as the full length is not -// needed and is a bit unwieldy. The precise form is +// needed and is a bit unwieldy. The precise forms are // -// actionID/[.../]contentID +// actionID/contentID(export)/contentID(object) +// actionID(link)/actionID(build)/contentID(build object)/contentID(link) // -// where the actionID and contentID are prepared by buildid.HashToString below. +// where the first form is used for build actions and the second form is used +// for link actions. The actionID and contentID are prepared by buildid.HashToString below. // and are found by looking for the first or last slash. -// Usually the buildID is simply actionID/contentID, but see below for an -// exception. +// gccgo actions use the same gccgo output for the export and object +// content ids because they do not have separate export data files. // // The build ID serves two primary purposes. // -// 1. The action ID half allows installed packages and binaries to serve as +// 1. The action ID part allows installed packages and binaries to serve as // one-element cache entries. If we intend to build math.a with a given // set of inputs summarized in the action ID, and the installed math.a already // has that action ID, we can reuse the installed math.a instead of rebuilding it. // -// 2. The content ID half allows the easy preparation of action IDs for steps -// that consume a particular package or binary. The content hash of every -// input file for a given action must be included in the action ID hash. -// Storing the content ID in the build ID lets us read it from the file with -// minimal I/O, instead of reading and hashing the entire file. -// This is especially effective since packages and binaries are typically +// 2. The content ID parts allow the easy preparation of action IDs for steps +// that consume a particular package's export or object files or its binary. +// The content hash of every input file for a given action must be included +// in the action ID hash. Storing the content IDs in the build ID lets us read +// it from the file with minimal I/O, instead of reading and hashing the entire +// file. This is especially effective since packages and binaries are typically // the largest inputs to an action. // -// Separating action ID from content ID is important for reproducible builds. +// Separating action ID from content IDs is important for reproducible builds. // The compiler is compiled with itself. If an output were represented by its -// own action ID (instead of content ID) when computing the action ID of +// own action ID (instead of content IDs) when computing the action ID of // the next step in the build process, then the compiler could never have its // own input action ID as its output action ID (short of a miraculous hash collision). // Instead we use the content IDs to compute the next action ID, and because @@ -75,20 +81,19 @@ import ( // means knowing the content ID of main.a, which we did not keep. // To sidestep this problem, each binary actually stores an expanded build ID: // -// actionID(binary)/actionID(main.a)/contentID(main.a)/contentID(binary) +// actionID(binary)/actionID(main.a)/contentID(main.a object)/contentID(binary) // -// (Note that this can be viewed equivalently as: -// -// actionID(binary)/buildID(main.a)/contentID(binary) -// -// Storing the buildID(main.a) in the middle lets the computations that care -// about the prefix or suffix halves ignore the middle and preserves the -// original build ID as a contiguous string.) +// where contentID(main.a object) only includes the hash of main.a's object +// files but not its export data. +// Storing the action and object content ids of the build action in the middle +// lets the computations that care about the prefix or suffix halves ignore the middle, +// while keeping the action and output info needed to check that the build +// action does not need to be rerun. // // During the build, when it's time to build main.a, the gofmt binary has the // information needed to decide whether the eventual link would produce // the same binary: if the action ID for main.a's inputs matches and then -// the action ID for the link step matches when assuming the given main.a +// the action ID for the link step matches when assuming the given main.a's object's // content ID, then the binary as a whole is up-to-date and need not be rebuilt. // // This is all a bit complex and may be simplified once we can rely on the @@ -98,8 +103,8 @@ import ( const buildIDSeparator = "/" -// actionID returns the action ID half of a build ID. -func actionID(buildID string) string { +// buildActionID returns the action ID part of a build ID. +func buildActionID(buildID string) string { i := strings.Index(buildID, buildIDSeparator) if i < 0 { return buildID @@ -107,11 +112,20 @@ func actionID(buildID string) string { return buildID[:i] } -// contentID returns the content ID half of a build ID. -func contentID(buildID string) string { +// buildObjectID returns the content ID for the object data. +func buildObjectID(buildID string) string { return buildID[strings.LastIndex(buildID, buildIDSeparator)+1:] } +// buildExportID returns the content ID for the export data. +func buildExportID(buildID string) string { + if buildID == "" { + return "" + } + chopContent := buildID[:strings.LastIndex(buildID, buildIDSeparator)] + return chopContent[strings.LastIndex(chopContent, buildIDSeparator)+1:] +} + // toolID returns the unique ID to use for the current copy of the // named tool (asm, compile, cover, link). // @@ -175,7 +189,7 @@ func (b *Builder) toolID(name string) string { } if strings.Contains(f[2], "devel") { // On the development branch, use the content ID part of the build ID. - return contentID(f[len(f)-1]) + return buildObjectID(f[len(f)-1]) } // For a release, the output is like: "compile version go1.9.1 X:framepointer". // Use the whole line. @@ -464,13 +478,13 @@ func (b *Builder) useCache(a *Action, actionHash cache.ActionID, target string, a.json.ActionID = actionID } contentID := actionID // temporary placeholder, likely unique - a.buildID = actionID + buildIDSeparator + contentID + a.buildID = actionID + buildIDSeparator + contentID + buildIDSeparator + contentID - // Executable binaries also record the main build ID in the middle. + // Executable binaries also record the action and object content id of the build id in the middle. // See "Build IDs" comment above. if a.Mode == "link" { mainpkg := a.Deps[0] - a.buildID = actionID + buildIDSeparator + mainpkg.buildID + buildIDSeparator + contentID + a.buildID = actionID + buildIDSeparator + buildActionID(mainpkg.buildID) + buildIDSeparator + buildObjectID(mainpkg.buildID) + buildIDSeparator + contentID } // If user requested -a, we force a rebuild, so don't use the cache. @@ -530,7 +544,7 @@ func (b *Builder) useCache(a *Action, actionHash cache.ActionID, target string, // other than a.buildID, b.linkActionID is only accessing // build IDs of completed actions. oldBuildID := a.buildID - a.buildID = id[1] + buildIDSeparator + id[2] + a.buildID = id[1] + buildIDSeparator + id[2] + buildIDSeparator + id[2] linkID := buildid.HashToString(b.linkActionID(a.triggers[0])) if id[0] == linkID { // Best effort attempt to display output from the compile and link steps. @@ -707,17 +721,52 @@ func (b *Builder) updateBuildID(a *Action, target string) error { } } + var matches []int64 + var exportHash [32]byte + var objectOffset int64 // where to start hashing the object data from // Find occurrences of old ID and compute new content-based ID. r, err := os.Open(target) if err != nil { return err } - matches, hash, err := buildid.FindAndHash(r, a.buildID, 0) - r.Close() + // Hash export id if this is an archive with an export data file. + if v, err := archive.Parse(r, false); err == nil && len(v.Entries) > 0 && v.Entries[0].Type == archive.EntryPkgDef { + pkgEntry := v.Entries[0] + exportMatches, contentHash, err := buildid.FindAndHash(io.NewSectionReader(r, pkgEntry.Offset, pkgEntry.Size), a.buildID, 0) + if err != nil { + r.Close() + return err + } + exportHash = contentHash + for _, m := range exportMatches { + matches = append(matches, pkgEntry.Offset+m) + } + objectOffset = pkgEntry.Offset + pkgEntry.Size + } + if _, err := r.Seek(objectOffset, io.SeekStart); err != nil { + r.Close() + return err + } + objectMatches, objectHash, err := buildid.FindAndHash(r, a.buildID, 0) if err != nil { return err } - newID := a.buildID[:strings.LastIndex(a.buildID, buildIDSeparator)] + buildIDSeparator + buildid.HashToString(hash) + for _, m := range objectMatches { + matches = append(matches, objectOffset+m) + } + if err := r.Close(); err != nil { + return err + } + + var newID string + if a.Mode == "build" { + if exportHash == [32]byte{} { + exportHash = objectHash // gccgo does not have export data + } + newID = buildActionID(a.buildID) + buildIDSeparator + buildid.HashToString(exportHash) + buildIDSeparator + buildid.HashToString(objectHash) + } else { + newID = a.buildID[:strings.LastIndex(a.buildID, buildIDSeparator)] + buildIDSeparator + buildid.HashToString(objectHash) + } if len(newID) != len(a.buildID) { return fmt.Errorf("internal error: build ID length mismatch %q vs %q", a.buildID, newID) } diff --git a/src/cmd/go/internal/work/exec.go b/src/cmd/go/internal/work/exec.go index e61d891d6cfebe..c711a906202495 100644 --- a/src/cmd/go/internal/work/exec.go +++ b/src/cmd/go/internal/work/exec.go @@ -373,7 +373,7 @@ func (b *Builder) buildActionID(a *Action) cache.ActionID { for _, a1 := range a.Deps { p1 := a1.Package if p1 != nil && p1 != p { // p can show up in its own action deps in a cache or cgo action - fmt.Fprintf(h, "import %s %s\n", p1.ImportPath, contentID(a1.buildID)) + fmt.Fprintf(h, "import %s %s\n", p1.ImportPath, buildExportID(a1.buildID)) } if a1.Mode == "preprocess PGO profile" { fmt.Fprintf(h, "pgofile %s\n", b.fileHash(a1.built)) @@ -1868,7 +1868,7 @@ func (b *Builder) exportActionID(a *Action, ecfg *exportConfig) cache.ActionID { } // Any dependencies. for _, dep := range a.Deps { - fmt.Fprintf(h, "packageFile %s=%s\n", dep.Package.ImportPath, contentID(dep.buildID)) + fmt.Fprintf(h, "packageFile %s=%s\n", dep.Package.ImportPath, buildExportID(dep.buildID)) } return cache.ActionID(h.Sum()) } @@ -1915,15 +1915,17 @@ func (b *Builder) linkActionID(a *Action) cache.ActionID { if buildID == "" { buildID = b.buildID(a1.built) } - fmt.Fprintf(h, "packagefile %s=%s\n", p1.ImportPath, contentID(buildID)) + fmt.Fprintf(h, "packagefile %s=%s\n", p1.ImportPath, buildObjectID(buildID)) } - // Because we put package main's full action ID into the binary's build ID, - // we must also put the full action ID into the binary's action ID hash. + // Because we put package main's action ID and object data content ID into the binary's build ID, + // we must also put the action ID and object data content ID into the binary's action ID hash. + // We only put in the object data content ID, which was hashed from everything other than the export data, + // and not the export data content ID, because the export data is not given to the linker. if p1.Name == "main" { - fmt.Fprintf(h, "packagemain %s\n", a1.buildID) + fmt.Fprintf(h, "packagemain %s\n", buildActionID(a1.buildID)+buildIDSeparator+buildObjectID(a1.buildID)) } if p1.Shlib != "" { - fmt.Fprintf(h, "packageshlib %s=%s\n", p1.ImportPath, contentID(b.buildID(p1.Shlib))) + fmt.Fprintf(h, "packageshlib %s=%s\n", p1.ImportPath, buildObjectID(b.buildID(p1.Shlib))) } } } @@ -2248,16 +2250,16 @@ func (b *Builder) linkSharedActionID(a *Action) cache.ActionID { continue } if p1 != nil { - fmt.Fprintf(h, "packagefile %s=%s\n", p1.ImportPath, contentID(b.buildID(a1.built))) + fmt.Fprintf(h, "packagefile %s=%s\n", p1.ImportPath, buildObjectID(b.buildID(a1.built))) if p1.Shlib != "" { - fmt.Fprintf(h, "packageshlib %s=%s\n", p1.ImportPath, contentID(b.buildID(p1.Shlib))) + fmt.Fprintf(h, "packageshlib %s=%s\n", p1.ImportPath, buildObjectID(b.buildID(p1.Shlib))) } } } // Files named on command line are special. for _, a1 := range a.Deps[0].Deps { p1 := a1.Package - fmt.Fprintf(h, "top %s=%s\n", p1.ImportPath, contentID(b.buildID(a1.built))) + fmt.Fprintf(h, "top %s=%s\n", p1.ImportPath, buildObjectID(b.buildID(a1.built))) } return h.Sum() diff --git a/src/cmd/go/internal/work/gc.go b/src/cmd/go/internal/work/gc.go index fa60d533bd1cde..ec198b6469dee2 100644 --- a/src/cmd/go/internal/work/gc.go +++ b/src/cmd/go/internal/work/gc.go @@ -583,14 +583,14 @@ func pluginPath(a *Action) string { // For linking, use the main package's build ID instead of // the binary's build ID, so it is the same hash used in // compiling and linking. - // When compiling, we use actionID/actionID (instead of - // actionID/contentID) as a temporary build ID to compute + // When compiling, we use actionID/actionID/actionID, instead of + // actionID/contentID(export)/contentID(object), as a temporary build ID to compute // the hash. Do the same here. (See buildid.go:useCache) // The build ID matters because it affects the overall hash // in the plugin's pseudo-import path returned below. // We need to use the same import path when compiling and linking. id := strings.Split(buildID, buildIDSeparator) - buildID = id[1] + buildIDSeparator + id[1] + buildID = id[1] + buildIDSeparator + id[1] + buildIDSeparator + id[1] } fmt.Fprintf(h, "build ID: %s\n", buildID) for _, file := range str.StringList(p.GoFiles, p.CgoFiles, p.SFiles) { diff --git a/src/cmd/go/testdata/script/cover_list.txt b/src/cmd/go/testdata/script/cover_list.txt index 7d723490f6c0a1..f7694576d841a3 100644 --- a/src/cmd/go/testdata/script/cover_list.txt +++ b/src/cmd/go/testdata/script/cover_list.txt @@ -15,7 +15,7 @@ stale -cover m/example # Collect build ID from for m/example built with -cover. go list -cover -export -f '{{.BuildID}}' m/example -cp stdout $WORK/listbuildid.txt +cp stdout $WORK/rawlistbuildid.txt # Now build the m/example binary with coverage. go build -cover -o $WORK/m.exe m/example @@ -26,9 +26,11 @@ cp stdout $WORK/rawtoolbuildid.txt # Make sure that the two build IDs agree with respect to the # m/example package. Build IDs from binaries are of the form X/Y/Z/W -# where Y/Z is the package build ID; running the program below will -# pick out the parts of the ID that we want. +# where Y is the package action ID and Z is the package's object's content ID. +# Running the program below will pick out the parts of the ID that we want. env GOCOVERDIR=$WORK +exec $WORK/m.exe $WORK/rawlistbuildid.txt +cp stdout $WORK/listbuildid.txt exec $WORK/m.exe $WORK/rawtoolbuildid.txt cp stdout $WORK/toolbuildid.txt @@ -59,8 +61,16 @@ func main() { os.Exit(1) } fields := strings.Split(strings.TrimSpace(string(content)), "/") - if len(fields) != 4 { - os.Exit(2) + switch len(fields) { + case 3: + // We're reading a build action id X/Y/Z: return X/Z, the + // part that will be embedded in the link's build id. + fmt.Println(fields[0] + "/" + fields[2]) + case 4: + // We're reading a link action id X/Y/Z/W: return Y/Z the part + // derived from the main package's build action. + fmt.Println(fields[1] + "/" + fields[2]) + default: + os.Exit(2) } - fmt.Println(fields[1] + "/" + fields[2]) }