Merge pull request #72576 from Snowy1803/6.0-verifier-conflicting-debug-value-types

[6.0] [SILVerifier] Add detection of conflicting debug variables

Author: adrian-prantl

include/swift/SIL/SILCloner.h

@@ -875,6 +875,8 @@ SILCloner<ImplClass>::visitAllocStackInst(AllocStackInst *Inst) {
     Loc = MandatoryInlinedLocation::getAutoGeneratedLocation();
     VarInfo = std::nullopt;
   }
+  if (VarInfo && VarInfo->Type)
+    VarInfo->Type = getOpType(*VarInfo->Type);
   auto *NewInst = getBuilder().createAllocStack(
       Loc, getOpType(Inst->getElementType()), VarInfo,
       Inst->hasDynamicLifetime(), Inst->isLexical(), Inst->isFromVarDecl(),
@@ -1408,6 +1410,8 @@ SILCloner<ImplClass>::visitDebugValueInst(DebugValueInst *Inst) {
   // Since we want the debug info to survive, we do not remap the location here.
   SILDebugVariable VarInfo = *Inst->getVarInfo();
   getBuilder().setCurrentDebugScope(getOpScope(Inst->getDebugScope()));
+  if (VarInfo.Type)
+    VarInfo.Type = getOpType(*VarInfo.Type);
   auto *NewInst = getBuilder().createDebugValue(
       Inst->getLoc(), getOpValue(Inst->getOperand()), VarInfo,
       Inst->poisonRefs(), Inst->usesMoveableValueDebugInfo(), Inst->hasTrace());

include/swift/SIL/SILLocation.h

@@ -773,4 +773,26 @@ class SILDebugLocation {
 
 } // end swift namespace
 
+namespace llvm {
+
+template<>
+struct DenseMapInfo<swift::SILLocation> {
+  static inline swift::SILLocation getEmptyKey() {
+    return swift::SILLocation::invalid();
+  }
+  static inline swift::SILLocation getTombstoneKey() {
+    return swift::SILLocation::invalid().asAutoGenerated();
+  }
+  static inline unsigned getHashValue(swift::SILLocation id) {
+    if (id.isFilenameAndLocation())
+      return hash_value(id);
+    return 0;
+  }
+  static bool isEqual(swift::SILLocation a, swift::SILLocation b) {
+    return a == b;
+  }
+};
+
+} // end namespace llvm
+
 #endif

lib/SIL/Verifier/SILVerifier.cpp

@@ -820,6 +820,11 @@ class SILVerifier : public SILVerifierBase<SILVerifier> {
   /// fix this for each of its uses.
   llvm::DenseSet<std::pair<SILValue, const Operand *>> isOperandInValueUsesCache;
 
+  /// Used for checking all equivalent variables have the same type
+  using VarID = std::tuple<const SILDebugScope *, llvm::StringRef, SILLocation>;
+  llvm::DenseMap<VarID, SILType> DebugVarTypes;
+  llvm::StringSet<> VarNames;
+
   /// Check that this operand appears in the use-chain of the value it uses.
   bool isOperandInValueUses(const Operand *operand) {
     SILValue value = operand->get();
@@ -1520,6 +1525,26 @@ class SILVerifier : public SILVerifierBase<SILVerifier> {
               "Scope of the debug variable should have the same parent function"
               " as that of instruction.");
 
+    // Check that every var info with the same name, scope and location, refer
+    // to a variable of the same type
+    llvm::StringRef UniqueName = VarNames.insert(varInfo->Name).first->getKey();
+    if (!varInfo->Loc)
+      varInfo->Loc = inst->getLoc();
+    if (!varInfo->Loc)
+      varInfo->Loc = SILLocation::invalid();
+    VarID Key(varInfo->Scope ? varInfo->Scope : debugScope,
+              UniqueName, *varInfo->Loc);
+    auto CachedVar = DebugVarTypes.insert({Key, DebugVarTy});
+    if (!CachedVar.second) {
+      auto lhs = CachedVar.first->second.removingMoveOnlyWrapper();
+      auto rhs = DebugVarTy.removingMoveOnlyWrapper();
+
+      require(lhs == rhs ||
+              (lhs.isAddress() && lhs.getObjectType() == rhs) ||
+              (DebugVarTy.isAddress() && lhs == rhs.getObjectType()),
+              "Two variables with different type but same scope!");
+    }
+
     // Check debug info expression
     if (const auto &DIExpr = varInfo->DIExpr) {
       bool HasFragment = false;

lib/SILGen/SILGenPattern.cpp

@@ -2870,7 +2870,12 @@ void PatternMatchEmission::emitAddressOnlyAllocations() {
       if (!ty.isAddressOnly(SGF.F))
         continue;
       assert(!Temporaries[vd]);
-      Temporaries[vd] = SGF.emitTemporaryAllocation(vd, ty);
+      // Don't generate debug info for the temporary, as another debug_value
+      // will be created in the body, with the same scope and a different type
+      // Not sure if this is the best way to avoid that?
+      Temporaries[vd] = SGF.emitTemporaryAllocation(
+        vd, ty, DoesNotHaveDynamicLifetime, IsNotLexical, IsNotFromVarDecl,
+        /* generateDebugInfo = */ false);
     }
   }
 

lib/SILGen/SILGenProlog.cpp

@@ -1297,7 +1297,7 @@ void SILGenFunction::emitProlog(
 
   emitExpectedExecutor();
 
-  // IMPORTANT: This block should be the last one in `emitProlog`, 
+  // IMPORTANT: This block should be the last one in `emitProlog`,
   // since it terminates BB and no instructions should be insterted after it.
   // Emit an unreachable instruction if a parameter type is
   // uninhabited
@@ -1501,5 +1501,18 @@ uint16_t SILGenFunction::emitBasicProlog(
     B.createDebugValue(loc, undef.getValue(), dbgVar);
   }
 
+  for (auto &i : *B.getInsertionBB()) {
+    auto *alloc = dyn_cast<AllocStackInst>(&i);
+    if (!alloc)
+      continue;
+    auto varInfo = alloc->getVarInfo();
+    if (!varInfo || varInfo->ArgNo)
+      continue;
+    // The allocation has a varinfo but no argument number, which should not
+    // happen in the prolog. Unfortunately, some copies can generate wrong
+    // debug info, so we have to fix it here, by invalidating it.
+    alloc->invalidateVarInfo();
+  }
+
   return ArgNo;
 }

lib/SILOptimizer/Differentiation/PullbackCloner.cpp

@@ -1552,8 +1552,11 @@ class PullbackCloner::Implementation final
       auto adjVal = materializeAdjointDirect(getAdjointValue(bb, inst), loc);
       // Allocate a local buffer and store the adjoint value. This buffer will
       // be used for accumulation into the adjoint buffer.
-      auto adjBuf = builder.createAllocStack(
-          loc, adjVal->getType(), SILDebugVariable());
+      auto adjBuf = builder.createAllocStack(loc, adjVal->getType(), {},
+                                             DoesNotHaveDynamicLifetime,
+                                             IsNotLexical, IsNotFromVarDecl,
+                                             DoesNotUseMoveableValueDebugInfo,
+                                             /* skipVarDeclAssert = */ true);
       auto copy = builder.emitCopyValueOperation(loc, adjVal);
       builder.emitStoreValueOperation(loc, copy, adjBuf,
                                       StoreOwnershipQualifier::Init);

test/SILGen/switch.swift

@@ -1461,9 +1461,9 @@ func testVoidType() {
 
 // CHECK-LABEL: sil hidden [ossa] @$s6switch28addressOnlyFallthroughCalleeyyAA015MultipleAddressC8CaseEnumOyxGSzRzlF : $@convention(thin) <T where T : BinaryInteger> (@in_guaranteed MultipleAddressOnlyCaseEnum<T>) -> () {
 // CHECK: bb0([[ARG:%.*]] :
-// CHECK:   [[AB_PHI:%.*]] = alloc_stack $T, let, name "x"
-// CHECK:   [[ABB_PHI:%.*]] = alloc_stack $T, let, name "x"
-// CHECK:   [[ABBC_PHI:%.*]] = alloc_stack $T, let, name "x"
+// CHECK:   [[AB_PHI:%.*]] = alloc_stack $T
+// CHECK:   [[ABB_PHI:%.*]] = alloc_stack $T
+// CHECK:   [[ABBC_PHI:%.*]] = alloc_stack $T
 // CHECK:   [[SWITCH_ENUM_ARG:%.*]] = alloc_stack $MultipleAddressOnlyCaseEnum<T>
 // CHECK:   copy_addr [[ARG]] to [init] [[SWITCH_ENUM_ARG]]
 // CHECK:   switch_enum_addr [[SWITCH_ENUM_ARG]] : $*MultipleAddressOnlyCaseEnum<T>, case #MultipleAddressOnlyCaseEnum.a!enumelt: [[BB_A:bb[0-9]+]], case #MultipleAddressOnlyCaseEnum.b!enumelt: [[BB_B:bb[0-9]+]], case #MultipleAddressOnlyCaseEnum.c!enumelt: [[BB_C:bb[0-9]+]]

test/SILOptimizer/init_accessors.swift

@@ -330,7 +330,7 @@ struct TestGenericTuple<T, U> {
   //
   // CHECK: bb0([[A_REF:%.*]] : $*T, [[B_REF:%.*]] : $*(T, U), [[A_VALUE:%.*]] : $*T, [[B_VALUE:%.*]] : $*T, [[C_VALUE:%.*]] : $*U, [[METATYPE:%.*]] : $@thin TestGenericTuple<T, U>.Type):
   //
-  // CHECK: [[INIT_VALUE_1:%.*]] = alloc_stack $(T, U), let, name "initialValue"
+  // CHECK: [[INIT_VALUE_1:%.*]] = alloc_stack $(T, U)
   // CHECK-NEXT: [[INIT_VALUE_1_0:%.*]] = tuple_element_addr [[INIT_VALUE_1]] : $*(T, U), 0
   // CHECK-NEXT: copy_addr [take] [[B_VALUE]] to [init] [[INIT_VALUE_1_0]]
   // CHECK-NEXT: [[INIT_VALUE_1_1:%.*]] = tuple_element_addr [[INIT_VALUE_1]] : $*(T, U), 1