[analyzer] pr28449 - Move literal rvalue construction away from RegionStore.

[NoQ]

Summary

When binding string literal regions to char arrays, RegionStore's bindArray() method converts the string literals to their lazy compound values before binding. Bug https://llvm.org/bugs/show_bug.cgi?id=28449 shows that similar behavior is required for handling compound literals (brace initializers).

However, it seems illogical to me that RegionStore modifies the value it was asked to bind - it should be handled by the external code, and RegionStore's bind...() interface should perhaps only bind the given value to the given location, without improvising.

This patch conducts the necessary refactoring to avoid the issue. Now all compound values should be correct to begin with. The patch accidentally fixes the bug.

Additionally, this patch enables loading values from compound-literal regions - it was explicitly disabled, but the problem due to which it was disabled seems already resolved; made the respective tests stronger to see that it's actually working correctly.

Additionally, this patch tweaks the dump method of compound literal regions, addressing a FIXME there and making things prettier.

Didn't notice significant changes on large codebase runs - no new crashes, no changes in positives.

[NoQ]

lib/StaticAnalyzer/Core/ExprEngineC.cpp

669

Whoops, was a note to myself, didn't mean to leave it here, sorry. Will actually have a look and update.

[dcoughlin]

I agree that it is weird that region store modifies the value it was asked to bind and over all this seems like the right approach.

My big concern with this patch is that the logic that looks whether an lval is being stored into a non-reference location and dereferences it seems overly broad. I'm worried that this could paper over other issues by "fixing up" mismatches that are really the result of some other bug. Is there a way to make that logic more targeted? For example, if it only applies to initializing char arrays with string literals, that is a much more precise check for when to load from the lvalue.

lib/StaticAnalyzer/Core/ExprEngineC.cpp

536

It seems like this comment ("We bound the temp obj region to the CXXConstructorExpr...") is incomplete/misleading (and has been for a while) because this code is executed when there is no constructor. Can it be updated to something more helpful?

665

I am surprised Sema wouldn't have already added an implicit cast with an lvalue to rvalue conversion when needed. It seems in many cases it already does. Can you be more explicit in your comments about why this is needed and under what situations Sema doesn't handle this logic? Is initializing an array with a string literal the only time this is needed? If so, can the check be more targeted? I'll note that InitListExpr has an isStringLiteralInit() method and that CodeGen has a special case for it.

669

It is probably also worth looking into what it would take to support unions here.

673

If you are going to iterate through the initializer expressions in reverse, you also have to iterate through the field decls in reverse. Also, I think it is worth commenting why you iterate in reverse order here.

679

This logic for checking with an expression is an lvalue and whether the type of the storage location is reference and loading from the lvalue if not is duplicated 3 times in this patch. Can you factor it out and give an meaningful name?

696

I think it would good to add a test with array fillers if we don't already have them. For example:

int a[2000] = {1, 2, 3};
clang_analyzer_eval(a[0] == 1);
clang_analyzer_eval(a[1999] == 0);

699

Should there be an assert here to make sure we have enumerated all the cases? I'm worried about the 'etc'. How will we know if we have not considered a case?

[NoQ]

Because i didn't get back to this in a while, and similar crashes keep coming, i decided to leave this refactoring as a FIXME.

[dcoughlin]

OK. Seems reasonable!

lib/StaticAnalyzer/Core/ExprEngine.cpp

if (auto *CtorExpr = findDirectConstructorForCurrentCFGElement()) {
         if (InitVal.isUnknownOrUndef()) {
           SValBuilder &SVB = getSValBuilder();
           InitVal = SVB.conjureSymbolVal(BMI->getInit(), stackFrame,
                                          Field->getType(),
                                          currBldrCtx->blockCount());
         }
       } else {
         InitVal = State->getSVal(BMI->getInit(), stackFrame);
+
+        // If the initializer is an lvalue, and the field is not a reference,
+        // then the initializer's rvalue is actually being copied,
+        // and therefore we need to do an extra getSVal()-dereference.
+        const Expr *ExprForReasoningIfLValue = BMI->getInit();
+        if (const CXXDefaultInitExpr *DIE =
+                dyn_cast<CXXDefaultInitExpr>(ExprForReasoningIfLValue))
+          ExprForReasoningIfLValue = DIE->getExpr();
+
+        if (ExprForReasoningIfLValue->isLValue() &&
+            !InitVal.isUnknownOrUndef() &&
+            !Field->getType()->isReferenceType()) {
+          InitVal = State->getSVal(InitVal.castAs<Loc>(), Field->getType());
+        }
       }
 
       assert(Tmp.size() == 1 && "have not generated any new nodes yet");
       assert(*Tmp.begin() == Pred && "have not generated any new nodes yet");
       Tmp.clear();
 
       PostInitializer PP(BMI, FieldLoc.getAsRegion(), stackFrame);
       evalBind(Tmp, Init, Pred, FieldLoc, InitVal, /*isInit=*/true, &PP);

