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

[Pavel Labath via Phabricator via llvm-commits]

labath marked 5 inline comments as done.
labath added a comment.

The way I read this review, there are no major issues with this patch. There are a couple of minor remarks, for which I am waiting for more feedback to see how to go about resolving them. I've added fresh replies to all comments that I think are still unresolved, so you can see what I mean.

Can you please take another look and let me know how to move this forward?



================
Comment at: include/llvm/CodeGen/AccelTable.h:255
+
+#ifndef NDEBUG
+  void print(raw_ostream &OS) const override;
----------------
labath wrote:
> 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.
If we want to go that way, I can swap ifndef NDEBUG for LLVM_DUMP_METHOD for the whole hierarchy in a separate patch.


================
Comment at: lib/CodeGen/AsmPrinter/AccelTable.cpp:408
+    else {
+      assert(CompUnits.size() < 0x1000000ull && "Too many compilation units");
+      Form = dwarf::DW_FORM_data4;
----------------
labath wrote:
> labath wrote:
> > 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).)
> I've replaced this with a call to abort(). Does that count as a hard error?
> BTW, the reason why we cannot describe more than this many CUs is because of the CompUnitCount field, which is 32-bit (even for DWARF64). Right now we don't generate DWARF64, and for DWARF32 with this many compilation units, I would expect things to break way before we reach this part.
As a consequence of using the DIEInteger class, there are no asserts here anymore (it will just automatically emit this field as DW_FORM_data8). However, someone does manage (which I doubt) to create 2^32 compilation units then we will generate broken tables, as the CompUnitCount field will be truncated. I don't think it's necessary, but it may still be interesting to add a call to abort() or similar in this case.


================
Comment at: lib/CodeGen/AsmPrinter/DwarfDebug.cpp:297
+  } else
+    AccelTableKind = DwarfAccelTables;
 
----------------
JDevlieghere wrote:
> labath wrote:
> > 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.
> Yeah, I think that'd be less confusing. 
I've renamed the command line option to "AccelTables". I'm not sure if that makes things better or worse...


================
Comment at: test/DebugInfo/Generic/debug-names-hash-collisions.ll:30
+; CHECK: Bucket 0
+; CHECK:     Hash: 0xF8CF70D
+; CHECK:     String: 0x{{[0-9a-f]*}} "_ZN4lldb7SBBlockaSERKS0_"
----------------
labath wrote:
> aprantl wrote:
> > CHECK-NEXT?
> I've added check-next where it's possible right now (the String: line immediately follows the Hash: line). I can't use it everywhere without adding expectations about the stuff I don't care about for this test.  I'm not sure where you were heading with this, but if the issue was to make sure the Buckets don't contain any extra names, i've added some CHECK-NOT lines to prevent that.
Is this CHECK-NEXT-ified enough?


Repository:
  rL LLVM

https://reviews.llvm.org/D43286