Skip to content

[Clang] Check for uninitialized use in lambda within CXXOperatorCallExpr #129198

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.

Sign up for GitHub

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

Already on GitHub? Sign in to your account

Merged
merged 3 commits into from
Mar 7, 2025
Merged

[Clang] Check for uninitialized use in lambda within CXXOperatorCallExpr #129198

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
6667d8b
[Clang] Check for uninitialized use in lambda within CXXOperatorCallExpr
zhaohuiw42 Feb 28, 2025
83df149
Use dyn_cast_if_present instead
zhaohuiw42 Mar 4, 2025
bd6e64f
add test case about explicitly capturing variable
zhaohuiw42 Mar 4, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions clang/docs/ReleaseNotes.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -255,6 +255,7 @@ Bug Fixes to C++ Support
Bug Fixes to AST Handling
^^^^^^^^^^^^^^^^^^^^^^^^^
- Fixed type checking when a statement expression ends in an l-value of atomic type. (#GH106576)
- Fixed uninitialized use check in a lambda within CXXOperatorCallExpr. (#GH129198)

Miscellaneous Bug Fixes
^^^^^^^^^^^^^^^^^^^^^^^
Expand Down
17 changes: 17 additions & 0 deletions clang/lib/Sema/SemaDecl.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -60,6 +60,7 @@
#include "llvm/ADT/STLForwardCompat.h"
#include "llvm/ADT/SmallString.h"
#include "llvm/ADT/StringExtras.h"
#include "llvm/Support/SaveAndRestore.h"
#include "llvm/TargetParser/Triple.h"
#include <algorithm>
#include <cstring>
Expand Down Expand Up @@ -12597,6 +12598,7 @@ namespace {
bool isRecordType;
bool isPODType;
bool isReferenceType;
bool isInCXXOperatorCall;

bool isInitList;
llvm::SmallVector<unsigned, 4> InitFieldIndex;
Expand All @@ -12609,6 +12611,7 @@ namespace {
isPODType = false;
isRecordType = false;
isReferenceType = false;
isInCXXOperatorCall = false;
Copy link
Collaborator

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.

Copy link
Contributor Author

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... ?

isInitList = false;
if (ValueDecl *VD = dyn_cast<ValueDecl>(OrigDecl)) {
isPODType = VD->getType().isPODType(S.Context);
Expand Down Expand Up @@ -12796,6 +12799,7 @@ namespace {
}

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

Choose a reason for hiding this comment

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

We should be using bugprone-argument-comment style for all literal arguments like this.

Expr *Callee = E->getCallee();

if (isa<UnresolvedLookupExpr>(Callee))
Expand All @@ -12806,6 +12810,19 @@ namespace {
HandleValue(Arg->IgnoreParenImpCasts());
}

void VisitLambdaExpr(LambdaExpr *E) {
if (!isInCXXOperatorCall) {
Inherited::VisitLambdaExpr(E);
return;
}

for (Expr *Init : E->capture_inits())
if (DeclRefExpr *DRE = dyn_cast_if_present<DeclRefExpr>(Init))
HandleDeclRefExpr(DRE);
else if (Init)
Visit(Init);
}

void VisitUnaryOperator(UnaryOperator *E) {
// For POD record types, addresses of its own members are well-defined.
if (E->getOpcode() == UO_AddrOf && isRecordType &&
Expand Down
5 changes: 5 additions & 0 deletions clang/test/SemaCXX/uninitialized.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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}}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do we now also catch this case: https://godbolt.org/z/bnYEqhqY9

Copy link
Contributor Author

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.

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
A a7 = [&a7] { return a7; }(); // expected-warning{{variable 'a7' is uninitialized when used within its own initialization}}
}
}

Expand Down