[lld] Use a real timestamp, with the option to use hash or explicit value

[zturner]

Summary

Using a deterministic value breaks Windows' AppCompat database in strange ways.

https://bugs.chromium.org/p/chromium/issues/detail?id=843199#29

for more information.

The TL;DR is that by default we need to write a real timestamp. We can still get build determinism without breaking AppCompat, but the application will need to specify an explicit value to use for the time stamp. So the method of always using a hash of the binary by default is changed to using the time stamp by default, but we add an option to use an explicit value or the old method of using a hash.

[ruiu]

lld/COFF/Config.h

184

Sort

198

Ditto

lld/COFF/Driver.cpp

1037

nit: add {}

1038–1039

Since Config members are global, you don't need to pass them. Maybe I'd just inline that function here.

lld/COFF/Writer.cpp

1195

Do you need this temporary variable?

[zturner]

lld/COFF/Writer.cpp

1195

No, but with a ternary operator I would have to use a line break, and I think it's easier to read with no line breaks. I can change it if you like though.

[ruiu]

lld/COFF/Driver.cpp

1041

This seems like a mistake. If your test passed with this patch, you should add a test to execute this path.

[zturner]

lld/COFF/Driver.cpp

1041

Actually my test didn't pass. I just didn't re-run them after I inlined the function. Thanks!

[pcc]

lld/COFF/Driver.cpp

1038

It looks like this is spelt /Brepro in the MSVC linker. Should we do the same?

[zturner]

We can do /Brepro but we do a little more than /Brepro. We allow
/timestamp:hash as well as setting an explicit value. We could keep it for
compatibility though and map it to /timestamp:0. Thoughts?

[pcc]

According to my experiments, the /Brepro linker flag does set the timestamp field to a hash (unlike the compiler flag which sets it to 0). So I was thinking we'd do:

• /Brepro would trigger the hashing behaviour
• /timestamp would only accept integers.

[pcc]

I also noticed that /Brepro causes link.exe to write a debug directory entry of type IMAGE_DEBUG_TYPE_REPRO with a larger hash. Now I'm curious as to whether the presence of this debug directory would disable the timestamp-dependent app compat behaviour from the bug.

[zturner]

Rename /TIMESTAMP:HASH to /Brepro for parity with link.exe

[pcc]

LGTM

[hans]

lld/COFF/Driver.cpp

1045

Maybe the error message could be clearer, indicating that it expects an integer?

lld/COFF/Options.td

61

Perhaps just the shorter "Specify the PE header timestamp" is enough?

lld/test/COFF/timestamp.test

10

Should there be a test for the default case, without /Brepro or /timestamp?

[hans]

(Please don't wait for me to land this though, as I'll be ooo for the next couple of days.)

[llvm-commits]

• msg-12284-175.txt162 BDownload

[thakis]

The test seems to fail on some bots: http://lab.llvm.org:8011/builders/clang-with-thin-lto-ubuntu/builds/10372

[zturner]

That is bizarre. It's the /Brepro code path that's failing, and this
codepath does exactly what it used to do, which didn't fail on any bots.
Any ideas?

[zturner]

Let's see if it fails with the same stack trace the next time around.
Given that it's only on the stage 4 (all 3 previous stages ran the same
test and passed), and given that the stack trace shows something about
threads, it could be some flake that has nothing to do with my change. If
it comes back with the same stack trace again though, then we can start
head scratching.

[zturner]

Seems like it was a flake on the buildbot, it's passing now.

lld/COFF/Config.h

