This is an archive of the discontinued LLVM Phabricator instance.

Deferred Concept Instantiation Implementation Take 2
ClosedPublic

Authored by erichkeane on Jun 2 2022, 12:01 PM.

Details

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.

Diff Detail

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes

See here: https://github.com/llvm/llvm-project/issues/55673

This might be the same issue, the crash at least looks the same.

Note: Looks to not be the same problem :/ Additionally, the fix I thought would work here does not, so I'm back at square 1 on that crash again. I was trying to move the 'isEvaluatingAConstraint' out of Sema (since we don't want to isntantiate a type constraint until we are checking it!) but that causes us to fail a number of OTHER tests for checking an uninstantiated constraint.

From what I can see, the crash reason would be the mismatch depth and the setting of MultiLevelTemplateArgumentList. In ::CheckConstraintSatisfaction, the depth of RawCompletionToken is 0, while the depth of corresponding MultiLevelTemplateArgumentList is 2. So the compiler would get the outermost template argument incorrectly (which is a template pack in the example) in TransformTemplateTypeParmType.

The first though was that we should set the depth of RawCompletionToken to 1 correctly. But I felt this might not be good later since in the normal process of template instantiation (without concepts and constraints), the depth of RawCompletionToken is 0 indeed. What is different that time is the depth of corresponding MultiLevelTemplateArgumentList is 1. So it looks like the process is constructed on the fly. It makes sense for the perspective of compilation speed.

So I feel like what we should do here is in Sema::CheckInstantiatedFunctionTemplateConstraints, when we are computing the desired MultiLevelTemplateArgumentList, we should compute a result with depth of 1 in the particular case.


Another idea is to not do instantiation when we're checking constraints. But I need to think more about this.

From what I can see, the crash reason would be the mismatch depth and the setting of MultiLevelTemplateArgumentList. In ::CheckConstraintSatisfaction, the depth of RawCompletionToken is 0, while the depth of corresponding MultiLevelTemplateArgumentList is 2. So the compiler would get the outermost template argument incorrectly (which is a template pack in the example) in TransformTemplateTypeParmType.

The first though was that we should set the depth of RawCompletionToken to 1 correctly. But I felt this might not be good later since in the normal process of template instantiation (without concepts and constraints), the depth of RawCompletionToken is 0 indeed. What is different that time is the depth of corresponding MultiLevelTemplateArgumentList is 1. So it looks like the process is constructed on the fly. It makes sense for the perspective of compilation speed.

So I feel like what we should do here is in Sema::CheckInstantiatedFunctionTemplateConstraints, when we are computing the desired MultiLevelTemplateArgumentList, we should compute a result with depth of 1 in the particular case.


Another idea is to not do instantiation when we're checking constraints. But I need to think more about this.

I don't know of the 'another idea'? We have to do instantiation before checking, and if we do it too early, we aren't doing the deferred instantiation. And the problem is that the RawCompletionToken uses a concept to refer to 'itself'. However, this version of 'itself' isn't valid, since the 'depth' is wrong once it tries to instantiate relative to the 'top level'. However, this IS happening during instantiation?

My latest thought is that perhaps the "IsEvaluatingAConstraint" should NOT happen at the Sema level (but at the instantiator level), since it ends up catching things it shouldn't? I tried it and have a handful of failures of trying to check uninstantiated constraints, but I've not dug completely into it.

From what I can see, the crash reason would be the mismatch depth and the setting of MultiLevelTemplateArgumentList. In ::CheckConstraintSatisfaction, the depth of RawCompletionToken is 0, while the depth of corresponding MultiLevelTemplateArgumentList is 2. So the compiler would get the outermost template argument incorrectly (which is a template pack in the example) in TransformTemplateTypeParmType.

The first though was that we should set the depth of RawCompletionToken to 1 correctly. But I felt this might not be good later since in the normal process of template instantiation (without concepts and constraints), the depth of RawCompletionToken is 0 indeed. What is different that time is the depth of corresponding MultiLevelTemplateArgumentList is 1. So it looks like the process is constructed on the fly. It makes sense for the perspective of compilation speed.

So I feel like what we should do here is in Sema::CheckInstantiatedFunctionTemplateConstraints, when we are computing the desired MultiLevelTemplateArgumentList, we should compute a result with depth of 1 in the particular case.


Another idea is to not do instantiation when we're checking constraints. But I need to think more about this.

I don't know of the 'another idea'? We have to do instantiation before checking, and if we do it too early, we aren't doing the deferred instantiation. And the problem is that the RawCompletionToken uses a concept to refer to 'itself'. However, this version of 'itself' isn't valid, since the 'depth' is wrong once it tries to instantiate relative to the 'top level'. However, this IS happening during instantiation?

My latest thought is that perhaps the "IsEvaluatingAConstraint" should NOT happen at the Sema level (but at the instantiator level), since it ends up catching things it shouldn't? I tried it and have a handful of failures of trying to check uninstantiated constraints, but I've not dug completely into it.

Yeah, we have to do instantiation before checking. My point is that it looks like we're doing another instantiation when we check the concepts. And my point is that if it we should avoid the second instantiation.

From what I can see, the crash reason would be the mismatch depth and the setting of MultiLevelTemplateArgumentList. In ::CheckConstraintSatisfaction, the depth of RawCompletionToken is 0, while the depth of corresponding MultiLevelTemplateArgumentList is 2. So the compiler would get the outermost template argument incorrectly (which is a template pack in the example) in TransformTemplateTypeParmType.

