[clang][deps] Do check for relocated modules

[jansvoboda11]

This essentially reverts commit 227f719 (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

[llvmbot]

@llvm/pr-subscribers-clang

Author: Jan Svoboda (jansvoboda11)

Changes

This essentially reverts commit 227f719 (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

---

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

3 Files Affected:

• (modified) clang/include/clang/Driver/Options.td (+1-1)
• (modified) clang/lib/Tooling/DependencyScanning/DependencyScanningWorker.cpp (-3)
• (added) clang/test/ClangScanDeps/modules-relocated-module-map.c (+36)

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

[cyndyishida]

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.

Is this because a multi-project workspace results in separate build-products directories but a single module cache? IOW, a single build session should never depend on a module with the same name but derived from different search paths?

Note that this slows down scans by ~7.5%

Do you know where the added overhead comes from, or why lazy modulemap parsing would avoid some of it? Is it caused by the need to rebuild the chain of modules that were invalidated?

[jansvoboda11]

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.

Is this because a multi-project workspace results in separate build-products directories but a single module cache? IOW, a single build session should never depend on a module with the same name but derived from different search paths?

They use single build products directory and the same module cache with the same context hash (at least under Swift EBM).

Note that this slows down scans by ~7.5%

Do you know where the added overhead comes from, or why lazy modulemap parsing would avoid some of it? Is it caused by the need to rebuild the chain of modules that were invalidated?

That comes from extra module map parsing and loading. Lazy module map loading would avoid some of it, yes. This number is for the case where we do not need to rebuild the chain of modules - that will likely see more significant slowdown.

I talked with @Bigcheese and @benlangmuir at length about this issue and my takeaway is that this patch just papers over the build system issues and slows down scans of projects that are configured correctly. I think a better solution would be to make it so that the scan of each project uses a separate context hash, which removes the "unfixable by developers" aspect of this issue. That would essentially reduce this issue to the one we already know about where within a single project build, we may first see module defined by the SDK module map and then by the module map in the build directory, which is easily fixable by each individual project.