[PATCH] D33807: [CodeView] Allow Debug Subsections to be read/written in any order

[Bob Haarman via Phabricator via llvm-commits]

inglorion requested changes to this revision.
inglorion added a comment.
This revision now requires changes to proceed.

Happy you're doing this, but see inline comments for a few requested changes.



================
Comment at: llvm/lib/DebugInfo/CodeView/DebugStringTableSubsection.cpp:79
 
 uint32_t DebugStringTableSubsection::getStringId(StringRef S) const {
+  assert(exists(S));
----------------
This will call StringToId.find(S) twice when asserts are enabled. Since that returns an iterator, can we instead do

  auto It = StringToId.find(S);
  assert(It != StringToId.end() && "getStringId called with string that does not exist in the string table");
  return It->second;

and save the second lookup?


================
Comment at: llvm/lib/DebugInfo/PDB/Native/DbiModuleDescriptorBuilder.cpp:42
+  uint32_t Size = sizeof(uint32_t);   // Signature
+  Size += alignTo(SymbolByteSize, 4); // Symbol Data
+  Size += 0;                          // TODO: Layout.C11Bytes
----------------
Do we need this? I thought we made it so that SymbolByteSize is always a multiple of 4 when writing PDBs. Also, is this related to the other changes here or a holdover from your earlier alignment fixes?


================
Comment at: llvm/lib/DebugInfo/PDB/Native/ModuleDebugStream.cpp:93
+ModuleDebugStreamRef::findChecksumsSubsection() const {
+  for (const auto &SS : subsections()) {
+    if (SS.kind() != DebugSubsectionKind::FileChecksums)
----------------
If I'm reading the code in ModuleDebugStreamRef::reload() correctly, we read the subsections before it makes sense to get to this code. Given that, can we store which one is the FileChecksums subsection at reload() time so that we don't have to iterate over all the subsections to find it?


https://reviews.llvm.org/D33807