<div dir="ltr"><div dir="ltr">On Mon, Oct 4, 2021 at 5:09 PM Adrian Prantl <<a href="mailto:aprantl@apple.com" target="_blank">aprantl@apple.com</a>> wrote:<br></div><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div><br><div><br><blockquote type="cite"><div>On Oct 4, 2021, at 10:54 AM, David Blaikie <<a href="mailto:dblaikie@gmail.com" target="_blank">dblaikie@gmail.com</a>> wrote:</div><br><div><div dir="ltr"><div dir="ltr"><br></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Tue, Sep 21, 2021 at 1:58 PM Adrian Prantl <<a href="mailto:aprantl@apple.com" target="_blank">aprantl@apple.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div><br><div><br><blockquote type="cite"><div>On Sep 21, 2021, at 1:11 PM, David Blaikie <<a href="mailto:dblaikie@gmail.com" target="_blank">dblaikie@gmail.com</a>> wrote:</div><br><div><div dir="ltr">Ping</div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Thu, Aug 12, 2021 at 7:42 PM David Blaikie <<a href="mailto:dblaikie@gmail.com" target="_blank">dblaikie@gmail.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir="ltr"><div dir="ltr">On Wed, Oct 11, 2017 at 10:03 AM Adrian Prantl <<a href="mailto:aprantl@apple.com" target="_blank">aprantl@apple.com</a>> wrote:<br></div><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><br>
<br>
> On Oct 11, 2017, at 9:45 AM, David Blaikie <<a href="mailto:dblaikie@gmail.com" target="_blank">dblaikie@gmail.com</a>> wrote:<br>
> <br>
> 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)<br>
<br>
It looks like this was introduced by <a href="https://reviews.llvm.org/D35521" rel="noreferrer" target="_blank">https://reviews.llvm.org/D35521</a><br>
<br>
"This patch modifies the handleDebugInfo() function so that we verify the contents of each unit<br>
in the .debug_info section only if its header has been successfully verified."<br>
<br>
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.<br>
<br>
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.<br>
<br>
If we can get away with deleting the redundant code that would definitely be a win.<br></blockquote><div><br>Resurrecting this thread, since I just came across this issue again - while trying to improve the verifier (after seeing a lot of "<span style="font-variant-ligatures:no-common-ligatures;font-family:Menlo;font-size:11px">error: invalid DIE reference 0x0000015f. Offset is in between DIEs:"</span> followed by a series of blank lines - caused by <br>
<span style="font-variant-ligatures:no-common-ligatures;font-family:Menlo;font-size:11px">ReferenceToDIEOffsets</span> 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).<br></div></div></div></blockquote></div></div></blockquote><div><br></div><div>I read through <a href="https://reviews.llvm.org/D35521" target="_blank">https://reviews.llvm.org/D35521</a> 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?</div></div></div></blockquote><div><br>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..<br><br>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:<br> DWARFUnitSection<DWARFCompileUnit> CUSection{};<br> Unit.reset(new DWARFCompileUnit(<br> DCtx, DCtx.getInfoSection(), DCtx.getDebugAbbrev(),<br> &DCtx.getRangeSection(), DCtx.getStringSection(),<br> DCtx.getStringOffsetSection(), &DCtx.getAppleObjCSection(),<br> DCtx.getLineSection(), DCtx.isLittleEndian(), false, CUSection,<br> nullptr));<br>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).<br><br>Does that make sense/accurately describe a/the problem?<br></div></div></div></div></blockquote><div><br></div><div>Outside of DWARFVerifier who is usually responsible for populating DCtx.compile_units?</div></div></div></blockquote><div><br>DWARFContext does when you query it for the compile units ( <a href="https://github.com/llvm-mirror/llvm/blob/2c4ca6832fa6b306ee6a7010bfb80a3f2596f824/include/llvm/DebugInfo/DWARF/DWARFContext.h#L151-L161">https://github.com/llvm-mirror/llvm/blob/2c4ca6832fa6b306ee6a7010bfb80a3f2596f824/include/llvm/DebugInfo/DWARF/DWARFContext.h#L151-L161</a> ).<br> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div><div><div> 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!</div></div></div></blockquote><div><br></div><div>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?<br><br>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. </div></div></div>