[PATCH] D43092: [DebugInfo] Prevent infinite recursion for malformed DWARF

[David Blaikie via llvm-commits]

On Mon, Apr 30, 2018 at 11:46 AM Greg Clayton <clayborg at gmail.com> wrote:

>
> On Apr 30, 2018, at 11:41 AM, Jonas Devlieghere <jdevlieghere at apple.com>
> wrote:
>
>
>
> On Apr 30, 2018, at 6:28 PM, David Blaikie <dblaikie at gmail.com> wrote:
>
> Sorry - could we revisit this?
>
>
> Of course :-)
>
>
> I think my concerns expressed still haven't been resolved:
>
> "Could you check the revision history here? I'm pretty sure the first
> version of this I reviewed from Greg wasn't recursive - and then it became
> recursive at some point to handle something needed, but maybe those
> decisions need to be reexamined?" (& related comments back there)
>
>
> Greg added the recursiveness in D40156 because he saw DWARFDie::getName()
> failing “in the wild”. I think it was for some Google project but I’d have
> to double check the mail thread.
>
> IIRC we all agreed that for “valid” DWARF we shouldn’t need the recursion.
> The question is whether we want to support the “invalid case or not, as
> it’s definitely being generated (but maybe not by clang?). If the answer is
> yes, then r331200 is strictly better than what we had before. Now we don’t
> crash if there’s a cycle. If the answer is no, we just need to go back to
> the non-recursive implementation. Personally I don’t have a strong opinion,
> as long as we don’t crash.
>
>
> I indeed did see a DIE that had a DW_AT_specification and pointed to
> another die that also had a DW_AT_specification. Regardless of what the
> DWARF spec says, compilers or post production DWARF tools are are producing
> DWARF that has these multiple chaining of specifications or abstract
> origins. There is also no enforcement of the DWARF spec anywhere, so
> compilers produce a wide variety of different DWARF that may or may not be
> legal. We have "llvm-dwarfdump --verify" but it isn't exhaustive in what it
> reports. So I would vote that we have a DWARF parser that can handle what
> ever is thrown at it when possible. Just my 2 cents.
>

I'm not suggesting the DWARF parser should crash or be unhelpful on such
inputs - but the convenience of following abstract origin/specification DIE
references to print names seems OK if it's limited to the situations we
know make sense until we see something else?

So if we could see/figure out what producer is generating such DWARF and
look an some examples, decide if it's meaningful/useful to support name
printing in that case, I'd be all for it. Without that, I'd rather err on
the side of simplicity and not support the recursive case (& then have to
handle/defend against cycles, etc).

- Dave


>
> Greg
>
>
> Cheers,
> Jonas
>
>
> On Mon, Apr 30, 2018 at 10:06 AM Jonas Devlieghere via Phabricator <
> reviews at reviews.llvm.org> wrote:
>
>> This revision was automatically updated to reflect the committed changes.
>> Closed by commit rL331200: [DebugInfo] Prevent infinite recursion for
>> malformed DWARF (authored by JDevlieghere, committed by ).
>>
>> Changed prior to commit:
>>   https://reviews.llvm.org/D43092?vs=144485&id=144582#toc
>>
>> Repository:
>>   rL LLVM
>>
>> https://reviews.llvm.org/D43092
>>
>> Files:
>>   llvm/trunk/lib/DebugInfo/DWARF/DWARFDie.cpp
>>   llvm/trunk/test/tools/llvm-dwarfdump/X86/invalid_abstract_origin.s
>>
>>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20180430/a9610706/attachment.html>