Make timestamp writing in WinCOFFObjectWriter.cpp independent of ENABLE_TIMESTAMPS

[thakis]

Summary

LLVM_ENABLE_TIMESTAMPS controls if timestamps are embedded into llvm's binaries. Turning it off is useful for deterministic builds.

r246905 made it so that the define suddenly also controls if the binaries that the llvm binaries _create_ embed timestamps or not – but this shouldn't be a configure-time option. r256203/r256204 added a driver option to toggle this on and off, so this patch now passes this driver option in LLVM_ENABLE_TIMESTAMPS builds so that if LLVM_ENABLE_TIMESTAMPS is set, the build of LLVM is deterministic – but the built clang can still write timestamps into other executables when requested.

This also allows removing some of the test machinery added in r292012 to work around this problem.

See PR24740 for background.

[thakis]

A new version of http://reviews.llvm.org/D15782 since I didn't add llvm-commits correctly there.

I didn't test the cmake goop yet, but it should be roughly correct.

[thakis]

cmake/modules/HandleLLVMOptions.cmake

371

Turns out this doesn't work since cl.exe only warns about unknown options (D9002) and it looks like there's no way to turn this into an error :-/

[thakis]

Ok, please take a look. I checked that this does what I expect with cl.exe, clang-cl.exe that does understand /Brepro, clang-cl.exe that does understand /Brepro with an explicit -DCMAKE_EXE_LINKER_FLAGS=/incremental, and clang-cl.exe that does not yet understand /Brepro.

[majnemer]

LGTM

[thakis]

r256958, thanks!

cmake/modules/HandleLLVMOptions.cmake

if( MSVC )
   endif (LLVM_ENABLE_WERROR)
 
   foreach(flag ${msvc_warning_flags})
     append("${flag}" CMAKE_C_FLAGS CMAKE_CXX_FLAGS)
   endforeach(flag)
 
   append("/Zc:inline" CMAKE_C_FLAGS CMAKE_CXX_FLAGS)
 
+  if (NOT LLVM_ENABLE_TIMESTAMPS AND CMAKE_CXX_COMPILER_ID MATCHES "Clang")
+    # clang-cl and cl by default produce non-deterministic binaries because
+    # link.exe /incremental requires a timestamp in the .obj file.  clang-cl
+    # has the flag /Brepro to force deterministic binaries, so pass that when
+    # LLVM_ENABLE_TIMESTAMPS is turned off.
+    # This checks CMAKE_CXX_COMPILER_ID in addition to check_cxx_compiler_flag()
+    # because cl.exe does not emit an error on flags it doesn't understand,
+    # letting check_cxx_compiler_flag() claim it understands all flags.
+    check_cxx_compiler_flag("/Brepro" SUPPORTS_BREPRO)
+    append_if(SUPPORTS_BREPRO "/Brepro" CMAKE_C_FLAGS CMAKE_CXX_FLAGS)
+
+    if (SUPPORTS_BREPRO)
+      # Check if /INCREMENTAL is passed to the linker and complain that it
+      # won't work with /Brepro.
+      string(TOUPPER "${CMAKE_EXE_LINKER_FLAGS}" upper_exe_flags)
+      string(TOUPPER "${CMAKE_MODULE_LINKER_FLAGS}" upper_module_flags)
+      string(TOUPPER "${CMAKE_SHARED_LINKER_FLAGS}" upper_shared_flags)
+
+      string(FIND "${upper_exe_flags}" "/INCREMENTAL" exe_index)
+      string(FIND "${upper_module_flags}" "/INCREMENTAL" module_index)
+      string(FIND "${upper_shared_flags}" "/INCREMENTAL" shared_index)
+      
+      if (${exe_index} GREATER -1 OR
+          ${module_index} GREATER -1 OR
+          ${shared_index} GREATER -1)
+        message(FATAL_ERROR "LLVM_ENABLE_TIMESTAMPS not compatible with /INCREMENTAL linking")
+      endif()
+    endif()
+  endif()
+
   # Disable sized deallocation if the flag is supported. MSVC fails to compile
   # the operator new overload in User otherwise.
   check_c_compiler_flag("/WX /Zc:sizedDealloc-" SUPPORTS_SIZED_DEALLOC)
   append_if(SUPPORTS_SIZED_DEALLOC "/Zc:sizedDealloc-" CMAKE_C_FLAGS CMAKE_CXX_FLAGS)
 
 elseif( LLVM_COMPILER_IS_GCC_COMPATIBLE )
   if (LLVM_ENABLE_WARNINGS)
     append("-Wall -W -Wno-unused-parameter -Wwrite-strings" CMAKE_C_FLAGS CMAKE_CXX_FLAGS)

