[clang] [clang][modules] Guard against bad -fmodule-file mappings (#132059) (PR #133462)
https://github.com/naveen-seth edited https://github.com/llvm/llvm-project/pull/133462 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang][modules] Guard against bad -fmodule-file mappings (#132059) (PR #133462)
naveen-seth wrote: Hi @ChuanqiXu9, @mpark, @shafik, For context, here is how this PR would change clang's behavior when replicating the original crash in #132059: The original crash, where the module `hello` wasn’t mapped to any BMI file, would now display the following: ```bash clang++ -fmodule-file=std=std.pcm -fmodule-file=std=hello.pcm -stdlib=libc++ -std=c++23 main.ccp ``` ```bash main.cpp:2:8: fatal error: module 'hello' not found 2 | import hello; | ~~~^ 1 error generated. ``` When assigning the matching BMI file to `hello`, but the wrong one to `std`: ```bash clang++ -stdlib=libc++ -std=c++23 -fmodule-file=std=std.pcm -fmodule-file=std=hello.pcm -fmodule-file=hello=hello.pcm main.ccp ``` ```bash main.ccp:2:1: fatal error: tried loading module 'std' from 'hello.pcm' but found module 'hello' instead 2 | import hello; | ^ main.ccp:2:1: note: imported by module 'hello' in 'hello.pcm' 1 error generated. ``` Would you be able to review the PR? https://github.com/llvm/llvm-project/pull/133462 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang][modules] Guard against bad -fmodule-file mappings (#132059) (PR #133462)
https://github.com/naveen-seth updated
https://github.com/llvm/llvm-project/pull/133462
>From 74ace4fd3c71ff59f2d89680c0f1382d0f9933f4 Mon Sep 17 00:00:00 2001
From: naveen-seth
Date: Fri, 28 Mar 2025 06:59:06 +0100
Subject: [PATCH] [clang][modules] Guard against bad -fmodule-file mappings
(#132059)
Fix #132059.
Providing incorrect mappings via -fmodule-file==,
such that the BMI file corresponds to a different module which
transitively imports the specified module, could previously crash the
compiler.
The crash is caused during serialization, when trying to resolve
declaration IDs in the AST body after having loaded the wrong module.
This commit fixes the issue by checking the module's identity while
reading the AST's control block and erroring out if a mismatch is
detected.
---
.../Basic/DiagnosticSerializationKinds.td | 2 +
clang/include/clang/Serialization/ASTReader.h | 17 +
clang/lib/Frontend/ASTUnit.cpp| 1 +
clang/lib/Frontend/ChainedIncludesSource.cpp | 1 +
clang/lib/Frontend/CompilerInstance.cpp | 15 ++--
clang/lib/Serialization/ASTReader.cpp | 73 +--
.../fmodule-file-bad-transitive-mapping.cpp | 46
7 files changed, 124 insertions(+), 31 deletions(-)
create mode 100644 clang/test/Modules/fmodule-file-bad-transitive-mapping.cpp
diff --git a/clang/include/clang/Basic/DiagnosticSerializationKinds.td
b/clang/include/clang/Basic/DiagnosticSerializationKinds.td
index 3914d3930bec7..16cc946e3f3d9 100644
--- a/clang/include/clang/Basic/DiagnosticSerializationKinds.td
+++ b/clang/include/clang/Basic/DiagnosticSerializationKinds.td
@@ -98,6 +98,8 @@ def err_imported_module_relocated : Error<
def err_module_different_modmap : Error<
"module '%0' %select{uses|does not use}1 additional module map '%2'"
"%select{| not}1 used when the module was built">;
+def err_module_mismatch : Error<
+ "tried loading module '%0' from '%1' but found module '%2' instead">,
DefaultFatal;
def err_ast_file_macro_def_undef : Error<
"macro '%0' was %select{defined|undef'd}1 in the AST file '%2' but "
diff --git a/clang/include/clang/Serialization/ASTReader.h
b/clang/include/clang/Serialization/ASTReader.h
index 2779b3d1cf2ea..57f2a08e09359 100644
--- a/clang/include/clang/Serialization/ASTReader.h
+++ b/clang/include/clang/Serialization/ASTReader.h
@@ -423,6 +423,9 @@ class ASTReader
/// configuration.
ConfigurationMismatch,
+/// The AST file contains a different module than expected.
+ModuleMismatch,
+
/// The AST file has errors.
HadErrors
};
@@ -1512,11 +1515,13 @@ class ASTReader
SourceLocation ImportLoc, ModuleFile *ImportedBy,
SmallVectorImpl &Loaded,
off_t ExpectedSize, time_t ExpectedModTime,
+StringRef ExpectedModuleName,
ASTFileSignature ExpectedSignature,
unsigned ClientLoadCapabilities);
ASTReadResult ReadControlBlock(ModuleFile &F,
SmallVectorImpl &Loaded,
const ModuleFile *ImportedBy,
+ StringRef ExpectedModuleName,
unsigned ClientLoadCapabilities);
static ASTReadResult
ReadOptionsBlock(llvm::BitstreamCursor &Stream, StringRef Filename,
@@ -1819,6 +1824,18 @@ class ASTReader
unsigned ClientLoadCapabilities,
ModuleFile **NewLoadedModuleFile = nullptr);
+ /// \overload
+ ///
+ /// Calls the above function and checks if the AST file contains the expected
+ /// module. Returns ASTReadResult::Failure on mismatch.
+ ///
+ /// \param ExpectedModuleName The expected name of the new loaded module.
+ ASTReadResult ReadAST(StringRef FileName, ModuleKind Type,
+SourceLocation ImportLoc,
+unsigned ClientLoadCapabilities,
+StringRef ExpectedModuleName,
+ModuleFile **NewLoadedModuleFile = nullptr);
+
/// Make the entities in the given module and any of its (non-explicit)
/// submodules visible to name lookup.
///
diff --git a/clang/lib/Frontend/ASTUnit.cpp b/clang/lib/Frontend/ASTUnit.cpp
index 0a5f1cfd1a264..7500be81ea976 100644
--- a/clang/lib/Frontend/ASTUnit.cpp
+++ b/clang/lib/Frontend/ASTUnit.cpp
@@ -887,6 +887,7 @@ std::unique_ptr ASTUnit::LoadFromASTFile(
case ASTReader::OutOfDate:
case ASTReader::VersionMismatch:
case ASTReader::ConfigurationMismatch:
+ case ASTReader::ModuleMismatch:
case ASTReader::HadErrors:
AST->getDiagnostics().Report(diag::err_fe_unable_to_load_pch);
return nullptr;
diff --git a/clang/lib/Frontend/ChainedIncludesSource.cpp
b/clang/lib/Frontend/ChainedIncludesSource.cpp
index a7096e27796a0..ee0363249124b 100644
--- a/clang/lib/Frontend/ChainedIncludesSource.cpp
+++
[clang] [clang][modules] Guard against bad -fmodule-file mappings (#132059) (PR #133462)
https://github.com/naveen-seth updated
https://github.com/llvm/llvm-project/pull/133462
>From cfbc14d381b5daed70ca812daac9ceed0039b5f6 Mon Sep 17 00:00:00 2001
From: naveen-seth
Date: Fri, 28 Mar 2025 06:59:06 +0100
Subject: [PATCH] [clang][modules] Guard against bad -fmodule-file mappings
(#132059)
When using -fmodule-file== with incorrect inputs,
the compiler crashes in two scenarios:
1. A module is mapped to the right BMI file but one of its
transitively exported module dependencies is also mapped to the same
BMI file.
2. A module is mapped to a wrong BMI file which also transitively
exports that module.
The crash is caused during serialization, when trying to resolve
declaration IDs in the AST body after having imported the wrong
module.
Because the 2nd scenario can only be detected after reading the BMI's
module name, checking for duplicate values while parsing command-line
options is not enough to fix the crash.
This commit fixes the issue by validating module identity after having
read the AST's ControlBlock.
---
.../Basic/DiagnosticSerializationKinds.td | 2 +
clang/include/clang/Serialization/ASTReader.h | 17 +
clang/lib/Frontend/ASTUnit.cpp| 1 +
clang/lib/Frontend/ChainedIncludesSource.cpp | 1 +
clang/lib/Frontend/CompilerInstance.cpp | 15 ++--
clang/lib/Serialization/ASTReader.cpp | 73 +--
.../fmodule-file-bad-transitive-mapping.cpp | 52 +
7 files changed, 130 insertions(+), 31 deletions(-)
create mode 100644 clang/test/Modules/fmodule-file-bad-transitive-mapping.cpp
diff --git a/clang/include/clang/Basic/DiagnosticSerializationKinds.td
b/clang/include/clang/Basic/DiagnosticSerializationKinds.td
index 3914d3930bec7..16cc946e3f3d9 100644
--- a/clang/include/clang/Basic/DiagnosticSerializationKinds.td
+++ b/clang/include/clang/Basic/DiagnosticSerializationKinds.td
@@ -98,6 +98,8 @@ def err_imported_module_relocated : Error<
def err_module_different_modmap : Error<
"module '%0' %select{uses|does not use}1 additional module map '%2'"
"%select{| not}1 used when the module was built">;
+def err_module_mismatch : Error<
+ "tried loading module '%0' from '%1' but found module '%2' instead">,
DefaultFatal;
def err_ast_file_macro_def_undef : Error<
"macro '%0' was %select{defined|undef'd}1 in the AST file '%2' but "
diff --git a/clang/include/clang/Serialization/ASTReader.h
b/clang/include/clang/Serialization/ASTReader.h
index 2779b3d1cf2ea..57f2a08e09359 100644
--- a/clang/include/clang/Serialization/ASTReader.h
+++ b/clang/include/clang/Serialization/ASTReader.h
@@ -423,6 +423,9 @@ class ASTReader
/// configuration.
ConfigurationMismatch,
+/// The AST file contains a different module than expected.
+ModuleMismatch,
+
/// The AST file has errors.
HadErrors
};
@@ -1512,11 +1515,13 @@ class ASTReader
SourceLocation ImportLoc, ModuleFile *ImportedBy,
SmallVectorImpl &Loaded,
off_t ExpectedSize, time_t ExpectedModTime,
+StringRef ExpectedModuleName,
ASTFileSignature ExpectedSignature,
unsigned ClientLoadCapabilities);
ASTReadResult ReadControlBlock(ModuleFile &F,
SmallVectorImpl &Loaded,
const ModuleFile *ImportedBy,
+ StringRef ExpectedModuleName,
unsigned ClientLoadCapabilities);
static ASTReadResult
ReadOptionsBlock(llvm::BitstreamCursor &Stream, StringRef Filename,
@@ -1819,6 +1824,18 @@ class ASTReader
unsigned ClientLoadCapabilities,
ModuleFile **NewLoadedModuleFile = nullptr);
+ /// \overload
+ ///
+ /// Calls the above function and checks if the AST file contains the expected
+ /// module. Returns ASTReadResult::Failure on mismatch.
+ ///
+ /// \param ExpectedModuleName The expected name of the new loaded module.
+ ASTReadResult ReadAST(StringRef FileName, ModuleKind Type,
+SourceLocation ImportLoc,
+unsigned ClientLoadCapabilities,
+StringRef ExpectedModuleName,
+ModuleFile **NewLoadedModuleFile = nullptr);
+
/// Make the entities in the given module and any of its (non-explicit)
/// submodules visible to name lookup.
///
diff --git a/clang/lib/Frontend/ASTUnit.cpp b/clang/lib/Frontend/ASTUnit.cpp
index 0a5f1cfd1a264..7500be81ea976 100644
--- a/clang/lib/Frontend/ASTUnit.cpp
+++ b/clang/lib/Frontend/ASTUnit.cpp
@@ -887,6 +887,7 @@ std::unique_ptr ASTUnit::LoadFromASTFile(
case ASTReader::OutOfDate:
case ASTReader::VersionMismatch:
case ASTReader::ConfigurationMismatch:
+ case ASTReader::ModuleMismatch:
case ASTReader::HadErrors:
AST->getDiagnostics().Report(diag::err
[clang] [clang][modules] Guard against bad -fmodule-file mappings (#132059) (PR #133462)
https://github.com/naveen-seth edited https://github.com/llvm/llvm-project/pull/133462 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang][modules] Guard against bad -fmodule-file mappings (#132059) (PR #133462)
https://github.com/naveen-seth updated
https://github.com/llvm/llvm-project/pull/133462
>From 804617c3083935d10f98b10e86f965e7508eb587 Mon Sep 17 00:00:00 2001
From: naveen-seth
Date: Fri, 28 Mar 2025 06:59:06 +0100
Subject: [PATCH] [clang][modules] Guard against bad -fmodule-file mappings
(#132059)
When using -fmodule-file== with incorrect inputs,
the compiler crashes in two scenarios:
1. A module is mapped to the right BMI file but one of its
transitively exported module dependencies is also mapped to the same
BMI file.
2. A module is mapped to a wrong BMI file which also transitively
exports that module.
The crash is caused during serialization, when trying to resolve
declaration IDs in the AST body after having imported the wrong
module.
Because the 2nd scenario can only be detected after reading the BMI's
module name, checking for duplicate values while parsing command-line
options is not enough to fix the crash.
This commit fixes the issue by validating module identity after having
read the AST's ControlBlock.
---
.../Basic/DiagnosticSerializationKinds.td | 2 +
clang/include/clang/Serialization/ASTReader.h | 17 +
clang/lib/Frontend/ASTUnit.cpp| 1 +
clang/lib/Frontend/ChainedIncludesSource.cpp | 1 +
clang/lib/Frontend/CompilerInstance.cpp | 15 ++--
clang/lib/Serialization/ASTReader.cpp | 73 +--
.../fmodule-file-bad-transitive-mapping.cpp | 52 +
7 files changed, 130 insertions(+), 31 deletions(-)
create mode 100644 clang/test/Modules/fmodule-file-bad-transitive-mapping.cpp
diff --git a/clang/include/clang/Basic/DiagnosticSerializationKinds.td
b/clang/include/clang/Basic/DiagnosticSerializationKinds.td
index 3914d3930bec7..16cc946e3f3d9 100644
--- a/clang/include/clang/Basic/DiagnosticSerializationKinds.td
+++ b/clang/include/clang/Basic/DiagnosticSerializationKinds.td
@@ -98,6 +98,8 @@ def err_imported_module_relocated : Error<
def err_module_different_modmap : Error<
"module '%0' %select{uses|does not use}1 additional module map '%2'"
"%select{| not}1 used when the module was built">;
+def err_module_mismatch : Error<
+ "tried loading module '%0' from '%1' but found module '%2' instead">,
DefaultFatal;
def err_ast_file_macro_def_undef : Error<
"macro '%0' was %select{defined|undef'd}1 in the AST file '%2' but "
diff --git a/clang/include/clang/Serialization/ASTReader.h
b/clang/include/clang/Serialization/ASTReader.h
index 2779b3d1cf2ea..57f2a08e09359 100644
--- a/clang/include/clang/Serialization/ASTReader.h
+++ b/clang/include/clang/Serialization/ASTReader.h
@@ -423,6 +423,9 @@ class ASTReader
/// configuration.
ConfigurationMismatch,
+/// The AST file contains a different module than expected.
+ModuleMismatch,
+
/// The AST file has errors.
HadErrors
};
@@ -1512,11 +1515,13 @@ class ASTReader
SourceLocation ImportLoc, ModuleFile *ImportedBy,
SmallVectorImpl &Loaded,
off_t ExpectedSize, time_t ExpectedModTime,
+StringRef ExpectedModuleName,
ASTFileSignature ExpectedSignature,
unsigned ClientLoadCapabilities);
ASTReadResult ReadControlBlock(ModuleFile &F,
SmallVectorImpl &Loaded,
const ModuleFile *ImportedBy,
+ StringRef ExpectedModuleName,
unsigned ClientLoadCapabilities);
static ASTReadResult
ReadOptionsBlock(llvm::BitstreamCursor &Stream, StringRef Filename,
@@ -1819,6 +1824,18 @@ class ASTReader
unsigned ClientLoadCapabilities,
ModuleFile **NewLoadedModuleFile = nullptr);
+ /// \overload
+ ///
+ /// Calls the above function and checks if the AST file contains the expected
+ /// module. Returns ASTReadResult::Failure on mismatch.
+ ///
+ /// \param ExpectedModuleName The expected name of the new loaded module.
+ ASTReadResult ReadAST(StringRef FileName, ModuleKind Type,
+SourceLocation ImportLoc,
+unsigned ClientLoadCapabilities,
+StringRef ExpectedModuleName,
+ModuleFile **NewLoadedModuleFile = nullptr);
+
/// Make the entities in the given module and any of its (non-explicit)
/// submodules visible to name lookup.
///
diff --git a/clang/lib/Frontend/ASTUnit.cpp b/clang/lib/Frontend/ASTUnit.cpp
index 0a5f1cfd1a264..7500be81ea976 100644
--- a/clang/lib/Frontend/ASTUnit.cpp
+++ b/clang/lib/Frontend/ASTUnit.cpp
@@ -887,6 +887,7 @@ std::unique_ptr ASTUnit::LoadFromASTFile(
case ASTReader::OutOfDate:
case ASTReader::VersionMismatch:
case ASTReader::ConfigurationMismatch:
+ case ASTReader::ModuleMismatch:
case ASTReader::HadErrors:
AST->getDiagnostics().Report(diag::err
[clang] [clang][modules] Guard against bad -fmodule-file mappings (#132059) (PR #133462)
https://github.com/naveen-seth closed https://github.com/llvm/llvm-project/pull/133462 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang][modules] Guard against bad -fmodule-file mappings (#132059) (PR #133462)
https://github.com/naveen-seth edited https://github.com/llvm/llvm-project/pull/133462 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang][modules] Guard against bad -fmodule-file mappings (#132059) (PR #133462)
https://github.com/naveen-seth reopened https://github.com/llvm/llvm-project/pull/133462 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang][modules] Guard against bad -fmodule-file mappings (#132059) (PR #133462)
https://github.com/naveen-seth edited https://github.com/llvm/llvm-project/pull/133462 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang][modules] Guard against bad -fmodule-file mappings (#132059) (PR #133462)
https://github.com/naveen-seth updated
https://github.com/llvm/llvm-project/pull/133462
>From 74ace4fd3c71ff59f2d89680c0f1382d0f9933f4 Mon Sep 17 00:00:00 2001
From: naveen-seth
Date: Fri, 28 Mar 2025 06:59:06 +0100
Subject: [PATCH 1/2] [clang][modules] Guard against bad -fmodule-file mappings
(#132059)
Fix #132059.
Providing incorrect mappings via -fmodule-file==,
such that the BMI file corresponds to a different module which
transitively imports the specified module, could previously crash the
compiler.
The crash is caused during serialization, when trying to resolve
declaration IDs in the AST body after having loaded the wrong module.
This commit fixes the issue by checking the module's identity while
reading the AST's control block and erroring out if a mismatch is
detected.
---
.../Basic/DiagnosticSerializationKinds.td | 2 +
clang/include/clang/Serialization/ASTReader.h | 17 +
clang/lib/Frontend/ASTUnit.cpp| 1 +
clang/lib/Frontend/ChainedIncludesSource.cpp | 1 +
clang/lib/Frontend/CompilerInstance.cpp | 15 ++--
clang/lib/Serialization/ASTReader.cpp | 73 +--
.../fmodule-file-bad-transitive-mapping.cpp | 46
7 files changed, 124 insertions(+), 31 deletions(-)
create mode 100644 clang/test/Modules/fmodule-file-bad-transitive-mapping.cpp
diff --git a/clang/include/clang/Basic/DiagnosticSerializationKinds.td
b/clang/include/clang/Basic/DiagnosticSerializationKinds.td
index 3914d3930bec7..16cc946e3f3d9 100644
--- a/clang/include/clang/Basic/DiagnosticSerializationKinds.td
+++ b/clang/include/clang/Basic/DiagnosticSerializationKinds.td
@@ -98,6 +98,8 @@ def err_imported_module_relocated : Error<
def err_module_different_modmap : Error<
"module '%0' %select{uses|does not use}1 additional module map '%2'"
"%select{| not}1 used when the module was built">;
+def err_module_mismatch : Error<
+ "tried loading module '%0' from '%1' but found module '%2' instead">,
DefaultFatal;
def err_ast_file_macro_def_undef : Error<
"macro '%0' was %select{defined|undef'd}1 in the AST file '%2' but "
diff --git a/clang/include/clang/Serialization/ASTReader.h
b/clang/include/clang/Serialization/ASTReader.h
index 2779b3d1cf2ea..57f2a08e09359 100644
--- a/clang/include/clang/Serialization/ASTReader.h
+++ b/clang/include/clang/Serialization/ASTReader.h
@@ -423,6 +423,9 @@ class ASTReader
/// configuration.
ConfigurationMismatch,
+/// The AST file contains a different module than expected.
+ModuleMismatch,
+
/// The AST file has errors.
HadErrors
};
@@ -1512,11 +1515,13 @@ class ASTReader
SourceLocation ImportLoc, ModuleFile *ImportedBy,
SmallVectorImpl &Loaded,
off_t ExpectedSize, time_t ExpectedModTime,
+StringRef ExpectedModuleName,
ASTFileSignature ExpectedSignature,
unsigned ClientLoadCapabilities);
ASTReadResult ReadControlBlock(ModuleFile &F,
SmallVectorImpl &Loaded,
const ModuleFile *ImportedBy,
+ StringRef ExpectedModuleName,
unsigned ClientLoadCapabilities);
static ASTReadResult
ReadOptionsBlock(llvm::BitstreamCursor &Stream, StringRef Filename,
@@ -1819,6 +1824,18 @@ class ASTReader
unsigned ClientLoadCapabilities,
ModuleFile **NewLoadedModuleFile = nullptr);
+ /// \overload
+ ///
+ /// Calls the above function and checks if the AST file contains the expected
+ /// module. Returns ASTReadResult::Failure on mismatch.
+ ///
+ /// \param ExpectedModuleName The expected name of the new loaded module.
+ ASTReadResult ReadAST(StringRef FileName, ModuleKind Type,
+SourceLocation ImportLoc,
+unsigned ClientLoadCapabilities,
+StringRef ExpectedModuleName,
+ModuleFile **NewLoadedModuleFile = nullptr);
+
/// Make the entities in the given module and any of its (non-explicit)
/// submodules visible to name lookup.
///
diff --git a/clang/lib/Frontend/ASTUnit.cpp b/clang/lib/Frontend/ASTUnit.cpp
index 0a5f1cfd1a264..7500be81ea976 100644
--- a/clang/lib/Frontend/ASTUnit.cpp
+++ b/clang/lib/Frontend/ASTUnit.cpp
@@ -887,6 +887,7 @@ std::unique_ptr ASTUnit::LoadFromASTFile(
case ASTReader::OutOfDate:
case ASTReader::VersionMismatch:
case ASTReader::ConfigurationMismatch:
+ case ASTReader::ModuleMismatch:
case ASTReader::HadErrors:
AST->getDiagnostics().Report(diag::err_fe_unable_to_load_pch);
return nullptr;
diff --git a/clang/lib/Frontend/ChainedIncludesSource.cpp
b/clang/lib/Frontend/ChainedIncludesSource.cpp
index a7096e27796a0..ee0363249124b 100644
--- a/clang/lib/Frontend/ChainedIncludesSource.cpp
[clang] [clang][modules] Guard against bad -fmodule-file mappings (#132059) (PR #133462)
naveen-seth wrote:
Hi, sorry! I left a comment earlier but noticed that a large part of the
explanation was incorrect and deleted it. Here is the fixed comment:
At first I thought that the issue can be fixed by just checking for duplicates
while parsing the command-line arguments, but that doesn't seem to be the case.
I will try to describe the crash point and the crash reason with this example
(without duplicate arguments):
```cpp
//--- a.cppm
export module A;
export int a() {
return 41;
}
```
```cpp
//--- b.cppm
export module B;
import A;
export int b() {
return a() + 1;
}
```
```cpp
//--- c.cppm
export module C;
export int c() {
return 42;
}
```
```cpp
//--- main.cpp
import B;
int main() {
return b();
}
```
```
# Correctly precompile the modules
clang++ -std=c++20 --precompile a.cppm -o a.pcm
clang++ -std=c++20 --precompile b.cppm -fmodule-file=A=a.pcm -o b.pcm
clang++ -std=c++20 --precompile c.cppm -o c.pcm
# Bad use of -fmodule-file=...
clang++ -std=c++20 main.cpp -std=c++20 main.cpp -fmodule-file=B=b.pcm
-fmodule-file=A=c.pcm
```
After `import B;` is parsed from `main.cpp`, the following sequence of function
calls occurs (from top to bottom):
…
`Sema::ActOnModuleImport`
`CompilerInstance::loadModule`
`CompilerInstance::findOrCompileModuleAndReadAST`
`ASTReader::ReadAST`
`ASTReader::ReadASTCore`
`ASTReader::ReadControlBlock`
When reading a modules control block in `ASTReader::ReadControlBlock`, Clang
also attempts to get the `ModuleFile` for each of its imports ([see
here](https://github.com/llvm/llvm-project/blob/943a70717c629f43b309ab56e8141ffb131871a6/clang/lib/Serialization/ASTReader.cpp#L3024)).
First, the matching BMI file for an imported module is looked up, then Clang
calls back to `ASTReader::ReadASTCore` to ensure the BMI files `ModuleFile`.
Currently (prior to this PR), there is no validation to ensure that the BMI
file actually contains the module clang is trying to load after having read the
module name in `ReadControlBlock`.
Because of the faulty `-fmodule-file=` mapping, the `ModuleFile` of the import
will refer to a BMI built for a different module than the one clang is trying
to load.
Here, the import of `A` in `b.cppm` will result in loading the `ModuleFile`
corresponding to `c.pcm` while deserializing `b.pcm`.
The crash occurs because Clang, assuming it has the correct `ModuleFile`s,
continues reading AST blocks and attempts to resolve `ModuleFile` remappings
(any of the mappings mentioned
[here](https://github.com/llvm/llvm-project/blob/74ace4fd3c71ff59f2d89680c0f1382d0f9933f4/clang/lib/Serialization/ASTReader.cpp#L8765)).
This results in invalid access into members of the invalid `ModuleFile`s and
finally the crash.
(Depending on the specific example, the exact location of the crash will be
different, but it will always happen while trying to resolve the remappings.)
While writing this comment, I noticed that my commit and PR message describe
the cause of the crash incorrectly and have edited both.
https://github.com/llvm/llvm-project/pull/133462
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang][modules] Guard against bad -fmodule-file mappings (#132059) (PR #133462)
https://github.com/naveen-seth edited https://github.com/llvm/llvm-project/pull/133462 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang][modules] Guard against bad -fmodule-file mappings (#132059) (PR #133462)
https://github.com/naveen-seth created
https://github.com/llvm/llvm-project/pull/133462
When using -fmodule-file== with incorrect inputs, the
compiler crashes in two scenarios:
1. A module is mapped to the right BMI file but one of its transitively
exported module dependencies is also mapped to the same BMI file.
2. A module is mapped to a wrong BMI file which also transitively exports that
module.
The crash is caused during serialization, when trying to resolve declaration
IDs in the AST body after having imported the wrong module.
Because the 2nd scenario can only be detected after reading the BMI's module
name, checking for duplicate values while parsing command-line options is not
enough to fix the crash.
This commit fixes the issue by validating module identity after having read the
AST's ControlBlock.
>From cfbc14d381b5daed70ca812daac9ceed0039b5f6 Mon Sep 17 00:00:00 2001
From: naveen-seth
Date: Fri, 28 Mar 2025 06:59:06 +0100
Subject: [PATCH] [clang][modules] Guard against bad -fmodule-file mappings
(#132059)
When using -fmodule-file== with incorrect inputs,
the compiler crashes in two scenarios:
1. A module is mapped to the right BMI file but one of its
transitively exported module dependencies is also mapped to the same
BMI file.
2. A module is mapped to a wrong BMI file which also transitively
exports that module.
The crash is caused during serialization, when trying to resolve
declaration IDs in the AST body after having imported the wrong
module.
Because the 2nd scenario can only be detected after reading the BMI's
module name, checking for duplicate values while parsing command-line
options is not enough to fix the crash.
This commit fixes the issue by validating module identity after having
read the AST's ControlBlock.
---
.../Basic/DiagnosticSerializationKinds.td | 2 +
clang/include/clang/Serialization/ASTReader.h | 17 +
clang/lib/Frontend/ASTUnit.cpp| 1 +
clang/lib/Frontend/ChainedIncludesSource.cpp | 1 +
clang/lib/Frontend/CompilerInstance.cpp | 15 ++--
clang/lib/Serialization/ASTReader.cpp | 73 +--
.../fmodule-file-bad-transitive-mapping.cpp | 52 +
7 files changed, 130 insertions(+), 31 deletions(-)
create mode 100644 clang/test/Modules/fmodule-file-bad-transitive-mapping.cpp
diff --git a/clang/include/clang/Basic/DiagnosticSerializationKinds.td
b/clang/include/clang/Basic/DiagnosticSerializationKinds.td
index 3914d3930bec7..16cc946e3f3d9 100644
--- a/clang/include/clang/Basic/DiagnosticSerializationKinds.td
+++ b/clang/include/clang/Basic/DiagnosticSerializationKinds.td
@@ -98,6 +98,8 @@ def err_imported_module_relocated : Error<
def err_module_different_modmap : Error<
"module '%0' %select{uses|does not use}1 additional module map '%2'"
"%select{| not}1 used when the module was built">;
+def err_module_mismatch : Error<
+ "tried loading module '%0' from '%1' but found module '%2' instead">,
DefaultFatal;
def err_ast_file_macro_def_undef : Error<
"macro '%0' was %select{defined|undef'd}1 in the AST file '%2' but "
diff --git a/clang/include/clang/Serialization/ASTReader.h
b/clang/include/clang/Serialization/ASTReader.h
index 2779b3d1cf2ea..57f2a08e09359 100644
--- a/clang/include/clang/Serialization/ASTReader.h
+++ b/clang/include/clang/Serialization/ASTReader.h
@@ -423,6 +423,9 @@ class ASTReader
/// configuration.
ConfigurationMismatch,
+/// The AST file contains a different module than expected.
+ModuleMismatch,
+
/// The AST file has errors.
HadErrors
};
@@ -1512,11 +1515,13 @@ class ASTReader
SourceLocation ImportLoc, ModuleFile *ImportedBy,
SmallVectorImpl &Loaded,
off_t ExpectedSize, time_t ExpectedModTime,
+StringRef ExpectedModuleName,
ASTFileSignature ExpectedSignature,
unsigned ClientLoadCapabilities);
ASTReadResult ReadControlBlock(ModuleFile &F,
SmallVectorImpl &Loaded,
const ModuleFile *ImportedBy,
+ StringRef ExpectedModuleName,
unsigned ClientLoadCapabilities);
static ASTReadResult
ReadOptionsBlock(llvm::BitstreamCursor &Stream, StringRef Filename,
@@ -1819,6 +1824,18 @@ class ASTReader
unsigned ClientLoadCapabilities,
ModuleFile **NewLoadedModuleFile = nullptr);
+ /// \overload
+ ///
+ /// Calls the above function and checks if the AST file contains the expected
+ /// module. Returns ASTReadResult::Failure on mismatch.
+ ///
+ /// \param ExpectedModuleName The expected name of the new loaded module.
+ ASTReadResult ReadAST(StringRef FileName, ModuleKind Type,
+SourceLocation ImportLoc,
+unsigned Cli
[clang] [clang][modules] Guard against bad -fmodule-file mappings (#132059) (PR #133462)
https://github.com/naveen-seth updated
https://github.com/llvm/llvm-project/pull/133462
>From a23ba4d51332180ff5d1b5bc9de2d0c6c04cbf66 Mon Sep 17 00:00:00 2001
From: naveen-seth
Date: Fri, 28 Mar 2025 06:59:06 +0100
Subject: [PATCH] [clang][modules] Guard against bad -fmodule-file mappings
(#132059)
Fix #132059.
Providing incorrect mappings via `-fmodule-file==`
can crash the compiler when loading a module that imports an
incorrectly mapped module.
The crash occurs during AST body deserialization, when the compiler
attempts to resolve remappings using the `ModuleFile` from the
incorrectly mapped module's BMI file.
The cause is an invalid access into an incorrectly loaded
`ModuleFile`.
This commit fixes the issue by verifying that the mapped BMI files
correspond to the mapped-from modules as soon as the module name is
read from the BMI's control block, and it errors out if there is a
mismatch.
---
.../Basic/DiagnosticSerializationKinds.td | 2 +
clang/include/clang/Serialization/ASTReader.h | 17 +
clang/lib/Frontend/ASTUnit.cpp| 1 +
clang/lib/Frontend/ChainedIncludesSource.cpp | 1 +
clang/lib/Frontend/CompilerInstance.cpp | 15 ++--
clang/lib/Serialization/ASTReader.cpp | 73 +--
.../fmodule-file-bad-transitive-mapping.cpp | 46
7 files changed, 124 insertions(+), 31 deletions(-)
create mode 100644 clang/test/Modules/fmodule-file-bad-transitive-mapping.cpp
diff --git a/clang/include/clang/Basic/DiagnosticSerializationKinds.td
b/clang/include/clang/Basic/DiagnosticSerializationKinds.td
index 3914d3930bec7..16cc946e3f3d9 100644
--- a/clang/include/clang/Basic/DiagnosticSerializationKinds.td
+++ b/clang/include/clang/Basic/DiagnosticSerializationKinds.td
@@ -98,6 +98,8 @@ def err_imported_module_relocated : Error<
def err_module_different_modmap : Error<
"module '%0' %select{uses|does not use}1 additional module map '%2'"
"%select{| not}1 used when the module was built">;
+def err_module_mismatch : Error<
+ "tried loading module '%0' from '%1' but found module '%2' instead">,
DefaultFatal;
def err_ast_file_macro_def_undef : Error<
"macro '%0' was %select{defined|undef'd}1 in the AST file '%2' but "
diff --git a/clang/include/clang/Serialization/ASTReader.h
b/clang/include/clang/Serialization/ASTReader.h
index 2779b3d1cf2ea..57f2a08e09359 100644
--- a/clang/include/clang/Serialization/ASTReader.h
+++ b/clang/include/clang/Serialization/ASTReader.h
@@ -423,6 +423,9 @@ class ASTReader
/// configuration.
ConfigurationMismatch,
+/// The AST file contains a different module than expected.
+ModuleMismatch,
+
/// The AST file has errors.
HadErrors
};
@@ -1512,11 +1515,13 @@ class ASTReader
SourceLocation ImportLoc, ModuleFile *ImportedBy,
SmallVectorImpl &Loaded,
off_t ExpectedSize, time_t ExpectedModTime,
+StringRef ExpectedModuleName,
ASTFileSignature ExpectedSignature,
unsigned ClientLoadCapabilities);
ASTReadResult ReadControlBlock(ModuleFile &F,
SmallVectorImpl &Loaded,
const ModuleFile *ImportedBy,
+ StringRef ExpectedModuleName,
unsigned ClientLoadCapabilities);
static ASTReadResult
ReadOptionsBlock(llvm::BitstreamCursor &Stream, StringRef Filename,
@@ -1819,6 +1824,18 @@ class ASTReader
unsigned ClientLoadCapabilities,
ModuleFile **NewLoadedModuleFile = nullptr);
+ /// \overload
+ ///
+ /// Calls the above function and checks if the AST file contains the expected
+ /// module. Returns ASTReadResult::Failure on mismatch.
+ ///
+ /// \param ExpectedModuleName The expected name of the new loaded module.
+ ASTReadResult ReadAST(StringRef FileName, ModuleKind Type,
+SourceLocation ImportLoc,
+unsigned ClientLoadCapabilities,
+StringRef ExpectedModuleName,
+ModuleFile **NewLoadedModuleFile = nullptr);
+
/// Make the entities in the given module and any of its (non-explicit)
/// submodules visible to name lookup.
///
diff --git a/clang/lib/Frontend/ASTUnit.cpp b/clang/lib/Frontend/ASTUnit.cpp
index 0a5f1cfd1a264..7500be81ea976 100644
--- a/clang/lib/Frontend/ASTUnit.cpp
+++ b/clang/lib/Frontend/ASTUnit.cpp
@@ -887,6 +887,7 @@ std::unique_ptr ASTUnit::LoadFromASTFile(
case ASTReader::OutOfDate:
case ASTReader::VersionMismatch:
case ASTReader::ConfigurationMismatch:
+ case ASTReader::ModuleMismatch:
case ASTReader::HadErrors:
AST->getDiagnostics().Report(diag::err_fe_unable_to_load_pch);
return nullptr;
diff --git a/clang/lib/Frontend/ChainedIncludesSource.cpp
b/clang/lib/Frontend/Ch
