[PATCH] D22308: [pdb] Introduce an MsfBuilder class for laying out PDB files

[Zachary Turner via llvm-commits]

zturner added inline comments.

================
Comment at: include/llvm/DebugInfo/PDB/Raw/MsfBuilder.h:38
@@ +37,3 @@
+  Error addStream(uint32_t Size, ArrayRef<uint32_t> Blocks);
+  Error addStream(uint32_t Size);
+  Error setStreamSize(uint32_t Idx, uint32_t Size);
----------------
amccarth wrote:
> Some comments describing these members would be nice.  I'd expect something like addStream to return something like an index or handle to refer to that new stream.  What's the difference between the two versions (with and without Blocks)?
There's two use cases I had in mind.  One is you're building a PDB from scratch.  You don't care what blocks a stream lies on, you just want the builder to figure it out.  That's where you use the overload with no block list.  

The other use case is you're editing a PDB.  You load up one from disk, make some edits to it, and write it back out.  This second use case is how I'm doing the Pdb <-> Yaml round-tripping, but it will be useful down the line when we want to integrate this into the linker.  In this use case, you want to change the layout of the PDB as little as possible.  If a stream used to take up blocks 10, 11, and 12, it should still *try* to take up blocks 10, 11, and 12.  As long as you don't make any edits to the stream, that should still be possible.  But if you remove a record or add a record, the stream extents could change.  

When you call this overload, the builder does all of this.  If it can use exactly the same set of blocks, it will.  If you add data it will keep the blocks you specified but add some new ones.  If you remove data it will keep the first N blocks you specified, but free up the other ones.

================
Comment at: lib/DebugInfo/PDB/Raw/MsfBuilder.cpp:30
@@ +29,3 @@
+
+  // The super block and block map address are not counted
+  BlockSize = PageSize;
----------------
amccarth wrote:
> I think you meant "addresses" (plural).
I actually meant address singular.  The "block map address" is one 32-bit integer.  

================
Comment at: lib/DebugInfo/PDB/Raw/MsfBuilder.cpp:108
@@ +107,3 @@
+
+bool MsfBuilder::isPageFree(uint32_t Idx) const { return FreePages[Idx]; }
+
----------------
amccarth wrote:
> Are pages and blocks the same thing?  Maybe blocks are ranges of pages.  It seems `Idx` is used for both stream indexes and page/block indexes.
Yes

================
Comment at: lib/DebugInfo/PDB/Raw/MsfBuilder.cpp:143
@@ +142,3 @@
+  // stream to
+  // automatically.
+  uint32_t ReqBlocks = bytesToBlocks(Size, BlockSize);
----------------
amccarth wrote:
> I think this comment would be better in the header by the declaration of the method, as it explains the difference between the two `addStreams`, which users of the API care about.
> 
> The funny line break also makes me wonder if you left out some words.
The funny line breaks happen when you clang-format a multi-line comment where one line went past 80 characters.  I always miss a few when going over a CL after clang-formatting.


http://reviews.llvm.org/D22308