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

[Victor Leschuk via Phabricator via llvm-commits]

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?

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


https://reviews.llvm.org/D30456