Author: Chuanqi Xu Date: 2024-04-28T15:23:39+08:00 New Revision: 487967af82053cd08022635a2ff768385d936c80
URL: https://github.com/llvm/llvm-project/commit/487967af82053cd08022635a2ff768385d936c80 DIFF: https://github.com/llvm/llvm-project/commit/487967af82053cd08022635a2ff768385d936c80.diff LOG: [Modules] Don't replace local declarations with external declaration with lower visibility Close https://github.com/llvm/llvm-project/issues/88400 For the reproducer: ``` //--- header.h namespace N { template<typename T> concept X = true; template<X T> class Y { public: template<X U> friend class Y; }; inline Y<int> x; } //--- bar.cppm module; export module bar; namespace N { // To make sure N::Y won't get elided. using N::x; } //--- foo.cc // expected-no-diagnostics import bar; void y() { N::Y<int> y{}; }; ``` it will crash. The root cause is that in `StoredDeclsList::replaceExternalDecls`, we will replace the existing declarations with external declarations. Then for the reproducer, the redecl chain for Y is like: ``` Y (Local) -> Y (Local, friend) -> Y (Imported) -> Y(Imported, friend) ``` Before the lookup, the stored lookup result is `Y(Local)` then we find `Y(Imported)`. And now we repalce `Y(Local)` with `Y(Imported)`. But `Y(Imported)` is not visible. So we tried to find if there is any redeclarations visible but we find `Y(Local, friend)`, then problem happens. The solution is try to avoid the replace to happen if the external declaration has lower visibility then we can always find the local declarations. This may help the lookup performance slightly. Also I found the implementation of `StoredDeclsList::replaceExternalDecls` is not efficiency. It has an `O(n*m)` complexities. But let's improve that in the future. Added: clang/test/Modules/pr88400.cppm Modified: clang/include/clang/AST/DeclContextInternals.h Removed: ################################################################################ diff --git a/clang/include/clang/AST/DeclContextInternals.h b/clang/include/clang/AST/DeclContextInternals.h index c4734ab5789538..42cc677f82135e 100644 --- a/clang/include/clang/AST/DeclContextInternals.h +++ b/clang/include/clang/AST/DeclContextInternals.h @@ -160,12 +160,16 @@ class StoredDeclsList { void replaceExternalDecls(ArrayRef<NamedDecl*> Decls) { // Remove all declarations that are either external or are replaced with - // external declarations. + // external declarations with higher visibilities. erase_if([Decls](NamedDecl *ND) { if (ND->isFromASTFile()) return true; + // FIXME: Can we get rid of this loop completely? for (NamedDecl *D : Decls) - if (D->declarationReplaces(ND, /*IsKnownNewer=*/false)) + // Only replace the local declaration if the external declaration has + // higher visibilities. + if (D->getModuleOwnershipKind() <= ND->getModuleOwnershipKind() && + D->declarationReplaces(ND, /*IsKnownNewer=*/false)) return true; return false; }); diff --git a/clang/test/Modules/pr88400.cppm b/clang/test/Modules/pr88400.cppm new file mode 100644 index 00000000000000..ff69137a0b9040 --- /dev/null +++ b/clang/test/Modules/pr88400.cppm @@ -0,0 +1,61 @@ +// RUN: rm -rf %t +// RUN: mkdir -p %t +// RUN: split-file %s %t +// +// RUN: %clang_cc1 -std=c++20 %t/bar.cppm -emit-module-interface -o %t/bar.pcm +// RUN: %clang_cc1 -std=c++20 %t/foo.cc -fmodule-file=bar=%t/bar.pcm -fsyntax-only -verify +// RUN: %clang_cc1 -std=c++20 %t/bar.cc -fmodule-file=bar=%t/bar.pcm -fsyntax-only -verify +// +// RUN: %clang_cc1 -std=c++20 %t/bar.cppm -emit-reduced-module-interface -o %t/bar.pcm +// RUN: %clang_cc1 -std=c++20 %t/foo.cc -fmodule-file=bar=%t/bar.pcm -fsyntax-only -verify +// RUN: %clang_cc1 -std=c++20 %t/bar.cc -fmodule-file=bar=%t/bar.pcm -fsyntax-only -verify + +//--- header.h +#pragma once + +namespace N { + template<typename T> + concept X = true; + + template<X T> + class Y { + public: + template<X U> + friend class Y; + }; + + inline Y<int> x; +} + +//--- bar.cppm +module; + +#include "header.h" + +export module bar; + +namespace N { + // To make sure N::Y won't get elided. + using N::x; +} + +//--- foo.cc +// expected-no-diagnostics +#include "header.h" + +import bar; + +void y() { + N::Y<int> y{}; +}; + +//--- bar.cc +// expected-no-diagnostics +import bar; + +#include "header.h" + +void y() { + N::Y<int> y{}; +}; + _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
