[PCH] Fix timestamp check on windows hosts.

[pgousseau]

Summary

On Linux, if the timestamp of a header file, included in the pch, is modified, then including the pch without regenerating it causes a fatal error, which is reasonable.
On Windows the check is ifdefed out, allowing the compilation to continue in a broken state.
The root of the broken state is that, if timestamps dont match, the preprocessor will reparse a header without discarding the pch data.
This leads to "#pragma once" header to be included twice.
The reason behind the ifdefing of the check lacks documentation, and was done 6 years ago.
This change tentatively removes the ifdefing and adds a cc1 option to disable the inclusion of timestamps in pch files, giving some flexibility to build systems such as distributed builds.

This change is a follow up to the discussion started in http://reviews.llvm.org/D20243

[bruno]

test/PCH/include-timestamp.cpp

5 ↗

(On Diff #59225)

Can you also check the timestamp by looking at llvm-bcanalyzer output (assuming the tool outputs such info)?

7 ↗

(On Diff #59225)

Why use sleep 1 here?

16 ↗

(On Diff #59225)

It make sense for the context of this patch, but if http://reviews.llvm.org/D20243 gets in, you should re-use the same headers you're adding there.

[pgousseau]

Following Bruno's comments:

• Update input files' name to match D20243 review.
• Add llvm-bcanalyzer checks

[pgousseau]

test/PCH/include-timestamp.cpp

8 ↗

(On Diff #59355)

Without the sleep the test fails for me, as it seems the call to touch does not have time to take effect. It might be something specific to gnuwin32 tools on Windows I am not sure.

[bruno]

test/PCH/include-timestamp.cpp

8 ↗

(On Diff #59355)

What about using more options for touch, something like touch -m -a -t <timestamp> <file>?

[pgousseau]

Following Bruno's comment:

• Remove call to sleep using touch -m -a -t

[pgousseau]

On Linux, if the timestamp of a header file, included in the pch, is modified, then including the pch without regenerating it causes a fatal error, which is reasonable.
On Windows the check is ifdefed out, allowing the compilation to continue in a broken state.
The root of the broken state is that, if timestamps dont match, the preprocessor will reparse a header without discarding the pch data.
This leads to "#pragma once" header to be included twice.
The reason behind the ifdefing of the check lacks documentation, and was done 6 years ago.

It's documented in the comment you deleted:

// In our regression testing, the Windows file system seems to
// have inconsistent modification times that sometimes
// erroneously trigger this error-handling path.

We should confirm whether this is in fact still the case. Maybe this is caused by building on a networked file system, where a locally-changed file might have a different mtime locally and remotely (the local mtime may be precise where the remote one has been rounded to a multiple of 2 seconds by an underlying FAT filesystem)? If it's something like that, we could perhaps work around this by rounding the mtime to a multiple of 2 seconds ourselves.

I am not sure how to reproduce this build scenario, would you be able to provide some more stepped details?
I have tried emitting and including a PCH on a networked FAT32 drive, but no false warnings observed so far.

Are you asking for the 2 seconds tolerance to be part of this patch?
Or, as it seems the main problem here is the lack of a reproducible, are you ok with the idea of committing this patch first, to see if the inconsistent time stamps is still an issue?

[pgousseau]

Are you asking for the 2 seconds tolerance to be part of this patch?
Or, as it seems the main problem here is the lack of a reproducible, are you ok with the idea of committing this patch first, to see if the inconsistent time stamps is still an issue?

Yes, let's commit a patch to remove the #if first, and see if anything actually breaks (and if so, what). There's no need to add an option for this we have no uses for it.

Sounds good, does it mean it is ok to commit?
Sorry, regarding the option, not sure if you are saying: "if we have no uses for it" or "as we have no uses for it"?
The idea behind the option is to allow distributed builds system, that do not preserve time stamps, to use PCH files.

[pgousseau]

Ping!

[pgousseau]

Ping!

[pgousseau]

Ping!

[pgousseau]

Ping!

[rsmith]

You have two independent functional changes in this patch: one adds a flag to control the emission of timestamps into PCH files, and the other re-enables timestamp checking on Win32. Please separate them out into distinct patches to be committed separately.

Both parts of this LGTM.

[pgousseau]

In D20867#482465, @rsmith wrote:

You have two independent functional changes in this patch: one adds a flag to control the emission of timestamps into PCH files, and the other re-enables timestamp checking on Win32. Please separate them out into distinct patches to be committed separately.

Both parts of this LGTM.

Splitting the patch sounds good, thanks Richard and Bruno for reviewing, will push soon.

cfe/trunk/lib/Serialization/ASTReader.cpp

FileMgr.modifyFileEntry(const_cast<FileEntry*>(File),
                             StoredSize, StoredTime);
   }
 
   bool IsOutOfDate = false;
 
   // For an overridden file, there is nothing to validate.
   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.
-       //
-       // 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).
       SmallVector<ModuleFile *, 4> ImportStack(1, &F);
       while (ImportStack.back()->ImportedBy.size() > 0)
         ImportStack.push_back(ImportStack.back()->ImportedBy[0]);
 
       // The top-level PCH is stale.