[clang] [analyzer] Resolve lambda captures for explicit object parameters (PR #219726)

[Benedek Kaibas via cfe-commits]

================
@@ -3016,43 +3018,62 @@ void ExprEngine::processSwitch(const SwitchStmt *Switch, ExplodedNode *Pred,
 // Transfer functions: Loads and stores.
 //===----------------------------------------------------------------------===//
 
-void ExprEngine::VisitCommonDeclRefExpr(const Expr *Ex, const NamedDecl *D,
-                                        ExplodedNode *Pred,
-                                        ExplodedNodeSet &Dst) {
+std::optional<std::pair<SVal, QualType>>
+ExprEngine::resolveAsLambdaCapturedVar(const Expr *Ex, const ValueDecl *VD,
+                                       ExplodedNode *Pred) {
   ProgramStateRef state = Pred->getState();
   const StackFrame *SF = Pred->getStackFrame();
 
-  auto resolveAsLambdaCapturedVar =
-      [&](const ValueDecl *VD) -> std::optional<std::pair<SVal, QualType>> {
-    const auto *MD = dyn_cast<CXXMethodDecl>(SF->getDecl());
-    const auto *DeclRefEx = dyn_cast<DeclRefExpr>(Ex);
-    if (AMgr.options.ShouldInlineLambdas && DeclRefEx &&
-        DeclRefEx->refersToEnclosingVariableOrCapture() && MD &&
-        MD->getParent()->isLambda()) {
-      // Lookup the field of the lambda.
-      const CXXRecordDecl *CXXRec = MD->getParent();
-      llvm::DenseMap<const ValueDecl *, FieldDecl *> LambdaCaptureFields;
-      FieldDecl *LambdaThisCaptureField;
-      CXXRec->getCaptureFields(LambdaCaptureFields, LambdaThisCaptureField);
-
-      // Sema follows a sequence of complex rules to determine whether the
-      // variable should be captured.
-      if (const FieldDecl *FD = LambdaCaptureFields[VD]) {
+  const auto *MD = dyn_cast<CXXMethodDecl>(SF->getDecl());
+  const auto *DeclRefEx = dyn_cast<DeclRefExpr>(Ex);
+  if (AMgr.options.ShouldInlineLambdas && DeclRefEx &&
+      DeclRefEx->refersToEnclosingVariableOrCapture() && MD &&
+      MD->getParent()->isLambda()) {
+    // Lookup the field of the lambda.
+    const CXXRecordDecl *CXXRec = MD->getParent();
+    llvm::DenseMap<const ValueDecl *, FieldDecl *> LambdaCaptureFields;
+    FieldDecl *LambdaThisCaptureField;
+    CXXRec->getCaptureFields(LambdaCaptureFields, LambdaThisCaptureField);
+
+    // Sema follows a sequence of complex rules to determine whether the
+    // variable should be captured.
+    if (const FieldDecl *FD = LambdaCaptureFields[VD]) {
+      if (MD->isImplicitObjectMemberFunction()) {
         Loc CXXThis = svalBuilder.getCXXThis(MD, SF);
         SVal CXXThisVal = state->getSVal(CXXThis);
-        return std::make_pair(state->getLValue(FD, CXXThisVal), FD->getType());
+        return {{state->getLValue(FD, CXXThisVal), FD->getType()}};
+      }
+      const ParmVarDecl *PVD = MD->getParamDecl(0);
+      if (const Expr *CallSite = SF->getCallSite()) {
+        const ParamVarRegion *PVR =
+            MRMgr.getParamVarRegion(CallSite, /*Index=*/0, SF);
+        const Expr *SelfArgExpr = cast<CallExpr>(CallSite)->getArg(0);
+        if (PVD->getType()->isReferenceType()) {
+          state =
+              state->bindLoc(loc::MemRegionVal(PVR),
+                             state->getSVal(SelfArgExpr, SF->getParent()), SF);
+          SVal ParamSVal = state->getSVal(loc::MemRegionVal(PVR));
----------------
benedekaibas wrote:

> why are you discarding it

My goal was to keep the original return type/signature of the lambda which was returning ` std::optional<std::pair<SVal, QualType>>` and I kept this as well after moving the original `resolveAsLambdaCapturedVar` lambda into its own dedicated function. But you are right that it should not be discarded, but I was thinking that it might be out of scope of this PR. 

> Wouldn't it be more accurate to use a state that contains this binding during the rest of the analysis as well? 

Yes, that can definitely work and I think it would make more sense keeping the binding throught the new state and using that (the new state) during the rest of the analysis. This would require to update the dedicated function `resolveAsLambdaCapturedVar`. 

>Also, in this case, is this the most natural place to add this binding to the state?

The `getCXXThis` does not handle explicit object parameters, so while the binding is recorded in `CXXInstanceCall::getInitialStackFrameContents` that is only recorded for the implicit object parameters.

I think the fix as of now is instead of discarding the new state, the new state is what should be consumed for the rest of the analysis. For the implicit object parameter case the binding is recorded at the call entry. I have traced back and the binding gets recorded in `CXXInstanceCall::getInitialStackFrameContents`. What if I would do something close to this and record the binding for the explicit object parameter case at the call entry as well?

> Was this value written to that memory location just now? Or was it already written there by some earlier step (which is then not modeled properly)

The binding was not modeled earlier. I just created the binding now to resolve the issue.

https://github.com/llvm/llvm-project/pull/219726