[PCH] Disable inclusion of timestamps when generating pch files on windows.

[pgousseau]

Summary

On Linux, if a header file included in the pch is modified then a fatal error is emitted, which is reasonable.
On Windows the check is ifdefed out, allowing the compilation to continue in a broken state.
This leads to "#pragma once" directives to be ignored.
This change disables the inclusion of timestamps in pch files on Windows, for now.

[probinson]

Please make sure the test files have newlines at the end.
"touch-pragma-once.cpp" should probably be named something like "pragma-once-timestamp.cpp".

[pgousseau]

In D20243#429811, @probinson wrote:

Please make sure the test files have newlines at the end.
"touch-pragma-once.cpp" should probably be named something like "pragma-once-timestamp.cpp".

Will fix thanks!

[pgousseau]

Add newlines and change test's name following Paul's comments.

[probinson]

This is fine as far as I'm concerned, but I think somebody more familiar with the area ought to chime in.

[cameron314]

I've seen rare cases where parses using the PCH files were yielding completely invalid data, almost as if they were corrupted. I wonder now if this could be the cause. (We're on Windows too.)

[thakis]

Did you see http://reviews.llvm.org/D19815 ? Does that help? Warren might have opinions on this.

[wristow]

In D20243#433615, @thakis wrote:

Did you see http://reviews.llvm.org/D19815 ? Does that help? Warren might have opinions on this.

Yes, these are definitely related. Fixing that other problem does not also fix the issue here, however the fix here will only work if that other fix is also applied.

In my view, that other issue is a fairly overt bug (PR24387) where #pragma once is blatantly ignored in a header-file processed for creating a PCH. That can result in incorrect behavior, and always results in a misleading/confusing warning. That issue also is independent of the host environment.

Whereas the timestamp issue described here is much more subtle, and it only impacts Windows hosts (due to some problem with the timestamps on the Windows file system).

Essentially, the proposed fix here is blocked by that other issue. I should have realized and commented here earlier. Thanks for bringing it to my attention.

[pgousseau]

In D20243#433615, @thakis wrote:

Did you see http://reviews.llvm.org/D19815 ? Does that help? Warren might have opinions on this.

Yes thanks, I agree with Warren, this is a separate issue.
In the test I am adding I avoid the issue that Warren's review is for by only using '#pragma once' in a sub header of the PCH.

[bruno]

Hi Pierre,

test/PCH/pragma-once-timestamp.cpp

18

Can you move this to the beginning of the file? It makes it easier to spot that this is windows only.

[thakis]

Hm, the ASTReader code this works around is over 6 years old (r100866). Maybe we could try enabling the access time check instead?

[pgousseau]

Moving REQUIRE line higher following Bruno's comments.

[pgousseau]

In D20243#437192, @thakis wrote:

Hm, the ASTReader code this works around is over 6 years old (r100866). Maybe we could try enabling the access time check instead?

Yes the issue described in ASTReader lacks a reproducible so it makes sense to tentatively re-enabling the timestamp check on windows. If the issue still occurs on some bots then at least we can document the issue better before re-disabling it.

I also would like to provide a switch to disable inclusion of timestamps. I will hopefully be proposing a patch for it soon.

[pgousseau]

Superseded by http://reviews.llvm.org/D20867

lib/Frontend/FrontendActions.cpp

GeneratePCHAction::CreateASTConsumer(CompilerInstance &CI, StringRef InFile) {
   if (!OS)
     return nullptr;
 
   if (!CI.getFrontendOpts().RelocatablePCH)
     Sysroot.clear();
 
   auto Buffer = std::make_shared<PCHBuffer>();
   std::vector<std::unique_ptr<ASTConsumer>> Consumers;
+  // FIXME: There is a known issue with timestamps appearing to be inconsistent
+  // on Windows (c.f. ASTReader::getInputFile) so we disable timestamps checks
+  // on Windows for now.
+  llvm::Triple HostTriple(LLVM_HOST_TRIPLE);
   Consumers.push_back(llvm::make_unique<PCHGenerator>(
                         CI.getPreprocessor(), OutputFile, nullptr, Sysroot,
-                        Buffer, CI.getFrontendOpts().ModuleFileExtensions));
+                        Buffer, CI.getFrontendOpts().ModuleFileExtensions,
+                        /*AllowASTWithErrors*/false,
+                        /*IncludeTimestamps*/!HostTriple.isOSWindows()));
   Consumers.push_back(CI.getPCHContainerWriter().CreatePCHContainerGenerator(
       CI, InFile, OutputFile, OS, Buffer));
 
   return llvm::make_unique<MultiplexConsumer>(std::move(Consumers));
 }
 
 raw_pwrite_stream *GeneratePCHAction::ComputeASTConsumerArguments(
     CompilerInstance &CI, StringRef InFile, std::string &Sysroot,

lib/Serialization/ASTReader.cpp

InputFile ASTReader::getInputFile(ModuleFile &F, unsigned ID, bool Complain) {
   if (!Overridden && //
       (StoredSize != File->getSize() ||
 #if defined(LLVM_ON_WIN32)
        false
 #else
        // In our regression testing, the Windows file system seems to
        // have inconsistent modification times that sometimes
        // erroneously trigger this error-handling path.
+       // For now timestamps are disabled for pch files on Windows (c.f
+       // GeneratePCHAction::CreateASTConsumer).
        //
        // FIXME: This probably also breaks HeaderFileInfo lookups on Windows.
        (StoredTime && StoredTime != File->getModificationTime() &&
         !DisableValidation)
 #endif
        )) {
     if (Complain) {
       // Build a list of the PCH imports that got us here (in reverse).

test/PCH/Inputs/pragma-once2-pch.h

+#include "pragma-once2.h"
+

test/PCH/Inputs/pragma-once2.h

+#pragma once
+
+inline void f() {}
+

test/PCH/pragma-once-timestamp.cpp

+// On Windows, timestamps for pch are not handled correctly.
+// This would cause pragma once to be ignored on distributed builds.
+// pragma-once2-pch.h includes pragma-once2.h which has a pragma once directive.
+// pragma-once2.h is then touched before using the generated pch.
+// On Linux this will cause an expected error, but on Windows we want to
+// ignore the timestamp as the timestamp handling on Windows is
+// inconsistent at the moment.
+
+// REQUIRES: system-windows
+
+// Test this without pch.
+// RUN: %clang_cc1 -include %S/Inputs/pragma-once2-pch.h -fsyntax-only -verify %s
+
+// Test with pch.
+// RUN: %clang_cc1 -emit-pch -o %t -x c++-header %S/Inputs/pragma-once2-pch.h
+// RUN: touch %S/Inputs/pragma-once2.h
+// RUN: %clang_cc1 -include-pch %t -fsyntax-only -verify %s
+
+// expected-no-diagnostics
+
+#include "Inputs/pragma-once2.h"
+
+void g() { f(); }
+