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.
Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
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. |
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. |
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? |
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? |
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) |
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. |
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.