[RFC][Debugify] Remove false positve debug location losses

[ntesic]

Summary

The problem of "false positive results" in Debugify reports was discussed at one of the LLVM Dev meetings. This RFC aims at solving this problem for reports about lost Debug Locations (instruction source locations).

Discourse discussion: https://discourse.llvm.org/t/debugify-false-positives-elimination/69614

There are several scenarios where debug locations are dropped, but should not be reported by Debugify as losses:

• instruction's DebugLoc should be dropped if it cannot be unambiguously determined (according to How-to-update-debug-info document)
• instruction's DebugLoc is copied from an other instruction (loss should not be reported for both instructions)

In my opinion, those cases should not be treated as losses, since DebugLocs are explicitly updated, but couldn't be reconstructed.

My proposal, in this patch, is to add a flag to the Instruction (droppedLocation), which should be set if the DebugLoc is explicitly set to empty for that instruction. Then, during Debugify analysis, we should not report DebugLoc loss if instruction's DebugLoc is explicitly updated (by checking the flag).

Proposed solution was tested by compiling clang-14 with -fverify-debuginfo-preserve. The results show around 65% of reported DebugLoc losses is eliminated.

[Orlando]

This looks useful and the results in the RFC are promising. I like the approach, however, I worry that adding a field to Instruction may be expensive - have you measured the performance and memory usage impact?

