[Clang] Check for uninitialized use in lambda within CXXOperatorCallExpr by zhaohuiw42 · Pull Request #129198 · llvm/llvm-project (original) (raw)

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Conversation28 Commits3 Checks8 Files changed

Conversation

This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters

[ Show hidden characters]({{ revealButtonHref }})

Fix #128058.
Track whether a LambdaExpr is an immediate operand of a CXXOperatorCallExpr using a new flag, isInCXXOperatorCall. This enables special handling of capture initializations to detect uninitialized variable uses, such as in S s = [&]() { return s; }();.

Thank you for submitting a Pull Request (PR) to the LLVM Project!

This PR will be automatically labeled and the relevant teams will be notified.

If you wish to, you can add reviewers by using the "Reviewers" section on this page.

If this is not working for you, it is probably because you do not have write permissions for the repository. In which case you can instead tag reviewers by name in a comment by using @ followed by their GitHub username.

If you have received no comments on your PR for a week, you can request a review by "ping"ing the PR by adding a comment “Ping”. The common courtesy "ping" rate is once a week. Please remember that you are asking for valuable time from other developers.

If you have further questions, they may be answered by the LLVM GitHub User Guide.

You can also ask questions in a comment on this PR, on the LLVM Discord or on the forums.

llvmbot added clang

Clang issues not falling into any other category

clang:frontend

Language frontend issues, e.g. anything involving "Sema"

labels

Feb 28, 2025

@llvm/pr-subscribers-clang

Author: zhaohui (zhaohuiw42)

Changes

Full diff: https://github.com/llvm/llvm-project/pull/129198.diff

2 Files Affected:

(modified) clang/lib/Sema/SemaDecl.cpp (+22)
(modified) clang/test/SemaCXX/uninitialized.cpp (+4)

