]> Cypherpunks repositories - gostls13.git/commitdiff
cmd/compile: enhance debug_test for infinite loops
authorDavid Chase <drchase@google.com>
Wed, 20 Mar 2019 18:29:28 +0000 (14:29 -0400)
committerDavid Chase <drchase@google.com>
Wed, 27 Mar 2019 21:04:34 +0000 (21:04 +0000)
ssa/debug_test.go already had a step limit; this exposes
it to individual tests, and it is then set low for the
infinite loop tests.

That however is not enough; in an infinite loop debuggers
see an unchanging line number, and therefore keep trying
until they see a different one.  To do this, the concept
of a "bogus" line number is introduced, and on output
single-instruction infinite loops are detected and a
hardware nop with correct line number is inserted into
the loop; the branch itself receives a bogus line number.

This breaks up the endless stream of same line number and
causes both gdb and delve to not hang; Delve complains
about the incorrect line number while gdb does
a sort of odd step-to-nowhere that then steps back
to the loop.  Since repeats are suppressed in the reference
file, a single line is shown there.

(The wrong line number mentioned in previous message
was an artifact of debug_test.go, not Delve, and is now
fixed.)

The bogus line number exposed in Delve is less than
wonderful, but compared to hanging, it is better.

Fixes #30664.

Change-Id: I30c927cf8869a84c6c9b84033ee44d7044aab552
Reviewed-on: https://go-review.googlesource.com/c/go/+/168477
Run-TryBot: David Chase <drchase@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Keith Randall <khr@golang.org>
src/cmd/compile/internal/gc/ssa.go
src/cmd/compile/internal/ssa/debug_test.go
src/cmd/compile/internal/ssa/testdata/infloop.dlv-opt.nexts [new file with mode: 0644]
src/cmd/compile/internal/ssa/testdata/infloop.gdb-opt.nexts [new file with mode: 0644]
src/cmd/compile/internal/ssa/testdata/infloop.go [new file with mode: 0644]
src/cmd/internal/src/pos.go
src/cmd/internal/src/xpos.go

index a7c1917ff11ce7c00b611785440c338bfc4ed26d..17a9a2664c5fb5229e7e72f3ae3bcf2d1d75fe30 100644 (file)
@@ -5315,6 +5315,12 @@ func genssa(f *ssa.Func, pp *Progs) {
                                }
                        }
                }
