[PATCH] D34257: [PDB] Start emitting source file and line information

[Reid Kleckner via Phabricator via llvm-commits]

rnk marked 4 inline comments as done.
rnk added inline comments.


================
Comment at: lld/COFF/PDB.cpp:129
+/// - Filechecksum table offsets
+class DebugLinkerVisitor : public DebugSubsectionVisitor {
+  ObjectFile *File;
----------------
zturner wrote:
> I know I wrote it and I feel bad finding all these reasons *not* to use it, but maybe we just shouldn't use the visitor here.  The only sections that we have to rewrite stuff from a `.debug$S` section are the symbols, string table, and checksums.  Just have a loop that iterates over each record and handle the sections we care about using internal knowledge of the records format to deserialize as little as possible (e.g. in file checksums, you only care about the first 4 bytes)
File checksums are actually tricky because you have to extract the size, so I think we should leave that alone, and just be clever about line tables.


================
Comment at: lld/COFF/PDB.cpp:139
+
+  virtual Error visitLines(DebugLinesSubsectionRef &Lines,
+                           const StringsAndChecksumsRef &State);
----------------
ruiu wrote:
> Are these virtual functions actually `override`?
Right, they were, but this is gone now. I think our clang warnings are not set correctly for this on Windows.


================
Comment at: lld/COFF/PDB.cpp:186
+
+Error DebugLinkerVisitor::visitLines(DebugLinesSubsectionRef &Lines,
+                                     const StringsAndChecksumsRef &State) {
----------------
ruiu wrote:
> It'd be nice to add a function comment.
This is all gone now.


================
Comment at: lld/COFF/PDB.cpp:195
+  NewLines->setFlags(
+      static_cast<LineFlags>(static_cast<uint16_t>(Lines.header()->Flags)));
+  NewLines->setCodeSize(Lines.header()->CodeSize);
----------------
ruiu wrote:
> Do you actually need to do static_cast twice?
Gone now. I think I printed this at one point and operator<< doesn't work on ulittle16_t, so I needed it.


================
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 &&
----------------
ruiu wrote:
> It is probably more straightforward to define Buffer as `std::vector<uint8_t> Buffer(DebugChunk->getSize())`.
If I implement Zach's suggestion, I will have to pass ownership or use a longer-lived BumpPtrAllocator (probably the linker's).


https://reviews.llvm.org/D34257