[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:31-32
@@ +30,4 @@
+  explicit MsfBuilder(BumpPtrAllocator &Allocator);
+  Error initialize(uint32_t BlockSize, uint32_t MinBlockCount = 0,
+                   bool CanGrow = true);
+
----------------
majnemer wrote:
> zturner wrote:
> > majnemer wrote:
> > > What do `CanGrow` and `MinBlockCount` mean?
> > `MinBlockCount` means "assume I will have at least this many blocks in the PDB file".  The reason this is useful is because when you want to modify an existing PDB, you want to keep the layout as stable as possible.  So we will be calling `addStream` many times in succession and using the overload that takes a block list.  But if you do this without specifying a `MinBlockCount`, then it will think you are trying to reference blocks that are outside the bounds of the entire file.  For example, if you were to write this code:
> > 
> > ```
> > msf.initialize(4096);
> > msf.addStream(29083, {7, 8, 9, 12, 14, 15, 16, 17});
> > ```
> > 
> > `addStream` is going to complain that the file does not have space for that many blocks.  It could try to help you out and grow the file automatically, and just say "well you asked for block 17, i'll just make sure there's at least enough blocks to be able to fit 17", but this seems awfully inefficient primarily because in the use case where you'll be doing this (i.e. you are reading in an existing PDB file), the super block already tells you exactly how many blocks the file has.  
> > 
> > Even if we don't automatically grow the file here (and the current implementation doesn't), growing the file is still useful.  If you want to create a new PDB from scratch, then you will use the overload of `addStream()` that does not take the block list, and in this case every single call to `addStream()` necessitates a grow.  
> > 
> > Basically, `CanGrow` dictates what happens when the `FreeBlocks` bit vector has no entries.  If it is true, we will resize the bit vector and make new blocks.  If it is false, we return an error.  We could actually get by without it and just always use behave as if `CanGrow = true`, but it seems useful as a way to prevent programming errors in cases where you know you are not adding any records or doing anything which should cause the PDB to get larger.  I'm using `false` here for some unit tests, for example, because the test is intended to verify the internal block allocation behavior, and having it return an error if blocks are unexpectedly allocated is a useful diagnostic.
> Can some of this get added to the header?
Sure


https://reviews.llvm.org/D22308