[Debugify][Original DI] Test preservation of original debug var intrinsics in optimizations

[djtodoro]

Summary

This is an improvement of [0]. This adds checking of original llvm.dbg.values()/declares() instructions in optimizations.

We have picked a real issue that has been found with this (actually, picked one variable location missing from [1] and resolved the issue), and the result is the fix for that -- D100844.

Before applying the D100844, using the options from [0] (but with this patch applied) on the compilation of GDB 7.11, the final HTML report for the debug-info issues can be found at [1] (please scroll down, and look for "Summary of Variable Location Bugs"). After applying the D100844, the numbers has improved a bit -- please take a look into [2].

[0] https://llvm.org/docs/HowToUpdateDebugInfo.html#test-original-debug-info-preservation-in-optimizations
[1] https://djolertrk.github.io/di-check-before-adce-fix/
[2] https://djolertrk.github.io/di-check-after-adce-fix/

[Orlando]

Hi @djtodoro I am looking at this (slowly, sorry!).

IIUC this checks that the number of debug intrinsics for each non-inlined variable does not decrease after each optimisation pass. There are legitimate reasons for deleting debug intrinsics. RemoveRedundantDbgInstrs, for example, which is called in a few places. With that in mind I think I would prefer the table header "Number of bugs" to change as this number seems to be more of an indicator of possible bugs, rather than proof of their existence.

You might be able to reduce (*) the number of false positive results by ignoring "redundant" debug intrinsics in the count, though maybe that could be follow-up work?

(*) Another example of a false positive, or at least a true positive that we probably want to ignore: I tried this patch out out using a different target codebase and investigated a debug intrinsic dropped in SROA/mem2reg. Amusingly, git blame pointed right back at me: D89810. D89810, and D85555 which the former is based on, deliberately remove dbg.value+DW_OP_deref intrinsics to work around a common case of location coverage reduction ultimately caused by LowerDbgDeclare.

[djtodoro]

@Orlando Thanks for looking into this.

In D100845#2705685, @Orlando wrote:

Hi @djtodoro I am looking at this (slowly, sorry!).

IIUC this checks that the number of debug intrinsics for each non-inlined variable does not decrease after each optimisation pass. There are legitimate reasons for deleting debug intrinsics. RemoveRedundantDbgInstrs, for example, which is called in a few places. With that in mind I think I would prefer the table header "Number of bugs" to change as this number seems to be more of an indicator of possible bugs, rather than proof of their existence.

Sure. That is why we treat it as "WARNING". The same is happening with dropping of dbg location attached to instructions -- there are places where we cannot salvage !dbg, and we treat it as a warning as well (until we add some super cool logic to distinguish what was reasonable drop or opposite).

You might be able to reduce (*) the number of false positive results by ignoring "redundant" debug intrinsics in the count, though maybe that could be follow-up work?

That could be follow-up for sure. Thanks. In addition, I am seeing a lot of artifitial variables location dropping (such as __result, __s etc.), and I am not sure if we should care about these.

(*) Another example of a false positive, or at least a true positive that we probably want to ignore: I tried this patch out out using a different target codebase and investigated a debug intrinsic dropped in SROA/mem2reg. Amusingly, git blame pointed right back at me: D89810. D89810, and D85555 which the former is based on, deliberately remove dbg.value+DW_OP_deref intrinsics to work around a common case of location coverage reduction ultimately caused by LowerDbgDeclare.

[Orlando]

In D100845#2707872, @djtodoro wrote:

@Orlando Thanks for looking into this.

In D100845#2705685, @Orlando wrote:

Hi @djtodoro I am looking at this (slowly, sorry!).

IIUC this checks that the number of debug intrinsics for each non-inlined variable does not decrease after each optimisation pass. There are legitimate reasons for deleting debug intrinsics. RemoveRedundantDbgInstrs, for example, which is called in a few places. With that in mind I think I would prefer the table header "Number of bugs" to change as this number seems to be more of an indicator of possible bugs, rather than proof of their existence.

Sure. That is why we treat it as "WARNING". The same is happening with dropping of dbg location attached to instructions -- there are places where we cannot salvage !dbg, and we treat it as a warning as well (until we add some super cool logic to distinguish what was reasonable drop or opposite).

That makes sense, cool. Out of curiosity, did you run into many false positives before you found the ADCE bug (D100844)?

I investigated another couple of dropped locations this morning which both look like false positive results too. One was in ipsccp where a dead block which contains a dbg.value is deleted. The other was in Early CSE w/ MemorySSA, which looks like it was being flagged because it makes a dbg.value undef. I've made an inline comment about this last point - undef dbg.values are currently not counted by this patch and I'm not sure if that should be the case.

You might be able to reduce (*) the number of false positive results by ignoring "redundant" debug intrinsics in the count, though maybe that could be follow-up work?

That could be follow-up for sure. Thanks. In addition, I am seeing a lot of artifitial variables location dropping (such as __result, __s etc.), and I am not sure if we should care about these.

I think the artificial variables are still useful to us. It may not matter to users so much if the locations are incorrect, but for us the intrinsics just provide more test coverage for this tool. wdyt?

llvm/lib/Transforms/Utils/Debugify.cpp

566

Why skip undef values here? Skipping undef values in the count means that any pass that makes a debug intrinsic undef will be flagged.

