[PATCH] D43286: [CodeGen] Generate DWARF v5 Accelerator Tables

[Jonas Devlieghere via Phabricator via llvm-commits]

JDevlieghere added a comment.

This is looking very good. A few small comment inline but I don't expect this will need a lot of additional work.



================
Comment at: include/llvm/MC/MCObjectFileInfo.h:258
 
   // DWARF5 Experimental Debug Info Sections
+  MCSection *getDwarfDebugNamesSection() const {
----------------
Obsolete now we have the "real" thing? :-) 


================
Comment at: lib/CodeGen/AsmPrinter/AccelTable.cpp:15
 #include "llvm/CodeGen/AccelTable.h"
+#include "DwarfCompileUnit.h"
 #include "llvm/ADT/STLExtras.h"
----------------
should be on top (clang-format should do this for you IIRC)


================
Comment at: lib/CodeGen/AsmPrinter/AccelTable.cpp:193
+    uint32_t AugmentationStringSize = sizeof(AugmentationString);
+    char AugmentationString[8] = {'L', 'L', 'V', 'M', '0', '7', '0', '0'};
+    static_assert(sizeof(AugmentationString) % 4 == 0, "");
----------------
Hmm, we should probably move this elsewhere and have Cmake generate it for us? This seems hard to maintain and quite a burden, wouldn't you agree?


================
Comment at: lib/CodeGen/AsmPrinter/AccelTable.cpp:233
+                        const DwarfDebug &DD,
+                        ArrayRef<std::unique_ptr<DwarfCompileUnit>> CompUnits);
+
----------------
It seems odd that we sink the DwarfCompileUnits, why's that? 


================
Comment at: lib/CodeGen/AsmPrinter/AccelTable.cpp:362
+  Asm->OutStreamer->EmitLabel(Ctx.ContributionStart);
+  Asm->OutStreamer->AddComment("Header: version");
+  Asm->EmitInt16(Version);
----------------
This is different from the comments we currently generate for the Apple table header where we have "Header Version". I'm fine with either (or maybe I slightly prefer this). Reminder to self that I should unify this in a follow up commit. 


================
Comment at: lib/CodeGen/AsmPrinter/DwarfDebug.cpp:297
+  } else
+    AccelTableKind = DwarfAccelTables;
 
----------------
This doesn't seem right, if you specify apple explicitly you'll end up with DWARF? Shouldn't we just omit the else?


Repository:
  rL LLVM

https://reviews.llvm.org/D43286