Merge pull request #2143 from cpunion/codex/dwarf-opt-pipeline

debug: keep optimization and SSA rewrites DWARF-safe

Author: xushiwei

cl/rewrite_internal_test.go

@@ -31,14 +31,22 @@ func compileWithRewrites(t *testing.T, src string, rewrites map[string]string) s
 }
 
 func compileWithRewritesTarget(t *testing.T, src string, rewrites map[string]string, target *llssa.Target) string {
+	return compileWithRewritesModeTarget(t, src, rewrites,
+		ssa.SanityCheckFunctions|ssa.InstantiateGenerics, target)
+}
+
+func compileWithRewritesMode(t *testing.T, src string, rewrites map[string]string, mode ssa.BuilderMode) string {
+	return compileWithRewritesModeTarget(t, src, rewrites, mode, nil)
+}
+
+func compileWithRewritesModeTarget(t *testing.T, src string, rewrites map[string]string, mode ssa.BuilderMode, target *llssa.Target) string {
 	t.Helper()
 	fset := token.NewFileSet()
 	file, err := parser.ParseFile(fset, "rewrite.go", src, parser.ParseComments)
 	if err != nil {
 		t.Fatalf("parse failed: %v", err)
 	}
 	importer := gpackages.NewImporter(fset)
-	mode := ssa.SanityCheckFunctions | ssa.InstantiateGenerics
 	pkg, _, err := ssautil.BuildPackage(&types.Config{Importer: importer}, fset,
 		types.NewPackage(file.Name.Name, file.Name.Name), []*ast.File{file}, mode)
 	if err != nil {
@@ -234,6 +242,75 @@ func Use() callbackType { return CallbackTypes[1] }
 	}
 }
 
