NoQ created this revision.
NoQ added reviewers: dcoughlin, xazax.hun, a_sidorin, george.karpenkov,
rnkovacs, Szelethus, mikhail.ramalho, baloghadamsoftware.
Herald added subscribers: cfe-commits, dkrupp, donat.nagy, a.sidorin, szepet.
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
Repository:
rC Clang
https://reviews.llvm.org/D55875
Files:
lib/StaticAnalyzer/Core/Store.cpp
test/Analysis/bstring.cpp
test/Analysis/casts.c
test/Analysis/pointer-to-member.cpp
Index: test/Analysis/pointer-to-member.cpp
===================================================================
--- test/Analysis/pointer-to-member.cpp
+++ test/Analysis/pointer-to-member.cpp
@@ -253,11 +253,10 @@
clang_analyzer_eval(&A::y); // expected-warning{{TRUE}}
clang_analyzer_eval(&A::z); // expected-warning{{TRUE}}
- // FIXME: These should be true.
int A::*l = &A::x, A::*m = &A::y, A::*n = &A::z;
- clang_analyzer_eval(l); // expected-warning{{UNKNOWN}}
- clang_analyzer_eval(m); // expected-warning{{UNKNOWN}}
- clang_analyzer_eval(n); // expected-warning{{UNKNOWN}}
+ clang_analyzer_eval(l); // expected-warning{{TRUE}}
+ clang_analyzer_eval(m); // expected-warning{{TRUE}}
+ clang_analyzer_eval(n); // expected-warning{{TRUE}}
// FIXME: These should be true as well.
A a;
Index: test/Analysis/casts.c
===================================================================
--- test/Analysis/casts.c
+++ test/Analysis/casts.c
@@ -213,3 +213,14 @@
}
#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);
+}
Index: test/Analysis/bstring.cpp
===================================================================
--- test/Analysis/bstring.cpp
+++ test/Analysis/bstring.cpp
@@ -44,7 +44,9 @@
int i = buf[0];
+ // The TRUE warning shows up on the path on which the vector is empty.
clang_analyzer_eval(i == 66); // expected-warning {{UNKNOWN}}
+ // expected-warning@-1 {{TRUE}}
return buf;
}
@@ -60,7 +62,9 @@
int i = buf[0];
+ // The TRUE warning shows up on the path on which the vector is empty.
clang_analyzer_eval(i == 66); // expected-warning {{UNKNOWN}}
+ // expected-warning@-1 {{TRUE}}
return buf;
}
Index: lib/StaticAnalyzer/Core/Store.cpp
===================================================================
--- lib/StaticAnalyzer/Core/Store.cpp
+++ lib/StaticAnalyzer/Core/Store.cpp
@@ -394,14 +394,28 @@
return UnknownVal();
}
+static bool isScalarEnoughToAttemptACast(QualType T) {
+ return T->isIntegralOrEnumerationType() || T->isAnyPointerType() ||
+ T->isReferenceType();
+}
+
/// 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())
+ QualType CastTy) {
+ if (CastTy.isNull() || V.isUnknownOrUndef())
return V;
+ QualType OrigTy = R->getValueType();
+
+ if (!isScalarEnoughToAttemptACast(OrigTy) ||
+ !isScalarEnoughToAttemptACast(CastTy)) {
+ if (OrigTy == CastTy)
+ return V;
+ 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
@@ -410,13 +424,13 @@
// 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.
- if (castTy->isPointerType() && !castTy->isVoidPointerType())
+ if (CastTy->isPointerType() && !CastTy->isVoidPointerType())
if (const auto *SR = dyn_cast_or_null<SymbolicRegion>(V.getAsRegion()))
if (SR->getSymbol()->getType().getCanonicalType() !=
- castTy.getCanonicalType())
- return loc::MemRegionVal(castRegion(SR, castTy));
+ CastTy.getCanonicalType())
+ return loc::MemRegionVal(castRegion(SR, CastTy));
- return svalBuilder.dispatchCast(V, castTy);
+ return svalBuilder.dispatchCast(V, CastTy);
}
SVal StoreManager::getLValueFieldOrIvar(const Decl *D, SVal Base) {
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
