This is an archive of the discontinued LLVM Phabricator instance.

[Clang][C++20] Support capturing structured bindings in lambdas
ClosedPublic

Authored by cor3ntin on Mar 30 2022, 3:23 PM.

Details

Summary

This completes the implementation of P1091R3 and P1381R1.

This patch allow the capture of structured bindings
both for C++20+ and C++17, with extension/compat warning.

In addition, capturing an anonymous union member,
a bitfield, or a structured binding thereof now has a
better diagnostic.

We only support structured bindings - as opposed to other kinds
of structured statements/blocks. We still emit an error for those.

In addition, support for structured bindings capture is entirely disabled in
OpenMP mode as this needs more investigation - a specific diagnostic indicate the feature is not yet supported there.

Note that the rest of P1091R3 (static/thread_local structured bindings) was already implemented.

at the request of @shafik, i can confirm the correct behavior of lldb wit this change.

Fixes https://github.com/llvm/llvm-project/issues/54300
Fixes https://github.com/llvm/llvm-project/issues/54300
Fixes https://github.com/llvm/llvm-project/issues/52720

Diff Detail

Event Timeline

cor3ntin created this revision.Mar 30 2022, 3:23 PM
Herald added a project: Restricted Project. · View Herald Transcript
cor3ntin requested review of this revision.Mar 30 2022, 3:23 PM
Herald added projects: Restricted Project, Restricted Project. · View Herald Transcript
cor3ntin retitled this revision from [Clang] Support capturing structured bindings in lambdas to [WIP][Clang] Support capturing structured bindings in lambdas.Mar 30 2022, 3:23 PM

This is not quite mature.
I think it needs a few more tests, notably codegen tests which I'm not sure how to write properly.

cor3ntin updated this revision to Diff 419281.Mar 30 2022, 3:46 PM

Remote commented-out code

cor3ntin added inline comments.Mar 30 2022, 4:01 PM
clang/lib/CodeGen/CGOpenMPRuntime.cpp
9060

I'm not sure we currently prohibit capturing a structured binding in openmp, i should fix that

cor3ntin updated this revision to Diff 419650.Apr 1 2022, 12:20 AM

Fix tests?

cor3ntin updated this revision to Diff 419657.Apr 1 2022, 12:50 AM
  • formatting
  • add more tests
cor3ntin updated this revision to Diff 419710.Apr 1 2022, 4:15 AM

Fix typo in test, fix captureInLambda (was crashing in release and not debug)

cor3ntin updated this revision to Diff 419765.Apr 1 2022, 8:12 AM

Add a diagnostic in openmp mode.

Supporting structured bindings in OpenMP mode requires OpenMP expertise,
so we emit a diagnostic in -fopenmp mode informing that feature is not supported
yet.
My idea is that whether and how openmp should handle this feature should not
delay support of the c++20 features for people who do not depend on OpenMP.

cor3ntin updated this revision to Diff 419880.Apr 1 2022, 3:55 PM

Try to rerun the build

cor3ntin updated this revision to Diff 419970.Apr 2 2022, 5:29 AM
  • Add codegen test
  • Fixed a typo that caused openmp test failures
  • Cleanups & formatting
cor3ntin retitled this revision from [WIP][Clang] Support capturing structured bindings in lambdas to [Clang][C++20] Support capturing structured bindings in lambdas.Apr 2 2022, 5:52 AM
cor3ntin edited the summary of this revision. (Show Details)
cor3ntin added inline comments.Apr 2 2022, 5:55 AM
clang/lib/CodeGen/CGOpenMPRuntime.cpp
9060

I disabled the feature in OpenMP mode, as this needs more investigation

cor3ntin updated this revision to Diff 419988.Apr 2 2022, 8:48 AM

Better codegen test

cor3ntin updated this revision to Diff 423228.Apr 16 2022, 1:01 AM

Fix several conflicrs with the recently committed change to lambda captures
(D119136).

Also add to rewrite the code gen test. I'm not sure what, if anything
changed there, and the generated IR *seems* fine?

cor3ntin updated this revision to Diff 436212.Jun 12 2022, 5:55 AM
  • Rebase
  • Cleanup codegen test
cor3ntin added inline comments.Jun 28 2022, 5:49 AM
clang/tools/libclang/CIndex.cpp
3353–3357

Note to self: typo

I think I need to do another sweep of this PR but mostly minor comments.

You should add tests for structured bindings binding to array and tuple-like types b/c the AST under the BindingDecl will be different for each of those cases and it would be worth verifying it works for all the cases. Although I don't see anything that looks like it would not.

