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

[David Blaikie via llvm-commits]

(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.
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20211025/503dfe0b/attachment.html>