[PATCH] D74158: make the AsmPrinterHandler array public

[Reid Kleckner via Phabricator via llvm-commits]

rnk added a comment.

I can take a look.



================
Comment at: llvm/include/llvm/CodeGen/AsmPrinter.h:113
+  // Custom assembly annotations writer customizations
+  std::unique_ptr<AsmPrinterHandler> AAW;
+
----------------
Maybe a better API for this would be to have an `AsmPrinter::addAsmPrinterHandler` method, so there could be more than one annotation writer.

Either way, something in-tree should either call the API or set the field. You could extend the AsmPrinterDwarfTest unit test to exercise this API.

BTW, this would be a great way to implement source code interleaving:
https://bugs.llvm.org/show_bug.cgi?id=17465
https://bugs.llvm.org/show_bug.cgi?id=39360


================
Comment at: llvm/include/llvm/CodeGen/AsmPrinter.h:155
 
+  /// Simple adapter class to work around lack of a garbage collector:
+  /// this turns the expected unique_ptr back into an unmanaged pointer
----------------
We are always managing object lifetimes in C++, so I don't think it's useful to say we are "working around the lack of a garbage collector." What this class does is to make the AsmPrinterHandler not be owned by the Handler vector.

However, I think it would be simpler to store a boolean "unowned" bit in HandlerInfo and then have the destructor check that and release the unique_ptr member.

Alternatively, maybe we can extend the lifetime of all handlers. As written, you are only extending the AAW lifetime from `doFinalization` to `~AsmPrinter`, so there's really very little difference. I checked `~EHStreamer`, `~DwarfDebug`, `~CodeViewDebug`, and none of them do interesting things in the destructor. Maybe you can remove the `Handlers.clear()` call later or remove it completely, and have standard ownership semantics, which would be best.


================
Comment at: llvm/lib/CodeGen/AsmPrinter/AsmPrinter.cpp:319
+  if (AAW) {
+    Handlers.emplace_back(std::make_unique<AsmPrinterHandlerAdapter>(*AAW),
+                          AAWTimerName, AAWTimerDescription, AAWGroupName,
----------------
I suppose if it were an API, you would have to supply these timer names. Bleh.


================
Comment at: llvm/lib/CodeGen/AsmPrinter/AsmPrinter.cpp:1113
 
-      if (ShouldPrintDebugScopes) {
-        for (const HandlerInfo &HI : Handlers) {
----------------
This is the main change: now the handlers get called back when there is no debug info, and most of the changes are to cope with that. I'm OK with it.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D74158/new/

https://reviews.llvm.org/D74158