[analyzer] pr38668: RegionStore: Do not attempt to cast loaded values of non-scalar types.

[NoQ]

Summary

It is expected to have the same object (memory region) treated as if it has different types in different program points. Like, the object may be a union or contain a union, or a pointer to it can be reinterpreted as a pointer to a different type. The correct behavior for RegionStore when an object is stored as an object of type T1 but loaded as an object of type T2 is to store the object as if it has type T1 but cast it to T2 during load. This is what CastRetrievedVal is responsible for.

Note that the cast here is some sort of a "reinterpret_cast" (even in C). For instance, if you store a float and load an integer, you won't have your float rounded to an integer; instead, you will have garbage.

Therefore i propose to admit that we cannot perform the cast as long as types we're dealing with are non-trivial (neither integers, nor pointers). Our modeling would still be weird in this case when dealing with integer casts, but so is support for integer casts in general.

Of course, if the cast is not necessary (eg, T1 == T2), we can still load the value just fine. This trivial observation in fact improves our behavior on tests and addresses some old FIXMEs: previously we would have tried to perform the cast and fail. We should probably also teach SValBuilder to perform casts in this case.

Fixes https://bugs.llvm.org/show_bug.cgi?id=38668

[dcoughlin]

These seems reasonable, although it does also seem like there could be quite a few unintended consequences that we haven't discovered yet.

I'm also a bit worried about the change in the analyzer's behavior on copy(). Do you have a sense of how much of an effect this will have and how easy potential false positives from this will be to suppress?

test/Analysis/bstring.cpp

47 ↗

