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

Tue Oct 5 12:09:06 PDT 2021

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.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20211005/3b75ff59/attachment.html>