This implements P2036R3 and P2579R0.
That is, explicit, int, and implicit capture become visible
at the start of the parameter head.
Details
Details
- Reviewers
rsmith aaron.ballman erichkeane MaskRay jdoerfert rupprecht shafik - Group Reviewers
Restricted Project - Commits
- rG93d7002dc464: [Clang] Implement Change scope of lambda trailing-return-type
rGd708a186b6a9: [Clang] Implement Change scope of lambda trailing-return-type
Diff Detail
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
There are a very large number of changes, so older changes are hidden. Show Older Changes
Comment Actions
@aaron.ballman Thanks for the review! I hope we won't find further issue with the paper this time :)
Comment Actions
FYI this causes a minor compile-time regression (around 0.35% on 7zip at O0): http://llvm-compile-time-tracker.com/compare.php?from=cd173cbd7cca69c29df42cd4b42e60433435c29b&to=d708a186b6a9b050d09558163dd353d9f738c82d&stat=instructions%3Au
Just wanted to check whether that's expected.
Comment Actions
It's certainly not great but not entirely unexpected as we are doing quite a bit more work, although it should only affect code that constructs a lot of distinct lambda. Maybe an issue to track that would be useful, there may be some improvements opportunities
Thanks a lot for letting me know
Comment Actions
Does 7zip meet that criteria? As a gut-check that this only costs in that case, would be nice to know if the 7zip regression is that case, or is this the cost even when that isn't true - so we could expect higher costs when it is true?
Comment Actions
I just downloaded their latest sources and didn't spot any lambdas in the C++ code with a simple grep. They also don't use auto so I suspect lambda usage is minimal. (It's possible I missed something, I didn't look exhaustively.)
Comment Actions
Thanks. Then it's lookup related... I'll have to investigate. Coming up with a meaningful benchmark will be a challenge though.
Comment Actions
Hi, me again :)
I ran into an interesting build breakage from this, I can't tell if it's a legitimate breakage based on reading P2036R3 and P2579R0 (mostly I'm not a language lawyer).
struct StringLiteral {
template <int N>
StringLiteral(const char (&array)[N])
__attribute__((enable_if(N > 0 && N == __builtin_strlen(array) + 1,
"invalid string literal")));
};
struct Message {
Message(StringLiteral);
};
void Func1() {
auto x = Message("x"); // Note: this is fine
// Note: "xx\0" to force a different type, StringLiteral<3>, otherwise this
// successfully builds.
auto y = [&](decltype(Message("xx"))) {};
// ^ fails with: repro.cc:18:13: error: reference to local variable 'array'
// declared in enclosing function 'StringLiteral::StringLiteral<3>'
(void)x;
(void)y;
}https://godbolt.org/z/M4E6jKxxn
Does this look like an intended breakage from this patch?
Comment Actions
Looks like we fail to enter the appropriate context somewhere (my guess is that it might be specific to the attribute but it's hard to say without picking around), definitely a bug
I'll be away the next 2 weeks, I'll look at it when I get back. Maybe we should track it in an issue though.
Comment Actions
Looks like we fail to enter the appropriate context somewhere (my guess is that it might be specific to the attribute but it's hard to say without picking around), definitely a bug
I'll be away the next 2 weeks, I'll look at it when I get back. Maybe we should track it in an issue though.
Should we revert this then?
Comment Actions
Yes, I think we should. We should also file an issue so @cor3ntin doesn't lose track of the issue. Anyone want to volunteer to do that? (I can do it early next week otherwise.)
Comment Actions
Done in 74ce297045bac4bc475b8e762d2a1ea19bb16d3c and filed https://github.com/llvm/llvm-project/issues/60518 to track re-landing this.
| clang/lib/Sema/SemaExpr.cpp | ||
|---|---|---|
| 19145 | Formatting nit. I have a hazy recollection that we sometimes have a weird declaration context for parameters where the parameter doesn't think it's associated with a function DC but instead with the translation unit DC. I seem to recall this having something to do with PCH/AST importing, but I can't seem to find any open issues about it. So this might be fine, but it may have some weird edge cases still. | |
| clang/test/SemaCXX/lambda-expressions.cpp | ||
| 675–676 |
template <int N>
void foo(const char (&array)[N]) [[clang::annotate_type("test", array)]]; | |
| clang/lib/Sema/SemaExpr.cpp | ||
|---|---|---|
| 19145 | Yep, parameters are all created in the top level dc before being attached to the function. but by the time they get to be captured by a lambda they are attached to the function | |
| clang/test/SemaCXX/lambda-expressions.cpp | ||
| 675–676 | Yes, this is something that was broken by this patch. | |
| clang/lib/Sema/SemaExpr.cpp | ||
|---|---|---|
| 19145 | Phew, thanks for checking! | |
| clang/test/SemaCXX/lambda-expressions.cpp | ||
| 675–676 | So I'm not certain what's being tested then -- how are you validating that array is no longer ODR used? In terms of annotate_type, that attribute accepts a string literal followed by a variable number of expression arguments, so I think it should have the same issues as a GNU-style attribute accepting an expression argument. I was mostly trying to wrap my head around whether [[]] and __attribute__ are expected to have different behaviors when written in that position. | |
| clang/test/SemaCXX/lambda-expressions.cpp | ||
|---|---|---|
| 675–676 | ||
Comment Actions
LGTM
| clang/test/SemaCXX/lambda-expressions.cpp | ||
|---|---|---|
| 675–676 | I talked to @cor3ntin off-list to get a better understanding of the test situation, and I realized the issue was that this code used to give a diagnostic that it shouldn't have, so this is demonstrating we accept the code rather than reject it. That makes a lot more sense now! | |
| clang/lib/Sema/SemaConcept.cpp | ||
|---|---|---|
| 537 | This is just duplicated across the if and else, please refactor it out. If this ever gets modified folks have to fix it in both places and that is easy to mess up. | |
| clang/lib/Sema/SemaExpr.cpp | ||
|---|---|---|
| 19068 | We have a bunch of bugs that crash in the method and it would be good to rescreen those bugs after landing this to see if it fixes any of those. | |
| clang/lib/Sema/SemaLambda.cpp | ||
| 928 | This has changed a bit so might be worth updating the wording here to reflect C++20. | |
| 940–942 | minor fix | |
| 1017 | ||
| 1020 | Same as comment above about updating wording to reflect C++20. | |
| 1254–1255 | ||
| clang/lib/Sema/TreeTransform.h | ||
| 13254 | ||
Comment Actions
@rupprecht Great to hear! Sorry again for the disruption
@shafik Thanks for the review!
| clang/lib/Sema/SemaLambda.cpp | ||
|---|---|---|
| 1020 | I don't even think we should repeat that comment here. | |
Comment Actions
@shafik Can you let me know if you are happy with the changes i made to address your feedback? Thanks!
Comment Actions
We're seeing new clang crashes that bisect to this commit, with modules only.
I have it mostly-reduced and will post shortly, trying to see if I can simplify any further (since modules reproducers are a pain).
Meanwhile, the assert/stack in case it's already useful:
assertion failed at third_party/llvm/llvm-project/clang/lib/Sema/SemaTemplateInstantiate.cpp:4065 in llvm::PointerUnion<Decl *, LocalInstantiationScope::DeclArgumentPack *> *clang::LocalInstantiationScope::findInstantiationOf(const Decl *): isa<LabelDecl>(D) && "declaration not instantiated in this scope"
*** Check failure stack trace: ***
@ 0x5609e4f16f44 __assert_fail
@ 0x5609e1894234 clang::LocalInstantiationScope::findInstantiationOf()
@ 0x5609e18d073c clang::Sema::FindInstantiatedDecl()
@ 0x5609e18a99d0 clang::TreeTransform<>::TransformLambdaExpr()
@ 0x5609e189dbee (anonymous namespace)::TemplateInstantiator::TransformLambdaExpr()
@ 0x5609e1892442 clang::TreeTransform<>::TransformExprs()
@ 0x5609e189a71a clang::TreeTransform<>::TransformCallExpr()
@ 0x5609e189097a clang::TreeTransform<>::TransformStmt()
@ 0x5609e18afa54 clang::TreeTransform<>::TransformCompoundStmt()
@ 0x5609e1890902 clang::Sema::SubstStmt()
@ 0x5609e18e31df clang::Sema::InstantiateFunctionDefinition()
@ 0x5609e18e5ed2 clang::Sema::PerformPendingInstantiations()
@ 0x5609e0fcad44 clang::Sema::ActOnEndOfTranslationUnitFragment()
@ 0x5609e0fcbb66 clang::Sema::ActOnEndOfTranslationUnit()
@ 0x5609e0d298e6 clang::Parser::ParseTopLevelDecl()
@ 0x5609e0d2388e clang::ParseAST()
@ 0x5609e0a647c3 clang::FrontendAction::Execute()
@ 0x5609e09d81ad clang::CompilerInstance::ExecuteAction()
@ 0x5609dfa05b08 clang::ExecuteCompilerInvocation()
@ 0x5609df9f99f1 cc1_main()
@ 0x5609df9f5d28 ExecuteCC1Tool()
@ 0x5609e0b868be llvm::function_ref<>::callback_fn<>()
@ 0x5609e4d9ec35 llvm::CrashRecoveryContext::RunSafely()
@ 0x5609e0b86103 clang::driver::CC1Command::Execute()
@ 0x5609e0b44166 clang::driver::Compilation::ExecuteCommand()
@ 0x5609e0b4448f clang::driver::Compilation::ExecuteJobs()
@ 0x5609e0b63e70 clang::driver::Driver::ExecuteCompilation()
@ 0x5609df9f4ee7 clang_main()
@ 0x5609df9f1bc4 main
@ 0x7fad2cda4633 __libc_start_main
@ 0x5609df9f1b2a _startComment Actions
Thanks for letting me know.
A reproduction will be very useful to track that down indeed, I'll look into it as soon as i hear back from you. The stack trace doesn't tell me much unfortunately, although it definitely appears related.
Comment Actions
Sorry for the delay, extracting the repro from the build system seems about as much work as minimizing it :-)
(I suspect something much smaller is breaking the expected invariant, I just can't get a smaller crasher)
Comment Actions
@ChuanqiXu @erichkeane I started to look at this repro but I might need help, it seems extremely weird (I failed to reduce it much further), and I don't have a good grasp on how modules affect things
Comment Actions
I'm trying to help out here as well, but in addition to @ChuanqiXu and @erichkeane, I wonder if @iains or @urnathan have ideas as well (they've both done modules work before).
Comment Actions
Please keep in mind that I don't have a lot of expertise in modules, but this sure seems like a modules bug from what I can tell, and it might even relate to work happening in D145737 (CC @rsmith). That patch does not seem to address this issue, but what I'm seeing when I try to debug this is a bit confusing to me:
In the reproducer from Sam, there are three lambdas:
create::Create<R>([] {}); in main.cpp, but this does not participate in AST merging because it's within the main source file
BuildWidget<T>([dummy](Widget<T>*) {}); in create.h
create::Create<float>([] {}); in instantiate_create.h
There are two modules involved (wrap_create and instantiate_create). I would expect the wrap_create module to contain one lambda (from including create.h) and the instantiate_create module to contain two lambdas (one from InstantiateCreate() and one from including create.h).
So I would expect ASTDeclReader::ReadCXXDefinitionData() to find three lambdas, but instead it finds *four*. The first lambda it reads is the one in InstantiateCreate(). The next three all have captures of dummy; the first two come from the instantiate_create module and the last one comes from the wrap_create module.
Comment Actions
There should be three of these in total: one from wrap_create.h's textual inclusion of create.h, one from instantiate_create.h's textual inclusion of create.h, and one from instantiate_create.h's instantiation of Create<float>. The first two should get merged together if they're both loaded. This runs into some issues, because the local dummy variable in the two different bodies of the Create template don't get merged. In rG4a1ccfe8a3cfd4569bc962a38b6117a9a2b8ad21 I attempted to fix that by tracking all the lists of captures from different merged copies of the lambdas, so we can find the right way to name (eg) dummy in the enclosing context. Possibly some more work is needed here, to make template instantiation look through that list when instantiating a reference to a declaration, in the case where we pick a lambda body from one module but pick the enclosing function's body from a different module.
The first lambda it reads is the one in InstantiateCreate(). The next three all have captures of dummy; the first two come from the instantiate_create module and the last one comes from the wrap_create module.
That sounds right to me. One of the ones from instantiate_create (the one taking Widget<T>*, not the one taking Widget<float>*) should be merged with the one from wrap_create.h.
| clang/lib/Sema/SemaCXXScopeSpec.cpp | ||
|---|---|---|
| 295–298 | Will this also reject __super in a local class inside a lambda? That seems like something we should still permit. | |
| clang/lib/Sema/SemaCXXScopeSpec.cpp | ||
|---|---|---|
| 295–298 | https://godbolt.org/z/dEj3vG8nc In both cases we create a non capturing function scope so getCurLambda returns nullptr | |
Comment Actions
It looks like that the Create template functions get merged expectedly. And the dummy variable shouldn't get merged since it is not in a mergeable primary context (function context is not a mergeable primary context) (see https://github.com/llvm/llvm-project/blob/23bd0e037b744d1f93bdfad59b7575017725a96c/clang/lib/Serialization/ASTReaderDecl.cpp#L3107-L3151).
When I remove the use of dummy in the captures list, https://github.com/llvm/llvm-project/blob/23bd0e037b744d1f93bdfad59b7575017725a96c/clang/lib/Sema/SemaTemplateInstantiateDecl.cpp#L6079 is not executed. No matter if the dummy variable is captured or not, https://github.com/llvm/llvm-project/blob/23bd0e037b744d1f93bdfad59b7575017725a96c/clang/lib/Sema/SemaTemplateInstantiate.cpp#L4069-L4088 will execute twice for the dummy variable.
Comment Actions
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.
Comment Actions
Thanks :)
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 - and 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.
Comment Actions
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!