[PATCH] D74158: make the AsmPrinterHandler array public

[Jameson Nash via Phabricator via llvm-commits]

vtjnash added a comment.

@rnk I've generalized the API to your suggestion and added a test. Let me know what you think, thanks!



================
Comment at: llvm/include/llvm/CodeGen/AsmPrinter.h:113
+  // Custom assembly annotations writer customizations
+  std::unique_ptr<AsmPrinterHandler> AAW;
+
----------------
rnk wrote:
> 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
> BTW, this would be a great way to implement source code interleaving:

I didn't know there were issues for it! But yeah, that's essentially what I had created this for! (I already had the code plugin to do it for arbitrary IR, so this was just exposing the equivalent surface API that I could use it here too.)

I've attempted to add a test where you described. Let me know if that looks right!


================
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
----------------
rnk wrote:
> 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.
I went ahead and changed the API to be more general with a AsmPrinter::addAsmPrinterHandler method, and in the process did away with the need for this adapter class. I distinguish between user and ephemeral handlers by keeping track of how many are in the array, and only erasing the ones that are ephemeral while preserving those added by the user up front.


================
Comment at: llvm/lib/CodeGen/AsmPrinter/AsmPrinter.cpp:319
+  if (AAW) {
+    Handlers.emplace_back(std::make_unique<AsmPrinterHandlerAdapter>(*AAW),
+                          AAWTimerName, AAWTimerDescription, AAWGroupName,
----------------
rnk wrote:
> I suppose if it were an API, you would have to supply these timer names. Bleh.
Yeah, my usage only needed to add a single item, so it seemed somewhat more minimal to deal with it this way, but it seems now as simple to expose the whole struct type.


================
Comment at: llvm/lib/CodeGen/AsmPrinter/AsmPrinter.cpp:1113
 
-      if (ShouldPrintDebugScopes) {
-        for (const HandlerInfo &HI : Handlers) {
----------------
rnk wrote:
> 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.
Correct. Previously, it appears that the code only partially handled the notion of having multiple handlers—particularly for having the given handler enabled without having all debug information present due to the calls to the shared state `setDebugInfoAvailability(false)`. Fortunately, there were just a few spots where those assertions needed to be removed and some conditions localized to remove that global assumption.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D74158