[PATCH] D87009: [DebugInfo] Fix DIE value emitters to be compatible with DWARF64 (2/19).

[Igor Kudrin via Phabricator via llvm-commits]

ikudrin added a comment.

In D87009#2266648 <https://reviews.llvm.org/D87009#2266648>, @dblaikie wrote:

>> Let's take, for example, `DIEExpr`. As far as I can find, with `DW_FORM_sec_offset` it is used only in the `DebugInfo/DWARF` unit tests, in `dwarfgen::DIE::addStrOffsetsBaseAttribute()`. If we took the approach with end-to-end functional testing, this class would not be updated. And the path is not simple to track, by the way.
>
> API only intended for unit tests would be tested/updated with unit tests.

The `DIEExpr` class is not intended to be used only in unit tests; the `CodeGen` library uses it, but with different DWARF forms. The generator in `DebugInfo/DWARF` unit tests does not test the class, it just uses it to create some bits of DWARF data. Moreover, these unit tests are for a different library and as such are not the best place to be updated if we want to fix the issue in a class of the `CodeGen` library. It is much better to test the implementation in the library's own unit tests, so this patch does exactly that. I do not want to leave `DIEExpr` in the not updated state, because the new code may use it without noticing that the implementation is broken for DWARF64.

>> Other classes in this patch are used to emit compilation units, so moving the changes there would bloat that patch up, making it less focused and harder to track how each individual change is tested because, when looking to the test source, it is not evident which attributes use a particular class to emit their values.
>
> I think what might've been tidier could've been dumping sections alone - guess it's always going to be broken in some way in this intermediate state of some sections with DWARF64 and some without, until the whole patch series is in. But perhaps adding support for one section (while potentially mangling another section/causing it to be partly DWARF32/DWARF64, if it used some common API but had bits of its own implementation - like the loclist/rnglist situation) - adding tests for that section (llvm-dwarfdumping only that section - perhaps that deosn't work for some DWARFv4 sections where it's not self-describing), then adding another, etc. (so, for instance, doing str_offsets first, then doing debug_info that depends on str_offsets, etc)

I believe that my set of patches does exactly that. After a few preparation steps, which are tested separately with unit tests, there is a series of patches for individual sections, where interferences between patches are minimal. And a patch for each section is tested with `llvm-dwarfdump` which extracts only the required section(s), ignoring others, which might still be broken.

> Also would've probably be good to do more of this incrementally as you went - as much as I appreciate figuring out the end picture and then trying to piece together an ideal patch series, it makes it harder to change things (having to adjust all the subsequent patches) than if we could've discussed them earlier, etc. (& just a lot to review at one time, compared to a small patch here and there)

Well, the current state of the series is definitely not the way I worked on the patches. It is maybe the third iteration, cleaned, polished, and rearranged so that each step is easily observable and thus more convenient for review. I guess if I had been posting the patches in progress, that would be a real mess.

> Anyway - in the interests of practicality, I'll try to get through these mostly on the "what does it look like overall" rather than the nitpicking nuance of what the perfect patch series might look like from my perspective.

Thank you for reviewing the patches!


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

https://reviews.llvm.org/D87009