[DebugInfo] Improve stepping behavior for switch case stmts

vedantk · vedantk · commit f9372fdb621d · 2018-03-12T15:21:46.000-07:00
Assign the location of a switch statement's subject expression to all of
its case statements.

This improves the debugger's stepping behavior in switch statements.
Stepping into a switch now goes directly to the first matching case
(possibly one with a `where` clause that may or may not match). It's
still possible to set breakpoints within `where` clauses.

rdar://35628672
diff --git a/include/swift/SIL/SILBuilder.h b/include/swift/SIL/SILBuilder.h
@@ -47,6 +47,7 @@ class SILBuilder {
   SILBasicBlock *BB;
   SILBasicBlock::iterator InsertPt;
   const SILDebugScope *CurDebugScope = nullptr;
+  Optional<SILLocation> CurDebugLocOverride = None;
 
   /// If this pointer is non-null, then any inserted instruction is
   /// recorded in this list.
@@ -141,14 +142,27 @@ class SILBuilder {
   void setCurrentDebugScope(const SILDebugScope *DS) { CurDebugScope = DS; }
   const SILDebugScope *getCurrentDebugScope() const { return CurDebugScope; }
 
+  /// Apply a debug location override. If loc is None, the current override is
+  /// removed. Otherwise, newly created debug locations use the given location.
+  /// Note: the override location does not apply to debug_value[_addr].
+  void applyDebugLocOverride(Optional<SILLocation> loc) {
+    CurDebugLocOverride = loc;
+  }
+
+  /// Get the current debug location override.
+  Optional<SILLocation> getCurrentDebugLocOverride() const {
+    return CurDebugLocOverride;
+  }
+
   /// Convenience function for building a SILDebugLocation.
   SILDebugLocation getSILDebugLocation(SILLocation Loc) {
     // FIXME: Audit all uses and enable this assertion.
     // assert(getCurrentDebugScope() && "no debug scope");
     auto Scope = getCurrentDebugScope();
     if (!Scope && F)
         Scope = F->getDebugScope();
-    return SILDebugLocation(Loc, Scope);
+    auto overriddenLoc = CurDebugLocOverride ? *CurDebugLocOverride : Loc;
+    return SILDebugLocation(overriddenLoc, Scope);
   }
 
   //===--------------------------------------------------------------------===//
@@ -733,16 +747,9 @@ class SILBuilder {
   }
 
   DebugValueInst *createDebugValue(SILLocation Loc, SILValue src,
-                                   SILDebugVariable Var) {
-    return insert(DebugValueInst::create(getSILDebugLocation(Loc), src,
-                                         getModule(), Var));
-  }
-  DebugValueAddrInst *
-  createDebugValueAddr(SILLocation Loc, SILValue src,
-                       SILDebugVariable Var) {
-    return insert(DebugValueAddrInst::create(getSILDebugLocation(Loc), src,
-                                             getModule(), Var));
-  }
+                                   SILDebugVariable Var);
+  DebugValueAddrInst *createDebugValueAddr(SILLocation Loc, SILValue src,
+                                           SILDebugVariable Var);
 
   LoadWeakInst *createLoadWeak(SILLocation Loc, SILValue src, IsTake_t isTake) {
     return insert(new (getModule())
@@ -2095,6 +2102,35 @@ class SavedInsertionPointRAII {
   }
 };
 
+/// Apply a debug location override for the duration of the current scope.
+class DebugLocOverrideRAII {
+  SILBuilder &Builder;
+  Optional<SILLocation> oldOverride;
+#ifndef NDEBUG
+  Optional<SILLocation> installedOverride;
+#endif
+
+public:
+  DebugLocOverrideRAII(SILBuilder &B, Optional<SILLocation> Loc) : Builder(B) {
+    oldOverride = B.getCurrentDebugLocOverride();
+    Builder.applyDebugLocOverride(Loc);
+#ifndef NDEBUG
+    installedOverride = Loc;
+#endif
+  }
+
+  ~DebugLocOverrideRAII() {
+    assert(Builder.getCurrentDebugLocOverride() == installedOverride &&
+           "Restoring debug location override to an unexpected state");
+    Builder.applyDebugLocOverride(oldOverride);
+  }
+
+  DebugLocOverrideRAII(const DebugLocOverrideRAII &) = delete;
+  DebugLocOverrideRAII &operator=(const DebugLocOverrideRAII &) = delete;
+  DebugLocOverrideRAII(DebugLocOverrideRAII &&) = delete;
+  DebugLocOverrideRAII &operator=(DebugLocOverrideRAII &&) = delete;
+};
+
 } // end swift namespace
 
 #endif
diff --git a/lib/SIL/SILBuilder.cpp b/lib/SIL/SILBuilder.cpp
@@ -521,3 +521,20 @@ void SILBuilder::emitShallowDestructureAddressOperation(
               return P.createAddressProjection(*this, Loc, V).get();
             });
 }