While making a debug intrinsic undef can sometimes be suboptimal (i.e. where a salvage or value replacement is possible), I don't think it ever reduces the correctness of the debug info.

[djtodoro]

In D100845#2707961, @Orlando wrote:

In D100845#2707872, @djtodoro wrote:

@Orlando Thanks for looking into this.

In D100845#2705685, @Orlando wrote:

Hi @djtodoro I am looking at this (slowly, sorry!).

IIUC this checks that the number of debug intrinsics for each non-inlined variable does not decrease after each optimisation pass. There are legitimate reasons for deleting debug intrinsics. RemoveRedundantDbgInstrs, for example, which is called in a few places. With that in mind I think I would prefer the table header "Number of bugs" to change as this number seems to be more of an indicator of possible bugs, rather than proof of their existence.

Sure. That is why we treat it as "WARNING". The same is happening with dropping of dbg location attached to instructions -- there are places where we cannot salvage !dbg, and we treat it as a warning as well (until we add some super cool logic to distinguish what was reasonable drop or opposite).

That makes sense, cool. Out of curiosity, did you run into many false positives before you found the ADCE bug (D100844)?

I am seeing some false positives for sure (most of these artifital vars locs are being removed after some "constant propagation" passes). We definitely need to investigate how to remove these false positives here (for both, instr and var locations).

I investigated another couple of dropped locations this morning which both look like false positive results too. One was in ipsccp where a dead block which contains a dbg.value is deleted. The other was in Early CSE w/ MemorySSA, which looks like it was being flagged because it makes a dbg.value undef. I've made an inline comment about this last point - undef dbg.values are currently not counted by this patch and I'm not sure if that should be the case.

I don't know how (yet), but we can (somehow) mark some places in pipeline as "valid for dropping" or something like that. Please note that not all setting of "undef" is the only thing we can do -- for example, calling the salvageDebugInfo() sometimes prevents a pass of making some llvm.dbg.value() first operand as undef.

You might be able to reduce (*) the number of false positive results by ignoring "redundant" debug intrinsics in the count, though maybe that could be follow-up work?

That could be follow-up for sure. Thanks. In addition, I am seeing a lot of artifitial variables location dropping (such as __result, __s etc.), and I am not sure if we should care about these.

I think the artificial variables are still useful to us. It may not matter to users so much if the locations are incorrect, but for us the intrinsics just provide more test coverage for this tool. wdyt?

llvm/lib/Transforms/Utils/Debugify.cpp

566

While making a debug intrinsic undef can sometimes be suboptimal (i.e. where a salvage or value replacement is possible), I don't think it ever reduces the correctness of the debug info.

Hmmm... it does not affect the correctness, but it does affect completeness (e.g. in optimized code, variable should be covered for the places it is alive, but in many cases it is impossible). How are we going to find cases where we have missed to call salvageDebugInfo()?
In addition, we can add some checking levels -- e.g. there could be level that doesn't consider "undef" locations, which will reduce the number of false positives, for sure...

[Orlando]

I am seeing some false positives for sure (most of these artifital vars locs are being removed after some "constant propagation" passes). We definitely need to investigate how to remove these false positives here (for both, instr and var locations).

I guess that can all fall under "follow up work" too? I think it is worth mentioning the caveat that there are known false positive cases in https://llvm.org/docs/HowToUpdateDebugInfo.html either way (for both source locations and this new debug intrinsic checking).

Bringing the undef discussion out from the inline comments to reduce clutter:

@Orlando said:
While making a debug intrinsic undef can sometimes be suboptimal (i.e. where a salvage or value replacement is possible), I don't think it ever reduces the correctness of the debug info.

@djtodoro said:
Hmmm... it does not affect the correctness, but it does affect completeness (e.g. in optimized code, variable should be covered for the places it is alive, but in many cases it is impossible). How are we going to find cases where we have missed to call salvageDebugInfo()?
In addition, we can add some checking levels -- e.g. there could be level that doesn't consider "undef" locations, which will reduce the number of false positives, for sure...

The best thing to do here probably depends on the goals of the tool. Is it trying to find passes that introduce correctness bugs or find passes that reduce coverage unnecessarily? If the answer is "both" then I agree that a checking-level option is probably a good way forward to help reduce the noise when looking for correctness bugs.

On the whole this SGTM but I haven't contributed to or recently used debugify so I would like see if anyone else has any comments on the approach in general before reviewing the code changes.

[djtodoro]

In D100845#2708702, @Orlando wrote:

I am seeing some false positives for sure (most of these artifital vars locs are being removed after some "constant propagation" passes). We definitely need to investigate how to remove these false positives here (for both, instr and var locations).

I guess that can all fall under "follow up work" too? I think it is worth mentioning the caveat that there are known false positive cases in https://llvm.org/docs/HowToUpdateDebugInfo.html either way (for both source locations and this new debug intrinsic checking).

Good suggestion, thanks!

Bringing the undef discussion out from the inline comments to reduce clutter:

@Orlando said:
While making a debug intrinsic undef can sometimes be suboptimal (i.e. where a salvage or value replacement is possible), I don't think it ever reduces the correctness of the debug info.

@djtodoro said:
Hmmm... it does not affect the correctness, but it does affect completeness (e.g. in optimized code, variable should be covered for the places it is alive, but in many cases it is impossible). How are we going to find cases where we have missed to call salvageDebugInfo()?
In addition, we can add some checking levels -- e.g. there could be level that doesn't consider "undef" locations, which will reduce the number of false positives, for sure...

