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

[David Blaikie via llvm-commits]

On Tue, Oct 10, 2017 at 2:19 PM Jonas Devlieghere <jdevlieghere at apple.com>
wrote:

>
> 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>
> wrote:
>
>>
>> On Oct 10, 2017, at 6:24 PM, David Blaikie <dblaikie at gmail.com> wrote:
>>
>>
>>
>> On Tue, Oct 10, 2017 at 10:06 AM Jonas Devlieghere via Phabricator <
>> 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.
>

Adrian - could you take a look here & see why
DWARFVerifier::handleDebugInfo is creating empty TUSection/CUSection,
rather than passing the DCtx's populated sections in? (that's my rough
reading of the code, at least)


>
>
> 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
>>
>>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20171011/9e0c4358/attachment-0001.html>