[clang] NRVO: Improvements and handling of more cases.

[mizvekov]

Summary

This expands NRVO propagation for more cases:

Parse analysis improvement:

• Lambdas and Blocks with dependent return type can have their variables marked as NRVO Candidates.

Variable instantiation improvements:

• Fixes crash when instantiating NRVO variables in Blocks.
• Functions, Lambdas, and Blocks which have auto return type have their variables' NRVO status propagated. For Blocks with non-auto return type, as a limitation, this propagation does not consider the actual return type.

This also implements exclusion of VarDecls which are references to
dependent types.

Signed-off-by: Matheus Izvekov <[e-mail]>

[mizvekov]

clang/lib/Sema/SemaStmt.cpp

3429–3432

Yeah, what you suggested is what I tried on a previous patch in this DR, but then studying the NRVO tracker carefully I thought about this counter example:

template<bool B> static auto bar() {
  {                                 
    Foo foo;                        
    if constexpr(B)                 
      return foo;                  
  }                                 
  {                                 
    Bar bar;                        
    if constexpr(!B)                
      return bar;                   
  }                                 
}                                   
`

Since we run the tracker before instantiation, we would see both return statements and mark both foo and bar as NRVO variables.
Ofcourse in the B = false case, we would end up constructing a Foo in a Bar return slot....

As a side note, It is actually funny that we currently perform this optimization (most likely accidentally):

template<bool B> static Foo bar() {
  {                                 
    Foo foo1;                        
    if constexpr(B)                 
      return foo1;                  
  }                                 
  {
    Foo foo2;
    return foo2                                 
  }                                 
}

In the B = false case, we end up constructing foo1 in the return slot even though we actually never return it.

[mizvekov]

clang/lib/Sema/SemaStmt.cpp

3429–3432

Compiler explorer link for the accidental optimization I am talking about: https://godbolt.org/z/czdWodW87

[rsmith]

clang/lib/Sema/SemaStmt.cpp

3429–3432

That's amazing, and makes total sense. Yeah, if constexpr pretty thoroughly destroys the assumptions we make in the Scope-based NRVO computation about what a templated function will look like after instantiation.

[mizvekov]

• Address rsmith review comments.

[mizvekov]

clang/include/clang/Sema/Sema.h

4757–4764

I like NamedReturnInfo, will change.

clang/lib/Sema/SemaStmt.cpp

3337–3346

How about demoteNRVOStatus?

3435

Using short utility lambda as an alternative to keep the main logic clear from clutter.

3576

This goes to the documentation on getNamedReturnInfo, return value, where it says that the Canidate member will be non-null in case implicit move would be permitted under the most permissive language standard.

So if Try

So if we are guarded under that condition, we would never

3576

Skipping it is just a small compilation performance optimization:

We are going to try that TryMoveInitialization again just below, this time with equivalent of ForceCXX20 = true. If we were already in c++20 mode, then that second call is going to be made with basically the same parameters and perform basically the same work and reach the same conclusion: Faliure, and the diagnostic won't be produced.

Now my take on why that looks confusing: To me TryMoveInitialization has a similar problem to the one I addressed in this patch for getCopyElisionCandidate, it relies on you calling it multiple times, once to try the the actual work, and if that fails, then you have to call it again, this time in 'permissive' more, to get the information you need for the diagnostics.

Now I did not try giving it the same treatment of returning an aggregate with all the information you need, so you can call it just once with one less parameter.
If you agree on that take, I'd be happy to try a similar improvement there as well.
If you don't think it's worth the noise, then it's all good, no worries :)

As a side note, I just noticed that the
if (QT->isLValueReferenceType()) { below is dead code, we are never going to consider this move eligible in any mode, and so Res.Candidate is going to be null.

We should not need to repeat here the checks we already do in getNRVOResult, so I am going to go ahead and remove it.

I think it may have been added because until just recently getCopyElisionCandidate was bugged and accepted LValue references, and I guess the problem was "fixed" in this diagnostic first.

clang/lib/Sema/SemaTemplateInstantiateDecl.cpp

1094

Thanks a lot! That works and now we get copy elision on dependent blocks :)

[mizvekov]

clang/lib/Sema/SemaStmt.cpp

3576

Disregard this comment, I realized I was answering the wrong question and for some reason it was not discarded.

[• Quuxplusone]

clang/lib/Sema/SemaStmt.cpp

3337–3346

I'm with Richard here: if you mean "disallow copy elision," say so!
IMO this should be member functions of NamedReturnInfo, and it should just be

void disallowCopyElision() {
    S &= ~CopyElidable;  // obviously this "should" work;
}                    // ensure that your enum values actually make it work

void disallowImplicitMove() {
    S = None;
}

The way you use it below is equivalent to e.g.

if (VD->isExceptionVariable()) {
  Info.disallowCopyElision();
  if (!hasCXX20)
    Info.disallowImplicitMove();
}

which I think is a lot easier to puzzle out what's going on and how it corresponds to the standardese.

3362–3363

Nit: please declare variables one-per-line. Change that , to a ; and write bool again on line 3085 instead of four space characters.

[mizvekov]

clang/lib/Sema/SemaStmt.cpp

3337–3346

@Quuxplusone But I did not mean "disallow copy elision", the function will also disallow implicit move based on a parameter, so would be a bit misleading.

That solution you proposed though is more repetitive and prone to error: it does not enforce the invariant that you should always disallow copy elision when disallowing implicit move.
Even if you amend that to having one manipulator function which enforces the invariant, you still have two bools totaling four states, where only three states make sense.

I would consider a better name though, not necessarily happy with this one either.

[mizvekov]

clang/lib/Sema/SemaStmt.cpp

3337–3346

Also I did consider making this a method, but this function is more of an implementation detail that is better to hide anyway.

[mizvekov]

@rsmith:

Actually, there is still something not quite right with this patch that I have to finish investigating.

The B5 test should not have gotten copy elision. I thought at first that we had gotten the function return type deduced during VarDecl instantiation, but we actually have a dependant type instead:

dump of FT:

FunctionProtoType 0x18597a3ed10 '<dependent type> (void)' dependent cdecl
`-BuiltinType 0x185960144e0 '<dependent type>' dependent

