[AsmPrinter] Reintroduce full AsmPrinterHandler API (llvm#122297)

This restores the functionality of AsmPrinterHandlers to what it was
prior to llvm#96785. The attempted
hack there of adding a duplicate DebugHandlerBase handling added a lot
of hidden state and assumptions, which just segfaulted when we tried to
continuing using this API. Instead, this just goes back to the old
design, but adds a separate array for the basic EH handles. The
duplicate array is identical to the other array of handler, but which
doesn't get their begin/endInstruction callbacks called. This still
saves the negligible but measurable amount of virtual function calls as
was the goal of llvm#96785, while restoring the API to the pre-LLVM-19
status quo.

(cherry picked from commit f6b0555)

Co-authored-by: Zentrik <[email protected]>

Author: 2 people

Diff for: llvm/include/llvm/CodeGen/AsmPrinter.h

@@ -44,6 +44,7 @@ class DebugHandlerBase;
 class DIE;
 class DIEAbbrev;
 class DwarfDebug;
+class EHStreamer;
 class GCMetadataPrinter;
 class GCStrategy;
 class GlobalAlias;
@@ -187,15 +188,17 @@ class AsmPrinter : public MachineFunctionPass {
   /// For dso_local functions, the current $local alias for the function.
   MCSymbol *CurrentFnBeginLocal = nullptr;
 
-  /// A vector of all debug/EH info emitters we should use. This vector
-  /// maintains ownership of the emitters.
+  /// A handle to the EH info emitter (if present).
+  // Only for EHStreamer subtypes, but some C++ compilers will incorrectly warn
+  // us if we declare that directly.
+  SmallVector<std::unique_ptr<AsmPrinterHandler>, 1> EHHandlers;
+
+  // A vector of all Debuginfo emitters we should use. Protected so that
+  // targets can add their own. This vector maintains ownership of the
+  // emitters.
   SmallVector<std::unique_ptr<AsmPrinterHandler>, 2> Handlers;
   size_t NumUserHandlers = 0;
 
-  /// Debuginfo handler. Protected so that targets can add their own.
-  SmallVector<std::unique_ptr<DebugHandlerBase>, 1> DebugHandlers;
-  size_t NumUserDebugHandlers = 0;
-
   StackMaps SM;
 
 private:
@@ -521,8 +524,6 @@ class AsmPrinter : public MachineFunctionPass {
 
   void addAsmPrinterHandler(std::unique_ptr<AsmPrinterHandler> Handler);
 
-  void addDebugHandler(std::unique_ptr<DebugHandlerBase> Handler);
-
   // Targets can, or in the case of EmitInstruction, must implement these to
   // customize output.
 

Diff for: llvm/include/llvm/CodeGen/AsmPrinterHandler.h

@@ -64,6 +64,16 @@ class AsmPrinterHandler {
   /// immediately prior to markFunctionEnd.
   virtual void endBasicBlockSection(const MachineBasicBlock &MBB) {}
 
+  /// For symbols that have a size designated (e.g. common symbols),
+  /// this tracks that size.
+  virtual void setSymbolSize(const MCSymbol *Sym, uint64_t Size) {}
+
+  /// Process beginning of an instruction.
+  virtual void beginInstruction(const MachineInstr *MI) {}
+
+  /// Process end of an instruction.
+  virtual void endInstruction() {}
+
   /// Emit target-specific EH funclet machinery.
   virtual void beginFunclet(const MachineBasicBlock &MBB,
                             MCSymbol *Sym = nullptr) {}

Diff for: llvm/include/llvm/CodeGen/DebugHandlerBase.h

@@ -50,14 +50,10 @@ struct DbgVariableLocation {
 
 /// Base class for debug information backends. Common functionality related to
 /// tracking which variables and scopes are alive at a given PC live here.
-class DebugHandlerBase {
+class DebugHandlerBase : public AsmPrinterHandler {
 protected:
   DebugHandlerBase(AsmPrinter *A);
 
-public:
-  virtual ~DebugHandlerBase();
-
-protected:
   /// Target of debug info emission.
   AsmPrinter *Asm = nullptr;
 
@@ -120,22 +116,20 @@ class DebugHandlerBase {
 private:
   InstructionOrdering InstOrdering;
 
+  // AsmPrinterHandler overrides.
 public:
-  /// For symbols that have a size designated (e.g. common symbols),
-  /// this tracks that size. Only used by DWARF.
-  virtual void setSymbolSize(const MCSymbol *Sym, uint64_t Size) {}
+  virtual ~DebugHandlerBase() override;
 
-  virtual void beginModule(Module *M);
-  virtual void endModule() = 0;
+  void beginModule(Module *M) override;
 
-  virtual void beginInstruction(const MachineInstr *MI);
-  virtual void endInstruction();
+  void beginInstruction(const MachineInstr *MI) override;
+  void endInstruction() override;
 
-  void beginFunction(const MachineFunction *MF);
-  void endFunction(const MachineFunction *MF);
+  void beginFunction(const MachineFunction *MF) override;
+  void endFunction(const MachineFunction *MF) override;
 
-  void beginBasicBlockSection(const MachineBasicBlock &MBB);
-  void endBasicBlockSection(const MachineBasicBlock &MBB);
+  void beginBasicBlockSection(const MachineBasicBlock &MBB) override;
+  void endBasicBlockSection(const MachineBasicBlock &MBB) override;
 
   /// Return Label preceding the instruction.
   MCSymbol *getLabelBeforeInsn(const MachineInstr *MI);

Diff for: llvm/lib/CodeGen/AsmPrinter/AsmPrinter.cpp

@@ -539,12 +539,12 @@ bool AsmPrinter::doInitialization(Module &M) {
   if (MAI->doesSupportDebugInformation()) {
     bool EmitCodeView = M.getCodeViewFlag();
     if (EmitCodeView && TM.getTargetTriple().isOSWindows())
-      DebugHandlers.push_back(std::make_unique<CodeViewDebug>(this));
+      Handlers.push_back(std::make_unique<CodeViewDebug>(this));
     if (!EmitCodeView || M.getDwarfVersion()) {
       assert(MMI && "MMI could not be nullptr here!");
       if (MMI->hasDebugInfo()) {
         DD = new DwarfDebug(this);
-        DebugHandlers.push_back(std::unique_ptr<DwarfDebug>(DD));
+        Handlers.push_back(std::unique_ptr<DwarfDebug>(DD));
       }
     }
   }
@@ -611,12 +611,12 @@ bool AsmPrinter::doInitialization(Module &M) {
 
   // Emit tables for any value of cfguard flag (i.e. cfguard=1 or cfguard=2).
   if (mdconst::extract_or_null<ConstantInt>(M.getModuleFlag("cfguard")))
-    Handlers.push_back(std::make_unique<WinCFGuard>(this));
+    EHHandlers.push_back(std::make_unique<WinCFGuard>(this));
 
-  for (auto &Handler : DebugHandlers)
-    Handler->beginModule(&M);
   for (auto &Handler : Handlers)
     Handler->beginModule(&M);
+  for (auto &Handler : EHHandlers)
+    Handler->beginModule(&M);
 
   return false;
 }
@@ -763,7 +763,7 @@ void AsmPrinter::emitGlobalVariable(const GlobalVariable *GV) {
   // sections and expected to be contiguous (e.g. ObjC metadata).
   const Align Alignment = getGVAlignment(GV, DL);
 
-  for (auto &Handler : DebugHandlers)
+  for (auto &Handler : Handlers)
     Handler->setSymbolSize(GVSym, Size);
 
   // Handle common symbols
@@ -1035,14 +1035,14 @@ void AsmPrinter::emitFunctionHeader() {
   }
 
   // Emit pre-function debug and/or EH information.
-  for (auto &Handler : DebugHandlers) {
+  for (auto &Handler : Handlers) {
     Handler->beginFunction(MF);
     Handler->beginBasicBlockSection(MF->front());
   }
-  for (auto &Handler : Handlers)
+  for (auto &Handler : EHHandlers) {
     Handler->beginFunction(MF);
-  for (auto &Handler : Handlers)
     Handler->beginBasicBlockSection(MF->front());
+  }
 
   // Emit the prologue data.
   if (F.hasPrologueData())
@@ -1737,7 +1737,7 @@ void AsmPrinter::emitFunctionBody() {
       if (MDNode *MD = MI.getPCSections())
         emitPCSectionsLabel(*MF, *MD);
 
-      for (auto &Handler : DebugHandlers)
+      for (auto &Handler : Handlers)
         Handler->beginInstruction(&MI);
 
       if (isVerbose())
@@ -1832,7 +1832,7 @@ void AsmPrinter::emitFunctionBody() {
       if (MCSymbol *S = MI.getPostInstrSymbol())
         OutStreamer->emitLabel(S);
 
-      for (auto &Handler : DebugHandlers)
+      for (auto &Handler : Handlers)
         Handler->endInstruction();
     }
 
@@ -1966,13 +1966,15 @@ void AsmPrinter::emitFunctionBody() {
   // Call endBasicBlockSection on the last block now, if it wasn't already
   // called.
   if (!MF->back().isEndSection()) {
-    for (auto &Handler : DebugHandlers)
-      Handler->endBasicBlockSection(MF->back());
     for (auto &Handler : Handlers)
       Handler->endBasicBlockSection(MF->back());
+    for (auto &Handler : EHHandlers)
+      Handler->endBasicBlockSection(MF->back());
   }
   for (auto &Handler : Handlers)
     Handler->markFunctionEnd();
+  for (auto &Handler : EHHandlers)
+    Handler->markFunctionEnd();
 
   assert(!MBBSectionRanges.contains(MF->front().getSectionID()) &&
          "Overwrite section range");
@@ -1983,10 +1985,10 @@ void AsmPrinter::emitFunctionBody() {
   emitJumpTableInfo();
 
   // Emit post-function debug and/or EH information.
-  for (auto &Handler : DebugHandlers)
-    Handler->endFunction(MF);
   for (auto &Handler : Handlers)
     Handler->endFunction(MF);
+  for (auto &Handler : EHHandlers)
+    Handler->endFunction(MF);
 
   // Emit section containing BB address offsets and their metadata, when
   // BB labels are requested for this function. Skip empty functions.
@@ -2425,17 +2427,16 @@ bool AsmPrinter::doFinalization(Module &M) {
     emitGlobalIFunc(M, IFunc);
 
   // Finalize debug and EH information.
-  for (auto &Handler : DebugHandlers)
-    Handler->endModule();
   for (auto &Handler : Handlers)
     Handler->endModule();
+  for (auto &Handler : EHHandlers)
+    Handler->endModule();
 
   // This deletes all the ephemeral handlers that AsmPrinter added, while
   // keeping all the user-added handlers alive until the AsmPrinter is
   // destroyed.
+  EHHandlers.clear();
   Handlers.erase(Handlers.begin() + NumUserHandlers, Handlers.end());
-  DebugHandlers.erase(DebugHandlers.begin() + NumUserDebugHandlers,
-                      DebugHandlers.end());
   DD = nullptr;
 
   // If the target wants to know about weak references, print them all.
@@ -3957,6 +3958,10 @@ void AsmPrinter::emitBasicBlockStart(const MachineBasicBlock &MBB) {
       Handler->endFunclet();
       Handler->beginFunclet(MBB);
     }
+    for (auto &Handler : EHHandlers) {
+      Handler->endFunclet();
+      Handler->beginFunclet(MBB);
+    }
   }
 
   // Switch to a new section if this basic block must begin a section. The
@@ -4026,21 +4031,21 @@ void AsmPrinter::emitBasicBlockStart(const MachineBasicBlock &MBB) {
   // if it begins a section (Entry block call is handled separately, next to
   // beginFunction).
   if (MBB.isBeginSection() && !MBB.isEntryBlock()) {
-    for (auto &Handler : DebugHandlers)
-      Handler->beginBasicBlockSection(MBB);
     for (auto &Handler : Handlers)
       Handler->beginBasicBlockSection(MBB);
+    for (auto &Handler : EHHandlers)
+      Handler->beginBasicBlockSection(MBB);
   }
 }
 
 void AsmPrinter::emitBasicBlockEnd(const MachineBasicBlock &MBB) {
   // Check if CFI information needs to be updated for this MBB with basic block
   // sections.
   if (MBB.isEndSection()) {
-    for (auto &Handler : DebugHandlers)
-      Handler->endBasicBlockSection(MBB);
     for (auto &Handler : Handlers)
       Handler->endBasicBlockSection(MBB);
+    for (auto &Handler : EHHandlers)
+      Handler->endBasicBlockSection(MBB);
   }
 }
 
@@ -4174,12 +4179,7 @@ void AsmPrinter::addAsmPrinterHandler(
   NumUserHandlers++;
 }
 
-void AsmPrinter::addDebugHandler(std::unique_ptr<DebugHandlerBase> Handler) {
-  DebugHandlers.insert(DebugHandlers.begin(), std::move(Handler));
-  NumUserDebugHandlers++;
-}
-
-/// Pin vtable to this file.
+/// Pin vtables to this file.
 AsmPrinterHandler::~AsmPrinterHandler() = default;
 
 void AsmPrinterHandler::markFunctionEnd() {}

Diff for: llvm/lib/CodeGen/AsmPrinter/PseudoProbePrinter.h

@@ -14,7 +14,6 @@
 #define LLVM_LIB_CODEGEN_ASMPRINTER_PSEUDOPROBEPRINTER_H
 
 #include "llvm/ADT/DenseMap.h"
-#include "llvm/CodeGen/AsmPrinterHandler.h"
 
 namespace llvm {
 

Diff for: llvm/lib/Target/BPF/BPFAsmPrinter.cpp

@@ -62,7 +62,7 @@ bool BPFAsmPrinter::doInitialization(Module &M) {
   // Only emit BTF when debuginfo available.
   if (MAI->doesSupportDebugInformation() && !M.debug_compile_units().empty()) {
     BTF = new BTFDebug(this);
-    DebugHandlers.push_back(std::unique_ptr<BTFDebug>(BTF));
+    Handlers.push_back(std::unique_ptr<BTFDebug>(BTF));
   }
 
   return false;

Diff for: llvm/unittests/CodeGen/AsmPrinterDwarfTest.cpp

@@ -384,10 +384,13 @@ class AsmPrinterHandlerTest : public AsmPrinterFixtureBase {
   public:
     TestHandler(AsmPrinterHandlerTest &Test) : Test(Test) {}
     virtual ~TestHandler() {}
+    virtual void setSymbolSize(const MCSymbol *Sym, uint64_t Size) override {}
     virtual void beginModule(Module *M) override { Test.BeginCount++; }
     virtual void endModule() override { Test.EndCount++; }
     virtual void beginFunction(const MachineFunction *MF) override {}
     virtual void endFunction(const MachineFunction *MF) override {}
+    virtual void beginInstruction(const MachineInstr *MI) override {}
+    virtual void endInstruction() override {}
   };
 
 protected:
@@ -424,54 +427,4 @@ TEST_F(AsmPrinterHandlerTest, Basic) {
   ASSERT_EQ(EndCount, 3);
 }
 
-class AsmPrinterDebugHandlerTest : public AsmPrinterFixtureBase {
-  class TestDebugHandler : public DebugHandlerBase {
-    AsmPrinterDebugHandlerTest &Test;
-
-  public:
-    TestDebugHandler(AsmPrinterDebugHandlerTest &Test, AsmPrinter *AP)
-        : DebugHandlerBase(AP), Test(Test) {}
-    virtual ~TestDebugHandler() {}
-    virtual void beginModule(Module *M) override { Test.BeginCount++; }
-    virtual void endModule() override { Test.EndCount++; }
-    virtual void beginFunctionImpl(const MachineFunction *MF) override {}
-    virtual void endFunctionImpl(const MachineFunction *MF) override {}
-    virtual void beginInstruction(const MachineInstr *MI) override {}
-    virtual void endInstruction() override {}
-  };
-
-protected:
-  bool init(const std::string &TripleStr, unsigned DwarfVersion,
-            dwarf::DwarfFormat DwarfFormat) {
-    if (!AsmPrinterFixtureBase::init(TripleStr, DwarfVersion, DwarfFormat))
-      return false;
-
-    auto *AP = TestPrinter->getAP();
-    AP->addDebugHandler(std::make_unique<TestDebugHandler>(*this, AP));
-    LLVMTargetMachine *LLVMTM = static_cast<LLVMTargetMachine *>(&AP->TM);
-    legacy::PassManager PM;
-    PM.add(new MachineModuleInfoWrapperPass(LLVMTM));
-    PM.add(TestPrinter->releaseAP()); // Takes ownership of destroying AP
-    LLVMContext Context;
-    std::unique_ptr<Module> M(new Module("TestModule", Context));
-    M->setDataLayout(LLVMTM->createDataLayout());
-    PM.run(*M);
-    // Now check that we can run it twice.
-    AP->addDebugHandler(std::make_unique<TestDebugHandler>(*this, AP));
-    PM.run(*M);
-    return true;
-  }
-
-  int BeginCount = 0;
-  int EndCount = 0;
-};
-
-TEST_F(AsmPrinterDebugHandlerTest, Basic) {
-  if (!init("x86_64-pc-linux", /*DwarfVersion=*/4, dwarf::DWARF32))
-    GTEST_SKIP();
-
-  ASSERT_EQ(BeginCount, 3);
-  ASSERT_EQ(EndCount, 3);
-}
-
 } // end namespace