[PATCH] D30456: [DebugInfo] Unique abbrevs for DIEs with different implicit_const values

[David Blaikie via llvm-commits]

On Tue, Feb 28, 2017 at 9:26 AM Victor Leschuk via Phabricator <
reviews at reviews.llvm.org> wrote:

> vleschuk marked 2 inline comments as done.
> vleschuk added inline comments.
>
>
> ================
> Comment at: lib/CodeGen/AsmPrinter/DIE.cpp:45
>    ID.AddInteger(unsigned(Form));
> +  if (dwarf::DW_FORM_implicit_const == Form)
> +    ID.AddInteger(Value);
> ----------------
> dblaikie wrote:
> > Probably switch this around (don't usually see this kind of reverse
> comparison in the LLVM codebase):
> >   if (Form == dwarf::DW_FORM_implicit_const)
> >
> Will do.
>
>
> ================
> Comment at: lib/CodeGen/AsmPrinter/DIE.cpp:46
> +  if (dwarf::DW_FORM_implicit_const == Form)
> +    ID.AddInteger(Value);
>  }
> ----------------
> dblaikie wrote:
> > Does this need an unsigned cast like the other calls in this function?
> Nope, Value is signed integer (implicit_const values are always signed
> according to DWARF standard). And there is proper overloaded AddInteger
> method for signed ints.
>
>
> ================
> Comment at: lib/CodeGen/AsmPrinter/DIE.cpp:114-117
> +    if (dwarf::DW_FORM_implicit_const == Data[i].getForm())
> +      O << " " << Data[i].getValue();
> +
> +    O << '\n';
> ----------------
> dblaikie wrote:
> > This should be tested (probably by modifying the existing test that
> covers implicit_const to verify the value in the abbrev printed here)
> Actually this is dump method for debugging purposes. If I am not mistaken
> we do not test debug helpers.
>
>
> ================
> Comment at: unittests/DebugInfo/DWARF/DwarfGenerator.h:135-137
> +
> +  /// Get actual DIE object
> +  llvm::DIE &getDIE() { return *Die; }
> ----------------
> dblaikie wrote:
> > This seems like a significant API change (exposing this implementation
> detail) & not really using this API for its original purpose (which was to
> generate DWARF bytes to test the DWARF parsing APIs, rather than for
> testing the DWARF generation APIs)
> >
> > But it's not a bad test, as such... not sure what the right answer is
> here. Usually the DWARF generation APIs are tested in-situ (in LLVM test
> cases that exercise them, rather than in unit tests). It might be best to
> continue to stick with that for now, as much as I do like unit tests.
> >
> > Well, I guess the parsing APIs need testing too, though - so if you
> actually make this a round-trip test (then you won't need this new getDIE
> API) that would be good/acceptable. You might be able to test the dumping
> support in the unit test too?
> Maybe it would be better to proxy AbbrevSet.uniqueAbbreviation() through
> dwarfgen? In this case we will not expose actual DIE object to caller. What
> do you think?
>

I don't think that'd be much better - wouldn't address the broader question
of how these unit tests are intended (for testing parsing, not for testing
the generation - though I understand they necessarily test both, somewhat).


> Also I am afraid I don't understand how to test dumping from such unit
> test... Could you please point me in right direction?
>

Take a look at the other unit tests there - they generate and then parse
dwarf bytes, then query the resulting DWARFDebugInfoEntry objects, etc. And
those objects have the 'dump(raw_ostream&)' functions used by
llvm-dwarfdump. So you could call them in the unit test, using a
raw_string_ostream to write to, then check that it contains the desired
bytes.

I'm not sure this is the best idea (compared to checking in a
binary/assembly code and running that through llvm-dwarfdump, using
LIT/FileCheck testing) - but it's an option.


>
>
> https://reviews.llvm.org/D30456
>
>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20170228/6e690e3c/attachment.html>