It may be that we need to come up with another mechanism for storing this info (nothing concrete comes to mind right now - I'm not terribly familiar with Instruction's internal state; I'm not sure if there are any spare bits hanging around that we might be able to use instead e.g. in SubclassData or something like that).

It seems like a good idea though. Have you managed to find any bugs (or perhaps parts of the compiler that should be marking DebugLocs as dropped) using this yet?

[myhsu]

I also feel like we should be a little more discreet on adding new Instruction fields. Assuming a debug location is set to (0,0) if and only if we intentionally drop a debug location, then isLocationDropped can simply be implemented as checking if debugloc is equal to (0,0) rather than adding a new Instruction field. Unfortunately, correct me if I'm wrong, I don't think that's the case. Namely, we need a separate state to represent removing debugloc completely without setting it to (0,0). A potential solution might be pointing debugloc to an unique, special DILocation representing such state when that happens, such that intentionally dropping debugloc is embodied by either (0,0) or that special DILocation. No additional instruction field is needed.

[ntesic]

Hi, thanks for your comments!

@Orlando
I haven't done any advanced performance measurements, but for the simple build time & size checking, I didn't detect any increase in time nor size.
Regarding bugs detection, in this patch I was focused on removing results from debugify reports that can lead to a dead-end investigation of debug-location drops.
According to how-to-update-debug-info document , the dropLocation() should be used whereever we want to explicitly remove debug-location. However, in most places
we still use setDebugLoc() with nullptr or empty DebugLoc parameter. It would take a lot of time to update all of these to use dropLocation().
Using spare bits in some of the existing bit vectors is definitely a better idea, I will investigate that further.

@myhsu
I think we are on the same page. There are situations where empty debug-loc is not intentionally dropped, so we need to detect the intentional drop in some way.
The (0,0) debug-loc is already a special kind of DILocation, representing an unknown source location, but doesn't imply that the location is implicitly or explicitly dropped.
Thanks for the idea.

Based on that, I am thinking about using (0,1) debug location if it is intentionally dropped, but I am not sure if some other parts of the debug-info infrastructure expect to see exactly (0,0) debug-loc.
What are the reviewers’ opinions about this idea?

llvm/include/llvm/IR/Instruction.h

public:
   void setAAMetadata(const AAMDNodes &N);
 
   /// Retrieve total raw weight values of a branch.
   /// Returns true on success with profile total weights filled in.
   /// Returns false if no metadata was found.
   bool extractProfTotalWeight(uint64_t &TotalVal) const;
 
   /// Set the debug location information for this instruction.
-  void setDebugLoc(DebugLoc Loc) { DbgLoc = std::move(Loc); }
+  void setDebugLoc(DebugLoc Loc) { 
+    if (!Loc || (Loc.getLine() == 0 && Loc.getCol() == 0))
+      droppedLocation = true;
+    else
+      droppedLocation = false;
+    DbgLoc = std::move(Loc);
+  }
 
   /// Return the debug location for this node as a DebugLoc.
   const DebugLoc &getDebugLoc() const { return DbgLoc; }
 
   /// Set or clear the nuw flag on this instruction, which must be an operator
   /// which supports this flag. See LangRef.html for the meaning of this flag.
   void setHasNoUnsignedWrap(bool b = true);
 
public:
   /// instruction in \p SourceInstructions needs to have DIAssignID
   /// metadata. If none of them do then nothing happens. If this instruction
   /// does not have a DIAssignID attachment but at least one in \p
   /// SourceInstructions does then the merged one will be attached to
   /// it. However, instructions without attachments in \p SourceInstructions
   /// are not modified.
   void mergeDIAssignID(ArrayRef<const Instruction *> SourceInstructions);
 
+  bool isLocationDropped() const { return droppedLocation; }
 private:
+  bool droppedLocation = false;
   // These are all implemented in Metadata.cpp.
   MDNode *getMetadataImpl(unsigned KindID) const;
   MDNode *getMetadataImpl(StringRef Kind) const;
   void
   getAllMetadataImpl(SmallVectorImpl<std::pair<unsigned, MDNode *>> &) const;
 
   /// Update the LLVMContext ID-to-Instruction(s) mapping. If \p ID is nullptr
   /// then clear the mapping for this instruction.

llvm/lib/IR/DebugInfo.cpp

if (*It != MergeID)
       at::RAUW(*It, MergeID);
   }
   setMetadata(LLVMContext::MD_DIAssignID, MergeID);
 }
 
 void Instruction::updateLocationAfterHoist() { dropLocation(); }
 
 void Instruction::dropLocation() {
+  // Mark this Instructions location as intentionally dropped.
+  droppedLocation = true;
   const DebugLoc &DL = getDebugLoc();
   if (!DL)
     return;
 
   // If this isn't a call, drop the location to allow a location from a
   // preceding instruction to propagate.
   bool MayLowerToCall = false;
   if (isa<CallBase>(this)) {

llvm/lib/Transforms/Utils/Debugify.cpp

for (const auto &L : DILocsAfter) {
     auto Instr = L.first;
 
     // In order to avoid pointer reuse/recycling, skip the values that might
     // have been deleted during a pass.
     auto WeakInstrPtr = InstToDelete.find(Instr);
     if (WeakInstrPtr != InstToDelete.end() && !WeakInstrPtr->second)
       continue;
 
+    // Skip instruction if its DebugLoc is intentionally dropped.
+    if (Instr->isLocationDropped())
+      continue;
+
     auto FnName = Instr->getFunction()->getName();
     auto BB = Instr->getParent();
     auto BBName = BB->hasName() ? BB->getName() : "no-name";
     auto InstName = Instruction::getOpcodeName(Instr->getOpcode());
 
     auto InstrIt = DILocsBefore.find(Instr);
     if (InstrIt == DILocsBefore.end()) {
       if (ShouldWriteIntoJSON)
for (Instruction &I : instructions(F)) {
         continue;
 
       auto DL = I.getDebugLoc();
       if (DL && DL.getLine() != 0) {
         MissingLines.reset(DL.getLine() - 1);
         continue;
       }
 
+      // Skip instruction if its DebugLoc is intentionally dropped.
+      if (!DL && I.isLocationDropped())
+        continue;
+
       if (!isa<PHINode>(&I) && !DL) {
         dbg() << "WARNING: Instruction with empty DebugLoc in function ";
         dbg() << F.getName() << " --";
         I.print(dbg());
         dbg() << "\n";
       }
     }
 

llvm/test/DebugInfo/Generic/licm-hoist-debug-loc.ll

 ; }
 ;
 ; We make sure that the instruction that is hoisted into the preheader
 ; does not have a debug location.
 ; CHECK: for.body.lr.ph:
 ; CHECK: getelementptr{{.*}}%p.addr, i64 4{{$}}
 ; CHECK: for.body:
 ;
+; RUN: opt < %s -S -verify-debuginfo-preserve -passes=licm 2>&1 | FileCheck %s --check-prefix=DEBUGIFY
+; DEBUGIFY: CheckModuleDebugify (original debuginfo): PASS
+;
 ; ModuleID = 't.ll'
 source_filename = "test.c"
 
 ; Function Attrs: nounwind sspstrong uwtable
 define void @foo(i32 %k, i32 %p) !dbg !7 {
 entry:
   %p.addr = alloca i32, align 4
   store i32 %p, i32* %p.addr, align 4

llvm/test/DebugInfo/debugify.ll

 ; --- Metadata counts
 ; CHECK-DAG: ![[NUM_INSTS]] = !{i32 6}
 ; CHECK-DAG: ![[NUM_VARS]] = !{i32 3}
 
 ; --- Repeat case
 ; CHECK-REPEAT: ModuleDebugify: Skipping module with debug info
 
 ; --- Failure case
-; CHECK-WARN: WARNING: Instruction with empty DebugLoc in function foo --   ret void
-; CHECK-WARN: WARNING: Instruction with empty DebugLoc in function bar --   call void @foo()
-; CHECK-WARN: WARNING: Instruction with empty DebugLoc in function bar --   {{.*}} add i32 0, 1
-; CHECK-WARN: WARNING: Instruction with empty DebugLoc in function bar --   ret i32 0
+;; Strip (debug info) is explicit removal of debug-loc, therefore, not a debug-loc drop.
+; CHECK-WARN-NOT: WARNING: Instruction with empty DebugLoc in function foo --   ret void
+; CHECK-WARN-NOT: WARNING: Instruction with empty DebugLoc in function bar --   call void @foo()
+; CHECK-WARN-NOT: WARNING: Instruction with empty DebugLoc in function bar --   {{.*}} add i32 0, 1
+; CHECK-WARN-NOT: WARNING: Instruction with empty DebugLoc in function bar --   ret i32 0
 ; CHECK-WARN: WARNING: Missing line 1
 ; CHECK-WARN: WARNING: Missing line 2
 ; CHECK-WARN: WARNING: Missing line 3
 ; CHECK-WARN: WARNING: Missing line 4
+; CHECK-WARN: WARNING: Missing line 5
+; CHECK-WARN: WARNING: Missing line 6
 ; CHECK-WARN: WARNING: Missing variable 1
+; CHECK-WARN: WARNING: Missing variable 2
+; CHECK-WARN: WARNING: Missing variable 3
 ; CHECK-WARN: CheckModuleDebugify: PASS
 
 ; PASS: CheckModuleDebugify: PASS

llvm/unittests/IR/InstructionsTest.cpp

TEST(InstructionsTest, AlterCallBundles) {
   Value *Args[] = {ConstantInt::get(Int32Ty, 42)};
   OperandBundleDef OldBundle("before", UndefValue::get(Int32Ty));
   std::unique_ptr<CallInst> Call(
       CallInst::Create(FnTy, Callee, Args, OldBundle, "result"));
   Call->setTailCallKind(CallInst::TailCallKind::TCK_NoTail);
   AttrBuilder AB(C);
   AB.addAttribute(Attribute::Cold);
   Call->setAttributes(AttributeList::get(C, AttributeList::FunctionIndex, AB));
-  Call->setDebugLoc(DebugLoc(MDNode::get(C, std::nullopt)));
+  Call->setDebugLoc(DebugLoc());
 
   OperandBundleDef NewBundle("after", ConstantInt::get(Int32Ty, 7));
   std::unique_ptr<CallInst> Clone(CallInst::Create(Call.get(), NewBundle));
   EXPECT_EQ(Call->arg_size(), Clone->arg_size());
   EXPECT_EQ(Call->getArgOperand(0), Clone->getArgOperand(0));
   EXPECT_EQ(Call->getCallingConv(), Clone->getCallingConv());
   EXPECT_EQ(Call->getTailCallKind(), Clone->getTailCallKind());
   EXPECT_TRUE(Clone->hasFnAttr(Attribute::AttrKind::Cold));
TEST(InstructionsTest, AlterInvokeBundles) {
   OperandBundleDef OldBundle("before", UndefValue::get(Int32Ty));
   std::unique_ptr<InvokeInst> Invoke(
       InvokeInst::Create(FnTy, Callee, NormalDest.get(), UnwindDest.get(), Args,
                          OldBundle, "result"));
   AttrBuilder AB(C);
   AB.addAttribute(Attribute::Cold);
   Invoke->setAttributes(
       AttributeList::get(C, AttributeList::FunctionIndex, AB));
-  Invoke->setDebugLoc(DebugLoc(MDNode::get(C, std::nullopt)));
+  Invoke->setDebugLoc(DebugLoc());
 
   OperandBundleDef NewBundle("after", ConstantInt::get(Int32Ty, 7));
   std::unique_ptr<InvokeInst> Clone(
       InvokeInst::Create(Invoke.get(), NewBundle));
   EXPECT_EQ(Invoke->getNormalDest(), Clone->getNormalDest());
   EXPECT_EQ(Invoke->getUnwindDest(), Clone->getUnwindDest());
   EXPECT_EQ(Invoke->arg_size(), Clone->arg_size());
   EXPECT_EQ(Invoke->getArgOperand(0), Clone->getArgOperand(0));

llvm/unittests/Transforms/Utils/DebugifyTest.cpp

 //===- DebugifyTest.cpp - Debugify unit tests -----------------------------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 #include "llvm/ADT/SmallVector.h"
 #include "llvm/AsmParser/Parser.h"
 #include "llvm/IR/DebugInfoMetadata.h"
 #include "llvm/IR/IntrinsicInst.h"
 #include "llvm/IR/LegacyPassManager.h"
 #include "llvm/Support/SourceMgr.h"
 #include "llvm/Transforms/Utils/Debugify.h"
+#include "llvm/IR/IRBuilder.h"
+
 #include "gtest/gtest.h"
 
 using namespace llvm;
 
 static std::unique_ptr<Module> parseIR(LLVMContext &C, const char *IR) {
   SMDiagnostic Err;
   std::unique_ptr<Module> Mod = parseAssemblyString(IR, Err, C);
   if (!Mod)
     Err.print("DebugifyTest", errs());
   return Mod;
 }
 
 namespace llvm {
 void initializeDebugInfoDropPass(PassRegistry &);
+void initializeDebugLocDropPass(PassRegistry &);
 void initializeDebugInfoDummyAnalysisPass(PassRegistry &);
 
 namespace {
 struct DebugInfoDrop : public FunctionPass {
   static char ID;
   bool runOnFunction(Function &F) override {
     // Drop DISubprogram.
     F.setSubprogram(nullptr);
     for (BasicBlock &BB : F) {
       // Remove debug locations.
       for (Instruction &I : BB)
         I.setDebugLoc(DebugLoc());
     }
 
     return false;
   }
 
   void getAnalysisUsage(AnalysisUsage &AU) const override {
     AU.setPreservesCFG();
   }
 
   DebugInfoDrop() : FunctionPass(ID) {}
 };
 
+struct DebugLocDrop : public FunctionPass {
+  static char ID;
+  bool runOnFunction(Function &F) override {
+    BasicBlock *BB = &*F.begin();
+    Instruction* Instr = BB->getFirstNonPHIOrDbg();
+    Instr->setDebugLoc(DebugLoc());
+    Instruction *I = Instr->clone();
+    I->insertInto(BB, BB->begin());
+    Instr->replaceAllUsesWith(I);
+    Instr->eraseFromParent();
+
+    return false;
+  }
+
+  void getAnalysisUsage(AnalysisUsage &AU) const override {
+    AU.setPreservesCFG();
+  }
+
+  DebugLocDrop() : FunctionPass(ID) {}
+};
+
 struct DebugValueDrop : public FunctionPass {
   static char ID;
   bool runOnFunction(Function &F) override {
     SmallVector<DbgVariableIntrinsic *, 4> Dbgs;
     for (BasicBlock &BB : F) {
       // Remove dbg var intrinsics.
       for (Instruction &I : BB) {
         if (auto *DVI = dyn_cast<DbgVariableIntrinsic>(&I))
void getAnalysisUsage(AnalysisUsage &AU) const override {
     AU.setPreservesAll();
   }
 
   DebugInfoDummyAnalysis() : FunctionPass(ID) {}
 };
 }
 
 char DebugInfoDrop::ID = 0;
+char DebugLocDrop::ID = 0;
 char DebugValueDrop::ID = 0;
 char DebugInfoDummyAnalysis::ID = 0;
 
 TEST(DebugInfoDrop, DropOriginalDebugInfo) {
   LLVMContext C;
   std::unique_ptr<Module> M = parseIR(C, R"(
     define i16 @f(i16 %a) !dbg !6 {
       %b = add i16 %a, 1, !dbg !11
TEST(DebugInfoDrop, DropOriginalDebugInfo) {
 
   std::string StdOut = testing::internal::GetCapturedStderr();
 
   std::string ErrorForSP = "ERROR:  dropped DISubprogram of";
   std::string WarningForLoc = "WARNING:  dropped DILocation of";
   std::string FinalResult = "CheckModuleDebugify (original debuginfo): FAIL";
 
   EXPECT_TRUE(StdOut.find(ErrorForSP) != std::string::npos);
+  // explicit removal is not a drop
+  EXPECT_TRUE(StdOut.find(WarningForLoc) == std::string::npos);
+  EXPECT_TRUE(StdOut.find(FinalResult) != std::string::npos);
+}
+
+TEST(DebugLocDrop, DropOriginalDebugLoc) {
+  LLVMContext C;
+  std::unique_ptr<Module> M = parseIR(C, R"(
+    define i16 @f(i16 %a) !dbg !6 {
+      %b = add i16 %a, 1, !dbg !11
+      ret i16 0, !dbg !11
+    }
+    declare void @llvm.dbg.value(metadata, metadata, metadata)
+
+    !llvm.dbg.cu = !{!0}
+    !llvm.module.flags = !{!5}
+
+    !0 = distinct !DICompileUnit(language: DW_LANG_C, file: !1, producer: "debugify", isOptimized: true, runtimeVersion: 0, emissionKind: FullDebug, enums: !2)
+    !1 = !DIFile(filename: "t.ll", directory: "/")
+    !2 = !{}
+    !5 = !{i32 2, !"Debug Info Version", i32 3}
+    !6 = distinct !DISubprogram(name: "f", linkageName: "f", scope: null, file: !1, line: 1, type: !7, scopeLine: 1, spFlags: DISPFlagDefinition | DISPFlagOptimized, unit: !0, retainedNodes: !8)
+    !7 = !DISubroutineType(types: !2)
+    !8 = !{!9}
+    !9 = !DILocalVariable(name: "b", scope: !6, file: !1, line: 1, type: !10)
+    !10 = !DIBasicType(name: "ty16", size: 16, encoding: DW_ATE_unsigned)
+    !11 = !DILocation(line: 1, column: 1, scope: !6)
+  )");
+
+  DebugLocDrop *P = new DebugLocDrop();
+
+
+  DebugInfoPerPass DIBeforePass;
+  DebugifyCustomPassManager Passes;
+  Passes.setDebugInfoBeforePass(DIBeforePass);
+  Passes.add(createDebugifyModulePass(DebugifyMode::OriginalDebugInfo, "",
+                                      &(Passes.getDebugInfoPerPass())));
+  Passes.add(P);
+  Passes.add(createCheckDebugifyModulePass(false, "", nullptr,
+                                           DebugifyMode::OriginalDebugInfo,
+                                           &(Passes.getDebugInfoPerPass())));
+
+  testing::internal::CaptureStderr();
+  Passes.run(*M);
+
+  std::string StdOut = testing::internal::GetCapturedStderr();
+
+  std::string WarningForLoc = "WARNING:  did not generate DILocation";
+  std::string FinalResult = "CheckModuleDebugify (original debuginfo): FAIL";
+
   EXPECT_TRUE(StdOut.find(WarningForLoc) != std::string::npos);
   EXPECT_TRUE(StdOut.find(FinalResult) != std::string::npos);
 }
 
 TEST(DebugValueDrop, DropOriginalDebugValues) {
   LLVMContext C;
   std::unique_ptr<Module> M = parseIR(C, R"(
     define i16 @f(i16 %a) !dbg !6 {

 
 } // end namespace llvm
 
 INITIALIZE_PASS_BEGIN(DebugInfoDrop, "debuginfodroppass", "debuginfodroppass",
                       false, false)
 INITIALIZE_PASS_END(DebugInfoDrop, "debuginfodroppass", "debuginfodroppass", false,
                     false)
 
+INITIALIZE_PASS_BEGIN(DebugLocDrop, "debuglocdroppass", "debuglocdroppass",
+                      false, false)
+INITIALIZE_PASS_END(DebugLocDrop, "debuglocdroppass", "debuglocdroppass", false,
+                    false)
+
 INITIALIZE_PASS_BEGIN(DebugInfoDummyAnalysis, "debuginfodummyanalysispass",
                       "debuginfodummyanalysispass", false, false)
 INITIALIZE_PASS_END(DebugInfoDummyAnalysis, "debuginfodummyanalysispass",
                     "debuginfodummyanalysispass", false, false)