Ignore duplicated, identical fragment declarations

[dotdash]

Summary

Some passes might duplicate calls to llvm.dbg.declare causing
duplicated, identical fragment declarations which currently cause an
assertion might is meant to catch erroneous overlapping fragment
declarations. But for declarations that are simply duplicated we can be
a bit more lenient and just ignore the duplicates.

PR33157 was fixed by a similar change for non-fragment declarations in
r305244.

[rnk]

DbgVariable::addMMIEntry already handles de-duplication of non-fragment FrameIndexExprs, so I think we should put the logic for frame index exprs there. In fact, what you're doing here is plainly more general the special case code that we have there, and we should be able to get rid of it.

I'd also move addMMIEntry out of line. It has clearly grown in complexity over time.

[dotdash]

Updated to address comments by rnk.

[aprantl]

Thanks!

lib/CodeGen/AsmPrinter/DwarfDebug.cpp

240 ↗

(On Diff #117789)

This strips out true duplicates (which was the *intention* behind the old code). The old code also deleted all but the first non-fragment entry.

This only affects inputs that are arguably already broken (two non-fragment entries for the same variable at different frame indices), so I don't think the old behavior is worth keeping.

llvm/trunk/lib/CodeGen/AsmPrinter/DwarfDebug.h

public:
   DIE *getDIE() const { return TheDIE; }
   void setDebugLocListIndex(unsigned O) { DebugLocListIndex = O; }
   unsigned getDebugLocListIndex() const { return DebugLocListIndex; }
   StringRef getName() const { return Var->getName(); }
   const MachineInstr *getMInsn() const { return MInsn; }
   /// Get the FI entries, sorted by fragment offset.
   ArrayRef<FrameIndexExpr> getFrameIndexExprs() const;
   bool hasFrameIndexExprs() const { return !FrameIndexExprs.empty(); }
-
-  void addMMIEntry(const DbgVariable &V) {
-    assert(DebugLocListIndex == ~0U && !MInsn && "not an MMI entry");
-    assert(V.DebugLocListIndex == ~0U && !V.MInsn && "not an MMI entry");
-    assert(V.Var == Var && "conflicting variable");
-    assert(V.IA == IA && "conflicting inlined-at location");
-
-    assert(!FrameIndexExprs.empty() && "Expected an MMI entry");
-    assert(!V.FrameIndexExprs.empty() && "Expected an MMI entry");
-
-    if (FrameIndexExprs.size()) {
-      auto *Expr = FrameIndexExprs.back().Expr;
-      // Get rid of duplicate non-fragment entries. More than one non-fragment
-      // dbg.declare makes no sense so ignore all but the first.
-      if (!Expr || !Expr->isFragment())
-        return;
-    }
-    FrameIndexExprs.append(V.FrameIndexExprs.begin(), V.FrameIndexExprs.end());
-    assert(llvm::all_of(FrameIndexExprs,
-                        [](FrameIndexExpr &FIE) {
-                          return FIE.Expr && FIE.Expr->isFragment();
-                        }) &&
-           "conflicting locations for variable");
-  }
+  void addMMIEntry(const DbgVariable &V);
 
   // Translate tag to proper Dwarf tag.
   dwarf::Tag getTag() const {
     // FIXME: Why don't we just infer this tag and store it all along?
     if (Var->isParameter())
       return dwarf::DW_TAG_formal_parameter;
 
     return dwarf::DW_TAG_variable;

llvm/trunk/lib/CodeGen/AsmPrinter/DwarfDebug.cpp

assert(llvm::all_of(FrameIndexExprs,
                         return A.Expr->isFragment();
                       }) &&
          "multiple FI expressions without DW_OP_LLVM_fragment");
   std::sort(FrameIndexExprs.begin(), FrameIndexExprs.end(),
             [](const FrameIndexExpr &A, const FrameIndexExpr &B) -> bool {
               return A.Expr->getFragmentInfo()->OffsetInBits <
                      B.Expr->getFragmentInfo()->OffsetInBits;
             });
+
   return FrameIndexExprs;
 }
 
+void DbgVariable::addMMIEntry(const DbgVariable &V) {
+  assert(DebugLocListIndex == ~0U && !MInsn && "not an MMI entry");
+  assert(V.DebugLocListIndex == ~0U && !V.MInsn && "not an MMI entry");
+  assert(V.Var == Var && "conflicting variable");
+  assert(V.IA == IA && "conflicting inlined-at location");
+
+  assert(!FrameIndexExprs.empty() && "Expected an MMI entry");
+  assert(!V.FrameIndexExprs.empty() && "Expected an MMI entry");
+
+  for (const auto &FIE : V.FrameIndexExprs)
+    // Ignore duplicate entries.
+    if (llvm::none_of(FrameIndexExprs, [&](const FrameIndexExpr &Other) {
+          return FIE.FI == Other.FI && FIE.Expr == Other.Expr;
+        }))
+      FrameIndexExprs.push_back(FIE);
+
+  assert((FrameIndexExprs.size() == 1 ||
+          llvm::all_of(FrameIndexExprs,
+                       [](FrameIndexExpr &FIE) {
+                         return FIE.Expr && FIE.Expr->isFragment();
+                       })) &&
+         "conflicting locations for variable");
+}
+
 static const DwarfAccelTable::Atom TypeAtoms[] = {
     DwarfAccelTable::Atom(dwarf::DW_ATOM_die_offset, dwarf::DW_FORM_data4),
     DwarfAccelTable::Atom(dwarf::DW_ATOM_die_tag, dwarf::DW_FORM_data2),
     DwarfAccelTable::Atom(dwarf::DW_ATOM_type_flags, dwarf::DW_FORM_data1)};
 
 DwarfDebug::DwarfDebug(AsmPrinter *A, Module *M)
     : DebugHandlerBase(A), DebugLocs(A->OutStreamer->isVerboseAsm()),
       InfoHolder(A, "info_string", DIEValueAllocator),