[PATCH] D124351: [Clang] Implement Change scope of lambda trailing-return-type

[Elizabeth Andrews via Phabricator via cfe-commits]

eandrews added a comment.

In D124351#4208842 <https://reviews.llvm.org/D124351#4208842>, @cor3ntin wrote:

> In D124351#4207501 <https://reviews.llvm.org/D124351#4207501>, @eandrews wrote:
>
>> This patch causes an assertion when the attribute argument is an integer constant expression - https://godbolt.org/z/osKx5ejMb and has resulted in test fails downstream since any attribute which uses `VerifyIntegerConstantExpression` now hits this assert if used with lambdas.
>>
>> The assert hit is in `getCurLambda()` :
>>
>>   auto *CurLSI = dyn_cast<LambdaScopeInfo>(*I);
>>    if (CurLSI && CurLSI->Lambda && CurLSI->CallOperator &&
>>        !CurLSI->Lambda->Encloses(CurContext) && CurLSI->AfterParameterList) {
>>      // We have switched contexts due to template instantiation.
>>      assert(!CodeSynthesisContexts.empty()); <----- Hits this
>>      return nullptr;
>>    }
>>
>> Prior to this patch, Lambda Scope Information was not populated till after semantic analysis of attributes. This meant `CurLSI->Lambda` returned nullptr and we never entered the `if`. This patch however populates LSI during semantic analysis of lambda expression after introducer, which means we now enter the `if` during semantic analysis of the attribute` (`AfterParameterList` will be true at this point since this assert is hit way past parsing the parameters. `CurContext` is `foo`. I don't know if `CurContext` is right/wrong without further debugging.)
>>
>> For the godbolt test pasted above, neither before nor after this patch do we hit the code where `CodeSynthesisContexts` is populated. I wanted to see what code actually enters that block and so I tried deleting the code to see what tests fails. What is interesting is that nothing does :/ So I don't know if that means our tests are incomplete or if this code is not required.



> Thanks :)
> then make sure AfterParameterList is appropriately set to false when parsing GNU attributes. I might have time to look into it later today if you don't.

I did look into `AfterParameterList` a bit yesterday. I don't fully understand this PR yet and so I could be wrong, but I think it is handled correctly at the moment. This assert is hit during semantic analysis of attributes (i.e. ProcessDeclAttributes in ActOnLambdaDefinition). Shouldn't `AfterParameterList` be true at this point (like it is now)?

By the way we see this crash with some of our clang attributes downstream as well.

> As far i could tell, this check is only there to catch developer mistakes. We could probably hide it under #ifndef NDEBUG - seems preferable than to remove it entirely

Any attribute which takes the `VerifyIntegerConstantExpression` path fails. Since this assert is hit for valid code, I do think it needs to be fixed because valid code will fail in debug build even if we hide it under NDEBUG. If you can look into it today, that would be great!


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D124351/new/

https://reviews.llvm.org/D124351