[PATCH] D28386: Add the ability to iterate across all attributes in a DIE.

[David Blaikie via llvm-commits]

On Mon, Jan 9, 2017 at 3:46 PM Greg Clayton <clayborg at gmail.com> wrote:

> On Jan 9, 2017, at 3:04 PM, David Blaikie <dblaikie at gmail.com> wrote:
>
>
>
> On Mon, Jan 9, 2017 at 2:55 PM Greg Clayton <clayborg at gmail.com> wrote:
>
> On Jan 9, 2017, at 2:41 PM, David Blaikie <dblaikie at gmail.com> wrote:
>
>
>
> On Mon, Jan 9, 2017 at 2:29 PM Greg Clayton <clayborg at gmail.com> wrote:
>
> On Jan 9, 2017, at 2:23 PM, David Blaikie <dblaikie at gmail.com> wrote:
>
> I'm not sure optional error handling's ideal - seems errors should always
> be handled (which is the strong premise behind Lang's work introducing
> llvm::Error).
>
>
> What is a DWARF parser during a debug session gonna do with any of these
> errors? Emit them in the console? If we have borked DWARF we can’t just
> emit every error we come across, the user will not like that.
>
>
> Quite possibly, yes. That seems to be what LLDB does already for DWARF it
> can't handle - and I imagine not reading any attributes would start to look
> like "not handling" to a user when things show up without names, address
> ranges, or any other functionality?
>
>
> I guess after we parse the DWARF and make the skeleton of the DIEs
> successfully that the input is good and that is kind of an invariant. We
> wouldn’t be extracting any data from DIEs using a DWARFDie if we weren’t
> able to parse the debug info already to get the layout of the DWARF. I
> would like to assume that the DWARF is good at this point and we are just
> accessing it.
>
>
> But that doesn't seem to be true - we don't parse most of the DWARF we
> just skip over the bytes to find the next DIE because we know the size.
> Seems possible some of those bytes could be invalid, perhaps? *looks
> through the decoding code... * perhaps not - I'm not sure if there are any
> cases of invalid bit patterns in the attributes
>
>
> If a value says it is a DW_FORM_data2, we can’t say anything about the bit
> pattern in there unless we know what the attribute is and what values are
> valid.
>

Fair point - so perhaps we could assert in the extractValue as well, since
we should never reach the point of attempting to extract an attribute for
an unknown form?


> Was there a recommendation here?
>
>
>
>
>
>
> If we add error handling, I don’t really plan on integrating it into the
> LLDB parser, but you seemed to want this so I added it. I personally don’t
> think it is useful.
>
>
> What should LLDB do when it encounters broken input? What does it do when
> it encounters input it can't parse today?
>
>
> Again, the DWARF would have failed to parse and no DIEs would be visited
> if the pass through the compile units died in the first place.
>
>
> Then it sounds like we shouldn't ever stop short because we should never
> encounter errors - instead we should assert that we don't encounter errors,
> instead of writing code to handle a case we believe to be an invariant
> violation.
>
>
> So remove the error handling and just assert that everything goes well in
> the iterator code that grabs the attribute at index?
>

If you've got a strong basis/proof/belief that the error cases are
unreachable (ie: there's no object file I could feed the parser that would
fail there), then yes - just assert that these are expected invariants.

In some cases (like the ones I mentioned - getForm/AttrByIndex) the
assertion could be sunk into lower levels.


>
>
> Indeed, some of the APIs could be changed to have stronger preconditions
> rather than handling error cases (getForm/AttrByIndex should probably just
> assert that the index is valid - same as std::vector[] does, basically? and
> this caller could assert that DWARFFormValue::extractValue returns true -
> so long as we check up-front hat we aren't parsing any unknown forms... but
> I think we want to support unknown forms, so perhaps that operation should
> be fallible?
>
>
> If we don’t know a form, there is no way to recover as you must understand
> what a form contains or you can’t parse.
>
> & thus walking the attributes could fail at that operation (or we could
> not expose a value there? Seems like a erasonable definition of behavior
> and not a failure mode - and we can/should easily write a test for that))
>
>
> That can fail, but we would have failed to parse the DIEs in the compile
> unit because first we would have made a DWARFAbbreviationDeclaration and
> then we would try to use that to skip the form value, and the
> DWARFFormValue::skipValue() would return false and the DIE would fail to
> parse and we would then fail all DIEs in that compile unit.
>
>
>
> We just try to iterate through the attributes and if anything goes wrong
> we exit the attribute loop. I really don’t see the point of the errors, or
> if they have to exist then they should be optional.
>
> Greg
>
>
>
>
> Greg
>
> On Mon, Jan 9, 2017 at 2:21 PM Greg Clayton via Phabricator <
> reviews at reviews.llvm.org> wrote:
>
> clayborg updated this revision to Diff 83698.
> clayborg added a comment.
>
> Added optional error handling to the attributes iterators. You can now
> pass an "llvm::Error *" to the attributes():
>
>   attribute_iterator DWARFDie::attributes(llvm::Error *Err);
>
> If the error is non-NULL, then the error will be filled in. This iteration
> error is the same method used in Archive.h/Archive.cpp after I spoke with
> Lang Hames.
>
>
> https://reviews.llvm.org/D28386
>
> Files:
>   include/llvm/DebugInfo/DWARF/DWARFAbbreviationDeclaration.h
>   include/llvm/DebugInfo/DWARF/DWARFAttribute.h
>   include/llvm/DebugInfo/DWARF/DWARFDie.h
>   lib/DebugInfo/DWARF/DWARFDie.cpp
>   unittests/DebugInfo/DWARF/DWARFDebugInfoTest.cpp
>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20170109/62138cda/attachment.html>