I think I assumed wrong that these blocks would behave similarly to lambdas with auto return.

[mizvekov]

• Changed the downgrade* function name to disallowNRVO, given second thought, I think it's appropriate name.
• Change to one variable declaration per statement as per Arthur's review.

[aaronpuchert]

clang/lib/Sema/SemaStmt.cpp

3337–3346

You can have more aptly named functions without dropping the invariant. I'd say you only need disallowCopyElision though, instead of disallowImplicitMove you could just directly overwrite with None.

3362–3363

I don't think the coding standards have a rule about this. I tend to advocate the opposite in most cases to avoid repetition, but for bool it's probably fine to repeat.

3422–3423

So the return value is trivially deducible from the post-state of Info (as Info.isCopyElidable() ? Info.Candidate : nullptr)? I guess it makes some code shorter to write, but generally it's preferable to have fewer implicit invariants.

Also it's a getter function that modifies its arguments... not saying there isn't precedent in the code, but I'd say less surprising function signatures are still better where we can have them.

[mizvekov]

clang/lib/Sema/SemaStmt.cpp

3337–3346

But the dominant use case here is wanting to choose either one based on the language mode. So the signature taking a bool is more convenient.

[• Quuxplusone]

clang/lib/Sema/SemaStmt.cpp

3337–3346

Just to be clear, I don't see the use-case anywhere as wanting to "choose" anything based on a parameter. As my code indicated, I see the logic here as being simply "For exception variables [or whatever], disallow copy elision. _Additionally_, pre-'20, disallow even implicit move for them." And so on. No functions of boolean parameters, just straightforward English if-thens, based on the English in the standard.
I get that you're probably not going to write the straightforward code in the end; but I just wanted to push a tiny bit harder to see if anyone else sees what I see. :)

[aaronpuchert]

clang/lib/Sema/SemaStmt.cpp

3337–3346

Your argument makes sense to me, but the function is called 5 times with a runtime parameter, so it's going to be quite a bit more code.

just straightforward English if-thens, based on the English in the standard.

Technically aren't we handling different standards at once here? So there is actually no language in any standard that contains these particular if-thens.

[mizvekov]

• Added doc to disallowNRVO
• Also detect implicit return type for blocks.

[mizvekov]

Ping.

[• Quuxplusone]

@mizvekov, my understanding is:

• D99696 was temporarily blocked on D103720, but now D103720 is landed and D99696 is unblocked
• D99696 is a codegen improvement in all language modes, and is also blocking D99005
• D99005 is blocking our getting experience with p2266 for C++23

IMHO we should just plow ahead here. @aaronpuchert what's your take on D99696 at this point; is it good to go or still unresolved issues?

[mizvekov]

In D99696#2808592, @Quuxplusone wrote:

@mizvekov, my understanding is:

• D99696 was temporarily blocked on D103720, but now D103720 is landed and D99696 is unblocked
• D99696 is a codegen improvement in all language modes, and is also blocking D99005
• D99005 is blocking our getting experience with p2266 for C++23

IMHO we should just plow ahead here. @aaronpuchert what's your take on D99696 at this point; is it good to go or still unresolved issues?

No, actually, D99696 (This DR) was not blocked on any other DRs, I just put it as parent here for my convenience, since I had them all stacked on my workstation to make my life easier.
Sorry if that caused confusion and anyone thought it was blocked....

D99005 (The P2266 implementation) is blocked just on the present DR, but that was because there are common changes and I decided to do them in this order since it was not clear when P2266 impl could be merged.

D103720 was a dependency for D100733 proposed by @rsmith, which is a dependency of D100000 (This is the one that implements the 'almost xvalue' thing that Jason Merril was pushing on the reflector, in order to replace the two-phase overload resolution).

[aaronpuchert]

In D99696#2808592, @Quuxplusone wrote:

@aaronpuchert what's your take on D99696 at this point; is it good to go or still unresolved issues?

Overall this looks sensible to me, and I don't really have anything to add. Parts of the code seem a bit complex, but that just seems to reflect the complexity of the standard(s).

[stella.stamenova]

It looks like this caused a couple of failures on the Windows LLDB bot due to crashes in clang:

https://lab.llvm.org/buildbot/#/builders/83/builds/7142

[aeubanks]

Crashes on a stage 2 build on Windows:

