Deferred Concept Instantiation Implementation Take 2

[erichkeane]

Summary

This is a continuation of D119544. Based on @rsmith 's feed back
showing me https://eel.is/c++draft/temp#friend-9, We should properly
handle friend functions now.

There are two followups that will need to be made on this (1 to make
sure these get properly differentiated in modules, and 1 to make sure
they get mangled differently see: https://reviews.llvm.org/D110641), but
they will need the visitor we developed here.

Note the changes vs the previous accepted versions of this all start in
SemaOverload.cpp, and call into the SemaTemplate work (For the visitor)
plus a declaration in Sema.h.

[erichkeane]

In D126907#3711956, @ChuanqiXu wrote:

In D126907#3710656, @erichkeane wrote:

Ok, fixed the test failure in clang, AND it managed to fix
all the failures in libcxx!

HOWEVER, it appears that libcxx/test/libcxx/modules_include.sh.cpp
is now hanging?

I don't know much about the modules implementation (perhaps someone
like @ChuanqiXu can help out?), so I'm at least somewhat stuck until
I can figure out how to get it to give me more info.

I may not be able to look into the details recently. What's the error message from the modules?

At the moment, there IS no error message! Just that the modules_include.sh.cpp test now takes a REALLY long time (I thought it was a 'hang', but it finished overnight). So it is more like an extreme compile-time regression. I can't make heads or tails of what the test is SUPPOSED to be doing, so I don't have a good idea what the issue is, nor what is happening...

[erichkeane]

In D126907#3711956, @ChuanqiXu wrote:

In D126907#3710656, @erichkeane wrote:

Ok, fixed the test failure in clang, AND it managed to fix
all the failures in libcxx!

HOWEVER, it appears that libcxx/test/libcxx/modules_include.sh.cpp
is now hanging?

I don't know much about the modules implementation (perhaps someone
like @ChuanqiXu can help out?), so I'm at least somewhat stuck until
I can figure out how to get it to give me more info.

I may not be able to look into the details recently. What's the error message from the modules?

Looking deeper... this test is a monstrosity (https://github.com/llvm/llvm-project/blob/main/libcxx/test/libcxx/modules_include.sh.cpp), and I'm not necessarily sure it is necessarily modules-related, other than enabling it on every header. So it just seems like one of the STL headers is taking significantly longer to compile? I have no idea how to move forward with this... it AT LEAST "finishes", but it takes a very long time to get through now.

[ChuanqiXu]

In D126907#3712363, @erichkeane wrote:

In D126907#3711956, @ChuanqiXu wrote:

In D126907#3710656, @erichkeane wrote:

Ok, fixed the test failure in clang, AND it managed to fix
all the failures in libcxx!

HOWEVER, it appears that libcxx/test/libcxx/modules_include.sh.cpp
is now hanging?

I don't know much about the modules implementation (perhaps someone
like @ChuanqiXu can help out?), so I'm at least somewhat stuck until
I can figure out how to get it to give me more info.

I may not be able to look into the details recently. What's the error message from the modules?

Looking deeper... this test is a monstrosity (https://github.com/llvm/llvm-project/blob/main/libcxx/test/libcxx/modules_include.sh.cpp), and I'm not necessarily sure it is necessarily modules-related, other than enabling it on every header. So it just seems like one of the STL headers is taking significantly longer to compile? I have no idea how to move forward with this... it AT LEAST "finishes", but it takes a very long time to get through now.

I tried to reproduce and I am not sure if reproduced. With this patch, modules_include.sh.cpp takes 562s to complete. And without this patch, it takes a 557s. So it looks not your fault.

[erichkeane]

In D126907#3715436, @ChuanqiXu wrote:

In D126907#3712363, @erichkeane wrote:

In D126907#3711956, @ChuanqiXu wrote:

In D126907#3710656, @erichkeane wrote:

Ok, fixed the test failure in clang, AND it managed to fix
all the failures in libcxx!

HOWEVER, it appears that libcxx/test/libcxx/modules_include.sh.cpp
is now hanging?

I don't know much about the modules implementation (perhaps someone
like @ChuanqiXu can help out?), so I'm at least somewhat stuck until
I can figure out how to get it to give me more info.

I may not be able to look into the details recently. What's the error message from the modules?

Looking deeper... this test is a monstrosity (https://github.com/llvm/llvm-project/blob/main/libcxx/test/libcxx/modules_include.sh.cpp), and I'm not necessarily sure it is necessarily modules-related, other than enabling it on every header. So it just seems like one of the STL headers is taking significantly longer to compile? I have no idea how to move forward with this... it AT LEAST "finishes", but it takes a very long time to get through now.

I tried to reproduce and I am not sure if reproduced. With this patch, modules_include.sh.cpp takes 562s to complete. And without this patch, it takes a 557s. So it looks not your fault.

Hmm... ok, thanks for checking! I'm going to try to prove to myself that it isn't too bad then. This might just be a case of the Debug version of this taking a horribly long-time and me not noticing it normally.

If that is the case, I think this is ready for review!

[erichkeane]

I think I've proved to myself that there IS a compile-time regression, but it is reasonably minor. I suspect a lot of it is going to be the 'friend' comparisons not caching, which I might look into doing.

[erichkeane]

SO, I charted the differences in runtimes, and my patch is BETTER time-wise in release mode, but WORSE with asserts enabled. I don't know of any expensive assert that I added. SO, if everyone could review this, I'd love to get this committed to see if it beats the buildbots this time:)

