Fix some differences between lld and MSVC generated PDBs.

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.

Differential Revision: https://reviews.llvm.org/D35086

git-svn-id: https://llvm.org/svn/llvm-project/llvm/trunk@307422 91177308-0d34-0410-b5e6-96231b3b80d8

diff --git a/include/llvm/DebugInfo/PDB/Native/DbiStreamBuilder.h b/include/llvm/DebugInfo/PDB/Native/DbiStreamBuilder.h
index 7d11cf3d6bb5621c4464b3e07b56da9dcc0d561a..7d676e87ecd7780ff687816ab2e9ab2fd67b52d3 100644 (file)
--- a/include/llvm/DebugInfo/PDB/Native/DbiStreamBuilder.h
+++ b/include/llvm/DebugInfo/PDB/Native/DbiStreamBuilder.h
@@ -78,7 +78,7 @@ public:
 private:
   struct DebugStream {
     ArrayRef<uint8_t> Data;
-    uint16_t StreamNumber = 0;
+    uint16_t StreamNumber = kInvalidStreamIndex;
   };
 
   Error finalize();

diff --git a/lib/DebugInfo/PDB/Native/DbiStream.cpp b/lib/DebugInfo/PDB/Native/DbiStream.cpp
index 78cf2c551c2a0daf78649d915981882fccd8689d..0eeac7e4c0847de30ff5ae5c8f4e0a963c493fec 100644 (file)
--- a/lib/DebugInfo/PDB/Native/DbiStream.cpp
+++ b/lib/DebugInfo/PDB/Native/DbiStream.cpp
@@ -252,6 +252,9 @@ Error DbiStream::initializeSectionHeadersData() {
     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);
 

diff --git a/lib/DebugInfo/PDB/Native/DbiStreamBuilder.cpp b/lib/DebugInfo/PDB/Native/DbiStreamBuilder.cpp
index 2993f788434626c10534370dcf2101c283274a57..e49a77b4fcec09ea4955eed7b65f2ee7806ec50b 100644 (file)
--- a/lib/DebugInfo/PDB/Native/DbiStreamBuilder.cpp
+++ b/lib/DebugInfo/PDB/Native/DbiStreamBuilder.cpp
@@ -51,7 +51,7 @@ void DbiStreamBuilder::setSectionMap(ArrayRef<SecMapEntry> 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());

diff --git a/lib/DebugInfo/PDB/Native/PDBFile.cpp b/lib/DebugInfo/PDB/Native/PDBFile.cpp
index 4f6ebb0cb3428aee8fe9493548e9146b5b56ae25..0b6492efc70f3b09ed655613225de89a08eca94c 100644 (file)
--- a/lib/DebugInfo/PDB/Native/PDBFile.cpp
+++ b/lib/DebugInfo/PDB/Native/PDBFile.cpp
@@ -385,8 +385,11 @@ 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();
 }
 
@@ -396,8 +399,10 @@ 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();
 }
 

diff --git a/tools/llvm-pdbutil/Diff.cpp b/tools/llvm-pdbutil/Diff.cpp
index eda0ce275affd153775cd6a04178f96b4de7d6fb..fc374ad3a4ca0f169d934446394ef91f9dba41ab 100644 (file)
--- a/tools/llvm-pdbutil/Diff.cpp
+++ b/tools/llvm-pdbutil/Diff.cpp
@@ -108,7 +108,8 @@ static std::string shortFilePath(StringRef Path, uint32_t Width) {
 }
 
 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));
@@ -121,7 +122,8 @@ Error DiffStyle::diffSuperBlock() {
 }
 
 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));
@@ -163,7 +165,8 @@ Error DiffStyle::diffStreamDirectory() {
 }
 
 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));
@@ -251,7 +254,8 @@ Error DiffStyle::diffStringTable() {
 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));
@@ -345,10 +349,11 @@ getModuleDescriptors(const DbiModuleList &ML) {
 }
 
 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();

diff --git a/tools/llvm-pdbutil/DiffPrinter.cpp b/tools/llvm-pdbutil/DiffPrinter.cpp
index b608d54d0e28735da335aebe4300b56e0bdec983..8deb69309b055becbf441a12dfc5337fcaeb678f 100644 (file)
--- a/tools/llvm-pdbutil/DiffPrinter.cpp
+++ b/tools/llvm-pdbutil/DiffPrinter.cpp
@@ -22,35 +22,60 @@ static void setColor(llvm::raw_ostream &OS, DiffResult Result) {
 
 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));
-  OS << '+';
-  OS << formatv("{0}", fmt_repeat('-', FieldWidth));
-  OS << '+';
-  OS << formatv("{0}", fmt_repeat('-', FieldWidth));
+  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) {
@@ -61,34 +86,40 @@ void DiffPrinter::newLine(char 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::printValue(StringRef Value, DiffResult C, AlignStyle Style,
+                             uint32_t Width, bool Force) {
+  if (!Force && !PrintValues)
+    return;
 
-void DiffPrinter::printField(StringRef Value, DiffResult C, AlignStyle Style,
-                             uint32_t Width) {
   if (Style == AlignStyle::Right)
     --Width;
 

diff --git a/tools/llvm-pdbutil/DiffPrinter.h b/tools/llvm-pdbutil/DiffPrinter.h
index 8a4527c62742ab597e6241bfea2a2d1fc6270462..1235befa1c24d0c8328aad7932c30bc89a5d9418 100644 (file)
--- a/tools/llvm-pdbutil/DiffPrinter.h
+++ b/tools/llvm-pdbutil/DiffPrinter.h
@@ -51,7 +51,8 @@ struct EquivalentDiffProvider {
 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 {};
@@ -138,15 +139,17 @@ public:
   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;

diff --git a/tools/llvm-pdbutil/llvm-pdbutil.cpp b/tools/llvm-pdbutil/llvm-pdbutil.cpp
index f3bfa8f7b35354ced66f27ea7ecf8bcd691a8224..6432b9ce86c05d023b3d0e5188370b0a3da3fe39 100644 (file)
--- a/tools/llvm-pdbutil/llvm-pdbutil.cpp
+++ b/tools/llvm-pdbutil/llvm-pdbutil.cpp
@@ -284,6 +284,15 @@ cl::opt<bool> NoEnumDefs("no-enum-definitions",
 }
 
 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));
@@ -1079,6 +1088,11 @@ int main(int argc_, const char *argv_[]) {
     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);
 

diff --git a/tools/llvm-pdbutil/llvm-pdbutil.h b/tools/llvm-pdbutil/llvm-pdbutil.h
index 9ee5866bbeffcdc7223b8e0f97f21e5d324e02fc..b8e1cf4ed15c5e6a5082bb96577c342e9c0feba6 100644 (file)
--- a/tools/llvm-pdbutil/llvm-pdbutil.h
+++ b/tools/llvm-pdbutil/llvm-pdbutil.h
@@ -168,6 +168,11 @@ 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