cmd/compile: spos handling fixes to improve prolog debuggability

With the new register ABI, the compiler sometimes introduces spills of
argument registers in function prologs; depending on the positions
assigned to these spills and whether they have the IsStmt flag set,
this can degrade the debugging experience. For example, in this
function from one of the Delve regression tests:

L13:  func foo((eface interface{}) {
L14:	if eface != nil {
L15:		n++
L16:	}
L17   }

we wind up with a prolog containing two spill instructions, the first
with line 14, the second with line 13.  The end result for the user
is that if you set a breakpoint in foo and run to it, then do "step",
execution will initially stop at L14, then jump "backwards" to L13.

The root of the problem in this case is that an ArgIntReg pseudo-op is
introduced during expand calls, then promoted (due to lowering) to a
first-class statement (IsStmt flag set), which in turn causes
downstream handling to propagate its position to the first of the register
spills in the prolog.

To help improve things, this patch changes the rewriter to avoid
moving an "IsStmt" flag from a deleted/replaced instruction to an
Arg{Int,Float}Reg value, and adds Arg{Int,Float}Reg to the list of
opcodes not suitable for selection as statement boundaries, and
suppresses generation of additional register spills in defframe() when
optimization is disabled (since in that case things will get spilled
in any case).

This is not a comprehensive/complete fix; there are still cases where
we get less-than-ideal source position markers (ex: issue 45680).

Updates #40724.

Change-Id: Ica8bba4940b2291bef6b5d95ff0cfd84412a2d40
Reviewed-on: https://go-review.googlesource.com/c/go/+/312989
Trust: Than McIntosh <thanm@google.com>
Run-TryBot: Than McIntosh <thanm@google.com>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: David Chase <drchase@google.com>
Reviewed-by: Cherry Zhang <cherryyz@google.com>

diff --git a/src/cmd/compile/internal/amd64/ssa.go b/src/cmd/compile/internal/amd64/ssa.go
index 38d2d43e2790be01d9aeb0fa742c96fb13db9cea..6a478de2a0c288d1ad0ef84f4876ae7de4251a38 100644 (file)
--- a/src/cmd/compile/internal/amd64/ssa.go
+++ b/src/cmd/compile/internal/amd64/ssa.go
@@ -1370,5 +1370,6 @@ func spillArgReg(pp *objw.Progs, p *obj.Prog, f *ssa.Func, t *types.Type, reg in
        p = pp.Append(p, storeByType(t), obj.TYPE_REG, reg, 0, obj.TYPE_MEM, 0, n.FrameOffset()+off)
        p.To.Name = obj.NAME_PARAM
        p.To.Sym = n.Linksym()
+       p.Pos = p.Pos.WithNotStmt()
        return p
 }

diff --git a/src/cmd/compile/internal/ssa/numberlines.go b/src/cmd/compile/internal/ssa/numberlines.go
index 54a158ff8769e0c7aef27d5ccb924d6193f1734f..9d6aeca9c0dc019b6cbafc7bd05f61b51383a036 100644 (file)
--- a/src/cmd/compile/internal/ssa/numberlines.go
+++ b/src/cmd/compile/internal/ssa/numberlines.go
@@ -16,7 +16,8 @@ func isPoorStatementOp(op Op) bool {
        // so that a debugger-user sees the stop before the panic, and can examine the value.
        case OpAddr, OpLocalAddr, OpOffPtr, OpStructSelect, OpPhi, OpITab, OpIData,
                OpIMake, OpStringMake, OpSliceMake, OpStructMake0, OpStructMake1, OpStructMake2, OpStructMake3, OpStructMake4,
-               OpConstBool, OpConst8, OpConst16, OpConst32, OpConst64, OpConst32F, OpConst64F, OpSB, OpSP:
+               OpConstBool, OpConst8, OpConst16, OpConst32, OpConst64, OpConst32F, OpConst64F, OpSB, OpSP,
+               OpArgIntReg, OpArgFloatReg:
                return true
        }
        return false
@@ -61,7 +62,7 @@ func nextGoodStatementIndex(v *Value, i int, b *Block) int {
 // statement boundary.
 func notStmtBoundary(op Op) bool {
        switch op {
-       case OpCopy, OpPhi, OpVarKill, OpVarDef, OpVarLive, OpUnknown, OpFwdRef, OpArg:
+       case OpCopy, OpPhi, OpVarKill, OpVarDef, OpVarLive, OpUnknown, OpFwdRef, OpArg, OpArgIntReg, OpArgFloatReg:
                return true
        }
        return false

diff --git a/src/cmd/compile/internal/ssa/rewrite.go b/src/cmd/compile/internal/ssa/rewrite.go
index bdc4f799aa242b9922fed2eaa2b4a60c6921f320..375c4d5a5605f531b6b5ab761be276f41d72031b 100644 (file)
--- a/src/cmd/compile/internal/ssa/rewrite.go
+++ b/src/cmd/compile/internal/ssa/rewrite.go
@@ -159,7 +159,7 @@ func applyRewrite(f *Func, rb blockRewriter, rv valueRewriter, deadcode deadValu
                                f.freeValue(v)
                                continue
                        }
-                       if v.Pos.IsStmt() != src.PosNotStmt && pendingLines.get(vl) == int32(b.ID) {
+                       if v.Pos.IsStmt() != src.PosNotStmt && !notStmtBoundary(v.Op) && pendingLines.get(vl) == int32(b.ID) {
                                pendingLines.remove(vl)
                                v.Pos = v.Pos.WithIsStmt()
                        }

diff --git a/src/cmd/compile/internal/ssagen/ssa.go b/src/cmd/compile/internal/ssagen/ssa.go
index 800d6a0b635eed902468fff83321ed8d1ae32bba..891047f56d9b432b5cc7634d11f17f15a959c2ec 100644 (file)
--- a/src/cmd/compile/internal/ssagen/ssa.go
+++ b/src/cmd/compile/internal/ssagen/ssa.go
@@ -7071,8 +7071,10 @@ func defframe(s *State, e *ssafn, f *ssa.Func) {
        // slot. This can only happen with aggregate-typed arguments that are SSA-able
        // and not address-taken (for non-SSA-able or address-taken arguments we always
        // spill upfront).
+       // Note: spilling is unnecessary in the -N/no-optimize case, since all values
+       // will be considered non-SSAable and spilled up front.
        // TODO(register args) Make liveness more fine-grained to that partial spilling is okay.
-       if f.OwnAux.ABIInfo().InRegistersUsed() != 0 {
+       if f.OwnAux.ABIInfo().InRegistersUsed() != 0 && base.Flag.N == 0 {
                // First, see if it is already spilled before it may be live. Look for a spill
                // in the entry block up to the first safepoint.
                type nameOff struct {