This is an archive of the discontinued LLVM Phabricator instance.

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

Authored by mizvekov on Mar 31 2021, 5:42 PM.

Details

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 <mizvekov@gmail.com>

Diff Detail

Event Timeline

mizvekov created this revision.Mar 31 2021, 5:42 PM
mizvekov requested review of this revision.Mar 31 2021, 5:42 PM
Herald added a project: Restricted Project. · View Herald TranscriptMar 31 2021, 5:42 PM
Herald added a subscriber: cfe-commits. · View Herald Transcript

Wow, nice catches. For posterity, here's the missed-optimization being addressed IIUC: https://godbolt.org/z/MEoKGs7cE

Wow, nice catches. For posterity, here's the missed-optimization being addressed IIUC: https://godbolt.org/z/MEoKGs7cE

Thanks! And again thank you for providing test cases, it's what I planned to start doing next :)

mizvekov updated this revision to Diff 334575.Mar 31 2021, 6:44 PM

fix clang-tidy warning.

mizvekov updated this revision to Diff 335045.Apr 2 2021, 4:32 PM

Fixes for auto&, decltype(auto).
Exclude candidates from functions with dependent / undeduced return types which are references.
Adjusts the CodeGen tests from D99225.

mizvekov edited the summary of this revision. (Show Details)Apr 2 2021, 4:38 PM
mizvekov retitled this revision from [clang] WIP: NRVO: Improvements and handling of more cases. to [clang] NRVO: Improvements and handling of more cases..Apr 3 2021, 8:34 PM
mizvekov added reviewers: rsmith, aaronpuchert.
mizvekov updated this revision to Diff 335532.Apr 6 2021, 9:01 AM
  • small Doxygen fix.
nick added a subscriber: nick.Apr 7 2021, 11:28 AM

Didn't really check for correctness yet, just a superficial review.

I like the idea, splitting the functionality up definitely helps understanding this.

clang/include/clang/Sema/Sema.h
4764

Nitpick: I'd go with != None here.

clang/lib/Sema/SemaCoroutine.cpp
1585–1587

Perhaps this should just be direct initialization? Can't really find anything in the standard though.

clang/lib/Sema/SemaExprCXX.cpp
857

Any reason why you've moved that away from the comment that wants to explain it?

clang/lib/Sema/SemaStmt.cpp
3311–3313

The way you've designed the enumeration, couldn't you use std::min here?

3317–3318

The second backslash seems superfluous.

3367–3368

These comments seem to be separated from their context now...

3368

Braces are usually omitted if both branches are single statements.

3381–3382

Just remove "Return false if VD is a __block variable." No need to repeat the code, what follows is important.

3415

Can we maybe move this function up one place to make the diff a bit smaller? It appears to contain the first part of the original getCopyElisionCandidate.

3448

NRVOResult seems to be small, why not make this a proper function and let it return the result?

3464

parenthesized

3465–3467

Can't we just do this when we know what it deduces to? It sounds weird to handle dependent contexts here because we shouldn't attempt any return value initialization then.

clang/lib/Sema/SemaTemplateInstantiateDecl.cpp
1097

llvm_unreachable

mizvekov added inline comments.Apr 9 2021, 7:41 PM
clang/lib/Sema/SemaCoroutine.cpp
1585–1587

So I just checked this again. Regarding our conversation on IRC, what I said initially was correct: ReturnValue is always a member function expression, built by makeReturnObject -> buildPromiseCall -> buildMemberCall. So implicit move would never trigger here, and as far as I see there is no reason for this code to have used PerformMoveOrCopyInitialization in the first place.

clang/lib/Sema/SemaExprCXX.cpp
857

Yes, on C++2b this might modify Ex, and I moved it so this would happen before we do the check with the type of the expression just below here.

clang/lib/Sema/SemaStmt.cpp
3311–3313

Yes exactly!
The previous version of this patch I submitted used exactly that, but then for some reason I thought people could find std::min a bit odd here, and so I changed it.
But now I think I was right the first time around :)

3448

It does use the result parameter as input and output. This function can only "downgrade" a result it received previously.
I could make it receive a result and return the result, but then would probably just use it like this:

NRVORes = updNRVOResultWithRetType(NRVORes, ...);

Do you think that looks clearer?

mizvekov added inline comments.Apr 9 2021, 7:43 PM
clang/lib/Sema/SemaCoroutine.cpp
1585–1587

Err I meant: member function *call* expression

mizvekov added inline comments.Apr 9 2021, 7:47 PM
clang/lib/Sema/SemaExprCXX.cpp
857

But yeah I forgot to move the comment, good catch.

mizvekov added inline comments.Apr 10 2021, 9:31 AM
clang/lib/Sema/SemaCoroutine.cpp
1585–1587