diff --git a/clang/lib/Sema/SemaDecl.cpp b/clang/lib/Sema/SemaDecl.cpp index 285bd27a35a76..ad93b4a858543 100644 --- a/clang/lib/Sema/SemaDecl.cpp +++ b/clang/lib/Sema/SemaDecl.cpp @@ -12597,6 +12597,12 @@ namespace { bool isRecordType; bool isPODType; bool isReferenceType;

// Tracks whether the current expression is being visited within a
// CXXOperatorCallExpr. This flag is set to true when entering a
// CXXOperatorCallExpr and reset to false upon exit. It is used to detect
// when a LambdaExpr is an operand of an operator call, enabling special
// handling of its capture initializations.
bool isInCXXOperatorCall;

bool isInitList; llvm::SmallVector<unsigned, 4> InitFieldIndex;

@@ -12609,6 +12615,7 @@ namespace { isPODType = false; isRecordType = false; isReferenceType = false;

 isInCXXOperatorCall = false;
 isInitList = false;
 if (ValueDecl *VD = dyn_cast<ValueDecl>(OrigDecl)) {
   isPODType = VD->getType().isPODType(S.Context);

@@ -12796,6 +12803,7 @@ namespace { }

 void VisitCXXOperatorCallExpr(CXXOperatorCallExpr *E) {

 isInCXXOperatorCall = true;
 Expr *Callee = E->getCallee();

 if (isa<UnresolvedLookupExpr>(Callee))

@@ -12804,6 +12812,20 @@ namespace { Visit(Callee); for (auto Arg: E->arguments()) HandleValue(Arg->IgnoreParenImpCasts());

```
 isInCXXOperatorCall = false;
```
}
void VisitLambdaExpr(LambdaExpr *E) {
```
 if (isInCXXOperatorCall) {
```

   for (const auto &init : E->capture_inits()) {

     if (DeclRefExpr *DRE = dyn_cast<DeclRefExpr>(init))

```
       HandleDeclRefExpr(DRE);
```
```
     else
```
```
       Visit(init);
```
```
   }
```
```
   return;
```
```
 }
```
```
 Inherited::VisitLambdaExpr(E);
```
}

void VisitUnaryOperator(UnaryOperator *E) {

diff --git a/clang/test/SemaCXX/uninitialized.cpp b/clang/test/SemaCXX/uninitialized.cpp index 4af2c998f082e..654d955b3cc72 100644 --- a/clang/test/SemaCXX/uninitialized.cpp +++ b/clang/test/SemaCXX/uninitialized.cpp @@ -892,6 +892,10 @@ namespace lambdas { return a1.x; }); A a2([&] { return a2.x; }); // ok

A a3([=]{ return a3.x; }()); // expected-warning{{variable 'a3' is uninitialized when used within its own initialization}}
A a4([&]{ return a4.x; }()); // expected-warning{{variable 'a4' is uninitialized when used within its own initialization}}
A a5([&]{ return a5; }()); // expected-warning{{variable 'a5' is uninitialized when used within its own initialization}}
A a6([&]{ return a5.x; }()); // ok

} }

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the contribution, and this makes sense to me. However I'd like others to take a look too.

Of course this should come with a release note, in clang/docs/ReleaseNotes.rst.

Comment on lines 12600 to 12604

// Tracks whether the current expression is being visited within a
// CXXOperatorCallExpr. This flag is set to true when entering a
// CXXOperatorCallExpr and reset to false upon exit. It is used to detect
// when a LambdaExpr is an operand of an operator call, enabling special
// handling of its capture initializations.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is a little chatty ... The name is already self-explanatory from what I see.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, just remove them.

@@ -12796,6 +12803,7 @@ namespace {
}

void VisitCXXOperatorCallExpr(CXXOperatorCallExpr *E) {
isInCXXOperatorCall = true;

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can use llvm::SaveAndRestore to avoid restoring it manually

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You have to, the early return on 12802/12810 means that this isn't going to be properly unset.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed, thanks.

}

void VisitLambdaExpr(LambdaExpr *E) {
if (isInCXXOperatorCall) {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit:

if (!isInCXXOperatorCall) return; // ...

I.e. prefer the early-return style

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changed to this style, thanks.

@@ -12796,6 +12799,7 @@ namespace {
}

void VisitCXXOperatorCallExpr(CXXOperatorCallExpr *E) {
llvm::SaveAndRestore cxxOpCallScope(isInCXXOperatorCall, true);

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

llvm::SaveAndRestore cxxOpCallScope(isInCXXOperatorCall, true);
llvm::SaveAndRestore CxxOpCallScope(isInCXXOperatorCall, true);

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed.

return;
}

for (const auto &init : E->capture_inits())

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

for (const auto &init : E->capture_inits())
for (const Expr *Init : E->capture_inits())

Note the not using auto per coding standard, and the capital 'I'.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fine, I'll pay attention to that in the future.

for (const auto &init : E->capture_inits())
if (DeclRefExpr *DRE = dyn_cast(init))
HandleDeclRefExpr(DRE);
else

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

According to the base implementation of this, we end up expecting this could be null sometimes.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed, and changed to use dyn_cast_or_null

Co-authored-by: Yanzuo Liu zwuis@outlook.com

Can you add tests about explicitly capturing variables? E.g. S s = [&s] { return s; }();

Can you add tests about explicitly capturing variables? E.g. S s = [&s] { return s; }();

@zwuis Added, thanks.

@zhaohuiw42 Congratulations on having your first Pull Request (PR) merged into the LLVM Project!

Your changes will be combined with recent changes from other authors, then tested by our build bots. If there is a problem with a build, you may receive a report in an email or a comment on this PR.

Please check whether problems have been caused by your change specifically, as the builds can include changes from many authors. It is not uncommon for your change to be included in a build that fails due to someone else's changes, or infrastructure issues.

How to do this, and the rest of the post-merge process, is covered in detail here.

If your change does cause a problem, it may be reverted, or you can revert it yourself. This is a normal part of LLVM development. You can fix your changes and open a new PR to merge them again.

If you don't get any reports, no action is required from you. Your changes are working as expected, well done!

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for the PR, some after commit review. The bugprone comment should be addressed and if there are more cases we catch that are not covered by existing tests, we should add more tests.

The in class member initializers feels like it would be worth doing but I would not insist on it.

@@ -12609,6 +12611,7 @@ namespace {
isPODType = false;
isRecordType = false;
isReferenceType = false;
isInCXXOperatorCall = false;

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It feels like changing this to use in class member initializers would have been a big win.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So I need to change all the other fields to this form as well, like isPODType, isRecordType... ?

@@ -12796,6 +12799,7 @@ namespace {
}

void VisitCXXOperatorCallExpr(CXXOperatorCallExpr *E) {
llvm::SaveAndRestore CxxOpCallScope(isInCXXOperatorCall, true);

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@@ -892,6 +892,11 @@ namespace lambdas {
return a1.x;
});
A a2([&] { return a2.x; }); // ok
A a3([=] { return a3.x; }()); // expected-warning{{variable 'a3' is uninitialized when used within its own initialization}}

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think not, SelfReferenceChecker only checks whether the declaration is evaluated in its own initialization, an additional value-tracking mechanism is needed for the case you mentioned.

jph-13 pushed a commit to jph-13/llvm-project that referenced this pull request

Mar 21, 2025

…xpr (llvm#129198)

Track whether a LambdaExpr is an immediate operand of a CXXOperatorCallExpr using a new flag, isInCXXOperatorCall. This enables special handling of capture initializations to detect uninitialized variable uses, such as in S s = [&]() { return s; }();.

Fix llvm#128058

Reviewers

shafik shafik left review comments

erichkeane erichkeane approved these changes

zwuis zwuis approved these changes

cor3ntin Awaiting requested review from cor3ntin

AaronBallman Awaiting requested review from AaronBallman

zyn0217 Awaiting requested review from zyn0217

Labels

clang:frontend

Language frontend issues, e.g. anything involving "Sema"

clang

Clang issues not falling into any other category