Page MenuHomePhabricator

DWARF: Fix a bug in array size computation
ClosedPublic

Authored by labath on Dec 21 2018, 2:10 AM.

Details

Summary

r346165 introduced a bug, where we would fail to parse the size of an
array if that size happened to match an existing die offset.

The logic was:
if (DWARFDIE count = die.GetReferencedDie(DW_AT_count))

num_elements = compute_vla_size(count);

else

num_elements = die.GetUsigned(DW_AT_count); // a fixed-size array

The problem with this logic was that GetReferencedDie did not take the
form class of the attribute into account, and would happily return a die
reference for any form, if its value happened to match some die.

As this behavior is inconsistent with how llvm's DWARFFormValue class
operates, I chose to fix the problem by making our version of this class
match the llvm behavior. For this to work, I had to add an explicit form
class check to the .apple_XXX tables parsing code, because they do
(incorrectly?) use data forms as die references.

Diff Detail

Repository
rL LLVM

Event Timeline

labath created this revision.Dec 21 2018, 2:10 AM
clayborg requested changes to this revision.Dec 21 2018, 7:26 AM
clayborg added inline comments.
source/Plugins/SymbolFile/DWARF/DWARFFormValue.cpp
619 ↗(On Diff #179254)

DW_FORM_ref_sig8 is a type signature and shouldn't be returned as a valid DWARF DIE reference. In the .debug_types patch, this uses m_cu to get to the debug info and uses the type signature to DWARF offset map to return a valid .debug_info offset. If the DWARF is older where we have both .debug_info and .debug_types. we add the size of the .debug_info to the result since we concatenate the .debug_info and .debug_types and treat it as one large DWARF section

620 ↗(On Diff #179254)

Not sure that this is, but if it isn't an absolute .debug_info offset, then we shouldn't return it here

639 ↗(On Diff #179254)

Ditto above

641 ↗(On Diff #179254)

Ditto above

This revision now requires changes to proceed.Dec 21 2018, 7:26 AM

Hm.. I can't say I have given much thought about this beforehand, as I just copied this code from llvm. However, now that I did think about it, I believe the current implementation makes sense. Nothing about DWARFFormValue::Reference (well, except the local variable name, which I guess I should rename) says it must return a DIE offset. DW_FORM_ref_sig8 is not a die offset, but it is certainly a DIE reference, albeit one that must be resolved in a more complicated way. This is also the view taken by the DWARF 5 spec (section 7.5.5 "Classes and Forms" lists DW_FORM_ref_sig8 as a reference form). And I expect this is the what the llvm implementation is based on.

So, in order to minimize confusion, I think we should stick to this definition. The code for resolving the more complex reference forms can live in some higher level code (DWARFDIE::GetReferencedDie ?), where it will be in a better position to locate and extract the DIE from other sections/object files.

clayborg accepted this revision.Dec 21 2018, 9:43 AM
This revision is now accepted and ready to land.Dec 21 2018, 9:43 AM
jankratochvil added inline comments.
source/Plugins/SymbolFile/DWARF/DWARFFormValue.cpp
620 ↗(On Diff #179254)

It is DWZ's reference from main debug file into shared DWZ common file - implemented by pending D40474.
I agree it needs more complicated handling than this implementation so it should not be here.

This revision was automatically updated to reflect the committed changes.