[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 ↗

(On Diff #147142)

Sort

198 ↗

(On Diff #147142)

Ditto

lld/COFF/Driver.cpp

1037 ↗

(On Diff #147142)

nit: add {}

1038–1039 ↗

(On Diff #147142)

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 ↗

(On Diff #147142)

Do you need this temporary variable?

[zturner]

lld/COFF/Writer.cpp

1195 ↗

(On Diff #147142)

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 ↗

(On Diff #147146)

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 ↗

(On Diff #147146)

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 ↗

(On Diff #147146)

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 ↗

(On Diff #147166)

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

lld/COFF/Options.td

61 ↗

(On Diff #147166)

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

lld/test/COFF/timestamp.test

9 ↗

(On Diff #147166)

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/trunk/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 Repro = false;
 };
 
 extern Configuration *Config;
 
 } // namespace coff
 } // namespace lld
 
 #endif

lld/trunk/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 (llvm::opt::Arg *Arg = Args.getLastArg(OPT_timestamp, OPT_repro)) {
+    if (Arg->getOption().getID() == OPT_repro) {
+      Config->Timestamp = 0;
+      Config->Repro = true;
+    } else {
+      Config->Repro = false;
+      StringRef Value(Arg->getValue());
+      if (Value.getAsInteger(0, Config->Timestamp))
+        fatal(Twine("invalid timestamp: ") + Value +
+              ".  Expected 32-bit integer");
+    }
+  } else {
+    Config->Repro = 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());
 

lld/trunk/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 PE header timestamp">;
 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">;

 def debug : F<"debug">, HelpText<"Embed a symbol table in the image">;
 def debug_full : F<"debug:full">, Alias<debug>;
 def debugtype : P<"debugtype", "Debug Info Options">;
 def dll : F<"dll">, HelpText<"Create a DLL">;
 def driver : P<"driver", "Generate a Windows NT Kernel Mode Driver">;
 def nodefaultlib_all : F<"nodefaultlib">;
 def noentry : F<"noentry">;
 def profile : F<"profile">;
+def repro : F<"Brepro">, HelpText<"Use a hash of the executable as the PE header timestamp">;
 def swaprun_cd : F<"swaprun:cd">;
 def swaprun_net : F<"swaprun:net">;
 def verbose : F<"verbose">;
 def wholearchive_flag : F<"wholearchive">;
 
 def force : F<"force">,
     HelpText<"Allow undefined symbols when creating executables">;
 def force_unresolved : F<"force:unresolved">;

lld/trunk/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->Repro)
+    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/trunk/test/COFF/timestamp.test

+rm %t.*.exe
+RUN: yaml2obj %p/Inputs/generic.yaml > %t.obj
+RUN: lld-link %t.obj /debug /Brepro /entry:main /nodefaultlib /out:%t.1.exe
+RUN: lld-link %t.obj /debug /Brepro /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)