Page MenuHomePhabricator

[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

There are a very large number of changes, so older changes are hidden. Show Older Changes

@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.Thu, Jul 28, 8:09 AM
clang/include/clang/AST/Stmt.h
63–64

We usually keep these alphabetical.

clang/lib/AST/ExprCXX.cpp
1213–1215

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

clang/lib/AST/StmtPrinter.cpp
2167–2168
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
8973

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

clang/lib/Sema/ScopeInfo.cpp
223–224 ↗(On Diff #444330)

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
16405–16407

Is cast<> safe here?

18338
18541

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

18545

Can this return nullptr?

18548–18549
18553

Can this return nullptr?

18565
18760–18768
18761–18767
clang/lib/Sema/SemaInit.cpp
7851

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?

1716–1718
clang/lib/Sema/TreeTransform.h
12989
13174

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
1143
erichkeane added inline comments.Thu, Jul 28, 8:19 AM
clang/lib/AST/ExprCXX.cpp
1213–1215

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
14686–14687
cor3ntin updated this revision to Diff 448524.Thu, Jul 28, 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.Fri, Jul 29, 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.Fri, Jul 29, 1:26 AM
cor3ntin marked 7 inline comments as done.

Add comments

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

Using isa_and_nonnull looks like the best solution here

clang/lib/Sema/SemaDecl.cpp
14686–14687

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
16405–16407

Yes, I added a comment to make that clear

18338

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

18545

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

18553

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
1213–1215

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
18399

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.Fri, Jul 29, 7:51 AM
clang/lib/Sema/SemaExpr.cpp
18399

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.Fri, Jul 29, 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.Fri, Jul 29, 2:59 PM

Fix codegen test (Thanks Erich!)

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

Fix formatting

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

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.Tue, Aug 2, 9:40 PM
clang/lib/CodeGen/CGOpenMPRuntime.cpp
8973

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

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

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

clang/docs/ReleaseNotes.rst
111–112

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
8973

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
18324
clang/lib/Sema/SemaLambda.cpp
1207–1211
This revision is now accepted and ready to land.Wed, Aug 3, 9:35 AM
cor3ntin updated this revision to Diff 449690.Wed, Aug 3, 9:42 AM

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

cor3ntin updated this revision to Diff 449696.Wed, Aug 3, 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.Wed, Aug 3, 11:00 AM
This revision was automatically updated to reflect the committed changes.
cor3ntin reopened this revision.Wed, Aug 3, 1:53 PM
This revision is now accepted and ready to land.Wed, Aug 3, 1:53 PM
cor3ntin updated this revision to Diff 449780.Wed, Aug 3, 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.Thu, Aug 4, 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.