[PDB] Improve error handling when writes fail

[rnk]

Summary

Handle PDB writing errors like any other error in LLD: emit an error and
continue. This allows the linker to print timing data and summary data
after linking, which can be helpful for finding PDB size problems. Also
report how large the file would have been.

Example output:

lld-link: error: Output data is larger than 4 GiB. File size would have been 6,937,108,480
lld-link: error: failed to write PDB file ./chrome.dll.pdb

Summary

---

   33282 Input OBJ files (expanded from all cmd-line inputs)
       4 PDB type server dependencies
       0 Precomp OBJ dependencies
33396931 Input type records

... snip ...

Input File Reading:           59756 ms ( 45.5%)
GC:                            7500 ms (  5.7%)
ICF:                           3336 ms (  2.5%)
Code Layout:                   6329 ms (  4.8%)
PDB Emission (Cumulative):    46192 ms ( 35.2%)
  Add Objects:                27609 ms ( 21.0%)
    Type Merging:             16740 ms ( 12.8%)
    Symbol Merging:           10761 ms (  8.2%)
  Publics Stream Layout:       9383 ms (  7.1%)
  TPI Stream Layout:           1678 ms (  1.3%)
  Commit to Disk:              3461 ms (  2.6%)

---

Total Link Time: 131244 ms (100.0%)

[aganea]

Strangely, a few weeks ago I had a similair issue while trying to link an executable with MSVC-built .OBJ files. My PDB with lld-link.exe would have been 7.7 GB, but when linking the same .OBJ files with link.exe the PDB is 2.7 GB. Since we moved off MSVC, I haven't investigated in detail why. The problem doesn't occur on the same target when using Clang. I'm curious, is that your case?

In any case, LTGM.

[rnk]

In D102713#2766856, @aganea wrote:

Strangely, a few weeks ago I had a similair issue while trying to link an executable with MSVC-built .OBJ files. My PDB with lld-link.exe would have been 7.7 GB, but when linking the same .OBJ files with link.exe the PDB is 2.7 GB. Since we moved off MSVC, I haven't investigated in detail why. The problem doesn't occur on the same target when using Clang. I'm curious, is that your case?

In any case, LTGM.

