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

[David Blaikie via Phabricator via llvm-commits]

dblaikie accepted this revision.
dblaikie added a comment.
This revision is now accepted and ready to land.

In D87009#2255662 <https://reviews.llvm.org/D87009#2255662>, @ikudrin wrote:

> In D87009#2255174 <https://reviews.llvm.org/D87009#2255174>, @dblaikie wrote:
>
>> In D87009#2253973 <https://reviews.llvm.org/D87009#2253973>, @ikudrin wrote:
>>
>>> That would require to update a particular method several times and potentially would leave it in a partially updated state because some code paths are hard to reach.
>>
>> Could you explain this in more detail?
>
> 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.

> 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)

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)

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.


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

https://reviews.llvm.org/D87009