../rel/bin/clang-cl /nologo /showIncludes /Foobj/llvm/lib/MC/MCParser/MCParser.MasmParser.obj /c ../../llvm/lib/MC/MCParser/MasmParser.cpp -D_CRT_SECURE_NO_DEPRECATE -D_CRT_SECURE_NO_WARNINGS -D_CRT_NONSTDC_NO_DEPRECATE -D_CRT_NONSTDC_NO_WARNINGS -D_SCL_SECURE_NO_DEPRECATE -D_SCL_SECURE_NO_WARNINGS -D_HAS_EXCEPTIONS=0 -D_UNICODE -DUNICODE -I../../llvm/include -Igen/llvm/include /Zi /FS -gline-tables-only -gcodeview-ghash /O2 /Gw /Zc:inline /EHs-c- /W4 -Wno-unused-parameter -Wdelete-non-virtual-dtor -Wstring-conversion -no-canonical-prefixes -Werror=date-time -fmsc-version=1916 /Brepro /winsysroot../../../sysroot -Wcovered-switch-default /GR-
Assertion failed: isa<X>(Val) && "cast<Ty>() argument of incompatible type!", file ../../llvm/include/llvm/Support/Casting.h, line 262
PLEASE submit a bug report to https://bugs.llvm.org/ and include the crash backtrace, preprocessed source, and associated run script.
Stack dump:
0.      Program arguments: ../rel/bin/clang-cl /nologo /showIncludes /Foobj/llvm/lib/MC/MCParser/MCParser.MasmParser.obj /c ../../llvm/lib/MC/MCParser/MasmParser.cpp -D_CRT_SECURE_NO_DEPRECATE -D_CRT_SECURE_NO_WARNINGS -D_CRT_NONSTDC_NO_DEPRECATE -D_CRT_NONSTDC_NO_WARNINGS -D_SCL_SECURE_NO_DEPRECATE -D_SCL_SECURE_NO_WARNINGS -D_HAS_EXCEPTIONS=0 -D_UNICODE -DUNICODE -I../../llvm/include -Igen/llvm/include /Zi /FS -gline-tables-only -gcodeview-ghash /O2 /Gw /Zc:inline /EHs-c- /W4 -Wno-unused-parameter -Wdelete-non-virtual-dtor -Wstring-conversion -no-canonical-prefixes -Werror=date-time -fmsc-version=1916 /Brepro /winsysroot../../../sysroot -Wcovered-switch-default /GR-
1.      <eof> parser at end of file
2.      ../../../sysroot\VC\Tools\MSVC\14.16.27023\include\ostream:320:36: instantiating function definition 'std::basic_ostream<char>::operator<<'
3.      ../../../sysroot\VC\Tools\MSVC\14.16.27023\include\xlocale:503:26: instantiating function definition 'std::use_facet<std::num_put<char>>'
4.      ../../../sysroot\VC\Tools\MSVC\14.16.27023\include\xlocnum:1417:35: instantiating function definition 'std::num_put<char>::_Getcat'
5.      ../../../sysroot\VC\Tools\MSVC\14.16.27023\include\xlocnum:1446:21: instantiating function definition 'std::num_put<char>::num_put'
6.      ../../../sysroot\VC\Tools\MSVC\14.16.27023\include\xlocnum:1504:36: instantiating function definition 'std::num_put<char>::do_put'
7.      ../../../sysroot\VC\Tools\MSVC\14.16.27023\include\xlocale:503:26: instantiating function definition 'std::use_facet<std::numpunct<char>>'
8.      ../../../sysroot\VC\Tools\MSVC\14.16.27023\include\xlocnum:163:16: instantiating function definition 'std::numpunct<char>::_Getcat'
9.      ../../../sysroot\VC\Tools\MSVC\14.16.27023\include\xlocnum:157:2: instantiating function definition 'std::numpunct<char>::numpunct'
10.     ../../../sysroot\VC\Tools\MSVC\14.16.27023\include\xlocnum:199:7: instantiating function definition 'std::numpunct<char>::_Init'
11.     ../../../sysroot\VC\Tools\MSVC\14.16.27023\include\xlocale:662:19: instantiating function definition 'std::_Maklocstr<char>'
 #0 0x00007ff7a6b48596 HandleAbort /b/f/w/llvm-project/build/rel/../../llvm/lib/Support/Windows/Signals.inc:408:0
 #1 0x00007ff7a9cee690 raise C:\src\llvm-project\build\rel\minkernel\crts\ucrt\src\appcrt\misc\signal.cpp:547:0
 #2 0x00007ff7a9ce3fd0 abort C:\src\llvm-project\build\rel\minkernel\crts\ucrt\src\appcrt\startup\abort.cpp:71:0
 #3 0x00007ff7a9ce4672 common_assert_to_stderr<wchar_t> C:\src\llvm-project\build\rel\minkernel\crts\ucrt\src\appcrt\startup\assert.cpp:186:0
 #4 0x00007ff7a9ce451a _wassert C:\src\llvm-project\build\rel\minkernel\crts\ucrt\src\appcrt\startup\assert.cpp:443:0
 #5 0x00007ff7a8899933 llvm::PointerUnion<const clang::Type *,const clang::ExtQuals *>::isNull /b/f/w/llvm-project/build/rel/../../llvm/include/llvm/ADT/PointerUnion.h:172:0
 #6 0x00007ff7a8899933 clang::QualType::isNull /b/f/w/llvm-project/build/rel/../../clang/include/clang/AST/Type.h:734:0
 #7 0x00007ff7a8899933 clang::QualType::getCommonPtr /b/f/w/llvm-project/build/rel/../../clang/include/clang/AST/Type.h:684:0
 #8 0x00007ff7a8899933 clang::QualType::getTypePtr /b/f/w/llvm-project/build/rel/../../clang/include/clang/AST/Type.h:6428:0
 #9 0x00007ff7a8899933 llvm::simplify_type<clang::QualType>::getSimplifiedValue /b/f/w/llvm-project/build/rel/../../clang/include/clang/AST/Type.h:1318:0
