[PATCH] D41825: [Docs][PDB] Update MSF File documentation

[Colden Cullen via Phabricator via llvm-commits]

colden added inline comments.


================
Comment at: llvm/docs/PDB/MsfFile.rst:97
+series of blocks which contains a bit flag for every block in the file. The
+flag will be set to 1 if the block is in use, and 0 if the block is unused.
+
----------------
zturner wrote:
> This is backwards (yes it's confusing).  Since it's the //free// block map, a value of 1 indicates that the block is free, and a value of 0 indicates that the value is not free.
Aaah so close.


================
Comment at: llvm/docs/PDB/MsfFile.rst:107-110
+at intervals of BlockSize. Each block of the FPM refers to the blocks in the
+interval that FPM block is part of. For example, bit 0 represents the first
+data block, bit 1 represents FPM1, bit 2 represents FPM2, and the rest of the
+block refers to the data blocks following the FPMs.
----------------
zturner wrote:
> This is also not quite true.  The `k`'th bit in the `j`'th FPM block refers to the `4096*j + k`'th block in the file.  So, for example, consider blocks 1 and 4097.  These are the first and second blocks of FPM1, respectively.  To find out the allocation status of block 4097, you look at bit 1 of byte 512  (512*8 + 1 = 4097).  
> 
> There's actually a comment in the MSF reference implementation that indicates this was probably a design oversight, because this results in far more FPM blocks being reserved in a file than are actually necessary (by a factor of 8).  But since it's already that way and everything is relying on it, and files are out there in the wild using it, and it results in neglible file bloat, it just stays that way.
> 
> This is actually the whole point of the `IncludeUnusedFpmData` flag from my previous patch.  If you specify `true` for it, it will give you a stream consisting of every block that *could* be an FPM block (i.e. the index is of the form 4096*k + 1), but if you pass `false` it truncates the extraneous ones at the end that would otherwise refer to blocks that are not even in the file.
Oh fascinating. I was wondering about that `/ 8`. I'll see if I can figure out a good way to phrase this.


Repository:
  rL LLVM

https://reviews.llvm.org/D41825