lib/StaticAnalyzer/Core/ExprEngineC.cpp

if (const Expr *InitEx = VD->getInit()) {
       assert(DS->isSingleDecl());
       if (auto *CtorExpr = findDirectConstructorForCurrentCFGElement()) {
         assert(InitEx->IgnoreImplicit() == CtorExpr);
         (void)CtorExpr;
         // We constructed the object directly in the variable.
         // No need to bind anything.
         B.generateNode(DS, UpdatedN, state);
       } else {
         // We bound the temp obj region to the CXXConstructExpr. Now recover
         // the lazy compound value when the variable is not a reference.
-        if (AMgr.getLangOpts().CPlusPlus && VD->getType()->isRecordType() &&
-            !VD->getType()->isReferenceType()) {
-          if (Optional<loc::MemRegionVal> M =
-                  InitVal.getAs<loc::MemRegionVal>()) {
-            InitVal = state->getSVal(M->getRegion());
-            assert(InitVal.getAs<nonloc::LazyCompoundVal>());
-          }
-        }
+        if (InitEx->isLValue() && !InitVal.isUnknownOrUndef() &&
+            !VD->getType()->isReferenceType())
+          InitVal = state->getSVal(InitVal.castAs<Loc>(), InitEx->getType());
 
         // Recover some path-sensitivity if a scalar value evaluated to
         // UnknownVal.
         if (InitVal.isUnknown()) {
           QualType Ty = InitEx->getType();
           if (InitEx->isGLValue()) {
             Ty = getContext().getPointerType(Ty);
           }
if (!IE->isGLValue() &&
     // Handle base case where the initializer has no elements.
     // e.g: static int* myArray[] = {};
     if (NumInitElements == 0) {
       SVal V = svalBuilder.makeCompoundVal(T, vals);
       B.generateNode(IE, Pred, state->BindExpr(IE, LCtx, V));
       return;
     }
 
-    for (InitListExpr::const_reverse_iterator it = IE->rbegin(),
-         ei = IE->rend(); it != ei; ++it) {
-      SVal V = state->getSVal(cast<Expr>(*it), LCtx);
+    // We need to recover the rvalue of the initializer list's element in case
+    // the value it's going to be bound to is not a reference.
+    // The AST represents it by making the initializer list have the correct
+    // type, as any expression. We need to read this type and make the
+    // necessary conversions.
+    if (const RecordType *RT =
+            IE->getType().getCanonicalType()->getAs<RecordType>()) {
+      // For structures, check if the respective field is a reference.
+      // FIXME: What if fields mismatch?
+      const RecordDecl *RD = RT->getDecl();
+      assert(RD);
+      auto FieldI = RD->field_begin(), FieldE = RD->field_end();
+      for (auto InitI = IE->rbegin(), InitE = IE->rend();
+           InitI != InitE; ++InitI) {
+        if (FieldI == FieldE)
+          break;
+        const Expr *E = cast<Expr>(*InitI);
+        SVal V = state->getSVal(E, LCtx);
+        if (E->isLValue() && !V.isUnknownOrUndef() &&
+            !FieldI->getType()->isReferenceType())
+          V = state->getSVal(V.castAs<Loc>(), FieldI->getType());
+        vals = getBasicVals().consVals(V, vals);
+        ++FieldI;
+      }
+    } else if (const ArrayType *AT = getContext().getAsArrayType(
+                   IE->getType().getCanonicalType())) {
+      // There cannot be arrays of references. Every lvalue expression
+      // deserves to be dereferenced.
+      QualType ET = AT->getElementType();
+      for (auto InitI = IE->rbegin(), InitE = IE->rend();
+           InitI != InitE; ++InitI) {
+        const Expr *E = cast<Expr>(*InitI);
+        SVal V = state->getSVal(E, LCtx);
+        if (E->isLValue() && !V.isUnknownOrUndef())
+          V = state->getSVal(V.castAs<Loc>(), ET);
         vals = getBasicVals().consVals(V, vals);
       }
+    } else {
+      // Vector values, complex numbers, etc: No lvalues to expect.
+      for (auto InitI = IE->rbegin(), InitE = IE->rend();
+           InitI != InitE; ++InitI) {
+        SVal V = state->getSVal(cast<Expr>(*InitI), LCtx);
+        vals = getBasicVals().consVals(V, vals);
+      }
+    }
 
     B.generateNode(IE, Pred,
                    state->BindExpr(IE, LCtx,
                                    svalBuilder.makeCompoundVal(T, vals)));
     return;
   }
 
   // Handle scalars: int{5} and int{} and GLvalues.

lib/StaticAnalyzer/Core/MemRegion.cpp

for (BlockDataRegion::referenced_vars_iterator
          I = referenced_vars_begin(),
          E = referenced_vars_end(); I != E; ++I)
     os << "(" << I.getCapturedRegion() << "," <<
                  I.getOriginalRegion() << ") ";
   os << '}';
 }
 
 void CompoundLiteralRegion::dumpToStream(raw_ostream &os) const {
-  // FIXME: More elaborate pretty-printing.
-  os << "{ " << static_cast<const void*>(CL) <<  " }";
+  os << "{ ";
+  CL->printPretty(os, nullptr, PrintingPolicy(getContext().getLangOpts()));
+  os << " }";
 }
 
 void CXXTempObjectRegion::dumpToStream(raw_ostream &os) const {
   os << "temp_object{" << getValueType().getAsString() << ','
      << static_cast<const void*>(Ex) << '}';
 }
 
 void CXXBaseObjectRegion::dumpToStream(raw_ostream &os) const {

lib/StaticAnalyzer/Core/RegionStore.cpp

if (Result.second)
                                                             Result.second);
   }
 
   return Result;
 }
 
 SVal RegionStoreManager::getBindingForElement(RegionBindingsConstRef B,
                                               const ElementRegion* R) {
-  // We do not currently model bindings of the CompoundLiteralregion.
-  if (isa<CompoundLiteralRegion>(R->getBaseRegion()))
-    return UnknownVal();
-
   // Check if the region has a binding.
   if (const Optional<SVal> &V = B.getDirectBinding(R))
     return *V;
 
   const MemRegion* superR = R->getSuperRegion();
 
   // Check if the region is an element region of a string literal.
   if (const StringRegion *StrR=dyn_cast<StringRegion>(superR)) {
RegionStoreManager::bindArray(RegionBindingsConstRef B,
 
   const ArrayType *AT =cast<ArrayType>(Ctx.getCanonicalType(R->getValueType()));
   QualType ElementTy = AT->getElementType();
   Optional<uint64_t> Size;
 
   if (const ConstantArrayType* CAT = dyn_cast<ConstantArrayType>(AT))
     Size = CAT->getSize().getZExtValue();
 
-  // Check if the init expr is a string literal.
-  if (Optional<loc::MemRegionVal> MRV = Init.getAs<loc::MemRegionVal>()) {
-    const StringRegion *S = cast<StringRegion>(MRV->getRegion());
-
-    // Treat the string as a lazy compound value.
-    StoreRef store(B.asStore(), *this);
-    nonloc::LazyCompoundVal LCV = svalBuilder.makeLazyCompoundVal(store, S)
-        .castAs<nonloc::LazyCompoundVal>();
-    return bindAggregate(B, R, LCV);
-  }
-
   // Handle lazy compound values.
   if (Init.getAs<nonloc::LazyCompoundVal>())
     return bindAggregate(B, R, Init);
 
   // Remaining case: explicit compound values.
 
   if (Init.isUnknown())
     return setImplicitDefaultValue(B, R, ElementTy);

test/Analysis/compound-literals.c

+// RUN: %clang_cc1 -triple=i386-apple-darwin10 -analyze -analyzer-checker=debug.ExprInspection -verify %s
+void clang_analyzer_eval(int);
+
+// pr28449: Used to crash.
+void foo(void) {
+  static const unsigned short array[] = (const unsigned short[]){0x0F00};
+  clang_analyzer_eval(array[0] == 0x0F00); // expected-warning{{TRUE}}
+}

test/Analysis/region-store.c

 // RUN: %clang_cc1 -analyze -analyzer-checker=core,unix,debug.ExprInspection -verify %s
 
 int printf(const char *restrict,...);
+void clang_analyzer_eval();
 
 // Testing core functionality of the region store.
 // radar://10127782
 int compoundLiteralTest() {
     int index = 0;
     for (index = 0; index < 2; index++) {
         int thing = (int []){0, 1}[index];
         printf("thing: %i\n", thing);
+        clang_analyzer_eval(thing == index); // expected-warning{{TRUE}}
     }
     return 0;
 }
 
 int compoundLiteralTest2() {
     int index = 0;
     for (index = 0; index < 3; index++) {
         int thing = (int [][3]){{0,0,0}, {1,1,1}, {2,2,2}}[index][index];
         printf("thing: %i\n", thing);
+        clang_analyzer_eval(thing == index); // expected-warning{{TRUE}}
     }
     return 0;
 }
 
 int concreteOffsetBindingIsInvalidatedBySymbolicOffsetAssignment(int length,
                                                                  int i) {
   int values[length];
   values[i] = 4;