RFC: More principled implementation of DISubprogram::describes()

[aprantl]

Summary

commit 3c2d70434bd750683baa94432bfe9fd1f639f968
Author: Duncan P. N. Exon Smith <[e-mail]>
Date:   Mon Apr 13 19:07:27 2015 +0000

    DebugInfo: Migrate DISubprogram::describes() to new hierarchy, NFC
    
    I don't really like this function at all -- I think it should be as
    simple as `return getFunction() == F` -- but for now this seems like the
    best we can do.
    
    llvm-svn: 234778

I'm not sure if we still need the name check. Perhaps there are some LTO cases where this is necessary but it's not clear to me what they are.

[dexonsmith]

If you're worried about LTO, I suggest trying a clang bootstrap with each of -flto=full and -flto=thin. I was using the former as a testcase when I was doing this work, so I think we'd get good signal from that.

llvm/lib/IR/DebugInfoMetadata.cpp

663

Nitpick, I think we usually leave the redundant parens off for return statements.

[aprantl]

ThinLTO and FullLTO stage builds of all dependencies of check-clang succeeded with this change.

[aprantl]

I'm inclined to try to commit this and see if we get any "describes" assertion reports.

[dexonsmith]

In D75213#1898320, @aprantl wrote:

I'm inclined to try to commit this and see if we get any "describes" assertion reports.

SGTM.

llvm/lib/IR/Verifier.cpp

2397

Should this FIXME be updated?

2401

It's not obvious to me why this call is no longer necessary.

[aprantl]

llvm/lib/IR/Verifier.cpp

2401

because the SP->describes(&F) call on line 2398 will either

• fail, in which case we found an error and can stop
• succeed, in which case we must have visited the subprogram already when we entered the function

[dexonsmith]

Okay, LGTM then, as long as...

In D75213#1896981, @aprantl wrote:

ThinLTO and FullLTO stage builds of all dependencies of check-clang succeeded with this change.

Did you confirm that none of the debug info was stripped? IIRC, if debug info doesn't verify during LTO it gets stripped rather than causing a failure.

[aprantl]

In D75213#1898741, @dexonsmith wrote:

Okay, LGTM then, as long as...

In D75213#1896981, @aprantl wrote:

ThinLTO and FullLTO stage builds of all dependencies of check-clang succeeded with this change.

Did you confirm that none of the debug info was stripped? IIRC, if debug info doesn't verify during LTO it gets stripped rather than causing a failure.

Good point! I should have seen that as a warning in the linker output if it happened.

llvm/lib/IR/DebugInfoMetadata.cpp

DISubprogram *DISubprogram::getImpl(
   DEFINE_GETIMPL_STORE_N(
       DISubprogram,
       (Line, ScopeLine, VirtualIndex, ThisAdjustment, Flags, SPFlags), Ops,
       Ops.size());
 }
 
 bool DISubprogram::describes(const Function *F) const {
   assert(F && "Invalid function");
-  if (F->getSubprogram() == this)
-    return true;
-  StringRef Name = getLinkageName();
-  if (Name.empty())
-    Name = getName();
-  return F->getName() == Name;
+  return F->getSubprogram() == this;
 }
 
 DILexicalBlock *DILexicalBlock::getImpl(LLVMContext &Context, Metadata *Scope,
                                         Metadata *File, unsigned Line,
                                         unsigned Column, StorageType Storage,
                                         bool ShouldCreate) {
   // Fixup column.
   adjustColumn(Column);

llvm/lib/IR/Verifier.cpp

if (F.hasAddressTaken(&U))
       Assert(false, "Invalid user of intrinsic instruction!", U);
   }
 
   auto *N = F.getSubprogram();
   HasDebugInfo = (N != nullptr);
   if (!HasDebugInfo)
     return;
 
-  // Check that all !dbg attachments lead to back to N (or, at least, another
-  // subprogram that describes the same function).
+  // Check that all !dbg attachments lead to back to N.
   //
   // FIXME: Check this incrementally while visiting !dbg attachments.
   // FIXME: Only check when N is the canonical subprogram for F.
   SmallPtrSet<const MDNode *, 32> Seen;
   auto VisitDebugLoc = [&](const Instruction &I, const MDNode *Node) {
     // Be careful about using DILocation here since we might be dealing with
     // broken code (this is the Verifier after all).
     const DILocation *DL = dyn_cast_or_null<DILocation>(Node);
auto VisitDebugLoc = [&](const Instruction &I, const MDNode *Node) {
 
     DISubprogram *SP = Scope ? Scope->getSubprogram() : nullptr;
 
     // Scope and SP could be the same MDNode and we don't want to skip
     // validation in that case
     if (SP && ((Scope != SP) && !Seen.insert(SP).second))
       return;
 
-    // FIXME: Once N is canonical, check "SP == &N".
     AssertDI(SP->describes(&F),
              "!dbg attachment points at wrong subprogram for function", N, &F,
              &I, DL, Scope, SP);
-    visitMDNode(*SP);
   };
   for (auto &BB : F)
     for (auto &I : BB) {
       VisitDebugLoc(I, I.getDebugLoc().getAsMDNode());
       // The llvm.loop annotations also contain two DILocations.
       if (auto MD = I.getMetadata(LLVMContext::MD_loop))
         for (unsigned i = 1; i < MD->getNumOperands(); ++i)
           VisitDebugLoc(I, dyn_cast_or_null<MDNode>(MD->getOperand(i)));

llvm/test/Verifier/disubprogram-name-match-only.ll

+; RUN: llvm-as -disable-output <%s 2>&1| FileCheck %s
+
+define void @f() !dbg !14 {
+  ret void, !dbg !5
+}
+
+!llvm.module.flags = !{!15}
+!llvm.dbg.cu = !{!4}
+
+!0 = !{null}
+!1 = distinct !DICompositeType(tag: DW_TAG_structure_type)
+!2 = !DIFile(filename: "f.c", directory: "/")
+!3 = !DISubroutineType(types: !0)
+!4 = distinct !DICompileUnit(language: DW_LANG_C, file: !2)
+; CHECK: !dbg attachment points at wrong subprogram for function
+; CHECK: warning: ignoring invalid debug info
+!5 = !DILocation(line: 1, scope: !9)
+!9 = distinct !DISubprogram(name: "f", scope: !1,
+                            file: !2, line: 1, type: !3, isLocal: true,
+                            isDefinition: true, scopeLine: 2,
+                            unit: !4)
+!14 = distinct !DISubprogram(name: "f", scope: !1,
+                            file: !2, line: 1, type: !3, isLocal: true,
+                            isDefinition: true, scopeLine: 2,
+                            unit: !4)
+!15 = !{i32 1, !"Debug Info Version", i32 3}