#10 0x00007ff7a8899933 llvm::simplify_type<const clang::QualType>::getSimplifiedValue /b/f/w/llvm-project/build/rel/../../llvm/include/llvm/Support/Casting.h:48:0
#11 0x00007ff7a8899933 llvm::isa_impl_wrap<clang::FunctionType,const clang::QualType,const clang::Type *>::doit /b/f/w/llvm-project/build/rel/../../llvm/include/llvm/Support/Casting.h:121:0
#12 0x00007ff7a8899933 llvm::isa /b/f/w/llvm-project/build/rel/../../llvm/include/llvm/Support/Casting.h:142:0
#13 0x00007ff7a8899933 llvm::cast /b/f/w/llvm-project/build/rel/../../llvm/include/llvm/Support/Casting.h:263:0
#14 0x00007ff7a8899933 clang::TemplateDeclInstantiator::VisitVarDecl(class clang::VarDecl *, bool, class llvm::ArrayRef<class clang::BindingDecl *> *) /b/f/w/llvm-project/build/rel/../../clang/lib/Sema/SemaTemplateInstantiateDecl.cpp:1110:0
#15 0x00007ff7a88b10f8 clang::Sema::SubstDecl::<lambda_0>::operator() /b/f/w/llvm-project/build/rel/../../clang/lib/Sema/SemaTemplateInstantiateDecl.cpp:3837:0
#16 0x00007ff7a88b10f8 llvm::function_ref<void ()>::callback_fn<`lambda at ../../clang/lib/Sema/SemaTemplateInstantiateDecl.cpp:3836:49'> /b/f/w/llvm-project/build/rel/../../llvm/include/llvm/ADT/STLExtras.h:177:0
#17 0x00007ff7a72917fd clang::Sema::runWithSufficientStackSpace(class clang::SourceLocation, class llvm::function_ref<(void)>) /b/f/w/llvm-project/build/rel/../../clang/lib/Sema/Sema.cpp:463:0
#18 0x00007ff7a88a9dce clang::Sema::SubstDecl(class clang::Decl *, class clang::DeclContext *, class clang::MultiLevelTemplateArgumentList const &) /b/f/w/llvm-project/build/rel/../../clang/lib/Sema/SemaTemplateInstantiateDecl.cpp:3839:0
#19 0x00007ff7a8a11c2d `anonymous namespace'::TemplateInstantiator::TransformDefinition /b/f/w/llvm-project/build/rel/../../clang/lib/Sema/SemaTemplateInstantiate.cpp:1253:0
#20 0x00007ff7a8a11c2d clang::TreeTransform<(anonymous namespace)::TemplateInstantiator>::TransformDeclStmt /b/f/w/llvm-project/build/rel/../../clang/lib/Sema/TreeTransform.h:7603:0
#21 0x00007ff7a89e3a61 clang::TreeTransform<(anonymous namespace)::TemplateInstantiator>::TransformStmt /b/f/w/llvm-project/build/rel/gen/clang/include/clang/AST/StmtNodes.inc:97:0
#22 0x00007ff7a8a05ab8 clang::ActionResult<clang::Stmt *,1>::isInvalid /b/f/w/llvm-project/build/rel/../../clang/include/clang/Sema/Ownership.h:207:0
#23 0x00007ff7a8a05ab8 clang::TreeTransform<(anonymous namespace)::TemplateInstantiator>::TransformCompoundStmt /b/f/w/llvm-project/build/rel/../../clang/lib/Sema/TreeTransform.h:7209:0
#24 0x00007ff7a8a11450 clang::TreeTransform<(anonymous namespace)::TemplateInstantiator>::TransformCompoundStmt /b/f/w/llvm-project/build/rel/../../clang/lib/Sema/TreeTransform.h:7192:0
#25 0x00007ff7a89e39a0 clang::TreeTransform<(anonymous namespace)::TemplateInstantiator>::TransformStmt /b/f/w/llvm-project/build/rel/gen/clang/include/clang/AST/StmtNodes.inc:73:0
#26 0x00007ff7a89e36a4 llvm::DenseMap<clang::Decl *,clang::Decl *,llvm::DenseMapInfo<clang::Decl *>,llvm::detail::DenseMapPair<clang::Decl *,clang::Decl *> >::~DenseMap /b/f/w/llvm-project/build/rel/../../llvm/include/llvm/ADT/DenseMap.h:755:0
#27 0x00007ff7a89e36a4 clang::TreeTransform<(anonymous namespace)::TemplateInstantiator>::~TreeTransform /b/f/w/llvm-project/build/rel/../../clang/lib/Sema/TreeTransform.h:101:0
#28 0x00007ff7a89e36a4 clang::Sema::SubstStmt(class clang::Stmt *, class clang::MultiLevelTemplateArgumentList const &) /b/f/w/llvm-project/build/rel/../../clang/lib/Sema/SemaTemplateInstantiate.cpp:3463:0
#29 0x00007ff7a88ac61a clang::Sema::InstantiateFunctionDefinition(class clang::SourceLocation, class clang::FunctionDecl *, bool, bool, bool) /b/f/w/llvm-project/build/rel/../../clang/lib/Sema/SemaTemplateInstantiateDecl.cpp:4961:0
#30 0x00007ff7a88af1ac clang::FunctionDecl::isDefined /b/f/w/llvm-project/build/rel/../../clang/include/clang/AST/Decl.h:2084:0
#31 0x00007ff7a88af1ac clang::Sema::PerformPendingInstantiations(bool) /b/f/w/llvm-project/build/rel/../../clang/lib/Sema/SemaTemplateInstantiateDecl.cpp:6207:0
#32 0x00007ff7a88ac6fa clang::Sema::InstantiateFunctionDefinition(class clang::SourceLocation, class clang::FunctionDecl *, bool, bool, bool) /b/f/w/llvm-project/build/rel/../../clang/lib/Sema/SemaTemplateInstantiateDecl.cpp:4986:0
#33 0x00007ff7a88af1ac clang::FunctionDecl::isDefined /b/f/w/llvm-project/build/rel/../../clang/include/clang/AST/Decl.h:2084:0
#34 0x00007ff7a88af1ac clang::Sema::PerformPendingInstantiations(bool) /b/f/w/llvm-project/build/rel/../../clang/lib/Sema/SemaTemplateInstantiateDecl.cpp:6207:0
#35 0x00007ff7a88ac6fa clang::Sema::InstantiateFunctionDefinition(class clang::SourceLocation, class clang::FunctionDecl *, bool, bool, bool) /b/f/w/llvm-project/build/rel/../../clang/lib/Sema/SemaTemplateInstantiateDecl.cpp:4986:0
#36 0x00007ff7a88af1ac clang::FunctionDecl::isDefined /b/f/w/llvm-project/build/rel/../../clang/include/clang/AST/Decl.h:2084:0
#37 0x00007ff7a88af1ac clang::Sema::PerformPendingInstantiations(bool) /b/f/w/llvm-project/build/rel/../../clang/lib/Sema/SemaTemplateInstantiateDecl.cpp:6207:0
#38 0x00007ff7a88ac6fa clang::Sema::InstantiateFunctionDefinition(class clang::SourceLocation, class clang::FunctionDecl *, bool, bool, bool) /b/f/w/llvm-project/build/rel/../../clang/lib/Sema/SemaTemplateInstantiateDecl.cpp:4986:0
#39 0x00007ff7a88af1ac clang::FunctionDecl::isDefined /b/f/w/llvm-project/build/rel/../../clang/include/clang/AST/Decl.h:2084:0
#40 0x00007ff7a88af1ac clang::Sema::PerformPendingInstantiations(bool) /b/f/w/llvm-project/build/rel/../../clang/lib/Sema/SemaTemplateInstantiateDecl.cpp:6207:0
#41 0x00007ff7a88ac6fa clang::Sema::InstantiateFunctionDefinition(class clang::SourceLocation, class clang::FunctionDecl *, bool, bool, bool) /b/f/w/llvm-project/build/rel/../../clang/lib/Sema/SemaTemplateInstantiateDecl.cpp:4986:0
#42 0x00007ff7a88af1ac clang::FunctionDecl::isDefined /b/f/w/llvm-project/build/rel/../../clang/include/clang/AST/Decl.h:2084:0
#43 0x00007ff7a88af1ac clang::Sema::PerformPendingInstantiations(bool) /b/f/w/llvm-project/build/rel/../../clang/lib/Sema/SemaTemplateInstantiateDecl.cpp:6207:0
#44 0x00007ff7a88ac6fa clang::Sema::InstantiateFunctionDefinition(class clang::SourceLocation, class clang::FunctionDecl *, bool, bool, bool) /b/f/w/llvm-project/build/rel/../../clang/lib/Sema/SemaTemplateInstantiateDecl.cpp:4986:0
#45 0x00007ff7a88af1ac clang::FunctionDecl::isDefined /b/f/w/llvm-project/build/rel/../../clang/include/clang/AST/Decl.h:2084:0
#46 0x00007ff7a88af1ac clang::Sema::PerformPendingInstantiations(bool) /b/f/w/llvm-project/build/rel/../../clang/lib/Sema/SemaTemplateInstantiateDecl.cpp:6207:0
#47 0x00007ff7a88ac6fa clang::Sema::InstantiateFunctionDefinition(class clang::SourceLocation, class clang::FunctionDecl *, bool, bool, bool) /b/f/w/llvm-project/build/rel/../../clang/lib/Sema/SemaTemplateInstantiateDecl.cpp:4986:0
#48 0x00007ff7a88af1ac clang::FunctionDecl::isDefined /b/f/w/llvm-project/build/rel/../../clang/include/clang/AST/Decl.h:2084:0
#49 0x00007ff7a88af1ac clang::Sema::PerformPendingInstantiations(bool) /b/f/w/llvm-project/build/rel/../../clang/lib/Sema/SemaTemplateInstantiateDecl.cpp:6207:0
#50 0x00007ff7a88ac6fa clang::Sema::InstantiateFunctionDefinition(class clang::SourceLocation, class clang::FunctionDecl *, bool, bool, bool) /b/f/w/llvm-project/build/rel/../../clang/lib/Sema/SemaTemplateInstantiateDecl.cpp:4986:0
#51 0x00007ff7a88af1ac clang::FunctionDecl::isDefined /b/f/w/llvm-project/build/rel/../../clang/include/clang/AST/Decl.h:2084:0
#52 0x00007ff7a88af1ac clang::Sema::PerformPendingInstantiations(bool) /b/f/w/llvm-project/build/rel/../../clang/lib/Sema/SemaTemplateInstantiateDecl.cpp:6207:0
#53 0x00007ff7a88ac6fa clang::Sema::InstantiateFunctionDefinition(class clang::SourceLocation, class clang::FunctionDecl *, bool, bool, bool) /b/f/w/llvm-project/build/rel/../../clang/lib/Sema/SemaTemplateInstantiateDecl.cpp:4986:0
#54 0x00007ff7a88af1ac clang::FunctionDecl::isDefined /b/f/w/llvm-project/build/rel/../../clang/include/clang/AST/Decl.h:2084:0
#55 0x00007ff7a88af1ac clang::Sema::PerformPendingInstantiations(bool) /b/f/w/llvm-project/build/rel/../../clang/lib/Sema/SemaTemplateInstantiateDecl.cpp:6207:0
#56 0x00007ff7a88ac6fa clang::Sema::InstantiateFunctionDefinition(class clang::SourceLocation, class clang::FunctionDecl *, bool, bool, bool) /b/f/w/llvm-project/build/rel/../../clang/lib/Sema/SemaTemplateInstantiateDecl.cpp:4986:0
#57 0x00007ff7a88af1ac clang::FunctionDecl::isDefined /b/f/w/llvm-project/build/rel/../../clang/include/clang/AST/Decl.h:2084:0
#58 0x00007ff7a88af1ac clang::Sema::PerformPendingInstantiations(bool) /b/f/w/llvm-project/build/rel/../../clang/lib/Sema/SemaTemplateInstantiateDecl.cpp:6207:0
#59 0x00007ff7a7293711 llvm::TimeTraceScope::~TimeTraceScope /b/f/w/llvm-project/build/rel/../../llvm/include/llvm/Support/TimeProfiler.h:86:0
#60 0x00007ff7a7293711 clang::Sema::ActOnEndOfTranslationUnitFragment(enum clang::Sema::TUFragmentKind) /b/f/w/llvm-project/build/rel/../../clang/lib/Sema/Sema.cpp:1001:0
#61 0x00007ff7a72943ac clang::Sema::ActOnEndOfTranslationUnit(void) /b/f/w/llvm-project/build/rel/../../clang/lib/Sema/Sema.cpp:1042:0
#62 0x00007ff7a98dc220 clang::Parser::ParseTopLevelDecl(class clang::OpaquePtr<class clang::DeclGroupRef> &, bool) C:\src\llvm-project\clang\lib\Parse\Parser.cpp:693:0
#63 0x00007ff7a8bfe1ae clang::ParseAST(class clang::Sema &, bool, bool) C:\src\llvm-project\clang\lib\Parse\ParseAST.cpp:157:0
#64 0x00007ff7a7378a64 clang::FrontendAction::Execute(void) C:\src\llvm-project\clang\lib\Frontend\FrontendAction.cpp:955:0
#65 0x00007ff7a6830593 llvm::Error::getPtr C:\src\llvm-project\llvm\include\llvm\Support\Error.h:273:0
#66 0x00007ff7a6830593 llvm::Error::operator bool C:\src\llvm-project\llvm\include\llvm\Support\Error.h:236:0
#67 0x00007ff7a6830593 clang::CompilerInstance::ExecuteAction(class clang::FrontendAction &) C:\src\llvm-project\clang\lib\Frontend\CompilerInstance.cpp:977:0
#68 0x00007ff7a68c5ba3 clang::ExecuteCompilerInvocation(class clang::CompilerInstance *) C:\src\llvm-project\clang\lib\FrontendTool\ExecuteCompilerInvocation.cpp:278:0
#69 0x00007ff7a67b1a81 cc1_main(class llvm::ArrayRef<char const *>, char const *, void *) /b/f/w/llvm-project/build/rel/../../clang/tools/driver/cc1_main.cpp:246:0
#70 0x00007ff7a67c4df4 ExecuteCC1Tool /b/f/w/llvm-project/build/rel/../../clang/tools/driver/driver.cpp:338:0
#71 0x00007ff7a71584e6 clang::driver::CC1Command::Execute::<lambda_1>::operator() C:\src\llvm-project\clang\lib\Driver\Job.cpp:404:0
#72 0x00007ff7a71584e6 llvm::function_ref<void ()>::callback_fn<`lambda at ../../clang/lib/Driver/Job.cpp:404:22'> C:\src\llvm-project\llvm\include\llvm\ADT\STLExtras.h:177:0
#73 0x00007ff7a6b08cf2 llvm::CrashRecoveryContext::RunSafely(class llvm::function_ref<(void)>) /b/f/w/llvm-project/build/rel/../../llvm/lib/Support/CrashRecoveryContext.cpp:233:0
#74 0x00007ff7a7158171 clang::driver::CC1Command::Execute(class llvm::ArrayRef<class llvm::Optional<class llvm::StringRef>>, class std::basic_string<char, struct std::char_traits<char>, class std::allocator<char>> *, bool *) const C:\src\llvm-project\clang\lib\Driver\Job.cpp:404:0
#75 0x00007ff7a67f2345 clang::driver::Compilation::ExecuteCommand(class clang::driver::Command const &, class clang::driver::Command const *&) const /b/f/w/llvm-project/build/rel/../../clang/lib/Driver/Compilation.cpp:196:0
#76 0x00007ff7a67f282b clang::driver::Compilation::ExecuteJobs(class clang::driver::JobList const &, class llvm::SmallVectorImpl<struct std::pair<int, class clang::driver::Command const *>> &) const /b/f/w/llvm-project/build/rel/../../clang/lib/Driver/Compilation.cpp:249:0
#77 0x00007ff7a6809107 llvm::SmallVectorBase<unsigned int>::empty C:\src\llvm-project\llvm\include\llvm\ADT\SmallVector.h:72:0
#78 0x00007ff7a6809107 clang::driver::Driver::ExecuteCompilation(class clang::driver::Compilation &, class llvm::SmallVectorImpl<struct std::pair<int, class clang::driver::Command const *>> &) C:\src\llvm-project\clang\lib\Driver\Driver.cpp:1571:0
#79 0x00007ff7a67c468e main /b/f/w/llvm-project/build/rel/../../clang/tools/driver/driver.cpp:510:0
#80 0x00007ff7a9cd11e8 invoke_main d:\agent\_work\2\s\src\vctools\crt\vcstartup\src\startup\exe_common.inl:78:0
#81 0x00007ff7a9cd11e8 __scrt_common_main_seh d:\agent\_work\2\s\src\vctools\crt\vcstartup\src\startup\exe_common.inl:288:0
#82 0x00007fff3e777c24 (C:\Windows\System32\KERNEL32.DLL+0x17c24)
#83 0x00007fff3ecad721 (C:\Windows\SYSTEM32\ntdll.dll+0x6d721)
clang-cl: error: clang frontend command failed due to signal (use -v to see invocation)
clang version 13.0.0
Target: x86_64-pc-windows-msvc
Thread model: posix
InstalledDir: ../rel/bin
clang-cl: note: diagnostic msg:
********************

