revert breaking API changes from "Reduce AsmPrinterHandlers virt. fn calls"

[vtjnash]

This restores the functionality of AsmPrinterHandlers to what it was
prior to #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 #96785, while restoring the API to the pre-LLVM-19
status quo.

[llvmbot]

@llvm/pr-subscribers-debuginfo

Author: Jameson Nash (vtjnash)

Changes

As foreshadowed by the author's comment before merging #96785 (comment), this turned out to be pretty awkward for user handlers, since overriding DebugHandlerBase itself adds a lot of undesirable hidden state and assumptions, which segfault if we overload it. Add an extra hierarchy in the class structure so that we keep the performance gains from splitting up the basic EH handles, without needing to break the API from LLVM v18.

@aengelke does this look okay to you?

---

Full diff: https://github.com/llvm/llvm-project/pull/122297.diff

8 Files Affected:

• (modified) llvm/include/llvm/CodeGen/AsmPrinter.h (+6-7)
• (modified) llvm/include/llvm/CodeGen/AsmPrinterHandler.h (+19-2)
• (modified) llvm/include/llvm/CodeGen/DebugHandlerBase.h (+10-18)
• (modified) llvm/lib/CodeGen/AsmPrinter/AsmPrinter.cpp (+5-9)
• (modified) llvm/lib/CodeGen/AsmPrinter/EHStreamer.h (+1-1)
• (modified) llvm/lib/CodeGen/AsmPrinter/PseudoProbePrinter.h (+1-1)
• (modified) llvm/lib/CodeGen/AsmPrinter/WinCFGuard.h (+1-1)
• (modified) llvm/unittests/CodeGen/AsmPrinterDwarfTest.cpp (+3-50)

