https://github.com/benlangmuir created
https://github.com/llvm/llvm-project/pull/122955
When we mark a module visible, we normally mark all of its non-explicit
submodules and other exports as visible. However, when we first enter a
submodule we should not make them visible to the submodule itself until they
are actually imported. Marking exports visible before import would cause
bizarre behaviour with local submodule visibility, because it happened before
we discovered the submodule's transitive imports and could fail to make them
visible in the parent module depending on whether the submodules involved were
explicitly defined (module X) or implicitly defined from an umbrella (module *).
rdar://136524433
>From 5541ea0e31d35dfd20a3157a52997b1b2f6d3966 Mon Sep 17 00:00:00 2001
From: Ben Langmuir <blangm...@apple.com>
Date: Tue, 14 Jan 2025 10:57:56 -0800
Subject: [PATCH] [clang][modules] Fix local submodule visibility of macros
from transitive import
When we mark a module visible, we normally mark all of its non-explicit
submodules and other exports as visible. However, when we first enter a
submodule we should not make them visible to the submodule itself until
they are actually imported. Marking exports visible before import would
cause bizarre behaviour with local submodule visibility, because it
happened before we discovered the submodule's transitive imports and
could fail to make them visible in the parent module depending on
whether the submodules involved were explicitly defined (module X) or
implicitly defined from an umbrella (module *).
rdar://136524433
---
clang/include/clang/Basic/Module.h | 9 +--
clang/include/clang/Lex/Preprocessor.h | 3 +-
clang/lib/Basic/Module.cpp | 17 ++---
clang/lib/Lex/PPLexerChange.cpp | 5 +-
clang/lib/Lex/Preprocessor.cpp | 5 +-
...l-submodule-visibility-transitive-import.c | 62 +++++++++++++++++++
6 files changed, 85 insertions(+), 16 deletions(-)
create mode 100644
clang/test/Modules/local-submodule-visibility-transitive-import.c
diff --git a/clang/include/clang/Basic/Module.h
b/clang/include/clang/Basic/Module.h
index dd384c1d76c5fd..1506481fc1ff72 100644
--- a/clang/include/clang/Basic/Module.h
+++ b/clang/include/clang/Basic/Module.h
@@ -881,10 +881,11 @@ class VisibleModuleSet {
StringRef Message)>;
/// Make a specific module visible.
- void setVisible(Module *M, SourceLocation Loc,
- VisibleCallback Vis = [](Module *) {},
- ConflictCallback Cb = [](ArrayRef<Module *>, Module *,
- StringRef) {});
+ void setVisible(
+ Module *M, SourceLocation Loc, bool IncludeExports = true,
+ VisibleCallback Vis = [](Module *) {},
+ ConflictCallback Cb = [](ArrayRef<Module *>, Module *, StringRef) {});
+
private:
/// Import locations for each visible module. Indexed by the module's
/// VisibilityID.
diff --git a/clang/include/clang/Lex/Preprocessor.h
b/clang/include/clang/Lex/Preprocessor.h
index 3d223c345ea156..1f8de1254249b7 100644
--- a/clang/include/clang/Lex/Preprocessor.h
+++ b/clang/include/clang/Lex/Preprocessor.h
@@ -1755,7 +1755,8 @@ class Preprocessor {
bool LexAfterModuleImport(Token &Result);
void CollectPpImportSuffix(SmallVectorImpl<Token> &Toks);
- void makeModuleVisible(Module *M, SourceLocation Loc);
+ void makeModuleVisible(Module *M, SourceLocation Loc,
+ bool IncludeExports = true);
SourceLocation getModuleImportLoc(Module *M) const {
return CurSubmoduleState->VisibleModules.getImportLoc(M);
diff --git a/clang/lib/Basic/Module.cpp b/clang/lib/Basic/Module.cpp
index 330108d5b3e47f..bb85a4010931fc 100644
--- a/clang/lib/Basic/Module.cpp
+++ b/clang/lib/Basic/Module.cpp
@@ -662,7 +662,8 @@ LLVM_DUMP_METHOD void Module::dump() const {
}
void VisibleModuleSet::setVisible(Module *M, SourceLocation Loc,
- VisibleCallback Vis, ConflictCallback Cb) {
+ bool IncludeExports, VisibleCallback Vis,
+ ConflictCallback Cb) {
// We can't import a global module fragment so the location can be invalid.
assert((M->isGlobalModule() || Loc.isValid()) &&
"setVisible expects a valid import location");
@@ -688,12 +689,14 @@ void VisibleModuleSet::setVisible(Module *M,
SourceLocation Loc,
Vis(V.M);
// Make any exported modules visible.
- SmallVector<Module *, 16> Exports;
- V.M->getExportedModules(Exports);
- for (Module *E : Exports) {
- // Don't import non-importable modules.
- if (!E->isUnimportable())
- VisitModule({E, &V});
+ if (IncludeExports) {
+ SmallVector<Module *, 16> Exports;
+ V.M->getExportedModules(Exports);
+ for (Module *E : Exports) {
+ // Don't import non-importable modules.
+ if (!E->isUnimportable())
+ VisitModule({E, &V});
+ }
}
for (auto &C : V.M->Conflicts) {
diff --git a/clang/lib/Lex/PPLexerChange.cpp b/clang/lib/Lex/PPLexerChange.cpp
index 22d7c4e0132732..e1dcc5499170eb 100644
--- a/clang/lib/Lex/PPLexerChange.cpp
+++ b/clang/lib/Lex/PPLexerChange.cpp
@@ -754,9 +754,10 @@ void Preprocessor::EnterSubmodule(Module *M,
SourceLocation ImportLoc,
// Switch to this submodule as the current submodule.
CurSubmoduleState = &State;
- // This module is visible to itself.
+ // This module is visible to itself, but exports should not be made visible
+ // until they are imported.
if (FirstTime)
- makeModuleVisible(M, ImportLoc);
+ makeModuleVisible(M, ImportLoc, /*IncludeExports=*/false);
}
bool Preprocessor::needModuleMacros() const {
diff --git a/clang/lib/Lex/Preprocessor.cpp b/clang/lib/Lex/Preprocessor.cpp
index 5ac5e6feae1443..7256473d54ed2f 100644
--- a/clang/lib/Lex/Preprocessor.cpp
+++ b/clang/lib/Lex/Preprocessor.cpp
@@ -1331,9 +1331,10 @@ bool Preprocessor::LexAfterModuleImport(Token &Result) {
return true;
}
-void Preprocessor::makeModuleVisible(Module *M, SourceLocation Loc) {
+void Preprocessor::makeModuleVisible(Module *M, SourceLocation Loc,
+ bool IncludeExports) {
CurSubmoduleState->VisibleModules.setVisible(
- M, Loc, [](Module *) {},
+ M, Loc, IncludeExports, [](Module *) {},
[&](ArrayRef<Module *> Path, Module *Conflict, StringRef Message) {
// FIXME: Include the path in the diagnostic.
// FIXME: Include the import location for the conflicting module.
diff --git a/clang/test/Modules/local-submodule-visibility-transitive-import.c
b/clang/test/Modules/local-submodule-visibility-transitive-import.c
new file mode 100644
index 00000000000000..333d4860e9123b
--- /dev/null
+++ b/clang/test/Modules/local-submodule-visibility-transitive-import.c
@@ -0,0 +1,62 @@
+// Checks that macros from transitive imports work with local submodule
+// visibility. In the below test, previously a() and d() failed because
+// OTHER_MACRO1 and OTHER_MACRO3 were not visible at the use site.
+
+// RUN: rm -rf %t
+// RUN: split-file %s %t
+// RUN: %clang_cc1 -fmodules -fimplicit-module-maps -fmodules-cache-path=%t \
+// RUN: -fmodules-local-submodule-visibility -I%t %t/tu.c -verify
+
+//--- Other1.h
+#define OTHER_MACRO1(...)
+
+//--- Other2.h
+#define OTHER_MACRO2(...)
+
+//--- Other3.h
+#define OTHER_MACRO3(...)
+
+//--- module.modulemap
+module Other {
+ module O1 { header "Other1.h" }
+ module O2 { header "Other2.h" }
+ module O3 { header "Other3.h" }
+}
+
+//--- Top/A.h
+#include "Other1.h"
+#define MACRO_A OTHER_MACRO1(x, y)
+
+//--- Top/B.h
+#include "Other2.h"
+#define MACRO_B OTHER_MACRO2(x, y)
+
+//--- Top/C.h
+#include "D.h"
+
+//--- Top/D.h
+#include "Other3.h"
+#define MACRO_D OTHER_MACRO3(x, y)
+
+//--- Top/Top.h
+#include "A.h"
+#include "B.h"
+#include "C.h"
+
+void a() MACRO_A;
+void b() MACRO_B;
+void d() MACRO_D;
+
+//--- Top/module.modulemap
+module Top {
+ umbrella header "Top.h"
+ module A { header "A.h" export * }
+ module D { header "D.h" export * }
+ module * { export * }
+ export *
+ export Other.O3
+}
+
+//--- tu.c
+#include "Top/Top.h"
+// expected-no-diagnostics
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
