Skip to content

Commit

Permalink
[Modules] Process include files changes (#90319)
Browse files Browse the repository at this point in the history
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](https://reviews.llvm.org/D67249) introduced the
`-fmodules-validate-input-files-content` option, which allows the use of
file content hash in addition to the modification time.
- [D141632](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: 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.
  • Loading branch information
ivanmurashko committed May 1, 2024
1 parent 0c42fa3 commit 9a9cff1
Show file tree
Hide file tree
Showing 2 changed files with 42 additions and 0 deletions.
8 changes: 8 additions & 0 deletions clang/lib/Serialization/ASTReader.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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()) {
Expand Down
34 changes: 34 additions & 0 deletions clang/test/Modules/implicit-module-no-timestamp.cpp
Original file line number Diff line number Diff line change
@@ -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

0 comments on commit 9a9cff1

Please sign in to comment.