[clang] 60eb1da - [Modules] [Sema] Don't try to getAcceptableDecls during the iteration of noload_lookups
Chuanqi Xu via cfe-commits
cfe-commits at lists.llvm.org
Thu Jun 8 20:44:03 PDT 2023
Author: Chuanqi Xu
Date: 2023-06-09T11:32:06+08:00
New Revision: 60eb1da315559a9e20f9bf502fd10ba68f6d4085
URL: https://github.com/llvm/llvm-project/commit/60eb1da315559a9e20f9bf502fd10ba68f6d4085
DIFF: https://github.com/llvm/llvm-project/commit/60eb1da315559a9e20f9bf502fd10ba68f6d4085.diff
LOG: [Modules] [Sema] Don't try to getAcceptableDecls during the iteration of noload_lookups
I found this during the support of modules for clangd. The reason for
the issue is that the iterator returned by noload_lookups is fast-fail
after the lookup table changes by the design of llvm::DenseMap. And
originally the lookup will try to use getAcceptableDecl to filter the
invisible decls. The key point here is that the function
"getAcceptableDecl" wouldn't stop after it find the specific decl is
invisble. It will continue to visit its redecls to find a visible one.
However, such process involves loading decls from external sources,
which results the invalidation.
Note that the use of "noload_lookups" is rare. It is only used in tools
like FixTypos and CodeCompletions. So it is completely fine for the
tranditional compiler. This is the reason why I can't reproduce it by a
lit test.
Added:
clang/unittests/Sema/SemaNoloadLookupTest.cpp
Modified:
clang/lib/Sema/SemaLookup.cpp
clang/unittests/Sema/CMakeLists.txt
Removed:
################################################################################
diff --git a/clang/lib/Sema/SemaLookup.cpp b/clang/lib/Sema/SemaLookup.cpp
index fad8405b12743..ba873b0187454 100644
--- a/clang/lib/Sema/SemaLookup.cpp
+++ b/clang/lib/Sema/SemaLookup.cpp
@@ -4149,22 +4149,21 @@ class LookupVisibleHelper {
// Enumerate all of the results in this context.
for (DeclContextLookupResult R :
Load ? Ctx->lookups()
- : Ctx->noload_lookups(/*PreserveInternalState=*/false)) {
- for (auto *D : R) {
- if (auto *ND = Result.getAcceptableDecl(D)) {
- // Rather than visit immediately, we put ND into a vector and visit
- // all decls, in order, outside of this loop. The reason is that
- // Consumer.FoundDecl() may invalidate the iterators used in the two
- // loops above.
- DeclsToVisit.push_back(ND);
- }
+ : Ctx->noload_lookups(/*PreserveInternalState=*/false))
+ for (auto *D : R)
+ // Rather than visit immediately, we put ND into a vector and visit
+ // all decls, in order, outside of this loop. The reason is that
+ // Consumer.FoundDecl() and LookupResult::getAcceptableDecl(D)
+ // may invalidate the iterators used in the two
+ // loops above.
+ DeclsToVisit.push_back(D);
+
+ for (auto *D : DeclsToVisit)
+ if (auto *ND = Result.getAcceptableDecl(D)) {
+ Consumer.FoundDecl(ND, Visited.checkHidden(ND), Ctx, InBaseClass);
+ Visited.add(ND);
}
- }
- for (auto *ND : DeclsToVisit) {
- Consumer.FoundDecl(ND, Visited.checkHidden(ND), Ctx, InBaseClass);
- Visited.add(ND);
- }
DeclsToVisit.clear();
// Traverse using directives for qualified name lookup.
diff --git a/clang/unittests/Sema/CMakeLists.txt b/clang/unittests/Sema/CMakeLists.txt
index 82d309375eda6..7ded562e8edfa 100644
--- a/clang/unittests/Sema/CMakeLists.txt
+++ b/clang/unittests/Sema/CMakeLists.txt
@@ -8,6 +8,7 @@ add_clang_unittest(SemaTests
CodeCompleteTest.cpp
GslOwnerPointerInference.cpp
SemaLookupTest.cpp
+ SemaNoloadLookupTest.cpp
)
clang_target_link_libraries(SemaTests
diff --git a/clang/unittests/Sema/SemaNoloadLookupTest.cpp b/clang/unittests/Sema/SemaNoloadLookupTest.cpp
new file mode 100644
index 0000000000000..2e74ad3a30a93
--- /dev/null
+++ b/clang/unittests/Sema/SemaNoloadLookupTest.cpp
@@ -0,0 +1,193 @@
+//== unittests/Sema/SemaNoloadLookupTest.cpp -------------------------========//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+#include "clang/AST/DeclLookups.h"
+#include "clang/AST/DeclarationName.h"
+#include "clang/ASTMatchers/ASTMatchFinder.h"
+#include "clang/ASTMatchers/ASTMatchers.h"
+#include "clang/Frontend/CompilerInstance.h"
+#include "clang/Frontend/FrontendAction.h"
+#include "clang/Frontend/FrontendActions.h"
+#include "clang/Parse/ParseAST.h"
+#include "clang/Sema/Lookup.h"
+#include "clang/Sema/Sema.h"
+#include "clang/Sema/SemaConsumer.h"
+#include "clang/Tooling/Tooling.h"
+#include "gtest/gtest.h"
+
+using namespace llvm;
+using namespace clang;
+using namespace clang::tooling;
+
+namespace {
+
+class NoloadLookupTest : public ::testing::Test {
+ void SetUp() override {
+ ASSERT_FALSE(
+ sys::fs::createUniqueDirectory("modules-no-comments-test", TestDir));
+ }
+
+ void TearDown() override { sys::fs::remove_directories(TestDir); }
+
+public:
+ SmallString<256> TestDir;
+
+ void addFile(StringRef Path, StringRef Contents) {
+ ASSERT_FALSE(sys::path::is_absolute(Path));
+
+ SmallString<256> AbsPath(TestDir);
+ sys::path::append(AbsPath, Path);
+
+ ASSERT_FALSE(
+ sys::fs::create_directories(llvm::sys::path::parent_path(AbsPath)));
+
+ std::error_code EC;
+ llvm::raw_fd_ostream OS(AbsPath, EC);
+ ASSERT_FALSE(EC);
+ OS << Contents;
+ }
+
+ std::string GenerateModuleInterface(StringRef ModuleName,
+ StringRef Contents) {
+ std::string FileName = llvm::Twine(ModuleName + ".cppm").str();
+ addFile(FileName, Contents);
+
+ IntrusiveRefCntPtr<DiagnosticsEngine> Diags =
+ CompilerInstance::createDiagnostics(new DiagnosticOptions());
+ CreateInvocationOptions CIOpts;
+ CIOpts.Diags = Diags;
+ CIOpts.VFS = llvm::vfs::createPhysicalFileSystem();
+
+ std::string CacheBMIPath =
+ llvm::Twine(TestDir + "/" + ModuleName + " .pcm").str();
+ std::string PrebuiltModulePath =
+ "-fprebuilt-module-path=" + TestDir.str().str();
+ const char *Args[] = {"clang++",
+ "-std=c++20",
+ "--precompile",
+ PrebuiltModulePath.c_str(),
+ "-working-directory",
+ TestDir.c_str(),
+ "-I",
+ TestDir.c_str(),
+ FileName.c_str(),
+ "-o",
+ CacheBMIPath.c_str()};
+ std::shared_ptr<CompilerInvocation> Invocation =
+ createInvocation(Args, CIOpts);
+ EXPECT_TRUE(Invocation);
+
+ CompilerInstance Instance;
+ Instance.setDiagnostics(Diags.get());
+ Instance.setInvocation(Invocation);
+ GenerateModuleInterfaceAction Action;
+ EXPECT_TRUE(Instance.ExecuteAction(Action));
+ EXPECT_FALSE(Diags->hasErrorOccurred());
+
+ return CacheBMIPath;
+ }
+};
+
+struct TrivialVisibleDeclConsumer : public VisibleDeclConsumer {
+ TrivialVisibleDeclConsumer() {}
+ void EnteredContext(DeclContext *Ctx) override {}
+ void FoundDecl(NamedDecl *ND, NamedDecl *Hiding, DeclContext *Ctx,
+ bool InBaseClass) override {
+ FoundNum++;
+ }
+
+ int FoundNum = 0;
+};
+
+class NoloadLookupConsumer : public SemaConsumer {
+public:
+ void InitializeSema(Sema &S) override { SemaPtr = &S; }
+
+ bool HandleTopLevelDecl(DeclGroupRef D) override {
+ if (!D.isSingleDecl())
+ return true;
+
+ Decl *TD = D.getSingleDecl();
+
+ auto *ID = dyn_cast<ImportDecl>(TD);
+ if (!ID)
+ return true;
+
+ Module *M = ID->getImportedModule();
+ assert(M);
+ if (M->Name != "R")
+ return true;
+
+ auto *Std = SemaPtr->getStdNamespace();
+ EXPECT_TRUE(Std);
+ TrivialVisibleDeclConsumer Consumer;
+ SemaPtr->LookupVisibleDecls(Std, Sema::LookupNameKind::LookupOrdinaryName,
+ Consumer,
+ /*IncludeGlobalScope=*/true,
+ /*IncludeDependentBases=*/false,
+ /*LoadExternal=*/false);
+ EXPECT_EQ(Consumer.FoundNum, 1);
+ return true;
+ }
+
+private:
+ Sema *SemaPtr = nullptr;
+};
+
+class NoloadLookupAction : public ASTFrontendAction {
+ std::unique_ptr<ASTConsumer>
+ CreateASTConsumer(CompilerInstance &CI, StringRef /*Unused*/) override {
+ return std::make_unique<NoloadLookupConsumer>();
+ }
+};
+
+TEST_F(NoloadLookupTest, NonModulesTest) {
+ GenerateModuleInterface("M", R"cpp(
+module;
+namespace std {
+ int What();
+
+ void bar(int x = What()) {
+ }
+}
+export module M;
+export using std::bar;
+ )cpp");
+
+ GenerateModuleInterface("R", R"cpp(
+module;
+namespace std {
+ class Another;
+ int What(Another);
+ int What();
+}
+export module R;
+ )cpp");
+
+ const char *test_file_contents = R"cpp(
+import M;
+namespace std {
+ void use() {
+ bar();
+ }
+}
+import R;
+ )cpp";
+ std::string DepArg = "-fprebuilt-module-path=" + TestDir.str().str();
+ EXPECT_TRUE(runToolOnCodeWithArgs(std::make_unique<NoloadLookupAction>(),
+ test_file_contents,
+ {
+ "-std=c++20",
+ DepArg.c_str(),
+ "-I",
+ TestDir.c_str(),
+ },
+ "test.cpp"));
+}
+
+} // namespace
More information about the cfe-commits
mailing list