The best thing to do here probably depends on the goals of the tool. Is it trying to find passes that introduce correctness bugs or find passes that reduce coverage unnecessarily? If the answer is "both" then I agree that a checking-level option is probably a good way forward to help reduce the noise when looking for correctness bugs.

On the whole this SGTM but I haven't contributed to or recently used debugify so I would like see if anyone else has any comments on the approach in general before reviewing the code changes.

[djtodoro]

• Add note for in docs for known false positives
• Refactor the code a bit
• Rebase

[StephenTozer]

This looks mostly good to me right now - I haven't gone through the HTML template code in-depth, but it looks to do what it says on the tin at least. I agree with the prior discussion that we will probably see a lot of false positives emerge from this, but I think the most important thing is that we have the tool. Once we have a larger spread of results, we can examine them and determine how best to tackle false positives so that we can get useful output.

llvm/lib/Transforms/Utils/Debugify.cpp

331

As of the current main, there is an isUndef function in DbgVariableIntrinsic that can be used for this check - though I've just noticed that it doesn't account for null operands, which is also something that should be handled in principle (since they aren't explicitly disallowed, and may be produced in a few places).

567

DVI->isUndef() can be used instead, as in above comment.

[djtodoro]

llvm/lib/Transforms/Utils/Debugify.cpp

331

OK, thanks -- I think the null op should be handled as well.

[djtodoro]

• address comments

[djtodoro]

This is ready to go now, right?

[StephenTozer]

LGTM - since this adds new output to debugify I would prefer if there was a review from someone with more authority regarding that sort of thing, but I think this should be harmless: this should only affect original DI preservation checks, since debugify does not add variables and should not be used on modules with existing variables, and I don't believe we depend on debugify's original DI preservation in any tests.

[StephenTozer]

In D100845#2765479, @StephenTozer wrote:

LGTM - since this adds new output to debugify I would prefer if there was a review from someone with more authority regarding that sort of thing, but I think this should be harmless: this should only affect original DI preservation checks, since debugify does not add variables and should not be used on modules with existing variables, and I don't believe we depend on debugify's original DI preservation in any tests.

Actually this needs to be corrected, debugify does add variables, but I believe that this change isn't going to affect check-debugify, is that correct?

[djtodoro]

In D100845#2765535, @StephenTozer wrote:

In D100845#2765479, @StephenTozer wrote:

LGTM - since this adds new output to debugify I would prefer if there was a review from someone with more authority regarding that sort of thing, but I think this should be harmless: this should only affect original DI preservation checks, since debugify does not add variables and should not be used on modules with existing variables, and I don't believe we depend on debugify's original DI preservation in any tests.

Actually this needs to be corrected, debugify does add variables, but I believe that this change isn't going to affect check-debugify, is that correct?

This affects DebugifyMode::OriginalDIMode only, that checks preservation original debug info (-g generated) in optimizations -- https://llvm.org/docs/HowToUpdateDebugInfo.html#test-original-debug-info-preservation-in-optimizations.

[djtodoro]

@StephenTozer Thanks for the review!

[jmorse]

Minor inline nits that can be skipped, plus a question about relying on DenseMap ordering.

Looks good, although I've no familiarity with the python.

llvm/lib/Transforms/Utils/Debugify.cpp

305–306

I know this isn't code changed by this patch; but can't we early-exit if there's no subprogram here? As far as I understand it, no subprogram means that there are no debugging intrinsics or !dbg attachments to be found in the function (and anything otherwise is a verifier error).

466

Given that DIFunctionsBefore is a DenseMap, and the order of iteration here is being preserved by the Bugs array, won't this be vulnerable to DenseMaps non-deterministic iteration order? (Easily fixed by making everything MapVectors).

543

The additions to this function seem very similar to the additions to collectDebugInfoMetadata, would it not benefit from a refactor / shared utilities? I don't know / feel enough about Debugify to know if there's some context I've missed.

llvm/unittests/Transforms/Utils/DebugifyTest.cpp

66–68

Mega nit, this is redundant given the loop below simply won't iterate over Dbgs if it's empty, no? (This might be a style thing).

[djtodoro]

• address a comment

[djtodoro]

llvm/lib/Transforms/Utils/Debugify.cpp

305–306

There could be a case where we have forgotten to do F->setSubprogram(SP) when doing some optimizations on F (more precisely, newF->setSubprogram(SP) -- but newF is representing the same function F). I've found a case like that when doing full LLVM-projects build.

466

We have used the DenseMap for each DI Metadata checking, since we thought the order of reported bugs doesn't matter; do you think we should care about it?

543

I guess there are existing lines as well that are very similar in both collectDebugInfoMetadata() and checkDebugInfoMetadata(). Can that refactoring be done as an incremental NFC patch?

llvm/unittests/Transforms/Utils/DebugifyTest.cpp

66–68

It makes sense :)

[jmorse]

llvm/lib/Transforms/Utils/Debugify.cpp

466

IMO yes: any kind of unexpected variation in the output is going to be fatal to anyone trying to script around this tooling. We should be conservative in what's outputted.

543

Works for me.

[djtodoro]

llvm/lib/Transforms/Utils/Debugify.cpp

