From f22cf7131a322f958d07c087e8d6a95723262180 Mon Sep 17 00:00:00 2001 From: David Chase Date: Thu, 30 Nov 2017 15:11:34 -0500 Subject: [PATCH] cmd/compile: use src.NoXPos for entry-block constants The ssa backend is aggressive about placing constants and certain other values in the Entry block. It's implausible that the original line numbers for these constants makes any sort of sense when it appears to a user stepping in a debugger, and they're also not that useful in dumps since entry-block instructions tend to be constants (i.e., unlikely to be the cause of a crash). Therefore, use src.NoXPos for any values that are explicitly inserted into a function's entry block. Passes all tests, including ssa/debug_test.go with both gdb and a fairly recent dlv. Hand-verified that it solves the reported problem; constructed a test that reproduced a problem, and fixed it. Modified test harness to allow injection of slightly more interesting inputs. Fixes #22558. Change-Id: I4476927067846bc4366da7793d2375c111694c55 Reviewed-on: https://go-review.googlesource.com/81215 Run-TryBot: David Chase TryBot-Result: Gobot Gobot Reviewed-by: Keith Randall --- src/cmd/compile/internal/gc/ssa.go | 10 ++--- src/cmd/compile/internal/ssa/debug_test.go | 11 +++-- src/cmd/compile/internal/ssa/func.go | 5 ++- .../internal/ssa/testdata/hist.dbg-dlv.nexts | 7 +-- .../internal/ssa/testdata/hist.dbg-gdb.nexts | 8 ++-- src/cmd/compile/internal/ssa/testdata/hist.go | 9 ++-- .../internal/ssa/testdata/hist.opt-dlv.nexts | 7 +-- .../internal/ssa/testdata/hist.opt-gdb.nexts | 8 ++-- .../ssa/testdata/i22558.dbg-22558-dlv.nexts | 11 +++++ .../ssa/testdata/i22558.dbg-22558-gdb.nexts | 11 +++++ .../compile/internal/ssa/testdata/i22558.go | 43 +++++++++++++++++++ .../ssa/testdata/i22600.dbg-race-gdb.nexts | 3 +- .../compile/internal/ssa/testdata/i22600.go | 6 ++- 13 files changed, 109 insertions(+), 30 deletions(-) create mode 100644 src/cmd/compile/internal/ssa/testdata/i22558.dbg-22558-dlv.nexts create mode 100644 src/cmd/compile/internal/ssa/testdata/i22558.dbg-22558-gdb.nexts create mode 100644 src/cmd/compile/internal/ssa/testdata/i22558.go diff --git a/src/cmd/compile/internal/gc/ssa.go b/src/cmd/compile/internal/gc/ssa.go index dfa2d081d1..36dd1a4be4 100644 --- a/src/cmd/compile/internal/gc/ssa.go +++ b/src/cmd/compile/internal/gc/ssa.go @@ -493,27 +493,27 @@ func (s *state) entryNewValue0(op ssa.Op, t *types.Type) *ssa.Value { // entryNewValue0A adds a new value with no arguments and an aux value to the entry block. func (s *state) entryNewValue0A(op ssa.Op, t *types.Type, aux interface{}) *ssa.Value { - return s.f.Entry.NewValue0A(s.peekPos(), op, t, aux) + return s.f.Entry.NewValue0A(src.NoXPos, op, t, aux) } // entryNewValue1 adds a new value with one argument to the entry block. func (s *state) entryNewValue1(op ssa.Op, t *types.Type, arg *ssa.Value) *ssa.Value { - return s.f.Entry.NewValue1(s.peekPos(), op, t, arg) + return s.f.Entry.NewValue1(src.NoXPos, op, t, arg) } // entryNewValue1 adds a new value with one argument and an auxint value to the entry block. func (s *state) entryNewValue1I(op ssa.Op, t *types.Type, auxint int64, arg *ssa.Value) *ssa.Value { - return s.f.Entry.NewValue1I(s.peekPos(), op, t, auxint, arg) + return s.f.Entry.NewValue1I(src.NoXPos, op, t, auxint, arg) } // entryNewValue1A adds a new value with one argument and an aux value to the entry block. func (s *state) entryNewValue1A(op ssa.Op, t *types.Type, aux interface{}, arg *ssa.Value) *ssa.Value { - return s.f.Entry.NewValue1A(s.peekPos(), op, t, aux, arg) + return s.f.Entry.NewValue1A(src.NoXPos, op, t, aux, arg) } // entryNewValue2 adds a new value with two arguments to the entry block. func (s *state) entryNewValue2(op ssa.Op, t *types.Type, arg0, arg1 *ssa.Value) *ssa.Value { - return s.f.Entry.NewValue2(s.peekPos(), op, t, arg0, arg1) + return s.f.Entry.NewValue2(src.NoXPos, op, t, arg0, arg1) } // const* routines add a new const value to the entry block. diff --git a/src/cmd/compile/internal/ssa/debug_test.go b/src/cmd/compile/internal/ssa/debug_test.go index 0e34902512..2eb4f73ee2 100644 --- a/src/cmd/compile/internal/ssa/debug_test.go +++ b/src/cmd/compile/internal/ssa/debug_test.go @@ -135,6 +135,9 @@ func TestNexting(t *testing.T) { t.Run("dbg-race-"+debugger, func(t *testing.T) { testNexting(t, "i22600", "dbg-race", "-N -l", "-race") }) + t.Run("dbg-22558-"+debugger, func(t *testing.T) { + testNexting(t, "i22558", "dbg-22558", "-N -l") + }) t.Run("opt-"+debugger, func(t *testing.T) { // If this is test is run with a runtime compiled with -N -l, it is very likely to fail. // This occurs in the noopt builders (for example). @@ -501,7 +504,7 @@ func (s *delveState) stepnext(ss string) bool { func (s *delveState) start() { if *dryrun { fmt.Printf("%s\n", asCommandLine("", s.cmd)) - fmt.Printf("b main.main\n") + fmt.Printf("b main.test\n") fmt.Printf("c\n") return } @@ -511,7 +514,7 @@ func (s *delveState) start() { panic(fmt.Sprintf("There was an error [start] running '%s', %v\n", line, err)) } s.ioState.readExpecting(-1, 5000, "Type 'help' for list of commands.") - expect("Breakpoint [0-9]+ set at ", s.ioState.writeReadExpect("b main.main\n", "[(]dlv[)] ")) + expect("Breakpoint [0-9]+ set at ", s.ioState.writeReadExpect("b main.test\n", "[(]dlv[)] ")) s.stepnext("c") } @@ -555,7 +558,7 @@ func (s *gdbState) start() { } if *dryrun { fmt.Printf("%s\n", asCommandLine("", s.cmd)) - fmt.Printf("tbreak main.main\n") + fmt.Printf("tbreak main.test\n") fmt.Printf("%s\n", run) return } @@ -565,7 +568,7 @@ func (s *gdbState) start() { panic(fmt.Sprintf("There was an error [start] running '%s', %v\n", line, err)) } s.ioState.readExpecting(-1, -1, "[(]gdb[)] ") - x := s.ioState.writeReadExpect("b main.main\n", "[(]gdb[)] ") + x := s.ioState.writeReadExpect("b main.test\n", "[(]gdb[)] ") expect("Breakpoint [0-9]+ at", x) s.stepnext(run) } diff --git a/src/cmd/compile/internal/ssa/func.go b/src/cmd/compile/internal/ssa/func.go index 01966adb0f..62550df0cc 100644 --- a/src/cmd/compile/internal/ssa/func.go +++ b/src/cmd/compile/internal/ssa/func.go @@ -407,6 +407,7 @@ func (b *Block) NewValue4(pos src.XPos, op Op, t *types.Type, arg0, arg1, arg2, // constVal returns a constant value for c. func (f *Func) constVal(pos src.XPos, op Op, t *types.Type, c int64, setAuxInt bool) *Value { + // TODO remove unused pos parameter, both here and in *func.ConstXXX callers. if f.constants == nil { f.constants = make(map[int64][]*Value) } @@ -421,9 +422,9 @@ func (f *Func) constVal(pos src.XPos, op Op, t *types.Type, c int64, setAuxInt b } var v *Value if setAuxInt { - v = f.Entry.NewValue0I(pos, op, t, c) + v = f.Entry.NewValue0I(src.NoXPos, op, t, c) } else { - v = f.Entry.NewValue0(pos, op, t) + v = f.Entry.NewValue0(src.NoXPos, op, t) } f.constants[c] = append(vv, v) return v diff --git a/src/cmd/compile/internal/ssa/testdata/hist.dbg-dlv.nexts b/src/cmd/compile/internal/ssa/testdata/hist.dbg-dlv.nexts index f4fe2af161..ec79b77de2 100644 --- a/src/cmd/compile/internal/ssa/testdata/hist.dbg-dlv.nexts +++ b/src/cmd/compile/internal/ssa/testdata/hist.dbg-dlv.nexts @@ -1,12 +1,12 @@ ./testdata/hist.go -55: func main() { +55: func test() { 57: l := line{point{1 + zero, 2 + zero}, point{3 + zero, 4 + zero}} 58: tinycall() // this forces l etc to stack 59: dx := l.end.x - l.begin.x //gdb-dbg=(l.begin.x,l.end.y)//gdb-opt=(l,dx/O,dy/O) 60: dy := l.end.y - l.begin.y //gdb-opt=(dx,dy/O) 61: sink = dx + dy //gdb-opt=(dx,dy) -63: hist := make([]int, 7) //gdb-opt=(sink,dx/O,dy/O) -64: var reader io.Reader = strings.NewReader(cannedInput) //gdb-dbg=(hist/A,cannedInput/A) +63: hist := make([]int, 7) //gdb-opt=(dx/O,dy/O) // TODO sink is missing if this code is in 'test' instead of 'main' +64: var reader io.Reader = strings.NewReader(cannedInput) //gdb-dbg=(hist/A) // TODO cannedInput/A is missing if this code is in 'test' instead of 'main' 65: if len(os.Args) > 1 { 73: scanner := bufio.NewScanner(reader) 74: for scanner.Scan() { //gdb-opt=(scanner/A) @@ -96,3 +96,4 @@ 87: if a == 0 { //gdb-opt=(a,n,t) 88: continue 86: for i, a := range hist { +98: } diff --git a/src/cmd/compile/internal/ssa/testdata/hist.dbg-gdb.nexts b/src/cmd/compile/internal/ssa/testdata/hist.dbg-gdb.nexts index abd4535ca5..fe000147bd 100644 --- a/src/cmd/compile/internal/ssa/testdata/hist.dbg-gdb.nexts +++ b/src/cmd/compile/internal/ssa/testdata/hist.dbg-gdb.nexts @@ -1,5 +1,5 @@ src/cmd/compile/internal/ssa/testdata/hist.go -55: func main() { +55: func test() { 57: l := line{point{1 + zero, 2 + zero}, point{3 + zero, 4 + zero}} 58: tinycall() // this forces l etc to stack 59: dx := l.end.x - l.begin.x //gdb-dbg=(l.begin.x,l.end.y)//gdb-opt=(l,dx/O,dy/O) @@ -7,10 +7,9 @@ l.begin.x = 1 l.end.y = 4 60: dy := l.end.y - l.begin.y //gdb-opt=(dx,dy/O) 61: sink = dx + dy //gdb-opt=(dx,dy) -63: hist := make([]int, 7) //gdb-opt=(sink,dx/O,dy/O) -64: var reader io.Reader = strings.NewReader(cannedInput) //gdb-dbg=(hist/A,cannedInput/A) +63: hist := make([]int, 7) //gdb-opt=(dx/O,dy/O) // TODO sink is missing if this code is in 'test' instead of 'main' +64: var reader io.Reader = strings.NewReader(cannedInput) //gdb-dbg=(hist/A) // TODO cannedInput/A is missing if this code is in 'test' instead of 'main' hist = []int = {0, 0, 0, 0, 0, 0, 0} -cannedInput = "1\n1\n1\n2\n2\n2\n4\n4\n5\n" 65: if len(os.Args) > 1 { 73: scanner := bufio.NewScanner(reader) 74: for scanner.Scan() { //gdb-opt=(scanner/A) @@ -121,3 +120,4 @@ t = 22 87: if a == 0 { //gdb-opt=(a,n,t) 88: continue 86: for i, a := range hist { +98: } diff --git a/src/cmd/compile/internal/ssa/testdata/hist.go b/src/cmd/compile/internal/ssa/testdata/hist.go index 7d1d06b47d..8a0cc27280 100644 --- a/src/cmd/compile/internal/ssa/testdata/hist.go +++ b/src/cmd/compile/internal/ssa/testdata/hist.go @@ -52,7 +52,7 @@ var cannedInput string = `1 5 ` -func main() { +func test() { // For #19868 l := line{point{1 + zero, 2 + zero}, point{3 + zero, 4 + zero}} tinycall() // this forces l etc to stack @@ -60,8 +60,8 @@ func main() { dy := l.end.y - l.begin.y //gdb-opt=(dx,dy/O) sink = dx + dy //gdb-opt=(dx,dy) // For #21098 - hist := make([]int, 7) //gdb-opt=(sink,dx/O,dy/O) - var reader io.Reader = strings.NewReader(cannedInput) //gdb-dbg=(hist/A,cannedInput/A) + hist := make([]int, 7) //gdb-opt=(dx/O,dy/O) // TODO sink is missing if this code is in 'test' instead of 'main' + var reader io.Reader = strings.NewReader(cannedInput) //gdb-dbg=(hist/A) // TODO cannedInput/A is missing if this code is in 'test' instead of 'main' if len(os.Args) > 1 { var err error reader, err = os.Open(os.Args[1]) @@ -91,5 +91,8 @@ func main() { n += a fmt.Fprintf(os.Stderr, "%d\t%d\t%d\t%d\t%d\n", i, a, n, i*a, t) //gdb-dbg=(n,i,t) } +} +func main() { + test() } diff --git a/src/cmd/compile/internal/ssa/testdata/hist.opt-dlv.nexts b/src/cmd/compile/internal/ssa/testdata/hist.opt-dlv.nexts index 7aab219552..b98e3c6e65 100644 --- a/src/cmd/compile/internal/ssa/testdata/hist.opt-dlv.nexts +++ b/src/cmd/compile/internal/ssa/testdata/hist.opt-dlv.nexts @@ -1,13 +1,13 @@ ./testdata/hist.go -55: func main() { +55: func test() { 57: l := line{point{1 + zero, 2 + zero}, point{3 + zero, 4 + zero}} 58: tinycall() // this forces l etc to stack 57: l := line{point{1 + zero, 2 + zero}, point{3 + zero, 4 + zero}} 59: dx := l.end.x - l.begin.x //gdb-dbg=(l.begin.x,l.end.y)//gdb-opt=(l,dx/O,dy/O) 60: dy := l.end.y - l.begin.y //gdb-opt=(dx,dy/O) 61: sink = dx + dy //gdb-opt=(dx,dy) -63: hist := make([]int, 7) //gdb-opt=(sink,dx/O,dy/O) -64: var reader io.Reader = strings.NewReader(cannedInput) //gdb-dbg=(hist/A,cannedInput/A) +63: hist := make([]int, 7) //gdb-opt=(dx/O,dy/O) // TODO sink is missing if this code is in 'test' instead of 'main' +64: var reader io.Reader = strings.NewReader(cannedInput) //gdb-dbg=(hist/A) // TODO cannedInput/A is missing if this code is in 'test' instead of 'main' 65: if len(os.Args) > 1 { 74: for scanner.Scan() { //gdb-opt=(scanner/A) 76: i, err := strconv.ParseInt(s, 10, 64) @@ -102,3 +102,4 @@ 92: fmt.Fprintf(os.Stderr, "%d\t%d\t%d\t%d\t%d\n", i, a, n, i*a, t) //gdb-dbg=(n,i,t) 87: if a == 0 { //gdb-opt=(a,n,t) 88: continue +98: } diff --git a/src/cmd/compile/internal/ssa/testdata/hist.opt-gdb.nexts b/src/cmd/compile/internal/ssa/testdata/hist.opt-gdb.nexts index f6c6a3c9be..e4dc280869 100644 --- a/src/cmd/compile/internal/ssa/testdata/hist.opt-gdb.nexts +++ b/src/cmd/compile/internal/ssa/testdata/hist.opt-gdb.nexts @@ -1,5 +1,5 @@ src/cmd/compile/internal/ssa/testdata/hist.go -55: func main() { +55: func test() { 57: l := line{point{1 + zero, 2 + zero}, point{3 + zero, 4 + zero}} 58: tinycall() // this forces l etc to stack 57: l := line{point{1 + zero, 2 + zero}, point{3 + zero, 4 + zero}} @@ -13,11 +13,10 @@ dy = 61: sink = dx + dy //gdb-opt=(dx,dy) dx = 2 dy = 2 -63: hist := make([]int, 7) //gdb-opt=(sink,dx/O,dy/O) -sink = 4 +63: hist := make([]int, 7) //gdb-opt=(dx/O,dy/O) // TODO sink is missing if this code is in 'test' instead of 'main' dx = dy = -64: var reader io.Reader = strings.NewReader(cannedInput) //gdb-dbg=(hist/A,cannedInput/A) +64: var reader io.Reader = strings.NewReader(cannedInput) //gdb-dbg=(hist/A) // TODO cannedInput/A is missing if this code is in 'test' instead of 'main' 65: if len(os.Args) > 1 { 73: scanner := bufio.NewScanner(reader) 74: for scanner.Scan() { //gdb-opt=(scanner/A) @@ -180,3 +179,4 @@ a = 0 n = 9 t = 22 88: continue +98: } diff --git a/src/cmd/compile/internal/ssa/testdata/i22558.dbg-22558-dlv.nexts b/src/cmd/compile/internal/ssa/testdata/i22558.dbg-22558-dlv.nexts new file mode 100644 index 0000000000..3c33fe0bfd --- /dev/null +++ b/src/cmd/compile/internal/ssa/testdata/i22558.dbg-22558-dlv.nexts @@ -0,0 +1,11 @@ + ./testdata/i22558.go +19: func test(t *thing, u *thing) { +20: if t.next != nil { +23: fmt.Fprintf(os.Stderr, "%s\n", t.name) +24: u.self = u +25: t.self = t +26: t.next = u +27: for _, p := range t.stuff { +28: if isFoo(t, p) { +29: return +43: } diff --git a/src/cmd/compile/internal/ssa/testdata/i22558.dbg-22558-gdb.nexts b/src/cmd/compile/internal/ssa/testdata/i22558.dbg-22558-gdb.nexts new file mode 100644 index 0000000000..b88a227ec6 --- /dev/null +++ b/src/cmd/compile/internal/ssa/testdata/i22558.dbg-22558-gdb.nexts @@ -0,0 +1,11 @@ + src/cmd/compile/internal/ssa/testdata/i22558.go +19: func test(t *thing, u *thing) { +20: if t.next != nil { +23: fmt.Fprintf(os.Stderr, "%s\n", t.name) +24: u.self = u +25: t.self = t +26: t.next = u +27: for _, p := range t.stuff { +28: if isFoo(t, p) { +29: return +43: } diff --git a/src/cmd/compile/internal/ssa/testdata/i22558.go b/src/cmd/compile/internal/ssa/testdata/i22558.go new file mode 100644 index 0000000000..a62e11e5eb --- /dev/null +++ b/src/cmd/compile/internal/ssa/testdata/i22558.go @@ -0,0 +1,43 @@ +package main + +import ( + "fmt" + "os" +) + +type big struct { + pile [768]int8 +} + +type thing struct { + name string + next *thing + self *thing + stuff []big +} + +func test(t *thing, u *thing) { + if t.next != nil { + return + } + fmt.Fprintf(os.Stderr, "%s\n", t.name) + u.self = u + t.self = t + t.next = u + for _, p := range t.stuff { + if isFoo(t, p) { + return + } + } +} + +//go:noinline +func isFoo(t *thing, b big) bool { + return true +} + +func main() { + t := &thing{name: "t", self: nil, next: nil, stuff: make([]big, 1)} + u := thing{name: "u", self: t, next: t, stuff: make([]big, 1)} + test(t, &u) +} diff --git a/src/cmd/compile/internal/ssa/testdata/i22600.dbg-race-gdb.nexts b/src/cmd/compile/internal/ssa/testdata/i22600.dbg-race-gdb.nexts index 681167d3af..bfffec4a5d 100644 --- a/src/cmd/compile/internal/ssa/testdata/i22600.dbg-race-gdb.nexts +++ b/src/cmd/compile/internal/ssa/testdata/i22600.dbg-race-gdb.nexts @@ -1,6 +1,7 @@ src/cmd/compile/internal/ssa/testdata/i22600.go -8: func main() { +8: func test() { 9: pwd, err := os.Getwd() 10: if err != nil { 14: fmt.Println(pwd) 15: } +19: } diff --git a/src/cmd/compile/internal/ssa/testdata/i22600.go b/src/cmd/compile/internal/ssa/testdata/i22600.go index 8cecd14a4b..f7a7ade374 100644 --- a/src/cmd/compile/internal/ssa/testdata/i22600.go +++ b/src/cmd/compile/internal/ssa/testdata/i22600.go @@ -5,7 +5,7 @@ import ( "os" ) -func main() { +func test() { pwd, err := os.Getwd() if err != nil { fmt.Println(err) @@ -13,3 +13,7 @@ func main() { } fmt.Println(pwd) } + +func main() { + test() +} -- 2.50.0