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

[David Blaikie via llvm-commits]

On Mon, Apr 30, 2018 at 2:44 PM Greg Clayton <clayborg at gmail.com> wrote:

>
> On Apr 30, 2018, at 11:54 AM, David Blaikie <dblaikie at gmail.com> wrote:
>
>
>
> 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).
>
>
> We have many tools here at Facebook that use LLVM's DWARF parser to
> symbolicate information and rely on the getting the names correctly from
> any producer of DWARF.
>

"correctly" is what I'm wondering about - without seeing the example DWARF
it's hard to know if this is the correct answer or not.


> I can't tell you exactly which compilers generate DWARF with multiple
> indirections, but I do know it happens. There are also many post production
> tools that we have here and take an existing linked binary and produce a
> new binary and fix the DWARF up. So in these cases, it isn't the compiler
> itself that ends up vending the final DWARF. So even if we identify the
> compiler that might produce this DWARF, there is no guarantee that the
> DWARF won't get changed. Granted, none of these tools will change
> abbreviation declarations that I know of.
>
> That being said I would be fine not dealing with recursion as the case
> that I fixed didn't have recursion. And if the DWARF is invalid I don't
> know how much we can/should do to try and recover.
>

Sorry, when I say "recursion" I mean "more than one specification or more
than one abstract_origin in the chain" - which I think is what your patch
that switched to recursion was implementing, right?

I'd like to see an example of that, to know what it is, why it exists/what
it means & so we can know why we're supporting it, before we do.

Yeah, even if it's a chain of weird tools - if it's producing dwarf that's
not really meaningful (perhaps the tools are buggy, etc) then it's probably
best not to add support for it in Clang, for example.

- Dave


>
>
> - 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/8ccc11ac/attachment.html>