PLEASE ATTACH THE FOLLOWING FILES TO THE BUG REPORT:
Preprocessed source(s) and associated run script(s) are located at:
clang-cl: note: diagnostic msg: C:\src\tmp\MasmParser-573e32.sh
clang-cl: note: diagnostic msg:

********************
ninja: build stopped: subcommand failed.

MasmParser-44ec46.cpp7 MBDownload

MasmParser-44ec46.sh4 KBDownload

[mizvekov]

Thank you @stella.stamenova and @aeubanks for reporting and reverting this.

Turns out was a silly mistake where we were not digging down through AttributedType nodes in order to get the function type.
My bad and will try landing one more time once I do a stage2 build locally.

[mizvekov]

Look through AttributedType when obtaining FunctionDecl return type.

Adds a couple more test cases to cover this.

[hans]

We're seeing new build errors in Chromium after this (http://crbug.com/1219457). Here's a reduced example:

$ cat /tmp/a.mm
#include <utility>

struct Callback {
  // Move-only!
  Callback(const Callback&) = delete;
  Callback& operator=(const Callback&) = delete;
  Callback(Callback&&) = default;
  Callback& operator=(Callback&&) = default;

  void operator()() {}
};

void f(Callback c) {
  __block auto x = std::move(c);

  (void)^(void) {
    x();
  };
}

