Skip to content

[stable/20240723][clang][modules] Fix local submodule visibility of macros from transitive import #10086

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 5 additions & 4 deletions clang/include/clang/Basic/Module.h
Original file line number Diff line number Diff line change
Expand Up @@ -907,10 +907,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.
Expand Down
3 changes: 2 additions & 1 deletion clang/include/clang/Lex/Preprocessor.h
Original file line number Diff line number Diff line change
Expand Up @@ -1766,7 +1766,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);
Expand Down
17 changes: 10 additions & 7 deletions clang/lib/Basic/Module.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -678,7 +678,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");
Expand All @@ -704,12 +705,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) {
Expand Down
5 changes: 3 additions & 2 deletions clang/lib/Lex/PPLexerChange.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -759,9 +759,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 {
Expand Down
5 changes: 3 additions & 2 deletions clang/lib/Lex/Preprocessor.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -1336,9 +1336,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.
Expand Down
Original file line number Diff line number Diff line change
@@ -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