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

[Donát Nagy 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));
----------------
NagyDonat wrote:

This code seems to be incorrect, because you compute a new, updated `state`, which is then discarded after one `getSVal` and one `getLValue` call.

This naturally raises the question that which `ProgramState` is the accurate representation of the state (in particular memory contents) that could be observed if the analyzed code was executed?
- If the accurate representation is the new, updated state (where the value `state->getSVal(SelfArgExpr, SF->getParent())` is stored at the location `loc::MemRegionVal(PVR)`), then why are you discarding it? Wouldn't it be more accurate to use a state that contains this binding during the rest of the analysis as well?
  - Also, in this case, is this the most natural place to add this binding to the state? 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)?
- If the accurate representation is the original `state`, then why do you define `ParamSVal` in this roundabout way instead of writing `SVal ParamSVal = state->getSVal(SelfArgExpr, SF->getParent());`?
  - Is it significant that the new, updated state participates in the `getLValue()` call? (I'd guess yes, but I'm not sure.)
  - Could you avoid creating the updated state (which – in this case – does not represent real behavior of the program) by calling `desugarReferece` etc. directly?



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