This is an archive of the discontinued LLVM Phabricator instance.

[DebugInfo] Fix methods of AsmPrinter to emit values corresponding to the DWARF format (1/19).
ClosedPublic

Authored by ikudrin on Sep 2 2020, 5:47 AM.

Details

Summary

These methods are used to emit values which are 32-bit in DWARF32 and 64-bit in DWARF64. The patch fixes them so that they choose the length automatically, depending on the DWARF format set in the Context.

Diff Detail

Event Timeline

ikudrin created this revision.Sep 2 2020, 5:47 AM
ikudrin requested review of this revision.Sep 2 2020, 5:47 AM

It's a fair bit of unit test infrastructure - what's the tradeoff between API/unit testing versus functional testing with the command line tools as most of the testing is done? (FileChecking the resulting assembly)

llvm/lib/CodeGen/AsmPrinter/AsmPrinterDwarf.cpp
157

If this is reachable with the right flags, but unimplemented and more a "user error" than something that needs to be handled gracefully, it might be appropriate to use report_fatal_error rather than assert.

If this is unreachable(due to prior data validation) from the command line of user-facing tools, then the assert is OK.

It's a fair bit of unit test infrastructure - what's the tradeoff between API/unit testing versus functional testing with the command line tools as most of the testing is done? (FileChecking the resulting assembly)

Initially, I tried to fix the emitting functions in the patches for individual tables. That worked much worse because the patches for different tables happened to be more tied than necessary. Moreover, some code paths are hard to reach. For example, DwarfUsesRelocationsAcrossSections is set to false only for Darwin and, conditionally, for BPF. The patches do not enable DWARF64 for the former and I doubt that DWARF64 makes any sense for the latter. As another example, some forms for DIE emitters are used only in unit tests, thus if we fixed only code paths that are reachable from the command line, some methods would be updated only partially.

The approach with unit tests enables making the patches in the series both more focused and more independent from each other.

I also hope to resurrect D85293, as the only reason for the condition in DIEInteger::SizeOf() is that the unit tests do not create and pass an AsmPrinter to the testing methods.

llvm/lib/CodeGen/AsmPrinter/AsmPrinterDwarf.cpp
157

This should not be reachable. These patches do not enable DWARF64 for COFF targets, the only ones that currently set NeedsDwarfSectionOffsetDirective to true.

Ping (for this and other not yet accepted revisions in the stack).

dblaikie accepted this revision.Sep 10 2020, 7:08 PM
dblaikie added inline comments.
llvm/unittests/CodeGen/AsmPrinterDwarfTest.cpp
35

Maybe nicer to write this as "return TestPrinter != nullptr;" ? (if you've got to write it out verbosely anyway due to the explicit conversion)

154

Ekxtra parens look like they're unnecessary here?

177

Is there an assert non-null you could use more directly?

This revision is now accepted and ready to land.Sep 10 2020, 7:08 PM
ikudrin added inline comments.Sep 11 2020, 4:52 AM
llvm/unittests/CodeGen/AsmPrinterDwarfTest.cpp
35

I'll update that to return TestPrinter != nullptr; then, thanks.

154

They are added only to improve readability. They are not required for the compiler, sure. Do you prefer them to be removed?

177

Apart from more exotic variants, we can use ASSERT_TRUE(ActualArg0), ASSERT_TRUE(ActualArg0 != nullptr) or ASSERT_NE(ActualArg0, nullptr) here. I have chosen a variant based on my understanding of expressibility. What is your preference?

dblaikie added inline comments.Sep 11 2020, 4:20 PM
llvm/unittests/CodeGen/AsmPrinterDwarfTest.cpp
154

Marginally prefer not to have them - but no harm if you prefer them there, your call.

177

Looks like the LLVM unittests use NE/EQ about 4:1, so maybe switch to that? (makes for slightly more legible error generally, though probably no major difference for nullptr testing)

ikudrin marked 2 inline comments as done.Sep 14 2020, 12:53 AM
ikudrin added inline comments.
llvm/unittests/CodeGen/AsmPrinterDwarfTest.cpp
154

Let's keep them as is, then.

177

Ok, will change to ASSERT_NE(ActualArg0, nullptr), this and similar places.

For the record, ASSERT_TRUE(ActualArg0 != nullptr) generates

.../unittests/CodeGen/AsmPrinterDwarfTest.cpp:176: Failure
Value of: ActualArg0 != nullptr
  Actual: false
Expected: true

while ASSERT_NE(ActualArg0, nullptr) results in

.../unittests/CodeGen/AsmPrinterDwarfTest.cpp:176: Failure
Expected: (ActualArg0) != (nullptr), actual: NULL vs 8-byte object <00-00 00-00 00-00 00-00>

which, indeed, seems more legible.

This revision was landed with ongoing or failed builds.Sep 14 2020, 10:24 PM
This revision was automatically updated to reflect the committed changes.
ikudrin marked an inline comment as done.