+func TestStaticGlobalSliceLiteralInitWithDebugRefs(t *testing.T) {
+	const src = `package staticinit
+
+var CallbackTypes = []string{"BeforeCreate", "AfterCreate"}
+
+func Use() string { return CallbackTypes[1] }
+`
+	ir := compileWithRewritesMode(t, src, nil,
+		ssa.SanityCheckFunctions|ssa.InstantiateGenerics|ssa.GlobalDebug)
+	for _, want := range []string{
+		`@"staticinit.CallbackTypes$data" = global [2 x %"github.com/goplus/llgo/runtime/internal/runtime.String"]`,
+		`@staticinit.CallbackTypes = global %"github.com/goplus/llgo/runtime/internal/runtime.Slice" { ptr @"staticinit.CallbackTypes$data", i64 2, i64 2 }`,
+		`c"BeforeCreate"`,
+		`c"AfterCreate"`,
+	} {
+		if !strings.Contains(ir, want) {
+			t.Fatalf("missing static slice initializer %q with debug refs:\n%s", want, ir)
+		}
+	}
+	assertNoStoreToGlobal(t, ir, "@staticinit.CallbackTypes")
+	if strings.Contains(ir, "runtime.AllocZ") {
+		t.Fatalf("static slice initializer allocates at runtime with debug refs:\n%s", ir)
+	}
+}
+
+func TestStaticSliceInitRejectsExecutableReferrers(t *testing.T) {
+	const src = `package foo
+
+var Values []int
+
+func useSlice([]int) {}
+func usePointer(*int) {}
+
+func sliceUser() {
+	backing := [2]int{1, 2}
+	values := backing[:]
+	Values = values
+	useSlice(values)
+}
+
+func elementUser() {
+	var backing [2]int
+	elem := &backing[0]
+	*elem = 1
+	usePointer(elem)
+	Values = backing[:]
+}
+`
+	ssapkg := buildSSAPackage(t, src)
+	global := ssapkg.Members["Values"].(*ssa.Global)
+	for _, name := range []string{"sliceUser", "elementUser"} {
+		fn := ssapkg.Func(name)
+		var globalStore *ssa.Store
+		for _, block := range fn.Blocks {
+			for _, instr := range block.Instrs {
+				if store, ok := instr.(*ssa.Store); ok && store.Addr == global {
+					globalStore = store
+				}
+			}
+		}
+		if globalStore == nil {
+			t.Fatalf("%s: store to Values not found", name)
+		}
+		if _, ok := staticSliceInitOf(globalStore); ok {
+			t.Fatalf("%s: static slice init accepted an executable referrer", name)
+		}
+	}
+}
+
 func TestStaticGlobalZeroSizedSliceLiteralFallsBack(t *testing.T) {
 	const src = `package staticinit
 

cl/static_init.go

@@ -209,16 +209,16 @@ func staticSliceInitOf(store *ssa.Store) (*staticSliceInit, bool) {
 		values: make(map[int]*ssa.Const),
 		instrs: []ssa.Instruction{alloc, slice, store},
 	}
-	sliceRefs := slice.Referrers()
-	if sliceRefs == nil || len(*sliceRefs) != 1 || (*sliceRefs)[0] != store {
+	sliceRefs, ok := nonDebugReferrers(slice)
+	if !ok || len(sliceRefs) != 1 || sliceRefs[0] != store {
 		return nil, false
 	}
-	refs := alloc.Referrers()
-	if refs == nil {
+	refs, ok := nonDebugReferrers(alloc)
+	if !ok {
 		return nil, false
 	}
 	seenSlice := false
-	for _, ref := range *refs {
+	for _, ref := range refs {
 		switch ref := ref.(type) {
 		case *ssa.Slice:
 			if ref != slice || seenSlice {
@@ -233,11 +233,11 @@ func staticSliceInitOf(store *ssa.Store) (*staticSliceInit, bool) {
 			if !ok || index >= int(array.Len()) {
 				return nil, false
 			}
-			indexRefs := ref.Referrers()
-			if indexRefs == nil || len(*indexRefs) != 1 {
+			indexRefs, ok := nonDebugReferrers(ref)
+			if !ok || len(indexRefs) != 1 {
 				return nil, false
 			}
-			elemStore, ok := (*indexRefs)[0].(*ssa.Store)
+			elemStore, ok := indexRefs[0].(*ssa.Store)
 			if !ok || elemStore.Addr != ref {
 				return nil, false
 			}

internal/build/build.go

@@ -591,17 +591,15 @@ func Build(inv Invocation) ([]Package, error) {
 
 	buildMode := ssaBuildMode
 	cabiOptimize := true
-	passOpt := true
-	if emitDebugInfo || mode == ModeGen {
-		passOpt = false
-	}
+	passOpt := shouldRunLLVMPasses(mode)
 	if emitDebugInfo {
 		buildMode |= ssa.GlobalDebug
 		cabiOptimize = false
 	}
 	if !IsOptimizeEnabled() {
 		buildMode |= ssa.NaiveForm
 	}
+	prog.SetDebugInfoOptimized(passOpt && conf.OptLevel != optlevel.O0)
 	progSSA := ssa.NewProgram(initial[0].Fset, buildMode)
 	patches := make(cl.Patches, len(altPkgPaths))
 	altEntries := registerAltSSAPkgs(progSSA, patches, altPkgs[1:], conf, verbose)
@@ -2618,6 +2616,10 @@ func llvmPassPipeline(level optlevel.Level, ltoMode lto.Mode) string {
 	}
 }
 
+func shouldRunLLVMPasses(mode Mode) bool {
+	return mode != ModeGen
+}
+
 func IsWasiThreadsEnabled() bool {
 	return isEnvOn(llgoWasiThreads, true)
 }

internal/build/optlevel_test.go

@@ -71,3 +71,14 @@ func TestLLVMPassPipeline(t *testing.T) {
 		}
 	}
 }
+
+func TestShouldRunLLVMPasses(t *testing.T) {
+	for _, mode := range []Mode{ModeBuild, ModeInstall, ModeRun, ModeTest, ModeCmpTest} {
+		if !shouldRunLLVMPasses(mode) {
+			t.Errorf("shouldRunLLVMPasses(%v) = false, want true", mode)
+		}
+	}
+	if shouldRunLLVMPasses(ModeGen) {
+		t.Fatal("shouldRunLLVMPasses(ModeGen) = true, want false")
+	}
+}

internal/build/ssa_order_fix.go

@@ -21,9 +21,10 @@ import (
 // the first return value as a load before the call, which makes o appear unchanged
 // to the return value in our backend.
 //
-// This pass moves loads of local allocs used only for the final Return results
-// to after any intervening calls that use the same alloc pointer, matching the
-// behavior of the Go compiler for the stdlib cases we rely on (e.g. crypto/x509.ParseOID).
+// This pass moves loads of local allocs that feed a Return result and have no
+// intervening executable use before that Return to after any intervening calls
+// that use the same alloc pointer, matching the behavior of the Go compiler for
+// the stdlib cases we rely on (e.g. crypto/x509.ParseOID).
 func fixSSAOrder(pkg *ssa.Package, files []*ast.File) {
 	if pkg == nil {
 		return
@@ -186,7 +187,11 @@ func moveAssignDepsAfterRecv(b *ssa.BasicBlock, roots []ssa.Value, recv ssa.Valu
 	if len(move) == 0 {
 		return false
 	}
-	if moveWouldBreakSSA(b.Instrs, move, recvIdx) {
+	// Metadata uses must follow the definitions they describe rather than
+	// blocking an otherwise safe source-order repair.
+	moved := movedValuesForIndices(b.Instrs, move)
+	includeDebugRefsForMovedValues(b.Instrs, move, moved, 0, recvIdx)
+	if moveWouldBreakSSA(b.Instrs, move, recvIdx, moved) {
 		return false
 	}
 	deps := make([]ssa.Instruction, 0, len(move))
@@ -205,13 +210,47 @@ func moveAssignDepsAfterRecv(b *ssa.BasicBlock, roots []ssa.Value, recv ssa.Valu
 	return true
 }
 
-func moveWouldBreakSSA(instrs []ssa.Instruction, move map[int]struct{}, recvIdx int) bool {
+func movedValuesForIndices(instrs []ssa.Instruction, move map[int]struct{}) map[ssa.Value]struct{} {
 	moved := make(map[ssa.Value]struct{}, len(move))
 	for i := range move {
+		if i < 0 || i >= len(instrs) {
+			continue
+		}
 		if v, ok := instrs[i].(ssa.Value); ok && v != nil {
 			moved[v] = struct{}{}
 		}
 	}
+	return moved
+}
+
+// includeDebugRefsForMovedValues adds metadata-only uses of moved values to
+// move. The moved value set is supplied by the caller so the same set can be
+// reused by the subsequent SSA safety check without rescanning move.
+func includeDebugRefsForMovedValues(instrs []ssa.Instruction, move map[int]struct{}, moved map[ssa.Value]struct{}, from, through int) {
+	if from < 0 {
+		from = 0
+	}
+	if through > len(instrs) {
+		through = len(instrs)
+	}
+	for i := from; i < through; 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, moved map[ssa.Value]struct{}) bool {
 	for i := 0; i <= recvIdx && i < len(instrs); i++ {
 		if _, moving := move[i]; moving {
 			continue
@@ -302,11 +341,20 @@ 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.
+		movingIndices := map[int]struct{}{loadIdx: {}}
+		moved := movedValuesForIndices(b.Instrs, movingIndices)
+		includeDebugRefsForMovedValues(b.Instrs, movingIndices, moved, loadIdx+1, retIdx)
+		moving := make(map[ssa.Instruction]struct{}, len(movingIndices))
+		for i := range movingIndices {
+			moving[b.Instrs[i]] = struct{}{}
+		}
 		usedBeforeReturn := false
 		for i := loadIdx + 1; i < retIdx; i++ {
+			if _, moving := movingIndices[i]; moving {
+				continue
+			}
 			if instrUsesValue(b.Instrs[i], u) {
 				usedBeforeReturn = true
 				break
@@ -316,9 +364,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 +437,34 @@ 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. The anchor must not be in moving; callers use an instruction
+// that remains in the block. It returns instrs unchanged when moving is empty,
+// or anchor is nil or absent.
+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 _, ok := moving[anchor]; ok {
+		panic("moveInstrsAfter: anchor is in moving set")
 	}
-	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
 }