[ChuanqiXu]

I am not sure if I don't miss any points. But if it is OK to run libc++/libstdc++, I think it should be good to land this.

clang/include/clang/Sema/Sema.h

3678

The meaning of TemplateDepth is unclear. Do it mean top-down or start from the constraint expression?

clang/include/clang/Sema/Template.h

508–519

Could we remove the duplicates? For example, is it possible to make ConstraintEvalRAII a subclass of Sema?

clang/lib/Sema/SemaTemplateDeduction.cpp

2851–2864

nit:

clang/lib/Sema/SemaTemplateInstantiate.cpp

1232

Do we have any method to detect if the template parameter list lives in a require clause or not? The current duplicating looks a little bit bad.

If there is no such methods, I guess it may be better by using enums:

TemplateParameterList *TransformTemplateParameterList(
                              TemplateParameterList *OrigTPL, enum IsInRequire = Normal) {
   ...
   if (IsInRequire == ...)
      DeclInstantiator.setEvaluateConstraints(EvaluateConstraints);
   ...
}

3598–3599

clang/lib/Sema/SemaTemplateInstantiateDecl.cpp

2448

Is this used?

[erichkeane]

clang/include/clang/Sema/Sema.h

3678

It is top-down, as the constraint is uninstantiated. I've added a comment to clarify.

clang/include/clang/Sema/Template.h

508–519

It ends up having to be a template, and for a 'getter' to exist, but done.

clang/lib/Sema/SemaTemplateDeduction.cpp

2851–2864

These are specializations, so they cant have a storage class. They inherit them from the primary template.

clang/lib/Sema/SemaTemplateInstantiate.cpp

1232

Turns out I don't think this was required anyway, so I just removed it.

clang/lib/Sema/SemaTemplateInstantiateDecl.cpp

2448

Yes, see line 2453, 2459, 2470, and 2476.

[erichkeane]

Respond/fix all of the things @ChuanqiXu mentioned. Intend to commit early tomorrow based on latest feedback unless others have concerns.

[Mordante]

This change breaks libc++'s modular build see build https://buildkite.com/llvm-project/libcxx-ci/builds/12991
Reverting this commit on top of yesterday's build (before your revert) fixes the issue.

libc++'s modular build uses Clang's pre-C++20 modules.

It can be reproduced building libc++ and running the following lit invocation
${LIT} -Denable_modules=True libcxx/test/libcxx/ranges/range.adaptors/range.all/all.nodiscard.verify.cpp

If you need assistance testing a new version of this patch on libc++ please let me know.

[erichkeane]

In D126907#3737375, @Mordante wrote:

This change breaks libc++'s modular build see build https://buildkite.com/llvm-project/libcxx-ci/builds/12991
Reverting this commit on top of yesterday's build (before your revert) fixes the issue.

libc++'s modular build uses Clang's pre-C++20 modules.

It can be reproduced building libc++ and running the following lit invocation
${LIT} -Denable_modules=True libcxx/test/libcxx/ranges/range.adaptors/range.all/all.nodiscard.verify.cpp

If you need assistance testing a new version of this patch on libc++ please let me know.

Is this not part of check-runtimes or check-cxx? I ran both of those INCLUDING a modules-builds-all...

[Mordante]

In D126907#3737641, @erichkeane wrote:

In D126907#3737375, @Mordante wrote:

This change breaks libc++'s modular build see build https://buildkite.com/llvm-project/libcxx-ci/builds/12991
Reverting this commit on top of yesterday's build (before your revert) fixes the issue.

libc++'s modular build uses Clang's pre-C++20 modules.

It can be reproduced building libc++ and running the following lit invocation
${LIT} -Denable_modules=True libcxx/test/libcxx/ranges/range.adaptors/range.all/all.nodiscard.verify.cpp

If you need assistance testing a new version of this patch on libc++ please let me know.

Is this not part of check-runtimes or check-cxx? I ran both of those INCLUDING a modules-builds-all...

It should be tested in check-cxx when you use the cmake option -DLIBCXX_TEST_PARAMS="enable_modules=True" and you need to configure libc++ to use your just build clang to be used for the tests. (The command I posted just does a single test, which is faster during bisecting.)

I'm not sure what you mean with modules-builds-all.

[erichkeane]

In D126907#3737986, @Mordante wrote:

In D126907#3737641, @erichkeane wrote:

In D126907#3737375, @Mordante wrote:

This change breaks libc++'s modular build see build https://buildkite.com/llvm-project/libcxx-ci/builds/12991
Reverting this commit on top of yesterday's build (before your revert) fixes the issue.

libc++'s modular build uses Clang's pre-C++20 modules.

