Page MenuHomePhabricator

Deferred Concept Instantiation Implementation Take 2
AcceptedPublic

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
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
1944–1948 ↗(On Diff #440712)

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
7130–7141

Remove this before landing.

7131

ditto

clang/lib/AST/Decl.cpp
3788 ↗(On Diff #440712)

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
64
68
183–184
clang/lib/Sema/SemaTemplate.cpp
2343–2344
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
1944–1948 ↗(On Diff #440712)

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
7130–7141

Yikes! Thanks for catching this!

clang/lib/AST/Decl.cpp
3788 ↗(On Diff #440712)

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
1944–1948 ↗(On Diff #440712)

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
1944–1948 ↗(On Diff #440712)

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
977–978

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
977–978

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
977–978

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
977–978

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
3642

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

clang/include/clang/Sema/Template.h
507–518

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

clang/lib/Sema/SemaTemplateDeduction.cpp
2786–2799

nit:

clang/lib/Sema/SemaTemplateInstantiate.cpp
1233

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);
   ...
}
3609–3610
clang/lib/Sema/SemaTemplateInstantiateDecl.cpp
2435

Is this used?