[PATCH] D101894: [llvm-dwp] Add support for rnglists and loclists

[Kim-Anh Tran via Phabricator via llvm-commits]

kimanh added a comment.

In D101894#2758200 <https://reviews.llvm.org/D101894#2758200>, @dblaikie wrote:

> In D101894#2753779 <https://reviews.llvm.org/D101894#2753779>, @kimanh wrote:
>
>> In D101894#2745302 <https://reviews.llvm.org/D101894#2745302>, @dblaikie wrote:
>>
>>>> Yes exactly, for CU units there are "CUIdentifiers" (Signature, name, dwo id) that are recorded for error purposes, if a duplicate CU is detected. Before extracting these attributes there is a check whether this unit is actually is a CU unit or not, so we need the abbrev section to at least have info on the unit type. This check does currently not exist for TU units (for DWARF5 this will be checked using the unit type field, for DWARF4 type units this is however not checked at the moment). I think the check if it's a compile unit should stay where it is (given that we need to access the attributes for error purposes afterwards), so I'm not sure how we should change the code to allow for no DIEs at all.
>>>
>>> Ah, right right, I remember adding that - potentially that code could be refactored to not try to parse the name until the dwo ID collision occurs - then these tests wouldn't need any DIEs, for instance (which isn't, in and of itself, a valuable thing - but does show that usually the content of the DIE tree is not examined/used/relevant to the DWP tool, only a "nice to have" for error messages).
>>
>> I had another look at the code: not all of the information that is used in the CUIdentifiers is actually needed at that point in time (although it is set, but it is set to null). The only piece that definitely is required is the signature of compile units if parsing DWARFv4, since the signature is used to build the index. The signature is taken from the .debug_abbrev section, so the check if it's actually a compile unit or not should stay (at least for v4), so I tend towards keeping it this way. WDYT?
>
> I don't think there's anything you /have/ to do here - my suggestion was more a "could be nice", sorry for the tone/confusion.
>
> I think in principle it would be nice if we didn't parse stuff we didn't need to - yeah, that means in DWARFv4 we still need to parse the CU's main DIE to get the dwo_id. But in DWARFv5 we can get away with only reading the header - then if there's a duplicate dwo_id /then/ we could try to parse the DIE to get the dwo name, etc.
>
> Just a nice to have, maybe, at some point. Though there are far bigger performance problems to deal with with llvm-dwp anyway, so not to worry. Sorry for the diversion.

Oh, no worries at all, and thanks for the clarification! :) Ok, I'll keep it this way then. Since this patch is dependent on the others I'll ping this thread again as soon as the parent patches are ready.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D101894