[clang] [Modules] Process include files changes with -fmodules-validate-input-files-content and -fno-pch-timestamp options (PR #90319)

[Ivan Murashko via cfe-commits]

https://github.com/ivanmurashko updated https://github.com/llvm/llvm-project/pull/90319

>From 71796a80e15b6700fec23fbec70635aae455b188 Mon Sep 17 00:00:00 2001
From: Ivan Murashko <ivanmurashko at meta.com>
Date: Fri, 26 Apr 2024 22:45:26 +0100
Subject: [PATCH] [Modules] Process include files changes with
 -fmodules-validate-input-files-content and -fno-pch-timestamp options

There are two diffs that introduce some options required when you build modules
externally and cannot rely on file modification time as a key for detecting
input file changes.

https://reviews.llvm.org/D67249 introduced the
`-fmodules-validate-input-files-content` option, which allows the use of file
content hash instead of modification time.

https://reviews.llvm.org/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.

The issue can be fixed using the same trick that was applied during the
processing of ForceCheckCXX20ModulesInputFiles. The patch suggests a solution
and includes a LIT test to verify it.
---
 clang/lib/Serialization/ASTReader.cpp         |  8 +++++
 .../Modules/implicit-module-no-timestamp.cpp  | 34 +++++++++++++++++++
 2 files changed, 42 insertions(+)
 create mode 100644 clang/test/Modules/implicit-module-no-timestamp.cpp

diff --git a/clang/lib/Serialization/ASTReader.cpp b/clang/lib/Serialization/ASTReader.cpp
index 143fbc7feb3ab7..29b81c1a753cf5 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..1b681a610bab22
--- /dev/null
+++ b/clang/test/Modules/implicit-module-no-timestamp.cpp
@@ -0,0 +1,34 @@
+// UNSUPPORTED: system-windows
+// RUN: rm -rf %t
+// RUN: split-file %s %t
+// RUN: cd %t
+//
+// RUN: cp a1.h a.h
+// RUN: %clang_cc1 -fmodules -fvalidate-ast-input-files-content -fno-pch-timestamp -fmodule-map-file=module.modulemap -fmodules-cache-path=%t test1.cpp
+// RUN: cp a2.h a.h
+// RUN: %clang_cc1 -fmodules -fvalidate-ast-input-files-content -fno-pch-timestamp -fmodule-map-file=module.modulemap -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