Also, I don't think we could force direct initialization here, if the object returned by Gro is volatile for example.
With that said, copy elision from the sema action on the return statement of get_return_object should take care of removing this copy here, I think, I don't see any reason it would not work here just as well as for expressions coming from the parser.

clang/lib/Sema/SemaExprCXX.cpp
857

Actually, had my wires crossed there with the P2266 DR, I only need to move this line there.

clang/lib/Sema/SemaStmt.cpp
3465–3467

I think there are problems regarding the limitations of the current NRVO algorithm, but I had not had much time to study it or think of improvements, so I follow the current approach of trying to use as much available information as possible in order to eliminate candidates early.

With that said, this is not well covered by the test suite and I could probably get away with a lot less without breaking any existing tests.

mizvekov updated this revision to Diff 336626.Apr 10 2021, 11:59 AM
  • Addresses aaronpuchert's review points.
mizvekov marked 8 inline comments as done.Apr 10 2021, 12:03 PM
mizvekov added inline comments.
clang/lib/Sema/SemaStmt.cpp
3367–3368

This one comment here is talking about the line just below it.
Any other examples where it seems separated?

mizvekov marked an inline comment as done.Apr 10 2021, 12:04 PM
aaronpuchert added inline comments.Apr 10 2021, 3:07 PM
clang/lib/Sema/SemaStmt.cpp
3311–3313

Perhaps you can just add a comment to the enumeration saying that the values are totally ordered with regards to implication so that ⇒ is >=.

3367–3368

I meant the context of the comment itself. Originally this was quoting a clause

// - in a return statement in a function [where] ...
// ... the expression is the name of a non-volatile automatic object ...

and then additional parts later. Now you're having that initial part in a different function and it's not immediately clear what you're quoting here.

To be clear, this was about "(other than a function ... parameter)" below.

3448

Yes, that would seem more natural to me. Technically a caller could decide to not use the same variable in both places, for example it could pass a temporary or pass the result on directly.

mizvekov added inline comments.Apr 10 2021, 8:02 PM
clang/lib/Sema/SemaStmt.cpp
3448

But for this function, I don't see how it could make sense to actually use it like that.

You are either using it in a context where you don't have a ReturnType (throw, co_return), and you only care about the initial NRVOResult and don't call this function at all, or you are in a return statement, where you will call this function passing the initial NRVOResult, which will then have no further utility.

clang/lib/Sema/SemaStmt.cpp
3448

I haven't looked at the code, but FWIW, I'm always in favor of more "function-style" functions. Which would you rather deal with — void square(int& x) or int square(int x)? The same rationale applies uniformly everywhere. Even if we are "consuming" the argument in this particular instance, that might change after another few years of maintenance; and regardless, it might be easier for the maintainer to understand "function-style" code even if they aren't going to change it.

aaronpuchert added inline comments.Apr 11 2021, 1:46 PM
clang/lib/Sema/SemaCoroutine.cpp
1585–1587

In InitializationKind there is a distinction between IK_Copy (basically T x = init;) and IK_Direct (basically T x(init);). I don't know how this would be related to volatile, but it is related to explicit.

mizvekov updated this revision to Diff 336736.Apr 11 2021, 8:27 PM
mizvekov marked an inline comment as done.
  • Removes code that tried to allow copy elision for functions with dependent 'auto' return types. The reason is explained in new comments. Will try to address these in future work.
  • Addresses aaronpuchert's review points.
mizvekov marked an inline comment as done.Apr 11 2021, 8:30 PM
mizvekov added inline comments.
clang/lib/Sema/SemaStmt.cpp
3448

I changed the function name and made it return instead the candidate itself in case it was copy elisible, which allowed simplifying some other expressions afterwards.

But in answer to the points made in this thread: I will generally prefer the syntax which mostly matches the intended usage, making it easier to do the right thing and harder to do the wrong thing.

This function implements a fragment of the C++ standard text in a very specific context within clang. It is called from two places.
It's not a major piece of infrastructure or anything like that, to be concerned with unforeseen possible future uses :)

rsmith added inline comments.Apr 12 2021, 6:39 PM
clang/include/clang/Sema/Sema.h
4757–4764

We generally use FooResult to mean "either a Foo or an indicator that an error has been diagnosed". Would the name NRVOInfo work instead?

Actually, I think the "O" here is misleading, because we're not talking about performing the optimization here, just about what variable was named in a return statement. And I think calling this "NRVO" is a bit misleading because it also covers implicit move, which isn't traditionally part of NRVO. So how about NamedReturnValueInfo or NamedReturnInfo or NamedReturnValue or ReturnValueName or something like that?

clang/lib/Sema/SemaStmt.cpp
3337–3346

