Merge pull request #35405 from davidungar/fingerprint-assert

Author: David Ungar

include/swift/Basic/Fingerprint.h

@@ -71,22 +71,16 @@ class Fingerprint final {
   explicit Fingerprint(Fingerprint::Core value) : core(value) {}
 
   /// Creates a fingerprint value from the given input string that is known to
-  /// be a 32-byte hash value.
+  /// be a 32-byte hash value, i.e. that represent a valid 32-bit hex integer.
   ///
-  /// In +asserts builds, strings that violate this invariant will crash. If a
-  /// fingerprint value is needed to represent an "invalid" state, use a
-  /// vocabulary type like \c Optional<Fingerprint> instead.
-  static Fingerprint fromString(llvm::StringRef value);
-
-  /// Creates a fingerprint value from the given input string literal.
-  template <std::size_t N>
-  explicit Fingerprint(const char (&literal)[N])
-    : Fingerprint{Fingerprint::fromString({literal, N-1}).core} {
-      static_assert(N == Fingerprint::DIGEST_LENGTH + 1,
-                    "String literal must be 32 bytes in length!");
-    }
-
-  /// Creates a fingerprint value by consuming the given \c MD5Result from LLVM.
+  /// Strings that violate this invariant will return a null optional.
+  static llvm::Optional<Fingerprint> fromString(llvm::StringRef value);
+
+  /// For mocking only:
+  /// Aborts if unconvertible, returns None for an empty string.
+  static llvm::Optional<Fingerprint> mockFromString(llvm::StringRef value);
+
+ /// Creates a fingerprint value by consuming the given \c MD5Result from LLVM.
   explicit Fingerprint(llvm::MD5::MD5Result &&MD5Value)
       : core{MD5Value.words()} {}
 

lib/AST/FineGrainedDependencyFormat.cpp

@@ -231,8 +231,10 @@ bool Deserializer::readFineGrainedDependencyGraph(SourceFileDepGraph &g,
       // FINGERPRINT_NODE must follow a SOURCE_FILE_DEP_GRAPH_NODE.
       if (node == nullptr)
         llvm::report_fatal_error("Unexpected FINGERPRINT_NODE record");
-
-      node->setFingerprint(Fingerprint::fromString(BlobData));
+      if (auto fingerprint = Fingerprint::fromString(BlobData))
+        node->setFingerprint(fingerprint.getValue());
+      else
+        llvm::report_fatal_error(Twine("Unconvertable FINGERPRINT_NODE record: '") + BlobData + "'" );
       break;
     }
 

lib/Basic/Fingerprint.cpp

@@ -29,7 +29,7 @@ void swift::simple_display(llvm::raw_ostream &out, const Fingerprint &fp) {
   out << fp.getRawValue();
 }
 
-Fingerprint Fingerprint::fromString(StringRef value) {
+Optional<Fingerprint> Fingerprint::fromString(StringRef value) {
   assert(value.size() == Fingerprint::DIGEST_LENGTH &&
          "Only supports 32-byte hash values!");
   auto fp = Fingerprint::ZERO();
@@ -41,14 +41,35 @@ Fingerprint Fingerprint::fromString(StringRef value) {
     std::istringstream s(value.drop_front(Fingerprint::DIGEST_LENGTH/2).str());
     s >> std::hex >> fp.core.second;
   }
+  // If the input string is not valid hex, the conversion above can fail.
+  if (value != fp.getRawValue())
+    return None;
+
   return fp;
 }
 
+Optional<Fingerprint> Fingerprint::mockFromString(llvm::StringRef value) {
+  auto contents = value.str();
+  const auto n = value.size();
+  if (n == 0 || n > Fingerprint::DIGEST_LENGTH)
+    return None;
+  // Insert at start so that "1" and "10" are distinct
+  contents.insert(0, Fingerprint::DIGEST_LENGTH - n, '0');
+  auto fingerprint = fromString(contents);
+    if (!fingerprint) {
+    llvm::errs() << "unconvertable fingerprint from switdeps ':"
+                 << contents << "'\n";
+    abort();
+  }
+  return fingerprint;
+}
+
+
+
 llvm::SmallString<Fingerprint::DIGEST_LENGTH> Fingerprint::getRawValue() const {
   llvm::SmallString<Fingerprint::DIGEST_LENGTH> Str;
   llvm::raw_svector_ostream Res(Str);
   Res << llvm::format_hex_no_prefix(core.first, 16);
   Res << llvm::format_hex_no_prefix(core.second, 16);
-  assert(*this == Fingerprint::fromString(Str));
   return Str;
 }

lib/Serialization/ModuleFile.cpp

@@ -986,7 +986,12 @@ void ModuleFile::collectBasicSourceFileInfo(
 
     BasicSourceFileInfo info;
     info.FilePath = filePath;
-    info.InterfaceHash = Fingerprint::fromString(fpStr);
+    if (auto fingerprint = Fingerprint::fromString(fpStr))
+      info.InterfaceHash = fingerprint.getValue();
+    else {
+      llvm::errs() << "Unconvertable fingerprint '" << fpStr << "'\n";
+      abort();
+    }
     info.LastModified =
         llvm::sys::TimePoint<>(std::chrono::nanoseconds(timestamp));
     info.FileSize = fileSize;

lib/Serialization/ModuleFileCoreTableInfo.h

@@ -602,7 +602,10 @@ class ModuleFileSharedCore::DeclFingerprintsTableInfo {
     using namespace llvm::support;
     auto str = llvm::StringRef{reinterpret_cast<const char *>(data),
                                Fingerprint::DIGEST_LENGTH};
-    return Fingerprint::fromString(str);
+    if (auto fp = Fingerprint::fromString(str))
+      return fp.getValue();
+    llvm::errs() << "Unconvertable fingerprint '" << str << "'\n";
+    abort();
   }
 };
 

test/Driver/Dependencies/Inputs/chained-private-after-fine/main.swift

@@ -6,7 +6,7 @@ allNodes:
       aspect:          interface
       context:         ''
       name:            main.swiftdeps
-    fingerprint:     afterachangewasmadetoswiftsource
+    fingerprint:     88888888888888888888888888888888
     sequenceNumber:  0
     defsIDependUpon: [ 2, 4 ]
     isProvides:      true
@@ -15,7 +15,7 @@ allNodes:
       aspect:          implementation
       context:         ''
       name:            main.swiftdeps
-    fingerprint:     afterachangewasmadetoswiftsource
+    fingerprint:     88888888888888888888888888888888
     sequenceNumber:  1
     defsIDependUpon: [  ]
     isProvides:      true

test/Driver/Dependencies/Inputs/chained-private-after-fine/main.swiftdeps

@@ -6,7 +6,7 @@ allNodes:
       aspect:          interface
       context:         ''
       name:            bad.swiftdeps
-    fingerprint:     afterachangewasmadetoswiftsource
+    fingerprint:     22222222222222222222222222222222
     sequenceNumber:  0
     defsIDependUpon: [ 2 ]
     isProvides:      true
@@ -15,7 +15,7 @@ allNodes:
       aspect:          implementation
       context:         ''
       name:            bad.swiftdeps
-    fingerprint:     afterachangewasmadetoswiftsource
+    fingerprint:     22222222222222222222222222222222
     sequenceNumber:  1
     defsIDependUpon: [  ]
     isProvides:      true

test/Driver/Dependencies/Inputs/fail-interface-hash-fine/bad.swift

@@ -6,7 +6,7 @@ allNodes:
       aspect:          interface
       context:         ''
       name:            dependsonbad.swiftdeps
-    fingerprint:     afterachangewasmadetoswiftsource
+    fingerprint:     22222222222222222222222222222222
     sequenceNumber:  0
     defsIDependUpon: [ 2 ]
     isProvides:      true
@@ -15,7 +15,7 @@ allNodes:
       aspect:          implementation
       context:         ''
       name:            dependsonbad.swiftdeps
-    fingerprint:     afterachangewasmadetoswiftsource
+    fingerprint:     22222222222222222222222222222222
     sequenceNumber:  1
     defsIDependUpon: [  ]
     isProvides:      true

test/Driver/Dependencies/Inputs/fail-interface-hash-fine/bad.swiftdeps

@@ -6,7 +6,7 @@ allNodes:
       aspect:          interface
       context:         ''
       name:            dependsonmain.swiftdeps
-    fingerprint:     afterachangewasmadetoswiftsource
+    fingerprint:     22222222222222222222222222222222
     sequenceNumber:  0
     defsIDependUpon: [ 2 ]
     isProvides:      true
@@ -15,7 +15,7 @@ allNodes:
       aspect:          implementation
       context:         ''
       name:            dependsonmain.swiftdeps
-    fingerprint:     afterachangewasmadetoswiftsource
+    fingerprint:     22222222222222222222222222222222
     sequenceNumber:  1
     defsIDependUpon: [  ]
     isProvides:      true

test/Driver/Dependencies/Inputs/fail-interface-hash-fine/depends-on-bad.swift

@@ -6,7 +6,7 @@ allNodes:
       aspect:          interface
       context:         ''
       name:            main.swiftdeps
-    fingerprint:     afterachangewasmadetoswiftsource
+    fingerprint:     22222222222222222222222222222222
     sequenceNumber:  0
     defsIDependUpon: [ 2 ]
     isProvides:      true
@@ -15,7 +15,7 @@ allNodes:
       aspect:          implementation
       context:         ''
       name:            main.swiftdeps
-    fingerprint:     afterachangewasmadetoswiftsource
+    fingerprint:     22222222222222222222222222222222
     sequenceNumber:  1
     defsIDependUpon: [  ]
     isProvides:      true

test/Driver/Dependencies/Inputs/fail-interface-hash-fine/depends-on-bad.swiftdeps

@@ -6,7 +6,7 @@ allNodes:
       aspect:          interface
       context:         ''
       name:            './bad.swiftdeps'
-    fingerprint:     afterachangewasmadetoswiftsource
+    fingerprint:     aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa
     sequenceNumber:  0
     defsIDependUpon: [ 2, 5, 4 ]
     isProvides:      true
@@ -15,7 +15,7 @@ allNodes:
       aspect:          implementation
       context:         ''
       name:            './bad.swiftdeps'
-    fingerprint:     afterachangewasmadetoswiftsource
+    fingerprint:     aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa
     sequenceNumber:  1
     defsIDependUpon: [  ]
     isProvides:      true

test/Driver/Dependencies/Inputs/fail-interface-hash-fine/depends-on-main.swift

@@ -6,7 +6,7 @@ allNodes:
       aspect:          interface
       context:         ''
       name:            './depends-on-bad.swiftdeps'
-    fingerprint:     afterachangewasmadetoswiftsource
+    fingerprint:     aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa
     sequenceNumber:  0
     defsIDependUpon: [ 2, 4 ]
     isProvides:      true
@@ -15,7 +15,7 @@ allNodes:
       aspect:          implementation
       context:         ''
       name:            './depends-on-bad.swiftdeps'
-    fingerprint:     afterachangewasmadetoswiftsource
+    fingerprint:     aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa
     sequenceNumber:  1
     defsIDependUpon: [  ]
     isProvides:      true

test/Driver/Dependencies/Inputs/fail-interface-hash-fine/depends-on-main.swiftdeps

@@ -6,7 +6,7 @@ allNodes:
       aspect:          interface
       context:         ''
       name:            './depends-on-main.swiftdeps'
-    fingerprint:     afterachangewasmadetoswiftsource
+    fingerprint:     aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa
     sequenceNumber:  0
     defsIDependUpon: [ 2, 4 ]
     isProvides:      true
@@ -15,7 +15,7 @@ allNodes:
       aspect:          implementation
       context:         ''
       name:            './depends-on-main.swiftdeps'
-    fingerprint:     afterachangewasmadetoswiftsource
+    fingerprint:     aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa
     sequenceNumber:  1
     defsIDependUpon: [  ]
     isProvides:      true

test/Driver/Dependencies/Inputs/fail-interface-hash-fine/main.swift

@@ -6,7 +6,7 @@ allNodes:
       aspect:          interface
       context:         ''
       name:            './depends-on-main.swiftdeps'
-    fingerprint:     afterachangewasmadetoswiftsource
+    fingerprint:     aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa
     sequenceNumber:  0
     defsIDependUpon: [ 2, 4 ]
     isProvides:      true
@@ -15,7 +15,7 @@ allNodes:
       aspect:          implementation
       context:         ''
       name:            './depends-on-main.swiftdeps'
-    fingerprint:     afterachangewasmadetoswiftsource
+    fingerprint:     aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa
     sequenceNumber:  1
     defsIDependUpon: [  ]
     isProvides:      true

test/Driver/Dependencies/Inputs/fail-interface-hash-fine/main.swiftdeps

@@ -6,7 +6,7 @@ allNodes:
       aspect:          interface
       context:         ''
       name:            main.swiftdeps
-    fingerprint:     afterachangewasmadetoswiftsource
+    fingerprint:     22222222222222222222222222222222
     sequenceNumber:  0
     defsIDependUpon: [ 4, 8, 2, 7, 6 ]
     isProvides:      true
@@ -15,7 +15,7 @@ allNodes:
       aspect:          implementation
       context:         ''
       name:            main.swiftdeps
-    fingerprint:     afterachangewasmadetoswiftsource
+    fingerprint:     22222222222222222222222222222222
     sequenceNumber:  1
     defsIDependUpon: [  ]
     isProvides:      true

test/Driver/Dependencies/Inputs/fail-with-bad-deps-fine/bad.swift

@@ -6,7 +6,7 @@ allNodes:
       aspect:          interface
       context:         ''
       name:            other.swiftdeps
-    fingerprint:     sameasiteverwassameasiteverwassa
+    fingerprint:     11111111111111111111111111111111
     sequenceNumber:  0
     defsIDependUpon: [ 8, 7, 2, 4, 6 ]
     isProvides:      true
@@ -15,7 +15,7 @@ allNodes:
       aspect:          implementation
       context:         ''
       name:            other.swiftdeps
-    fingerprint:     sameasiteverwassameasiteverwassa
+    fingerprint:     11111111111111111111111111111111
     sequenceNumber:  1
     defsIDependUpon: [  ]
     isProvides:      true

test/Driver/Dependencies/Inputs/fail-with-bad-deps-fine/bad.swiftdeps

@@ -6,7 +6,7 @@ allNodes:
       aspect:          interface
       context:         ''
       name:            does-change.swiftdeps
-    fingerprint:     afterachangewasmadetoswiftsource
+    fingerprint:      22222222222222222222222222222222 
     sequenceNumber:  0
     defsIDependUpon: [ 4, 8, 2, 7, 6 ]
     isProvides:      true
@@ -15,7 +15,7 @@ allNodes:
       aspect:          implementation
       context:         ''
       name:            does-change.swiftdeps
-    fingerprint:     afterachangewasmadetoswiftsource
+    fingerprint:      22222222222222222222222222222222 
     sequenceNumber:  1
     defsIDependUpon: [  ]
     isProvides:      true

test/Driver/Dependencies/Inputs/fail-with-bad-deps-fine/depends-on-bad.swift

@@ -6,7 +6,7 @@ allNodes:
       aspect:          interface
       context:         ''
       name:            does-not-change.swiftdeps
-    fingerprint:     sameasiteverwassameasiteverwassa
+    fingerprint:       11111111111111111111111111111111
     sequenceNumber:  0
     defsIDependUpon: [ 8, 7, 2, 4, 6 ]
     isProvides:      true
@@ -15,7 +15,7 @@ allNodes:
       aspect:          implementation
       context:         ''
       name:            does-not-change.swiftdeps
-    fingerprint:     sameasiteverwassameasiteverwassa
+    fingerprint:       11111111111111111111111111111111
     sequenceNumber:  1
     defsIDependUpon: [  ]
     isProvides:      true

test/Driver/Dependencies/Inputs/fail-with-bad-deps-fine/depends-on-bad.swiftdeps

@@ -1,22 +1,22 @@
-// XFAIL: linux, openbsd
+// XFAIL: linux, openbsd, windows
 
 // RUN: %empty-directory(%t)
 // RUN: cp -r %S/Inputs/only-skip-once/* %t
 // RUN: touch -t 201401240005 %t/*
 
-// RUN: cd %t && %target-swiftc_driver -driver-show-job-lifecycle -output-file-map %t/output-file-map.json -incremental main.swift file1.swift file2.swift -j1 2>%t/stderr.txt | %FileCheck -check-prefix=CHECK-INITIAL %s
+// RUN: cd %t && %target-swiftc_driver -driver-show-job-lifecycle -output-file-map %t/output-file-map.json -incremental main.swift file1.swift file2.swift -j1 2>&1 | tee /tmp/x | %FileCheck -check-prefix=CHECK-INITIAL %s
 
-// CHECK-INITIAL: Job finished: {compile: main.o <= main.swift}
-// CHECK-INITIAL: Job finished: {compile: file1.o <= file1.swift}
-// CHECK-INITIAL: Job finished: {compile: file2.o <= file2.swift}
-// CHECK-INITIAL: Job finished: {link: main <= main.o file1.o file2.o}
+// CHECK-INITIAL: {{(Job finished: {compile: main.o <= main.swift}|Finished Compiling main.swift)}}
+// CHECK-INITIAL: {{(Job finished: {compile: file1.o <= file1.swift}|Finished Compiling file1.swift)}}
+// CHECK-INITIAL: {{(Job finished: {compile: file2.o <= file2.swift}|Finished Compiling file2.swift)}}
+// CHECK-INITIAL: {{(Job finished: {link: main <= main.o file1.o file2.o}|Finished Linking main)}}
 
 // RUN: touch -t 201401240006 %t/file2.swift
-// RUN: cd %t && %target-swiftc_driver -driver-show-job-lifecycle -output-file-map %t/output-file-map.json -incremental main.swift file1.swift file2.swift -j1 2>%t/stderr.txt | %FileCheck -check-prefix=CHECK-REBUILD %s
+// RUN: cd %t && %target-swiftc_driver -driver-show-job-lifecycle -output-file-map %t/output-file-map.json -incremental main.swift file1.swift file2.swift -j1 2>&1 |tee /tmp/y |  %FileCheck -check-prefix=CHECK-REBUILD %s
 
 // We should skip the main and file1 rebuilds here, but we should only note skipping them _once_
-// CHECK-REBUILD: Job finished: {compile: file2.o <= file2.swift}
-// CHECK-REBUILD: Job skipped: {compile: main.o <= main.swift}
-// CHECK-REBUILD: Job skipped: {compile: file1.o <= file1.swift}
-// CHECK-REBUILD: Job finished: {link: main <= main.o file1.o file2.o}
-// CHECK-REBUILD-NOT: Job skipped:
+// CHECK-REBUILD-DAG: {{(Job finished: {compile: file2.o <= file2.swift}|Finished Compiling file2.swift)}}
+// CHECK-REBUILD-DAG: {{(Job skipped: {compile: main.o <= main.swift}|Skipped Compiling main.swift)}}
+// CHECK-REBUILD-DAG: {{(Job skipped: {compile: file1.o <= file1.swift}|Skipped Compiling file1.swift)}}
+// CHECK-REBUILD-DAG: {{(Job finished: {link: main <= main.o file1.o file2.o}|Finished Linking main)}}
+// CHECK-REBUILD-NOT: {{(Job skipped:|Skipped)}}

test/Driver/Dependencies/Inputs/fail-with-bad-deps-fine/depends-on-main.swift

@@ -69,7 +69,12 @@ template <> struct ScalarTraits<swift::Fingerprint> {
     os << fp.getRawValue();
   }
   static StringRef input(StringRef s, void *, swift::Fingerprint &fp) {
-    fp = swift::Fingerprint::fromString(s);
+    if (auto convertedFP = swift::Fingerprint::fromString(s))
+      fp = convertedFP.getValue();
+    else {
+      llvm::errs() << "Failed to convert fingerprint '" << s << "'\n";
+      exit(1);
+    }
     return StringRef();
   }
   static QuotingType mustQuote(StringRef S) { return needsQuotes(S); }