The first though was that we should set the depth of RawCompletionToken to 1 correctly. But I felt this might not be good later since in the normal process of template instantiation (without concepts and constraints), the depth of RawCompletionToken is 0 indeed. What is different that time is the depth of corresponding MultiLevelTemplateArgumentList is 1. So it looks like the process is constructed on the fly. It makes sense for the perspective of compilation speed.

So I feel like what we should do here is in Sema::CheckInstantiatedFunctionTemplateConstraints, when we are computing the desired MultiLevelTemplateArgumentList, we should compute a result with depth of 1 in the particular case.


Another idea is to not do instantiation when we're checking constraints. But I need to think more about this.

I don't know of the 'another idea'? We have to do instantiation before checking, and if we do it too early, we aren't doing the deferred instantiation. And the problem is that the RawCompletionToken uses a concept to refer to 'itself'. However, this version of 'itself' isn't valid, since the 'depth' is wrong once it tries to instantiate relative to the 'top level'. However, this IS happening during instantiation?

My latest thought is that perhaps the "IsEvaluatingAConstraint" should NOT happen at the Sema level (but at the instantiator level), since it ends up catching things it shouldn't? I tried it and have a handful of failures of trying to check uninstantiated constraints, but I've not dug completely into it.

Yeah, we have to do instantiation before checking. My point is that it looks like we're doing another instantiation when we check the concepts. And my point is that if it we should avoid the second instantiation.

Hmm... yeah, I think we actually want to skip the FIRST instantiation? I think the work I did above in Sema for IsEvaluatingAConstraint was too greedy, it ends up instantiating constraints on the 'while we are there' type things, rather than the things being currently checked. I found that if I can make it a state of the instantiators themselves, it seems to work, at least for your example. I've got it down to 1 'lit' test failure, but have been looking at the other problem first (below).

The Github-Issue crash IS a regression from this patch, and I've minimized it sufficiently. I believe I have a hold on how to fix it, I just have to do so :) If I make any further progress, I'll clean this up and put it up here, even if it DOES have the lit test failure.

Made good progress today, but got trapped down the CheckDeducedArgs path for quite a bit longer with partial specializations. I have all of the crashes I know of 'fixed', but have a few tests that still fail for unknown reasons. Because of that, i don't have anything I can really upload, so I'll have to do so mid-next-week.

As promised, got it down to the 1 failure, and added tests for @ChuanqiXu and the std::vector example. That all seems to work, just a problem with the CXX/temp/temp.arg/temp.arg.template/p3-2a.cpp test left.

Note that the failure comes down to:

template<typename T> concept C = T::f();
template<template<C> class P> struct S1{};
template<C> struct X{};
S1<X> s11;

and requires the -frelaxed-template-template-args flag:

[ekeane1@scsel-clx-24 build]$  ./bin/clang -cc1 -std=c++20 temp.cpp -frelaxed-template-template-args
temp.cpp:5:4: error: template template argument 'X' is more constrained than template template parameter 'P'
S1<X> s11;
   ^
temp.cpp:3:29: note: 'P' declared here
template <template<C> class P> struct S1{};
                            ^
temp.cpp:4:20: note: 'X' declared here
template<C> struct X{};
                   ^
1 error generated.

Great progress!

Note that the failure comes down to:

template<typename T> concept C = T::f();
template<template<C> class P> struct S1{};
template<C> struct X{};
S1<X> s11;

and requires the -frelaxed-template-template-args flag:

[ekeane1@scsel-clx-24 build]$  ./bin/clang -cc1 -std=c++20 temp.cpp -frelaxed-template-template-args
temp.cpp:5:4: error: template template argument 'X' is more constrained than template template parameter 'P'
S1<X> s11;
   ^
temp.cpp:3:29: note: 'P' declared here
template <template<C> class P> struct S1{};
                            ^
temp.cpp:4:20: note: 'X' declared here
template<C> struct X{};
                   ^
1 error generated.

As far as I could tell, we could omit the diagnostic by deleting https://github.com/llvm/llvm-project/blob/bc74bca5363270e987c2e3c263bfaaeb6ceab66f/clang/include/clang/Sema/SemaConcept.h#L45-L53

This change is obsolutely wrong but it shows the bug comes from ParameterMapping so we could locate https://github.com/llvm/llvm-project/blob/bc74bca5363270e987c2e3c263bfaaeb6ceab66f/clang/lib/Sema/SemaConcept.cpp#L723 further.

