WIP: [Verifier] Flag dbg.declares which specify different addresses for the same fragment

[vsk]

Summary

When verifying a basic block, keep track of the mapping between
!DILocalVariable fragments and addresses (typically stack slots). This
can help identify variables that are accidentally described as being in
two different stack slots.

Known check-llvm failures:

LLVM :: Transforms/Coroutines/coro-debug-frame-variable.ll
LLVM :: Transforms/LoopVectorize/discriminator.ll

[vsk]

Ignore inlined entities. It's not unexpected for a function that's inlined multiple times to introduce dbg.declares for the same inlined entity.

[vsk]

@modocache Do you know anyone who could take a look at coro-debug-frame-variable.ll? This is what I'm seeing:

dbg.declares specify different addresses for the same fragment
  call void @llvm.dbg.declare(metadata i32* %i.reload.addr, metadata !14, metadata !DIExpression()), !dbg !16
  call void @llvm.dbg.declare(metadata i32* %i.reload.addr12, metadata !14, metadata !DIExpression()), !dbg !16
label %init.ready
void (%f.Frame*)* @f.resume
!14 = !DILocalVariable(name: "i", scope: !15, file: !1, line: 24, type: !10)
LLVM ERROR: Broken function

[Orlando]

Hi @vsk, just a inline question from me. And in addition, I noticed that SourceLevelDebugging specifically states "there can only be one call to llvm.dbg.declare for a given concrete local variable". I wonder if we should update this to say something like "there can only be one call to llvm.dbg.declare for a given fragment of a local variable"?

Please let me know if these comments should wait til the patch is no longer WIP instead.

llvm/lib/IR/Verifier.cpp

270

Could we ever see two dbg.declares for the same variable fragment with different expressions?

dbg.declare(%x, metadata !1, metadata !DIExpression())
dbg.declare(%x, metadata !1, metadata !DIExpression(DW_OP_plus, DW_OP_constu, 1))

This is contrived and I don't have a reproducer, so I'm not sure if it is something worth considering. I was just reminded of PR41992 and PR43957 when I saw std::pair<DILocalVariable *, DIExpression *> being used to identify fragments of a variable.

[vsk]

The SourceLevelDebugging documentation update makes sense to me. And this check should probably make use of the DebugVariable wrapper.

[aprantl]

Without a testcase it's not immediately obvious what this is catching. It's legal for two *different* fragments of the same variable to point to two separate allocas, I think that's the output of SROA. It's not legal for any one variable (fragment) to be described by a dbg.declare and any other instrinsic. A dbg.declare is supposed to be the only, authoritative location of a variable. So maybe the test could be even stricter.

llvm/lib/IR/Verifier.cpp

class Verifier : public InstVisitor<Verifier>, VerifierSupport {
 
   /// When verifying a basic block, keep track of all of the
   /// instructions we have seen so far.
   ///
   /// This allows us to do efficient dominance checks for the case when an
   /// instruction has an operand that is an instruction in the same block.
   SmallPtrSet<Instruction *, 16> InstsInThisBlock;
 
+  /// When verifying a basic block, keep track of the mapping between
+  /// !DILocalVariable fragments and addresses (typically stack slots). This
+  /// can help identify variables that are accidentally described as being in
+  /// two different stack slots.
+  DenseMap<std::pair<DILocalVariable *, DIExpression *>, DbgDeclareInst *>
+      LocalFragment2Address;
+
   /// Keep track of the metadata nodes that have been checked already.
   SmallPtrSet<const Metadata *, 32> MDNodes;
 
   /// Keep track which DISubprogram is attached to which function.
   DenseMap<const DISubprogram *, const Function *> DISubprogramAttachments;
 
   /// Track all DICompileUnits visited.
   SmallPtrSet<const Metadata *, 2> CUVisited;
for (auto &I : BB) {
         return;
     }
 }
 
 // verifyBasicBlock - Verify that a basic block is well formed...
 //
 void Verifier::visitBasicBlock(BasicBlock &BB) {
   InstsInThisBlock.clear();
+  LocalFragment2Address.clear();
 
   // Ensure that basic blocks have terminators!
   Assert(BB.getTerminator(), "Basic Block does not have terminator!", &BB);
 
   // Check constraints that this basic block imposes on all of the PHI nodes in
   // it.
   if (isa<PHINode>(BB.front())) {
     SmallVector<BasicBlock*, 8> Preds(pred_begin(&BB), pred_end(&BB));
AssertDI(VarSP == LocSP, "mismatched subprogram between llvm.dbg." + Kind +
                                " variable and !dbg attachment",
            &DII, BB, F, Var, Var->getScope()->getSubprogram(), Loc,
            Loc->getScope()->getSubprogram());
 
   // This check is redundant with one in visitLocalVariable().
   AssertDI(isType(Var->getRawType()), "invalid type ref", Var,
            Var->getRawType());
   verifyFnArgs(DII);
+
+  // A block should not have two dbg.declares which map the same variable
+  // to two different addresses. However, if a function is inlined more than
+  // once, it's not unexpected for there to be multiple dbg.declares for an
+  // inlined entity.
+  bool InlinedDef = Loc->getInlinedAt();
+  if (!InlinedDef && isa<DbgDeclareInst>(&DII)) {
+    DbgDeclareInst *&Address =
+        LocalFragment2Address[{Var, DII.getExpression()}];
+    if (!Address)
+      Address = cast<DbgDeclareInst>(&DII);
+    else
+      AssertDI(Address->getVariableLocation() == DII.getVariableLocation(),
+               "dbg.declares specify different addresses for the same fragment",
+               Address, &DII, BB, F, Var);
+  }
 }
 
 void Verifier::visitDbgLabelIntrinsic(StringRef Kind, DbgLabelInst &DLI) {
   AssertDI(isa<DILabel>(DLI.getRawLabel()),
          "invalid llvm.dbg." + Kind + " intrinsic variable", &DLI,
          DLI.getRawLabel());
 
   // Ignore broken !dbg attachments; they're checked elsewhere.