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

[Colden Cullen via Phabricator via llvm-commits]

colden added inline comments.


================
Comment at: docs/PDB/MsfFile.rst:25
+2. Free Block Map 2 (corresponds to ``SuperBlock::FreeBlockMapBlock`` 2)
+3. ``SuperBlock::BlockSize - 2`` blocks of data
+
----------------
zturner wrote:
> Shouldn't this be BlockSize - 3?  The pattern is Block 0, Block 1, Block 2, <BlockSize - 3 more blocks>
I don't think so. I may be wrong, but here was my thinking.

If the second FPM0 is at index 4097, that means that there are 4097 blocks before it. BUT, if the interval size is 4096, that means that there's 1 extra block in the first interval, which *I think* would have to be the super block.

I can probably word this better, so I'll work on it, but I think the pattern is Super Block, followed by 1+ 4096 block-intervals repeating


================
Comment at: docs/PDB/MsfFile.rst:35
++=============+=======================+==================+==================+==========+======+======+=============+=====+
+| Meaning     | :ref:`msf_superblock` | Free Block Map 1 | Free Block Map 2 | Data     | FBM1 | FBM2 | Data        | ... |
++-------------+-----------------------+------------------+------------------+----------+------+------+-------------+-----+
----------------
zturner wrote:
> Can we call it FPM instead of FBM?  We use the terms Page and Block sometimes interchangeably, but we never refer to anything other than FPM in the code.
Yeah for sure.

I'm glad you brought this up though, because the the code use is actually super confusing. Whenever we use the whole words, it's always FreeBlockMap (see SuperBlock struct), but whenever its an acronym, it's FPM. Should we just pick one and rename everything to that? I'd be happy to do that as a separate change, if we can agree on which one to go with. I personally prefer FBM, because everything else refers to them as "Blocks," not "Pages."

Maybe I'm just bitter because it took me about an hour to figure out what FPM stood for :)


================
Comment at: docs/PDB/MsfFile.rst:102-105
+Because each interval includes the FPMs associated with it, the first two bytes
+of each FPM refer to FPM1 and FPM2, and are by definition 1 (the FPMs can't be
+unused). The only exception to this rule is the first interval, where byte 0
+refers to the Superblock, bype 1 refers to FPM1, byte 2 refers to FPM2, and
----------------
zturner wrote:
> I think this sentence is a bit wrong.  The first two bytes of each FPM are not necessarily 1.  But the first *three bits* of each FPM are.  And not necessarily each FPM either, only the active one.  I would just say "There is one bit in the FPM for every block in the file, including the super block."  that should be clear enough that there are no exceptions.
So it seems like there's a couple issues here:
1. You are correct, I should have said "bits"
2. You are correct, this is only guaranteed of the active FPM.
3. 2 vs 3, This one I'm unsure about. The MS code does indeed mark the first 3 bits as "in-use,"[0] but thinking about this in conjunction with your first question, that doesn't really make sense. Because the first interval contains 4097 blocks, there *must* be one that isn't referenced by the FPM. My assumption would be it's the SuperBlock (how could it possibly be "free?").

Am I missing something obvious here? The more I think about this the more confusing it gets.

[0] The comment where you described the process of it marking 3 pages as allocated: https://reviews.llvm.org/D41734#967624


Repository:
  rL LLVM

https://reviews.llvm.org/D41825