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

[Adrian Prantl via Phabricator via llvm-commits]

aprantl added a comment.

Thanks for working on this!



================
Comment at: include/llvm/CodeGen/AccelTable.h:249
 
+class DWARF5AccelTableData : public AccelTableData {
+public:
----------------
Perhaps add a small doxygen comment to the class?


================
Comment at: include/llvm/CodeGen/AccelTable.h:255
+
+#ifndef NDEBUG
+  void print(raw_ostream &OS) const override;
----------------
Perhaps use `LLVM_DUMP_METHOD` instead?


================
Comment at: include/llvm/MC/MCObjectFileInfo.h:258
 
   // DWARF5 Experimental Debug Info Sections
+  MCSection *getDwarfDebugNamesSection() const {
----------------
JDevlieghere wrote:
> Obsolete now we have the "real" thing? :-) 
That comment seems to be out-of-date now.


================
Comment at: lib/CodeGen/AsmPrinter/AccelTable.cpp:181
+
+class Dwarf5AccelTableEmitter : public AccelTableEmitter {
+  struct Header {
----------------
Small doxygen comment ?


================
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, "");
----------------
JDevlieghere wrote:
> 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?
What does the '0700' derive from?


================
Comment at: lib/CodeGen/AsmPrinter/AccelTable.cpp:358
+  AsmPrinter *Asm = Ctx.Asm;
+  Asm->OutStreamer->AddComment("Header: unit length");
+  Asm->EmitLabelDifference(Ctx.ContributionEnd, Ctx.ContributionStart,
----------------
Maybe this gets too long, but it might be nice to say "DWARF v5 Accel Header" instead of just "Header"


================
Comment at: lib/CodeGen/AsmPrinter/AccelTable.cpp:403
+    dwarf::Form Form;
+    if (CompUnits.size() <= 0x100)
+      Form = dwarf::DW_FORM_data1;
----------------
should this be factored out?


================
Comment at: lib/CodeGen/AsmPrinter/AccelTable.cpp:408
+    else {
+      assert(CompUnits.size() < 0x1000000ull && "Too many compilation units");
+      Form = dwarf::DW_FORM_data4;
----------------
This should probably be a hard error rather than an assertion. We don't want the backend to be exploitable by malformed input more than necessary :-)


================
Comment at: lib/CodeGen/AsmPrinter/AccelTable.cpp:469
+  switch (Form) {
+  case dwarf::DW_FORM_data1:
+    Size = 1;
----------------
Don't we have this somewhere else already and if not, should it be factored into a common utility function?


================
Comment at: lib/CodeGen/AsmPrinter/AccelTable.cpp:501
+    default:
+      llvm_unreachable("Unexpected index attribute!");
+    }
----------------
Again, if it is possible to craft input that triggers this unreachable it should be a real error instead.


================
Comment at: lib/CodeGen/AsmPrinter/DwarfDebug.h:195
 
+enum class AccelTableKind { Default, None, Apple, Dwarf };
+
----------------
Doxygen comment explaining what these enumerators mean?


================
Comment at: test/DebugInfo/Generic/debug-names-hash-collisions.ll:30
+; CHECK: Bucket 0
+; CHECK:     Hash: 0xF8CF70D
+; CHECK:     String: 0x{{[0-9a-f]*}} "_ZN4lldb7SBBlockaSERKS0_"
----------------
CHECK-NEXT?


Repository:
  rL LLVM

https://reviews.llvm.org/D43286