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

[Rui Ueyama via Phabricator via llvm-commits]

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