[PATCH] D43286: [CodeGen] Generate DWARF v5 Accelerator Tables
Adrian Prantl via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Wed Mar 7 08:29:57 PST 2018
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
More information about the llvm-commits
mailing list