I find this name a little unclear (what do we mean by "downgrade"?). Can we call this something a bit more specific, such as disallowCopyElision or disallowNRVO?

3417

This comment needs re-flowing.

3423
3429–3432

How does this happen? Are there any cases where we could do NRVO or should do an implicit move that are blocked by this?

It seems to me that we should (nearly always) be able to work out whether copy elision is possible here without knowing the deduced type:

  • if the return type is cv auto then it will always be deduced to the type of the returned variable, so we can always perform copy elision
  • if the return type is decltype(auto), then we can perform copy elision if the expression is unparenthesized and otherwise cannot; we could perhaps track whether the expression was parenthesized in NRVOResult, and can conservatively disallow copy elision if we don't know (eg, from template instantiation, where we're only looking at the variable and not the return statements)
  • if the return type is anything else involving auto, it can't possibly instantiate to a class type, so we'll never perform copy elision
3431–3432
3435

Please add braces rather than using a comma expression here and below.

3576

Should we skip this if NRVORes.isMoveEligible() && getLangOpts().CPlusPlus20? In that case, we already tried an unrestricted move a couple of lines ago, so we know that adding std::move won't help. (I think we can still hit the "adding std::move will help" case in C++20, but only in obscure corners such as a volatile-qualified local variable.)

3753

I find the pre-existing NRVOCandidate != nullptr more readable than this !!NRVOCandidate.

clang/lib/Sema/SemaTemplateInstantiateDecl.cpp
1094

Can you use getCurBlock()->FunctionType for this? We won't have a Scope, but we should still have a ScopeInfo.

mizvekov added inline comments.Apr 12 2021, 7:20 PM
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 added inline comments.Apr 12 2021, 8:08 PM
clang/lib/Sema/SemaStmt.cpp
3429–3432

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

rsmith added inline comments.Apr 12 2021, 9:38 PM
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 updated this revision to Diff 337248.Apr 13 2021, 1:31 PM
mizvekov marked 7 inline comments as done.
  • Address rsmith review comments.
mizvekov added inline comments.Apr 13 2021, 1:33 PM
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 added inline comments.Apr 13 2021, 1:35 PM
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.

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 added inline comments.Apr 13 2021, 2:20 PM
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 added inline comments.Apr 13 2021, 2:27 PM
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.

@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 updated this revision to Diff 337266.Apr 13 2021, 3:02 PM
  • 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.
mizvekov marked 3 inline comments as done.Apr 13 2021, 3:52 PM
aaronpuchert added inline comments.Apr 13 2021, 3:56 PM
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 added inline comments.Apr 13 2021, 4:29 PM
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.

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 added inline comments.Apr 14 2021, 3:43 PM
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 updated this revision to Diff 337903.Apr 15 2021, 2:31 PM
  • Added doc to disallowNRVO
  • Also detect implicit return type for blocks.
Quuxplusone accepted this revision.Jun 9 2021, 10:46 AM

@mizvekov, my understanding is:

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?

This revision is now accepted and ready to land.Jun 9 2021, 10:46 AM

@mizvekov, my understanding is:

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

This revision was automatically updated to reflect the committed changes.

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

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.

mizvekov reopened this revision.Jun 11 2021, 8:42 AM

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.

This revision is now accepted and ready to land.Jun 11 2021, 8:42 AM
mizvekov updated this revision to Diff 351462.Jun 11 2021, 8:45 AM

Look through AttributedType when obtaining FunctionDecl return type.

Adds a couple more test cases to cover this.

This revision was automatically updated to reflect the committed changes.

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.

@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 added a comment.Jun 14 2021, 7:24 AM

@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 added a comment.Jun 14 2021, 7:48 AM

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.

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 reopened this revision.Jun 14 2021, 1:43 PM
This revision is now accepted and ready to land.Jun 14 2021, 1:43 PM

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 updated this revision to Diff 352271.Jun 15 2021, 3:24 PM

-bugs
+tests

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

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
44

Nit: ;; here

mizvekov updated this revision to Diff 352566.Jun 16 2021, 3:03 PM

Fix Arthur's nits.

This revision was landed with ongoing or failed builds.Jun 16 2021, 4:56 PM
This revision was automatically updated to reflect the committed changes.
anhtuyen added a subscriber: anhtuyen.EditedJul 2 2021, 3:08 PM

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
  1. Build llvm/clamg
  2. Compile the above reduced testcase (let's call it test.cpp)
bin/clang -std=c++11 -c test.cpp
  1. There should be no warning nor error
  1. Now, please pull in commit 12c90e2e25dfd1d38250055efc21acb42d158912
git pull origin 12c90e2e25dfd1d38250055efc21acb42d158912
  1. 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!

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.

@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

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!