[PATCH] D86069: [clang] When loading preamble from AST file, re-export modules in Sema.
adamcz created this revision.
Herald added subscribers: cfe-commits, usaxena95, kadircet, arphaman, jkorous.
Herald added a project: clang.
adamcz requested review of this revision.
This addresses a FIXME in ASTReader.
Modules were already re-exported for Preprocessor, but not for Sema.
The result was that, with -fmodules-local-submodule-visibility, all AST
nodes belonging to a module that was loaded in a premable where not
accesible from the main part of the file and a diagnostic recommending
importing those modules would be generated.
Repository:
rG LLVM Github Monorepo
https://reviews.llvm.org/D86069
Files:
clang-tools-extra/clangd/unittests/ModulesTests.cpp
clang/include/clang/Sema/Sema.h
clang/lib/Serialization/ASTReader.cpp
clang/test/PCH/Inputs/modules/Foo.h
clang/test/PCH/preamble-modules.cpp
Index: clang/test/PCH/preamble-modules.cpp
===
--- /dev/null
+++ clang/test/PCH/preamble-modules.cpp
@@ -0,0 +1,15 @@
+// Check that modules included in the preamble remain visible to the rest of the
+// file.
+
+// RUN: rm -rf %t.mcp
+// RUN: %clang_cc1 -emit-pch -o %t.pch %s -fmodules -fmodule-map-file=%S/Inputs/modules/module.modulemap -fmodules-local-submodule-visibility -fmodules-cache-path=%t.mcp
+// RUN: %clang_cc1 -include-pch %t.pch %s -fmodules -fmodule-map-file=%S/Inputs/modules/module.modulemap -fmodules-local-submodule-visibility -fmodules-cache-path=%t.mcp
+
+#ifndef MAIN_FILE
+#define MAIN_FILE
+// Premable section.
+#include "Inputs/modules/Foo.h"
+#else
+// Main section.
+MyType foo;
+#endif
Index: clang/test/PCH/Inputs/modules/Foo.h
===
--- clang/test/PCH/Inputs/modules/Foo.h
+++ clang/test/PCH/Inputs/modules/Foo.h
@@ -1 +1,3 @@
void make_foo(void);
+
+typedef int MyType;
Index: clang/lib/Serialization/ASTReader.cpp
===
--- clang/lib/Serialization/ASTReader.cpp
+++ clang/lib/Serialization/ASTReader.cpp
@@ -4987,10 +4987,8 @@
/*ImportLoc=*/Import.ImportLoc);
if (Import.ImportLoc.isValid())
PP.makeModuleVisible(Imported, Import.ImportLoc);
- // FIXME: should we tell Sema to make the module visible too?
}
}
- ImportedModules.clear();
}
void ASTReader::finalizeForWriting() {
@@ -7942,6 +7940,15 @@
SemaObj->FpPragmaStack.CurrentPragmaLocation = FpPragmaCurrentLocation;
}
}
+
+ // For non-modular AST files, restore visiblity of modules.
+ for (auto &Import : ImportedModules) {
+if (Import.ImportLoc.isInvalid())
+ continue;
+if (Module *Imported = getSubmodule(Import.ID)) {
+ SemaObj->makeModuleVisible(Imported, Import.ImportLoc);
+}
+ }
}
IdentifierInfo *ASTReader::get(StringRef Name) {
Index: clang/include/clang/Sema/Sema.h
===
--- clang/include/clang/Sema/Sema.h
+++ clang/include/clang/Sema/Sema.h
@@ -1912,6 +1912,12 @@
bool isModuleVisible(const Module *M, bool ModulePrivate = false);
+ // When loading a non-modular PCH files, this is used to restore module
+ // visibility.
+ void makeModuleVisible(Module *Mod, SourceLocation ImportLoc) {
+VisibleModules.setVisible(Mod, ImportLoc);
+ }
+
/// Determine whether a declaration is visible to name lookup.
bool isVisible(const NamedDecl *D) {
return D->isUnconditionallyVisible() || isVisibleSlow(D);
Index: clang-tools-extra/clangd/unittests/ModulesTests.cpp
===
--- clang-tools-extra/clangd/unittests/ModulesTests.cpp
+++ clang-tools-extra/clangd/unittests/ModulesTests.cpp
@@ -26,7 +26,7 @@
void foo() {}
)cpp");
TU.ExtraArgs.push_back("-fmodule-name=M");
- TU.ExtraArgs.push_back("-fmodule-map-file=m.modulemap");
+ TU.ExtraArgs.push_back("-fmodule-map-file=" + testPath("m.modulemap"));
TU.AdditionalFiles["Textual.h"] = "void foo();";
TU.AdditionalFiles["m.modulemap"] = R"modulemap(
module M {
@@ -39,6 +39,31 @@
TU.index();
}
+// Verify that visibility of AST nodes belonging to modules, but loaded from
+// preamble PCH, is restored.
+TEST(Modules, PreambleBuildVisibility) {
+ TestTU TU = TestTU::withCode(R"cpp(
+#include "module.h"
+
+foo x;
+)cpp");
+ TU.OverlayRealFileSystemForModules = true;
+ TU.ExtraArgs.push_back("-fmodules");
+ TU.ExtraArgs.push_back("-fmodules-strict-decluse");
+ TU.ExtraArgs.push_back("-Xclang");
+ TU.ExtraArgs.push_back("-fmodules-local-submodule-visibility");
+ TU.ExtraArgs.push_back("-fmodule-map-file=" + testPath("m.modulemap"));
+ TU.AdditionalFiles["module.h"] = R"cpp(
+typedef int foo;
+)cpp";
+ TU.AdditionalFiles["m.modulemap"] = R"modulemap(
+module M {
+ header "module.h"
+}
+)modulemap";
+ EXPECT_TRUE(TU.build().getDiagnostics().empty());
+}
+
} // namespace
} // namespace clangd
} // namespace
[PATCH] D86069: [clang] When loading preamble from AST file, re-export modules in Sema.
sammccall added inline comments. Comment at: clang/test/PCH/preamble-modules.cpp:8 + +#ifndef MAIN_FILE +#define MAIN_FILE what are these ifdefs about? you never seem to define this symbol (Possible you're copying from a test that is doing something tricky like being run in two modes or including itself? Or did you mean to give different flags for the include vs emit pch?) Comment at: clang/test/PCH/preamble-modules.cpp:14 +// Main section. +MyType foo; +#endif This seems like currently it's never getting parsed as MAIN_FILE is never defined. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D86069/new/ https://reviews.llvm.org/D86069 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D86069: [clang] When loading preamble from AST file, re-export modules in Sema.
adamcz added inline comments. Comment at: clang/test/PCH/preamble-modules.cpp:8 + +#ifndef MAIN_FILE +#define MAIN_FILE sammccall wrote: > what are these ifdefs about? you never seem to define this symbol > > (Possible you're copying from a test that is doing something tricky like > being run in two modes or including itself? Or did you mean to give different > flags for the include vs emit pch?) Oh, I stole this trick from Kadir. You compile this file twice. First one is to generate a PCH for preamble. MAIN_FILE is not set, so it only reads the preamble section. The second one has -include-pch. That means MAIN_FILE is now set (from the preamble PCH file), so we only compile the main section. It's a creative solution to a problem I wish I didn't have ;-) Is there a better way to do this, without splitting the file in two? preamble_bytes seems fragile. Comment at: clang/test/PCH/preamble-modules.cpp:14 +// Main section. +MyType foo; +#endif sammccall wrote: > This seems like currently it's never getting parsed as MAIN_FILE is never > defined. It's parsed. Without this fix, there's a warning generated here. See comment above for explanation. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D86069/new/ https://reviews.llvm.org/D86069 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D86069: [clang] When loading preamble from AST file, re-export modules in Sema.
sammccall accepted this revision. sammccall added a comment. This revision is now accepted and ready to land. Sorry, I was just not reading the test clearly. LGTM Comment at: clang/lib/Serialization/ASTReader.cpp:4990 PP.makeModuleVisible(Imported, Import.ImportLoc); - // FIXME: should we tell Sema to make the module visible too? } maybe replace this with a comment pointing at UpdateSema() for the sema part Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D86069/new/ https://reviews.llvm.org/D86069 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D86069: [clang] When loading preamble from AST file, re-export modules in Sema.
adamcz updated this revision to Diff 286561.
adamcz added a comment.
Added comment in place of a FIXME
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D86069/new/
https://reviews.llvm.org/D86069
Files:
clang-tools-extra/clangd/unittests/ModulesTests.cpp
clang/include/clang/Sema/Sema.h
clang/lib/Serialization/ASTReader.cpp
clang/test/PCH/Inputs/modules/Foo.h
clang/test/PCH/preamble-modules.cpp
Index: clang/test/PCH/preamble-modules.cpp
===
--- /dev/null
+++ clang/test/PCH/preamble-modules.cpp
@@ -0,0 +1,15 @@
+// Check that modules included in the preamble remain visible to the rest of the
+// file.
+
+// RUN: rm -rf %t.mcp
+// RUN: %clang_cc1 -emit-pch -o %t.pch %s -fmodules -fmodule-map-file=%S/Inputs/modules/module.modulemap -fmodules-local-submodule-visibility -fmodules-cache-path=%t.mcp
+// RUN: %clang_cc1 -include-pch %t.pch %s -fmodules -fmodule-map-file=%S/Inputs/modules/module.modulemap -fmodules-local-submodule-visibility -fmodules-cache-path=%t.mcp
+
+#ifndef MAIN_FILE
+#define MAIN_FILE
+// Premable section.
+#include "Inputs/modules/Foo.h"
+#else
+// Main section.
+MyType foo;
+#endif
Index: clang/test/PCH/Inputs/modules/Foo.h
===
--- clang/test/PCH/Inputs/modules/Foo.h
+++ clang/test/PCH/Inputs/modules/Foo.h
@@ -1 +1,3 @@
void make_foo(void);
+
+typedef int MyType;
Index: clang/lib/Serialization/ASTReader.cpp
===
--- clang/lib/Serialization/ASTReader.cpp
+++ clang/lib/Serialization/ASTReader.cpp
@@ -4987,10 +4987,10 @@
/*ImportLoc=*/Import.ImportLoc);
if (Import.ImportLoc.isValid())
PP.makeModuleVisible(Imported, Import.ImportLoc);
- // FIXME: should we tell Sema to make the module visible too?
+ // This updates visibility for Preprocessor only. For Sema, which can be
+ // nullptr here, we do the same later, in UpdateSema().
}
}
- ImportedModules.clear();
}
void ASTReader::finalizeForWriting() {
@@ -7942,6 +7942,15 @@
SemaObj->FpPragmaStack.CurrentPragmaLocation = FpPragmaCurrentLocation;
}
}
+
+ // For non-modular AST files, restore visiblity of modules.
+ for (auto &Import : ImportedModules) {
+if (Import.ImportLoc.isInvalid())
+ continue;
+if (Module *Imported = getSubmodule(Import.ID)) {
+ SemaObj->makeModuleVisible(Imported, Import.ImportLoc);
+}
+ }
}
IdentifierInfo *ASTReader::get(StringRef Name) {
Index: clang/include/clang/Sema/Sema.h
===
--- clang/include/clang/Sema/Sema.h
+++ clang/include/clang/Sema/Sema.h
@@ -1912,6 +1912,12 @@
bool isModuleVisible(const Module *M, bool ModulePrivate = false);
+ // When loading a non-modular PCH files, this is used to restore module
+ // visibility.
+ void makeModuleVisible(Module *Mod, SourceLocation ImportLoc) {
+VisibleModules.setVisible(Mod, ImportLoc);
+ }
+
/// Determine whether a declaration is visible to name lookup.
bool isVisible(const NamedDecl *D) {
return D->isUnconditionallyVisible() || isVisibleSlow(D);
Index: clang-tools-extra/clangd/unittests/ModulesTests.cpp
===
--- clang-tools-extra/clangd/unittests/ModulesTests.cpp
+++ clang-tools-extra/clangd/unittests/ModulesTests.cpp
@@ -26,7 +26,7 @@
void foo() {}
)cpp");
TU.ExtraArgs.push_back("-fmodule-name=M");
- TU.ExtraArgs.push_back("-fmodule-map-file=m.modulemap");
+ TU.ExtraArgs.push_back("-fmodule-map-file=" + testPath("m.modulemap"));
TU.AdditionalFiles["Textual.h"] = "void foo();";
TU.AdditionalFiles["m.modulemap"] = R"modulemap(
module M {
@@ -39,6 +39,31 @@
TU.index();
}
+// Verify that visibility of AST nodes belonging to modules, but loaded from
+// preamble PCH, is restored.
+TEST(Modules, PreambleBuildVisibility) {
+ TestTU TU = TestTU::withCode(R"cpp(
+#include "module.h"
+
+foo x;
+)cpp");
+ TU.OverlayRealFileSystemForModules = true;
+ TU.ExtraArgs.push_back("-fmodules");
+ TU.ExtraArgs.push_back("-fmodules-strict-decluse");
+ TU.ExtraArgs.push_back("-Xclang");
+ TU.ExtraArgs.push_back("-fmodules-local-submodule-visibility");
+ TU.ExtraArgs.push_back("-fmodule-map-file=" + testPath("m.modulemap"));
+ TU.AdditionalFiles["module.h"] = R"cpp(
+typedef int foo;
+)cpp";
+ TU.AdditionalFiles["m.modulemap"] = R"modulemap(
+module M {
+ header "module.h"
+}
+)modulemap";
+ EXPECT_TRUE(TU.build().getDiagnostics().empty());
+}
+
} // namespace
} // namespace clangd
} // namespace clang
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D86069: [clang] When loading preamble from AST file, re-export modules in Sema.
This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
Closed by commit rGbaeff989b050: [clang] When loading preamble from AST file,
re-export modules in Sema. (authored by adamcz).
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D86069/new/
https://reviews.llvm.org/D86069
Files:
clang-tools-extra/clangd/unittests/ModulesTests.cpp
clang/include/clang/Sema/Sema.h
clang/lib/Serialization/ASTReader.cpp
clang/test/PCH/Inputs/modules/Foo.h
clang/test/PCH/preamble-modules.cpp
Index: clang/test/PCH/preamble-modules.cpp
===
--- /dev/null
+++ clang/test/PCH/preamble-modules.cpp
@@ -0,0 +1,15 @@
+// Check that modules included in the preamble remain visible to the rest of the
+// file.
+
+// RUN: rm -rf %t.mcp
+// RUN: %clang_cc1 -emit-pch -o %t.pch %s -fmodules -fmodule-map-file=%S/Inputs/modules/module.modulemap -fmodules-local-submodule-visibility -fmodules-cache-path=%t.mcp
+// RUN: %clang_cc1 -include-pch %t.pch %s -fmodules -fmodule-map-file=%S/Inputs/modules/module.modulemap -fmodules-local-submodule-visibility -fmodules-cache-path=%t.mcp
+
+#ifndef MAIN_FILE
+#define MAIN_FILE
+// Premable section.
+#include "Inputs/modules/Foo.h"
+#else
+// Main section.
+MyType foo;
+#endif
Index: clang/test/PCH/Inputs/modules/Foo.h
===
--- clang/test/PCH/Inputs/modules/Foo.h
+++ clang/test/PCH/Inputs/modules/Foo.h
@@ -1 +1,3 @@
void make_foo(void);
+
+typedef int MyType;
Index: clang/lib/Serialization/ASTReader.cpp
===
--- clang/lib/Serialization/ASTReader.cpp
+++ clang/lib/Serialization/ASTReader.cpp
@@ -4987,10 +4987,10 @@
/*ImportLoc=*/Import.ImportLoc);
if (Import.ImportLoc.isValid())
PP.makeModuleVisible(Imported, Import.ImportLoc);
- // FIXME: should we tell Sema to make the module visible too?
+ // This updates visibility for Preprocessor only. For Sema, which can be
+ // nullptr here, we do the same later, in UpdateSema().
}
}
- ImportedModules.clear();
}
void ASTReader::finalizeForWriting() {
@@ -7943,6 +7943,15 @@
SemaObj->FpPragmaStack.CurrentPragmaLocation = FpPragmaCurrentLocation;
}
}
+
+ // For non-modular AST files, restore visiblity of modules.
+ for (auto &Import : ImportedModules) {
+if (Import.ImportLoc.isInvalid())
+ continue;
+if (Module *Imported = getSubmodule(Import.ID)) {
+ SemaObj->makeModuleVisible(Imported, Import.ImportLoc);
+}
+ }
}
IdentifierInfo *ASTReader::get(StringRef Name) {
Index: clang/include/clang/Sema/Sema.h
===
--- clang/include/clang/Sema/Sema.h
+++ clang/include/clang/Sema/Sema.h
@@ -1912,6 +1912,12 @@
bool isModuleVisible(const Module *M, bool ModulePrivate = false);
+ // When loading a non-modular PCH files, this is used to restore module
+ // visibility.
+ void makeModuleVisible(Module *Mod, SourceLocation ImportLoc) {
+VisibleModules.setVisible(Mod, ImportLoc);
+ }
+
/// Determine whether a declaration is visible to name lookup.
bool isVisible(const NamedDecl *D) {
return D->isUnconditionallyVisible() || isVisibleSlow(D);
Index: clang-tools-extra/clangd/unittests/ModulesTests.cpp
===
--- clang-tools-extra/clangd/unittests/ModulesTests.cpp
+++ clang-tools-extra/clangd/unittests/ModulesTests.cpp
@@ -26,7 +26,7 @@
void foo() {}
)cpp");
TU.ExtraArgs.push_back("-fmodule-name=M");
- TU.ExtraArgs.push_back("-fmodule-map-file=m.modulemap");
+ TU.ExtraArgs.push_back("-fmodule-map-file=" + testPath("m.modulemap"));
TU.AdditionalFiles["Textual.h"] = "void foo();";
TU.AdditionalFiles["m.modulemap"] = R"modulemap(
module M {
@@ -39,6 +39,31 @@
TU.index();
}
+// Verify that visibility of AST nodes belonging to modules, but loaded from
+// preamble PCH, is restored.
+TEST(Modules, PreambleBuildVisibility) {
+ TestTU TU = TestTU::withCode(R"cpp(
+#include "module.h"
+
+foo x;
+)cpp");
+ TU.OverlayRealFileSystemForModules = true;
+ TU.ExtraArgs.push_back("-fmodules");
+ TU.ExtraArgs.push_back("-fmodules-strict-decluse");
+ TU.ExtraArgs.push_back("-Xclang");
+ TU.ExtraArgs.push_back("-fmodules-local-submodule-visibility");
+ TU.ExtraArgs.push_back("-fmodule-map-file=" + testPath("m.modulemap"));
+ TU.AdditionalFiles["module.h"] = R"cpp(
+typedef int foo;
+)cpp";
+ TU.AdditionalFiles["m.modulemap"] = R"modulemap(
+module M {
+ header "module.h"
+}
+)modulemap";
+ EXPECT_TRUE(TU.build().getDiagnostics().empty());
+}
+
} // namespace
} // namespace clangd
} // namespace clang
___
cfe-commits mailing list
