[PATCH] D34257: [PDB] Start emitting source file and line information
Rui Ueyama via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Thu Jun 15 16:33:27 PDT 2017
ruiu added inline comments.
================
Comment at: lld/COFF/Chunks.h:67-68
void setRVA(uint64_t V) { RVA = V; }
void setOutputSectionOff(uint64_t V) { OutputSectionOff = V; }
+ uint64_t getOutputSectionOff() const { return OutputSectionOff; }
----------------
You can just make the member variable public because the functions doesn't hide anything now.
================
Comment at: lld/COFF/PDB.cpp:139
+
+ virtual Error visitLines(DebugLinesSubsectionRef &Lines,
+ const StringsAndChecksumsRef &State);
----------------
Are these virtual functions actually `override`?
================
Comment at: lld/COFF/PDB.cpp:186
+
+Error DebugLinkerVisitor::visitLines(DebugLinesSubsectionRef &Lines,
+ const StringsAndChecksumsRef &State) {
----------------
It'd be nice to add a function comment.
================
Comment at: lld/COFF/PDB.cpp:195
+ NewLines->setFlags(
+ static_cast<LineFlags>(static_cast<uint16_t>(Lines.header()->Flags)));
+ NewLines->setCodeSize(Lines.header()->CodeSize);
----------------
Do you actually need to do static_cast twice?
================
Comment at: lld/COFF/PDB.cpp:256
- // FIXME: Walk the .debug$S sections and add them. Do things like recording
- // source files.
-
- ArrayRef<uint8_t> Data = getDebugSection(File, ".debug$T");
- if (Data.empty())
+ // Before we can process symbol substreams from .debug$S, we need to process
+ // type information, file checksums, and the string table. Add type info to
----------------
Can you factor out the new code (or part of it) as a new function so that this function doesn't become too long?
================
Comment at: lld/COFF/PDB.cpp:268-269
+ // Do *NOT* check DebugChunk->isLive(). Linker GC and comdat associate
+ if (!DebugChunk->isCOMDAT() &&
+ DebugChunk->getSectionName() == ".debug$S") {
+ if (MainDebugSChunk) {
----------------
We usually reverse the condition and do `continue`.
================
Comment at: lld/COFF/PDB.cpp:321
+ // rather than having it re-serialize all of the subsections.
+ auto Buffer = std::make_unique<uint8_t[]>(DebugChunk->getSize());
+ assert(DebugChunk->getOutputSectionOff() == 0 &&
----------------
It is probably more straightforward to define Buffer as `std::vector<uint8_t> Buffer(DebugChunk->getSize())`.
https://reviews.llvm.org/D34257
More information about the llvm-commits
mailing list