466

OK, sure. This https://reviews.llvm.org/D102841 addresses that.

[jmorse]

LGTM with D102841

llvm/docs/HowToUpdateDebugInfo.rst

 .. code-block:: bash
 
   # Test each pass.
   $ clang -Xclang -fverify-debuginfo-preserve -g -O2 sample.c
 
   # Test each pass and export the issues report into the JSON file.
   $ clang -Xclang -fverify-debuginfo-preserve -Xclang -fverify-debuginfo-preserve-export=sample.json -g -O2 sample.c
 
+Please do note that there are some known false positives, for source locations
+and debug intrinsic checking, so that will be addressed as a future work.
+
 Mutation testing for MIR-level transformations
 ----------------------------------------------
 
 A variant of the ``debugify`` utility described in
 :ref:`Mutation testing for IR-level transformations<IRDebugify>` can be used
 for MIR-level transformations as well: much like the IR-level pass,
 ``mir-debugify`` inserts sequentially increasing line locations to each
 ``MachineInstr`` in a ``Module``. And the MIR-level ``mir-check-debugify`` is

llvm/include/llvm/Transforms/Utils/Debugify.h

 #include "llvm/Bitcode/BitcodeWriterPass.h"
 #include "llvm/IR/IRPrintingPasses.h"
 #include "llvm/IR/LegacyPassManager.h"
 #include "llvm/IR/PassManager.h"
 #include "llvm/IR/ValueHandle.h"
 
 using DebugFnMap = llvm::DenseMap<llvm::StringRef, const llvm::DISubprogram *>;
 using DebugInstMap = llvm::DenseMap<const llvm::Instruction *, bool>;