cor3ntin updated this revision to Diff 440912.Jun 29 2022, 2:47 AM
  • fix typo
  • add decompositions tests for the array and tuuple cases

Thank you for this work. This looks mostly good to me but I am not confident on all the code. I feel like there are some assumptions that maybe could use comments e.g. places where we know it can only be a VarDecl e.g. b/c it is an init capture.

clang/lib/CodeGen/CGExpr.cpp
2910

Why the assert? In the previous use of LambdaCaptureFields.lookup(...) we are not doing that.

clang/lib/Sema/SemaExpr.cpp
17858–17860

What are we checking here? Why does it only apply to VarDecls?

17930

This block does not follow the pattern of the blocks above, where they do if (Diagnose) and always return false. Your else if branch diagnoses but does not return false.

clang/lib/Sema/SemaInit.cpp
7792

I see we are doing this kind of check to see if we have a VarDecl and then check if it is an init capture and I wish there was a way not to repeat this but I don't see it.

clang/lib/Sema/SemaLambda.cpp
1213

We also do a hasLocalStorage() check in getParentOfCapturingContextOrNull(...) but only look for the VarDecl case there.

cor3ntin updated this revision to Diff 444283.Jul 13 2022, 8:42 AM
  • Rename diagnoseUncapturableValueReferenceOr => diagnoseUncapturableValueReferenceOrBinding
  • Remove superfluous assert
  • Check for unsupported binding decl in isVariableCapturable instead of BuildDeclarationNameExpr, which is cleaner and more consistent. Then getParentOfCapturingContextOrNull can check all non-captured variables and bindings.

@shafik Thanks for the review. I was able to reorganize the things you pointed out to be more consistent such that the checks for structured bindings and regular vars are made in the same place.
I think it's much better that way. It took me a while to find the correct way to do that, which was somewhat useful in hindsight.

clang/lib/Sema/SemaInit.cpp
7792

I considered having a function In ValueDecl, what do you think?

cor3ntin added inline comments.
clang/test/CodeGenCXX/cxx20-decomposition.cpp
2

@erichkeane This is my first time writing a code gen test, and I'm not exactly sure what I'm doing. Mind taking a look?
Thanks!

Just about all of the '%' variables in LLVM are unstable names, so you need to use wildcards. Additionally, it seems like you're checking EVERY line of the llvm function, which is likely a mistake, it ends up being incredibly unstable as a result. I'd suggest just checking the individual lines (and the bare minimum on them!) that you need to prove that the 'right thing' is being emitted.

clang/test/CodeGenCXX/cxx20-decomposition.cpp
18

Are all of these lines necessary to prove what you want to prove? This seems like a lot of extra check lines.

20

Names of LLVM variables are NOT stable, and in fact, some configs of the compiler remove them. You typically want to use filecheck 'wildcards' for them.

cor3ntin updated this revision to Diff 444330.Jul 13 2022, 10:34 AM

Simplify codegen test

Just about all of the '%' variables in LLVM are unstable names, so you need to use wildcards. Additionally, it seems like you're checking EVERY line of the llvm function, which is likely a mistake, it ends up being incredibly unstable as a result. I'd suggest just checking the individual lines (and the bare minimum on them!) that you need to prove that the 'right thing' is being emitted.

Thanks. I tried to simplify as much as possible. I *think* i did not remove anything of importance

erichkeane added inline comments.Jul 13 2022, 10:38 AM
clang/test/CodeGenCXX/cxx20-decomposition.cpp
27

Which is the important lines here? You might want to use the [[NAME:.whatever]] (then on the 'other' side: [[NAME]])syntax in here to make sure that the check-lines don't find something else.

You also likely want to use .+ to make sure there is actually a character in there.

cor3ntin added inline comments.Jul 13 2022, 10:48 AM
clang/test/CodeGenCXX/cxx20-decomposition.cpp
27

I'm trying to show i is captured by value and j isn't

cor3ntin marked 4 inline comments as done.Jul 14 2022, 7:44 AM
cor3ntin added inline comments.
clang/lib/CodeGen/CGExpr.cpp
2910

It can be ( and was) removed!

clang/lib/Sema/SemaExpr.cpp
17858–17860

You are right, we should check for Both kids of value. And we *did* but the check was split
in multiple locations, which was not great, changed that. Great point.

FYI, diagnoseUncapturableValueReferenceOrBinding is used both to diagnose bindings in modes they can't be captured and references to not captured variables.
I took the liberty to rename so the name is more explicit.