It can be reproduced building libc++ and running the following lit invocation
${LIT} -Denable_modules=True libcxx/test/libcxx/ranges/range.adaptors/range.all/all.nodiscard.verify.cpp

If you need assistance testing a new version of this patch on libc++ please let me know.

Is this not part of check-runtimes or check-cxx? I ran both of those INCLUDING a modules-builds-all...

It should be tested in check-cxx when you use the cmake option -DLIBCXX_TEST_PARAMS="enable_modules=True" and you need to configure libc++ to use your just build clang to be used for the tests. (The command I posted just does a single test, which is faster during bisecting.)

I'm not sure what you mean with modules-builds-all.

There was a test I dealt with previously where a ton of the header files were run with modules enabled (and an auto generated files), so I'm shocked to find there is MORE differences here. @ldionne : This is another example where trying to test libcxx is particularly difficult to do apparently.

[Mordante]

In D126907#3738383, @erichkeane wrote:

There was a test I dealt with previously where a ton of the header files were run with modules enabled (and an auto generated files), so I'm shocked to find there is MORE differences here. @ldionne : This is another example where trying to test libcxx is particularly difficult to do apparently.

I disagree; testing libcxx is easy. Unfortunately what's missing is good documentation explaining how to test different configurations. A similar libc++ related issue came up in another Clang review recently where the libc++ CI setup was unclear. Afterwards I had a talk with @aaron.ballman and I agreed to improve the documentation. While improving the documentation I noticed some issues with our CI setup. So before publishing documentation that doesn't reflect reality, I first wanted to fix these issues. While testing my fixes I ran into the build failure due to this patch. So now we're at a full circle.

With the revert of this patch it's possible to fix the CI issues for libc++ and I will continue with improving the documentation.

Note that this specific issue isn't directly detected in the libc++ CI. This would require a full clang build and testing with modules. That's not done, full builds are only tested without modules. If wanted I supply a patch how to test this configuration in the libc++ CI.

If you need further assistance feel free to reach out to me or other libc++ developers, the easiest way to reach us is #libcxx on Discord.

[aaron.ballman]

In D126907#3739750, @Mordante wrote:

In D126907#3738383, @erichkeane wrote:

There was a test I dealt with previously where a ton of the header files were run with modules enabled (and an auto generated files), so I'm shocked to find there is MORE differences here. @ldionne : This is another example where trying to test libcxx is particularly difficult to do apparently.

I disagree; testing libcxx is easy.

FWIW, that's not been my experience. I can't even get libcxx to build for me on Windows with Visual Studio cmake integration, let alone test it. These are the results I got when I tried again today from the instructions from the website:

CMake Error in F:/source/llvm-project/libcxx/benchmarks/CMakeLists.txt:
  No known features for CXX compiler

  "Clang"

  version 16.0.0.


CMake Generate step failed.  Build files cannot be regenerated correctly.
FAILED: runtimes/runtimes-stamps/runtimes-configure

That said, I recall at one point I was able to get libcxx to build for me, but then I ran into issues getting the tests to run (and we document this on the website: "Building with Visual Studio currently does not permit running tests."). The result is that I stopped trying to build or test libcxx locally ages ago and I rely entirely on the bots to tell me if something's gone wrong.

Unfortunately what's missing is good documentation explaining how to test different configurations. A similar libc++ related issue came up in another Clang review recently where the libc++ CI setup was unclear. Afterwards I had a talk with @aaron.ballman and I agreed to improve the documentation. While improving the documentation I noticed some issues with our CI setup. So before publishing documentation that doesn't reflect reality, I first wanted to fix these issues. While testing my fixes I ran into the build failure due to this patch. So now we're at a full circle.

And I greatly appreciate the improvements that have been going on here!

[erichkeane]

In D126907#3739750, @Mordante wrote:

In D126907#3738383, @erichkeane wrote:

There was a test I dealt with previously where a ton of the header files were run with modules enabled (and an auto generated files), so I'm shocked to find there is MORE differences here. @ldionne : This is another example where trying to test libcxx is particularly difficult to do apparently.

I disagree; testing libcxx is easy. Unfortunately what's missing is good documentation explaining how to test different configurations. A similar libc++ related issue came up in another Clang review recently where the libc++ CI setup was unclear. Afterwards I had a talk with @aaron.ballman and I agreed to improve the documentation. While improving the documentation I noticed some issues with our CI setup. So before publishing documentation that doesn't reflect reality, I first wanted to fix these issues. While testing my fixes I ran into the build failure due to this patch. So now we're at a full circle.

With the revert of this patch it's possible to fix the CI issues for libc++ and I will continue with improving the documentation.

Note that this specific issue isn't directly detected in the libc++ CI. This would require a full clang build and testing with modules. That's not done, full builds are only tested without modules. If wanted I supply a patch how to test this configuration in the libc++ CI.

If you need further assistance feel free to reach out to me or other libc++ developers, the easiest way to reach us is #libcxx on Discord.

This is the 3rd time some non-obvious-how-to-run libcxx test failure has caused a revert request to this patch, which is obviously getting frustrating. Additionally, in none of the 3 cases was I alerted automatically.

