[PDB] Error on too large stream directories

[hans]

Summary

We hit this in Chromium builds where the PDB file was just under 4GB, but the stream directory was actually too large to be correctly represented.

llvm-pdbutil would error about this in llvm::msf::validateSuperBlock, but lld should not write such PDB files in the first place.

[aganea]

In addition to these changes, do you think you can add a more descriptive error at a higher level in LLD, maybe in PDBLinker::commit(). It is not clear from the message what the corrective measure is. I suppose adding /pdbpagesize: with a higher block size would fix the problem, but this isn't obvious to everyone.

[hans]

In D144385#4139641, @aganea wrote:

In addition to these changes, do you think you can add a more descriptive error at a higher level in LLD, maybe in PDBLinker::commit(). It is not clear from the message what the corrective measure is. I suppose adding /pdbpagesize: with a higher block size would fix the problem, but this isn't obvious to everyone.

I've updated the patch. Please take another look.

[aganea]

Thanks for the changes!

I'm seeing that Zach added quite a few tests for MSF, not sure if you had the time/if it's worth crafting a test for this.

Otherwise, looks good.

lld/COFF/PDB.cpp

1695 ↗

(On Diff #499504)

Just a nit: How does the errors look like in stderr? I wonder if we shouldn't use joinErrors to have both errors contiguous on a single line?

llvm/lib/DebugInfo/MSF/MSFError.cpp

47

s/Stream/stream/ to be consistent with the other messages.

[hans]

In D144385#4145277, @aganea wrote:

I'm seeing that Zach added quite a few tests for MSF, not sure if you had the time/if it's worth crafting a test for this.

Thanks! I had a look, and it doesn't seem that those hit MSFBuilder::commit, and creating one is probably not worth the effort for this check.

lld/COFF/PDB.cpp

1695 ↗

(On Diff #499504)

It currently looks like this:

lld-link: error: PDB Stream directory too large. The directory block map (4148 bytes) doesn't fit in a block (4096 bytes)
lld-link: error: try using a larger /pdbpagesize:
lld-link: error: failed to write PDB file ./chrome.dll.pdb

I see your point about joining them, but I think with the separate line it's easier to read.

llvm/include/llvm/DebugInfo/MSF/MSFError.h

enum class msf_error_code {
   insufficient_buffer,
   size_overflow_4096,
   size_overflow_8192,
   size_overflow_16384,
   size_overflow_32768,
   not_writable,
   no_stream,
   invalid_format,
-  block_in_use
+  block_in_use,
+  stream_directory_overflow
 };
 } // namespace msf
 } // namespace llvm
 
 namespace std {
 template <>
 struct is_error_code_enum<llvm::msf::msf_error_code> : std::true_type {};
 } // namespace std

llvm/lib/DebugInfo/MSF/MSFBuilder.cpp

if (FileSize > getMaxFileSizeFromBlockSize(Layout.SB->BlockSize)) {
     }(Layout.SB->BlockSize);
 
     return make_error<MSFError>(
         error_code,
         formatv("File size {0,1:N} too large for current PDB page size {1}",
                 FileSize, Layout.SB->BlockSize));
   }
 
+  uint64_t NumDirectoryBlocks =
+      bytesToBlocks(Layout.SB->NumDirectoryBytes, Layout.SB->BlockSize);
+  uint64_t DirectoryBlockMapSize =
+      NumDirectoryBlocks * sizeof(support::ulittle32_t);
+  if (DirectoryBlockMapSize > Layout.SB->BlockSize) {
+    return make_error<MSFError>(msf_error_code::stream_directory_overflow,
+                                formatv("The directory block map ({0} bytes) "
+                                        "doesn't fit in a block ({1} bytes)",
+                                        DirectoryBlockMapSize,
+                                        Layout.SB->BlockSize));
+  }
+
   auto OutFileOrError = FileOutputBuffer::create(Path, FileSize);
   if (auto EC = OutFileOrError.takeError())
     return std::move(EC);
 
   FileBufferByteStream Buffer(std::move(*OutFileOrError),
                               llvm::support::little);
   BinaryStreamWriter Writer(Buffer);
 

llvm/lib/DebugInfo/MSF/MSFError.cpp

std::string message(int Condition) const override {
     case msf_error_code::not_writable:
       return "The specified stream is not writable.";
     case msf_error_code::no_stream:
       return "The specified stream does not exist.";
     case msf_error_code::invalid_format:
       return "The data is in an unexpected format.";
     case msf_error_code::block_in_use:
       return "The block is already in use.";
+    case msf_error_code::stream_directory_overflow:
+      return "PDB Stream directory too large.";
     }
     llvm_unreachable("Unrecognized msf_error_code");
   }
 };
 } // namespace
 
 const std::error_category &llvm::msf::MSFErrCategory() {
   static MSFErrorCategory MSFCategory;
   return MSFCategory;
 }
 
 char MSFError::ID;