[PATCH] D27326: Make a DWARF generator so we can unit test DWARF APIs with gtest.

[Adrian Prantl via Phabricator via llvm-commits]

aprantl added a comment.

Couple of inline comments.



================
Comment at: include/llvm/CodeGen/DIE.h:122
+/// into the .debug_abbrev section.
+
+class DIEAbbrevSet {
----------------
The extra newline could defeat doxygen..


================
Comment at: include/llvm/CodeGen/DIE.h:129
+  llvm::FoldingSet<DIEAbbrev> AbbreviationsSet;
+  // A list of all the unique abbreviations in use.
+  std::vector<DIEAbbrev *> Abbreviations;
----------------
///


================
Comment at: lib/CodeGen/AsmPrinter/DIE.cpp:388
   case dwarf::DW_FORM_ref_addr:
-    Size = SizeOf(Asm, dwarf::DW_FORM_ref_addr);
+    LLVM_FALLTHROUGH;
+    Size = Size = SizeOf(Asm, Form);
----------------
`LLVM_FALLTHROUGH` *and* `break`?


================
Comment at: lib/CodeGen/AsmPrinter/DIE.cpp:389
+    LLVM_FALLTHROUGH;
+    Size = Size = SizeOf(Asm, Form);
     break;
----------------
I think we might as well move the 
`Asm->OutStreamer->EmitIntValue(Integer, Size);`
up here and return since it is only reachable from this case?


================
Comment at: lib/CodeGen/AsmPrinter/DIE.cpp:437
+  case dwarf::DW_FORM_ref_sup:
+    // 4 bytes for DWARF32, 8 bytes for DWARF64.
+    return 4; // TODO: support DWARF64
----------------
maybe add a getPointerSize method somewhere and hardcode it there?


================
Comment at: lib/CodeGen/AsmPrinter/DIE.cpp:593
+void DIEInlineString::EmitValue(const AsmPrinter *AP, dwarf::Form Form) const {
+  if (Form == dwarf::DW_FORM_string) {
+    for (char ch : S)
----------------
better write
`assert(Form == dwarf::DW_FORM_string` && "Expected valid string form")`
and get rid of the extra indentation. I don't think there's a good reason for being defensive here, is there?


================
Comment at: lib/CodeGen/AsmPrinter/DIE.cpp:603
+unsigned DIEInlineString::SizeOf(const AsmPrinter *AP, dwarf::Form Form) const {
+  return S.size() + 1; // Emit string bytes + NULL byte.
+}
----------------
-Wpedantic: I think the coding guide lines prefer the commont on a separate line.


================
Comment at: lib/CodeGen/AsmPrinter/DIE.cpp:620
+  switch (Form) {
+  default:
+    llvm_unreachable("Improper form for DIE reference");
----------------
move to bottom to match other switch-stmts in this file?


================
Comment at: lib/CodeGen/AsmPrinter/DIE.cpp:674
+    // TODO: Support DWARF64 by returning 8 for DWARF64 files.
+    return 4;
+  }
----------------
see my other comment


================
Comment at: lib/CodeGen/DwarfGenerator.cpp:122
+
+dwarfgen::Generator::~Generator() = default;
+
----------------
inline in the header?


================
Comment at: lib/CodeGen/DwarfGenerator.cpp:124
+
+bool dwarfgen::Generator::init(Triple TheTriple, uint16_t V) {
+  Version = V;
----------------
should we use any of the new error handling mechanisms here instead of returning a bool?


================
Comment at: lib/CodeGen/DwarfGenerator.h:170
+  std::unique_ptr<MCContext> MC;
+  MCAsmBackend *MAB; // Owned by MCStreamer
+  std::unique_ptr<MCInstrInfo> MII;
----------------
can this be a nullptr? Should it be a reference?


================
Comment at: unittests/DebugInfo/DWARF/CMakeLists.txt:9
+  Support
+  Target
   )
----------------
Are we 100% sure that all users of libDebugInfoDWARF want to link agains  all of these extra libraries all the time? If not, we may need to layer this differently.


================
Comment at: unittests/DebugInfo/DWARF/DWARFDebugInfoTest.cpp:63-66
+  if (AddrSize == 8)
+    Triple = llvm::Triple("x86_64--");
+  else
+    Triple = llvm::Triple("i386--");
----------------
clayborg wrote:
> Is there a better way to ensure we get a 32 and 64 bit target we can use for generating DWARF? This is my only issue with the patch that I know of. This triple code is repeated 3 times in this file. What happens if we aren't able to get these architectures?
This definitely looks like it would break on the bots that build only an arm backend.


================
Comment at: unittests/DebugInfo/DWARF/DWARFDebugInfoTest.cpp:774
+  TestReferences<4, AddrType>();
+}
+
----------------
Thanks for add all these tests!


https://reviews.llvm.org/D27326