I appreciate that there are actually ways to RUN these tests on my machine (Aaron is not as lucky, see above), but the lack of a "test all the libcxx things" flag is shocking. It seems that I'd need at least 2 separate CMake directories to test these configurations? That seems shocking to me.

[erichkeane]

FWIW, while I wasn't able to reproduce the problem that @Mordante reported, I found that the test suite as given has a large number of asserts in debug mode while trying to compare parameter-mappings during constraint normalization(assert is clang-15: /iusers/ekeane1/workspaces/delayed-concepts/clang/lib/AST/ASTContext.cpp:4753: clang::QualType clang::ASTContext::getSubstTemplateTypeParmType(const clang::TemplateTypeParmType*, clang::QualType, llvm::Optional<unsigned int>) const: Assertion `Replacement.isCanonical() && "replacement types must always be canonical"' failed.`).

One Reproducer is:

#pragma clang module import std.array
template<std::input_or_output_iterator>
struct iter_value_or_void { using type = void; };
  
template<std::input_iterator I>
struct iter_value_or_void<I> {
    using type = std::iter_value_t<I>;

BUT debugging seems to show a lot of 'imported' symbols that I'm not particularly sure how they work, so I suspect they are coming from the clang-module-import.

So I'm going to have to try to figure out what is going on here with the clang modules.

[Mordante]

In D126907#3739923, @aaron.ballman wrote:

In D126907#3739750, @Mordante wrote:

In D126907#3738383, @erichkeane wrote:

There was a test I dealt with previously where a ton of the header files were run with modules enabled (and an auto generated files), so I'm shocked to find there is MORE differences here. @ldionne : This is another example where trying to test libcxx is particularly difficult to do apparently.

I disagree; testing libcxx is easy.

FWIW, that's not been my experience. I can't even get libcxx to build for me on Windows with Visual Studio cmake integration, let alone test it.

Fair point, I can't vouch for how well that build is supported, since I don't have access to Windows. This version isn't tested in libc++ CI. The other two Windows builds CMake + ninja (MSVC) and CMake + ninja (MSVC) are tested in libc++ CI. The wording on the website is similar to what is used in libc++ CI libcxx/utils/ci/run-buildbot but it misses the -DLIBCXX_EXTRA_SITE_DEFINES="_LIBCPP_HAS_NO_INT128".

Unfortunately what's missing is good documentation explaining how to test different configurations. A similar libc++ related issue came up in another Clang review recently where the libc++ CI setup was unclear. Afterwards I had a talk with @aaron.ballman and I agreed to improve the documentation. While improving the documentation I noticed some issues with our CI setup. So before publishing documentation that doesn't reflect reality, I first wanted to fix these issues. While testing my fixes I ran into the build failure due to this patch. So now we're at a full circle.

And I greatly appreciate the improvements that have been going on here!

You're welcome!

In D126907#3739967, @erichkeane wrote:

In D126907#3739750, @Mordante wrote:

If you need further assistance feel free to reach out to me or other libc++ developers, the easiest way to reach us is #libcxx on Discord.

This is the 3rd time some non-obvious-how-to-run libcxx test failure has caused a revert request to this patch, which is obviously getting frustrating. Additionally, in none of the 3 cases was I alerted automatically.

I understand that it's frustrating. I haven't seen the other two reverts, but by default non libc++ patches aren't tested in the libc++ pre-commit CI. The reason is simple; a pre-commit build runs 50+ builds and takes about one hour to complete. So the CI doesn't have the capacity to test every Clang diff. If you want to run the libc++ pre-commit CI the easiest way is to add a dummy file in the libc++ tree. Note that will add the libc++ review group to this review.

Next to testing pre-commit changes it tests main every 4 hours, but doesn't send e-mails when an error occurs. This is something @ldionne wanted to look into.

This behaviour is exactly the kind of documentation that is missing for Clang developers and what @aaron.ballman and I talked about. (There is more documentation that should be added, but that is generic information.)

I appreciate that there are actually ways to RUN these tests on my machine (Aaron is not as lucky, see above), but the lack of a "test all the libcxx things" flag is shocking. It seems that I'd need at least 2 separate CMake directories to test these configurations? That seems shocking to me.

You can use one libc++ installation to test multiple different configuration as long as they don't use different build-time options. So testing with the different language standards c++03, c++11, c++17, c++20, c++2b, the modular build, and several other options can be done from one build. But you need to invoke the tests for each option or combination of options separately.

If you want to test without wchar_t support you need to build a different libc++, simply since that removes parts of the dylib, like the wchar_t facets from the locales. (Basically the same as when you want to test Clang with or without assertions, there you need two builds too.)

What do you exactly mean with "test all the libcxx things" flag?

[erichkeane]

I get that libcxx doesn't run on precommit CI (which is unfortunate for those of us modifying the CFE), but it becomes difficult to run locally, when check-all (requires check-runtimes or check-cxx) don't cover it, and now even check-cxx doesn't cover it.

What do you exactly mean with "test all the libcxx things" flag?

For example, this enable-modules flag isn't covered by my local check-cxx, which I went out of my way to run on this patch locally.

[erichkeane]

In D126907#3740249, @erichkeane wrote:

FWIW, while I wasn't able to reproduce the problem that @Mordante reported, I found that the test suite as given has a large number of asserts in debug mode while trying to compare parameter-mappings during constraint normalization(assert is clang-15: /iusers/ekeane1/workspaces/delayed-concepts/clang/lib/AST/ASTContext.cpp:4753: clang::QualType clang::ASTContext::getSubstTemplateTypeParmType(const clang::TemplateTypeParmType*, clang::QualType, llvm::Optional<unsigned int>) const: Assertion `Replacement.isCanonical() && "replacement types must always be canonical"' failed.`).