$ build.release/bin/clang++ -c /tmp/a.mm -target x86_64-apple-darwin10 -stdlib=libc++
/tmp/a.mm:14:16: error: call to deleted constructor of 'Callback'
  __block auto x = std::move(c);
               ^
/tmp/a.mm:5:3: note: 'Callback' has been explicitly marked deleted here
  Callback(const Callback&) = delete;
  ^
1 error generated.

I don't know enough about blocks to know whether the new error makes sense or not.

+rjmccall who knows about blocks.

[• Quuxplusone]

@hans: FYI, that looks related to the immediately following D99005, not D99696 specifically. I suspect that reverting D99005 (but leaving D99696 in trunk) would suffice to fix that symptom. But I agree I don't see why it should be happening. That block code looks reasonable to me at first glance and wasn't intended to be affected by D99005 AFAIK.
@mizvekov: See why I'm skeptical about merging big patches back-to-back and/or on Fridays? ;)

[hans]

@hans: FYI, that looks related to the immediately following D99005, not D99696 specifically.

No, my test case passes at 0d9e8f5f4b68252c6caa1ef81a30777b2f5d7242 but fails at 1e50c3d785f4563873ab1ce86559f2a1285b5678 (D99696), so this does seem to be the one that introduced the problem.

[hans]

Since it seems more discussion is needed here, I've reverted in c60dd3b2626a4d9eefd9f82f9a406b0d28d3fd72. Since they were hard to tease apart, the revert is for both D99696 and D99005.

