[Debugify] Strip added metadata in the -debugify-each pipeline

[vsk]

Summary

Share logic to strip debugify metadata between the IR and MIR level
debugify passes. This makes it simpler to hunt for bugs by diffing IR
with vs. without -debugify-each turned on.

As a drive-by, fix an issue causing CallGraphNodes to become invalid
when a dead llvm.dbg.value prototype is deleted.

[dsanders]

LGTM with the behavioural change to StripDebugMachineModule fixed

llvm/include/llvm/IR/IntrinsicInst.h

84–85

I think there might be potential for confusion between isa<DbgInfoIntrinsic>() and DbgInfoIntrinsic::isA(). Maybe isInstForInstrinsic(), or implementsInstrinsic()?

llvm/lib/Transforms/Utils/Debugify.cpp

183–185

This will stop StripDebugMachineModule stripping debug info when OnlyDebugified is false.

[vsk]

Address review feedback.

[vsk]

Well, it looks like arcanist ate my patch and spit out a different one. Let me try that again...

[vsk]

Second try to upload patch with arcanist.

[aprantl]

llvm/lib/Transforms/Utils/Debugify.cpp

203

I don't think there is, but this could become a helper in MDNode.h

llvm/include/llvm/IR/IntrinsicInst.h

static bool classof(const CallInst *I) {
         return CF->isIntrinsic();
       return false;
     }
     static bool classof(const Value *V) {
       return isa<CallInst>(V) && classof(cast<CallInst>(V));
     }
   };
 
