[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.

So here's the relevant code:

https://github.com/Microsoft/microsoft-pdb/blob/master/PDB/msf/msf.cpp#L2508-L2516

  CB cbpg = cbPg();    // (1)
  
  // Calc number of bytes needed to represent FPM, aligned to FPM::native_word sizes
  //
  CB cbFpm = (pnMac() + CHAR_BIT - 1) / CHAR_BIT;   // (2)
  cbFpm = sizeof(FPM::native_word) * ((cbFpm + sizeof(FPM::native_word) - 1) / sizeof(FPM::native_word));   // (3)
  
  // Calc number of pages needed to represent that number of bytes
  UPN cpnFpm = (cbFpm + cbpg - 1) / cbpg;  // (4)

The way I read this code:

At (1), `cbpg = 4096`, the number of bytes in a page (always hardcoded to 4096 since we only support what they refer to as "BigMsf")
At (2), `cbFpm = ceil(# of pages / 8)`  Conceptually, this is since each byte refers to 8 pages, so this is really "# of bytes of fpm data needed given the number of pages we need to write"
At (3), 'cbFpm = # of bytes of fpm data needed rounded up to a multiple of pointer size`
At (4), `cpnFpm = # of bytes of fpm data needed rounded up to a multiple of pointer size and then rounded up to a multiple of block size`

rounding up to a multiple of pointer size and *then* rounding up to a multiple of block size seems to be redundant, so I interpret this as just:

- # of bytes of fpm data needed rounded up to a multiple of block size

But the question I kind of skipped over is "What is # of pages?  Does it include the FPM and super block."  I believe it does, For example, I ran `llvm-pdbutil` against `clang.pdb` which was generated by MSVC, and I get this:

  D:\src\llvmbuild\cl\Debug\x64>bin\llvm-pdbutil.exe dump --summary bin\clang.pdb
  
  
                            Summary
  ============================================================
    Block Size: 4096
    Number of blocks: 167547
    Number of streams: 1442
    Signature: 1514918418
    Age: 3
    GUID: {D1F84556-D25E-7045-BF75-5D4A1D9274AB}
    Features: 0x1
    Has Debug Info: true
    Has Types: true
    Has IDs: true
    Has Globals: true
    Has Publics: true
    Is incrementally linked: true
    Has conflicting types: false
    Is stripped: false
  
  D:\src\llvmbuild\cl\Debug\x64>dir bin\clang.pdb
   Volume in drive D is New Volume
   Volume Serial Number is 2EC5-6F01
  
   Directory of D:\src\llvmbuild\cl\Debug\x64\bin
  
  01/03/2018  10:57 AM       686,272,512 clang.pdb
                 1 File(s)    686,272,512 bytes
                 0 Dir(s)   4,718,669,824 bytes free

We have to assume this PDB is authoritative since it was generated by MSVC, so whatever it does it correct by definition.

686272512 / 4096 = 167547, which matches exactly the number of blocks in the MSF header.

So based on this analysis, it looks like:

1. The "# of pages" field in the MSF header definitely includes the first 3 reserved pages.
2. The calculation currently used matches the calculation in the reference implementation.

So either my analysis is wrong (very likely, it's always hard analyzing this kind of dense code when we can't step through it in a debugger or even run the real code), or the bug lies elsewhere (perhaps in the write function?).

Interested to hear your thoughts though since you've already spent much more time thinking about this then I have :)


Repository:
  rL LLVM

https://reviews.llvm.org/D41734