[rjmccall]

When __block variables get moved to the heap, they're supposed to be moved if possible, not copied. It looks like PerformMoveOrCopyInitialization requires NRVO info to be passed in to ever do a move. Maybe it's being passed in wrong when building __block copy expressions in some situation.

Was there really not a test case covering those semantics?

[mizvekov]

In D99696#2817126, @rjmccall wrote:

When __block variables get moved to the heap, they're supposed to be moved if possible, not copied. It looks like PerformMoveOrCopyInitialization requires NRVO info to be passed in to ever do a move. Maybe it's being passed in wrong when building __block copy expressions in some situation.

Found the culprit, that functionality was implemented in checkEscapingByref, and it was a bit of a shocker that such a thing was just hiding in a function with such an unassuming name :)
Also surprising to find here another user of the two-step overload resolution, that also escaped our radar.
There is a C++ proposal (P2266) to replace that mechanism with something much simpler.

So since this is an ObjectiveC thing, how is the semantics of this interaction defined, just in terms of C++ NRVO?
As this reuses PerformMoveOrCopyInitialization, it is also affected by the change going into C++20 where the restriction on only converting constructors was lifted. Was this accidental and nobody noticed?

For this patch I am going to revert to the original behavior, but I think this could be revisited in the future so it gets aligned with the C++2b rules after P2266, which would be just having VarRef be an XValue unconditionally and perform the regular copy initialization.