-  /// This is the common base class for debug info intrinsics.
-  class DbgInfoIntrinsic : public IntrinsicInst {
-  public:
-    /// \name Casting methods
-    /// @{
-    static bool classof(const IntrinsicInst *I) {
-      switch (I->getIntrinsicID()) {
-      case Intrinsic::dbg_declare:
-      case Intrinsic::dbg_value:
-      case Intrinsic::dbg_addr:
-      case Intrinsic::dbg_label:
-        return true;
-      default: return false;
-      }
+  /// Check if \p ID corresponds to a debug info intrinsic.
+  static inline bool isDbgInfoIntrinsic(Intrinsic::ID ID) {
+    switch (ID) {
+    default:
+      return false;
+    }
+
+  /// This is the common base class for debug info intrinsics.
+  class DbgInfoIntrinsic : public IntrinsicInst {
+  public:
+    /// \name Casting methods
+    /// @{
+    static bool classof(const IntrinsicInst *I) {
+      return isDbgInfoIntrinsic(I->getIntrinsicID());
     }
     static bool classof(const Value *V) {
       return isa<IntrinsicInst>(V) && classof(cast<IntrinsicInst>(V));
     }
     /// @}
   };
 
   /// This is the common base class for debug info intrinsics for variables.

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

 /// \param Functions A range of functions to add debug information to.
 /// \param Banner A prefix string to add to debug/error messages.
 /// \param ApplyToMF A call back that will add debug information to the
 ///                  MachineFunction for a Function. If nullptr, then the
 ///                  MachineFunction (if any) will not be modified.
 bool applyDebugifyMetadata(
     Module &M, iterator_range<Module::iterator> Functions, StringRef Banner,
     std::function<bool(DIBuilder &, Function &)> ApplyToMF);
+
+/// Strip out all of the metadata and debug info inserted by debugify. If no
+/// llvm.debugify module-level named metadata is present, this is a no-op.
+/// Returns true if any change was made.
+bool stripDebugifyMetadata(Module &M);
+
 } // namespace llvm
 
 llvm::ModulePass *createDebugifyModulePass();
 llvm::FunctionPass *createDebugifyFunctionPass();
 
 struct NewPMDebugifyPass : public llvm::PassInfoMixin<NewPMDebugifyPass> {
   llvm::PreservedAnalyses run(llvm::Module &M, llvm::ModuleAnalysisManager &AM);
 };

llvm/lib/Analysis/CallGraph.cpp

 //===- CallGraph.cpp - Build a Module's call graph ------------------------===//
 //
 // 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/Analysis/CallGraph.h"
 #include "llvm/ADT/STLExtras.h"
 #include "llvm/ADT/SmallVector.h"
 #include "llvm/Config/llvm-config.h"
 #include "llvm/IR/Function.h"
+#include "llvm/IR/IntrinsicInst.h"
 #include "llvm/IR/Intrinsics.h"
 #include "llvm/IR/Module.h"
 #include "llvm/IR/PassManager.h"
 #include "llvm/InitializePasses.h"
 #include "llvm/Pass.h"
 #include "llvm/Support/Compiler.h"
 #include "llvm/Support/Debug.h"
 #include "llvm/Support/raw_ostream.h"
 #include <algorithm>
 #include <cassert>
 
 using namespace llvm;
 
 //===----------------------------------------------------------------------===//
 // Implementations of the CallGraph class methods.
 //
 
 CallGraph::CallGraph(Module &M)
     : M(M), ExternalCallingNode(getOrInsertFunction(nullptr)),
       CallsExternalNode(std::make_unique<CallGraphNode>(nullptr)) {
-  // Add every function to the call graph.
+  // Add every interesting function to the call graph.
   for (Function &F : M)
+    if (!isDbgInfoIntrinsic(F.getIntrinsicID()))
       addToCallGraph(&F);
 }
 
 CallGraph::CallGraph(CallGraph &&Arg)
     : M(Arg.M), FunctionMap(std::move(Arg.FunctionMap)),
       ExternalCallingNode(Arg.ExternalCallingNode),
       CallsExternalNode(std::move(Arg.CallsExternalNode)) {
   Arg.FunctionMap.clear();
   Arg.ExternalCallingNode = nullptr;

llvm/lib/CodeGen/MachineStripDebug.cpp

 //===----------------------------------------------------------------------===//
 
 #include "llvm/CodeGen/MachineFunctionPass.h"
 #include "llvm/CodeGen/MachineModuleInfo.h"
 #include "llvm/CodeGen/Passes.h"
 #include "llvm/IR/DebugInfo.h"
 #include "llvm/InitializePasses.h"
 #include "llvm/Support/CommandLine.h"
+#include "llvm/Transforms/Utils/Debugify.h"
 
 #define DEBUG_TYPE "mir-strip-debug"
 
 using namespace llvm;
 
 namespace {
 cl::opt<bool>
     OnlyDebugifiedDefault("mir-strip-debugify-only",
for (Function &F : M.functions()) {
             continue;
           }
           LLVM_DEBUG(dbgs() << "Keeping " << *I);
           ++I;
         }
       }
     }
 
-    Changed |= StripDebugInfo(M);
-
-    NamedMDNode *NMD = M.getNamedMetadata("llvm.debugify");
-    if (NMD) {
-      NMD->eraseFromParent();
-      Changed |= true;
-    }
-
-    NMD = M.getModuleFlagsMetadata();
-    if (NMD) {
-      // There must be an easier way to remove an operand from a NamedMDNode.
-      SmallVector<MDNode *, 4> Flags;
-      for (MDNode *Flag : NMD->operands())
-        Flags.push_back(Flag);
-      NMD->clearOperands();
-      for (MDNode *Flag : Flags) {
-        MDString *Key = dyn_cast_or_null<MDString>(Flag->getOperand(1));
-        if (Key->getString() == "Debug Info Version") {
-          Changed |= true;
-          continue;
-        }
-        NMD->addOperand(Flag);
-      }
-      // If we left it empty we might as well remove it.
-      if (NMD->getNumOperands() == 0)
-        NMD->eraseFromParent();
-    }
+    Changed |= stripDebugifyMetadata(M);
 
     return Changed;
   }
 
   StripDebugMachineModule() : StripDebugMachineModule(OnlyDebugifiedDefault) {}
   StripDebugMachineModule(bool OnlyDebugified)
       : ModulePass(ID), OnlyDebugified(OnlyDebugified) {}
 

llvm/lib/Transforms/Utils/Debugify.cpp

bool llvm::applyDebugifyMetadata(
   // Claim that this synthetic debug info is valid.
   StringRef DIVersionKey = "Debug Info Version";
   if (!M.getModuleFlag(DIVersionKey))
     M.addModuleFlag(Module::Warning, DIVersionKey, DEBUG_METADATA_VERSION);
 
   return true;
 }
 
+bool llvm::stripDebugifyMetadata(Module &M) {
+  bool Changed = false;
+
+  // Remove the llvm.debugify module-level named metadata.
+  NamedMDNode *DebugifyMD = M.getNamedMetadata("llvm.debugify");
+  if (DebugifyMD) {
+    M.eraseNamedMetadata(DebugifyMD);
+    Changed = true;
+  }
+
+  // Strip out all debug intrinsics and supporting metadata (subprograms, types,
+  // variables, etc).
+  Changed |= StripDebugInfo(M);
+
+  // Strip out the dead dbg.value prototype.
+  Function *DbgValF = M.getFunction("llvm.dbg.value");
+  if (DbgValF) {
+    assert(DbgValF->isDeclaration() && DbgValF->use_empty() &&
+           "Not all debug info stripped?");
+    DbgValF->eraseFromParent();
+    Changed = true;
+  }
+
+  // Strip out the module-level Debug Info Version metadata.
+  // FIXME: There must be an easier way to remove an operand from a NamedMDNode.
+  NamedMDNode *NMD = M.getModuleFlagsMetadata();
+  assert(NMD && "debugify metadata present without Debug Info Version set?");
+  SmallVector<MDNode *, 4> Flags;
+  for (MDNode *Flag : NMD->operands())
+    Flags.push_back(Flag);
+  NMD->clearOperands();
+  for (MDNode *Flag : Flags) {
+    MDString *Key = dyn_cast_or_null<MDString>(Flag->getOperand(1));
+    if (Key->getString() == "Debug Info Version") {
+      Changed = true;
+      continue;
+    }
+    NMD->addOperand(Flag);
+  }
+  // If we left it empty we might as well remove it.
+  if (NMD->getNumOperands() == 0)
+    NMD->eraseFromParent();
+
+  return Changed;
+}
+
 namespace {
 /// Return true if a mis-sized diagnostic is issued for \p DVI.
 bool diagnoseMisSizedDbgValue(Module &M, DbgValueInst *DVI) {
   // The size of a dbg.value's value operand should match the size of the
   // variable it corresponds to.
   //
   // TODO: This, along with a check for non-null value operands, should be
   // promoted to verifier failures.
if (Stats) {
     Stats->NumDbgValuesMissing += MissingVars.count();
   }
 
   dbg() << Banner;
   if (!NameOfWrappedPass.empty())
     dbg() << " [" << NameOfWrappedPass << "]";
   dbg() << ": " << (HasErrors ? "FAIL" : "PASS") << '\n';
 
-  // Strip the Debugify Metadata if required.
-  if (Strip) {
-    StripDebugInfo(M);
-    M.eraseNamedMetadata(NMD);
-    return true;
-  }
+  // Strip debugify metadata if required.
+  if (Strip)
+    return stripDebugifyMetadata(M);
 
   return false;
 }
 
 /// ModulePass for attaching synthetic debug info to everything, used with the
 /// legacy module pass manager.
 struct DebugifyModulePass : public ModulePass {
   bool runOnModule(Module &M) override {

llvm/test/DebugInfo/debugify-each.ll

 ; Verify that debugify each can be safely used with piping
 ; RUN: opt -debugify-each -O1 < %s | opt -O2 -o /dev/null
 
 ; Check that the quiet mode emits no messages.
 ; RUN: opt -disable-output -debugify-quiet -debugify-each -O1 < %s 2>&1 | count 0
 
 ; Check that stripped textual IR compares equal before and after applying
 ; debugify.
-; RUN: opt -O1 < %s -S -o - | \
-; RUN:   opt -strip -strip-dead-prototypes -strip-named-metadata -S -o %t.before
-; RUN: opt -O1 -debugify-each < %s -S -o - | \
-; RUN:   opt -strip -strip-dead-prototypes -strip-named-metadata -S -o %t.after
+; RUN: opt -O1 < %s -S -o %t.before
+; RUN: opt -O1 -debugify-each < %s -S -o %t.after
 ; RUN: diff %t.before %t.after
 
 ; Check that stripped IR compares equal before and after applying debugify.
-; RUN: opt -O1 < %s | \
-; RUN:   opt -strip -strip-dead-prototypes -strip-named-metadata | \
-; RUN:   llvm-dis -o %t.before
-; RUN: opt -O1 -debugify-each < %s | \
-; RUN:   opt -strip -strip-dead-prototypes -strip-named-metadata | \
-; RUN:   llvm-dis -o %t.after
+; RUN: opt -O1 < %s | llvm-dis -o %t.before
+; RUN: opt -O1 -debugify-each < %s | llvm-dis -o %t.after
 ; RUN: diff %t.before %t.after
 
 define void @foo(i32 %arg) {
   call i32 asm "bswap $0", "=r,r"(i32 %arg)
   ret void
 }
 
 define void @bar() {