[PATCH] D75929: [DebugInfo] Support DWARFv5 index sections.

Wed Mar 11 06:18:06 PDT 2020

ikudrin added a comment.

In D75929#1916466 <https://reviews.llvm.org/D75929#1916466>, @labath wrote:

> In D75929#1916127 <https://reviews.llvm.org/D75929#1916127>, @ikudrin wrote:
>
> > I believe that this patch is more or less compatible with any approach which might be taken. The idea is that there is a set of constants for internal use and functions to translate them to/from external representation and both constants and translation functions might be adjusted when needed. In any case, the general design remains the same.
>
>
> 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.

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

> I'm wondering if if wouldn't be simpler to have two complete enums --  with the "gnu" scheme, and one with the "official" scheme -- and then to internally use the enum matching the on-disk format currently in use. To insulate the users from having to guess the right enum to use, we could add a series of accessors: `getLocOffset`, `getMacInfoOffset`, ..., which would use the appropriate enum based on the index version (or assert if there is no such constant in the given version).
> 
> WDYT?

Can't see how the accessors simplify the things. For me, `getLocOffset()` is almost the same as `getOffset(DW_SECT_LOC)` (or `getOffset(DW_SECT_GNU_LOC)`). That is no more than an agreement. Note that this patch does not have things like `DW_SECT_GNU_INFO`. There is only one constant for each section, so their usage is quite determined. The only exception is `DW_SECT_MACRO` and `DW_SECT_GNU_MACRO`, I just did not want to overcomplicate translation functions before receiving the feedback about the approach in general.

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

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D75929