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

[David Blaikie via Phabricator via llvm-commits]

dblaikie 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);
----------------
Probably switch this around (don't usually see this kind of reverse comparison in the LLVM codebase):
  if (Form == dwarf::DW_FORM_implicit_const)



================
Comment at: lib/CodeGen/AsmPrinter/DIE.cpp:46
+  if (dwarf::DW_FORM_implicit_const == Form)
+    ID.AddInteger(Value);
 }
----------------
Does this need an unsigned cast like the other calls in this function?


================
Comment at: lib/CodeGen/AsmPrinter/DIE.cpp:114-117
+    if (dwarf::DW_FORM_implicit_const == Data[i].getForm())
+      O << " " << Data[i].getValue();
+
+    O << '\n';
----------------
This should be tested (probably by modifying the existing test that covers implicit_const to verify the value in the abbrev printed here)


================
Comment at: unittests/DebugInfo/DWARF/DwarfGenerator.h:135-137
+
+  /// Get actual DIE object
+  llvm::DIE &getDIE() { return *Die; }
----------------
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?


https://reviews.llvm.org/D30456