17930

It still somewhat not completely consistent as we only emit a warning in the C++ case, but the check for captured bindings is now done there

martong removed a subscriber: martong.Jul 14 2022, 7:51 AM
cor3ntin marked 3 inline comments as done.Jul 23 2022, 10:29 PM

@shafik I just realized i forgot to submit a bunch of replies i had to your comments, sorry about that

shafik added inline comments.Jul 24 2022, 4:49 PM
clang/lib/AST/StmtPrinter.cpp
2074

I wonder if it is worth commenting that only a VarDecl can be an init capture and therefore we can unconditionally cast to VarDecl?

clang/lib/Sema/SemaInit.cpp
7792

I thought about that but when I looked at ValueDecl it seemed pretty lightweight.

@aaron.ballman wdyt is ValueDecl the right place or is this code repetition ok?

@aaron.ballman @shafik I'm perfectly happy to add isInitCapture in ValueDecl if that allows us to make progress on this. Let me know what you think!

aaron.ballman added inline comments.Jul 28 2022, 8:09 AM
clang/include/clang/AST/Stmt.h
61–63

We usually keep these alphabetical.

clang/lib/AST/ExprCXX.cpp
1210–1212

I think early returns help make this a bit more clear.

clang/lib/AST/StmtPrinter.cpp
2075
clang/lib/Analysis/AnalysisDeclContext.cpp
173

This looks dangerous -- isSelfDecl() uses the pointer variable in ways that would be Bad News for null pointers.

clang/lib/CodeGen/CGOpenMPRuntime.cpp
9060

Ping @jdoerfert -- do you happen to know what's up here?

clang/lib/Sema/ScopeInfo.cpp
223–225

This also has a bad code smell for isa<> followed by cast<>, but I'm not certain if the rewrite is actually better or not.

clang/lib/Sema/SemaExpr.cpp
16044

Is cast<> safe here?

17872
18073

I'm assuming, given that the function didn't used to accept nullptr for Var and the else if is using dyn_cast<>.

18077

Can this return nullptr?

18080–18081
18085

Can this return nullptr?

18097
18295–18303
18296–18302
clang/lib/Sema/SemaInit.cpp
7792

I think it makes sense to sink this into ValueDecl given how often it's come up. We really try to discourage people from doing isa<> followed by a cast<> as that's a bad code smell for dyn_cast<>.

clang/lib/Sema/SemaLambda.cpp
1169–1172
1208–1209

Does getDecomposedDecl() ever actually return nullptr?

1717–1719
clang/lib/Sema/TreeTransform.h
12884
13069

I'll stop commenting on these -- you should take a pass over your changes and use const auto * or auto * anywhere the type is spelled on the RHS via cast<>, dyn_cast<>`, or similar.

clang/www/cxx_status.html
1147
erichkeane added inline comments.Jul 28 2022, 8:19 AM
clang/lib/AST/ExprCXX.cpp
1210–1212

I might suggest making that:

if (const auto *VD = dyn_cast<VarDecl(C->getCapturedVar())
   return VD->...
return false;

But otherwise agree with the suggestion.

clang/lib/Analysis/AnalysisDeclContext.cpp
173

Yep, isSelfDecl seems to do:

return isa<ImplicitParamDecl>(VD) && VD->getName() == "self";

isa isn't nullptr safe, we have isa_and_nonnull for that (if we want to update isSelfDecl).

clang/lib/Sema/SemaDecl.cpp
14337–14342
cor3ntin updated this revision to Diff 448524.Jul 28 2022, 11:59 PM
cor3ntin marked 19 inline comments as done.

Address most comments, except for lifting isInitCapture into ValueDecl,
which I will do separately.

cor3ntin updated this revision to Diff 448539.Jul 29 2022, 1:09 AM

Put isInitCapture in ValueDecl.

This allows a few code simplification in the resst of the patch.

cor3ntin updated this revision to Diff 448543.Jul 29 2022, 1:26 AM
cor3ntin marked 7 inline comments as done.

Add comments

cor3ntin added inline comments.Jul 29 2022, 4:30 AM
clang/lib/Analysis/AnalysisDeclContext.cpp
173

Using isa_and_nonnull looks like the best solution here

clang/lib/Sema/SemaDecl.cpp
14337–14342

I'm not sure I understood your suggestion.
The reason there is a local ValueDecl in addition of the VarDecl is because we use it Line 14660 just below.
But that was reworked when lifting isInitCapture in ValueDecl anyway.

clang/lib/Sema/SemaExpr.cpp
16044

Yes, I added a comment to make that clear

17872

Fancy. The parentheses are still needed though, because C macros are amazing like that :)

