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

[Igor Kudrin via Phabricator via llvm-commits]

ikudrin added a comment.

In D87008#2252576 <https://reviews.llvm.org/D87008#2252576>, @dblaikie wrote:

> 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 <https://reviews.llvm.org/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.



================
Comment at: llvm/lib/CodeGen/AsmPrinter/AsmPrinterDwarf.cpp:157
     if (MAI->needsDwarfSectionOffsetDirective()) {
+      assert(!isDwarf64() &&
+             "emitting DWARF64 is not implemented for COFF targets");
----------------
dblaikie wrote:
> 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.
This should not be reachable. These patches do not enable DWARF64 for COFF targets, the only ones that currently set  `NeedsDwarfSectionOffsetDirective` to `true`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D87008