[Lldb-commits] [PATCH] D75929: [DebugInfo] Support DWARFv5 index sections.

Wed Mar 11 08:31:19 PDT 2020

labath added a comment.

In D75929#1916760 <https://reviews.llvm.org/D75929#1916760>, @ikudrin wrote:

> In D75929#1916466 <https://reviews.llvm.org/D75929#1916466>, @labath wrote:
>
> > That's true, but I'm not sure it is really the best solution.
>
>
> Well, I do not pretend this to be a perfect solution, but anything I can imagine has its drawbacks. The whole problem comes from overlapping values in both standards, and any solution has to fix that somehow.

Can't argue with that. :)

> 
> 
>> This way we will have three numbering schemes (four if you count the "index" thingy in llvm-dwp) floating around: the "gnu" scheme, the "official" scheme, and the "internal" scheme. That's quite a lot to keep in ones head at once.
> 
> However, you do not need to worry about all the schemas everywhere. The only place they meet is translation functions. Most of the code just use internal encoding, which is enough.

Well, that's where the accessor functions would come in. If everyone is calling that, then they are the only ones that need to handle the translation. One thing that bugs me about the current approach is that there are two more-or-less standardized enumerations, but we don't have enum types for either of those -- instead we have an enum containing the mangled internal constants.

However, I concede that this is just a matter of presentation, and the "internal" enum would be essentially implemented by this set of accessors.

What would you say to something different instead -- we do something similar to what was done with location lists, where we "upgrade" the format to the newest version during parsing? This is almost what you are doing right now -- the only difference is that the "internal" enum would no longer be internal -- it would actually match the on-disk format of a v5 index.  This v5 enum would contain the official DWARFv5 constants as well as the new extensions we want to introduce for mixed 5+4 indices.

This means that if we adopt the currently proposed 5+4 approach (which is looking increasingly likely -- if you hadn't posted this patch, I would probably be working on it now), there will only be two enums. But until we actually start writing files like this, the new extension constants will still only be kind of internal, and if there is a change in the mixed index approach we can always shuffle things around here.

>> Regardless of the outcome of that, I think it would be good to split this patch up and separate the enum shuffling from the new functionality (does this only add parsing support for v5 indexes, or is there something more?).
> 
> I'll prepare a separate review for the refactoring in `llvm-dwp.cpp`. As for the new identifiers and all shuffling stuff around, I am not sure it is really valuable to separate them because without the parser of v5 indexes they are meaningless and just dead code. Anyway, the splitting should wait until we decide whether the approach is viable in general.

Some of the files in this patch are only touched because of the introduction of `_GNU` in the DW_SECT constants. It would be clearer if that are done in an NFC patch. Also the parsing of a v5 index and making sure that a v5 dwp compile unit can find its location lists correctly look like they could be separate (you already have separate tests for those two things anyway).

But yea, that can wait until we have consensus on the overall direction.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D75929