[Modules] Process include files changes with -fmodules-validate-input-files-content and -fno-pch-timestamp options

[ivanmurashko]

There were two diffs that introduced some options useful when you build modules externally and cannot rely on file modification time as the key for detecting input file changes:

• D67249 introduced the -fmodules-validate-input-files-content option, which allows the use of file content hash in addition to the modification time.
• D141632 propagated the use of -fno-pch-timestamps with Clang modules.

There is a problem when the size of the input file (header) is not modified but the content is. In this case, Clang cannot detect the file change when the -fno-pch-timestamps option is used. The -fmodules-validate-input-files-content option should help, but there is an issue with its application: it's not applied when the modification time is stored as zero that is the case for -fno-pch-timestamps.

The issue can be fixed using the same trick that was applied during the processing of ForceCheckCXX20ModulesInputFiles:

  // When ForceCheckCXX20ModulesInputFiles and ValidateASTInputFilesContent
  // enabled, it is better to check the contents of the inputs. Since we can't
  // get correct modified time information for inputs from overriden inputs.
  if (HSOpts.ForceCheckCXX20ModulesInputFiles && ValidateASTInputFilesContent &&
      F.StandardCXXModule && FileChange.Kind == Change::None)
    FileChange = HasInputContentChanged(FileChange);

The patch suggests the solution similar to the presented above and includes a LIT test to verify it.

[llvmbot]

@llvm/pr-subscribers-clang-modules

@llvm/pr-subscribers-clang

Author: Ivan Murashko (ivanmurashko)

Changes

There were two diffs that introduced some options useful when you build modules externally and cannot rely on file modification time as the key for detecting input file changes:

• D67249 introduced the -fmodules-validate-input-files-content option, which allows the use of file content hash in addition to the modification time.
• D141632 propagated the use of -fno-pch-timestamps with Clang modules.

There is a problem when the size of the input file (header) is not modified but the content is. In this case, Clang cannot detect the file change when the -fno-pch-timestamps option is used. The -fmodules-validate-input-files-content option should help, but there is an issue with its application: it's not applied when the modification time is stored as zero that is the case for -fno-pch-timestamps.

The issue can be fixed using the same trick that was applied during the processing of ForceCheckCXX20ModulesInputFiles. The patch suggests the corresponding solution and includes a LIT test to verify it.

---

Full diff: https://github.com/llvm/llvm-project/pull/90319.diff

2 Files Affected:

• (modified) clang/lib/Serialization/ASTReader.cpp (+8)
• (added) clang/test/Modules/implicit-module-no-timestamp.cpp (+33)

diff --git a/clang/lib/Serialization/ASTReader.cpp b/clang/lib/Serialization/ASTReader.cpp
index 0ef57a3ea804ef..4609ca3e1428c8 100644
--- a/clang/lib/Serialization/ASTReader.cpp
+++ b/clang/lib/Serialization/ASTReader.cpp
@@ -2630,6 +2630,14 @@ InputFile ASTReader::getInputFile(ModuleFile &F, unsigned ID, bool Complain) {
       F.StandardCXXModule && FileChange.Kind == Change::None)
     FileChange = HasInputContentChanged(FileChange);
 
+  // When we have StoredTime equal to zero and ValidateASTInputFilesContent,
+  // it is better to check the content of the input files because we cannot rely
+  // on the file modification time, which will be the same (zero) for these
+  // files.
+  if (!StoredTime && ValidateASTInputFilesContent &&
+      FileChange.Kind == Change::None)
+    FileChange = HasInputContentChanged(FileChange);
+
   // For an overridden file, there is nothing to validate.
   if (!Overridden && FileChange.Kind != Change::None) {
     if (Complain && !Diags.isDiagnosticInFlight()) {
diff --git a/clang/test/Modules/implicit-module-no-timestamp.cpp b/clang/test/Modules/implicit-module-no-timestamp.cpp
new file mode 100644
index 00000000000000..457ad3c16e184c
--- /dev/null
+++ b/clang/test/Modules/implicit-module-no-timestamp.cpp
@@ -0,0 +1,33 @@
+// RUN: rm -rf %t
+// RUN: split-file %s %t
+// RUN: cd %t
+//
+// RUN: cp a1.h a.h
+// RUN: %clang -fmodules -fmodules-validate-input-files-content -Xclang -fno-pch-timestamp -fimplicit-modules -fmodule-map-file=module.modulemap -fsyntax-only  -fmodules-cache-path=%t test1.cpp
+// RUN: cp a2.h a.h
+// RUN: %clang -fmodules -fmodules-validate-input-files-content -Xclang -fno-pch-timestamp -fimplicit-modules -fmodule-map-file=module.modulemap -fsyntax-only  -fmodules-cache-path=%t test2.cpp
+
+//--- a1.h
+#define FOO
+
+//--- a2.h
+#define BAR
+
+//--- module.modulemap
+module a {
+  header "a.h"
+}
+
+//--- test1.cpp
+#include "a.h"
+
+#ifndef FOO
+#error foo
+#endif
+
+//--- test2.cpp
+#include "a.h"
+
+#ifndef BAR
+#error bar
+#endif

[benlangmuir]

Does this test need to use the clang driver for some reason, or can it be changed to use %clang_cc1, which is generally preferred for tests?

[ivanmurashko]

Does this test need to use the clang driver for some reason

Unfortunately, yes. The option -fmodules-validate-input-files-content is a driver only option.

[benlangmuir]

The cc1 version is -fvalidate-ast-input-files-content.

[ivanmurashko]

Fixed, Thanks for the hint!