Was there really not a test case covering those semantics?

Nope...

[mizvekov]

-bugs
+tests

[mizvekov]

@rjmccall Added some tests covering these semantics: clang/test/SemaObjCXX/block-capture.mm and also some extra documentation to checkEscapingByref. Not sure if I got the terminology right here..
@Quuxplusone any other interesting corner cases for these new tests?

[• Quuxplusone]

Some tiny nits. I can't think of any interesting block-related cases (but my knowledge of blocks is apparently very rusty at this point).

clang/lib/Sema/SemaTemplateInstantiateDecl.cpp

1100

/depdendent/dependent/

clang/test/CodeGen/nrvo-tracking.cpp

206

Formatting nit: I'd think

auto t = []<class T = X>() {
    return ^X() [[clang::vectorcall]] {
        T t;
        return t;
    };
}()();

would be less confusing to the eyes.

clang/test/SemaObjCXX/block-capture.mm

43

Nit: ;; here

[mizvekov]

Fix Arthur's nits.

[anhtuyen]

Hi Matheus @mizvekov,
The commit 12c90e2e25dfd1d38250055efc21acb42d158912 from this patch seems to cause a regression, where an assertion failure starts to occur with testcases such as

template <int A> int foo() {
  int a alignas(A) = 0;
  return a;
}

To reproduce the issue, please try:

1. Reset the HEAD of your branch to commit 12c90e2e25dfd1d38250055efc21acb42d158912, which was just before yours:

git reset --hard 12c90e2e25dfd1d38250055efc21acb42d158912

2. Build llvm/clamg
3. Compile the above reduced testcase (let's call it test.cpp)

bin/clang -std=c++11 -c test.cpp

4. There should be no warning nor error

5. Now, please pull in commit 12c90e2e25dfd1d38250055efc21acb42d158912

git pull origin 12c90e2e25dfd1d38250055efc21acb42d158912

6. Rebuild and rerun the above compilation command, you will see the following assertion failure

clang: tools/clang/include/clang/AST/AttrImpl.inc:1750: unsigned int clang::AlignedAttr::getAlignment(clang::ASTContext &) const: Assertion `!isAlignmentDependent()' failed.

GCC does not have that assertion, either.

Can you have a look, please!

[mizvekov]

In D99696#2856285, @anhtuyen wrote:

Hi Matheus @mizvekov,
The commit 12c90e2e25dfd1d38250055efc21acb42d158912 from this patch seems to cause a regression, where an assertion failure starts to occur with testcases such as

Hi Anh, thanks for reporting this problem!

I confirm it, can reproduce locally.
I will be working on a solution.

[mizvekov]

@anhtuyen

I pushed a DR with a preliminary fix for it: https://reviews.llvm.org/D105380

This is not ready to merge, still have to add test cases and also decide between a pointed fix like this, or improving the ergonomics of getDeclAlign by returning possible failure.

But it does fix your repro.

[anhtuyen]

In D99696#2856370, @mizvekov wrote:

@anhtuyen

I pushed a DR with a preliminary fix for it: https://reviews.llvm.org/D105380

This is not ready to merge, still have to add test cases and also decide between a pointed fix like this, or improving the ergonomics of getDeclAlign by returning possible failure.

But it does fix your repro.

Thank you very much Matheus @mizvekov for a quick fix.
I have tried your patch, and it worked for my testcase. I will wait for the official patch when you commit the changes.
Thanks, again!