18077

I think so

/// Get the expression to which this declaration is bound. This may be null
/// in two different cases: while parsing the initializer for the
/// decomposition declaration, and when the initializer is type-dependent.
Expr *getBinding() const { return Binding; }

The initializer could be type dependant here, and so it could be null

18085

Yes, same thing as above

clang/lib/Sema/SemaLambda.cpp
1208–1209

I don't think that's necessary indeed, nice catch

CodeGen tests is still not testing what we intend I believe (sorry if I missed the response to this!), plus I would like a quick explanation as to what is going on in 1 place, otherwise I think this is about right. Hopefully Aaron/Shafik can take a quick look through to confirm as well.

clang/lib/AST/ExprCXX.cpp
1210–1212

FWIW, this seems nicer now without the casts. I'm OK with this form, it is exactly what there was before minus some strange formatting/parens.

clang/lib/Sema/SemaExpr.cpp
17933

first, why are we checking for !CPlusPlus... aren't BindingDecl's C++ only? Or is there some ObjC thing for them?

second, can you clarify what the two diagnostic cases are doing? it isn't completely clear to me the purpose of the first call here.

clang/test/CodeGenCXX/cxx20-decomposition.cpp
27

Hmm... I don't think it is testing what you think you're testing, particularly with so many unnamed checks. If you send me the IR that this produces and highlight which are the 'important' lines, I can help write this for you.

cor3ntin added inline comments.Jul 29 2022, 7:51 AM
clang/lib/Sema/SemaExpr.cpp
17933

The first diagnostic is where we diagnose usages outside of C++ lambdas.
And below are just the C++ extension warnings

clang/test/CodeGenCXX/cxx20-decomposition.cpp
27

To be perfectly honest, I'm neither confident about which lines are important, nor whether the test case is as meaningful as it can be.
I'm happy to mail you the IR if you have time to look at it :)

erichkeane added inline comments.Jul 29 2022, 7:54 AM
clang/test/CodeGenCXX/cxx20-decomposition.cpp
27

Yep, feel free, I'll take a look when I get a chance.

cor3ntin updated this revision to Diff 448726.Jul 29 2022, 2:59 PM

Fix codegen test (Thanks Erich!)

cor3ntin updated this revision to Diff 448773.Jul 29 2022, 10:49 PM

Fix formatting

aaron.ballman added inline comments.
clang/lib/CodeGen/CGOpenMPRuntime.cpp
9060

Ping @mikerice or @ABataev in case @jdoerfert is on vacation.