diff --git a/llvm/include/llvm/CodeGen/AsmPrinter.h b/llvm/include/llvm/CodeGen/AsmPrinter.h
index c9a88d7b1c015c..e3e75f6f463c64 100644
--- a/llvm/include/llvm/CodeGen/AsmPrinter.h
+++ b/llvm/include/llvm/CodeGen/AsmPrinter.h
@@ -33,6 +33,7 @@
 namespace llvm {
 
 class AddrLabelMap;
+class AsmBasicPrinterHandler;
 class AsmPrinterHandler;
 class BasicBlock;
 class BlockAddress;
@@ -187,13 +188,13 @@ 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
+  /// A vector of all EH info emitters we should use. This vector
   /// maintains ownership of the emitters.
-  SmallVector<std::unique_ptr<AsmPrinterHandler>, 2> Handlers;
-  size_t NumUserHandlers = 0;
+  SmallVector<std::unique_ptr<AsmBasicPrinterHandler>, 2> Handlers;
 
-  /// Debuginfo handler. Protected so that targets can add their own.
-  SmallVector<std::unique_ptr<DebugHandlerBase>, 1> DebugHandlers;
+  // A vector of all Debuginfo emitters we should use. Protected so that
+  // targets can add their own.
+  SmallVector<std::unique_ptr<AsmPrinterHandler>, 1> DebugHandlers;
   size_t NumUserDebugHandlers = 0;
 
   StackMaps SM;
@@ -527,8 +528,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 --git a/llvm/include/llvm/CodeGen/AsmPrinterHandler.h b/llvm/include/llvm/CodeGen/AsmPrinterHandler.h
index ed73e618431ded..daf8793f4ddaf6 100644
--- a/llvm/include/llvm/CodeGen/AsmPrinterHandler.h
+++ b/llvm/include/llvm/CodeGen/AsmPrinterHandler.h
@@ -30,9 +30,9 @@ typedef MCSymbol *ExceptionSymbolProvider(AsmPrinter *Asm,
 
 /// Collects and handles AsmPrinter objects required to build debug
 /// or EH information.
-class AsmPrinterHandler {
+class AsmBasicPrinterHandler {
 public:
-  virtual ~AsmPrinterHandler();
+  virtual ~AsmBasicPrinterHandler();
 
   virtual void beginModule(Module *M) {}
 
@@ -70,6 +70,23 @@ class AsmPrinterHandler {
   virtual void endFunclet() {}
 };
 
+class AsmPrinterHandler : public AsmBasicPrinterHandler {
+public:
+  virtual ~AsmPrinterHandler();
+
+  /// 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) = 0;
+
+  /// Process end of an instruction.
+  virtual void endInstruction() = 0;
+
+  virtual void beginCodeAlignment(const MachineBasicBlock &MBB) {}
+};
+
 } // End of namespace llvm
 
 #endif
diff --git a/llvm/include/llvm/CodeGen/DebugHandlerBase.h b/llvm/include/llvm/CodeGen/DebugHandlerBase.h
index d39e7e68cb2558..f669bd311ff564 100644
--- a/llvm/include/llvm/CodeGen/DebugHandlerBase.h
+++ b/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,24 +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 void beginModule(Module *M);
-  virtual void endModule() = 0;
+  virtual ~DebugHandlerBase() override;
 
-  virtual void beginInstruction(const MachineInstr *MI);
-  virtual void endInstruction();
+  void beginModule(Module *M) override;
 
-  void beginFunction(const MachineFunction *MF);
-  void endFunction(const MachineFunction *MF);
+  void beginInstruction(const MachineInstr *MI) override;
+  void endInstruction() override;
 
-  void beginBasicBlockSection(const MachineBasicBlock &MBB);
-  void endBasicBlockSection(const MachineBasicBlock &MBB);
+  void beginFunction(const MachineFunction *MF) override;
+  void endFunction(const MachineFunction *MF) override;
 
-  virtual void beginCodeAlignment(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 --git a/llvm/lib/CodeGen/AsmPrinter/AsmPrinter.cpp b/llvm/lib/CodeGen/AsmPrinter/AsmPrinter.cpp
index 3ba45900e45691..5c952be3bce5ca 100644
--- a/llvm/lib/CodeGen/AsmPrinter/AsmPrinter.cpp
+++ b/llvm/lib/CodeGen/AsmPrinter/AsmPrinter.cpp
@@ -392,7 +392,7 @@ AsmPrinter::AsmPrinter(TargetMachine &tm, std::unique_ptr<MCStreamer> Streamer)
 }
 
 AsmPrinter::~AsmPrinter() {
-  assert(!DD && Handlers.size() == NumUserHandlers &&
+  assert(!DD && DebugHandlers.size() == NumUserDebugHandlers &&
          "Debug/EH info didn't get finalized");
 }
 
@@ -2591,7 +2591,7 @@ bool AsmPrinter::doFinalization(Module &M) {
   // This deletes all the ephemeral handlers that AsmPrinter added, while
   // keeping all the user-added handlers alive until the AsmPrinter is
   // destroyed.
-  Handlers.erase(Handlers.begin() + NumUserHandlers, Handlers.end());
+  Handlers.clear();
   DebugHandlers.erase(DebugHandlers.begin() + NumUserDebugHandlers,
                       DebugHandlers.end());
   DD = nullptr;
@@ -4411,19 +4411,15 @@ void AsmPrinter::emitStackMaps() {
 
 void AsmPrinter::addAsmPrinterHandler(
     std::unique_ptr<AsmPrinterHandler> Handler) {
-  Handlers.insert(Handlers.begin(), std::move(Handler));
-  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.
+AsmBasicPrinterHandler::~AsmBasicPrinterHandler() = default;
 AsmPrinterHandler::~AsmPrinterHandler() = default;
 
-void AsmPrinterHandler::markFunctionEnd() {}
+void AsmBasicPrinterHandler::markFunctionEnd() {}
 
 // In the binary's "xray_instr_map" section, an array of these function entries
 // describes each instrumentation point.  When XRay patches your code, the index
diff --git a/llvm/lib/CodeGen/AsmPrinter/EHStreamer.h b/llvm/lib/CodeGen/AsmPrinter/EHStreamer.h
index 705a61fb827f37..93d13f3e6b2036 100644
--- a/llvm/lib/CodeGen/AsmPrinter/EHStreamer.h
+++ b/llvm/lib/CodeGen/AsmPrinter/EHStreamer.h
@@ -27,7 +27,7 @@ class MCSymbol;
 template <typename T> class SmallVectorImpl;
 
 /// Emits exception handling directives.
-class LLVM_LIBRARY_VISIBILITY EHStreamer : public AsmPrinterHandler {
+class LLVM_LIBRARY_VISIBILITY EHStreamer : public AsmBasicPrinterHandler {
 protected:
   /// Target of directive emission.
   AsmPrinter *Asm;
diff --git a/llvm/lib/CodeGen/AsmPrinter/PseudoProbePrinter.h b/llvm/lib/CodeGen/AsmPrinter/PseudoProbePrinter.h
index 35461e53fbf19d..47fcbce2560272 100644
--- a/llvm/lib/CodeGen/AsmPrinter/PseudoProbePrinter.h
+++ b/llvm/lib/CodeGen/AsmPrinter/PseudoProbePrinter.h
@@ -14,7 +14,7 @@
 #define LLVM_LIB_CODEGEN_ASMPRINTER_PSEUDOPROBEPRINTER_H
 
 #include "llvm/ADT/DenseMap.h"
-#include "llvm/CodeGen/AsmPrinterHandler.h"
+#include "llvm/CodeGen/AsmPrinter.h"
 
 namespace llvm {
 
diff --git a/llvm/lib/CodeGen/AsmPrinter/WinCFGuard.h b/llvm/lib/CodeGen/AsmPrinter/WinCFGuard.h
index f94acc912483d5..9402c22a1b545b 100644
--- a/llvm/lib/CodeGen/AsmPrinter/WinCFGuard.h
+++ b/llvm/lib/CodeGen/AsmPrinter/WinCFGuard.h
@@ -20,7 +20,7 @@
 
 namespace llvm {
 
-class LLVM_LIBRARY_VISIBILITY WinCFGuard : public AsmPrinterHandler {
+class LLVM_LIBRARY_VISIBILITY WinCFGuard : public AsmBasicPrinterHandler {
   /// Target of directive emission.
   AsmPrinter *Asm;
   std::vector<const MCSymbol *> LongjmpTargets;
diff --git a/llvm/unittests/CodeGen/AsmPrinterDwarfTest.cpp b/llvm/unittests/CodeGen/AsmPrinterDwarfTest.cpp
index dc738d85547bbc..6c08173f786223 100644
--- a/llvm/unittests/CodeGen/AsmPrinterDwarfTest.cpp
+++ b/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));
-    TargetMachine *TM = &AP->TM;
-    legacy::PassManager PM;
-    PM.add(new MachineModuleInfoWrapperPass(TM));
-    PM.add(TestPrinter->releaseAP()); // Takes ownership of destroying AP
-    LLVMContext Context;
-    std::unique_ptr<Module> M(new Module("TestModule", Context));
-    M->setDataLayout(TM->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

[aengelke]

Looks generally fine, thanks for finding a way that doesn't seem to impact performance. (I put it up for llvm-compile-time-tracker for confirmation.)

I'm not quite happy with the naming, it should rather be AsmPrinterBasicHandler (AsmPrinter is the main class). Maybe WinCFGuard could inherit from the new AsmPrinterHandler, and instead of AsmBasicPrinterHandler we could just use and store EHStreamer? Also DebugHandlers is now a misleading name.

[aengelke]

Looks neutral on c-t-t.

[vtjnash]

That is true. I was parsing that name incorrectly in my head, but as you mentioned, it only ever had one meaningful subtype anyways (EHStreamer), so we can just delete that again too and go back to the way it was before, but with the added array just for the one EHHandler.

[github-actions]

✅ With the latest revision this PR passed the C/C++ code formatter.

[giordano]

Looks like there are some tests failures now.

[vtjnash]

Okay, looks like that was a latent bug from #96785 having forgotten to duplicate a line of code for endFunction, which triggered here due to being only a partial revert instead of a full revert. This rework is why I wanted to NAK the original PR, when it was not shown to be an actual measurable improvement, just API breakage. But anyways, this should be good to go now, with the requested improvements to this PR and still keeping the minor reduction in no-op function call counts. Thanks for reviewing.

[aengelke]

LGTM without the unneeded include changes.

[vtjnash]

(I don't have merge rights anymore, so you will need to merge for me)

[aengelke]

Ok. Could you update the PR title an description (which will become the commit message) to reflect the current changes and follow the commit message guidelines? Will merge once updated.

[vtjnash]

okay

[aengelke]

Thanks