Page MenuHomePhabricator

make the AsmPrinterHandler array public
ClosedPublic

Authored by vtjnash on Feb 6 2020, 1:12 PM.

Details

Summary

This lets external consumers customize the output, similar to how AssemblyAnnotationWriter lets the caller define callbacks when printing IR. The array of handlers already existed, this just cleans up the code so that it can be exposed publically.

Diff Detail

Event Timeline

vtjnash created this revision.Feb 6 2020, 1:12 PM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 6 2020, 1:12 PM
vtjnash added a subscriber: Restricted Project.Feb 6 2020, 1:22 PM

@rnk would you be an appropriate reviewer for this idea and willing to look it over? I see @timurrrr originally created this generalization that I'm proposing exposing (r196288 / 1cd1444449cb68e2091e8f36785403288ebd3326), but that he's not working with the llvm project anymore. Thanks!

vtjnash updated this revision to Diff 293497.Sep 22 2020, 9:43 AM

clang-format patch

rnk added a comment.Sep 22 2020, 11:33 AM

I can take a look.

llvm/include/llvm/CodeGen/AsmPrinter.h
111

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

179

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.

llvm/lib/CodeGen/AsmPrinter/AsmPrinter.cpp
312

I suppose if it were an API, you would have to supply these timer names. Bleh.

1152

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.

vtjnash updated this revision to Diff 298551.Oct 15 2020, 11:48 PM

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

llvm/include/llvm/CodeGen/AsmPrinter.h
111

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!

179

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.

llvm/lib/CodeGen/AsmPrinter/AsmPrinter.cpp
312

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.

1152

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.

rnk accepted this revision.Oct 16 2020, 11:33 AM

Looks good, thanks! Please implement the suggestions at your discretion before landing.

llvm/include/llvm/CodeGen/AsmPrinterHandler.h
41

Please remove the extra semicolon and reformat to add the space before the brace.

llvm/lib/CodeGen/AsmPrinter/AsmPrinter.cpp
1642–1646

Please add a comment here elaborating a bit on this. This is deleting all the handlers that AsmPrinter added, while keeping all the user-added handlers alive until the AsmPrinter is destroyed.

llvm/unittests/CodeGen/AsmPrinterDwarfTest.cpp
404

This is test code, but to have one less owning raw pointer, I suggest passing TestPrinter->releaseAP() here directly, and initialize the above AP variable with getAP().

This revision is now accepted and ready to land.Oct 16 2020, 11:33 AM
This revision was landed with ongoing or failed builds.Oct 16 2020, 1:28 PM
This revision was automatically updated to reflect the committed changes.
rsmith added a subscriber: rsmith.Oct 16 2020, 2:22 PM

FYI, it looks like this broke a bunch of build bots, for example: http://lab.llvm.org:8011/#/builders/105/builds/367

Yep, I saw that too. I pushed a revert commit while I wait for the tests locally to rerun and confirm my fix for that.

uabelho added a subscriber: uabelho.Wed, Nov 4, 4:42 AM

Hi!

Is this commit supposed to change the output from llc?

With

llc -disable-debug-info-print=true -o - unwind-tables.opt.ll

on

define i16 @main() nounwind {
entry:
  ret i16 0
}

!llvm.dbg.cu = !{!0}
!llvm.module.flags = !{!3, !4, !5}
!llvm.ident = !{!6}

!0 = distinct !DICompileUnit(language: DW_LANG_C99, file: !1, producer: "clang version 12.0.0", isOptimized: true, runtimeVersion: 0, emissionKind: LineTablesOnly, enums: !2, splitDebugInlining: false, nameTableKind: None)
!1 = !DIFile(filename: "unwind-tables.c", directory: "/tmp")
!2 = !{}
!3 = !{i32 7, !"Dwarf Version", i32 4}
!4 = !{i32 2, !"Debug Info Version", i32 3}
!5 = !{i32 1, !"wchar_size", i32 4}
!6 = !{!"clang version 12.0.0"}

I now get

.cfi_startproc
and
.cfi_endproc

in the output which I didn't before.

It seems like the behavior of -disable-debug-info-print=true has changed. Before -disable-debug-info-print=true would call MMI->setDebugInfoAvailability(false); in DwarfDebug::beginModule(), but that doesn't seem to happen anymore.

Is this expected?

Bah, I meant to post my comment in https://reviews.llvm.org/D89613. I did now.