[analyzer] Fix the assertion failure when static globals are used in lambda by reference

[george.karpenkov]

Summary

Also use the opportunity to clean up the code and remove unnecessary duplication.

[xazax.hun]

LGTM!

I am only wondering what is the policy regarding the tests? When should we add a new file or when should we just extend an existing one?

[george.karpenkov]

I am only wondering what is the policy regarding the tests?

@xazax.hun I don't think we have any, which is indeed often confusing. Which file did you have in mind?

[xazax.hun]

I was thinking about test/Analysis/lambdas.cpp as a possible candidate.

[NoQ]

I am only wondering what is the policy regarding the tests?

There doesn't seem to be any yet. We often have to create new files when we're disappointed with the run-line(s) of the existing files or want to use global declarations that conflict with those in existing files.

lib/StaticAnalyzer/Core/ExprEngine.cpp

   if (const auto *VD = dyn_cast<VarDecl>(D)) {
     // C permits "extern void v", and if you cast the address to a valid type,
     // you can even do things with it. We simply pretend
     assert(Ex->isGLValue() || VD->getType()->isVoidType());
     const LocationContext *LocCtxt = Pred->getLocationContext();
     const Decl *D = LocCtxt->getDecl();
     const auto *MD = D ? dyn_cast<CXXMethodDecl>(D) : nullptr;
     const auto *DeclRefEx = dyn_cast<DeclRefExpr>(Ex);
-    SVal V;
-    bool IsReference;
+    Optional<std::pair<SVal, QualType>> VInfo;
+
     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 VarDecl *, FieldDecl *> LambdaCaptureFields;
       FieldDecl *LambdaThisCaptureField;
       CXXRec->getCaptureFields(LambdaCaptureFields, LambdaThisCaptureField);
-      const FieldDecl *FD = LambdaCaptureFields[VD];
-      if (!FD) {
-        // When a constant is captured, sometimes no corresponding field is
-        // created in the lambda object.
-        assert(VD->getType().isConstQualified());
-        V = state->getLValue(VD, LocCtxt);
-        IsReference = false;
-      } else {
+
+      // Sema follows a sequence of complex rules to determine whether the
+      // variable should be captured.
+      if (const FieldDecl *FD = LambdaCaptureFields[VD]) {
         Loc CXXThis =
             svalBuilder.getCXXThis(MD, LocCtxt->getCurrentStackFrame());
         SVal CXXThisVal = state->getSVal(CXXThis);
-        V = state->getLValue(FD, CXXThisVal);
-        IsReference = FD->getType()->isReferenceType();
+        VInfo = std::make_pair(state->getLValue(FD, CXXThisVal), FD->getType());
       }
-    } else {
-      V = state->getLValue(VD, LocCtxt);
-      IsReference = VD->getType()->isReferenceType();
     }
 
+    if (!VInfo)
+      VInfo = std::make_pair(state->getLValue(VD, LocCtxt), VD->getType());
+
+    SVal V = VInfo->first;
+    bool IsReference = VInfo->second->isReferenceType();
+
     // For references, the 'lvalue' is the pointer address stored in the
     // reference region.
     if (IsReference) {
       if (const MemRegion *R = V.getAsRegion())
         V = state->getSVal(R);
       else
         V = UnknownVal();
     }

test/Analysis/lambda_no_capture.cpp

+// RUN: %clang_analyze_cc1 -std=c++11 -analyzer-checker=core -verify %s
+
+// expected-no-diagnostics
+static int b = 0;
+
+void f() {
+  b = 0;
+  auto &bm = b;
+  [&] {
+    bm++;
+    bm++;
+  }();
+  if (bm != 2) {
+    int *y = 0;
+    int x = *y; // no-warning
+  }
+}