From da5a3146ec903cdcb779d501be4ff88fd775820e Mon Sep 17 00:00:00 2001 From: Russ Cox Date: Mon, 8 May 2023 23:18:38 -0400 Subject: [PATCH] cmd/compile: use more of internal/bisect in HashDebug Using more of internal/bisect gives us more that will be deleted from base/hashdebug.go when we have updated the tools that need the old protocol. It is also cheaper: there is no allocation to make a decision about whether to enable, and no locking unless printing is needed. Change-Id: I43ec398461205a1a9e988512a134ed6b3a3b1587 Reviewed-on: https://go-review.googlesource.com/c/go/+/493736 Run-TryBot: Russ Cox TryBot-Result: Gopher Robot Reviewed-by: David Chase --- src/cmd/compile/internal/base/hashdebug.go | 198 ++++++++---------- .../compile/internal/base/hashdebug_test.go | 110 ++++------ src/cmd/compile/internal/loopvar/loopvar.go | 4 +- .../compile/internal/loopvar/loopvar_test.go | 2 +- src/cmd/compile/internal/ssa/func.go | 5 +- 5 files changed, 132 insertions(+), 187 deletions(-) diff --git a/src/cmd/compile/internal/base/hashdebug.go b/src/cmd/compile/internal/base/hashdebug.go index 5492d9cda2..46adaaacd7 100644 --- a/src/cmd/compile/internal/base/hashdebug.go +++ b/src/cmd/compile/internal/base/hashdebug.go @@ -6,7 +6,6 @@ package base import ( "bytes" - "cmd/internal/notsha256" "cmd/internal/obj" "cmd/internal/src" "fmt" @@ -19,11 +18,6 @@ import ( "sync" ) -type writeSyncer interface { - io.Writer - Sync() error -} - type hashAndMask struct { // a hash h matches if (h^hash)&mask == 0 hash uint64 @@ -36,7 +30,7 @@ type HashDebug struct { name string // base name of the flag/variable. // what file (if any) receives the yes/no logging? // default is os.Stdout - logfile writeSyncer + logfile io.Writer posTmp []src.Pos bytesTmp bytes.Buffer matches []hashAndMask // A hash matches if one of these matches. @@ -70,7 +64,7 @@ var hashDebug *HashDebug var FmaHash *HashDebug // for debugging fused-multiply-add floating point changes var LoopVarHash *HashDebug // for debugging shared/private loop variable changes -// DebugHashMatch reports whether debug variable Gossahash +// DebugHashMatchPkgFunc reports whether debug variable Gossahash // // 1. is empty (returns true; this is a special more-quickly implemented case of 4 below) // @@ -99,7 +93,7 @@ var LoopVarHash *HashDebug // for debugging shared/private loop variable changes // // Otherwise it returns false. // -// Unless Flags.Gossahash is empty, when DebugHashMatch returns true the message +// Unless Flags.Gossahash is empty, when DebugHashMatchPkgFunc returns true the message // // "%s triggered %s\n", varname, pkgAndName // @@ -135,12 +129,12 @@ var LoopVarHash *HashDebug // for debugging shared/private loop variable changes // // 6. gossahash should return a single function whose miscompilation // causes the problem, and you can focus on that. -func DebugHashMatch(pkgAndName string) bool { - return hashDebug.DebugHashMatch(pkgAndName) +func DebugHashMatchPkgFunc(pkg, fn string) bool { + return hashDebug.MatchPkgFunc(pkg, fn) } func DebugHashMatchPos(pos src.XPos) bool { - return hashDebug.DebugHashMatchPos(pos) + return hashDebug.MatchPos(pos) } // HasDebugHash returns true if Flags.Gossahash is non-empty, which @@ -150,6 +144,7 @@ func HasDebugHash() bool { return hashDebug != nil } +// TODO: Delete when we switch to bisect-only. func toHashAndMask(s, varname string) hashAndMask { l := len(s) if l > 64 { @@ -168,7 +163,7 @@ func toHashAndMask(s, varname string) hashAndMask { // NewHashDebug returns a new hash-debug tester for the // environment variable ev. If ev is not set, it returns // nil, allowing a lightweight check for normal-case behavior. -func NewHashDebug(ev, s string, file writeSyncer) *HashDebug { +func NewHashDebug(ev, s string, file io.Writer) *HashDebug { if s == "" { return nil } @@ -182,6 +177,8 @@ func NewHashDebug(ev, s string, file writeSyncer) *HashDebug { hd.bisect = m return hd } + + // TODO: Delete remainder of function when we switch to bisect-only. ss := strings.Split(s, "/") // first remove any leading exclusions; these are preceded with "-" i := 0 @@ -217,43 +214,7 @@ func NewHashDebug(ev, s string, file writeSyncer) *HashDebug { } -func hashOf(pkgAndName string, param uint64) uint64 { - return hashOfBytes([]byte(pkgAndName), param) -} - -func hashOfBytes(sbytes []byte, param uint64) uint64 { - hbytes := notsha256.Sum256(sbytes) - hash := uint64(hbytes[7])<<56 + uint64(hbytes[6])<<48 + - uint64(hbytes[5])<<40 + uint64(hbytes[4])<<32 + - uint64(hbytes[3])<<24 + uint64(hbytes[2])<<16 + - uint64(hbytes[1])<<8 + uint64(hbytes[0]) - - if param != 0 { - // Because param is probably a line number, probably near zero, - // hash it up a little bit, but even so only the lower-order bits - // likely matter because search focuses on those. - p0 := param + uint64(hbytes[9]) + uint64(hbytes[10])<<8 + - uint64(hbytes[11])<<16 + uint64(hbytes[12])<<24 - - p1 := param + uint64(hbytes[13]) + uint64(hbytes[14])<<8 + - uint64(hbytes[15])<<16 + uint64(hbytes[16])<<24 - - param += p0 * p1 - param ^= param>>17 ^ param<<47 - } - - return hash ^ param -} - -// DebugHashMatch returns true if either the variable used to create d is -// unset, or if its value is y, or if it is a suffix of the base-two -// representation of the hash of pkgAndName. If the variable is not nil, -// then a true result is accompanied by stylized output to d.logfile, which -// is used for automated bug search. -func (d *HashDebug) DebugHashMatch(pkgAndName string) bool { - return d.DebugHashMatchParam(pkgAndName, 0) -} - +// TODO: Delete when we switch to bisect-only. func (d *HashDebug) excluded(hash uint64) bool { for _, m := range d.excludes { if (m.hash^hash)&m.mask == 0 { @@ -263,6 +224,7 @@ func (d *HashDebug) excluded(hash uint64) bool { return false } +// TODO: Delete when we switch to bisect-only. func hashString(hash uint64) string { hstr := "" if hash == 0 { @@ -272,9 +234,13 @@ func hashString(hash uint64) string { hstr = string('0'+byte(hash&1)) + hstr } } + if len(hstr) > 24 { + hstr = hstr[len(hstr)-24:] + } return hstr } +// TODO: Delete when we switch to bisect-only. func (d *HashDebug) match(hash uint64) *hashAndMask { for i, m := range d.matches { if (m.hash^hash)&m.mask == 0 { @@ -284,102 +250,118 @@ func (d *HashDebug) match(hash uint64) *hashAndMask { return nil } -// DebugHashMatchParam returns true if either the variable used to create d is +// MatchPkgFunc returns true if either the variable used to create d is // unset, or if its value is y, or if it is a suffix of the base-two -// representation of the hash of pkgAndName and param. If the variable is not -// nil, then a true result is accompanied by stylized output to d.logfile, -// which is used for automated bug search. -func (d *HashDebug) DebugHashMatchParam(pkgAndName string, param uint64) bool { +// representation of the hash of pkg and fn. If the variable is not nil, +// then a true result is accompanied by stylized output to d.logfile, which +// is used for automated bug search. +func (d *HashDebug) MatchPkgFunc(pkg, fn string) bool { if d == nil { return true } + // Written this way to make inlining likely. + return d.matchPkgFunc(pkg, fn) +} - hash := hashOf(pkgAndName, param) +func (d *HashDebug) matchPkgFunc(pkg, fn string) bool { + hash := bisect.Hash(pkg, fn) if d.bisect != nil { if d.bisect.ShouldPrint(hash) { - d.logDebugHashMatch(d.name, pkgAndName, hash, param) + d.log(d.name, hash, pkg+"."+fn) } return d.bisect.ShouldEnable(hash) } + + // TODO: Delete rest of function body when we switch to bisect-only. if m := d.match(hash); m != nil { - d.logDebugHashMatch(m.name, pkgAndName, hash, param) + d.log(m.name, hash, pkg+"."+fn) return true } return false } -// DebugHashMatchPos is similar to DebugHashMatchParam, but for hash computation +// MatchPos is similar to MatchPkgFunc, but for hash computation // it uses the source position including all inlining information instead of -// package name and path. The mutex locking is more frequent and more granular. +// package name and path. // Note that the default answer for no environment variable (d == nil) // is "yes", do the thing. -func (d *HashDebug) DebugHashMatchPos(pos src.XPos) bool { +func (d *HashDebug) MatchPos(pos src.XPos) bool { if d == nil { return true } // Written this way to make inlining likely. - return d.debugHashMatchPos(Ctxt, pos) + return d.matchPos(Ctxt, pos) } -func (d *HashDebug) debugHashMatchPos(ctxt *obj.Link, pos src.XPos) bool { - // TODO: When we remove the old d.match code, we can use - // d.bisect.Hash instead of the locked buffer, and we can - // use d.bisect.MarkerOnly to decide whether to format a string. - d.mu.Lock() - defer d.mu.Unlock() +func (d *HashDebug) matchPos(ctxt *obj.Link, pos src.XPos) bool { + hash := d.hashPos(ctxt, pos) - b := d.bytesForPos(ctxt, pos) - hash := hashOfBytes(b, 0) if d.bisect != nil { if d.bisect.ShouldPrint(hash) { - d.logDebugHashMatchLocked(d.name, string(b), hash, 0) + d.log(d.name, hash, d.fmtPos(ctxt, pos)) } return d.bisect.ShouldEnable(hash) } + // TODO: Delete rest of function body when we switch to bisect-only. + // Return false for explicitly excluded hashes if d.excluded(hash) { return false } if m := d.match(hash); m != nil { - d.logDebugHashMatchLocked(m.name, string(b), hash, 0) + d.log(m.name, hash, d.fmtPos(ctxt, pos)) return true } return false } -// bytesForPos renders a position, including inlining, into d.bytesTmp -// and returns the byte array. d.mu must be locked. -func (d *HashDebug) bytesForPos(ctxt *obj.Link, pos src.XPos) []byte { - b := &d.bytesTmp - b.Reset() - format := func(p src.Pos) { - f := p.Filename() - if d.fileSuffixOnly { - f = filepath.Base(f) - } - fmt.Fprintf(b, "%s:%d:%d", f, p.Line(), p.Col()) +// short returns the form of file name to use for d. +// The default is the full path, but fileSuffixOnly selects +// just the final path element. +func (d *HashDebug) short(name string) string { + if d.fileSuffixOnly { + return filepath.Base(name) } + return name +} + +// hashPos returns a hash of the position pos, including its entire inline stack. +// If d.inlineSuffixOnly is true, hashPos only considers the innermost (leaf) position on the inline stack. +func (d *HashDebug) hashPos(ctxt *obj.Link, pos src.XPos) uint64 { if d.inlineSuffixOnly { - format(ctxt.InnermostPos(pos)) - } else { - ctxt.AllPos(pos, func(p src.Pos) { - if b.Len() > 0 { - b.WriteByte(';') - } - format(p) - }) + p := ctxt.InnermostPos(pos) + return bisect.Hash(d.short(p.Filename()), p.Line(), p.Col()) + } + h := bisect.Hash() + ctxt.AllPos(pos, func(p src.Pos) { + h = bisect.Hash(h, d.short(p.Filename()), p.Line(), p.Col()) + }) + return h +} + +// fmtPos returns a textual formatting of the position pos, including its entire inline stack. +// If d.inlineSuffixOnly is true, fmtPos only considers the innermost (leaf) position on the inline stack. +func (d *HashDebug) fmtPos(ctxt *obj.Link, pos src.XPos) string { + format := func(p src.Pos) string { + return fmt.Sprintf("%s:%d:%d", d.short(p.Filename()), p.Line(), p.Col()) + } + if d.inlineSuffixOnly { + return format(ctxt.InnermostPos(pos)) } - return b.Bytes() + var stk []string + ctxt.AllPos(pos, func(p src.Pos) { + stk = append(stk, format(p)) + }) + return strings.Join(stk, "; ") } -func (d *HashDebug) logDebugHashMatch(varname, name string, hash, param uint64) { +// log prints a match with the given hash and textual formatting. +// TODO: Delete varname parameter when we switch to bisect-only. +func (d *HashDebug) log(varname string, hash uint64, text string) { d.mu.Lock() defer d.mu.Unlock() - d.logDebugHashMatchLocked(varname, name, hash, param) -} -func (d *HashDebug) logDebugHashMatchLocked(varname, name string, hash, param uint64) { file := d.logfile if file == nil { if tmpfile := os.Getenv("GSHS_LOGFILE"); tmpfile != "" { @@ -395,21 +377,11 @@ func (d *HashDebug) logDebugHashMatchLocked(varname, name string, hash, param ui } d.logfile = file } - hstr := hashString(hash) - if len(hstr) > 24 { - hstr = hstr[len(hstr)-24:] - } - // External tools depend on this string - if param == 0 { - fmt.Fprintf(file, "%s triggered %s %s\n", varname, name, hstr) - } else { - fmt.Fprintf(file, "%s triggered %s:%d %s\n", varname, name, param, hstr) - } - // Print new bisect version too. - if param == 0 { - fmt.Fprintf(file, "%s %s\n", name, bisect.Marker(hash)) - } else { - fmt.Fprintf(file, "%s:%d %s\n", name, param, bisect.Marker(hash)) - } - file.Sync() + + // Bisect output. + fmt.Fprintf(file, "%s %s\n", text, bisect.Marker(hash)) + + // Gossahash output. + // TODO: Delete rest of function when we switch to bisect-only. + fmt.Fprintf(file, "%s triggered %s %s\n", varname, text, hashString(hash)) } diff --git a/src/cmd/compile/internal/base/hashdebug_test.go b/src/cmd/compile/internal/base/hashdebug_test.go index 0b83712532..e6ced4d148 100644 --- a/src/cmd/compile/internal/base/hashdebug_test.go +++ b/src/cmd/compile/internal/base/hashdebug_test.go @@ -6,26 +6,27 @@ package base import ( "bytes" + "internal/bisect" "strings" "testing" ) func TestHashDebugGossahashY(t *testing.T) { - hd := NewHashDebug("GOSSAHASH", "y", new(bufferWithSync)) + hd := NewHashDebug("GOSSAHASH", "y", new(bytes.Buffer)) if hd == nil { t.Errorf("NewHashDebug should not return nil for GOSSASHASH=y") } - if !hd.DebugHashMatch("anything") { + if !hd.MatchPkgFunc("anything", "anyfunc") { t.Errorf("NewHashDebug should return yes for everything for GOSSASHASH=y") } } func TestHashDebugGossahashN(t *testing.T) { - hd := NewHashDebug("GOSSAHASH", "n", new(bufferWithSync)) + hd := NewHashDebug("GOSSAHASH", "n", new(bytes.Buffer)) if hd == nil { t.Errorf("NewHashDebug should not return nil for GOSSASHASH=n") } - if hd.DebugHashMatch("anything") { + if hd.MatchPkgFunc("anything", "anyfunc") { t.Errorf("NewHashDebug should return no for everything for GOSSASHASH=n") } } @@ -49,74 +50,61 @@ func TestHashDebugMagic(t *testing.T) { } func TestHash(t *testing.T) { - h0 := hashOf("bar", 0) - h1 := hashOf("bar", 1) - t.Logf(`These values are used in other tests: hashOf("bar,0)"=0x%x, hashOf("bar,1)"=0x%x`, h0, h1) + h0 := bisect.Hash("bar", "0") + h1 := bisect.Hash("bar", "1") + t.Logf(`These values are used in other tests: Hash("bar", "0")=%#64b, Hash("bar", "1")=%#64b`, h0, h1) if h0 == h1 { t.Errorf("Hashes 0x%x and 0x%x should differ", h0, h1) } } func TestHashMatch(t *testing.T) { - ws := new(bufferWithSync) - hd := NewHashDebug("GOSSAHASH", "0011", ws) - check := hd.DebugHashMatch("bar") - msg := ws.String() + b := new(bytes.Buffer) + hd := NewHashDebug("GOSSAHASH", "1110", b) + check := hd.MatchPkgFunc("bar", "0") + msg := b.String() t.Logf("message was '%s'", msg) if !check { - t.Errorf("GOSSAHASH=0011 should have matched for 'bar'") + t.Errorf("GOSSAHASH=1110 should have matched for 'bar', '0'") } - wantPrefix(t, msg, "GOSSAHASH triggered bar ") - wantContains(t, msg, "\nbar [bisect-match ") -} - -func TestHashMatchParam(t *testing.T) { - ws := new(bufferWithSync) - hd := NewHashDebug("GOSSAHASH", "1010", ws) - check := hd.DebugHashMatchParam("bar", 1) - msg := ws.String() - t.Logf("message was '%s'", msg) - if !check { - t.Errorf("GOSSAHASH=1010 should have matched for 'bar', 1") - } - wantPrefix(t, msg, "GOSSAHASH triggered bar:1 ") - wantContains(t, msg, "\nbar:1 [bisect-match ") + wantPrefix(t, msg, "bar.0 [bisect-match ") + wantContains(t, msg, "\nGOSSAHASH triggered bar.0 ") } func TestYMatch(t *testing.T) { - ws := new(bufferWithSync) - hd := NewHashDebug("GOSSAHASH", "y", ws) - check := hd.DebugHashMatch("bar") - msg := ws.String() + b := new(bytes.Buffer) + hd := NewHashDebug("GOSSAHASH", "y", b) + check := hd.MatchPkgFunc("bar", "0") + msg := b.String() t.Logf("message was '%s'", msg) if !check { - t.Errorf("GOSSAHASH=y should have matched for 'bar'") + t.Errorf("GOSSAHASH=y should have matched for 'bar', '0'") } - wantPrefix(t, msg, "GOSSAHASH triggered bar 110101000010000100000011") - wantContains(t, msg, "\nbar [bisect-match ") + wantPrefix(t, msg, "bar.0 [bisect-match ") + wantContains(t, msg, "\nGOSSAHASH triggered bar.0 010100100011100101011110") } func TestNMatch(t *testing.T) { - ws := new(bufferWithSync) - hd := NewHashDebug("GOSSAHASH", "n", ws) - check := hd.DebugHashMatch("bar") - msg := ws.String() + b := new(bytes.Buffer) + hd := NewHashDebug("GOSSAHASH", "n", b) + check := hd.MatchPkgFunc("bar", "0") + msg := b.String() t.Logf("message was '%s'", msg) if check { - t.Errorf("GOSSAHASH=n should NOT have matched for 'bar'") + t.Errorf("GOSSAHASH=n should NOT have matched for 'bar', '0'") } - wantPrefix(t, msg, "GOSSAHASH triggered bar 110101000010000100000011") - wantContains(t, msg, "\nbar [bisect-match ") + wantPrefix(t, msg, "bar.0 [bisect-match ") + wantContains(t, msg, "\nGOSSAHASH triggered bar.0 010100100011100101011110") } func TestHashNoMatch(t *testing.T) { - ws := new(bufferWithSync) - hd := NewHashDebug("GOSSAHASH", "001100", ws) - check := hd.DebugHashMatch("bar") - msg := ws.String() + b := new(bytes.Buffer) + hd := NewHashDebug("GOSSAHASH", "01110", b) + check := hd.MatchPkgFunc("bar", "0") + msg := b.String() t.Logf("message was '%s'", msg) if check { - t.Errorf("GOSSAHASH=001100 should NOT have matched for 'bar'") + t.Errorf("GOSSAHASH=001100 should NOT have matched for 'bar', '0'") } if msg != "" { t.Errorf("Message should have been empty, instead %s", msg) @@ -125,41 +113,27 @@ func TestHashNoMatch(t *testing.T) { } func TestHashSecondMatch(t *testing.T) { - ws := new(bufferWithSync) - hd := NewHashDebug("GOSSAHASH", "001100/0011", ws) + b := new(bytes.Buffer) + hd := NewHashDebug("GOSSAHASH", "01110/11110", b) - check := hd.DebugHashMatch("bar") - msg := ws.String() + check := hd.MatchPkgFunc("bar", "0") + msg := b.String() t.Logf("message was '%s'", msg) if !check { - t.Errorf("GOSSAHASH=001100, GOSSAHASH0=0011 should have matched for 'bar'") + t.Errorf("GOSSAHASH=001100, GOSSAHASH0=0011 should have matched for 'bar', '0'") } - wantPrefix(t, msg, "GOSSAHASH0 triggered bar") -} - -type bufferWithSync struct { - b bytes.Buffer -} - -func (ws *bufferWithSync) Sync() error { - return nil -} - -func (ws *bufferWithSync) Write(p []byte) (n int, err error) { - return (&ws.b).Write(p) -} - -func (ws *bufferWithSync) String() string { - return strings.TrimSpace((&ws.b).String()) + wantContains(t, msg, "\nGOSSAHASH0 triggered bar") } func wantPrefix(t *testing.T, got, want string) { + t.Helper() if !strings.HasPrefix(got, want) { t.Errorf("want prefix %q, got:\n%s", want, got) } } func wantContains(t *testing.T, got, want string) { + t.Helper() if !strings.Contains(got, want) { t.Errorf("want contains %q, got:\n%s", want, got) } diff --git a/src/cmd/compile/internal/loopvar/loopvar.go b/src/cmd/compile/internal/loopvar/loopvar.go index 0d8a6d2d6e..7752cac535 100644 --- a/src/cmd/compile/internal/loopvar/loopvar.go +++ b/src/cmd/compile/internal/loopvar/loopvar.go @@ -91,7 +91,7 @@ func ForCapture(fn *ir.Func) []VarAndLoop { // subject to hash-variable debugging. maybeReplaceVar := func(k ir.Node, x *ir.RangeStmt) ir.Node { if n, ok := k.(*ir.Name); ok && possiblyLeaked[n] { - if base.LoopVarHash.DebugHashMatchPos(n.Pos()) { + if base.LoopVarHash.MatchPos(n.Pos()) { // Rename the loop key, prefix body with assignment from loop key transformed = append(transformed, VarAndLoop{n, x, lastPos}) tk := typecheck.Temp(n.Type()) @@ -199,7 +199,7 @@ func ForCapture(fn *ir.Func) []VarAndLoop { forAllDefInInit(x, func(z ir.Node) { if n, ok := z.(*ir.Name); ok && possiblyLeaked[n] { // Hash on n.Pos() for most precise failure location. - if base.LoopVarHash.DebugHashMatchPos(n.Pos()) { + if base.LoopVarHash.MatchPos(n.Pos()) { leaked = append(leaked, n) } } diff --git a/src/cmd/compile/internal/loopvar/loopvar_test.go b/src/cmd/compile/internal/loopvar/loopvar_test.go index 1a13f0e30c..5c7e11ac69 100644 --- a/src/cmd/compile/internal/loopvar/loopvar_test.go +++ b/src/cmd/compile/internal/loopvar/loopvar_test.go @@ -190,7 +190,7 @@ func TestLoopVarHashes(t *testing.T) { return string(b) } - m := f("000100000010011111101100") + m := f("011011011110011110111101") t.Logf(m) mCount := strings.Count(m, "loopvarhash triggered main.go:27:6") diff --git a/src/cmd/compile/internal/ssa/func.go b/src/cmd/compile/internal/ssa/func.go index f106cdd0b9..f2f548e9ba 100644 --- a/src/cmd/compile/internal/ssa/func.go +++ b/src/cmd/compile/internal/ssa/func.go @@ -773,8 +773,7 @@ func (f *Func) DebugHashMatch() bool { if !base.HasDebugHash() { return true } - name := f.fe.MyImportPath() + "." + f.Name - return base.DebugHashMatch(name) + return base.DebugHashMatchPkgFunc(f.fe.MyImportPath(), f.Name) } func (f *Func) spSb() (sp, sb *Value) { @@ -808,5 +807,5 @@ func (f *Func) useFMA(v *Value) bool { if base.FmaHash == nil { return true } - return base.FmaHash.DebugHashMatchPos(v.Pos) + return base.FmaHash.MatchPos(v.Pos) } -- 2.50.0