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

Wed Nov 24 14:46:11 PST 2021

On Mon, Oct 25, 2021 at 10:07 AM David Blaikie <dblaikie at gmail.com> wrote:

> (ping on this)
>
> On Tue, Oct 5, 2021 at 12:09 PM David Blaikie <dblaikie at gmail.com> wrote:
>
>> On Mon, Oct 4, 2021 at 5:09 PM Adrian Prantl <aprantl at apple.com> wrote:
>>
>>>
>>>
>>> On Oct 4, 2021, at 10:54 AM, David Blaikie <dblaikie at gmail.com> wrote:
>>>
>>>
>>>
>>> On Tue, Sep 21, 2021 at 1:58 PM Adrian Prantl <aprantl at apple.com> wrote:
>>>
>>>>
>>>>
>>>> On Sep 21, 2021, at 1:11 PM, David Blaikie <dblaikie at gmail.com> wrote:
>>>>
>>>> Ping
>>>>
>>>> On Thu, Aug 12, 2021 at 7:42 PM David Blaikie <dblaikie at gmail.com>
>>>> wrote:
>>>>
>>>>> On Wed, Oct 11, 2017 at 10:03 AM Adrian Prantl <aprantl at apple.com>
>>>>> wrote:
>>>>>
>>>>>>
>>>>>>
>>>>>> > On Oct 11, 2017, at 9:45 AM, David Blaikie <dblaikie at gmail.com>
>>>>>> wrote:
>>>>>> >
>>>>>> > 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)
>>>>>>
>>>>>> It looks like this was introduced by https://reviews.llvm.org/D35521
>>>>>>
>>>>>> "This patch modifies the handleDebugInfo() function so that we verify
>>>>>> the contents of each unit
>>>>>> in the .debug_info section only if its header has been successfully
>>>>>> verified."
>>>>>>
>>>>>> So it looks like the code is trying to avoid parsing the unit if the
>>>>>> header didn't verify? At the first glance this does look bit redundant to
>>>>>> me but I didn't give it a thorough reading.
>>>>>>
>>>>>> Jonas, can you make sense of it in the context of that commit? I
>>>>>> would not be surprised if this is just compensating for a missing feature
>>>>>> in the codebase at that point in time.
>>>>>>
>>>>>> If we can get away with deleting the redundant code that would
>>>>>> definitely be a win.
>>>>>>
>>>>>
>>>>> Resurrecting this thread, since I just came across this issue again -
>>>>> while trying to improve the verifier (after seeing a lot of "error:
>>>>> invalid DIE reference 0x0000015f. Offset is in between DIEs:" followed
>>>>> by  a series of blank lines - caused by
>>>>> ReferenceToDIEOffsets being kept and used over multiple sections
>>>>> (when it contains section relative offsets without any base section to know
>>>>> which section they are relative to) - so you get invalid references from
>>>>> type units, and then verifyDebugInfoReferences tries to dump the DIEs in
>>>>> the type units that have the "invalid" references and it can't dump those
>>>>> DIEs (just ends up with spooky blank lines) because it's looking in the
>>>>> wrong section for them).
>>>>>
>>>>
>>>> I read through https://reviews.llvm.org/D35521 but it wasn't enough
>>>> for me to understand the problem. Maybe you can help me clarify what's
>>>> going on. For what you are saying it sounds like the Verifier code was
>>>> written without regard for type units, and so if we have a verifier error
>>>> in a type unit verifyDebugInfoReferences returns incorrect results
>>>> because ReferenceToDIEOffsets has no concept of which unit a relative
>>>> offset should be resolved in, but I'm not sure if that's accurate. What
>>>> situations are currently not handled correctly?
>>>>
>>>
>>> Thanks for taking a look, sorry it's taken me a bit to come back to
>>> this. I'm trying to page the state back in now..
>>>
>>> D35521 changed handleDebugInfo - it previously used `for (const auto &CU
>>> : DCtx.compile_units()) {`, which would populate the DCtx's CU/TUSection
>>> members - but after the change that's no longer used, and DWARFUnit's are
>>> explicitly constructed like this:
>>>         DWARFUnitSection<DWARFCompileUnit> CUSection{};
>>>         Unit.reset(new DWARFCompileUnit(
>>>             DCtx, DCtx.getInfoSection(), DCtx.getDebugAbbrev(),
>>>             &DCtx.getRangeSection(), DCtx.getStringSection(),
>>>             DCtx.getStringOffsetSection(), &DCtx.getAppleObjCSection(),
>>>             DCtx.getLineSection(), DCtx.isLittleEndian(), false,
>>> CUSection,
>>>             nullptr));
>>> Which means the DCtxt's CU/TUSection members will be empty (so
>>> attempting to resolve a cross-CU reference in the verifier will break, I
>>> think?) and the UnitVector reference member of DWARFUnit will be left
>>> dangling (because the CUSection (similarly for the TUSection code nearby)
>>> variable above goes out of scope immediately after the construction of the
>>> unit).
>>>
>>> Does that make sense/accurately describe a/the problem?
>>>
>>>
>>> Outside of DWARFVerifier who is usually responsible for populating
>>> DCtx.compile_units?
>>>
>>
>> DWARFContext does when you query it for the compile units (
>> https://github.com/llvm-mirror/llvm/blob/2c4ca6832fa6b306ee6a7010bfb80a3f2596f824/include/llvm/DebugInfo/DWARF/DWARFContext.h#L151-L161
>> ).
>>
>>
>>> I don't have enough context right now to understand why
>>> handleDebugInfo() needs to be in the business of creating new Unit DIEs
>>> just for the purpose of Verification. Is that something you already figured
>>> out? If you don't know either, I can step through the code get a better
>>> sense of why that's necessary!
>>>
>>
>> No, I don't have more context here - my best guess was this was added to
>> possibly provide more verification (being more involved in the parsing of
>> headers - being able to verify various header fields even if the overall
>> unit fails to parse and thus wouldn't end up in the compile_units list,
>> etc) and maybe more ability to progress when failures are hit?
>>
>> I was hoping you/some folks involved in the change that caused this might
>> be able to help out/address this issue - I know it's rather belated code
>> review feedback at this point, though.
>>
>
Made a best effort go of this myself here:
https://github.com/llvm/llvm-project/commit/cd93ab8947a88470b3e1ef738a2947c0bfda667f

A simple example verification now only instantiates DWARFUnit once per unit
instead of twice. (tested with a debugger/breakpoint in the ctor)
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20211124/4e91bc9c/attachment.html>