Page MenuHomePhabricator

[C++20][Modules] Update ADL to handle basic.lookup.argdep p4 [P1815R2 part 1]
ClosedPublic

Authored by iains on Jul 6 2022, 1:14 AM.

Details

Summary

This includes the revised provisions of [basic.lookup.argdep] p4

  1. ADL is amended to handle p 4.3 where functions in trasitively imported modules may

become visible when they are exported in the same namespace as a visible type.

  1. If a function is in a different modular TU, and has internal-linkage, we invalidate

its entry in an overload set.

[basic.lookup.argdep] p5 ex 2 now passes.

Diff Detail

Event Timeline

iains created this revision.Jul 6 2022, 1:14 AM
Herald added a project: Restricted Project. · View Herald TranscriptJul 6 2022, 1:14 AM
iains published this revision for review.Jul 6 2022, 1:18 AM
iains added reviewers: urnathan, ChuanqiXu.
iains added a subscriber: Restricted Project.
Herald added a project: Restricted Project. · View Herald TranscriptJul 6 2022, 1:18 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript
iains edited the summary of this revision. (Show Details)Jul 6 2022, 1:20 AM
ChuanqiXu added inline comments.Jul 6 2022, 2:31 AM
clang/include/clang/Sema/Overload.h
800
clang/lib/Sema/SemaOverload.cpp
6403
6406

The current implementation may reject following two cases:

module;
static void foo(int); // Ignore header
...
export module A;
void bar() { foo(5); } // Should it be invalid?

and

export module A;
static void foo(int);
...
module :private;
void bar() { foo(5); } // Should it be invalid?

I mean in the above examples, the function of foo(int) is defined in the same TU but the call to it might be rejected.

