Fix some differences between LLD and MSVC generated PDBs

[zturner]

Summary

A couple of things were different about our generated PDBs.

1) We were outputting the wrong Version on the PDB Stream.
   The version we were setting was newer than what MSVC is setting.
   It's not clear what the implications are, but we change LLD
   to use PdbImplVC70, as MSVC does.
2) For the optional debug stream indices in the DBI Stream, we
   were outputting 0 to mean "the stream is not present".  MSVC
   outputs uint16_t(-1), which is the "correct" way to specify
   that a stream is not present.  So we fix that as well.
3) We were setting the PDB Stream signature to 0.  This is supposed
   to be the result of calling time(nullptr).  Although this leads
   to non-deterministic builds, a better way to solve that is by
   having a command line option explicitly for generating a
   reproducible build, and have the default behavior of lld-link
   match the default behavior of link.

To test this, I'm making use of the new and improved `pdb diff`
sub command.  To make it suitable for writing tests against, I had
to modify the diff subcommand slightly to print less verbose output.
Previously it would always print | <column> | <value1> | <value2> |
which is quite verbose, and the values are fragile.  All we really
want to know is "did we produce the same value as link?"  So I added
command line options to print a single character representing the
result status (different, identical, equivalent), and another to
hide the value display.  Note that just inspecting the diff output
used to write the test, you can see some things that are obviously
wrong.  That is just reflective of the fact that this is the state
of affairs today, not that we're asserting that this is "correct".
We can use this as a starting point to discover differences, fix
them, and update the test.

[rnk]

lld/COFF/PDB.cpp

344 ↗

