Skip to content

[C++20] [Modules] The entity with internal linkage shouldn't be accessed in other TU even if they are in the same module #61427

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
ChuanqiXu9 opened this issue Mar 15, 2023 · 7 comments · Fixed by #123059
Labels
clang:modules C++20 modules and Clang Header Modules

Comments

@ChuanqiXu9
Copy link
Member

ChuanqiXu9 commented Mar 15, 2023

Reproducer:

export module m:a;
static int a = 32;
export module m:b;
import :a;
int b() {
    return a;
}

Currently the reproducer is accepted. But we expect the compiler to reject it and saying that the partition b can't access the static variable 'a' in partition a.

@ChuanqiXu9 ChuanqiXu9 added the clang:modules C++20 modules and Clang Header Modules label Mar 15, 2023
@llvmbot
Copy link
Member

llvmbot commented Mar 15, 2023

@llvm/issue-subscribers-clang-modules

@iains
Copy link
Contributor

iains commented Mar 15, 2023

Also, I believe we should reject this simpler case where the implementation implicitly imports the interface.

export module S;
static void f() {}
module S;

void foo() {
  f ();
}

@iains
Copy link
Contributor

iains commented Mar 16, 2023

https://reviews.llvm.org/D126959 fixes some of the cases without any change to Sema::isModuleUnitOfCurrentTU since it makes the implementation unambiguously a different TU.

@iains
Copy link
Contributor

iains commented Mar 16, 2023

(there is still more to do, of course, I also plan to update https://reviews.llvm.org/D145965 to remove the changes to Sema::isModuleUnitOfCurrentTU [which @ChuanqiXu9 is intending to handle separately])

@cor3ntin
Copy link
Contributor

@iains @ChuanqiXu9 someone was asking me about this, are you still working on it?

@iains
Copy link
Contributor

iains commented Aug 24, 2024

@iains @ChuanqiXu9 someone was asking me about this, are you still working on it?

I am not, for the foreseeable future.

@ChuanqiXu9
Copy link
Member Author

@iains @ChuanqiXu9 someone was asking me about this, are you still working on it?

This is still on my TODO list. But it may not be too soon given my time schedule.

ChuanqiXu9 added a commit to ChuanqiXu9/llvm-project that referenced this issue Jan 15, 2025
…ther TU

Close llvm#61427

And this is also helpful to implement
llvm#112294 partially.

The implementation strategy mimics
llvm#122887. This patch split the
internal declarations from the general lookup table so that other TU
can't find the internal declarations.
ChuanqiXu9 added a commit to ChuanqiXu9/llvm-project that referenced this issue Jan 17, 2025
…ther TU

Close llvm#61427

And this is also helpful to implement
llvm#112294 partially.

The implementation strategy mimics
llvm#122887. This patch split the
internal declarations from the general lookup table so that other TU
can't find the internal declarations.
ChuanqiXu9 added a commit that referenced this issue Jan 17, 2025
…ther TU (#123059)

Close #61427

And this is also helpful to implement
#112294 partially.

The implementation strategy mimics
#122887. This patch split the
internal declarations from the general lookup table so that other TU
can't find the internal declarations.
github-actions bot pushed a commit to arm/arm-toolchain that referenced this issue Jan 17, 2025
… found by other TU (#123059)

Close llvm/llvm-project#61427

And this is also helpful to implement
llvm/llvm-project#112294 partially.

The implementation strategy mimics
llvm/llvm-project#122887. This patch split the
internal declarations from the general lookup table so that other TU
can't find the internal declarations.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
clang:modules C++20 modules and Clang Header Modules
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants