Skip to content

Commit 487967a

Browse files
committed
[Modules] Don't replace local declarations with external declaration with lower visibility
Close #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.
1 parent 37eb9c9 commit 487967a

File tree

2 files changed

+67
-2
lines changed

2 files changed

+67
-2
lines changed

clang/include/clang/AST/DeclContextInternals.h

Lines changed: 6 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -160,12 +160,16 @@ class StoredDeclsList {
160160

161161
void replaceExternalDecls(ArrayRef<NamedDecl*> Decls) {
162162
// Remove all declarations that are either external or are replaced with
163-
// external declarations.
163+
// external declarations with higher visibilities.
164164
erase_if([Decls](NamedDecl *ND) {
165165
if (ND->isFromASTFile())
166166
return true;
167+
// FIXME: Can we get rid of this loop completely?
167168
for (NamedDecl *D : Decls)
168-
if (D->declarationReplaces(ND, /*IsKnownNewer=*/false))
169+
// Only replace the local declaration if the external declaration has
170+
// higher visibilities.
171+
if (D->getModuleOwnershipKind() <= ND->getModuleOwnershipKind() &&
172+
D->declarationReplaces(ND, /*IsKnownNewer=*/false))
169173
return true;
170174
return false;
171175
});

clang/test/Modules/pr88400.cppm

Lines changed: 61 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,61 @@
1+
// RUN: rm -rf %t
2+
// RUN: mkdir -p %t
3+
// RUN: split-file %s %t
4+
//
5+
// RUN: %clang_cc1 -std=c++20 %t/bar.cppm -emit-module-interface -o %t/bar.pcm
6+
// RUN: %clang_cc1 -std=c++20 %t/foo.cc -fmodule-file=bar=%t/bar.pcm -fsyntax-only -verify
7+
// RUN: %clang_cc1 -std=c++20 %t/bar.cc -fmodule-file=bar=%t/bar.pcm -fsyntax-only -verify
8+
//
9+
// RUN: %clang_cc1 -std=c++20 %t/bar.cppm -emit-reduced-module-interface -o %t/bar.pcm
10+
// RUN: %clang_cc1 -std=c++20 %t/foo.cc -fmodule-file=bar=%t/bar.pcm -fsyntax-only -verify
11+
// RUN: %clang_cc1 -std=c++20 %t/bar.cc -fmodule-file=bar=%t/bar.pcm -fsyntax-only -verify
12+
13+
//--- header.h
14+
#pragma once
15+
16+
namespace N {
17+
template<typename T>
18+
concept X = true;
19+
20+
template<X T>
21+
class Y {
22+
public:
23+
template<X U>
24+
friend class Y;
25+
};
26+
27+
inline Y<int> x;
28+
}
29+
30+
//--- bar.cppm
31+
module;
32+
33+
#include "header.h"
34+
35+
export module bar;
36+
37+
namespace N {
38+
// To make sure N::Y won't get elided.
39+
using N::x;
40+
}
41+
42+
//--- foo.cc
43+
// expected-no-diagnostics
44+
#include "header.h"
45+
46+
import bar;
47+
48+
void y() {
49+
N::Y<int> y{};
50+
};
51+
52+
//--- bar.cc
53+
// expected-no-diagnostics
54+
import bar;
55+
56+
#include "header.h"
57+
58+
void y() {
59+
N::Y<int> y{};
60+
};
61+

0 commit comments

Comments
 (0)