[analyzer] const init: handle non-explicit cases more accurately

[r.stahl]

Summary

If the access is out of bounds, return UndefinedVal. If it is missing an explicit init, return the implicit zero value it must have.

[NoQ]

Yay thanks!

I think some cornercases would need to be dealt with.

lib/StaticAnalyzer/Core/RegionStore.cpp

1650

What if we have an out-of-bounds access to a variable-length array? I don't think it'd yield zero.

1650–1652

Would this work correctly if the element is not of an integral or enumeration type? I think this needs an explicit check.

1733

Same: would this work correctly if the field is not of an integral or enumeration type?

test/Analysis/initialization.c

3

We try to avoid using dump() on tests because it makes tests test the dump syntax, which isn't the point.

For checking constants, it's easier to do something like clang_analyzer_eval(parr[i] == 2); // expected-warning{{TRUE}}.

For finding undefined values, you can enable core.uninitialized checkers and receive warnings when the argument of clang_analyzer_eval is an uninitialized value. Or just increment the value.

[r.stahl]

updated test

lib/StaticAnalyzer/Core/RegionStore.cpp

1650

I'm getting "variable-sized object may not be initialized", so this case should not be possible.

1650–1652

I'm having a hard time reproducing this either.

struct S {
  int a = 3;
};
S const sarr[2] = {};
void definit() {
  int i = 1;
  clang_analyzer_dump(sarr[i].a); // expected-warning{{test}}
}

results in a symbolic value, because makeZeroVal returns an empty SVal list for arrays, records, vectors and complex types. Otherwise it just returns UnknownVal (for example for not-yet-implemented floats).

Can you think of a case where this would be an issue?

[NoQ]

Looks good!

Do you have commit access? I think you should get commit access.

lib/StaticAnalyzer/Core/RegionStore.cpp

1650

Yup, sounds reasonable.

1650–1652

Had a look. This indeed looks fine, but for a completely different reason. In fact structs don't appear in getBindingForElement() because they all go through getBindingForStruct() instead. Hopefully this does take care of other cornercases.

So your test case doesn't even trigger your code to be executed, neither would S s = sarr[i]; clang_analyzer_dump(s.a);.

Still, as far as i understand, sarr here should be zero-initialized, so i think the symbolic value can be improved upon, so we might as well add this as a FIXME test.

[r.stahl]

Added FIXME tests.

I know my example didn't exercise your scenario, but it was the only one I could think of where returning zero would have been straight up wrong. Note that I default initialized a to 3 there, so sarr is not just zero-initialized.

I do not have access yet, but applied today!

lib/StaticAnalyzer/Core/RegionStore.cpp

if (const StringRegion *StrR = dyn_cast<StringRegion>(superR)) {
     const VarDecl *VD = VR->getDecl();
     // Either the array or the array element has to be const.
     if (VD->getType().isConstQualified() || R->getElementType().isConstQualified()) {
       if (const Expr *Init = VD->getInit()) {
         if (const auto *InitList = dyn_cast<InitListExpr>(Init)) {
           // The array index has to be known.
           if (auto CI = R->getIndex().getAs<nonloc::ConcreteInt>()) {
             int64_t i = CI->getValue().getSExtValue();
-            // Return unknown value if index is out of bounds.
-            if (i < 0 || i >= InitList->getNumInits())
-              return UnknownVal();
+            // If it is known that the index is out of bounds, we can return
+            // an undefined value.
+            if (i < 0)
+              return UndefinedVal();
+
+            if (auto CAT = Ctx.getAsConstantArrayType(VD->getType()))
+              if (CAT->getSize().sle(i))
+                return UndefinedVal();
+
+            // If there is a list, but no init, it must be zero.
+            if (i >= InitList->getNumInits())
+              return svalBuilder.makeZeroVal(R->getElementType());
 
             if (const Expr *ElemInit = InitList->getInit(i))
               if (Optional<SVal> V = svalBuilder.getConstantVal(ElemInit))
                 return *V;
           }
         }
       }
     }
SVal RegionStoreManager::getBindingForField(RegionBindingsConstRef B,
   const MemRegion* superR = R->getSuperRegion();
   if (const auto *VR = dyn_cast<VarRegion>(superR)) {
     const VarDecl *VD = VR->getDecl();
     QualType RecordVarTy = VD->getType();
     unsigned Index = FD->getFieldIndex();
     // Either the record variable or the field has to be const qualified.
     if (RecordVarTy.isConstQualified() || Ty.isConstQualified())
       if (const Expr *Init = VD->getInit())
-        if (const auto *InitList = dyn_cast<InitListExpr>(Init))
-          if (Index < InitList->getNumInits())
+        if (const auto *InitList = dyn_cast<InitListExpr>(Init)) {
+          if (Index < InitList->getNumInits()) {
             if (const Expr *FieldInit = InitList->getInit(Index))
               if (Optional<SVal> V = svalBuilder.getConstantVal(FieldInit))
                 return *V;
+          } else {
+            return svalBuilder.makeZeroVal(Ty);
+          }
+        }
   }
 
   return getBindingForFieldOrElementCommon(B, R, Ty);
 }
 
 Optional<SVal>
 RegionStoreManager::getBindingForDerivedDefaultValue(RegionBindingsConstRef B,
                                                      const MemRegion *superR,

test/Analysis/initialization.c

-// RUN: %clang_analyze_cc1 -analyzer-checker=core -verify %s
-// expected-no-diagnostics
+// RUN: %clang_cc1 -triple i386-apple-darwin10 -analyze -analyzer-checker=core.builtin,debug.ExprInspection -verify %s
+
+void clang_analyzer_dump(int);
 
 void initbug() {
   const union { float a; } u = {};
   (void)u.a; // no-crash
 }
+
+int const parr[2] = {1};
+void constarr() {
+  int i = 2;
+  clang_analyzer_dump(parr[i]); // expected-warning{{Undefined}}
+  i = 1;
+  clang_analyzer_dump(parr[i]); // expected-warning{{0 S32b}}
+  i = -1;
+  clang_analyzer_dump(parr[i]); // expected-warning{{Undefined}}
+}
+
+struct SM {
+  int a;
+  int b;
+};
+const struct SM sm = {.a = 1};
+void multinit() {
+  clang_analyzer_dump(sm.a); // expected-warning{{1 S32b}}
+  clang_analyzer_dump(sm.b); // expected-warning{{0 S32b}}
+}