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

[Pavel Labath via Phabricator via llvm-commits]

labath marked an inline comment as done.
labath added a comment.

I responded as many comments as I could to get another roundtrip before tomorrow morning. I will respond to the rest tomorrow.



================
Comment at: include/llvm/CodeGen/AccelTable.h:255
+
+#ifndef NDEBUG
+  void print(raw_ostream &OS) const override;
----------------
aprantl wrote:
> Perhaps use `LLVM_DUMP_METHOD` instead?
You mean, instead of #ifndef NDEBUG ?
I can, but this method is declared abstract this way in the base class, so I'd need to change the whole hierarchy.


================
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, "");
----------------
aprantl wrote:
> 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?
If we always want to put the current version here, then that would definitely be a good thing to do. However, it wasn't clear to me whether we want to do that, or only bump the version when we make a substantial change in the information we generate (I hardcoded it because I assumed the latter). However, maybe we shouldn't even follow the llvm versions, but put some completely independent strings here ? I'm also starting to think the format should be more human-readable (e.g. "LLVM 7.0.0" instead of "LLVM0700").

Basically, I have no idea what would be a good thing to put here, because I don't know how (or if) will we end up using it. I am open to any suggestions...


================
Comment at: lib/CodeGen/AsmPrinter/AccelTable.cpp:233
+                        const DwarfDebug &DD,
+                        ArrayRef<std::unique_ptr<DwarfCompileUnit>> CompUnits);
+
----------------
JDevlieghere wrote:
> It seems odd that we sink the DwarfCompileUnits, why's that? 
What do you mean by "sink" ? I am not taking the ownership of the compile units.. I couldn't even do that as the (non-mutable) ArrayRef prevents me from modifying the unique_ptrs in the list.


================
Comment at: lib/CodeGen/AsmPrinter/AccelTable.cpp:358
+  AsmPrinter *Asm = Ctx.Asm;
+  Asm->OutStreamer->AddComment("Header: unit length");
+  Asm->EmitLabelDifference(Ctx.ContributionEnd, Ctx.ContributionStart,
----------------
aprantl wrote:
> Maybe this gets too long, but it might be nice to say "DWARF v5 Accel Header" instead of just "Header"
Yeah, I think that would be too long. When you look at the generated assembly, it should be fairly obvious that you are in the .debug_names section.


================
Comment at: lib/CodeGen/AsmPrinter/AccelTable.cpp:408
+    else {
+      assert(CompUnits.size() < 0x1000000ull && "Too many compilation units");
+      Form = dwarf::DW_FORM_data4;
----------------
aprantl wrote:
> 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 :-)
How do I do a "hard error"? (Also, technically, I don't think this should lead to UB, only to truncation of certain values (and generation of broken tables).)


================
Comment at: lib/CodeGen/AsmPrinter/AccelTable.cpp:469
+  switch (Form) {
+  case dwarf::DW_FORM_data1:
+    Size = 1;
----------------
aprantl wrote:
> Don't we have this somewhere else already and if not, should it be factored into a common utility function?
We have something in the DebugInfo library, but I haven't seen anything in CodeGen. I'll have another look tomorrow...


================
Comment at: lib/CodeGen/AsmPrinter/AccelTable.cpp:501
+    default:
+      llvm_unreachable("Unexpected index attribute!");
+    }
----------------
aprantl wrote:
> Again, if it is possible to craft input that triggers this unreachable it should be a real error instead.
This is not triggerable by user input. It just means somebody added a new entry to `getUniformAttributes` and forgot to update this function.


================
Comment at: lib/CodeGen/AsmPrinter/DwarfDebug.cpp:297
+  } else
+    AccelTableKind = DwarfAccelTables;
 
----------------
JDevlieghere wrote:
> This doesn't seem right, if you specify apple explicitly you'll end up with DWARF? Shouldn't we just omit the else?
You'll end up with whatever was you specified on the command line. DwarfAccelTables is the cl::opt option. Maybe I should rename it to something else to make it more clear.


Repository:
  rL LLVM

https://reviews.llvm.org/D43286