[PATCH] D94267: [PDB] Defer relocating .debug$S until commit time and parallelize it

[Reid Kleckner via Phabricator via llvm-commits]

rnk marked an inline comment as done.
rnk added a comment.

Thanks, I'll push this after a few more tests.



================
Comment at: lld/COFF/PDB.cpp:218
+  /// Includes record realignment.
+  uint32_t moduleStreamSize = 4;
+
----------------
aganea wrote:
> It's a bit strange that '4' means to do nothing in `finish()` but I understand that it makes the logic in `analyzeSymbolSubsection()` less complicated.
I went ahead and gave this a named constant so it's a bit more readable.


================
Comment at: lld/COFF/PDB.cpp:460
   case SymbolKind::S_LPROC32:
+  case SymbolKind::S_GPROC32_ID:
+  case SymbolKind::S_LPROC32_ID:
----------------
aganea wrote:
> Was this a divergence from MSVC link.exe, or was it handled somewhere else before your patch?
This is a necessary change because now this code runs before `translateIdSymbols` runs, so it has to include the `*_ID` procedure variants


================
Comment at: lld/COFF/PDB.cpp:1510
+    // Write ptrEnd for the S_THUNK32.
+    ulittle32_t *pEnd = reinterpret_cast<ulittle32_t *>(
+        const_cast<uint8_t *>(&newSym.data()[8]));
----------------
aganea wrote:
> I can't say I like poking plain memory without going through structured data, it makes the code less reader-friendly. It's a pity we don't have definitions for serialized structures :-( Nothing you can do now I guess.
I'll factor out the reinterpret_cast from the scope stack management code above so that it can be shared. That hopefully makes it a bit more readable.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D94267/new/

https://reviews.llvm.org/D94267