lib/MC/WinCOFFObjectWriter.cpp

for (const auto &Section : Asm) {
     Aux.Aux.SectionDefinition.NumberOfRelocations =
         Sec->Header.NumberOfRelocations;
     Aux.Aux.SectionDefinition.NumberOfLinenumbers =
         Sec->Header.NumberOfLineNumbers;
   }
 
   Header.PointerToSymbolTable = offset;
 
-  // FIXME: Remove the #else branch and make the #if branch unconditional once
-  // LLVM's self host configuration is aware of /Brepro.
-#if (ENABLE_TIMESTAMPS == 1)
   // MS LINK expects to be able to use this timestamp to implement their
   // /INCREMENTAL feature.
   if (Asm.isIncrementalLinkerCompatible()) {
     std::time_t Now = time(nullptr);
     if (Now < 0 || !isUInt<32>(Now))
       Now = UINT32_MAX;
     Header.TimeDateStamp = Now;
   } else {
+    // Have deterministic output if /INCREMENTAL isn't needed. Also matches GNU.
     Header.TimeDateStamp = 0;
   }
-#else
-  // We want a deterministic output. It looks like GNU as also writes 0 in here.
-  Header.TimeDateStamp = 0;
-#endif
 
   // Write it all to disk...
   WriteFileHeader(Header);
 
   {
     sections::iterator i, ie;
     MCAssembler::iterator j, je;
 

test/MC/COFF/timestamp.s

 // RUN: llvm-mc -filetype=obj -triple i686-pc-win32 -incremental-linker-compatible %s -o - | llvm-readobj -h | FileCheck %s
-// REQUIRES: timestamps
 
 // CHECK: ImageFileHeader {
 // CHECK:   TimeDateStamp:
 // CHECK-NOT: 1970-01-01 00:00:00 (0x0)

test/lit.cfg

 if platform.system() in ['Windows'] and re.match(r'.*-win32$', config.target_triple):
     # For tests that require Windows to run.
     config.available_features.add('system-windows')
 
 # .debug_frame is not emitted for targeting Windows x64.
 if not re.match(r'^x86_64.*-(mingw32|windows-gnu|win32)', config.target_triple):
     config.available_features.add('debug_frame')
 
-# Check if we are embedding timestamps.
-if config.enable_timestamps == '1':
-    config.available_features.add('timestamps')
-
 # Check if we should use gmalloc.
 use_gmalloc_str = lit_config.params.get('use_gmalloc', None)
 if use_gmalloc_str is not None:
     if use_gmalloc_str.lower() in ('1', 'true'):
         use_gmalloc = True
     elif use_gmalloc_str.lower() in ('', '0', 'false'):
         use_gmalloc = False
     else:

test/lit.site.cfg.in

 config.host_cc = "@HOST_CC@"
 config.host_cxx = "@HOST_CXX@"
 config.host_ldflags = "@HOST_LDFLAGS@"
 config.llvm_use_intel_jitevents = "@LLVM_USE_INTEL_JITEVENTS@"
 config.llvm_use_sanitizer = "@LLVM_USE_SANITIZER@"
 config.have_zlib = "@HAVE_LIBZ@"
 config.have_dia_sdk = @HAVE_DIA_SDK@
 config.enable_ffi = "@LLVM_ENABLE_FFI@"
-config.enable_timestamps = "@ENABLE_TIMESTAMPS@"
 config.test_examples = "@ENABLE_EXAMPLES@"
 
 # Support substitution of the tools_dir with user parameters. This is
 # used when we can't determine the tool dir at configuration time.
 try:
     config.llvm_tools_dir = config.llvm_tools_dir % lit_config.params
     config.llvm_shlib_dir = config.llvm_shlib_dir % lit_config.params
 except KeyError:
     e = sys.exc_info()[1]
     key, = e.args
     lit_config.fatal("unable to find %r parameter, use '--param=%s=VALUE'" % (key,key))
 
 # Let the main config do the real work.
 lit_config.load_config(config, "@LLVM_SOURCE_DIR@/test/lit.cfg")