[DebugInfo] Don't inherit debug location of previous instruction

felipepiovezan · felipepiovezan · commit f7f869c88dfe · 2024-11-18T10:23:41.000-08:00
Prior to this commit, when lowering SIL instructions that should are "hidden" for the purposes of debugging, the compiler just attaches the location of the previous instruction in the name of keeping a simpler line table. However, this is wrong for many reasons. One such reason is this: at the start of a basic block, inheriting the previous debug location will almost certainly cause the instruction to have a random location in the code, as it will depend on whatever BB was visited previously. Other examples can be seen in the tests affect by this commit, which changes lowering to use Line 0 instead of the line number of the previous instruction. CodeView doesn't handle line 0 the same way DWARF does, so this commit preserves the old behavior for the CodeView path. The test changes here are effectively undoing some of the diffs from 158772c. rdar://139826231&110187845
diff --git a/lib/IRGen/IRGenDebugInfo.cpp b/lib/IRGen/IRGenDebugInfo.cpp
@@ -2625,8 +2625,8 @@ IRGenDebugInfoImpl::computeLLVMLocCodeView(const SILDebugScope *DS,
                                            SILLocation Loc) {
   // If the scope has not changed and the line number is either zero or
   // artificial, we want to keep the most recent debug location.
-  if (DS == LastScope &&
-      (Loc.is<ArtificialUnreachableLocation>() || Loc.isLineZero(SM)))
+  if (DS == LastScope && (Loc.is<ArtificialUnreachableLocation>() ||
+                          Loc.isLineZero(SM) || Loc.isHiddenFromDebugInfo()))
     return LastFileAndLocation;
 
   // Decode the location.
@@ -2639,11 +2639,6 @@ IRGenDebugInfoImpl::computeLLVMLoc(const SILDebugScope *DS, SILLocation Loc) {
   if (Fn && (Fn->isThunk() || Fn->isTransparent()))
     return {0, 0, CompilerGeneratedFile};
 
-  // Reuse the last source location if we are still in the same scope to get a
-  // more contiguous line table.
-  if (DS == LastScope && Loc.isHiddenFromDebugInfo())
-    return LastFileAndLocation;
-
   if (Opts.DebugInfoFormat == IRGenDebugInfoFormat::CodeView)
     return computeLLVMLocCodeView(DS, Loc);
 
diff --git a/test/Backtracing/Crash.swift b/test/Backtracing/Crash.swift
@@ -60,7 +60,7 @@ struct Crash {
 // CHECK-NEXT: 4 [ra]          0x{{[0-9a-f]+}} level1() + {{[0-9]+}} in Crash at {{.*}}/Crash.swift:24:3
 // CHECK-NEXT: 5 [ra]          0x{{[0-9a-f]+}} static Crash.main() + {{[0-9]+}} in Crash at {{.*}}/Crash.swift:48:5
 // CHECK-NEXT: 6 [ra] [system] 0x{{[0-9a-f]+}} static Crash.$main() + {{[0-9]+}} in Crash at {{.*}}/<compiler-generated>
-// CHECK-NEXT: 7 [ra] 0x{{[0-9a-f]+}} main + {{[0-9]+}} in Crash at {{.*}}/Crash.swift
+// CHECK-NEXT: 7 [ra] [system] 0x{{[0-9a-f]+}} main + {{[0-9]+}} in Crash at {{.*}}/Crash.swift
 
 // CHECK: Registers:
 
diff --git a/test/Backtracing/CrashStatic.swift b/test/Backtracing/CrashStatic.swift
@@ -51,7 +51,7 @@ struct CrashStatic {
 // CHECK-NEXT: 4 [ra]          0x{{[0-9a-f]+}} level1() + {{[0-9]+}} in CrashStatic at {{.*}}/CrashStatic.swift:15:3
 // CHECK-NEXT: 5 [ra]          0x{{[0-9a-f]+}} static CrashStatic.main() + {{[0-9]+}} in CrashStatic at {{.*}}/CrashStatic.swift:39:5
 // CHECK-NEXT: 6 [ra] [system] 0x{{[0-9a-f]+}} static CrashStatic.$main() + {{[0-9]+}} in CrashStatic at {{.*}}/<compiler-generated>
-// CHECK-NEXT: 7 [ra] 0x{{[0-9a-f]+}} main + {{[0-9]+}} in CrashStatic at {{.*}}/CrashStatic.swift
+// CHECK-NEXT: 7 [ra] [system] 0x{{[0-9a-f]+}} main + {{[0-9]+}} in CrashStatic at {{.*}}/CrashStatic.swift
 
 // CHECK: Registers:
 
diff --git a/test/Backtracing/CrashWithThunk.swift b/test/Backtracing/CrashWithThunk.swift
@@ -38,7 +38,7 @@ struct CrashWithThunk {
 // CHECK-NEXT: 1 [ra] [thunk]  0x{{[0-9a-f]+}} thunk for @escaping @callee_guaranteed () -> () + {{[0-9]+}} in CrashWithThunk at {{.*}}/<compiler-generated>
 // CHECK-NEXT: 2 [ra]          0x{{[0-9a-f]+}} static CrashWithThunk.main() + {{[0-9]+}} in CrashWithThunk at {{.*}}/CrashWithThunk.swift:29:9
 // CHECK-NEXT: 3 [ra] [system] 0x{{[0-9a-f]+}} static CrashWithThunk.$main() + {{[0-9]+}} in CrashWithThunk at {{.*}}/<compiler-generated>
-// CHECK-NEXT: 4 [ra] 0x{{[0-9a-f]+}} main + {{[0-9]+}} in CrashWithThunk at {{.*}}/CrashWithThunk.swift
+// CHECK-NEXT: 4 [ra] [system] 0x{{[0-9a-f]+}} main + {{[0-9]+}} in CrashWithThunk at {{.*}}/CrashWithThunk.swift
 
 // CHECK: Registers:
 
diff --git a/test/Backtracing/FatalError.swift b/test/Backtracing/FatalError.swift
@@ -51,4 +51,4 @@ struct FatalError {
 // CHECK-NEXT: 5 [ra]          0x{{[0-9a-f]+}} level1() + {{[0-9]+}} in FatalError at {{.*}}/FatalError.swift:14:3
 // CHECK-NEXT: 6 [ra]          0x{{[0-9a-f]+}} static FatalError.main() + {{[0-9]+}} in FatalError at {{.*}}/FatalError.swift:36:5
 // CHECK-NEXT: 7 [ra] [system] 0x{{[0-9a-f]+}} static FatalError.$main() + {{[0-9]+}} in FatalError at {{.*}}/<compiler-generated>
-// CHECK-NEXT: 8 [ra] 0x{{[0-9a-f]+}} main + {{[0-9]+}} in FatalError at {{.*}}/FatalError.swift
+// CHECK-NEXT: 8 [ra] [system] 0x{{[0-9a-f]+}} main + {{[0-9]+}} in FatalError at {{.*}}/FatalError.swift
diff --git a/test/Backtracing/Overflow.swift b/test/Backtracing/Overflow.swift
@@ -52,7 +52,7 @@ struct Overflow {
 // CHECK-NEXT: 5 [ra]               0x{{[0-9a-f]+}} level1() + {{[0-9]+}} in Overflow at {{.*}}/Overflow.swift:15:3
 // CHECK-NEXT: 6 [ra]               0x{{[0-9a-f]+}} static Overflow.main() + {{[0-9]+}} in Overflow at {{.*}}/Overflow.swift:39:5
 // CHECK-NEXT: 7 [ra] [system]      0x{{[0-9a-f]+}} static Overflow.$main() + {{[0-9]+}} in Overflow at {{.*}}/<compiler-generated>
-// CHECK-NEXT: 8 [ra]               0x{{[0-9a-f]+}} main + {{[0-9]+}} in Overflow at {{.*}}/Overflow.swift
+// CHECK-NEXT: 8 [ra] [system]      0x{{[0-9a-f]+}} main + {{[0-9]+}} in Overflow at {{.*}}/Overflow.swift
 
 // CHECK: Registers:
 
diff --git a/test/Backtracing/StackOverflow.swift b/test/Backtracing/StackOverflow.swift
@@ -100,7 +100,7 @@ struct StackOverflow {
 
 // CHECK: {{[0-9]+}} [ra]          0x{{[0-9a-f]+}} static StackOverflow.main() + {{[0-9]+}} in StackOverflow at {{.*}}/StackOverflow.swift:25:5
 // CHECK-NEXT: {{[0-9]+}} [ra] [system] 0x{{[0-9a-f]+}} static StackOverflow.$main() + {{[0-9]+}} in StackOverflow at {{.*}}/<compiler-generated>
-// CHECK-NEXT: {{[0-9]+}} [ra]          0x{{[0-9a-f]+}} main + {{[0-9]+}} in StackOverflow at {{.*}}/StackOverflow.swift
+// CHECK-NEXT: {{[0-9]+}} [ra] [system] 0x{{[0-9a-f]+}} main + {{[0-9]+}} in StackOverflow at {{.*}}/StackOverflow.swift
 
 // CHECK: Registers:
 
@@ -129,7 +129,7 @@ struct StackOverflow {
 
 // LIMITED: {{[0-9]+}} [ra]          0x{{[0-9a-f]+}} static StackOverflow.main() + {{[0-9]+}} in StackOverflow at {{.*}}/StackOverflow.swift:25:5
 // LIMITED-NEXT: {{[0-9]+}} [ra] [system] 0x{{[0-9a-f]+}} static StackOverflow.$main() + {{[0-9]+}} in StackOverflow at {{.*}}/<compiler-generated>
-// LIMITED-NEXT: {{[0-9]+}} [ra]          0x{{[0-9a-f]+}} main + {{[0-9]+}} in StackOverflow at {{.*}}/StackOverflow.swift
+// LIMITED-NEXT: {{[0-9]+}} [ra] [system] 0x{{[0-9a-f]+}} main + {{[0-9]+}} in StackOverflow at {{.*}}/StackOverflow.swift
 
 // FRIENDLY: *** Program crashed: Bad pointer dereference at 0x{{[0-9a-f]+}} ***
 
diff --git a/test/Backtracing/StaticBacktracer.swift b/test/Backtracing/StaticBacktracer.swift
@@ -52,7 +52,7 @@ struct StaticBacktracer {
 // CHECK-NEXT: 4 [ra]          0x{{[0-9a-f]+}} level1() + {{[0-9]+}} in StaticBacktracer at {{.*}}/StaticBacktracer.swift:16:3
 // CHECK-NEXT: 5 [ra]          0x{{[0-9a-f]+}} static StaticBacktracer.main() + {{[0-9]+}} in StaticBacktracer at {{.*}}/StaticBacktracer.swift:40:5
 // CHECK-NEXT: 6 [ra] [system] 0x{{[0-9a-f]+}} static StaticBacktracer.$main() + {{[0-9]+}} in StaticBacktracer at {{.*}}/<compiler-generated>
-// CHECK-NEXT: 7 [ra] 0x{{[0-9a-f]+}} main + {{[0-9]+}} in StaticBacktracer at {{.*}}/StaticBacktracer.swift
+// CHECK-NEXT: 7 [ra] [system] 0x{{[0-9a-f]+}} main + {{[0-9]+}} in StaticBacktracer at {{.*}}/StaticBacktracer.swift
 
 // CHECK: Registers:
 
diff --git a/test/DebugInfo/async-let.swift b/test/DebugInfo/async-let.swift
@@ -17,7 +17,7 @@ public actor Alice {
   // CHECK:   load ptr, ptr {{.*}} !dbg ![[LET_HOP0:[0-9]+]]
 
   // CHECK: define {{.*}}$s1M5AliceC4callyyYaFSiyYaYbcfu_TY2_{{.*}} !dbg ![[LET_SCOPE1:[0-9]+]]
-  // CHECK:   load ptr, ptr {{.*}} !dbg ![[LET_HOP1:[0-9]+]]
+  // CHECK:   store i64 %{{.*}} !dbg ![[LET_HOP1:[0-9]+]]
   public func call() async {
     // CHECK: ![[SCOPE0]] = distinct !DISubprogram({{.*}}line: [[@LINE-1]]
     // CHECK: ![[HOP0]] = !DILocation(line: [[@LINE+1]], column: 11
diff --git a/test/DebugInfo/autoclosure.swift b/test/DebugInfo/autoclosure.swift
@@ -19,11 +19,10 @@ func &&&&&(lhs: Bool, rhs: @autoclosure () -> Bool) -> Bool {
 }
 
 func call_me(_ input: Int64) -> Void {
-// rdar://problem/14627460
 // An autoclosure should have a line number in the debug info and a scope line of 0.
 // CHECK-DAG: !DISubprogram({{.*}}linkageName: "$s11autoclosure7call_meyys5Int64VFSbyXEfu_",{{.*}} spFlags: DISPFlagLocalToUnit | DISPFlagDefinition
-// But not in the line table.
-// CHECK-DAG: ![[DBG]] = !DILocation(line: [[@LINE+1]],
+// Instructions setting up the closure should have a line number of 0.
+// CHECK-DAG: ![[DBG]] = !DILocation(line: 0,
   if input != 0 &&&&& ( get_truth (input * 2 + 1) > 0 ) {
   }
 
diff --git a/test/DebugInfo/callexpr.swift b/test/DebugInfo/callexpr.swift
@@ -19,10 +19,8 @@ markUsed(r)
 struct MyType {}
 func bar(x: MyType = MyType()) {}
 
-// Room for improvement:
-// Because the default argument is implicit it inherits the previous source location.
 // CHECK2: call {{.*}}MyType{{.*}}, !dbg ![[DEFAULTARG:.*]]
 // CHECK2: call {{.*}}bar{{.*}}, !dbg ![[BARCALL:.*]]
-bar() // CHECK2: ![[DEFAULTARG]] = !DILocation(line: [[@LINE-9]]
+bar() // CHECK2: ![[DEFAULTARG]] = !DILocation(line: 0
       // CHECK2: ![[BARCALL]] = !DILocation(line: [[@LINE-1]], column: 1
 
diff --git a/test/DebugInfo/returnlocation.swift b/test/DebugInfo/returnlocation.swift
@@ -184,9 +184,10 @@ public class Class1 {
   public required init?() {
     print("hello")
     // CHECK_INIT: call {{.*}}@"$ss5print_9separator10terminatoryypd_S2StF"{{.*}}, !dbg ![[PRINTLOC:[0-9]+]]
-    // FIXME: Why doesn't ret have the correct line number?
-    // CHECK_INIT: ret i{{32|64}} 0, !dbg ![[PRINTLOC]]
+    // FIXME: ret has an incorrect line number because it is generated with "isHiddenFromDebugInfo"
+    // CHECK_INIT: ret i{{32|64}} 0, !dbg ![[LINE_0:[0-9]+]]
     // CHECK_INIT-DAG: [[PRINTLOC]] = !DILocation(line: [[@LINE-4]]
+    // CHECK_INIT-DAG: [[LINE_0]] = !DILocation(line: 0
     return nil
   }
 }

Original file line number	Diff line number	Diff line change
`@@ -19,11 +19,10 @@ func &&&&&(lhs: Bool, rhs: @autoclosure () -> Bool) -> Bool {`
`19`	`19`	`}`
`20`	`20`
`21`	`21`	`func call_me(_ input: Int64) -> Void {`
`22`		`-// rdar://problem/14627460`
`23`	`22`	`// An autoclosure should have a line number in the debug info and a scope line of 0.`
`24`	`23`	`// CHECK-DAG: !DISubprogram({{.}}linkageName: "$s11autoclosure7call_meyys5Int64VFSbyXEfu_",{{.}} spFlags: DISPFlagLocalToUnit \| DISPFlagDefinition`
`25`		`-// But not in the line table.`
`26`		`-// CHECK-DAG: ![[DBG]] = !DILocation(line: [[@LINE+1]],`
	`24`	`+// Instructions setting up the closure should have a line number of 0.`
	`25`	`+// CHECK-DAG: ![[DBG]] = !DILocation(line: 0,`
`27`	`26`	`if input != 0 &&&&& ( get_truth (input * 2 + 1) > 0 ) {`
`28`	`27`	`}`
`29`	`28`