Yes, our PDBs end up getting too big when we use either coverage (https://crbug.com/1159468) or static O0 builds (https://crbug.com/1179085). I think the common denominator between those two build configs is that they do not delete trivial inline functions, so you end up with standalone definitions of every little helper like std::unique_ptr<>::get(), and that somehow adds up to more than if those things were inlined everywhere.

Thanks!

[aganea]

We had similar OOM issues on the bots, as mentionned in the two bug reports. Regarding https://bugs.chromium.org/p/chromium/issues/detail?id=1179085, if you're using /DEBUG:GHASH the following patch might fix it? (but still generates > 4 GB .PDBs, which is probably a merging bug)

diff --git a/lld/COFF/DebugTypes.cpp b/lld/COFF/DebugTypes.cpp
index fedcb054540f..8bd8723f4bf2 100644
--- a/lld/COFF/DebugTypes.cpp
+++ b/lld/COFF/DebugTypes.cpp
@@ -1064,7 +1064,8 @@ void TypeMerger::mergeTypesWithGHash() {
 
   // Cap the table size so that we can use 32-bit cell indices. Type indices are
   // also 32-bit, so this is an inherent PDB file format limit anyway.
-  tableSize = std::min(size_t(INT32_MAX), tableSize);
+  tableSize =
+      std::min(size_t(INT32_MAX) - TypeIndex::FirstNonSimpleIndex, tableSize);
   ghashState.table.init(static_cast<uint32_t>(tableSize));
 
   // Insert ghashes in parallel. During concurrent insertion, we cannot observe

The issue was that source->indexMapStorage[i] = TypeIndex::fromArrayIndex(cellIdx); would store invalid offsets since the high bit (DecoratedItemIdMask) is dismissed when doing TypeIndex::toArrayIndex().

[rnk]

In D102713#2767094, @aganea wrote:

We had similar OOM issues on the bots, as mentionned in the two bug reports. Regarding https://bugs.chromium.org/p/chromium/issues/detail?id=1179085, if you're using /DEBUG:GHASH the following patch might fix it? (but still generates > 4 GB .PDBs, which is probably a merging bug)

diff --git a/lld/COFF/DebugTypes.cpp b/lld/COFF/DebugTypes.cpp
index fedcb054540f..8bd8723f4bf2 100644
--- a/lld/COFF/DebugTypes.cpp
+++ b/lld/COFF/DebugTypes.cpp
@@ -1064,7 +1064,8 @@ void TypeMerger::mergeTypesWithGHash() {
 
   // Cap the table size so that we can use 32-bit cell indices. Type indices are
   // also 32-bit, so this is an inherent PDB file format limit anyway.
-  tableSize = std::min(size_t(INT32_MAX), tableSize);
+  tableSize =
+      std::min(size_t(INT32_MAX) - TypeIndex::FirstNonSimpleIndex, tableSize);
   ghashState.table.init(static_cast<uint32_t>(tableSize));
 
   // Insert ghashes in parallel. During concurrent insertion, we cannot observe

The issue was that source->indexMapStorage[i] = TypeIndex::fromArrayIndex(cellIdx); would store invalid offsets since the high bit (DecoratedItemIdMask) is dismissed when doing TypeIndex::toArrayIndex().

I see, I guess we do need that patch.

I thought I was using ghashing, but it turned out I wasn't. I switched to ghash, and things got faster, even without ghashes in object files. This has given me motivation to make ghashing the default type merging strategy. I'll have to follow up on that.

lld/COFF/PDB.cpp

void PDBLinker::addSections(ArrayRef<OutputSection *> outputSections,
   dbiBuilder.createSectionMap(sections);
 
   // Add COFF section header stream.
   exitOnErr(
       dbiBuilder.addDbgStream(pdb::DbgHeaderType::SectionHdr, sectionTable));
 }
 
 void PDBLinker::commit(codeview::GUID *guid) {
-  ExitOnError exitOnErr((config->pdbPath + ": ").str());
-  // Write to a file.
-  exitOnErr(builder.commit(config->pdbPath, guid));
+  // Print an error and continue if PDB writing fails. This is done mainly so
+  // the user can see the output of /time and /summary, which is very helpful
+  // when trying to figure out why a PDB file is too large.
+  if (Error e = builder.commit(config->pdbPath, guid)) {
+    checkError(std::move(e));
+    error("failed to write PDB file " + Twine(config->pdbPath));
+  }
 }
 
 static uint32_t getSecrelReloc() {
   switch (config->machine) {
   case AMD64:
     return COFF::IMAGE_REL_AMD64_SECREL;
   case I386:
     return COFF::IMAGE_REL_I386_SECREL;

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

 
 #include <string>
 
 namespace llvm {
 namespace msf {
 enum class msf_error_code {
   unspecified = 1,
   insufficient_buffer,
+  size_overflow,
   not_writable,
   no_stream,
   invalid_format,
   block_in_use
 };
 } // namespace msf
 } // namespace llvm
 

llvm/lib/DebugInfo/MSF/MSFBuilder.cpp

 #include "llvm/ADT/ArrayRef.h"
 #include "llvm/DebugInfo/MSF/MSFError.h"
 #include "llvm/DebugInfo/MSF/MappedBlockStream.h"
 #include "llvm/Support/BinaryByteStream.h"
 #include "llvm/Support/BinaryStreamWriter.h"
 #include "llvm/Support/Endian.h"
 #include "llvm/Support/Error.h"
 #include "llvm/Support/FileOutputBuffer.h"
+#include "llvm/Support/FormatVariadic.h"
 #include <algorithm>
 #include <cassert>
 #include <cstdint>
 #include <cstring>
 #include <memory>
 #include <utility>
 #include <vector>
 
Expected<FileBufferByteStream> MSFBuilder::commit(StringRef Path,
 
   uint64_t FileSize = uint64_t(Layout.SB->BlockSize) * Layout.SB->NumBlocks;
   if (FileSize > UINT32_MAX) {
     // FIXME: Changing the BinaryStream classes to use 64-bit numbers lets
     // us create PDBs larger than 4 GiB successfully. The file format is
     // block-based and as long as each stream is small enough, PDBs larger than
     // 4 GiB might work. Check if tools can handle these large PDBs, and if so
     // add support for writing them.
-    return make_error<MSFError>(msf_error_code::invalid_format,
-                                "Output larger than 4 GiB");
+    return make_error<MSFError>(
+        msf_error_code::size_overflow,
+        formatv("File size would have been {0,1:N}", FileSize));
   }
 
   auto OutFileOrError = FileOutputBuffer::create(Path, FileSize);
   if (auto EC = OutFileOrError.takeError())
     return std::move(EC);
 
   FileBufferByteStream Buffer(std::move(*OutFileOrError),
                               llvm::support::little);

llvm/lib/DebugInfo/MSF/MSFError.cpp

public:
   const char *name() const noexcept override { return "llvm.msf"; }
   std::string message(int Condition) const override {
     switch (static_cast<msf_error_code>(Condition)) {
     case msf_error_code::unspecified:
       return "An unknown error has occurred.";
     case msf_error_code::insufficient_buffer:
       return "The buffer is not large enough to read the requested number of "
              "bytes.";
+    case msf_error_code::size_overflow:
+      return "Output data is larger than 4 GiB.";
     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.";