(On Diff #105531)

Generally speaking we want our tool output to be deterministic. Clang has a /Brepro flag that controls the emission of timestamps in file headers. Can you add a FIXME here to add a flag for this?

lld/test/COFF/pdb-diff.test

10 ↗

(On Diff #105531)

Are you worried about the fragility of these CHECKs? FileCheck ignores whitespace differences, so it might not be so bad, but I'm a little concerned.

[rnk]

lld/COFF/PDB.cpp

344 ↗

(On Diff #105531)

Actually, ignore this. We already need to absolute-ize paths, so our output isn't really deterministic.

lld/test/COFF/pdb-diff.test

177 ↗

(On Diff #105531)

This isn't going to match on other people's systems.

[zturner]

(BTW, ignore in case you already noticed them, but I also have D35092 and D35039 if you don't mind taking a look at those as well)

lld/COFF/PDB.cpp

344 ↗

(On Diff #105531)

There are other places where we parts of LLD where we will need to by default produce non-reproducible builds as well (for example, the guid in a PDB needs to be unique). Probably one or two other examples that escape me at the moment. But yea, we should add a flag for this and use the flag in tests.

lld/test/COFF/pdb-diff.test

10 ↗

(On Diff #105531)

Doesn't it only ignore leading and trailing whitespace differences? We could add a "no table" mode that just outputs <Field>: <Result> but I think that can come later, right now it's just good to have minimal test coverage documenting the places we differ.

177 ↗

(On Diff #105531)

Yea my bad. I already fixed this to {{.*Inputs/pdb-diff.obj}} locally.

lld/trunk/COFF/PDB.cpp

void coff::createPDB(StringRef Path, SymbolTable *Symtab,
   // Create streams in MSF for predefined streams, namely
   // PDB, TPI, DBI and IPI.
   for (int I = 0; I < (int)pdb::kSpecialStreamCount; ++I)
     ExitOnErr(Builder.getMsfBuilder().addStream(0));
 
   // Add an Info stream.
   auto &InfoBuilder = Builder.getInfoBuilder();
   InfoBuilder.setAge(DI ? DI->PDB70.Age : 0);
+  InfoBuilder.setSignature(time(nullptr));
 
   llvm::SmallString<128> NativePath(Path.begin(), Path.end());
   llvm::sys::fs::make_absolute(NativePath);
   llvm::sys::path::native(NativePath);
 
   pdb::PDB_UniqueId uuid{};
   if (DI)
     memcpy(&uuid, &DI->PDB70.Signature, sizeof(uuid));
   InfoBuilder.setGuid(uuid);
   // Should be the current time, but set 0 for reproducibilty.
   InfoBuilder.setSignature(0);
   InfoBuilder.setVersion(pdb::PdbRaw_ImplVer::PdbImplVC70);
 
   // Add an empty DBI stream.
   pdb::DbiStreamBuilder &DbiBuilder = Builder.getDbiBuilder();
-  DbiBuilder.setVersionHeader(pdb::PdbDbiV110);
+  DbiBuilder.setVersionHeader(pdb::PdbDbiV70);
 
   // It's not entirely clear what this is, but the * Linker * module uses it.
   uint32_t PdbFilePathNI = DbiBuilder.addECName(NativePath);
 
   TypeTableBuilder TypeTable(BAlloc);
   TypeTableBuilder IDTable(BAlloc);
   addObjectsToPDB(Alloc, Symtab, Builder, TypeTable, IDTable);
 

lld/trunk/test/COFF/Inputs/pdb-diff.cpp

+// Build with cl:
+//    cl.exe /Z7 pdb-diff.cpp /link /debug /pdb:pdb-diff-cl.pdb
+//           /nodefaultlib /entry:main
+// Build with lld (after running the above cl command):
+//    lld-link.exe /debug /pdb:pdb-diff-lld.pdb /nodefaultlib
+//                 /entry:main pdb-diff.obj
+
+void *__purecall = 0;
+
+int main() { return 42; }

lld/trunk/test/COFF/pdb-diff.test

+This test verifies that we produce PDBs compatible with MSVC in various ways.
+We check in a cl-generated object file, PDB, and original source which serve
+as the "baseline" for us to measure against.  Then we link the same object
+file with LLD and compare the two PDBs.  Since the baseline object file and
+PDB are already checked in, we just run LLD on the object file.
+
+RUN: lld-link /debug /pdb:%T/pdb-diff-lld.pdb /nodefaultlib /entry:main %S/Inputs/pdb-diff.obj
+RUN: llvm-pdbutil diff -result -values=false %T/pdb-diff-lld.pdb %S/Inputs/pdb-diff-cl.pdb | FileCheck %s
+
+CHECK:        ----------------------
+CHECK-NEXT:   |  MSF Super Block   |
+CHECK-NEXT:   |----------------+---|
+CHECK-NEXT:   |           File |   |
+CHECK-NEXT:   |----------------+---|
+CHECK-NEXT:   |     Block Size | I |
+CHECK-NEXT:   |----------------+---|
+CHECK-NEXT:   |    Block Count |
+CHECK-NEXT:   |----------------+---|
+CHECK-NEXT:   |      Unknown 1 | I |
+CHECK-NEXT:   |----------------+---|
+CHECK-NEXT:   | Directory Size |
+CHECK-NEXT:   |----------------+---|
+CHECK-NEXT:   ------------------------------------
+CHECK-NEXT:   |         Stream Directory         |
+CHECK-NEXT:   |------------------------------+---|
+CHECK-NEXT:   |                         File |   |
+CHECK-NEXT:   |------------------------------+---|
+CHECK-NEXT:   |                 Stream Count | D |
+CHECK-NEXT:   |------------------------------+---|
+CHECK-NEXT:   |            Old MSF Directory | I |
+CHECK-NEXT:   |------------------------------+---|
+CHECK-NEXT:   |                   PDB Stream | I |
+CHECK-NEXT:   |------------------------------+---|
+CHECK-NEXT:   |                   TPI Stream | I |
+CHECK-NEXT:   |------------------------------+---|
+CHECK-NEXT:   |                   DBI Stream | I |
+CHECK-NEXT:   |------------------------------+---|
+CHECK-NEXT:   |                   IPI Stream | I |
+CHECK-NEXT:   |------------------------------+---|
+CHECK-NEXT:   |          Section Header Data | {{[EI]}} |
+CHECK-NEXT:   |------------------------------+---|
+CHECK-NEXT:   |        Named Stream "/names" | {{[EI]}} |
+CHECK-NEXT:   |------------------------------+---|
+CHECK-NEXT:   |     Named Stream "/LinkInfo" | {{[EI]}} |
+CHECK-NEXT:   |------------------------------+---|
+CHECK-NEXT:   | Named Stream "/src/heade..." | D |
+CHECK-NEXT:   |------------------------------+---|
+CHECK-NEXT:   | Module "{{.*}}pdb-diff.obj" | D |
+CHECK-NEXT:   |------------------------------+---|
+CHECK-NEXT:   |          Module "* Linker *" | {{[EI]}} |
+CHECK-NEXT:   |------------------------------+---|
+CHECK-NEXT:   |                     TPI Hash | {{[EI]}} |
+CHECK-NEXT:   |------------------------------+---|
+CHECK-NEXT:   |                     IPI Hash | {{[EI]}} |
+CHECK-NEXT:   |------------------------------+---|
+CHECK-NEXT:   |           Global Symbol Hash | D |
+CHECK-NEXT:   |------------------------------+---|
+CHECK-NEXT:   |           Public Symbol Hash | D |
+CHECK-NEXT:   |------------------------------+---|
+CHECK-NEXT:   |        Public Symbol Records | D |
+CHECK-NEXT:   |------------------------------+---|
+CHECK-NEXT:   | Module "{{.*}}pdb-diff.obj" | D |
+CHECK-NEXT:   |------------------------------+---|
+CHECK-NEXT:   |                 New FPO Data | D |
+CHECK-NEXT:   |------------------------------+---|
+CHECK-NEXT:   ------------------------------------
+CHECK-NEXT:   |           String Table           |
+CHECK-NEXT:   |------------------------------+---|
+CHECK-NEXT:   |                         File |   |
+CHECK-NEXT:   |------------------------------+---|
+CHECK-NEXT:   |            Number of Strings | D |
+CHECK-NEXT:   |------------------------------+---|
+CHECK-NEXT:   |                 Hash Version | I |
+CHECK-NEXT:   |------------------------------+---|
+CHECK-NEXT:   |                    Byte Size |
+CHECK-NEXT:   |------------------------------+---|
+CHECK-NEXT:   |                    Signature | I |
+CHECK-NEXT:   |------------------------------+---|
+CHECK-NEXT:   |                Empty Strings |
+CHECK-NEXT:   |------------------------------+---|
+CHECK-NEXT:   |  {{.*}}pdb-diff.cpp | {{[EI]}} |
+CHECK-NEXT:   |------------------------------+---|
+CHECK-NEXT:   |  $T0 $ebp = $...p $T0 8 + =  | D |
+CHECK-NEXT:   |------------------------------+---|
+CHECK-NEXT:   |  d:\src\llvm-...er internal) | D |
+CHECK-NEXT:   |------------------------------+---|
+CHECK-NEXT:   ----------------------------
+CHECK-NEXT:   |        PDB Stream        |
+CHECK-NEXT:   |----------------------+---|
+CHECK-NEXT:   |                 File |   |
+CHECK-NEXT:   |----------------------+---|
+CHECK-NEXT:   |          Stream Size |
+CHECK-NEXT:   |----------------------+---|
+CHECK-NEXT:   |                  Age | I |
+CHECK-NEXT:   |----------------------+---|
+CHECK-NEXT:   |                 Guid | D |
+CHECK-NEXT:   |----------------------+---|
+CHECK-NEXT:   |            Signature | D |
+CHECK-NEXT:   |----------------------+---|
+CHECK-NEXT:   |              Version | I |
+CHECK-NEXT:   |----------------------+---|
+CHECK-NEXT:   |       Features (set) | I |
+CHECK-NEXT:   |----------------------+---|
+CHECK-NEXT:   |              Feature | I |
+CHECK-NEXT:   |----------------------+---|
+CHECK-NEXT:   |    Named Stream Size | D |
+CHECK-NEXT:   |----------------------+---|
+CHECK-NEXT:   |  Named Streams (map) | D |
+CHECK-NEXT:   |----------------------+---|
+CHECK-NEXT:   |               /names | I |
+CHECK-NEXT:   |----------------------+---|
+CHECK-NEXT:   |            /LinkInfo | E |
+CHECK-NEXT:   |----------------------+---|
+CHECK-NEXT:   |     /src/headerblock | D |
+CHECK-NEXT:   |----------------------+---|
+CHECK-NEXT:   ----------------------------------------------
+CHECK-NEXT:   |                 DBI Stream                 |
+CHECK-NEXT:   |----------------------------------------+---|
+CHECK-NEXT:   |                                   File |   |
+CHECK-NEXT:   |----------------------------------------+---|
+CHECK-NEXT:   |                            Dbi Version | I |
+CHECK-NEXT:   |----------------------------------------+---|
+CHECK-NEXT:   |                                    Age | I |
+CHECK-NEXT:   |----------------------------------------+---|
+CHECK-NEXT:   |                                Machine | I |
+CHECK-NEXT:   |----------------------------------------+---|
+CHECK-NEXT:   |                                  Flags | D |
+CHECK-NEXT:   |----------------------------------------+---|
+CHECK-NEXT:   |                            Build Major | D |
+CHECK-NEXT:   |----------------------------------------+---|
+CHECK-NEXT:   |                            Build Minor | D |
+CHECK-NEXT:   |----------------------------------------+---|
+CHECK-NEXT:   |                           Build Number | D |
+CHECK-NEXT:   |----------------------------------------+---|
+CHECK-NEXT:   |                        PDB DLL Version | D |
+CHECK-NEXT:   |----------------------------------------+---|
+CHECK-NEXT:   |                           PDB DLL RBLD | I |
+CHECK-NEXT:   |----------------------------------------+---|
+CHECK-NEXT:   |                              DBG (FPO) | I |
+CHECK-NEXT:   |----------------------------------------+---|
+CHECK-NEXT:   |                        DBG (Exception) | I |
+CHECK-NEXT:   |----------------------------------------+---|
+CHECK-NEXT:   |                            DBG (Fixup) | I |
+CHECK-NEXT:   |----------------------------------------+---|
+CHECK-NEXT:   |                        DBG (OmapToSrc) | I |
+CHECK-NEXT:   |----------------------------------------+---|
+CHECK-NEXT:   |                      DBG (OmapFromSrc) | I |
+CHECK-NEXT:   |----------------------------------------+---|
+CHECK-NEXT:   |                       DBG (SectionHdr) | {{[EI]}} |
+CHECK-NEXT:   |----------------------------------------+---|
+CHECK-NEXT:   |                      DBG (TokenRidMap) | I |
+CHECK-NEXT:   |----------------------------------------+---|
+CHECK-NEXT:   |                            DBG (Xdata) | I |
+CHECK-NEXT:   |----------------------------------------+---|
+CHECK-NEXT:   |                            DBG (Pdata) | I |
+CHECK-NEXT:   |----------------------------------------+---|
+CHECK-NEXT:   |                           DBG (NewFPO) | D |
+CHECK-NEXT:   |----------------------------------------+---|
+CHECK-NEXT:   |                   DBG (SectionHdrOrig) | I |
+CHECK-NEXT:   |----------------------------------------+---|
+CHECK-NEXT:   |                         Globals Stream | D |
+CHECK-NEXT:   |----------------------------------------+---|
+CHECK-NEXT:   |                         Publics Stream | D |
+CHECK-NEXT:   |----------------------------------------+---|
+CHECK-NEXT:   |                         Symbol Records | D |
+CHECK-NEXT:   |----------------------------------------+---|
+CHECK-NEXT:   |                             Has CTypes | I |
+CHECK-NEXT:   |----------------------------------------+---|
+CHECK-NEXT:   |                Is Incrementally Linked | D |
+CHECK-NEXT:   |----------------------------------------+---|
+CHECK-NEXT:   |                            Is Stripped | I |
+CHECK-NEXT:   |----------------------------------------+---|
+CHECK-NEXT:   |                           Module Count | I |
+CHECK-NEXT:   |----------------------------------------+---|
+CHECK-NEXT:   |                      Source File Count | I |
+CHECK-NEXT:   |----------------------------------------+---|
+CHECK-NEXT:   |Module "{{.*}}Inputs/pdb-diff.obj"|
+CHECK-NEXT:   |----------------------------------------+---|
+CHECK-NEXT:   |                                 - Modi | D |
+CHECK-NEXT:   |----------------------------------------+---|
+CHECK-NEXT:   |            Module "* Linker *"             |
+CHECK-NEXT:   |----------------------------------------+---|
+CHECK-NEXT:   |                                 - Modi | I |
+CHECK-NEXT:   |----------------------------------------+---|
+CHECK-NEXT:   |                        - Obj File Name | I |
+CHECK-NEXT:   |----------------------------------------+---|
+CHECK-NEXT:   |                         - Debug Stream | {{[EI]}} |
+CHECK-NEXT:   |----------------------------------------+---|
+CHECK-NEXT:   |                        - C11 Byte Size | I |
+CHECK-NEXT:   |----------------------------------------+---|
+CHECK-NEXT:   |                        - C13 Byte Size | I |
+CHECK-NEXT:   |----------------------------------------+---|
+CHECK-NEXT:   |                           - # of files | I |
+CHECK-NEXT:   |----------------------------------------+---|
+CHECK-NEXT:   |                  - Pdb File Path Index | {{[EI]}} |
+CHECK-NEXT:   |----------------------------------------+---|
+CHECK-NEXT:   |               - Source File Name Index | {{[EI]}} |
+CHECK-NEXT:   |----------------------------------------+---|
+CHECK-NEXT:   |                     - Symbol Byte Size | D |
+CHECK-NEXT:   |----------------------------------------+---|

lld/trunk/test/COFF/pdb-source-lines.test

 $ cl -c -Z7 pdb_lines*.c
 
 RUN: yaml2obj %S/Inputs/pdb_lines_1.yaml -o %t.pdb_lines_1.obj
 RUN: yaml2obj %S/Inputs/pdb_lines_2.yaml -o %t.pdb_lines_2.obj
 RUN: lld-link -debug -entry:main -nodefaultlib -out:%t.exe -pdb:%t.pdb %t.pdb_lines_1.obj %t.pdb_lines_2.obj
 RUN: llvm-pdbutil pdb2yaml -modules -module-files -subsections=lines,fc %t.pdb | FileCheck %s
 
 CHECK-LABEL: DbiStream:       
-CHECK-NEXT:   VerHeader:       V110
+CHECK-NEXT:   VerHeader:       V70
 CHECK-NEXT:   Age:             1
 CHECK-NEXT:   BuildNumber:     0
 CHECK-NEXT:   PdbDllVersion:   0
 CHECK-NEXT:   PdbDllRbld:      0
 CHECK-NEXT:   Flags:           0
 CHECK-NEXT:   MachineType:     x86
 CHECK-NEXT:   Modules:         
 

lld/trunk/test/COFF/pdb.test

 # CHECK:      StreamMap:
 # CHECK:      PdbStream:
 # CHECK-NEXT:   Age:             1
 # CHECK-NEXT:   Guid:
 # CHECK-NEXT:   Signature:       0
 # CHECK-NEXT:   Features:        [ VC140 ]
 # CHECK-NEXT:   Version:         VC70
 # CHECK-NEXT: DbiStream:
-# CHECK-NEXT:   VerHeader:       V110
+# CHECK-NEXT:   VerHeader:       V70
 # CHECK-NEXT:   Age:             1
 # CHECK-NEXT:   BuildNumber:     0
 # CHECK-NEXT:   PdbDllVersion:   0
 # CHECK-NEXT:   PdbDllRbld:      0
 # CHECK-NEXT:   Flags:           0
 # CHECK-NEXT:   MachineType:     x86
 # CHECK-NEXT: TpiStream:
 # CHECK-NEXT:   Version:         VC80

llvm/trunk/include/llvm/DebugInfo/PDB/Native/DbiStreamBuilder.h

public:
 
   // A helper function to create a Section Map from a COFF section header.
   static std::vector<SecMapEntry>
   createSectionMap(ArrayRef<llvm::object::coff_section> SecHdrs);
 
 private:
   struct DebugStream {
     ArrayRef<uint8_t> Data;
-    uint16_t StreamNumber = 0;
+    uint16_t StreamNumber = kInvalidStreamIndex;
   };
 
   Error finalize();
   uint32_t calculateModiSubstreamSize() const;
   uint32_t calculateNamesOffset() const;
   uint32_t calculateSectionContribsStreamSize() const;
   uint32_t calculateSectionMapStreamSize() const;
   uint32_t calculateFileInfoSubstreamSize() const;

llvm/trunk/lib/DebugInfo/PDB/Native/DbiStream.cpp

 }
 
 // Initializes this->SectionHeaders.
 Error DbiStream::initializeSectionHeadersData() {
   if (DbgStreams.size() == 0)
     return Error::success();
 
   uint32_t StreamNum = getDebugStreamIndex(DbgHeaderType::SectionHdr);
+  if (StreamNum == kInvalidStreamIndex)
+    return Error::success();
+
   if (StreamNum >= Pdb.getNumStreams())
     return make_error<RawError>(raw_error_code::no_stream);
 
   auto SHS = MappedBlockStream::createIndexedStream(
       Pdb.getMsfLayout(), Pdb.getMsfBuffer(), StreamNum, Pdb.getAllocator());
 
   size_t StreamLen = SHS->getLength();
   if (StreamLen % sizeof(object::coff_section))

llvm/trunk/lib/DebugInfo/PDB/Native/DbiStreamBuilder.cpp

 void DbiStreamBuilder::setMachineType(PDB_Machine M) { MachineType = M; }
 
 void DbiStreamBuilder::setSectionMap(ArrayRef<SecMapEntry> SecMap) {
   SectionMap = SecMap;
 }
 
 Error DbiStreamBuilder::addDbgStream(pdb::DbgHeaderType Type,
                                      ArrayRef<uint8_t> Data) {
-  if (DbgStreams[(int)Type].StreamNumber)
+  if (DbgStreams[(int)Type].StreamNumber != kInvalidStreamIndex)
     return make_error<RawError>(raw_error_code::duplicate_entry,
                                 "The specified stream type already exists");
   auto ExpectedIndex = Msf.addStream(Data.size());
   if (!ExpectedIndex)
     return ExpectedIndex.takeError();
   uint32_t Index = std::move(*ExpectedIndex);
   DbgStreams[(int)Type].Data = Data;
   DbgStreams[(int)Type].StreamNumber = Index;

llvm/trunk/lib/DebugInfo/PDB/Native/PDBFile.cpp

if (Machine == PDB_Machine::Amd64)
     return 8;
   return 4;
 }
 
 bool PDBFile::hasPDBDbiStream() const { return StreamDBI < getNumStreams(); }
 
 bool PDBFile::hasPDBGlobalsStream() {
   auto DbiS = getPDBDbiStream();
-  if (!DbiS)
+  if (!DbiS) {
+    consumeError(DbiS.takeError());
     return false;
+  }
+
   return DbiS->getGlobalSymbolStreamIndex() < getNumStreams();
 }
 
 bool PDBFile::hasPDBInfoStream() { return StreamPDB < getNumStreams(); }
 
 bool PDBFile::hasPDBIpiStream() const { return StreamIPI < getNumStreams(); }
 
 bool PDBFile::hasPDBPublicsStream() {
   auto DbiS = getPDBDbiStream();
-  if (!DbiS)
+  if (!DbiS) {
+    consumeError(DbiS.takeError());
     return false;
+  }
   return DbiS->getPublicSymbolStreamIndex() < getNumStreams();
 }
 
 bool PDBFile::hasPDBSymbolStream() {
   auto DbiS = getPDBDbiStream();
   if (!DbiS)
     return false;
   return DbiS->getSymRecordStreamIndex() < getNumStreams();

llvm/trunk/tools/llvm-pdbutil/Diff.cpp

 static std::string shortFilePath(StringRef Path, uint32_t Width) {
   if (Path.size() <= Width)
     return Path;
   Path = Path.take_back(Width - 3);
   return std::string("...") + Path.str();
 }
 
 Error DiffStyle::diffSuperBlock() {
-  DiffPrinter D(2, "MSF Super Block", 16, 20, outs());
+  DiffPrinter D(2, "MSF Super Block", 16, 20, opts::diff::PrintResultColumn,
+                opts::diff::PrintValueColumns, outs());
   D.printExplicit("File", DiffResult::UNSPECIFIED,
                   shortFilePath(File1.getFilePath(), 18),
                   shortFilePath(File2.getFilePath(), 18));
   D.print("Block Size", File1.getBlockSize(), File2.getBlockSize());
   D.print("Block Count", File1.getBlockCount(), File2.getBlockCount());
   D.print("Unknown 1", File1.getUnknown1(), File2.getUnknown1());
   D.print("Directory Size", File1.getNumDirectoryBytes(),
           File2.getNumDirectoryBytes());
   return Error::success();
 }
 
 Error DiffStyle::diffStreamDirectory() {
-  DiffPrinter D(2, "Stream Directory", 30, 20, outs());
+  DiffPrinter D(2, "Stream Directory", 30, 20, opts::diff::PrintResultColumn,
+                opts::diff::PrintValueColumns, outs());
   D.printExplicit("File", DiffResult::UNSPECIFIED,
                   shortFilePath(File1.getFilePath(), 18),
                   shortFilePath(File2.getFilePath(), 18));
 
   SmallVector<std::string, 32> P;
   SmallVector<std::string, 32> Q;
   discoverStreamPurposes(File1, P, 28);
   discoverStreamPurposes(File2, Q, 28);
for (const auto &Q : QI) {
     D.printExplicit(Q.value(), DiffResult::DIFFERENT, "(not present)",
                     Q.index());
   }
 
   return Error::success();
 }
 
 Error DiffStyle::diffStringTable() {
-  DiffPrinter D(2, "String Table", 30, 20, outs());
+  DiffPrinter D(2, "String Table", 30, 20, opts::diff::PrintResultColumn,
+                opts::diff::PrintValueColumns, outs());
   D.printExplicit("File", DiffResult::UNSPECIFIED,
                   shortFilePath(File1.getFilePath(), 18),
                   shortFilePath(File2.getFilePath(), 18));
 
   auto ExpectedST1 = File1.getStringTable();
   auto ExpectedST2 = File2.getStringTable();
   bool Has1 = !!ExpectedST1;
   bool Has2 = !!ExpectedST2;
for (const auto &S : RS) {
     D.print<EquivalentDiffProvider>(Truncated, I, J);
   }
   return Error::success();
 }
 
 Error DiffStyle::diffFreePageMap() { return Error::success(); }
 
 Error DiffStyle::diffInfoStream() {
-  DiffPrinter D(2, "PDB Stream", 22, 40, outs());
+  DiffPrinter D(2, "PDB Stream", 22, 40, opts::diff::PrintResultColumn,
+                opts::diff::PrintValueColumns, outs());
   D.printExplicit("File", DiffResult::UNSPECIFIED,
                   shortFilePath(File1.getFilePath(), 38),
                   shortFilePath(File2.getFilePath(), 38));
 
   auto ExpectedInfo1 = File1.getPDBInfoStream();
   auto ExpectedInfo2 = File2.getPDBInfoStream();
 
   bool Has1 = !!ExpectedInfo1;
getModuleDescriptors(const DbiModuleList &ML) {
   std::vector<std::pair<uint32_t, DbiModuleDescriptor>> List;
   List.reserve(ML.getModuleCount());
   for (uint32_t I = 0; I < ML.getModuleCount(); ++I)
     List.emplace_back(I, ML.getModuleDescriptor(I));
   return List;
 }
 
 Error DiffStyle::diffDbiStream() {
-  DiffPrinter D(2, "DBI Stream", 40, 30, outs());
+  DiffPrinter D(2, "DBI Stream", 40, 30, opts::diff::PrintResultColumn,
+                opts::diff::PrintValueColumns, outs());
   D.printExplicit("File", DiffResult::UNSPECIFIED,
-                  shortFilePath(File1.getFilePath(), 38),
-                  shortFilePath(File2.getFilePath(), 38));
+                  shortFilePath(File1.getFilePath(), 28),
+                  shortFilePath(File2.getFilePath(), 28));
 
   auto ExpectedDbi1 = File1.getPDBDbiStream();
   auto ExpectedDbi2 = File2.getPDBDbiStream();
 
   bool Has1 = !!ExpectedDbi1;
   bool Has2 = !!ExpectedDbi2;
   if (!(Has1 && Has2)) {
     std::string L = Has1 ? "(present)" : "(not present)";

llvm/trunk/tools/llvm-pdbutil/DiffPrinter.h

struct EquivalentDiffProvider {
   template <typename T> std::string format(const T &Item) {
     return formatv("{0}", Item).str();
   }
 };
 
 class DiffPrinter {
 public:
   DiffPrinter(uint32_t Indent, StringRef Header, uint32_t PropertyWidth,
-              uint32_t FieldWidth, raw_ostream &Stream);
+              uint32_t FieldWidth, bool Result, bool Values,
+              raw_ostream &Stream);
   ~DiffPrinter();
 
   template <typename T, typename U> struct Identical {};
 
   template <typename Provider = IdenticalDiffProvider, typename T, typename U>
   void print(StringRef Property, const T &Left, const U &Right,
              Provider P = Provider()) {
     std::string L = P.format(Left);
for (const auto &R : RightCopy) {
       printExplicit(R.getKey(), DiffResult::DIFFERENT, "(not present)",
                     R.getValue());
     }
   }
 
   void printFullRow(StringRef Text);
 
 private:
-  void printSame(StringRef Property, StringRef Value);
-  void printDifferent(StringRef Property, StringRef Left, StringRef Right);
+  uint32_t tableWidth() const;
 
   void printHeaderRow();
   void printSeparatorRow();
   void newLine(char InitialChar = '|');
-  void printField(StringRef Value, DiffResult C, AlignStyle Style,
-                  uint32_t Width);
+  void printValue(StringRef Value, DiffResult C, AlignStyle Style,
+                  uint32_t Width, bool Force);
+  void printResult(DiffResult Result);
 
+  bool PrintResult;
+  bool PrintValues;
   uint32_t Indent;
   uint32_t PropertyWidth;
   uint32_t FieldWidth;
   raw_ostream &OS;
 };
 } // namespace pdb
 } // namespace llvm
 
 #endif

llvm/trunk/tools/llvm-pdbutil/DiffPrinter.cpp

static void setColor(llvm::raw_ostream &OS, DiffResult Result) {
   default:
     OS.changeColor(raw_ostream::Colors::RED, false);
     break;
   }
 }
 
 DiffPrinter::DiffPrinter(uint32_t Indent, StringRef Header,
                          uint32_t PropertyWidth, uint32_t FieldWidth,
-                         raw_ostream &Stream)
-    : Indent(Indent), PropertyWidth(PropertyWidth), FieldWidth(FieldWidth),
-      OS(Stream) {
+                         bool Result, bool Fields, raw_ostream &Stream)
+    : PrintResult(Result), PrintValues(Fields), Indent(Indent),
+      PropertyWidth(PropertyWidth), FieldWidth(FieldWidth), OS(Stream) {
   printHeaderRow();
   printFullRow(Header);
 }
 
 DiffPrinter::~DiffPrinter() {}
 
+uint32_t DiffPrinter::tableWidth() const {
+  // `|`
+  uint32_t W = 1;
+
+  // `<width>|`
+  W += PropertyWidth + 1;
+
+  if (PrintResult) {
+    // ` I |`
+    W += 4;
+  }
+
+  if (PrintValues) {
+    // `<width>|<width>|`
+    W += 2 * (FieldWidth + 1);
+  }
+  return W;
+}
+
 void DiffPrinter::printFullRow(StringRef Text) {
   newLine();
-  printField(Text, DiffResult::UNSPECIFIED, AlignStyle::Center,
-             PropertyWidth + 1 + FieldWidth + 1 + FieldWidth);
+  printValue(Text, DiffResult::UNSPECIFIED, AlignStyle::Center,
+             tableWidth() - 2, true);
   printSeparatorRow();
 }
 
 void DiffPrinter::printSeparatorRow() {
   newLine();
   OS << formatv("{0}", fmt_repeat('-', PropertyWidth));
+  if (PrintResult) {
+    OS << '+';
+    OS << formatv("{0}", fmt_repeat('-', 3));
+  }
+  if (PrintValues) {
     OS << '+';
     OS << formatv("{0}", fmt_repeat('-', FieldWidth));
     OS << '+';
     OS << formatv("{0}", fmt_repeat('-', FieldWidth));
+  }
   OS << '|';
 }
 
 void DiffPrinter::printHeaderRow() {
   newLine('-');
-  OS << formatv("{0}", fmt_repeat('-', PropertyWidth + 2 * FieldWidth + 3));
+  OS << formatv("{0}", fmt_repeat('-', tableWidth() - 1));
 }
 
 void DiffPrinter::newLine(char InitialChar) {
   OS << "\n";
   OS.indent(Indent) << InitialChar;
 }
 
 void DiffPrinter::printExplicit(StringRef Property, DiffResult C,
                                 StringRef Left, StringRef Right) {
   newLine();
-  printField(Property, DiffResult::UNSPECIFIED, AlignStyle::Right,
-             PropertyWidth);
-  printField(Left, C, AlignStyle::Center, FieldWidth);
-  printField(Right, C, AlignStyle::Center, FieldWidth);
+  printValue(Property, DiffResult::UNSPECIFIED, AlignStyle::Right,
+             PropertyWidth, true);
+  printResult(C);
+  printValue(Left, C, AlignStyle::Center, FieldWidth, false);
+  printValue(Right, C, AlignStyle::Center, FieldWidth, false);
   printSeparatorRow();
 }
 
-void DiffPrinter::printSame(StringRef Property, StringRef Value) {
-  newLine();
-  printField(Property, DiffResult::UNSPECIFIED, AlignStyle::Right,
-             PropertyWidth);
-  printField(Value, DiffResult::IDENTICAL, AlignStyle::Center,
-             FieldWidth + 1 + FieldWidth);
-  printSeparatorRow();
+void DiffPrinter::printResult(DiffResult Result) {
+  if (!PrintResult)
+    return;
+  switch (Result) {
+  case DiffResult::DIFFERENT:
+    printValue("D", Result, AlignStyle::Center, 3, true);
+    break;
+  case DiffResult::EQUIVALENT:
+    printValue("E", Result, AlignStyle::Center, 3, true);
+    break;
+  case DiffResult::IDENTICAL:
+    printValue("I", Result, AlignStyle::Center, 3, true);
+    break;
+  case DiffResult::UNSPECIFIED:
+    printValue(" ", Result, AlignStyle::Center, 3, true);
+    break;
+  default:
+    llvm_unreachable("unreachable!");
   }
-
-void DiffPrinter::printDifferent(StringRef Property, StringRef Left,
-                                 StringRef Right) {
-  newLine();
-  printField(Property, DiffResult::UNSPECIFIED, AlignStyle::Right,
-             PropertyWidth);
-  printField(Left, DiffResult::DIFFERENT, AlignStyle::Center, FieldWidth);
-  printField(Right, DiffResult::DIFFERENT, AlignStyle::Center, FieldWidth);
-  printSeparatorRow();
 }
 
-void DiffPrinter::printField(StringRef Value, DiffResult C, AlignStyle Style,
-                             uint32_t Width) {
+void DiffPrinter::printValue(StringRef Value, DiffResult C, AlignStyle Style,
+                             uint32_t Width, bool Force) {
+  if (!Force && !PrintValues)
+    return;
+
   if (Style == AlignStyle::Right)
     --Width;
 
   std::string FormattedItem =
       formatv("{0}", fmt_align(Value, Style, Width)).str();
   if (C != DiffResult::UNSPECIFIED) {
     setColor(OS, C);
     OS << FormattedItem;
     OS.resetColor();
   } else
     OS << FormattedItem;
   if (Style == AlignStyle::Right)
     OS << ' ';
   OS << '|';
 }

llvm/trunk/tools/llvm-pdbutil/llvm-pdbutil.h

 extern llvm::cl::opt<bool> TpiStream;
 extern llvm::cl::opt<bool> IpiStream;
 extern llvm::cl::list<std::string> InputFilename;
 extern llvm::cl::opt<bool> DumpModules;
 extern llvm::cl::opt<bool> DumpModuleFiles;
 extern llvm::cl::list<ModuleSubsection> DumpModuleSubsections;
 extern llvm::cl::opt<bool> DumpModuleSyms;
 } // namespace pdb2yaml
+
+namespace diff {
+extern llvm::cl::opt<bool> PrintValueColumns;
+extern llvm::cl::opt<bool> PrintResultColumn;
+} // namespace diff
 }
 
 #endif

llvm/trunk/tools/llvm-pdbutil/llvm-pdbutil.cpp

ExcludeSystemLibraries("no-system-libs",
                            cl::cat(FilterCategory), cl::sub(PrettySubcommand));
 
 cl::opt<bool> NoEnumDefs("no-enum-definitions",
                          cl::desc("Don't display full enum definitions"),
                          cl::cat(FilterCategory), cl::sub(PrettySubcommand));
 }
 
 namespace diff {
+cl::opt<bool> PrintValueColumns(
+    "values", cl::init(true),
+    cl::desc("Print one column for each PDB with the field value"),
+    cl::Optional, cl::sub(DiffSubcommand));
+cl::opt<bool>
+    PrintResultColumn("result", cl::init(false),
+                      cl::desc("Print a column with the result status"),
+                      cl::Optional, cl::sub(DiffSubcommand));
+
 cl::list<std::string> InputFilenames(cl::Positional,
                                      cl::desc("<first> <second>"),
                                      cl::OneOrMore, cl::sub(DiffSubcommand));
 }
 
 cl::OptionCategory FileOptions("Module & File Options");
 
 namespace bytes {
if (opts::PdbToYamlSubcommand) {
     simplifyChunkList(opts::pdb2yaml::DumpModuleSubsections);
 
     if (opts::pdb2yaml::DumpModuleSyms || opts::pdb2yaml::DumpModuleFiles)
       opts::pdb2yaml::DumpModules = true;
 
     if (opts::pdb2yaml::DumpModules)
       opts::pdb2yaml::DbiStream = true;
   }
+  if (opts::DiffSubcommand) {
+    if (!opts::diff::PrintResultColumn && !opts::diff::PrintValueColumns) {
+      llvm::errs() << "WARNING: No diff columns specified\n";
+    }
+  }
 
   llvm::sys::InitializeCOMRAII COM(llvm::sys::COMThreadingMode::MultiThreaded);
 
   if (opts::PdbToYamlSubcommand) {
     pdb2Yaml(opts::pdb2yaml::InputFilename.front());
   } else if (opts::YamlToPdbSubcommand) {
     if (opts::yaml2pdb::YamlPdbOutputFile.empty()) {
       SmallString<16> OutputFilename(opts::yaml2pdb::InputFilename.getValue());