(On Diff #178826)

This seems like it will be a big analysis policy change from the user's perspective and is likely to generate a bunch of new reports.

Can the user add an assertion that v.size() > 0 to tell the analyzer that the path on which the vector is empty is not feasible?

What are the diagnostic notes look like? Can the user tell that the the analyzer is assuming that begin() == end() on that path?

[NoQ]

test/Analysis/bstring.cpp

47 ↗

(On Diff #178826)

Ugh! This change is actually due to D55873, i just didn't run the tests properly. Will re-think :o

[NoQ]

Remove the test change that wasn't caused by this patch.

[dcoughlin]

Looks good to me.

[NoQ]

Revert!

lib/StaticAnalyzer/Core/Store.cpp

410 ↗

(On Diff #178984)

This is entirely incorrect. The whole point of this function is to handle the case when R->getValueType() has nothing to do with the original type of V.

Unfortunately, "type of an SVal" is not a thing, so it's going to be a bit more verbose.

[NoQ]

lib/StaticAnalyzer/Core/Store.cpp

410 ↗

(On Diff #178984)

Relevant test case:

double no_crash_reinterpret_double_as_int(double a) {
  *(int *)&a = 1;
  return a * a;
}

[NoQ]

lib/StaticAnalyzer/Core/Store.cpp

410 ↗

(On Diff #178984)

...which crashes after this patch while trying to multiply 1 by 1 and return result as double.

(sry for the noise)

[NoQ]

Attempt to define the notion of "type of SVal".

[NoQ]

Add a few more tests, just in case.

[NoQ]

I'll test this more thoroughly. In case it's still wrong, here's a safer fix.

[NoQ]

Add test case bool_to_nullptr in casts.cpp on which my second attempt crashes but the current code does not.

[NoQ]

// no-crash and a slightly cleaner test.

[george.karpenkov]

Seems good, but a comment explaining why this is necessary and why the crash follows otherwise would be great.

[NoQ]

Add a comment.

I guess it doesn't really matter why does this lead to a crash. The symbol itself is well-formed, but we probably don't support it yet in some place, and hopefully (but not necessarily) CastRetrievedVal is the only place we produce it. The point here is that the old behavior is clearly incorrect (i.e., the code doesn't do the right thing, and due to that the well-formed symbol is in fact not the symbol that we're looking for) and the new behavior is clearly conservative (i.e., returning UnknownVal should be pretty safe).

What we really need is a more direct test. Probably unit tests for SValBuilder, or some of those "denote - express" tests.

[NoQ]

Add a denote - express test.

cfe/trunk/lib/StaticAnalyzer/Checkers/ExprInspectionChecker.cpp

void ExprInspectionChecker::analyzerDenote(const CallExpr *CE,
   }
 
   SymbolRef Sym = C.getSVal(CE->getArg(0)).getAsSymbol();
   if (!Sym) {
     reportBug("Not a symbol", C);
     return;
   }
 
-  if (!isa<SymbolData>(Sym)) {
-    reportBug("Not an atomic symbol", C);
-    return;
-  }
-
   const auto *E = dyn_cast<StringLiteral>(CE->getArg(1)->IgnoreParenCasts());
   if (!E) {
     reportBug("Not a string literal", C);
     return;
   }
 
   ProgramStateRef State = C.getState();
 
   C.addTransition(C.getState()->set<DenotedSymbols>(Sym, E));
 }
 
 namespace {
 class SymbolExpressor
     : public SymExprVisitor<SymbolExpressor, Optional<std::string>> {
   ProgramStateRef State;
 
 public:
   SymbolExpressor(ProgramStateRef State) : State(State) {}
 
-  Optional<std::string> VisitSymExpr(const SymExpr *S) {
+  Optional<std::string> lookup(const SymExpr *S) {
     if (const StringLiteral *const *SLPtr = State->get<DenotedSymbols>(S)) {
       const StringLiteral *SL = *SLPtr;
       return std::string(SL->getBytes());
     }
     return None;
   }
 
+  Optional<std::string> VisitSymExpr(const SymExpr *S) {
+    return lookup(S);
+  }
+
   Optional<std::string> VisitSymIntExpr(const SymIntExpr *S) {
-    if (auto Str = Visit(S->getLHS()))
+    if (Optional<std::string> Str = lookup(S))
+      return Str;
+    if (Optional<std::string> Str = Visit(S->getLHS()))
       return (*Str + " " + BinaryOperator::getOpcodeStr(S->getOpcode()) + " " +
               std::to_string(S->getRHS().getLimitedValue()) +
               (S->getRHS().isUnsigned() ? "U" : ""))
           .str();
     return None;
   }
 
   Optional<std::string> VisitSymSymExpr(const SymSymExpr *S) {
-    if (auto Str1 = Visit(S->getLHS()))
-      if (auto Str2 = Visit(S->getRHS()))
+    if (Optional<std::string> Str = lookup(S))
+      return Str;
+    if (Optional<std::string> Str1 = Visit(S->getLHS()))
+      if (Optional<std::string> Str2 = Visit(S->getRHS()))
         return (*Str1 + " " + BinaryOperator::getOpcodeStr(S->getOpcode()) +
                 " " + *Str2).str();
     return None;
   }
+
+  Optional<std::string> VisitSymbolCast(const SymbolCast *S) {
+    if (Optional<std::string> Str = lookup(S))
+      return Str;
+    if (Optional<std::string> Str = Visit(S->getOperand()))
+      return (Twine("(") + S->getType().getAsString() + ")" + *Str).str();
+    return None;
+  }
 };
 } // namespace
 
 void ExprInspectionChecker::analyzerExpress(const CallExpr *CE,
                                             CheckerContext &C) const {
   if (CE->getNumArgs() == 0) {
     reportBug("clang_analyzer_express() requires a symbol", C);
     return;

cfe/trunk/lib/StaticAnalyzer/Core/Store.cpp

 /// CastRetrievedVal - Used by subclasses of StoreManager to implement
 ///  implicit casts that arise from loads from regions that are reinterpreted
 ///  as another region.
 SVal StoreManager::CastRetrievedVal(SVal V, const TypedValueRegion *R,
                                     QualType castTy) {
   if (castTy.isNull() || V.isUnknownOrUndef())
     return V;
 
+  // The dispatchCast() call below would convert the int into a float.
+  // What we want, however, is a bit-by-bit reinterpretation of the int
+  // as a float, which usually yields nothing garbage. For now skip casts
+  // from ints to floats.
+  // TODO: What other combinations of types are affected?
+  if (castTy->isFloatingType()) {
+    SymbolRef Sym = V.getAsSymbol();
+    if (Sym && !Sym->getType()->isFloatingType())
+      return UnknownVal();
+  }
+
   // When retrieving symbolic pointer and expecting a non-void pointer,
   // wrap them into element regions of the expected type if necessary.
   // SValBuilder::dispatchCast() doesn't do that, but it is necessary to
   // make sure that the retrieved value makes sense, because there's no other
   // cast in the AST that would tell us to cast it to the correct pointer type.
   // We might need to do that for non-void pointers as well.
   // FIXME: We really need a single good function to perform casts for us
   // correctly every time we need it.

cfe/trunk/test/Analysis/casts.c

void no_crash_on_symsym_cast_to_long() {
   #ifdef BIT32
   // expected-warning@-2{{The result of the left shift is undefined due to shifting by '48', which is greater or equal to the width of type 'long'}}
   #else
   // expected-no-diagnostics
   #endif
 }
 
 #endif
+
+char no_crash_SymbolCast_of_float_type_aux(int *p) {
+  *p += 1;
+  return *p;
+}
+
+void no_crash_SymbolCast_of_float_type() {
+  extern float x;
+  char (*f)() = no_crash_SymbolCast_of_float_type_aux;
+  f(&x);
+}
+
+double no_crash_reinterpret_double_as_int(double a) {
+  *(int *)&a = 1;
+  return a * a;
+}
+
+double no_crash_reinterpret_double_as_ptr(double a) {
+  *(void **)&a = 0;
+  return a * a;
+}
+
+double no_crash_reinterpret_double_as_sym_int(double a, int b) {
+  *(int *)&a = b;
+  return a * a;
+}
+
+double no_crash_reinterpret_double_as_sym_ptr(double a, void * b) {
+  *(void **)&a = b;
+  return a * a;
+}
+

cfe/trunk/test/Analysis/casts.cpp

 void foo(OpaqueRef ORef) {
   castToDerived(reinterpret_cast<Transparent *>(ORef))->getNotInt();
 }
 
 void foo(VeryOpaqueRef ORef) {
   castToDerived(reinterpret_cast<Transparent *>(ORef))->getNotInt();
 }
 } // namespace base_to_derived_opaque_class
+
+namespace bool_to_nullptr {
+struct S {
+  int *a[1];
+  bool b;
+};
+void foo(S s) {
+  s.b = true;
+  for (int i = 0; i < 2; ++i)
+    (void)(s.a[i] != nullptr); // no-crash
+}
+} // namespace bool_to_nullptr

cfe/trunk/test/Analysis/expr-inspection.cpp

void foo(int x, unsigned y) {
 
   clang_analyzer_denote(x, "$x");
   clang_analyzer_denote(y, "$y");
   clang_analyzer_express(x + y); // expected-warning{{$x + $y}}
 
   clang_analyzer_denote(1, "$z");     // expected-warning{{Not a symbol}}
   clang_analyzer_express(1);     // expected-warning{{Not a symbol}}
 
-  clang_analyzer_denote(x + 1, "$w"); // expected-warning{{Not an atomic symbol}}
-  clang_analyzer_express(x + 1); // expected-warning{{$x + 1}}
+  clang_analyzer_denote(x + 1, "$w");
+  clang_analyzer_express(x + 1); // expected-warning{{$w}}
   clang_analyzer_express(y + 1); // expected-warning{{$y + 1U}}
 }

cfe/trunk/test/Analysis/svalbuilder-float-cast.c

+// RUN: %clang_analyze_cc1 -analyzer-checker debug.ExprInspection -verify %s
+void clang_analyzer_denote(int, const char *);
+void clang_analyzer_express(int);
+
+void SymbolCast_of_float_type_aux(int *p) {
+  *p += 0;
+  // FIXME: Ideally, all unknown values should be symbolicated.
+  clang_analyzer_denote(*p, "$x"); // expected-warning{{Not a symbol}}
+
+  *p += 1;
+  // This should NOT be (float)$x + 1. Symbol $x was never casted to float.
+  // FIXME: Ideally, this should be $x + 1.
+  clang_analyzer_express(*p); // expected-warning{{Not a symbol}}
+}
+
+void SymbolCast_of_float_type() {
+  extern float x;
+  void (*f)() = SymbolCast_of_float_type_aux;
+  f(&x);
+}