struct Configuration {
   uint64_t StackReserve = 1024 * 1024;
   uint64_t StackCommit = 4096;
   uint64_t HeapReserve = 1024 * 1024;
   uint64_t HeapCommit = 4096;
   uint32_t MajorImageVersion = 0;
   uint32_t MinorImageVersion = 0;
   uint32_t MajorOSVersion = 6;
   uint32_t MinorOSVersion = 0;
+  uint32_t Timestamp = 0;
   bool DynamicBase = true;
   bool AllowBind = true;
   bool NxCompat = true;
   bool AllowIsolation = true;
   bool TerminalServerAware = true;
   bool LargeAddressAware = false;
   bool HighEntropyVA = false;
   bool AppContainer = false;
   bool MinGW = false;
   bool WarnMissingOrderSymbol = true;
   bool WarnLocallyDefinedImported = true;
   bool Incremental = true;
   bool KillAt = false;
+  bool TimestampIsHash = false;
 };
 
 extern Configuration *Config;
 
 } // namespace coff
 } // namespace lld
 
 #endif

lld/COFF/Driver.h

 
 // Parses a string in the form of "<integer>[.<integer>]".
 // Minor's default value is 0.
 void parseVersion(StringRef Arg, uint32_t *Major, uint32_t *Minor);
 
 // Parses a string in the form of "<subsystem>[,<integer>[.<integer>]]".
 void parseSubsystem(StringRef Arg, WindowsSubsystem *Sys, uint32_t *Major,
                     uint32_t *Minor);
-
+void parseTimestamp(StringRef Arg, uint32_t *Timestamp, bool *TimestampIsHash);
 void parseAlternateName(StringRef);
 void parseMerge(StringRef);
 void parseSection(StringRef);
 void parseAligncomm(StringRef);
 
 // Parses a string in the form of "EMBED[,=<integer>]|NO".
 void parseManifest(StringRef Arg);
 

lld/COFF/Driver.cpp

if (auto *Arg = Args.getLastArg(OPT_version))
     parseVersion(Arg->getValue(), &Config->MajorImageVersion,
                  &Config->MinorImageVersion);
 
   // Handle /subsystem
   if (auto *Arg = Args.getLastArg(OPT_subsystem))
     parseSubsystem(Arg->getValue(), &Config->Subsystem, &Config->MajorOSVersion,
                    &Config->MinorOSVersion);
 
+  // Handle /timestamp
+  if (auto *Arg = Args.getLastArg(OPT_timestamp))
+    parseTimestamp(Arg->getValue(), &Config->Timestamp,
+                   &Config->TimestampIsHash);
+  else {
+    Config->TimestampIsHash = false;
+    Config->Timestamp = time(nullptr);
+  }
+
   // Handle /alternatename
   for (auto *Arg : Args.filtered(OPT_alternatename))
     parseAlternateName(Arg->getValue());
 
   // Handle /include
   for (auto *Arg : Args.filtered(OPT_incl))
     addUndefined(Arg->getValue());
 
   // Handle /implib

lld/COFF/DriverUtils.cpp

*Sys = StringSwitch<WindowsSubsystem>(SysStr.lower())
     .Case("windows", IMAGE_SUBSYSTEM_WINDOWS_GUI)
     .Default(IMAGE_SUBSYSTEM_UNKNOWN);
   if (*Sys == IMAGE_SUBSYSTEM_UNKNOWN)
     fatal("unknown subsystem: " + SysStr);
   if (!Ver.empty())
     parseVersion(Ver, Major, Minor);
 }
 