clang/test/CXX/basic/basic.link/p10-ex2.cpp
10–35 ↗(On Diff #442459)

I feel like the test is irrelevant with the revision, isn't it? If yes, I think we could land this as a separate NFC patch.

clang/test/CXX/basic/basic.lookup/basic.lookup.argdep/p5-ex2.cpp
37–42

This looks more consistent with the example.

iains marked 2 inline comments as done.Jul 6 2022, 3:41 AM
iains added inline comments.
clang/lib/Sema/SemaOverload.cpp
6406

The current implementation may reject following two cases:

module;
static void foo(int); // Ignore header
...
export module A;
void bar() { foo(5); } // Should it be invalid?

and

export module A;
static void foo(int);
...
module :private;
void bar() { foo(5); } // Should it be invalid?

I mean in the above examples, the function of foo(int) is defined in the same TU but the call to it might be rejected.

6406

The current implementation may reject following two cases:

module;
static void foo(int); // Ignore header
...

Actually, I have a note to check on the global module case, since we have special rules that allow merging of items there,

export module A;
void bar() { foo(5); } // Should it be invalid?

and

export module A;
static void foo(int);
...
module :private;
void bar() { foo(5); } // Should it be invalid?

I mean in the above examples, the function of `foo(int)` is defined in the same TU but the call to it might be rejected.

otherwise, agreed, these cases should be ok - I guess we need a second test case with lookups that should succeed.

clang/test/CXX/basic/basic.link/p10-ex2.cpp
10–35 ↗(On Diff #442459)

well, it is related to the paper mentioned, but yes we can make this separate.

clang/test/CXX/basic/basic.lookup/basic.lookup.argdep/p5-ex2.cpp
37–42

This changes the example so that M is directly included in the implementation rather than transitively (the example does specifically use a different module name for the implementation)

I am not sure what you mean by "more consistent"
(the example will fail to reject some of the lookups with this change).

iains updated this revision to Diff 442505.Jul 6 2022, 3:52 AM
iains marked 4 inline comments as done.

address review comments.

iains added a comment.Jul 6 2022, 3:53 AM

TODO: second test-case for cases that should succeed, split out basic-link / p10-ex2 test case.

ChuanqiXu added inline comments.Jul 6 2022, 10:44 PM
clang/lib/Sema/SemaOverload.cpp
6406

Actually, I have a note to check on the global module case, since we have special rules that allow merging of items there,

Yeah, it is surprising that we couldn't handle the global module fragment case.

clang/test/CXX/basic/basic.lookup/basic.lookup.argdep/p5-ex2.cpp
37–42

Oh, I didn't notice it uses a different module name. My bad.

iains updated this revision to Diff 443194.Jul 8 2022, 3:40 AM
iains retitled this revision from [C++20][Modules] Invalidate internal-linkage functions in overload sets [P1815R2 part 1] to [C++20][Modules] Update ADL to handle basic.lookup.argdep p4 [P1815R2 part 1].
iains edited the summary of this revision. (Show Details)

updated after clarification of the provisions of basic.lookup.argdep p4

iains marked an inline comment as done.Jul 8 2022, 3:41 AM
ChuanqiXu added inline comments.Jul 10 2022, 7:49 PM
clang/lib/Sema/SemaLookup.cpp
3870

nit: Although it should be true due to D is ExportDeclContext, it looks better to add an assertion at the first sight.

3873–3874
3875–3889

nit: how do you think about the suggested style? (not required)

3878

The std says module instead of module unit. So I think we should consider partition unit here. So:

  • We need to add a test about partition in the revision.
  • We need to add isInSameModule methods in other revisions. I know we've talked this several times before... I'll take a look.
3882–3884

Maybe it is worth to implement getNonInlineEnclosingNamespaceContext and we could simplify the code here:

clang/lib/Sema/SemaOverload.cpp
6407

I introduced isModuleUnitOfCurrentTU to simplify the code a little bit.

iains updated this revision to Diff 445530.Mon, Jul 18, 9:00 AM
iains marked 5 inline comments as done.

rebased, addressed review comments.

iains added a comment.Mon, Jul 18, 9:01 AM

sorry got a little delayed in responding to this (sorting out some testing problems)

clang/lib/Sema/SemaLookup.cpp
3870

OK, Since exports must be in module purview and not in PMF (which we should expect from existing checks)
let's then check we have a correct context for export - which will avoid needing to re-check this below.

3873–3874

OK, we can use this here, although it is probably a little more than necessary, since we already know that FM cannot be a GMF or PMF ( but the function will check those anyway)

3875–3889

I guess it's a little shorter, and roughly the same in readability,

3878

The std says module instead of module unit. So I think we should consider partition unit here.

yes, I guess so - it is a bit unfortunate that we need to use the name comparison again.

So:

  • We need to add a test about partition in the revision.

TODO, probably better to add another example rather than modify the text of the standard one.

3882–3884

Perhaps. but I would prefer that to be a separate cleanup - since it would be touching code unrelated to this.
Also that does not actually save any work, it might be better to find a way to cache the computation at the time the associated entities are found.

ChuanqiXu accepted this revision.Mon, Jul 18, 7:27 PM

LGTM basically if the following comments addressed.

clang/lib/Sema/SemaLookup.cpp
3875–3889

The current implementation skips a break of the outer loop.

clang/lib/Sema/SemaOverload.cpp
6407

This is not addressed.

This revision is now accepted and ready to land.Mon, Jul 18, 7:27 PM
iains updated this revision to Diff 447046.Sat, Jul 23, 1:56 AM

rebased, addressed review comments

iains marked 2 inline comments as done.Sat, Jul 23, 2:00 AM

it would be good to get this landed before llvm-15 branches..

clang/lib/Sema/SemaLookup.cpp
3875–3889

I'm not sure exactly what you are referring to here (maybe I missed something) -

ChuanqiXu added inline comments.Sun, Jul 24, 7:01 PM
clang/lib/Sema/SemaLookup.cpp
3875–3889

hmmm, sorry, I misread something. It should be fine.

This revision was landed with ongoing or failed builds.Mon, Jul 25, 6:29 AM
This revision was automatically updated to reflect the committed changes.

FYI

When I implements/tests the modularized STL for libstdc++ (https://github.com/ChuanqiXu9/stdmodules), I find the patch blocks it. Due to the time is limited (I really wish we could have something workable in clang15), I made a pre-review commit in: https://github.com/llvm/llvm-project/commit/15ddc09ef9b05ffd5398165049b5202264fa203a

The problem shows in the tests:

struct A {};

 template<typename Func>
 constexpr bool __call_is_nt(A)
 {
     return true;
 }
 ns::A make();

 template <typename T>
 bool foo(T t) {
     auto func = [](){};
     return __call_is_nt<decltype(func)>(t);
 }

The linkage of __call_is_nt<decltype(func)> in clang is internal linkage. So it is refused by this patch. This is not the fault of the patch. According to [basic.link], only names have linkage. So __call_is_nt<decltype(func)> should have same linkage with its template, which should be external.

The root cause for the problem is that the semantics of internal linkage in clang is a mixed form of internal linkage and TU-Local in C++ Specifications. So here is the slight difference. It is hard to fix the root problem so I choose to give a workaround here.