Skip to content

Commit

Permalink
[Modules] Don't replace local declarations with external declaration …
Browse files Browse the repository at this point in the history
…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.
  • Loading branch information
ChuanqiXu9 committed Apr 28, 2024
1 parent 37eb9c9 commit 487967a
Show file tree
Hide file tree
Showing 2 changed files with 67 additions and 2 deletions.
8 changes: 6 additions & 2 deletions clang/include/clang/AST/DeclContextInternals.h
Expand Up @@ -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;
});
Expand Down
61 changes: 61 additions & 0 deletions 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{};
};

0 comments on commit 487967a

Please sign in to comment.