+void parseTimestamp(StringRef Arg, uint32_t *Timestamp, bool *TimestampIsHash) {
+  if (Arg.equals_lower("hash")) {
+    *Timestamp = 0;
+    *TimestampIsHash = true;
+    return;
+  }
+
+  *TimestampIsHash = false;
+  if (Arg.getAsInteger(0, *Timestamp))
+    fatal("invalid timestamp: " + Arg);
+}
+
 // Parse a string of the form of "<from>=<to>".
 // Results are directly written to Config.
 void parseAlternateName(StringRef S) {
   StringRef From, To;
   std::tie(From, To) = S.split('=');
   if (From.empty() || To.empty())
     fatal("/alternatename: invalid argument: " + S);
   auto It = Config->AlternateNames.find(From);

lld/COFF/Options.td

 def no_color_diagnostics: F<"no-color-diagnostics">,
   HelpText<"Do not use colors in diagnostics">;
 def pdb : P<"pdb", "PDB file path">;
 def pdbaltpath : P<"pdbaltpath", "PDB file path to embed in the image">;
 def section : P<"section", "Specify section attributes">;
 def stack   : P<"stack", "Size of the stack">;
 def stub    : P<"stub", "Specify DOS stub file">;
 def subsystem : P<"subsystem", "Specify subsystem">;
+def timestamp : P<"timestamp", "Specify the timestamp to be written to the PE header">;
 def version : P<"version", "Specify a version number in the PE header">;
 def wholearchive_file : P<"wholearchive", "Include all object files from this archive">;
 
 def disallowlib : Joined<["/", "-", "-?"], "disallowlib:">, Alias<nodefaultlib>;
 
 def manifest : F<"manifest">;
 def manifest_colon : P<"manifest", "Create manifest file">;
 def manifestuac : P<"manifestuac", "User access control">;

lld/COFF/Writer.cpp

void Writer::writeBuildId() {
   // At this point the only fields in the COFF file which remain unset are the
   // "timestamp" in the COFF file header, and the ones in the coff debug
   // directory.  Now we can hash the file and write that hash to the various
   // timestamp fields in the file.
   StringRef OutputFileData(
       reinterpret_cast<const char *>(Buffer->getBufferStart()),
       Buffer->getBufferSize());
 
-  uint32_t Hash = static_cast<uint32_t>(xxHash64(OutputFileData));
+  uint32_t Timestamp = Config->Timestamp;
+  if (Config->TimestampIsHash)
+    Timestamp = static_cast<uint32_t>(xxHash64(OutputFileData));
 
   if (DebugDirectory)
-    DebugDirectory->setTimeDateStamp(Hash);
+    DebugDirectory->setTimeDateStamp(Timestamp);
 
   uint8_t *Buf = Buffer->getBufferStart();
   Buf += DOSStubSize + sizeof(PEMagic);
   object::coff_file_header *CoffHeader =
       reinterpret_cast<coff_file_header *>(Buf);
-  CoffHeader->TimeDateStamp = Hash;
+  CoffHeader->TimeDateStamp = Timestamp;
 }
 
 // Sort .pdata section contents according to PE/COFF spec 5.5.
 void Writer::sortExceptionTable() {
   if (!FirstPdata)
     return;
   // We assume .pdata contains function table entries only.
   auto BufAddr = [&](Chunk *C) {

lld/test/COFF/timestamp.test

+rm %t.*.exe
+RUN: yaml2obj %p/Inputs/generic.yaml > %t.obj
+RUN: lld-link %t.obj /debug /timestamp:hash /entry:main /nodefaultlib /out:%t.1.exe
+RUN: lld-link %t.obj /debug /timestamp:hash /entry:main /nodefaultlib /out:%t.2.exe
+RUN: lld-link %t.obj /debug /timestamp:0 /entry:main /nodefaultlib /out:%t.3.exe
+RUN: llvm-readobj -file-headers -coff-debug-directory %t.1.exe | FileCheck %s --check-prefix=HASH
+RUN: llvm-readobj -file-headers -coff-debug-directory %t.2.exe | FileCheck %s --check-prefix=HASH
+RUN: llvm-readobj -file-headers -coff-debug-directory %t.3.exe | FileCheck %s --check-prefix=ZERO
+
+HASH: ImageFileHeader {
+HASH: TimeDateStamp: [[STAMP:.*]]
+HASH: DebugDirectory [
+HASH: TimeDateStamp: [[STAMP]]
+
+ZERO: ImageFileHeader {
+ZERO: TimeDateStamp: 1970-01-01 00:00:00 (0x0)
+ZERO: DebugDirectory [
+ZERO: TimeDateStamp: 1970-01-01 00:00:00 (0x0)