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