<div dir="ltr"><br><br><div class="gmail_quote"><div dir="ltr">On Tue, Feb 28, 2017 at 9:26 AM Victor Leschuk via Phabricator <<a href="mailto:reviews@reviews.llvm.org">reviews@reviews.llvm.org</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">vleschuk marked 2 inline comments as done.<br class="gmail_msg">
vleschuk added inline comments.<br class="gmail_msg">
<br class="gmail_msg">
<br class="gmail_msg">
================<br class="gmail_msg">
Comment at: lib/CodeGen/AsmPrinter/DIE.cpp:45<br class="gmail_msg">
   ID.AddInteger(unsigned(Form));<br class="gmail_msg">
+  if (dwarf::DW_FORM_implicit_const == Form)<br class="gmail_msg">
+    ID.AddInteger(Value);<br class="gmail_msg">
----------------<br class="gmail_msg">
dblaikie wrote:<br class="gmail_msg">
> Probably switch this around (don't usually see this kind of reverse comparison in the LLVM codebase):<br class="gmail_msg">
>   if (Form == dwarf::DW_FORM_implicit_const)<br class="gmail_msg">
><br class="gmail_msg">
Will do.<br class="gmail_msg">
<br class="gmail_msg">
<br class="gmail_msg">
================<br class="gmail_msg">
Comment at: lib/CodeGen/AsmPrinter/DIE.cpp:46<br class="gmail_msg">
+  if (dwarf::DW_FORM_implicit_const == Form)<br class="gmail_msg">
+    ID.AddInteger(Value);<br class="gmail_msg">
 }<br class="gmail_msg">
----------------<br class="gmail_msg">
dblaikie wrote:<br class="gmail_msg">
> Does this need an unsigned cast like the other calls in this function?<br class="gmail_msg">
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.<br class="gmail_msg">
<br class="gmail_msg">
<br class="gmail_msg">
================<br class="gmail_msg">
Comment at: lib/CodeGen/AsmPrinter/DIE.cpp:114-117<br class="gmail_msg">
+    if (dwarf::DW_FORM_implicit_const == Data[i].getForm())<br class="gmail_msg">
+      O << " " << Data[i].getValue();<br class="gmail_msg">
+<br class="gmail_msg">
+    O << '\n';<br class="gmail_msg">
----------------<br class="gmail_msg">
dblaikie wrote:<br class="gmail_msg">
> This should be tested (probably by modifying the existing test that covers implicit_const to verify the value in the abbrev printed here)<br class="gmail_msg">
Actually this is dump method for debugging purposes. If I am not mistaken we do not test debug helpers.<br class="gmail_msg">
<br class="gmail_msg">
<br class="gmail_msg">
================<br class="gmail_msg">
Comment at: unittests/DebugInfo/DWARF/DwarfGenerator.h:135-137<br class="gmail_msg">
+<br class="gmail_msg">
+  /// Get actual DIE object<br class="gmail_msg">
+  llvm::DIE &getDIE() { return *Die; }<br class="gmail_msg">
----------------<br class="gmail_msg">
dblaikie wrote:<br class="gmail_msg">
> 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)<br class="gmail_msg">
><br class="gmail_msg">
> 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.<br class="gmail_msg">
><br class="gmail_msg">
> 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?<br class="gmail_msg">
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?<br class="gmail_msg"></blockquote><div><br>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).<br> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Also I am afraid I don't understand how to test dumping from such unit test... Could you please point me in right direction?<br class="gmail_msg"></blockquote><div><br></div><div>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.<br><br>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.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br class="gmail_msg">
<br class="gmail_msg">
<a href="https://reviews.llvm.org/D30456" rel="noreferrer" class="gmail_msg" target="_blank">https://reviews.llvm.org/D30456</a><br class="gmail_msg">
<br class="gmail_msg">
<br class="gmail_msg">
<br class="gmail_msg">
</blockquote></div></div>