https://github.com/jansvoboda11 created https://github.com/llvm/llvm-project/pull/133827
This essentially reverts commit 227f71995804 (https://reviews.llvm.org/D150320). The original change to disable checking for relocated modules was built on the assumption that the file system is immutable and modules (their module maps) are not getting relocated. This turns out to not be true in Xcode projects where `.framework` directories may appear in a search path during the build. I was of the opinion then that this is a bug in the project build and that `.framework` directories appearing in a search path mid-build should not be observable (by either writing them at the very start of the build, or by not compiling any TUs that use the search path before it's populated). If this happened, it would result in the `module 'X' is defined in both 'MM1' and 'MM2'` error. While I still think this is a desirable property for a single-project build, multi-project workspace builds cannot avoid hitting this kind of issue (demonstrated by the test) and they are left with a build error they cannot fix. What's worse, since this situation is detected fairly late during deserialization (in `ReadASTBlock()`), Clang cannot graciously recover. At this point, `ASTReader` is no longer able to remove pending module files, since they might've been partially deserialized. `Preprocessor` considers this situation a fatal error and tries to abort the compilation by calling `CurLexer->cutOffLexing()`. However, the dependency scanning lexer ignores this and continues to supply more tokens to the compiler. This may eventually lead to a crash caused by accessing the invalid (partially-deserialized) `ASTReader` state. Fixing the dependency scanning lexer to respect the cut-off leads to more crashes somewhere in typo correction initiated by `Parser`/`Sema`. These crashes should be fixed separately, but right now I'm mostly concerned about fixing multi-project workspace builds by being more lenient about relocated modules. With this patch, we're again catching the cases of one module being described by multiple different module map files early in `ReadASTCore()`. In that case, we mark the module chain as out of date, remove any pending module files, and try to rebuild the module. Note that this slows down scans by ~7.5%. I suspect we might be able to get some of that back by avoiding module map parsing in these relocation checks and only looking at FS structure (at least for frameworks). rdar://133388373 >From 47162cd1ff22d850f7496e9502ba25b25f547976 Mon Sep 17 00:00:00 2001 From: Jan Svoboda <jan_svob...@apple.com> Date: Mon, 31 Mar 2025 16:34:37 -0700 Subject: [PATCH] [clang][deps] Do check for relocated modules This essentially reverts commit 227f71995804 (https://reviews.llvm.org/D150320). The original change to disable checking for relocated modules was built on the assumption that the file system is immutable and modules (their module maps) are not getting relocated. This turns out to not be true in Xcode projects where `.framework` directories may appear in a search path during the build. I was of the opinion then that this is a bug in the project build and that `.framework` directories appearing in a search path mid-build should not be observable (by either writing them at the very start of the build, or by not compiling any TUs that use the search path before populating it). This would result in the "module 'X' is defined in both 'MM1' and 'MM2'" error. While I still think this is a desirable property for a single-project build, multi-project workspace builds cannot avoid hitting this kind of issue and they are left with a build error they cannot fix. What's worse, since this is detected fairly late during deserialization (in `ReadASTBlock()`), Clang cannot graciously recover and rebuild the module. At this point, `ASTReader` is no longer able to remove pending module files (since they might've been partially deserialized). `Preprocessor` considers this situation a fatal error and tries to abort the compilation by calling `CurLexer->cutOffLexing()`. However, the dependency scanning lexer ignores this and continues to supply more tokens to the compiler. This may eventually lead to a crash caused by accessing the invalid `ASTReader` state. Fixing the dependency scanning lexer to respect the cut-off lead to more crashes somewhere during typo correction initiated by `Parser`/`Sema`. These issues should be fixed separately, but right now I'm mostly concerned about fixing multi-project workspace builds by being more lenient about relocated modules. With this patch, we're again catching the cases of one module being described by multiple different module map files early in `ReadASTCore()`. In that case, we mark the module chain as out of date, remove any pending module files, and try to rebuild the module. Note that this slows down scans by ~7.5%. I suspect we might be able to get some of that back by avoiding module map parsing in these relocation checks, and only looking at FS structure (at least for frameworks). rdar://133388373 --- clang/include/clang/Driver/Options.td | 2 +- .../DependencyScanningWorker.cpp | 3 -- .../modules-relocated-module-map.c | 36 +++++++++++++++++++ 3 files changed, 37 insertions(+), 4 deletions(-) create mode 100644 clang/test/ClangScanDeps/modules-relocated-module-map.c diff --git a/clang/include/clang/Driver/Options.td b/clang/include/clang/Driver/Options.td index 89cb03cc33b98..1d827ecbc3b06 100644 --- a/clang/include/clang/Driver/Options.td +++ b/clang/include/clang/Driver/Options.td @@ -3353,7 +3353,7 @@ defm implicit_modules : BoolFOption<"implicit-modules", NegFlag<SetFalse, [], [ClangOption, CC1Option]>, PosFlag<SetTrue>, BothFlags< [NoXarchOption], [ClangOption, CLOption]>>; -def fno_modules_check_relocated : Joined<["-"], "fno-modules-check-relocated">, +def fno_modules_check_relocated : Flag<["-"], "fno-modules-check-relocated">, Group<f_Group>, Visibility<[ClangOption, CC1Option]>, HelpText<"Skip checks for relocated modules when loading PCM files">, MarshallingInfoNegativeFlag<PreprocessorOpts<"ModulesCheckRelocated">>; diff --git a/clang/lib/Tooling/DependencyScanning/DependencyScanningWorker.cpp b/clang/lib/Tooling/DependencyScanning/DependencyScanningWorker.cpp index b7d44caca4954..df8db38131b95 100644 --- a/clang/lib/Tooling/DependencyScanning/DependencyScanningWorker.cpp +++ b/clang/lib/Tooling/DependencyScanning/DependencyScanningWorker.cpp @@ -427,9 +427,6 @@ class DependencyScanningAction : public tooling::ToolAction { ScanInstance.getHeaderSearchOpts().ModulesSkipPragmaDiagnosticMappings = true; - // Avoid some checks and module map parsing when loading PCM files. - ScanInstance.getPreprocessorOpts().ModulesCheckRelocated = false; - std::unique_ptr<FrontendAction> Action; if (Service.getFormat() == ScanningOutputFormat::P1689) diff --git a/clang/test/ClangScanDeps/modules-relocated-module-map.c b/clang/test/ClangScanDeps/modules-relocated-module-map.c new file mode 100644 index 0000000000000..3aad52027cc43 --- /dev/null +++ b/clang/test/ClangScanDeps/modules-relocated-module-map.c @@ -0,0 +1,36 @@ +// RUN: rm -rf %t +// RUN: split-file %s %t + +// RUN: mkdir %t/frameworks1 + +// RUN: clang-scan-deps -format experimental-full -- \ +// RUN: %clang -fmodules -fmodules-cache-path=%t/cache \ +// RUN: -F %t/frameworks1 -F %t/frameworks2 \ +// RUN: -c %t/tu1.m -o %t/tu1.o + +// RUN: cp -r %t/frameworks2/A.framework %t/frameworks1 + +// RUN: clang-scan-deps -format experimental-full -- \ +// RUN: %clang -fmodules -fmodules-cache-path=%t/cache \ +// RUN: -F %t/frameworks1 -F %t/frameworks2 \ +// RUN: -c %t/tu2.m -o %t/tu2.o + +//--- frameworks2/A.framework/Modules/module.modulemap +framework module A { header "A.h" } +//--- frameworks2/A.framework/Headers/A.h +#define MACRO_A 1 + +//--- frameworks2/B.framework/Modules/module.modulemap +framework module B { header "B.h" } +//--- frameworks2/B.framework/Headers/B.h +#include <A/A.h> + +//--- tu1.m +#include <B/B.h> + +//--- tu2.m +#include <A/A.h> +#include <B/B.h> + +#if MACRO_A == 3 +#endif _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
