build: preserve DebugRefs during SSA order repairs

internal/build/ssa_order_fix.go

@@ -186,6 +186,9 @@ func moveAssignDepsAfterRecv(b *ssa.BasicBlock, roots []ssa.Value, recv ssa.Valu
 	if len(move) == 0 {
 		return false
 	}
+	// Metadata uses must follow the definitions they describe rather than
+	// blocking an otherwise safe source-order repair.
+	includeDebugRefsForMovedValues(b.Instrs, move, recvIdx)
 	if moveWouldBreakSSA(b.Instrs, move, recvIdx) {
 		return false
 	}
@@ -205,6 +208,30 @@ func moveAssignDepsAfterRecv(b *ssa.BasicBlock, roots []ssa.Value, recv ssa.Valu
 	return true
 }
 
+func includeDebugRefsForMovedValues(instrs []ssa.Instruction, move map[int]struct{}, through int) {
+	moved := make(map[ssa.Value]struct{}, len(move))
+	for i := range move {
+		if v, ok := instrs[i].(ssa.Value); ok && v != nil {
+			moved[v] = struct{}{}
+		}
+	}
+	for i := 0; i <= through && i < len(instrs); i++ {
+		if _, moving := move[i]; moving {
+			continue
+		}
+		ref, ok := instrs[i].(*ssa.DebugRef)
+		if !ok {
+			continue
+		}
+		for v := range moved {
+			if instrUsesValue(ref, v) {
+				move[i] = struct{}{}
+				break
+			}
+		}
+	}
+}
+
 func moveWouldBreakSSA(instrs []ssa.Instruction, move map[int]struct{}, recvIdx int) bool {
 	moved := make(map[ssa.Value]struct{}, len(move))
 	for i := range move {
@@ -302,11 +329,15 @@ func fixSSAOrderBlock(b *ssa.BasicBlock) {
 			continue
 		}
 
-		// If the loaded value is used by any instruction between its current
-		// position and the return (excluding return itself), moving it may place
-		// its definition after one of those uses and break SSA form.
+		// DebugRefs are metadata-only and move with the value they describe. Any
+		// executable use before Return still makes reordering unsafe.
+		moving := map[ssa.Instruction]struct{}{u: {}}
 		usedBeforeReturn := false
 		for i := loadIdx + 1; i < retIdx; i++ {
+			if ref, ok := b.Instrs[i].(*ssa.DebugRef); ok && instrUsesValue(ref, u) {
+				moving[ref] = struct{}{}
+				continue
+			}
 			if instrUsesValue(b.Instrs[i], u) {
 				usedBeforeReturn = true
 				break
@@ -316,9 +347,7 @@ func fixSSAOrderBlock(b *ssa.BasicBlock) {
 			continue
 		}
 
-		// Move the load right after the last call (but before Return).
-		b.Instrs = moveInstr(b.Instrs, loadIdx, lastCallIdx+1)
-		// Adjust retIdx for subsequent moves in this block.
+		b.Instrs = moveInstrsAfter(b.Instrs, moving, b.Instrs[lastCallIdx])
 		retIdx = indexOfInstr(b.Instrs, ret)
 	}
 }
@@ -391,41 +420,29 @@ func valueDependsOn(v, target ssa.Value, seen map[ssa.Value]struct{}) bool {
 	return false
 }
 
-// moveInstr moves instrs[from] to position to (like inserting before to),
-// preserving relative order of other elements.
-func moveInstr(instrs []ssa.Instruction, from, to int) []ssa.Instruction {
-	if from < 0 || from >= len(instrs) {
-		return instrs
-	}
-	if to < 0 {
-		to = 0
-	}
-	if to > len(instrs) {
-		to = len(instrs)
-	}
-	if from == to || from+1 == to {
+// moveInstrsAfter moves selected instructions as a stable group immediately
+// after anchor.
+func moveInstrsAfter(instrs []ssa.Instruction, moving map[ssa.Instruction]struct{}, anchor ssa.Instruction) []ssa.Instruction {
+	if len(moving) == 0 || anchor == nil {
 		return instrs
 	}
-
-	ins := instrs[from]
-	// Remove.
-	copy(instrs[from:], instrs[from+1:])
-	instrs = instrs[:len(instrs)-1]
-
-	// Recompute insertion index after removal.
-	if to > from {
-		to--
-	}
-	if to < 0 {
-		to = 0
-	}
-	if to > len(instrs) {
-		to = len(instrs)
+	moved := make([]ssa.Instruction, 0, len(moving))
+	remaining := make([]ssa.Instruction, 0, len(instrs))
+	for _, instr := range instrs {
+		if _, ok := moving[instr]; ok {
+			moved = append(moved, instr)
+			continue
+		}
+		remaining = append(remaining, instr)
+	}
+	for i, instr := range remaining {
+		if instr == anchor {
+			ret := make([]ssa.Instruction, 0, len(instrs))
+			ret = append(ret, remaining[:i+1]...)
+			ret = append(ret, moved...)
+			ret = append(ret, remaining[i+1:]...)
+			return ret
+		}
 	}
-
-	// Insert.
-	instrs = append(instrs, nil)
-	copy(instrs[to+1:], instrs[to:])
-	instrs[to] = ins
 	return instrs
 }

internal/build/ssa_order_fix_test.go

@@ -11,6 +11,7 @@ import (
 	"strings"
 	"testing"
 
+	"golang.org/x/tools/go/packages"
 	"golang.org/x/tools/go/ssa"
 	"golang.org/x/tools/go/ssa/ssautil"
 )
@@ -28,11 +29,12 @@ func f() {
 	case *fp(&x, 100) = <-fc(c, 1):
 	}
 }`
-	fn := buildSSAOrderTestPackage(t, src)
-	got := instrOrder(fn, "fc(", "<-", "fp(", "*t")
-	if !inOrder(got, "fc(", "<-", "fp(") {
-		t.Fatalf("single-case select receive assignment order = %v, want fc/receive before fp", got)
-	}
+	testSSAOrderModes(t, src, func(t *testing.T, fn *ssa.Function) {
+		got := instrOrder(fn, "fc(", "<-", "fp(", "*t")
+		if !inOrder(got, "fc(", "<-", "fp(") {
+			t.Fatalf("single-case select receive assignment order = %v, want fc/receive before fp", got)
+		}
+	})
 }
 
 func TestFixSSAOrderPlainRecvAssignKeepsLeftToRight(t *testing.T) {
@@ -66,11 +68,12 @@ func f() {
 	case m[fn(13, 100)] = <-fc(c, 1):
 	}
 }`
-	fn := buildSSAOrderTestPackage(t, src)
-	got := instrOrder(fn, "fc(", "<-", "fn(")
-	if !inOrder(got, "fc(", "<-", "fn(") {
-		t.Fatalf("single-case select map receive assignment order = %v, want fc/receive before fn", got)
-	}
+	testSSAOrderModes(t, src, func(t *testing.T, fn *ssa.Function) {
+		got := instrOrder(fn, "fc(", "<-", "fn(")
+		if !inOrder(got, "fc(", "<-", "fn(") {
+			t.Fatalf("single-case select map receive assignment order = %v, want fc/receive before fn", got)
+		}
+	})
 }
 
 func TestFixSSAOrderSingleCaseSelectTwoValueRecv(t *testing.T) {
@@ -87,11 +90,12 @@ func f() {
 	case *fp(&x, 100), ok = <-fc(c, 1):
 	}
 }`
-	fn := buildSSAOrderTestPackage(t, src)
-	got := instrOrder(fn, "fc(", "<-", "fp(", "*t")
-	if !inOrder(got, "fc(", "<-", "fp(") {
-		t.Fatalf("single-case select two-value receive assignment order = %v, want fc/receive before fp", got)
-	}
+	testSSAOrderModes(t, src, func(t *testing.T, fn *ssa.Function) {
+		got := instrOrder(fn, "fc(", "<-", "fp(", "*t")
+		if !inOrder(got, "fc(", "<-", "fp(") {
+			t.Fatalf("single-case select two-value receive assignment order = %v, want fc/receive before fp", got)
+		}
+	})
 }
 
 func TestFixSSAOrderMultiCaseSelectKeepsLeftToRight(t *testing.T) {
@@ -115,7 +119,161 @@ func f() {
 	}
 }
 
+func TestFixSSAOrderReturnLoadWithDebugRefs(t *testing.T) {
+	const src = `package p
+type value struct { n int }
+func (v *value) mutate() bool { v.n = 1; return true }
+func f() (value, bool) {
+	var v value
+	return v, v.mutate()
+}`
+	base := ssa.SanityCheckFunctions | ssa.InstantiateGenerics
+	for _, test := range []struct {
+		name string
+		mode ssa.BuilderMode
+	}{
+		{name: "default", mode: base},
+		{name: "global-debug", mode: base | ssa.GlobalDebug},
+	} {
+		t.Run(test.name, func(t *testing.T) {
+			fn := buildSSAOrderTestPackageMode(t, src, test.mode)
+			checkReturnLoadAfterMutation(t, fn, "mutate")
+		})
+	}
+}
+
+func TestFixSSAOrderCryptoX509ParseOID(t *testing.T) {
+	fset := token.NewFileSet()
+	loaded, err := packages.Load(&packages.Config{
+		Mode: packages.LoadSyntax,
+		Fset: fset,
+	}, "crypto/x509")
+	if err != nil {
+		t.Fatal(err)
+	}
+	if packages.PrintErrors(loaded) != 0 || len(loaded) != 1 {
+		t.Fatal("failed to load crypto/x509")
+	}
+	mode := ssa.SanityCheckFunctions | ssa.InstantiateGenerics | ssa.GlobalDebug
+	prog, ssaPackages := ssautil.Packages(loaded, mode)
+	prog.Build()
+	pkg := ssaPackages[0]
+	fixSSAOrder(pkg, loaded[0].Syntax)
+	checkReturnLoadAfterMutation(t, pkg.Func("ParseOID"), "unmarshalOIDText")
+}
+
+func TestMoveInstrsAfter(t *testing.T) {
+	const src = `package p
+func f() {
+	println(1)
+	println(2)
+	println(3)
+}`
+	fn := buildSSAOrderTestPackage(t, src)
+	instrs := fn.Blocks[0].Instrs
+	if len(instrs) < 4 {
+		t.Fatalf("instructions = %d, want at least 4", len(instrs))
+	}
+
+	assertOrder := func(t *testing.T, got, want []ssa.Instruction) {
+		t.Helper()
+		if len(got) != len(want) {
+			t.Fatalf("instruction count = %d, want %d", len(got), len(want))
+		}
+		for i := range want {
+			if got[i] != want[i] {
+				t.Fatalf("instruction %d = %v, want %v", i, got[i], want[i])
+			}
+		}
+	}
+
+	t.Run("empty", func(t *testing.T) {
+		assertOrder(t, moveInstrsAfter(instrs, nil, instrs[1]), instrs)
+	})
+	t.Run("nil-anchor", func(t *testing.T) {
+		moving := map[ssa.Instruction]struct{}{instrs[0]: {}}
+		assertOrder(t, moveInstrsAfter(instrs, moving, nil), instrs)
+	})
+	t.Run("missing-anchor", func(t *testing.T) {
+		moving := map[ssa.Instruction]struct{}{instrs[0]: {}}
+		assertOrder(t, moveInstrsAfter(instrs, moving, &ssa.Return{}), instrs)
+	})
+	t.Run("stable", func(t *testing.T) {
+		moving := map[ssa.Instruction]struct{}{
+			instrs[0]: {},
+			instrs[2]: {},
+		}
+		want := []ssa.Instruction{instrs[1], instrs[0], instrs[2]}
+		want = append(want, instrs[3:]...)
+		assertOrder(t, moveInstrsAfter(instrs, moving, instrs[1]), want)
+	})
+}
+
+func checkReturnLoadAfterMutation(t *testing.T, fn *ssa.Function, mutation string) {
+	t.Helper()
+	var ret *ssa.Return
+	var mutationCall ssa.CallInstruction
+	for _, block := range fn.Blocks {
+		for _, instr := range block.Instrs {
+			if candidate, ok := instr.(*ssa.Return); ok {
+				ret = candidate
+			}
+			if call, ok := instr.(ssa.CallInstruction); ok {
+				callee := call.Common().StaticCallee()
+				if callee != nil && callee.Name() == mutation {
+					mutationCall = call
+				}
+			}
+		}
+	}
+	if ret == nil || len(ret.Results) != 2 || mutationCall == nil {
+		t.Fatalf("unexpected return instruction: %v", ret)
+	}
+	callInstr := mutationCall.(ssa.Instruction)
+	callIdx := indexOfInstr(callInstr.Block().Instrs, callInstr)
+	loadIdx := -1
+	debugRefIdx := -1
+	var resultLoad *ssa.UnOp
+	for i, instr := range callInstr.Block().Instrs {
+		if load, ok := instr.(*ssa.UnOp); ok && load.Op == token.MUL {
+			if alloc, ok := load.X.(*ssa.Alloc); ok && callUsesValue(mutationCall, alloc) {
+				loadIdx = i
+				resultLoad = load
+			}
+		}
+		if ref, ok := instr.(*ssa.DebugRef); ok && resultLoad != nil && instrUsesValue(ref, resultLoad) {
+			debugRefIdx = i
+		}
+	}
+	if callIdx < 0 || loadIdx <= callIdx {
+		t.Fatalf("return load index = %d, mutation call index = %d; want load after call\n%s", loadIdx, callIdx, fn)
+	}
+	if debugRefIdx >= 0 && debugRefIdx <= loadIdx {
+		t.Fatalf("DebugRef index = %d, load index = %d; want DebugRef after its load\n%s", debugRefIdx, loadIdx, fn)
+	}
+}
+
 func buildSSAOrderTestPackage(t *testing.T, src string) *ssa.Function {
+	return buildSSAOrderTestPackageMode(t, src, ssa.SanityCheckFunctions|ssa.InstantiateGenerics)
+}
+
+func testSSAOrderModes(t *testing.T, src string, check func(*testing.T, *ssa.Function)) {
+	t.Helper()
+	base := ssa.SanityCheckFunctions | ssa.InstantiateGenerics
+	for _, test := range []struct {
+		name string
+		mode ssa.BuilderMode
+	}{
+		{name: "default", mode: base},
+		{name: "global-debug", mode: base | ssa.GlobalDebug},
+	} {
+		t.Run(test.name, func(t *testing.T) {
+			check(t, buildSSAOrderTestPackageMode(t, src, test.mode))
+		})
+	}
+}
+
+func buildSSAOrderTestPackageMode(t *testing.T, src string, mode ssa.BuilderMode) *ssa.Function {
 	t.Helper()
 	fset := token.NewFileSet()
 	file, err := parser.ParseFile(fset, "p.go", src, 0)
@@ -129,7 +287,7 @@ func buildSSAOrderTestPackage(t *testing.T, src string) *ssa.Function {
 		fset,
 		pkg,
 		files,
-		ssa.SanityCheckFunctions|ssa.InstantiateGenerics,
+		mode,
 	)
 	if err != nil {
 		t.Fatalf("BuildPackage: %v", err)