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

[Artem Dergachev via Phabricator via cfe-commits]

NoQ updated this revision to Diff 179379.
NoQ added a comment.

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.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D55875/new/

https://reviews.llvm.org/D55875

Files:
  lib/StaticAnalyzer/Core/Store.cpp
  test/Analysis/casts.c
  test/Analysis/casts.cpp


Index: test/Analysis/casts.cpp
===================================================================
--- test/Analysis/casts.cpp
+++ test/Analysis/casts.cpp
@@ -102,3 +102,15 @@
   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
Index: test/Analysis/casts.c
===================================================================
--- test/Analysis/casts.c
+++ test/Analysis/casts.c
@@ -213,3 +213,35 @@
 }
 
 #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;
+}
+
Index: lib/StaticAnalyzer/Core/Store.cpp
===================================================================
--- lib/StaticAnalyzer/Core/Store.cpp
+++ lib/StaticAnalyzer/Core/Store.cpp
@@ -402,6 +402,16 @@
   if (castTy.isNull() || V.isUnknownOrUndef())
     return V;
 
+  // The dispatchCast() call below would round the int to 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


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D55875.179379.patch
Type: text/x-patch
Size: 2270 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20181222/d022d4e8/attachment-0001.bin>