+using DebugVarMap = llvm::DenseMap<const llvm::DILocalVariable *, unsigned>;
 using WeakInstValueMap =
     llvm::DenseMap<const llvm::Instruction *, llvm::WeakVH>;
 
 /// Used to track the Debug Info Metadata information.
 struct DebugInfoPerPass {
   // This maps a function name to its associated DISubprogram.
   DebugFnMap DIFunctions;
   // This maps an instruction and the info about whether it has !dbg attached.
   DebugInstMap DILocations;
   // This tracks value (instruction) deletion. If an instruction gets deleted,
   // WeakVH nulls itself.
   WeakInstValueMap InstToDelete;
+  // Maps variable into dbg users (#dbg values/declares for this variable).
+  DebugVarMap DIVariables;
 };
 
 /// Map pass names to a per-pass DebugInfoPerPass instance.
 using DebugInfoPerPassMap = llvm::MapVector<llvm::StringRef, DebugInfoPerPass>;
 
 namespace llvm {
 class DIBuilder;
 

llvm/lib/Transforms/Utils/Debugify.cpp

bool llvm::collectDebugInfoMetadata(Module &M,
   }
 
   // Visit each instruction.
   for (Function &F : Functions) {
     if (isFunctionSkipped(F))
       continue;
 
     // Collect the DISubprogram.
     auto *SP = F.getSubprogram();
     DIPreservationMap[NameOfWrappedPass].DIFunctions.insert({F.getName(), SP});
-    if (SP)
+    if (SP) {
       LLVM_DEBUG(dbgs() << "  Collecting subprogram: " << *SP << '\n');
+      for (const DINode *DN : SP->getRetainedNodes()) {
+        if (const auto *DV = dyn_cast<DILocalVariable>(DN)) {
+          DIPreservationMap[NameOfWrappedPass].DIVariables[DV] = 0;
+        }
+      }
+    }
 
     for (BasicBlock &BB : F) {
-      // Collect debug locations (!dbg).
-      // TODO: Collect dbg.values.
+      // Collect debug locations (!dbg) and debug variable intrinsics.
       for (Instruction &I : BB) {
         // Skip PHIs.
         if (isa<PHINode>(I))
           continue;
 
-        // Skip debug instructions.
+        // Collect dbg.values and dbg.declares.
+        if (auto *DVI = dyn_cast<DbgVariableIntrinsic>(&I)) {
+          if (!SP)
+            continue;
+          // Skip inlined variables.
+          if (I.getDebugLoc().getInlinedAt())
+            continue;
+          // Skip undef values.
+          auto *Op = DVI->getVariableLocationOp(0);
+          if (isa_and_nonnull<UndefValue>(Op))
+            continue;
+
+          auto *Var = DVI->getVariable();
+          DIPreservationMap[NameOfWrappedPass].DIVariables[Var]++;
+          continue;
+        }
+
+        // Skip debug instructions other than dbg.value and dbg.declare.
         if (isa<DbgInfoIntrinsic>(&I))
           continue;
 
         LLVM_DEBUG(dbgs() << "  Collecting info for inst: " << I << '\n');
         DIPreservationMap[NameOfWrappedPass].InstToDelete.insert({&I, &I});
 
         const DILocation *Loc = I.getDebugLoc().get();
         bool HasLoc = Loc != nullptr;
if (InstrIt == DILocsBefore.end()) {
               << ", File: " << FileNameFromCU << ")\n";
       Preserved = false;
     }
   }
 
   return Preserved;
 }
 
+// This checks the preservation of original debug variable intrinsics.
+static bool checkVars(const DebugVarMap &DIFunctionsBefore,
+                      const DebugVarMap &DIFunctionsAfter,
+                      StringRef NameOfWrappedPass, StringRef FileNameFromCU,
+                      bool ShouldWriteIntoJSON, llvm::json::Array &Bugs) {
+  bool Preserved = true;
+  for (const auto &V : DIFunctionsBefore) {
+    auto VarIt = DIFunctionsAfter.find(V.first);
+    if (VarIt == DIFunctionsAfter.end())
+      continue;
+
+    unsigned NumOfDbgValsAfter = VarIt->second;
+
+    if (V.second > NumOfDbgValsAfter) {
+      if (ShouldWriteIntoJSON)
+        Bugs.push_back(llvm::json::Object(
+            {{"metadata", "dbg-var-intrinsic"},
+             {"name", V.first->getName()},
+             {"fn-name", V.first->getScope()->getSubprogram()->getName()},
+             {"action", "drop"}}));
+      else
+        dbg() << "WARNING: " << NameOfWrappedPass
+              << " drops dbg.value()/dbg.declare() for " << V.first->getName()
+              << " from "
+              << "function " << V.first->getScope()->getSubprogram()->getName()
+              << " (file " << FileNameFromCU << ")\n";
+      Preserved = false;
+    }
+  }
+
+  return Preserved;
+}
+
 // Write the json data into the specifed file.
 static void writeJSON(StringRef OrigDIVerifyBugsReportFilePath,
                       StringRef FileNameFromCU, StringRef NameOfWrappedPass,
                       llvm::json::Array &Bugs) {
   std::error_code EC;
   raw_fd_ostream OS_FILE{OrigDIVerifyBugsReportFilePath, EC,
                          sys::fs::OF_Append | sys::fs::OF_TextWithCRLF};
   if (EC) {
for (Function &F : Functions) {
     if (isFunctionSkipped(F))
       continue;
 
     // TODO: Collect metadata other than DISubprograms.
     // Collect the DISubprogram.
     auto *SP = F.getSubprogram();
     DIPreservationAfter[NameOfWrappedPass].DIFunctions.insert(
         {F.getName(), SP});
-    if (SP)
+
+    if (SP) {
       LLVM_DEBUG(dbgs() << "  Collecting subprogram: " << *SP << '\n');
+      for (const DINode *DN : SP->getRetainedNodes()) {
+        if (const auto *DV = dyn_cast<DILocalVariable>(DN)) {
+          DIPreservationAfter[NameOfWrappedPass].DIVariables[DV] = 0;
+        }
+      }
+    }
 
     for (BasicBlock &BB : F) {
-      // Collect debug locations (!dbg attachments).
-      // TODO: Collect dbg.values.
+      // Collect debug locations (!dbg) and debug variable intrinsics.
       for (Instruction &I : BB) {
         // Skip PHIs.
         if (isa<PHINode>(I))
           continue;
 
-        // Skip debug instructions.
+        // Collect dbg.values and dbg.declares.
+        if (auto *DVI = dyn_cast<DbgVariableIntrinsic>(&I)) {
+          if (!SP)
+            continue;
+          // Skip inlined variables.
+          if (I.getDebugLoc().getInlinedAt())
+            continue;
+          // Skip undef values.
+          auto *Op = DVI->getVariableLocationOp(0);
+          if (isa_and_nonnull<UndefValue>(Op))
+            continue;
+
+          auto *Var = DVI->getVariable();
+          DIPreservationAfter[NameOfWrappedPass].DIVariables[Var]++;
+          continue;
+        }
+
+        // Skip debug instructions other than dbg.value and dbg.declare.
         if (isa<DbgInfoIntrinsic>(&I))
           continue;
 
         LLVM_DEBUG(dbgs() << "  Collecting info for inst: " << I << '\n');
 
         const DILocation *Loc = I.getDebugLoc().get();
         bool HasLoc = Loc != nullptr;
 
bool llvm::checkDebugInfoMetadata(Module &M,
   auto DIFunctionsBefore = DIPreservationMap[NameOfWrappedPass].DIFunctions;
   auto DIFunctionsAfter = DIPreservationAfter[NameOfWrappedPass].DIFunctions;
 
   auto DILocsBefore = DIPreservationMap[NameOfWrappedPass].DILocations;
   auto DILocsAfter = DIPreservationAfter[NameOfWrappedPass].DILocations;
 
   auto InstToDelete = DIPreservationAfter[NameOfWrappedPass].InstToDelete;
 
+  auto DIVarsBefore = DIPreservationMap[NameOfWrappedPass].DIVariables;
+  auto DIVarsAfter = DIPreservationAfter[NameOfWrappedPass].DIVariables;
+
   bool ShouldWriteIntoJSON = !OrigDIVerifyBugsReportFilePath.empty();
   llvm::json::Array Bugs;
 
   bool ResultForFunc =
       checkFunctions(DIFunctionsBefore, DIFunctionsAfter, NameOfWrappedPass,
                      FileNameFromCU, ShouldWriteIntoJSON, Bugs);
   bool ResultForInsts = checkInstructions(
       DILocsBefore, DILocsAfter, InstToDelete, NameOfWrappedPass,
       FileNameFromCU, ShouldWriteIntoJSON, Bugs);
-  bool Result = ResultForFunc && ResultForInsts;
+
+  bool ResultForVars = checkVars(DIVarsBefore, DIVarsAfter, NameOfWrappedPass,
+                                 FileNameFromCU, ShouldWriteIntoJSON, Bugs);
+
+  bool Result = ResultForFunc && ResultForInsts && ResultForVars;
 
   StringRef ResultBanner = NameOfWrappedPass != "" ? NameOfWrappedPass : Banner;
   if (ShouldWriteIntoJSON && !Bugs.empty())
     writeJSON(OrigDIVerifyBugsReportFilePath, FileNameFromCU, NameOfWrappedPass,
               Bugs);
 
   if (Result)
     dbg() << ResultBanner << ": PASS\n";

llvm/test/tools/llvm-original-di-preservation/Inputs/expected-sample.html

<tr>
       <th>LLVM Pass Name</th>
     <th>Number of bugs</th>
   </tr>
   <tr>
 <tr>
         <td colspan='2'> No bugs found </td>
       </tr>
     </table>
+<br>
+<br>
+<table>
+  <caption><b>Variable Location Bugs found by the Debugify</b></caption>
+  <tr>
+      <th>File</th>
+    <th>LLVM Pass Name</th>
+    <th>Variable</th>
+    <th>Function</th>
+    <th>Action</th>
+  </tr>
+<tr>
+        <td colspan='4'> No bugs found </td>
+      </tr>
+    </table>
+<br>
+<table>
+  <caption><b>Summary of Variable Location Bugs</b></caption>
+  <tr>
+      <th>LLVM Pass Name</th>
+    <th>Number of bugs</th>
+  </tr>
+  <tr>
+<tr>
+        <td colspan='2'> No bugs found </td>
+      </tr>
+    </table>
 </body>
   </html>
 No newline at end of file

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 "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)
bool runOnFunction(Function &F) override {
     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 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))
+          Dbgs.push_back(DVI);
+      }
+    }
+
+    if (Dbgs.empty())
+      return false;
+
+    for (auto &I : Dbgs)
+      I->eraseFromParent();
+
+    return false;
+  }
+
+  void getAnalysisUsage(AnalysisUsage &AU) const override {
+    AU.setPreservesCFG();
+  }
+
+  DebugValueDrop() : FunctionPass(ID) {}
+};
+
 struct DebugInfoDummyAnalysis : public FunctionPass {
   static char ID;
   bool runOnFunction(Function &F) override {
     // Do nothing, so debug info stays untouched.
     return false;
   }
   void getAnalysisUsage(AnalysisUsage &AU) const override {
     AU.setPreservesAll();
   }
 
   DebugInfoDummyAnalysis() : FunctionPass(ID) {}
 };
 }
 
 char DebugInfoDrop::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
       call void @llvm.dbg.value(metadata i16 %b, metadata !9, metadata !DIExpression()), !dbg !11
TEST(DebugInfoDrop, DropOriginalDebugInfo) {
   std::string WarningForLoc = "WARNING:  dropped DILocation of";
   std::string FinalResult = "CheckModuleDebugify (original debuginfo): FAIL";
 
   EXPECT_TRUE(StdOut.find(ErrorForSP) != std::string::npos);
   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 {
+      %b = add i16 %a, 1, !dbg !11
+      call void @llvm.dbg.value(metadata i16 %b, metadata !9, metadata !DIExpression()), !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)
+  )");
+
+  DebugValueDrop *P = new DebugValueDrop();
+
+  DebugInfoPerPassMap DIPreservationMap;
+  DebugifyCustomPassManager Passes;
+  Passes.setDIPreservationMap(DIPreservationMap);
+  Passes.add(createDebugifyModulePass(DebugifyMode::OriginalDebugInfo, "",
+                                      &(Passes.getDebugInfoPerPassMap())));
+  Passes.add(P);
+  Passes.add(createCheckDebugifyModulePass(false, "", nullptr,
+                                           DebugifyMode::OriginalDebugInfo,
+                                           &(Passes.getDebugInfoPerPassMap())));
+
+  testing::internal::CaptureStderr();
+  Passes.run(*M);
+
+  std::string StdOut = testing::internal::GetCapturedStderr();
+
+  std::string ErrorForSP = "ERROR:  dropped DISubprogram of";
+  std::string WarningForLoc = "WARNING:  dropped DILocation of";
+  std::string WarningForVars = "WARNING:  drops dbg.value()/dbg.declare() for";
+  std::string FinalResult = "CheckModuleDebugify (original debuginfo): FAIL";
+
+  EXPECT_TRUE(StdOut.find(ErrorForSP) == std::string::npos);
+  EXPECT_TRUE(StdOut.find(WarningForLoc) == std::string::npos);
+  EXPECT_TRUE(StdOut.find(WarningForVars) != std::string::npos);
+  EXPECT_TRUE(StdOut.find(FinalResult) != std::string::npos);
+}
+
 TEST(DebugInfoDummyAnalysis, PreserveOriginalDebugInfo) {
   LLVMContext C;
   std::unique_ptr<Module> M = parseIR(C, R"(
     define i32 @g(i32 %b) !dbg !6 {
       %c = add i32 %b, 1, !dbg !11
       call void @llvm.dbg.value(metadata i32 %c, metadata !9, metadata !DIExpression()), !dbg !11
       ret i32 1, !dbg !11
     }
TEST(DebugInfoDummyAnalysis, PreserveOriginalDebugInfo) {
 
   testing::internal::CaptureStderr();
   Passes.run(*M);
 
   std::string StdOut = testing::internal::GetCapturedStderr();
 
   std::string ErrorForSP = "ERROR:  dropped DISubprogram of";
   std::string WarningForLoc = "WARNING:  dropped DILocation of";
+  std::string WarningForVars = "WARNING:  drops dbg.value()/dbg.declare() for";
   std::string FinalResult = "CheckModuleDebugify (original debuginfo): PASS";
 
   EXPECT_TRUE(StdOut.find(ErrorForSP) == std::string::npos);
   EXPECT_TRUE(StdOut.find(WarningForLoc) == std::string::npos);
+  EXPECT_TRUE(StdOut.find(WarningForVars) == std::string::npos);
   EXPECT_TRUE(StdOut.find(FinalResult) != std::string::npos);
 }
 
 } // end namespace llvm
 
 INITIALIZE_PASS_BEGIN(DebugInfoDrop, "debuginfodroppass", "debuginfodroppass",
                       false, false)
 INITIALIZE_PASS_END(DebugInfoDrop, "debuginfodroppass", "debuginfodroppass", false,
                     false)
 
 INITIALIZE_PASS_BEGIN(DebugInfoDummyAnalysis, "debuginfodummyanalysispass",
                       "debuginfodummyanalysispass", false, false)
 INITIALIZE_PASS_END(DebugInfoDummyAnalysis, "debuginfodummyanalysispass",
                     "debuginfodummyanalysispass", false, false)

llvm/utils/llvm-original-di-preservation.py

def __init__(self, action, bb_name, fn_name, instr):
     self.fn_name = fn_name
     self.instr = instr
 
 class DISPBug:
   def __init__(self, action, fn_name):
     self.action = action
     self.fn_name = fn_name
 
+class DIVarBug:
+  def __init__(self, action, name, fn_name):
+    self.action = action
+    self.name = name
+    self.fn_name = fn_name
+
 # Report the bugs in form of html.
-def generate_html_report(di_location_bugs, di_subprogram_bugs, \
+def generate_html_report(di_location_bugs, di_subprogram_bugs, di_var_bugs, \
                          di_location_bugs_summary, di_sp_bugs_summary, \
-                         html_file):
+                         di_var_bugs_summary, html_file):
   fileout = open(html_file, "w")
 
   html_header = """ <html>
   <head>
   <style>
   table, th, td {
     border: 1px solid black;
   }
def generate_html_report(di_location_bugs, di_subprogram_bugs, di_var_bugs, \
   header_di_sp = ["File", "LLVM Pass Name", "Function Name", "Action"]
 
   for column in header_di_sp:
     table_di_sp += "    <th>{0}</th>\n".format(column.strip())
   table_di_sp += "  </tr>\n"
 
   at_least_one_bug_found = False
 
-  # Handle loction bugs.
+  # Handle fn bugs.
   for file, per_file_bugs in di_subprogram_bugs.items():
     for llvm_pass, per_pass_bugs in per_file_bugs.items():
       # No SP bugs for the pass.
       if len(per_pass_bugs) == 0:
         continue
       at_least_one_bug_found = True
       row = []
       table_di_sp += "  </tr>\n"
def generate_html_report(di_location_bugs, di_subprogram_bugs, di_var_bugs, \
 
   if not at_least_one_bug_found:
     table_di_sp_sum += """<tr>
         <td colspan='2'> No bugs found </td>
       </tr>
     """
   table_di_sp_sum += "</table>\n"
 
+  # Create the table for Variable bugs.
+  table_title_di_var = "Variable Location Bugs found by the Debugify"
+  table_di_var = """<table>
+  <caption><b>{}</b></caption>
+  <tr>
+  """.format(table_title_di_var)
+
+  header_di_var = ["File", "LLVM Pass Name", "Variable", "Function", "Action"]
+
+  for column in header_di_var:
+    table_di_var += "    <th>{0}</th>\n".format(column.strip())
+  table_di_var += "  </tr>\n"
+
+  at_least_one_bug_found = False
+
+  # Handle var bugs.
+  for file, per_file_bugs in di_var_bugs.items():
+    for llvm_pass, per_pass_bugs in per_file_bugs.items():
+      # No SP bugs for the pass.
+      if len(per_pass_bugs) == 0:
+        continue
+      at_least_one_bug_found = True
+      row = []
+      table_di_var += "  </tr>\n"
+      # Get the bugs info.
+      for x in per_pass_bugs:
+        row.append("    <tr>\n")
+        row.append(file)
+        row.append(llvm_pass)
+        row.append(x.name)
+        row.append(x.fn_name)
+        row.append(x.action)
+        row.append("    </tr>\n")
+      # Dump the bugs info into the table.
+      for column in row:
+        # The same file-pass pair can have multiple bugs.
+        if (column == "    <tr>\n" or column == "    </tr>\n"):
+          table_di_var += column
+          continue
+        table_di_var += "    <td>{0}</td>\n".format(column.strip())
+      table_di_var += "  <tr>\n"
+
+  if not at_least_one_bug_found:
+    table_di_var += """<tr>
+        <td colspan='4'> No bugs found </td>
+      </tr>
+    """
+  table_di_var += "</table>\n"
+
+  # Create the summary table for the sp bugs.
+  table_title_di_var_sum = "Summary of Variable Location Bugs"
+  table_di_var_sum = """<table>
+  <caption><b>{}</b></caption>
+  <tr>
+  """.format(table_title_di_var_sum)
+
+  header_di_var_sum = ["LLVM Pass Name", "Number of bugs"]
+
+  for column in header_di_var_sum:
+    table_di_var_sum += "    <th>{0}</th>\n".format(column.strip())
+  table_di_var_sum += "  </tr>\n"
+
+  # Print the summary.
+  row = []
+  for llvm_pass, num in sorted(di_var_bugs_summary.items()):
+    row.append("    <tr>\n")
+    row.append(llvm_pass)
+    row.append(str(num))
+    row.append("    </tr>\n")
+  for column in row:
+    if (column == "    <tr>\n" or column == "    </tr>\n"):
+      table_di_var_sum += column
+      continue
+    table_di_var_sum += "    <td>{0}</td>\n".format(column.strip())
+  table_di_var_sum += "  <tr>\n"
+
+  if not at_least_one_bug_found:
+    table_di_var_sum += """<tr>
+        <td colspan='2'> No bugs found </td>
+      </tr>
+    """
+  table_di_var_sum += "</table>\n"
+
   # Finish the html page.
   html_footer = """</body>
   </html>"""
 
   new_line = "<br>\n"
 
   fileout.writelines(html_header)
   fileout.writelines(table_di_loc)
   fileout.writelines(new_line)
   fileout.writelines(table_di_loc_sum)
   fileout.writelines(new_line)
   fileout.writelines(new_line)
   fileout.writelines(table_di_sp)
   fileout.writelines(new_line)
   fileout.writelines(table_di_sp_sum)
+  fileout.writelines(new_line)
+  fileout.writelines(new_line)
+  fileout.writelines(table_di_var)
+  fileout.writelines(new_line)
+  fileout.writelines(table_di_var_sum)
   fileout.writelines(html_footer)
   fileout.close()
 
   print("The " + html_file + " generated.")
 
 # Read the JSON file.
 def get_json(file):
   json_parsed = None
if not opts.html_file.endswith('.html'):
     print ("error: The output file must be '.html'.")
     sys.exit(1)
 
   debug_info_bugs = get_json(opts.file_name)
 
   # Use the defaultdict in order to make multidim dicts.
   di_location_bugs = defaultdict(lambda: defaultdict(dict))
   di_subprogram_bugs = defaultdict(lambda: defaultdict(dict))
+  di_variable_bugs = defaultdict(lambda: defaultdict(dict))
 
   # Use the ordered dict to make a summary.
   di_location_bugs_summary = OrderedDict()
   di_sp_bugs_summary = OrderedDict()
+  di_var_bugs_summary = OrderedDict()
 
   # Map the bugs into the file-pass pairs.
   for bugs_per_pass in debug_info_bugs:
     bugs_file = bugs_per_pass["file"]
     bugs_pass = bugs_per_pass["pass"]
 
     bugs = bugs_per_pass["bugs"][0]
 
     di_loc_bugs = []
     di_sp_bugs = []
+    di_var_bugs = []
+
     for bug in bugs:
       bugs_metadata = bug["metadata"]
       if bugs_metadata == "DILocation":
         action = bug["action"]
         bb_name = bug["bb-name"]
         fn_name = bug["fn-name"]
         instr = bug["instr"]
         di_loc_bugs.append(DILocBug(action, bb_name, fn_name, instr))
 
         # Fill the summary dict.
         if bugs_pass in di_location_bugs_summary:
           di_location_bugs_summary[bugs_pass] += 1
         else:
           di_location_bugs_summary[bugs_pass] = 1
       elif bugs_metadata == "DISubprogram":
         action = bug["action"]
         name = bug["name"]
         di_sp_bugs.append(DISPBug(action, name))
 
         # Fill the summary dict.
         if bugs_pass in di_sp_bugs_summary:
           di_sp_bugs_summary[bugs_pass] += 1
         else:
           di_sp_bugs_summary[bugs_pass] = 1
+      elif bugs_metadata == "dbg-var-intrinsic":
+        action = bug["action"]
+        fn_name = bug["fn-name"]
+        name = bug["name"]
+        di_var_bugs.append(DIVarBug(action, name, fn_name))
+
+        # Fill the summary dict.
+        if bugs_pass in di_var_bugs_summary:
+          di_var_bugs_summary[bugs_pass] += 1
+        else:
+          di_var_bugs_summary[bugs_pass] = 1
       else:
-        print ("error: Only DILocation and DISubprogram are supported.")
+        print ("error: Unsupported metadata.")
         sys.exit(1)
 
     di_location_bugs[bugs_file][bugs_pass] = di_loc_bugs
     di_subprogram_bugs[bugs_file][bugs_pass] = di_sp_bugs
+    di_variable_bugs[bugs_file][bugs_pass] = di_var_bugs
 
-  generate_html_report(di_location_bugs, di_subprogram_bugs, \
+  generate_html_report(di_location_bugs, di_subprogram_bugs, di_variable_bugs, \
                        di_location_bugs_summary, di_sp_bugs_summary, \
-                       opts.html_file)
+                       di_var_bugs_summary, opts.html_file)
 
 if __name__ == "__main__":
   Main()
   sys.exit(0)