+
+DebugValueInst *SILBuilder::createDebugValue(SILLocation Loc, SILValue src,
+                                             SILDebugVariable Var) {
+  // Debug location overrides cannot apply to debug value instructions.
+  DebugLocOverrideRAII LocOverride{*this, None};
+  return insert(
+      DebugValueInst::create(getSILDebugLocation(Loc), src, getModule(), Var));
+}
+
+DebugValueAddrInst *SILBuilder::createDebugValueAddr(SILLocation Loc,
+                                                     SILValue src,
+                                                     SILDebugVariable Var) {
+  // Debug location overrides cannot apply to debug addr instructions.
+  DebugLocOverrideRAII LocOverride{*this, None};
+  return insert(DebugValueAddrInst::create(getSILDebugLocation(Loc), src,
+                                           getModule(), Var));
+}
diff --git a/lib/SILGen/SILGenPattern.cpp b/lib/SILGen/SILGenPattern.cpp
@@ -421,6 +421,13 @@ class PatternMatchEmission {
     : SGF(SGF), PatternMatchStmt(S),
       CompletionHandler(completionHandler) {}
 
+  Optional<SILLocation> getSubjectLocationOverride(SILLocation loc) const {
+    if (auto *Switch = dyn_cast<SwitchStmt>(PatternMatchStmt))
+      if (!Switch->isImplicit())
+        return SILLocation(Switch->getSubjectExpr());
+    return None;
+  }
+
   void emitDispatch(ClauseMatrix &matrix, ArgArray args,
                     const FailureHandler &failure);
 
@@ -1144,6 +1151,7 @@ void PatternMatchEmission::bindRefutablePattern(Pattern *pattern,
 void PatternMatchEmission::bindExprPattern(ExprPattern *pattern,
                                            ConsumableManagedValue value,
                                            const FailureHandler &failure) {
+  DebugLocOverrideRAII LocOverride{SGF.B, getSubjectLocationOverride(pattern)};
   FullExpr scope(SGF.Cleanups, CleanupLocation(pattern));
   bindVariable(pattern, pattern->getMatchVar(), value,
                pattern->getType()->getCanonicalType(),
diff --git a/test/SILGen/sil_locations.swift b/test/SILGen/sil_locations.swift
@@ -129,9 +129,9 @@ func testSwitch() {
   var x:Int
   x = 0
   switch (switchfoo(), switchbar()) {
-  // CHECK: store {{.*}}, loc "{{.*}}":[[@LINE+1]]
+  // CHECK: store {{.*}}, loc "{{.*}}":[[@LINE-1]]
   case (1,2):
-  // CHECK: integer_literal $Builtin.Int2048, 2, loc "{{.*}}":[[@LINE-1]]:11
+  // CHECK: integer_literal $Builtin.Int2048, 2, loc "{{.*}}":[[@LINE-3]]:10
   // FIXME: Location info is missing.
   // CHECK: cond_br
   //
diff --git a/test/SILGen/switch_debuginfo.swift b/test/SILGen/switch_debuginfo.swift
@@ -0,0 +1,104 @@
+// RUN: %target-swift-frontend -g -Xllvm -sil-print-debuginfo -emit-silgen %s | %FileCheck %s
+
+func nop1() {}
+func nop2() {}
+
+enum Binary {
+  case On
+  case Off
+}
+
+func isOn(_ b: Binary) -> Bool {
+  return b == .On
+}
+
+// CHECK: [[LOC:loc "[^"]+"]]
+
+// First, check that we don't assign fresh locations to each case statement,
+// except for any relevant debug value instructions.
+// CHECK-LABEL: sil hidden @$S16switch_debuginfo5test11iySi_tF
+func test1(i: Int) {
+  switch i {
+           // CHECK-NOT: [[LOC]]:[[@LINE+1]]
+  case 0:  // CHECK: debug_value {{.*}} : $Int, let, name "$match", [[LOC]]:[[@LINE]]
+           // CHECK-NOT: [[LOC]]:[[@LINE-1]]
+    nop1()
+
+           // CHECK-NOT: [[LOC]]:[[@LINE+1]]
+  case 1:  // CHECK: debug_value {{.*}} : $Int, let, name "$match", [[LOC]]:[[@LINE]]
+           // CHECK-NOT: [[LOC]]:[[@LINE-1]]
+    nop1()
+
+  default: // CHECK-NOT: [[LOC]]:[[@LINE]]
+    nop1()
+  }
+}
+
+// Next, check that case statements and switch subjects have the same locations.
+// CHECK-LABEL: sil hidden @$S16switch_debuginfo5test21sySS_tF
+func test2(s: String) {
+  switch s {
+  case "a": // CHECK: string_literal utf8 "a", [[LOC]]:[[@LINE-1]]:10
+    nop1()
+  case "b": // CHECK: string_literal utf8 "b", [[LOC]]:[[@LINE-3]]:10
+    nop2()
+  default:
+    nop1()
+  }
+}
+
+// Fallthrough shouldn't affect case statement locations.
+// CHECK-LABEL: sil hidden @$S16switch_debuginfo5test31sySS_tF
+func test3(s: String) {
+  switch s {
+  case "a", "b":
+    // CHECK: string_literal utf8 "a", [[LOC]]:[[@LINE-2]]:10
+    // CHECK: string_literal utf8 "b", [[LOC]]:[[@LINE-3]]:10
+    nop1()
+    fallthrough
+  case "b", "c":
+    // CHECK: string_literal utf8 "b", [[LOC]]:[[@LINE-7]]:10
+    // CHECK: string_literal utf8 "c", [[LOC]]:[[@LINE-8]]:10
+    nop2()
+    fallthrough
+  default:
+    nop1()
+  }
+}
+
+// It should be possible to set breakpoints on where clauses.
+// CHECK-LABEL: sil hidden @$S16switch_debuginfo5test41byAA6BinaryO_tF
+func test4(b: Binary) {
+  switch b {
+  case let _        // CHECK-NOT: [[LOC]]:[[@LINE]]
+    where isOn(b):  // CHECK: [[LOC]]:[[@LINE]]:11
+    nop1()
+  case let _        // CHECK-NOT: [[LOC]]:[[@LINE]]
+    where !isOn(b): // CHECK: [[LOC]]:[[@LINE]]:11
+    nop2()
+  default:
+    nop1()
+  }
+}
+
+// Check that we set the right locations before/after nested switches.
+// CHECK-LABEL: sil hidden @$S16switch_debuginfo5test51sySS_tF
+func test5(s: String) {
+  switch s {
+  case "a":         // CHECK: string_literal utf8 "a", [[LOC]]:[[@LINE-1]]:10
+    switch "b" {
+    case "b":       // CHECK: string_literal utf8 "b", [[LOC]]:[[@LINE-1]]:12
+      nop1()
+    default:
+      nop2()
+    }
+    if "c" == "c" { // CHECK: string_literal utf8 "c", [[LOC]]:[[@LINE]]
+      nop1()
+    }
+  default:
+    nop1()
+  }
+  if "d" == "d" {   // CHECK: string_literal utf8 "d", [[LOC]]:[[@LINE]]
+    nop1()
+  }
+}