One Reproducer is:

#pragma clang module import std.array
template<std::input_or_output_iterator>
struct iter_value_or_void { using type = void; };
  
template<std::input_iterator I>
struct iter_value_or_void<I> {
    using type = std::iter_value_t<I>;

BUT debugging seems to show a lot of 'imported' symbols that I'm not particularly sure how they work, so I suspect they are coming from the clang-module-import.

So I'm going to have to try to figure out what is going on here with the clang modules.

Fixing the assert was pretty easy, and fixing it revealed the same 8 failures from the buildbot above.

[Mordante]

In D126907#3740536, @erichkeane wrote:

For example, this enable-modules flag isn't covered by my local check-cxx, which I went out of my way to run on this patch locally.

Unfortunately there are a lot of different options and combination of options to test libc++.
So it's indeed not possible to test all options with once check-cxx invocation.

In D126907#3746408, @erichkeane wrote:

Fixing the assert was pretty easy, and fixing it revealed the same 8 failures from the buildbot above.

Great to hear you can reproduce the issue!

[erichkeane]

Still WIP, but uploading to show that I'm still on this :/

The two modules related issues from libcxx are now fixed (as reported by @Mordante), and that configuration builds and passes all tests with MOST of this change.

HOWEVER, the first of two fixes for @wlei messes up how constraint expressions on class templates are compared, so the result is ~800 additional libcxx failures. I'm still working through that.

There is a 2nd bug I found from @wlei 's reproducer which is captured in a commented out test in concepts.cpp.

[ldionne]

In D126907#3746477, @Mordante wrote:

Unfortunately there are a lot of different options and combination of options to test libc++.
So it's indeed not possible to test all options with once check-cxx invocation.

This ^. We could include all the libc++ configurations in a single check-cxx invokation, but the tests would run for like 70 hours on most machines. Instead, we test different configurations in different CI jobs.

In D126907#3777056, @erichkeane wrote:

Still WIP, but uploading to show that I'm still on this :/

The two modules related issues from libcxx are now fixed (as reported by @Mordante), and that configuration builds and passes all tests with MOST of this change.

HOWEVER, the first of two fixes for @wlei messes up how constraint expressions on class templates are compared, so the result is ~800 additional libcxx failures. I'm still working through that.

Just to make sure we're on the same page, I assume most of those issues are things that would have been encountered in user code otherwise and filed as bugs. So in a way, I think libc++ is simply acting like some kind of early-in-the-loop pre-release testing. This is similar to what some other open-source projects like Chrome do -- they build from trunk often and will report actual issues to us early. I do get why it can be annoying and disruptive to landing patches sometimes, though.

Concretely, what we could do is probably add LLVM_ENABLE_RUNTIMES=libcxx to the Clang pre-commit CI that already runs. That would catch some (but of course not all) issues. In particular, that should catch issues that would otherwise immediately break the libc++ "Bootstrapping build" CI job. For other issues (like an arbitrary combination of -std=c++xy -fmodules -fno-rtti), I think we can only rely on slower feedback when libc++ updates the nightly version of Clang that we use in our CI (which would act like a super-early adopter from the POV of Clang at that point).

[erichkeane]

In D126907#3777088, @ldionne wrote:

In D126907#3746477, @Mordante wrote:

Unfortunately there are a lot of different options and combination of options to test libc++.
So it's indeed not possible to test all options with once check-cxx invocation.

This ^. We could include all the libc++ configurations in a single check-cxx invokation, but the tests would run for like 70 hours on most machines. Instead, we test different configurations in different CI jobs.

In D126907#3777056, @erichkeane wrote:

Still WIP, but uploading to show that I'm still on this :/

The two modules related issues from libcxx are now fixed (as reported by @Mordante), and that configuration builds and passes all tests with MOST of this change.

HOWEVER, the first of two fixes for @wlei messes up how constraint expressions on class templates are compared, so the result is ~800 additional libcxx failures. I'm still working through that.

Just to make sure we're on the same page, I assume most of those issues are things that would have been encountered in user code otherwise and filed as bugs. So in a way, I think libc++ is simply acting like some kind of early-in-the-loop pre-release testing. This is similar to what some other open-source projects like Chrome do -- they build from trunk often and will report actual issues to us early. I do get why it can be annoying and disruptive to landing patches sometimes, though.

Yep, this is the case. Just frustrating to be surprised by things like this as late in the process as I was, so I was a little grumpy above. Unfortunately it seems like each of these cycles costs a month here :/

Concretely, what we could do is probably add LLVM_ENABLE_RUNTIMES=libcxx to the Clang pre-commit CI that already runs. That would catch some (but of course not all) issues. In particular, that should catch issues that would otherwise immediately break the libc++ "Bootstrapping build" CI job. For other issues (like an arbitrary combination of -std=c++xy -fmodules -fno-rtti), I think we can only rely on slower feedback when libc++ updates the nightly version of Clang that we use in our CI (which would act like a super-early adopter from the POV of Clang at that point).

[erichkeane]

I have a replacement patch that should fix all of the bugs I'm aware of.

[erichkeane]

Now fully runs libcxx modules configuration as well, and passes the minimization examples from @wlei .

@wlei and anyone else: can you please try running this against your workloads before I commit it?

[erichkeane]

I spoke too soon, I found another crash that came out of @wlei s repro from last time.

[erichkeane]

In D126907#3805640, @erichkeane wrote:

I spoke too soon, I found another crash that came out of @wlei s repro from last time.

Actually, what I found was a reduction that had gone haywire before and generated invalid code, so I don't think it is worth blocking this patch. The actual preprocessed file from @wlei actually compiles perfectly.

[erichkeane]

Failure I noticed was NOT a regression: https://godbolt.org/z/MnvqP88vv

[wlei]

In D126907#3805606, @erichkeane wrote:

Now fully runs libcxx modules configuration as well, and passes the minimization examples from @wlei .

@wlei and anyone else: can you please try running this against your workloads before I commit it?

Thanks for the fix, I will try to run it internally today.

[Mordante]

In D126907#3805606, @erichkeane wrote:

Now fully runs libcxx modules configuration as well, and passes the minimization examples from @wlei .

@wlei and anyone else: can you please try running this against your workloads before I commit it?

I tried this with several configurations (modular, c++2b, c++20, and c++03) in libc++'s bootstrapping build and the tests passed.

[erichkeane]

In D126907#3806760, @Mordante wrote:

In D126907#3805606, @erichkeane wrote:

Now fully runs libcxx modules configuration as well, and passes the minimization examples from @wlei .

@wlei and anyone else: can you please try running this against your workloads before I commit it?

I tried this with several configurations (modular, c++2b, c++20, and c++03) in libc++'s bootstrapping build and the tests passed.

Thank you so much!

[ChuanqiXu]

Tested with our internal workloads and things look fine.

[wlei]

Tested this and confirmed the issue I reported is gone, thanks!

[erichkeane]

In D126907#3807707, @wlei wrote:

Tested this and confirmed the issue I reported is gone, thanks!

Thank you all for the quick responses!

[BertalanD]

Hi @erichkeane,

This change broke compilation of this program (https://godbolt.org/z/KrWGvcf8h; reduced from https://github.com/SerenityOS/ladybird):

template<typename T, typename U>
constexpr bool IsSame = false;

template<typename T>
constexpr bool IsSame<T, T> = true;

template<typename T>
struct Foo {
    template<typename U>
    Foo(U&&) requires (!IsSame<U, Foo>);
};

template<>
struct Foo<void> : Foo<int> {
    using Foo<int>::Foo;
};

Foo<void> test() { return 0; }

<source>:18:27: error: invalid reference to function 'Foo': constraints not satisfied
Foo<void> test() { return 0; }
                          ^
<source>:10:24: note: because substituted constraint expression is ill-formed: value of type '<dependent type>' is not contextually convertible to 'bool'
    Foo(U&&) requires (!IsSame<U, Foo>);
                       ^

[erichkeane]

In D126907#3844788, @BertalanD wrote:

Hi @erichkeane,

This change broke compilation of this program (https://godbolt.org/z/KrWGvcf8h; reduced from https://github.com/SerenityOS/ladybird):

template<typename T, typename U>
constexpr bool IsSame = false;

template<typename T>
constexpr bool IsSame<T, T> = true;

template<typename T>
struct Foo {
    template<typename U>
    Foo(U&&) requires (!IsSame<U, Foo>);
};

template<>
struct Foo<void> : Foo<int> {
    using Foo<int>::Foo;
};

Foo<void> test() { return 0; }

<source>:18:27: error: invalid reference to function 'Foo': constraints not satisfied
Foo<void> test() { return 0; }
                          ^
<source>:10:24: note: because substituted constraint expression is ill-formed: value of type '<dependent type>' is not contextually convertible to 'bool'
    Foo(U&&) requires (!IsSame<U, Foo>);
                       ^

Thanks for the report! I'll look into it ASAP.

[erichkeane]

In D126907#3846591, @erichkeane wrote:

In D126907#3844788, @BertalanD wrote:

Hi @erichkeane,

This change broke compilation of this program (https://godbolt.org/z/KrWGvcf8h; reduced from https://github.com/SerenityOS/ladybird):

template<typename T, typename U>
constexpr bool IsSame = false;

template<typename T>
constexpr bool IsSame<T, T> = true;

template<typename T>
struct Foo {
    template<typename U>
    Foo(U&&) requires (!IsSame<U, Foo>);
};

template<>
struct Foo<void> : Foo<int> {
    using Foo<int>::Foo;
};

Foo<void> test() { return 0; }

<source>:18:27: error: invalid reference to function 'Foo': constraints not satisfied
Foo<void> test() { return 0; }
                          ^
<source>:10:24: note: because substituted constraint expression is ill-formed: value of type '<dependent type>' is not contextually convertible to 'bool'
    Foo(U&&) requires (!IsSame<U, Foo>);
                       ^

Thanks for the report! I'll look into it ASAP.

Quick note: I believe I understand the cause of this, which requires a bit more work than I otherwise would have expected. I have a candidate patch I'm running through my testing right now that should fix this, but it still needs cleaning up. Expect it in the next day or two if all goes well.

[BertalanD]

In D126907#3849583, @erichkeane wrote:

In D126907#3846591, @erichkeane wrote:

In D126907#3844788, @BertalanD wrote:

Hi @erichkeane,

This change broke compilation of this program (https://godbolt.org/z/KrWGvcf8h; reduced from https://github.com/SerenityOS/ladybird):

template<typename T, typename U>
constexpr bool IsSame = false;

template<typename T>
constexpr bool IsSame<T, T> = true;

template<typename T>
struct Foo {
    template<typename U>
    Foo(U&&) requires (!IsSame<U, Foo>);
};

template<>
struct Foo<void> : Foo<int> {
    using Foo<int>::Foo;
};

Foo<void> test() { return 0; }

<source>:18:27: error: invalid reference to function 'Foo': constraints not satisfied
Foo<void> test() { return 0; }
                          ^
<source>:10:24: note: because substituted constraint expression is ill-formed: value of type '<dependent type>' is not contextually convertible to 'bool'
    Foo(U&&) requires (!IsSame<U, Foo>);
                       ^

Thanks for the report! I'll look into it ASAP.

Quick note: I believe I understand the cause of this, which requires a bit more work than I otherwise would have expected. I have a candidate patch I'm running through my testing right now that should fix this, but it still needs cleaning up. Expect it in the next day or two if all goes well.

Thank you for your quick response and for creating this massive yak shave of a patch :D

Please ping me if you want me to test the fix on our code, or if I can help in some other way.

[erichkeane]

In D126907#3849583, @erichkeane wrote:

In D126907#3846591, @erichkeane wrote:

In D126907#3844788, @BertalanD wrote:

Hi @erichkeane,

This change broke compilation of this program (https://godbolt.org/z/KrWGvcf8h; reduced from https://github.com/SerenityOS/ladybird):

template<typename T, typename U>
constexpr bool IsSame = false;

template<typename T>
constexpr bool IsSame<T, T> = true;

template<typename T>
struct Foo {
    template<typename U>
    Foo(U&&) requires (!IsSame<U, Foo>);
};

template<>
struct Foo<void> : Foo<int> {
    using Foo<int>::Foo;
};

Foo<void> test() { return 0; }

<source>:18:27: error: invalid reference to function 'Foo': constraints not satisfied
Foo<void> test() { return 0; }
                          ^
<source>:10:24: note: because substituted constraint expression is ill-formed: value of type '<dependent type>' is not contextually convertible to 'bool'
    Foo(U&&) requires (!IsSame<U, Foo>);
                       ^

Thanks for the report! I'll look into it ASAP.

Quick note: I believe I understand the cause of this, which requires a bit more work than I otherwise would have expected. I have a candidate patch I'm running through my testing right now that should fix this, but it still needs cleaning up. Expect it in the next day or two if all goes well.

In D126907#3850735, @BertalanD wrote:

In D126907#3849583, @erichkeane wrote:

In D126907#3846591, @erichkeane wrote:

In D126907#3844788, @BertalanD wrote:

Hi @erichkeane,

This change broke compilation of this program (https://godbolt.org/z/KrWGvcf8h; reduced from https://github.com/SerenityOS/ladybird):

template<typename T, typename U>
constexpr bool IsSame = false;

template<typename T>
constexpr bool IsSame<T, T> = true;

template<typename T>
struct Foo {
    template<typename U>
    Foo(U&&) requires (!IsSame<U, Foo>);
};

template<>
struct Foo<void> : Foo<int> {
    using Foo<int>::Foo;
};

Foo<void> test() { return 0; }

<source>:18:27: error: invalid reference to function 'Foo': constraints not satisfied
Foo<void> test() { return 0; }
                          ^
<source>:10:24: note: because substituted constraint expression is ill-formed: value of type '<dependent type>' is not contextually convertible to 'bool'
    Foo(U&&) requires (!IsSame<U, Foo>);
                       ^

Thanks for the report! I'll look into it ASAP.

Quick note: I believe I understand the cause of this, which requires a bit more work than I otherwise would have expected. I have a candidate patch I'm running through my testing right now that should fix this, but it still needs cleaning up. Expect it in the next day or two if all goes well.

Thank you for your quick response and for creating this massive yak shave of a patch :D

Please ping me if you want me to test the fix on our code, or if I can help in some other way.

Thanks for the offer! I ended up taking a less-aggressive yak shave on this one, and have a patch here: https://reviews.llvm.org/D135772

If you could give it a try, it would be very useful!

[BertalanD]

I tried out D135772, and our build got significantly farther than before! I unfortunately discovered another piece of code that pre babdef27c503c0bbbcc017e9f88affddda90ea4e Clang and GCC accept in C++20 mode, but Clang trunk does not (https://godbolt.org/z/q1q4nfobK):

struct String {
  String(char *);
  bool operator==(String const &);
  void operator!=(String const &);
};

extern char* c;
extern String s;
int test() {
  return c == s;
}

<source>:10:12: error: invalid operands to binary expression ('char *' and 'String')
  return c == s;
         ~ ^  ~

[erichkeane]

In D126907#3852909, @BertalanD wrote:

I tried out D135772, and our build got significantly farther than before! I unfortunately discovered another piece of code that pre babdef27c503c0bbbcc017e9f88affddda90ea4e Clang and GCC accept in C++20 mode, but Clang trunk does not (https://godbolt.org/z/q1q4nfobK):

struct String {
  String(char *);
  bool operator==(String const &);
  void operator!=(String const &);
};

extern char* c;
extern String s;
int test() {
  return c == s;
}

<source>:10:12: error: invalid operands to binary expression ('char *' and 'String')
  return c == s;
         ~ ^  ~

Thanks! I'll look into that as well.

[BertalanD]

(I know this is a very contrived example with void operator!=, but that is what CVise spat out, and the actual failures are related to comparison operators too)

edit: hah, it repros when it returns bool too -- not sure how that void came to be...

[erichkeane]

In D126907#3852975, @BertalanD wrote:

(I know this is a very contrived example with void operator!=, but that is what CVise spat out, and the actual failures are related to comparison operators too)

edit: hah, it repros when it returns bool too -- not sure how that void came to be...

Looking more closely at that, I don't think it has anything to do with this patch. I know operator== (and the materialized inverse) stuff was worked on separately recently by @Uthkarsh in 38b9d313e6945804fffc654f849cfa05ba2c713d. See: https://reviews.llvm.org/D134529

The Equality-operator stuff has been particularly awkward in the C++ committee I think (whether or not reversed candidates work or not have had a bunch of tweaking), so you'll have to follow that up there.

[cor3ntin]

@erichkeane I was looking at Daisy meta programming shenanigans and found this crashes https://cute.godbolt.org/z/6fWoEheKc - seems related to your work, probably? It used to work in Clang 15. I thought you might want to know!

[erichkeane]

In D126907#3854983, @cor3ntin wrote:

@erichkeane I was looking at Daisy meta programming shenanigans and found this crashes https://cute.godbolt.org/z/6fWoEheKc - seems related to your work, probably? It used to work in Clang 15. I thought you might want to know!

Thanks! I'll take a look. That DOES look like some other failures I've had with this patch.

[erichkeane]

In D126907#3855552, @erichkeane wrote:

In D126907#3854983, @cor3ntin wrote:

@erichkeane I was looking at Daisy meta programming shenanigans and found this crashes https://cute.godbolt.org/z/6fWoEheKc - seems related to your work, probably? It used to work in Clang 15. I thought you might want to know!

Thanks! I'll take a look. That DOES look like some other failures I've had with this patch.

I've reduced it a bit to remove the STL stuff, but it looks like the template-lambda is a part of it: https://cute.godbolt.org/z/Prh6xsesz

It manages to hit quite a few things that I suspect the getTemplateInstantiationArgs doesn't get right, so I'm guessing the 'fix' is going to be in there somewhere. I'll start taking a look this week.

[erichkeane]

Daisy's bug ended up being more complicated than I expected... there is a lot to unpack here. In the meantime, I've captured the bug here: https://github.com/llvm/llvm-project/issues/58368 and will continue looking at it.

[mizvekov]

clang/lib/Sema/SemaConcept.cpp

513–534

Unchecked access to MLTAL (Optional).

Following reduction reproduces a crash here: -cc1 -std=c++20 -fsyntax-only -ferror-limit 19.

template a b < ;
template c e ag < ;
ah) ;
am = ;
template <class... ap> class aq {
  aq(ap...; __attribute__) auto aj() requires(am)
}
f() {                  [;                  aq d;                  d.aj

[mizvekov]

clang/lib/Sema/SemaConcept.cpp

513–534

By the way, disregard that repro, it needs another patch that is not in master.

The unchecked access still does look suspicious though.

[erichkeane]

clang/lib/Sema/SemaConcept.cpp

513–534

Thanks for the comment! I'll look into it.

[erichkeane]

clang/lib/Sema/SemaConcept.cpp

513–534

Huh... that must be leftover from some other attempt I made at that, the MLTAL is always set. I'll do an NFC patch to remove it, but thanks for looking!

[erichkeane]

clang/lib/Sema/SemaConcept.cpp

513–534

Woops... nvm... you're right, in an error condition we can get an errored one. I'll add a check. Thanks again!