[clang] dba2b5c - [clang][modules] Skip submodule & framework re-definitions in module maps

[Jan Svoboda via cfe-commits]

Author: Jan Svoboda
Date: 2023-07-17T13:50:25-07:00
New Revision: dba2b5c9314e1d127ee5200e739e6c8ca53a9831

URL: https://github.com/llvm/llvm-project/commit/dba2b5c9314e1d127ee5200e739e6c8ca53a9831
DIFF: https://github.com/llvm/llvm-project/commit/dba2b5c9314e1d127ee5200e739e6c8ca53a9831.diff

LOG: [clang][modules] Skip submodule & framework re-definitions in module maps

Before D150478, there were situations when Clang avoided parsing a module map because it was likely to re-define an already defined module (either by a PCM or by previously-found module map). Since Clang no longer performs that check and does parse the extra module map (due to the FW/FW_Private issue described in D150478), this patch re-implements the same semantics by skipping the duplicate definition of the framework module while parsing the module map.

Depends on D150478.

Reviewed By: benlangmuir

Differential Revision: https://reviews.llvm.org/D150479

Added: 
    clang/test/Modules/no-check-relocated-fw-private-sub.c
    clang/test/Modules/shadow-framework.m

Modified: 
    clang/lib/Lex/ModuleMap.cpp

Removed: 
    


################################################################################
diff  --git a/clang/lib/Lex/ModuleMap.cpp b/clang/lib/Lex/ModuleMap.cpp
index c97642e5c6c87f..4f713fe6e4ad41 100644
--- a/clang/lib/Lex/ModuleMap.cpp
+++ b/clang/lib/Lex/ModuleMap.cpp
@@ -2016,12 +2016,28 @@ void ModuleMapParser::parseModuleDecl() {
   Module *ShadowingModule = nullptr;
   if (Module *Existing = Map.lookupModuleQualified(ModuleName, ActiveModule)) {
     // We might see a (re)definition of a module that we already have a
-    // definition for in three cases:
+    // definition for in four cases:
     //  - If we loaded one definition from an AST file and we've just found a
     //    corresponding definition in a module map file, or
     bool LoadedFromASTFile = Existing->IsFromModuleFile;
     //  - If we previously inferred this module from 
diff erent module map file.
     bool Inferred = Existing->IsInferred;
+    //  - If we're building a framework that vends a module map, we might've
+    //    previously seen the one in intermediate products and now the system
+    //    one.
+    // FIXME: If we're parsing module map file that looks like this:
+    //          framework module FW { ... }
+    //          module FW.Sub { ... }
+    //        We can't check the framework qualifier, since it's not attached to
+    //        the definition of Sub. Checking that qualifier on \c Existing is
+    //        not correct either, since we might've previously seen:
+    //          module FW { ... }
+    //          module FW.Sub { ... }
+    //        We should enforce consistency of redefinitions so that we can rely
+    //        that \c Existing is part of a framework iff the redefinition of FW
+    //        we have just skipped had it too. Once we do that, stop checking
+    //        the local framework qualifier and only rely on \c Existing.
+    bool PartOfFramework = Framework || Existing->isPartOfFramework();
     //  - If we're building a (preprocessed) module and we've just loaded the
     //    module map file from which it was created.
     bool ParsedAsMainInput =
@@ -2029,7 +2045,8 @@ void ModuleMapParser::parseModuleDecl() {
         Map.LangOpts.CurrentModule == ModuleName &&
         SourceMgr.getDecomposedLoc(ModuleNameLoc).first !=
             SourceMgr.getDecomposedLoc(Existing->DefinitionLoc).first;
-    if (!ActiveModule && (LoadedFromASTFile || Inferred || ParsedAsMainInput)) {
+    if (LoadedFromASTFile || Inferred || PartOfFramework || ParsedAsMainInput) {
+      ActiveModule = PreviousActiveModule;
       // Skip the module definition.
       skipUntil(MMToken::RBrace);
       if (Tok.is(MMToken::RBrace))

diff  --git a/clang/test/Modules/no-check-relocated-fw-private-sub.c b/clang/test/Modules/no-check-relocated-fw-private-sub.c
new file mode 100644
index 00000000000000..b2864c40cff665
--- /dev/null
+++ b/clang/test/Modules/no-check-relocated-fw-private-sub.c
@@ -0,0 +1,23 @@
+// RUN: rm -rf %t
+// RUN: split-file %s %t
+
+//--- frameworks1/FW1.framework/Modules/module.modulemap
+framework module FW1 { header "FW1.h" }
+//--- frameworks1/FW1.framework/Headers/FW1.h
+#import <FW2/FW2.h>
+
+//--- frameworks2/FW2.framework/Modules/module.modulemap
+framework module FW2 { header "FW2.h" }
+//--- frameworks2/FW2.framework/Modules/module.private.modulemap
+framework module FW2.Private { header "FW2_Private.h" }
+//--- frameworks2/FW2.framework/Headers/FW2.h
+//--- frameworks2/FW2.framework/PrivateHeaders/FW2_Private.h
+
+//--- tu.c
+#import <FW1/FW1.h>         // expected-remark{{importing module 'FW1'}} \
+                            // expected-remark{{importing module 'FW2'}}
+#import <FW2/FW2_Private.h> // expected-remark{{importing module 'FW2'}}
+
+// RUN: %clang_cc1 -fmodules -fmodules-cache-path=%t/cache -fimplicit-module-maps \
+// RUN:   -F %t/frameworks1 -F %t/frameworks2 -fsyntax-only %t/tu.c \
+// RUN:   -fno-modules-check-relocated -Rmodule-import -verify

diff  --git a/clang/test/Modules/shadow-framework.m b/clang/test/Modules/shadow-framework.m
new file mode 100644
index 00000000000000..0d5c439a8a31c2
--- /dev/null
+++ b/clang/test/Modules/shadow-framework.m
@@ -0,0 +1,20 @@
+// RUN: rm -rf %t
+// RUN: split-file %s %t
+
+// This test checks that redefinitions of frameworks are ignored.
+
+//--- include/module.modulemap
+module first { header "first.h" }
+module FW {}
+//--- include/first.h
+
+//--- frameworks/FW.framework/Modules/module.modulemap
+framework module FW { header "FW.h" }
+//--- frameworks/FW.framework/Headers/FW.h
+
+//--- tu.c
+#import "first.h" // expected-remark {{importing module 'first'}}
+#import <FW/FW.h>
+
+// RUN: %clang_cc1 -fmodules -fmodules-cache-path=%t/cache -fimplicit-module-maps \
+// RUN:   -I %t/include -F %t/frameworks -fsyntax-only %t/tu.c -Rmodule-import -verify