[PATCH] D36235: [pdb/lld] Write a valid FPM

[Zachary Turner via Phabricator via llvm-commits]

zturner added inline comments.


================
Comment at: llvm/include/llvm/DebugInfo/MSF/MSFCommon.h:115
+    // One interval every BlockSize blocks.
+    return alignTo(L.SB->NumBlocks, L.SB->BlockSize) / L.SB->BlockSize;
+  }
----------------
rnk wrote:
> Can you refactor this `alignTo(X, Y) / Y` pattern into a helper like `divideRoundUp` or `divideCeil`? The usual way to do that is `(X + Y - 1) / Y`. `alignTo` does stuff we don't need.
Yea I can add that.


================
Comment at: llvm/lib/DebugInfo/PDB/Native/PDBFileBuilder.cpp:158
 
+void PDBFileBuilder::commitFpm(WritableBinaryStream &MsfBuffer,
+                               const MSFLayout &Layout) {
----------------
rnk wrote:
> Why not make this part of MSFBuilder? This doesn't use any PDBFileBuilder state, just an MSFLayout.
`MSFBuilder` doesn't actually write any bytes, it just returns some bookeeping information (specifically, an `MSFLayout` structure) that describe the the MSF.  It might be worth doing some refactoring and changing `MSFBuilder::build()` to `MSFBuilder::commit()` and write the super block, stream directory, and other MSF specific stuff, but I'd rather do that separately.  `PDBFileBuilder::commit()` already writes all of that other MSF specific stuff anyway, so we might as well be consistent.


================
Comment at: llvm/lib/DebugInfo/PDB/Native/PDBFileBuilder.cpp:167-169
+  FpmStream =
+      WritableMappedBlockStream::createFpmStream(Layout, MsfBuffer, Allocator);
+  FpmWriter = BinaryStreamWriter(*FpmStream);
----------------
rnk wrote:
> Why do you need to recreate the stream and writer?
The stream doesn't have methods to update its length, so we need a new stream, and the writer doesn't have a method to set the stream to a new stream.


https://reviews.llvm.org/D36235