[PATCH] D38719: [llvm-dwarfdump] Verify compatible TAG for attributes.

[Jonas Devlieghere via llvm-commits]

> On Oct 10, 2017, at 8:17 PM, David Blaikie <dblaikie at gmail.com> wrote:
> 
> 
> 
> On Tue, Oct 10, 2017 at 12:06 PM Jonas Devlieghere <jdevlieghere at apple.com <mailto:jdevlieghere at apple.com>> wrote:
> 
>> On Oct 10, 2017, at 6:24 PM, David Blaikie <dblaikie at gmail.com <mailto:dblaikie at gmail.com>> wrote:
>> 
>> 
>> 
>> On Tue, Oct 10, 2017 at 10:06 AM Jonas Devlieghere via Phabricator <reviews at reviews.llvm.org <mailto:reviews at reviews.llvm.org>> wrote:
>> JDevlieghere marked 2 inline comments as done.
>> JDevlieghere added inline comments.
>> 
>> 
>> ================
>> Comment at: lib/DebugInfo/DWARF/DWARFVerifier.cpp:423
>> +    // Make sure specification and abstract origin have compatible tag.
>> +    if (auto Offset = AttrValue.Value.getAsReference()) {
>> +
>> ----------------
>> dblaikie wrote:
>> > Can you use DWARFDie::getAttributeValueAsReferencedDie here, perhaps?
>> Unfortunately not, because this makes use of `getUnitForOffset` which in turn uses a list of units that isn't build when doing the verification.
>> 
>> Not sure I understand - this code calls getDIEForOffset, which calls getUnitForOffset as well. So why would one path be usable and the other not?
> 
> I would have to double check with the code in front of me, but it uses an upper_bound to find the right unit based on the offset, and this code path doesn’t populate that list. I noticed this earlier when I was trying to understand why the name of the subroutine is shown when dumping the debug info but not when running the verifier. I think this is the result of only considering one unit at a time when verifying, but I didn’t dig much deeper once I had found the explanation. 
> 
> Yeah, I'm really not understanding the difference between these possible codepaths.
> 
> The code in this review calls:
> 
>   auto Offset = AttrValue.Value.getAsReference()
>   DCtx.getDIEForOffset(*Offset)
>   ->CUs.getUnitForOffset(*Offset)->getDIEForOffset(*Offset);
> 
> the code in DWARFDie::getAttributeValueAsReferencedDie calls:
> 
>   auto SpecRef = toReference(find(Attr));
>   U->getUnitSection().getUnitForOffset(*SpecRef)->getDIEForOffset(*SpecRef);
> 
> (where 'getUnitSection' returns DWARFUnitSectionBase, and 'CUs' is a DWARFUnitSectionBase (well, a class derived from it, DWARFUnitSection<DWARFCompileUnit>)

The difference is that DWARFVerifier::handleDebugInfo is using “dummy” objects for the DWARFUnitSection objects:

DWARFUnitSection<DWARFTypeUnit> TUSection{};
DWARFUnitSection<DWARFCompileUnit> CUSection{};

As a result the list with CUs is empty and U->getUnitSection().getUnitForOffset(*Offset) returns nothing while CUs.getUnitForOffset(*Offset) returns the correct unit. 

I want to emphasize that I’m not trying to defend the current approach. Could you have a quick look at DWARFVerifier::handleDebugInfo and help me understand if there’s a good reason for this or maybe it is something that just needs to be fixed. 

> 
> So I'm really having a hard time understanding why these codepaths would be different & it seems subtle/worth fixing if they are.

Definitely! I’m relatively new to the code base so it’s not always obvious when something is a bug or I just lack the necessary understanding. 

>  
> 
>>  
>> Furthermore, this is (probably) slightly more efficient as we already have the attribute value and therefore do not need to do any indirections via the abbrev declaration.
>> 
>> 
>> https://reviews.llvm.org/D38719 <https://reviews.llvm.org/D38719>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20171010/6182df3d/attachment.html>