(Note, I don't think an answer to this question is needed to accept this review.)

ABataev added inline comments.Aug 2 2022, 9:40 PM
clang/lib/CodeGen/CGOpenMPRuntime.cpp
9060

Need to support capturing of structured bindings in OpenMP regions, some people asked for it long time ago.

aaron.ballman accepted this revision.Aug 3 2022, 9:35 AM

I mostly only have minor nits, but otherwise this LGTM!

clang/docs/ReleaseNotes.rst
194–195

You should probably mention some of the issues that it fixes as well, since it handles a bunch of them.

clang/lib/CodeGen/CGOpenMPRuntime.cpp
9060

Thank you for confirming @ABataev -- I had missed that https://github.com/llvm/llvm-project/issues/33025 was already filed. Nothing to do here for this review, what's happening is an improvement.

clang/lib/Sema/SemaExpr.cpp
17864
clang/lib/Sema/SemaLambda.cpp
1207–1211
This revision is now accepted and ready to land.Aug 3 2022, 9:35 AM
cor3ntin updated this revision to Diff 449690.Aug 3 2022, 9:42 AM

Missed a comment (s/ValueDecl/auto/ on the LHS of a cast)

cor3ntin updated this revision to Diff 449696.Aug 3 2022, 10:03 AM
cor3ntin marked 2 inline comments as done.

Add fixed issues to the release notes + address aaron comments.

Thanks @aaron.ballman!
I'm gonna wait a bit before merging in case @erichkeane want to do another pass.

Last scroll-through looked fine to me, so don't worry about waiting for me.

This revision was landed with ongoing or failed builds.Aug 3 2022, 11:00 AM
This revision was automatically updated to reflect the committed changes.
cor3ntin reopened this revision.Aug 3 2022, 1:53 PM
This revision is now accepted and ready to land.Aug 3 2022, 1:53 PM
cor3ntin updated this revision to Diff 449780.Aug 3 2022, 2:01 PM

Fix self-build

  • The way we tested for unions was broken
  • It was also redundant, as we do that before in the same function (isVariableCapturable).

in the commit message

In addition, capturing an anonymous union member,
a bitfield, or a structured binding thereof now has a
better diagnostic.

"better diagnostic" implies there was already a diagnostic, but we're seeing new errors pop up. is this just a not precise commit message or are new errors related to capturing bitfields by reference unintentional?

in the commit message

In addition, capturing an anonymous union member,
a bitfield, or a structured binding thereof now has a
better diagnostic.

"better diagnostic" implies there was already a diagnostic, but we're seeing new errors pop up. is this just a not precise commit message or are new errors related to capturing bitfields by reference unintentional?

New errors when capturing bit fields are probably*not* intentional. You have some of the problematic code i could look at? (not that the patch was reverted and then re-landed to fix a bug there)
And... the commit message is a bit inaccurate as the change that was supposedly improving diagnostics for reference to bit fields was remove (and i forgot to update the commit message accordingly)

fhahn added a subscriber: fhahn.Aug 4 2022, 12:33 PM

in the commit message

In addition, capturing an anonymous union member,
a bitfield, or a structured binding thereof now has a
better diagnostic.

"better diagnostic" implies there was already a diagnostic, but we're seeing new errors pop up. is this just a not precise commit message or are new errors related to capturing bitfields by reference unintentional?

New errors when capturing bit fields are probably*not* intentional. You have some of the problematic code i could look at? (not that the patch was reverted and then re-landed to fix a bug there)
And... the commit message is a bit inaccurate as the change that was supposedly improving diagnostics for reference to bit fields was remove (and i forgot to update the commit message accordingly)

It looks like this may cause https://github.com/llvm/llvm-project/issues/56909. It would eb great if you could take a look

in the commit message

In addition, capturing an anonymous union member,
a bitfield, or a structured binding thereof now has a
better diagnostic.

"better diagnostic" implies there was already a diagnostic, but we're seeing new errors pop up. is this just a not precise commit message or are new errors related to capturing bitfields by reference unintentional?

New errors when capturing bit fields are probably*not* intentional. You have some of the problematic code i could look at? (not that the patch was reverted and then re-landed to fix a bug there)
And... the commit message is a bit inaccurate as the change that was supposedly improving diagnostics for reference to bit fields was remove (and i forgot to update the commit message accordingly)

reduced, this does seem wrong

$ cat /tmp/a.cc
struct S {
        int s : 4;
};

void f() {
        S s;
        int l = s.s;
        auto a = [&]() {
                l;
        };
}
$ clang++ -fsyntax-only /tmp/a.cc
/tmp/a.cc:10:3: error: cannot capture a bit-field by reference
                l;
                ^
/tmp/a.cc:8:6: note: 'l' declared here
        int l = s.s;
            ^
/tmp/a.cc:3:6: note: bit-field is declared here
        int s : 4;
            ^
1 error generated.

in the commit message

In addition, capturing an anonymous union member,
a bitfield, or a structured binding thereof now has a
better diagnostic.

"better diagnostic" implies there was already a diagnostic, but we're seeing new errors pop up. is this just a not precise commit message or are new errors related to capturing bitfields by reference unintentional?

New errors when capturing bit fields are probably*not* intentional. You have some of the problematic code i could look at? (not that the patch was reverted and then re-landed to fix a bug there)
And... the commit message is a bit inaccurate as the change that was supposedly improving diagnostics for reference to bit fields was remove (and i forgot to update the commit message accordingly)

reduced, this does seem wrong

$ cat /tmp/a.cc
struct S {
        int s : 4;
};

void f() {
        S s;
        int l = s.s;
        auto a = [&]() {
                l;
        };
}
$ clang++ -fsyntax-only /tmp/a.cc
/tmp/a.cc:10:3: error: cannot capture a bit-field by reference
                l;
                ^
/tmp/a.cc:8:6: note: 'l' declared here
        int l = s.s;
            ^
/tmp/a.cc:3:6: note: bit-field is declared here
        int s : 4;
            ^
1 error generated.

Thanks a lot for that, I confirmed the issue.
I will land a fix shortly

I landed https://github.com/llvm/llvm-project/commit/a1a71b7dc97b35133425a31ede90c40529f1be9e - which reverts entierely the bitfields related changes. It should fix the build issue you are encountering.
There are still pre-existing issues with the capture of bit fields and unions that needs more investigation.