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

zhaohuiw42 · 2025-02-28T06:18:58Z

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; }();.

github-actions · 2025-02-28T06:19:16Z

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 · 2025-02-28T06:19:48Z

@llvm/pr-subscribers-clang

Author: zhaohui (zhaohuiw42)

Changes

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; }();.

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
   }
 }

zyn0217

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.

zyn0217 · 2025-02-28T08:01:07Z

clang/lib/Sema/SemaDecl.cpp

+    // 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.


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

Ok, just remove them.

zyn0217 · 2025-02-28T08:01:56Z

clang/lib/Sema/SemaDecl.cpp

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

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


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

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

Fixed, thanks.

zyn0217 · 2025-02-28T08:02:48Z

clang/lib/Sema/SemaDecl.cpp

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


nit:

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

I.e. prefer the early-return style

Changed to this style, thanks.

erichkeane · 2025-02-28T15:54:13Z

clang/lib/Sema/SemaDecl.cpp

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

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


Suggested change

llvm::SaveAndRestore cxxOpCallScope(isInCXXOperatorCall, true);

llvm::SaveAndRestore CxxOpCallScope(isInCXXOperatorCall, true);

erichkeane · 2025-02-28T15:57:04Z

clang/lib/Sema/SemaDecl.cpp

+        return;
+      }
+
+      for (const auto &init : E->capture_inits())


Suggested change

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

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

erichkeane · 2025-02-28T15:57:49Z

clang/lib/Sema/SemaDecl.cpp

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


Suggested change

else

else if (Init)

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

Fixed, and changed to use dyn_cast_or_null

clang/lib/Sema/SemaDecl.cpp

Co-authored-by: Yanzuo Liu <[email protected]>

zwuis · 2025-03-04T10:30:46Z

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

zhaohuiw42 · 2025-03-04T10:49:52Z

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

@zwuis Added, thanks.

github-actions · 2025-03-07T08:01:44Z

@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!

shafik

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.

shafik · 2025-03-08T00:41:59Z

clang/lib/Sema/SemaDecl.cpp

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


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

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

shafik · 2025-03-08T00:42:52Z

clang/lib/Sema/SemaDecl.cpp

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

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


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

shafik · 2025-03-08T00:48:48Z

clang/test/SemaCXX/uninitialized.cpp

@@ -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}}


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

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.

…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

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

zyn0217 reviewed Feb 28, 2025

View reviewed changes

zyn0217 requested review from cor3ntin, AaronBallman and erichkeane February 28, 2025 08:11

zhaohuiw42 force-pushed the fix-self-reference-checker branch from f51a5ba to 54c65ab Compare February 28, 2025 15:47

zhaohuiw42 requested a review from zyn0217 February 28, 2025 15:49

erichkeane reviewed Feb 28, 2025

View reviewed changes

[Clang] Check for uninitialized use in lambda within CXXOperatorCallExpr

6667d8b

zhaohuiw42 force-pushed the fix-self-reference-checker branch from 54c65ab to 6667d8b Compare March 1, 2025 05:16

zhaohuiw42 requested a review from erichkeane March 1, 2025 06:11

erichkeane approved these changes Mar 3, 2025

View reviewed changes

zwuis reviewed Mar 4, 2025

View reviewed changes

clang/lib/Sema/SemaDecl.cpp Outdated Show resolved Hide resolved

Use dyn_cast_if_present instead

83df149

Co-authored-by: Yanzuo Liu <[email protected]>

add test case about explicitly capturing variable

bd6e64f

zhaohuiw42 requested review from erichkeane and zwuis March 6, 2025 05:37

zwuis approved these changes Mar 6, 2025

View reviewed changes

erichkeane approved these changes Mar 6, 2025

View reviewed changes

zyn0217 merged commit 3a67c7c into llvm:main Mar 7, 2025
7 checks passed

shafik reviewed Mar 8, 2025

View reviewed changes

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

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

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

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

Uh oh!

Conversation

zhaohuiw42 commented Feb 28, 2025

Uh oh!

github-actions bot commented Feb 28, 2025

Uh oh!

llvmbot commented Feb 28, 2025

Uh oh!

zyn0217 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

zwuis commented Mar 4, 2025

Uh oh!

zhaohuiw42 commented Mar 4, 2025

Uh oh!

Uh oh!

github-actions bot commented Mar 7, 2025

Uh oh!

shafik left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!