clang/lib/Sema/SemaTemplateInstantiate.cpp
2488
clang/test/Driver/crash-report.cpp
28 ↗(On Diff #438781)

What's the meaning of RUNX?

erichkeane marked an inline comment as done.Jun 22 2022, 6:05 AM

Great progress!

Note that the failure comes down to:

template<typename T> concept C = T::f();
template<template<C> class P> struct S1{};
template<C> struct X{};
S1<X> s11;

and requires the -frelaxed-template-template-args flag:

[ekeane1@scsel-clx-24 build]$  ./bin/clang -cc1 -std=c++20 temp.cpp -frelaxed-template-template-args
temp.cpp:5:4: error: template template argument 'X' is more constrained than template template parameter 'P'
S1<X> s11;
   ^
temp.cpp:3:29: note: 'P' declared here
template <template<C> class P> struct S1{};
                            ^
temp.cpp:4:20: note: 'X' declared here
template<C> struct X{};
                   ^
1 error generated.

As far as I could tell, we could omit the diagnostic by deleting https://github.com/llvm/llvm-project/blob/bc74bca5363270e987c2e3c263bfaaeb6ceab66f/clang/include/clang/Sema/SemaConcept.h#L45-L53

This change is obsolutely wrong but it shows the bug comes from ParameterMapping so we could locate https://github.com/llvm/llvm-project/blob/bc74bca5363270e987c2e3c263bfaaeb6ceab66f/clang/lib/Sema/SemaConcept.cpp#L723 further.

Thanks for the debugging help! I'm not sure what you mean by "Locating it further"? Best I can tell looking so far, is the fact that the 'depths' still point based on the 'root', but this is comparing thinking they are relative to the decl themselves. I see the 'SubstTemplateArguments' there that looks suspicious, so there is probably something about setting up my template parameters correctly there/the MLTAL.

Thanks for the comments and debugging help again!

clang/test/Driver/crash-report.cpp
28 ↗(On Diff #438781)

Woops! I didn't add this test change to GIT, but it apparently still showed up with 'diff'. For some reason, once I switched to lldb the llvm-symbolizer takes a HUGE amount of time during this test when doing check-all, so I used the 'X' to disable the test locally. This is not intended to be committed.

All tests pass now, I was able to get the template-template checks working correctly, and it passes all the tests I have available. @ChuanqiXu if you could review/run what tests you have, it would be greatly appreciated.

All tests pass now, I was able to get the template-template checks working correctly, and it passes all the tests I have available. @ChuanqiXu if you could review/run what tests you have, it would be greatly appreciated.

I've tested some of our workloads and libunifex (I know it contains a lot of uses for concept). And all the tests passed now. So it looks like it wouldn't cause regression failure.

The implementation basically looks good to me. (This is really large and I can't be sure I don't miss something). Only some minor issues remained.

clang/include/clang/AST/Decl.h
1961–1962

Could we give a new abstract like XXXInfo to replace NamedDecl* here? The NamedDecl* covers a lot of things. It looks more consistent.

clang/include/clang/Sema/Sema.h
7181–7192

Remove this before landing.

7182

ditto

clang/lib/AST/Decl.cpp
3793

static_cast is rarely used in clang/LLVM to me. I know the reason is that TemplateOrSpecialization contains a NamedDecl* member. I guess it might be better to refactor it.

clang/lib/AST/DeclBase.cpp
286–304 ↗(On Diff #440712)

How about:

clang/lib/Sema/SemaConcept.cpp
65
69
184–185
clang/lib/Sema/SemaTemplate.cpp
2346–2347
erichkeane marked 9 inline comments as done.Jun 29 2022, 8:03 AM

All tests pass now, I was able to get the template-template checks working correctly, and it passes all the tests I have available. @ChuanqiXu if you could review/run what tests you have, it would be greatly appreciated.

I've tested some of our workloads and libunifex (I know it contains a lot of uses for concept). And all the tests passed now. So it looks like it wouldn't cause regression failure.

The implementation basically looks good to me. (This is really large and I can't be sure I don't miss something). Only some minor issues remained.

Thank you SO much for your testing and reviews, I very much appreciate all the help you've given me. I'm sorry that this has become such a huge patch, I really wish it was something I could have done in smaller increments, but that doesn't seem like it is possible since it is changing something so fundamental, and the fallout from that is huge.

clang/include/clang/AST/Decl.h
1961–1962

All of those 'Info' types contain significantly more information, which we really don't have a need for here. Basically all this patch is doing is using the FunctionTemplateDecl* that was already in the list and generalizing it a bit. AND unfortunately, the only commonality between FunctionTemplateDecl and FunctionDecl is NamedDecl.

So any type we would use would end up causing an additional allocation here, and make its use require us to unpack it :/

I guess what I'm saying, is I'm not sure what that would look like in a way that wouldn't make this worse. Do you have an idea you could share that would improve that?

clang/include/clang/Sema/Sema.h
7181–7192

Yikes! Thanks for catching this!

clang/lib/AST/Decl.cpp
3793

Ah, looks like the static-casts weren't necessary anyway. No idea why I thought they were. Removed. See comment above about the refactor.

erichkeane marked 2 inline comments as done.

All but the 1 comment from @ChuanqiXu fixed, not sure what to do about the 'info'.

ChuanqiXu accepted this revision.Jun 29 2022, 7:15 PM

All but the 1 comment from @ChuanqiXu fixed, not sure what to do about the 'info'.

LGTM. But the 'info' comment doesn't matter a lot. The current complexity is acceptable and its semantics is stated clearly in the comments. My intention is to make its semantics more clear and more readable so that it is easier to maintain and change it. But this is required. Since the patch is really large. So my preference is to do other unnecessary changes in other revision to make the patch easier to read. (Just a preference. I know some people prefer large patches.)

clang/include/clang/AST/Decl.h
1961–1962

My idea would be something like:

C++
llvm::PointerUnion<FunctionDecl *, FunctionTemplateDecl*, MemberSpecializationInfo *,
                     FunctionTemplateSpecializationInfo *,

So any type we would use would end up causing an additional allocation here, and make its use require us to unpack it :/

This is a union so it wouldn't cause additional allocations?

This revision is now accepted and ready to land.Jun 29 2022, 7:15 PM
erichkeane added inline comments.Jun 30 2022, 6:16 AM
clang/include/clang/AST/Decl.h
1961–1962

I tried doing FunctionDecl and FunctionTemplateDecl at one point, but we endd up running out of bits for the pointer-union to work on 32 bit builds. Otherwise I would have done that.

I misunderstood the 'Info' thing and thought you wanted a new wrapper type (an 'Info' type), which would need to be allocated and contain just a pointer, which is why I was thinking about needing allocations.

This revision was landed with ongoing or failed builds.Jun 30 2022, 6:48 AM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptJun 30 2022, 6:48 AM

This change triggers an assertion when building an LLDB test case:

UNREACHABLE executed at /Users/buildslave/jenkins/workspace/lldb-cmake/llvm-project/clang/lib/Sema/SemaTemplate.cpp:1726!
Assertion failed: (InstantiatingSpecializations.empty() && "failed to clean up an InstantiatingTemplate?"), function ~Sema, file /Users/buildslave/jenkins/workspace/lldb-cmake/llvm-project/clang/lib/Sema/Sema.cpp, line 458.
PLEASE submit a bug report to https://github.com/llvm/llvm-project/issues/ and include the crash backtrace, preprocessed source, and associated run script.
Stack dump:
0.	Program arguments: /Users/buildslave/jenkins/workspace/lldb-cmake/lldb-build/bin/clang -fmodules -gmodules -fmodules-cache-path=/Users/buildslave/jenkins/workspace/lldb-cmake/lldb-build/lldb-test-build.noindex/module-cache-clang/lldb-api -gmodules -fcxx-modules -std=c++11 -g -O0 -isysroot /Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX10.15.sdk -arch x86_64 -I/Users/buildslave/jenkins/workspace/lldb-cmake/llvm-project/lldb/packages/Python/lldbsuite/test/make/../../../../../include -I/Users/buildslave/jenkins/workspace/lldb-cmake/llvm-project/lldb/test/API/functionalities/data-formatter/data-formatter-stl/libcxx/span -I/Users/buildslave/jenkins/workspace/lldb-cmake/llvm-project/lldb/packages/Python/lldbsuite/test/make -include /Users/buildslave/jenkins/workspace/lldb-cmake/llvm-project/lldb/packages/Python/lldbsuite/test/make/test_common.h -fno-limit-debug-info -fmodules -gmodules -fmodules-cache-path=/Users/buildslave/jenkins/workspace/lldb-cmake/lldb-build/lldb-test-build.noindex/module-cache-clang/lldb-api -gmodules -fcxx-modules -DLLDB_USING_LIBCPP -stdlib=libc++ -std=c++20 --driver-mode=g++ -MT main.o -MD -MP -MF main.d -c -o main.o /Users/buildslave/jenkins/workspace/lldb-cmake/llvm-project/lldb/test/API/functionalities/data-formatter/data-formatter-stl/libcxx/span/main.cpp
1.	/Users/buildslave/jenkins/workspace/lldb-cmake/llvm-project/lldb/test/API/functionalities/data-formatter/data-formatter-stl/libcxx/span/main.cpp:1:2: current parser token 'include'
Stack dump without symbol names (ensure you have llvm-symbolizer in your PATH or set the environment var `LLVM_SYMBOLIZER_PATH` to point to it):
0  clang                    0x0000000106bc157e llvm::sys::PrintStackTrace(llvm::raw_ostream&, int) + 46
1  clang                    0x0000000106bc0258 llvm::sys::RunSignalHandlers() + 248
2  clang                    0x0000000106bc0962 llvm::sys::CleanupOnSignal(unsigned long) + 210
3  clang                    0x0000000106adb82a (anonymous namespace)::CrashRecoveryContextImpl::HandleCrash(int, unsigned long) + 106
4  clang                    0x0000000106adba2e CrashRecoverySignalHandler(int) + 110
5  libsystem_platform.dylib 0x00007fff67b405fd _sigtramp + 29
6  libsystem_platform.dylib 0x00007ffeeab5b460 _sigtramp + 18446744071612509824
7  libsystem_c.dylib        0x00007fff67a12808 abort + 120
8  libsystem_c.dylib        0x00007fff67a11ac6 err + 0
9  clang                    0x0000000108a1325c clang::Sema::~Sema() + 5660
10 clang                    0x0000000107698e41 clang::CompilerInstance::~CompilerInstance() + 625
11 clang                    0x00000001076a7fe5 compileModuleImpl(clang::CompilerInstance&, clang::SourceLocation, llvm::StringRef, clang::FrontendInputFile, llvm::StringRef, llvm::StringRef, llvm::function_ref<void (clang::CompilerInstance&)>, llvm::function_ref<void (clang::CompilerInstance&)>) + 4517
12 clang                    0x00000001076a9b16 compileModuleAndReadASTImpl(clang::CompilerInstance&, clang::SourceLocation, clang::SourceLocation, clang::Module*, llvm::StringRef) + 1238
13 clang                    0x00000001076a370b compileModuleAndReadAST(clang::CompilerInstance&, clang::SourceLocation, clang::SourceLocation, clang::Module*, llvm::StringRef) + 1947
14 clang                    0x00000001076a2b97 clang::CompilerInstance::findOrCompileModuleAndReadAST(llvm::StringRef, clang::SourceLocation, clang::SourceLocation, bool) + 3783
15 clang                    0x00000001076a3aa7 clang::CompilerInstance::loadModule(clang::SourceLocation, llvm::ArrayRef<std::__1::pair<clang::IdentifierInfo*, clang::SourceLocation>>, clang::Module::NameVisibilityKind, bool) + 695
16 clang                    0x0000000109b8a630 clang::Preprocessor::HandleHeaderIncludeOrImport(clang::SourceLocation, clang::Token&, clang::Token&, clang::SourceLocation, clang::detail::SearchDirIteratorImpl<true>, clang::FileEntry const*) + 7792
17 clang                    0x0000000109b822e1 clang::Preprocessor::HandleIncludeDirective(clang::SourceLocation, clang::Token&, clang::detail::SearchDirIteratorImpl<true>, clang::FileEntry const*) + 177
18 clang                    0x0000000109b82f8c clang::Preprocessor::HandleDirective(clang::Token&) + 2604
19 clang                    0x0000000109b50877 clang::Lexer::LexTokenInternal(clang::Token&, bool) + 6199
20 clang                    0x0000000109b4cf15 clang::Lexer::Lex(clang::Token&) + 133
21 clang                    0x0000000109bc4889 clang::Preprocessor::Lex(clang::Token&) + 89
22 clang                    0x000000010899438a clang::Parser::Initialize() + 5018
23 clang                    0x00000001088b9759 clang::ParseAST(clang::Sema&, bool, bool) + 441
24 clang                    0x000000010772aac3 clang::FrontendAction::Execute() + 99
25 clang                    0x000000010769fb6f clang::CompilerInstance::ExecuteAction(clang::FrontendAction&) + 863
26 clang                    0x00000001077b6493 clang::ExecuteCompilerInvocation(clang::CompilerInstance*) + 707
27 clang                    0x00000001050aa241 cc1_main(llvm::ArrayRef<char const*>, char const*, void*) + 2065
28 clang                    0x00000001050a56ad ExecuteCC1Tool(llvm::SmallVectorImpl<char const*>&) + 285
29 clang                    0x00000001074b2227 void llvm::function_ref<void ()>::callback_fn<clang::driver::CC1Command::Execute(llvm::ArrayRef<llvm::Optional<llvm::StringRef>>, std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char>>*, bool*) const::$_4>(long) + 23
30 clang                    0x0000000106adb73c llvm::CrashRecoveryContext::RunSafely(llvm::function_ref<void ()>) + 236
31 clang                    0x00000001074b1c95 clang::driver::CC1Command::Execute(llvm::ArrayRef<llvm::Optional<llvm::StringRef>>, std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char>>*, bool*) const + 293
32 clang                    0x0000000107474af7 clang::driver::Compilation::ExecuteCommand(clang::driver::Command const&, clang::driver::Command const*&, bool) const + 1111
33 clang                    0x0000000107474de0 clang::driver::Compilation::ExecuteJobs(clang::driver::JobList const&, llvm::SmallVectorImpl<std::__1::pair<int, clang::driver::Command const*>>&, bool) const + 144
34 clang                    0x000000010749285f clang::driver::Driver::ExecuteCompilation(clang::driver::Compilation&, llvm::SmallVectorImpl<std::__1::pair<int, clang::driver::Command const*>>&) + 911
35 clang                    0x00000001050a4d1b clang_main(int, char**) + 10315
36 libdyld.dylib            0x00007fff67943cc9 start + 1
37 libdyld.dylib            0x0000000000000026 start + 18446603338778395486

https://green.lab.llvm.org/green/view/LLDB/job/lldb-cmake/44981/testReport/junit/lldb-api/functionalities_data-formatter_data-formatter-stl_libcxx_span/TestDataFormatterLibcxxSpan_py/

This change triggers an assertion when building an LLDB test case:

UNREACHABLE executed at /Users/buildslave/jenkins/workspace/lldb-cmake/llvm-project/clang/lib/Sema/SemaTemplate.cpp:1726!
Assertion failed: (InstantiatingSpecializations.empty() && "failed to clean up an InstantiatingTemplate?"), function ~Sema, file /Users/buildslave/jenkins/workspace/lldb-cmake/llvm-project/clang/lib/Sema/Sema.cpp, line 458.
PLEASE submit a bug report to https://github.com/llvm/llvm-project/issues/ and include the crash backtrace, preprocessed source, and associated run script.
Stack dump:
0.	Program arguments: /Users/buildslave/jenkins/workspace/lldb-cmake/lldb-build/bin/clang -fmodules -gmodules -fmodules-cache-path=/Users/buildslave/jenkins/workspace/lldb-cmake/lldb-build/lldb-test-build.noindex/module-cache-clang/lldb-api -gmodules -fcxx-modules -std=c++11 -g -O0 -isysroot /Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX10.15.sdk -arch x86_64 -I/Users/buildslave/jenkins/workspace/lldb-cmake/llvm-project/lldb/packages/Python/lldbsuite/test/make/../../../../../include -I/Users/buildslave/jenkins/workspace/lldb-cmake/llvm-project/lldb/test/API/functionalities/data-formatter/data-formatter-stl/libcxx/span -I/Users/buildslave/jenkins/workspace/lldb-cmake/llvm-project/lldb/packages/Python/lldbsuite/test/make -include /Users/buildslave/jenkins/workspace/lldb-cmake/llvm-project/lldb/packages/Python/lldbsuite/test/make/test_common.h -fno-limit-debug-info -fmodules -gmodules -fmodules-cache-path=/Users/buildslave/jenkins/workspace/lldb-cmake/lldb-build/lldb-test-build.noindex/module-cache-clang/lldb-api -gmodules -fcxx-modules -DLLDB_USING_LIBCPP -stdlib=libc++ -std=c++20 --driver-mode=g++ -MT main.o -MD -MP -MF main.d -c -o main.o /Users/buildslave/jenkins/workspace/lldb-cmake/llvm-project/lldb/test/API/functionalities/data-formatter/data-formatter-stl/libcxx/span/main.cpp
1.	/Users/buildslave/jenkins/workspace/lldb-cmake/llvm-project/lldb/test/API/functionalities/data-formatter/data-formatter-stl/libcxx/span/main.cpp:1:2: current parser token 'include'
Stack dump without symbol names (ensure you have llvm-symbolizer in your PATH or set the environment var `LLVM_SYMBOLIZER_PATH` to point to it):
0  clang                    0x0000000106bc157e llvm::sys::PrintStackTrace(llvm::raw_ostream&, int) + 46
1  clang                    0x0000000106bc0258 llvm::sys::RunSignalHandlers() + 248
2  clang                    0x0000000106bc0962 llvm::sys::CleanupOnSignal(unsigned long) + 210
3  clang                    0x0000000106adb82a (anonymous namespace)::CrashRecoveryContextImpl::HandleCrash(int, unsigned long) + 106
4  clang                    0x0000000106adba2e CrashRecoverySignalHandler(int) + 110
5  libsystem_platform.dylib 0x00007fff67b405fd _sigtramp + 29
6  libsystem_platform.dylib 0x00007ffeeab5b460 _sigtramp + 18446744071612509824
7  libsystem_c.dylib        0x00007fff67a12808 abort + 120
8  libsystem_c.dylib        0x00007fff67a11ac6 err + 0
9  clang                    0x0000000108a1325c clang::Sema::~Sema() + 5660
10 clang                    0x0000000107698e41 clang::CompilerInstance::~CompilerInstance() + 625
11 clang                    0x00000001076a7fe5 compileModuleImpl(clang::CompilerInstance&, clang::SourceLocation, llvm::StringRef, clang::FrontendInputFile, llvm::StringRef, llvm::StringRef, llvm::function_ref<void (clang::CompilerInstance&)>, llvm::function_ref<void (clang::CompilerInstance&)>) + 4517
12 clang                    0x00000001076a9b16 compileModuleAndReadASTImpl(clang::CompilerInstance&, clang::SourceLocation, clang::SourceLocation, clang::Module*, llvm::StringRef) + 1238
13 clang                    0x00000001076a370b compileModuleAndReadAST(clang::CompilerInstance&, clang::SourceLocation, clang::SourceLocation, clang::Module*, llvm::StringRef) + 1947
14 clang                    0x00000001076a2b97 clang::CompilerInstance::findOrCompileModuleAndReadAST(llvm::StringRef, clang::SourceLocation, clang::SourceLocation, bool) + 3783
15 clang                    0x00000001076a3aa7 clang::CompilerInstance::loadModule(clang::SourceLocation, llvm::ArrayRef<std::__1::pair<clang::IdentifierInfo*, clang::SourceLocation>>, clang::Module::NameVisibilityKind, bool) + 695
16 clang                    0x0000000109b8a630 clang::Preprocessor::HandleHeaderIncludeOrImport(clang::SourceLocation, clang::Token&, clang::Token&, clang::SourceLocation, clang::detail::SearchDirIteratorImpl<true>, clang::FileEntry const*) + 7792
17 clang                    0x0000000109b822e1 clang::Preprocessor::HandleIncludeDirective(clang::SourceLocation, clang::Token&, clang::detail::SearchDirIteratorImpl<true>, clang::FileEntry const*) + 177
18 clang                    0x0000000109b82f8c clang::Preprocessor::HandleDirective(clang::Token&) + 2604
19 clang                    0x0000000109b50877 clang::Lexer::LexTokenInternal(clang::Token&, bool) + 6199
20 clang                    0x0000000109b4cf15 clang::Lexer::Lex(clang::Token&) + 133
21 clang                    0x0000000109bc4889 clang::Preprocessor::Lex(clang::Token&) + 89
22 clang                    0x000000010899438a clang::Parser::Initialize() + 5018
23 clang                    0x00000001088b9759 clang::ParseAST(clang::Sema&, bool, bool) + 441
24 clang                    0x000000010772aac3 clang::FrontendAction::Execute() + 99
25 clang                    0x000000010769fb6f clang::CompilerInstance::ExecuteAction(clang::FrontendAction&) + 863
26 clang                    0x00000001077b6493 clang::ExecuteCompilerInvocation(clang::CompilerInstance*) + 707
27 clang                    0x00000001050aa241 cc1_main(llvm::ArrayRef<char const*>, char const*, void*) + 2065
28 clang                    0x00000001050a56ad ExecuteCC1Tool(llvm::SmallVectorImpl<char const*>&) + 285
29 clang                    0x00000001074b2227 void llvm::function_ref<void ()>::callback_fn<clang::driver::CC1Command::Execute(llvm::ArrayRef<llvm::Optional<llvm::StringRef>>, std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char>>*, bool*) const::$_4>(long) + 23
30 clang                    0x0000000106adb73c llvm::CrashRecoveryContext::RunSafely(llvm::function_ref<void ()>) + 236
31 clang                    0x00000001074b1c95 clang::driver::CC1Command::Execute(llvm::ArrayRef<llvm::Optional<llvm::StringRef>>, std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char>>*, bool*) const + 293
32 clang                    0x0000000107474af7 clang::driver::Compilation::ExecuteCommand(clang::driver::Command const&, clang::driver::Command const*&, bool) const + 1111
33 clang                    0x0000000107474de0 clang::driver::Compilation::ExecuteJobs(clang::driver::JobList const&, llvm::SmallVectorImpl<std::__1::pair<int, clang::driver::Command const*>>&, bool) const + 144
34 clang                    0x000000010749285f clang::driver::Driver::ExecuteCompilation(clang::driver::Compilation&, llvm::SmallVectorImpl<std::__1::pair<int, clang::driver::Command const*>>&) + 911
35 clang                    0x00000001050a4d1b clang_main(int, char**) + 10315
36 libdyld.dylib            0x00007fff67943cc9 start + 1
37 libdyld.dylib            0x0000000000000026 start + 18446603338778395486

https://green.lab.llvm.org/green/view/LLDB/job/lldb-cmake/44981/testReport/junit/lldb-api/functionalities_data-formatter_data-formatter-stl_libcxx_span/TestDataFormatterLibcxxSpan_py/

Yikes! Thanks for the revert. I didn't see the email from the bot until just about 2 minutes before you reverted. I'll see if I can reproduce.

Yikes! Thanks for the revert. I didn't see the email from the bot until just about 2 minutes before you reverted. I'll see if I can reproduce.

FYI. The same on a different bot https://lab.llvm.org/buildbot/#/builders/5/builds/25486/steps/19/logs/stdio

I tried the ninja stage2-check-all on my local build with this patch applied and don't reproduce the issue. Is there any way someone could get me the preprocessed source that this crashed on off the servers or something? @JDevlieghere or @vitalybuka ?

@shafik was able to help me figure out how to repro, and I think I've about fixed this, so I'll try another commit soon.

@erichkeane can you please make sure that committed revisions in git have "Differential Revision: ....". Usually it's added by "arc" when you upload review.
Also it would be nice if reapply have the same link.

Regarding reproducer, you can follow https://github.com/google/sanitizers/wiki/SanitizerBotReproduceBuild
But it's already related, let see if it resolved for our bot as well.

@erichkeane can you please make sure that committed revisions in git have "Differential Revision: ....". Usually it's added by "arc" when you upload review.
Also it would be nice if reapply have the same link.

Regarding reproducer, you can follow https://github.com/google/sanitizers/wiki/SanitizerBotReproduceBuild
But it's already related, let see if it resolved for our bot as well.

Yep, I'll make sure to remember that next time. Corporate IT prevents me from using arc, so I am stuck doing it m anually and forget sometimes.

Crash that required the revert : https://reviews.llvm.org/rGbefa8cf087dbb8159a4d9dc8fa4d6748d6d5049a

reduces to :

 template<class _Ip>
 concept sentinel_for =
   requires(_Ip __i) {
     __i++;
   };

 template<class _Ip>
 concept bidirectional_iterator =
   sentinel_for<_Ip>;

 
template <class _Iter>
class move_iterator
{
public:
    auto operator++(int)
        requires
        sentinel_for<_Iter>;
};
 
static_assert( bidirectional_iterator<move_iterator<int>>);

In other news, I discovered that 'check-all' and 'stage2-check-all' doesn't include 'check-runtimes', which managed to be why I missed this :/

I won't have time to poke that this until Tuesday since Monday is a US holiday and I'm done for the day, so if @ChuanqiXu or others wish to mess with this, feel free, otherwise I'll get back on it next week!

vitalybuka reopened this revision.Jul 1 2022, 2:59 PM

Not sure if the URL above was not related, as lease I see no red builds after "relanded patch"

However these are 100% relevant, I tested msan one locally 258c3aee54e1 vs 188582b7e0f3
https://lab.llvm.org/buildbot/#/builders/74/builds/11706
https://lab.llvm.org/buildbot/#/builders/85/builds/8938
https://lab.llvm.org/buildbot/#/builders/168/builds/7114

However sanitizers has nothing to do with the problems, you just need compilers builds with asserts, and then use that one to build and run libcxx tests.

This reproduces the issue on 188582b7e0f357bb9b137b995f773d114472a9c4

mkdir b0
cd b0/
# ${HOME}/src/clang/bin/clang++ is some stable good build of clang.
cmake -GNinja ${HOME}/src/llvm.git/llvm-project/llvm '-DLLVM_ENABLE_PROJECTS=clang;lld'   -DLLVM_CCACHE_BUILD=ON -DCMAKE_C_COMPILER=${HOME}/src/clang/bin/clang -DCMAKE_CXX_COMPILER=${HOME}/src/clang/bin/clang++  -DCMAKE_BUILD_TYPE=Release -DLLVM_ENABLE_ASSERTIONS=ON
ninja

mkdir ../b1
cd ../b1
cmake -GNinja ${HOME}/src/llvm.git/llvm-project/llvm '-DLLVM_ENABLE_PROJECTS=libcxx;libcxxabi'  -DCMAKE_C_COMPILER=$(readlink -f ../b0)/bin/clang -DCMAKE_CXX_COMPILER=$(readlink -f ../b0)/bin/clang++  -DCMAKE_BUILD_TYPE=Release -DLLVM_ENABLE_ASSERTIONS=ON
ninja check-cxx
This revision is now accepted and ready to land.Jul 1 2022, 2:59 PM

Welp, fixed THIS problem but lead to another issue in libc++ that I'm still running down. Stand by all :/

SO I've been playing with this for a while and all the libcxx issues. I THINK we really need to just bit the bullet and figure out how to correctly re-add things to the instantiation scope (after making the CheckFunctionConstraints LocalInstantiationScope 'false' for the 2nd param). I can do it well enough with functions since we have a function for it, but likely need to do the same for at least RecordDecl.

SO I've been playing with this for a while and all the libcxx issues. I THINK we really need to just bit the bullet and figure out how to correctly re-add things to the instantiation scope (after making the CheckFunctionConstraints LocalInstantiationScope 'false' for the 2nd param). I can do it well enough with functions since we have a function for it, but likely need to do the same for at least RecordDecl.

URGH.... And its not clear to me that I CAN do this, since a containing function template's body doesn't exist yet, so we cant go back and make those work. So we are at a point where we both have to inherit AND not inherit the paretn scope, and I have no idea on which basis to do so.

This version passes check-runtimes, so libc++ is fine, and it passes everything I have available. @ChuanqiXu : Would you be able to do 1 more run over this to make sure it won't break something? Thanks in advance either way!

This version passes check-runtimes, so libc++ is fine, and it passes everything I have available. @ChuanqiXu : Would you be able to do 1 more run over this to make sure it won't break something? Thanks in advance either way!

I've tested some our internal workloads and all of them looks fine. But it is expected since we don't use concept heavily. I had tried to run libcxx before but it looks like my previous config is wrong...

This version passes check-runtimes, so libc++ is fine, and it passes everything I have available. @ChuanqiXu : Would you be able to do 1 more run over this to make sure it won't break something? Thanks in advance either way!

I've tested some our internal workloads and all of them looks fine. But it is expected since we don't use concept heavily. I had tried to run libcxx before but it looks like my previous config is wrong...

Ok, thanks! I DID test the libcxx test suite on the above, I figured it out through check-runtimes. If you were able to take a quick look at the diff and make sure I'm not doing anything horrible, it would be appreciated.

This version passes check-runtimes, so libc++ is fine, and it passes everything I have available. @ChuanqiXu : Would you be able to do 1 more run over this to make sure it won't break something? Thanks in advance either way!

I've tested some our internal workloads and all of them looks fine. But it is expected since we don't use concept heavily. I had tried to run libcxx before but it looks like my previous config is wrong...

Ok, thanks! I DID test the libcxx test suite on the above, I figured it out through check-runtimes. If you were able to take a quick look at the diff and make sure I'm not doing anything horrible, it would be appreciated.

Yeah, things looks good to me basically.

clang/lib/Sema/SemaTemplateInstantiate.cpp
973–974

I would like to see this in call site and a comment to tell us that we don't want to evaluate constraints in dependent context.

erichkeane added inline comments.Jul 12 2022, 7:55 AM
clang/lib/Sema/SemaTemplateInstantiate.cpp
973–974

So you want this logic moved to the call-sites? Or am I misinterpreting that?

ChuanqiXu added inline comments.Jul 12 2022, 8:01 AM
clang/lib/Sema/SemaTemplateInstantiate.cpp
973–974

Yes. It is odd for me to do logic operations in the initialization list.

erichkeane added inline comments.Jul 12 2022, 8:22 AM
clang/lib/Sema/SemaTemplateInstantiate.cpp
973–974

Got it, that makes sense.

Though when looking at it, I found myself wondering why I did this change, it seems to 'undo' a lot of the deferred concepts work :/

Ah... I remember now, and I think that solution isn't quite right. The idea is that we want to defer constraint evaluation when the constraint is on a function decl/template decl. The PROBLEM I need to solve is when it is in a body of a struct or function.

Hmm... I think my approach was slightly off... I have to spend more time on this, but thanks for the review! I hope I'm learning each time we go again :/

The more I look at this... the more I question my approach. I now am definitely sure we won't make Clang15, and hope that I can figure something better out for 16 :/

The more I look at this... the more I question my approach. I now am definitely sure we won't make Clang15, and hope that I can figure something better out for 16 :/

I feel like it could be helpful to split it into several small patches next time.

The more I look at this... the more I question my approach. I now am definitely sure we won't make Clang15, and hope that I can figure something better out for 16 :/

I feel like it could be helpful to split it into several small patches next time.

I REALLY wish I knew how to make that happen. The problem is the main idea (deferring instantiation) is an 'all or nothing' that really seems to break things. There are a handful of small-ish 'cleanup' tasks that I did along the way, and could probably come up with another, but I'm not sure how much I can remove from the patch to split it up :/

I pulled out a good amount of this patch as NFC, which should shrink the patch.

I have a 'new approach' that I think will work, which is to suppress the constraint
evaluation the same way we did with the requires clause (though, with a flag,
rather than just being able to skip it).

At the moment, I have 1 lit test failure in Clang. Additionally, I have 2-3 libcxx
failures where we seem to static-assert where we didn't before, PLUS 1 test that seems
to hang. I'm hopeful I am on the right track and can continue to make progress.

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.

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?

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...

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.

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.

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!

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.

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:)

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
3671

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
2444

Is this used?

erichkeane marked 8 inline comments as done.Aug 16 2022, 9:57 AM
erichkeane added inline comments.
clang/include/clang/Sema/Sema.h
3671

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
2444

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

erichkeane marked 5 inline comments as done.

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

This revision was landed with ongoing or failed builds.Aug 17 2022, 6:25 AM
This revision was automatically updated to reflect the committed changes.

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.

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...

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.

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.

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.

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!

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.

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.

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!

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?

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.

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.

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.

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 updated this revision to Diff 458727.Sep 8 2022, 7:11 AM
erichkeane added a subscriber: wlei.

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.

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.

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).

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.

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 reopened this revision.Sep 21 2022, 5:55 AM

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

This revision is now accepted and ready to land.Sep 21 2022, 5:55 AM

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 spoke too soon, I found another crash that came out of @wlei s repro from last time.

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.

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

wlei added a comment.Sep 21 2022, 9:11 AM

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.

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.

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!

Tested with our internal workloads and things look fine.

wlei added a comment.Sep 21 2022, 11:01 PM

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

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

Thank you all for the quick responses!

This revision was landed with ongoing or failed builds.Sep 22 2022, 6:30 AM
This revision was automatically updated to reflect the committed changes.

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>);
                       ^

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.

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.

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.

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.

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!

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;
         ~ ^  ~

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 added a comment.EditedOct 12 2022, 8:56 AM

(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...

(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.

@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 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 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.

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 added inline comments.
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 added inline comments.Oct 20 2022, 1:43 PM
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 added inline comments.Oct 21 2022, 6:03 AM
clang/lib/Sema/SemaConcept.cpp
513–534

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

erichkeane added inline comments.Oct 21 2022, 7:14 AM
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 added inline comments.Oct 21 2022, 7:27 AM
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!