+               // If this is an empty infinite loop, stick a hardware NOP in there so that debuggers are less confused.
+               if s.bstart[b.ID] == s.pp.next && len(b.Succs) == 1 && b.Succs[0].Block() == b {
+                       p := thearch.Ginsnop(s.pp)
+                       p.Pos = p.Pos.WithIsStmt()
+                       b.Pos = b.Pos.WithBogusLine() // Debuggers are not good about infinite loops, force a change in line number
+               }
                // Emit control flow instructions for block
                var next *ssa.Block
                if i < len(f.Blocks)-1 && Debug['N'] == 0 {
index 7246a13ff6ce4cc0d92c910ddaf4cbfc64e73a99..8db2f8ef4171860da842dd4ce4493bef137de5ad 100644 (file)
@@ -156,33 +156,34 @@ func TestNexting(t *testing.T) {
 
        subTest(t, debugger+"-dbg-race", "i22600", dbgFlags, append(moreargs, "-race")...)
 
-       optSubTest(t, debugger+"-opt", "hist", optFlags, moreargs...)
-       optSubTest(t, debugger+"-opt", "scopes", optFlags, moreargs...)
+       optSubTest(t, debugger+"-opt", "hist", optFlags, 1000, moreargs...)
+       optSubTest(t, debugger+"-opt", "scopes", optFlags, 1000, moreargs...)
+       optSubTest(t, debugger+"-opt", "infloop", optFlags, 10, moreargs...)
 }
 
 // subTest creates a subtest that compiles basename.go with the specified gcflags and additional compiler arguments,
 // then runs the debugger on the resulting binary, with any comment-specified actions matching tag triggered.
 func subTest(t *testing.T, tag string, basename string, gcflags string, moreargs ...string) {
        t.Run(tag+"-"+basename, func(t *testing.T) {
-               testNexting(t, basename, tag, gcflags, moreargs...)
+               testNexting(t, basename, tag, gcflags, 1000, moreargs...)
        })
 }
 
 // optSubTest is the same as subTest except that it skips the test if the runtime and libraries
 // were not compiled with optimization turned on.  (The skip may not be necessary with Go 1.10 and later)
-func optSubTest(t *testing.T, tag string, basename string, gcflags string, moreargs ...string) {
+func optSubTest(t *testing.T, tag string, basename string, gcflags string, count int, moreargs ...string) {
        // If optimized test is run with unoptimized libraries (compiled with -N -l), it is very likely to fail.
        // This occurs in the noopt builders (for example).
        t.Run(tag+"-"+basename, func(t *testing.T) {
                if *force || optimizedLibs {
-                       testNexting(t, basename, tag, gcflags, moreargs...)
+                       testNexting(t, basename, tag, gcflags, count, moreargs...)
                } else {
                        t.Skip("skipping for unoptimized stdlib/runtime")
                }
        })
 }
 
-func testNexting(t *testing.T, base, tag, gcflags string, moreArgs ...string) {
+func testNexting(t *testing.T, base, tag, gcflags string, count int, moreArgs ...string) {
        // (1) In testdata, build sample.go into test-sample.<tag>
        // (2) Run debugger gathering a history
        // (3) Read expected history from testdata/sample.<tag>.nexts
@@ -219,7 +220,7 @@ func testNexting(t *testing.T, base, tag, gcflags string, moreArgs ...string) {
        } else {
                dbg = newGdb(tag, exe)
        }
-       h1 := runDbgr(dbg, 1000)
+       h1 := runDbgr(dbg, count)
        if *dryrun {
                fmt.Printf("# Tag for above is %s\n", dbg.tag())
                return
@@ -261,6 +262,7 @@ func runDbgr(dbg dbgr, maxNext int) *nextHist {
                        break
                }
        }
+       dbg.quit()
        h := dbg.hist()
        return h
 }
@@ -298,7 +300,7 @@ func (t tstring) String() string {
 }
 
 type pos struct {
-       line uint16
+       line uint32
        file uint8 // Artifact of plans to implement differencing instead of calling out to diff.
 }
 
@@ -386,7 +388,7 @@ func (h *nextHist) add(file, line, text string) bool {
                }
        }
        l := len(h.ps)
-       p := pos{line: uint16(li), file: fi}
+       p := pos{line: uint32(li), file: fi}
 
        if l == 0 || *repeats || h.ps[l-1] != p {
                h.ps = append(h.ps, p)
@@ -721,6 +723,15 @@ func varsToPrint(line, lookfor string) []string {
 func (s *gdbState) quit() {
        response := s.ioState.writeRead("q\n")
        if strings.Contains(response.o, "Quit anyway? (y or n)") {
+               defer func() {
+                       if r := recover(); r != nil {
+                               if s, ok := r.(string); !(ok && strings.Contains(s, "'Y\n'")) {
+                                       // Not the panic that was expected.
+                                       fmt.Printf("Expected a broken pipe panic, but saw the following panic instead")
+                                       panic(r)
+                               }
+                       }
+               }()
                s.ioState.writeRead("Y\n")
        }
 }
diff --git a/src/cmd/compile/internal/ssa/testdata/infloop.dlv-opt.nexts b/src/cmd/compile/internal/ssa/testdata/infloop.dlv-opt.nexts
new file mode 100644 (file)
index 0000000..19496de
--- /dev/null
@@ -0,0 +1,12 @@
+  ./testdata/infloop.go
+6:     func test() {
+8:             go func() {}()
+10:            for {
+1048575:
+10:            for {
+1048575:
+10:            for {
+1048575:
+10:            for {
+1048575:
+10:            for {
diff --git a/src/cmd/compile/internal/ssa/testdata/infloop.gdb-opt.nexts b/src/cmd/compile/internal/ssa/testdata/infloop.gdb-opt.nexts
new file mode 100644 (file)
index 0000000..d465ad1
--- /dev/null
@@ -0,0 +1,4 @@
+  src/cmd/compile/internal/ssa/testdata/infloop.go
+6:     func test() {
+8:             go func() {}()
+10:            for {
diff --git a/src/cmd/compile/internal/ssa/testdata/infloop.go b/src/cmd/compile/internal/ssa/testdata/infloop.go
new file mode 100644 (file)
index 0000000..cdb374f
--- /dev/null
@@ -0,0 +1,16 @@
+package main
+
+var sink int
+
+//go:noinline
+func test() {
+       // This is for #30167, incorrect line numbers in an infinite loop
+       go func() {}()
+
+       for {
+       }
+}
+
+func main() {
+       test()
+}
index a30b4b6e4a11b844cd939cba21748e9361bd1398..8344a5a612577076c6a6bd71cb352aa086e79728 100644 (file)
@@ -304,7 +304,8 @@ type lico uint32
 // TODO: Prologue and epilogue are perhaps better handled as psuedoops for the assembler,
 // because they have almost no interaction with other uses of the position.
 const (
-       lineBits, lineMax     = 20, 1<<lineBits - 1
+       lineBits, lineMax     = 20, 1<<lineBits - 2
+       bogusLine             = 1<<lineBits - 1 // Not a line number; used to disruopt infinite loops
        isStmtBits, isStmtMax = 2, 1<<isStmtBits - 1
        xlogueBits, xlogueMax = 2, 1<<xlogueBits - 1
        colBits, colMax       = 32 - lineBits - xlogueBits - isStmtBits, 1<<colBits - 1
@@ -355,6 +356,16 @@ const (
        PosEpilogueBegin
 )
 
+func makeLicoRaw(line, col uint) lico {
+       return lico(line<<lineShift | col<<colShift)
+}
+
+// This is a not-position that will not be elided.
+// Depending on the debugger (gdb or delve) it may or may not be displayed.
+func makeBogusLico() lico {
+       return makeLicoRaw(bogusLine, 0).withIsStmt()
+}
+
 func makeLico(line, col uint) lico {
        if line > lineMax {
                // cannot represent line, use max. line so we have some information
@@ -365,7 +376,7 @@ func makeLico(line, col uint) lico {
                col = colMax
        }
        // default is not-sure-if-statement
-       return lico(line<<lineShift | col<<colShift)
+       return makeLicoRaw(line, col)
 }
 
 func (x lico) Line() uint { return uint(x) >> lineShift }
index c94f9e997b62ac9704639425078dcf134e20dd9d..593251539c8c504b298e55180f57bba6b40674c9 100644 (file)
@@ -60,6 +60,16 @@ func (p XPos) WithIsStmt() XPos {
        return p
 }
 
+// WithBogusLine returns a bogus line that won't match any recorded for the source code.
+// Its use is to disrupt the statements within an infinite loop so that the debugger
+// will not itself loop infinitely waiting for the line number to change.
+// gdb chooses not to display the bogus line; delve shows it with a complaint, but the
+// alternative behavior is to hang.
+func (p XPos) WithBogusLine() XPos {
+       p.lico = makeBogusLico()
+       return p
+}
+
 // WithXlogue returns the same location but marked with DWARF function prologue/epilogue
 func (p XPos) WithXlogue(x PosXlogue) XPos {
        p.lico = p.lico.withXlogue(x)