[PATCH] D41734: [DebugInfo][PDB] Fix too many FPM blocks being written in some cases

[Zachary Turner via Phabricator via llvm-commits]

zturner added a comment.

In https://reviews.llvm.org/D41734#967677, @colden wrote:

> Yeah, that's the callstack I'm seeing. And I'm all for as many asserts as we can add to guarantee safety.
>
> Theoretically it should only add FPM blocks to the end when we've spilled over from the previous FPM's range, right? So unless we remove blocks from the end, there should never just be an FPM at the end.
>
> Maybe I'm misunderstanding, but are new blocks only ever added through getFpmStreamLayout or MSFBuilder::allocateBlocks? If that's the case, theoretically as long as those handle adding 2 FPMs correctly on overflow, we should be in good shape. Or do you think we got an FPM at the end from MSVC?
>
> If it's an MSVC thing, we should be able to fix it by just lopping the last FPMs off at load time. If they're the last blocks in a file, they can't possibly reference anything, and there's not much point in keeping them in memory.


It looks like in the case that causes the crash, which I reduced down to just using the unit test with `NumBlocks = 4097`, that means we will have blocks [0, 4096].  The two subsequent FPM blocks would have indices 4097 and 4098, and neither of those is allocated in the file.

So upon further thought I don't think the problem is that we're allocating one FPM block but not the other, but rather that we're not allocating *either* of them.  The layout is basically:

  // The FPM is going to look like this:
  // |  0  |   1   |   2   |  ...  ┊  4096  |  4097  |  4098  | ... | 
  // | SB  |  FPM0 | FPM1  | Data  ┊  Data  |  FPM0  |  FPM1  | ... |

Here I'm using a ┊ character to indicate the boundary for the FPM block interval period.

So the error happens because we're computing `divideCeil(NumBlocks, BlockSize)`.  This will report that we need an FPM interval for the second period, even though 4096 is the highest block number.  I think the fix is a more accurate computation here, very similar to what you started with actually :)

I have a patch, I'll upload it separately and Cc you.


Repository:
  rL LLVM

https://reviews.llvm.org/D41734