steakhal updated this revision to Diff 471112.
steakhal added a comment.
Previously, even in the `RegionStoreManager::getBinding()` even if `T` was
non-null, we replaced it with `TVR->getValueType()` in case the `MR` was
`TypedValueRegion`.
IMO we shouldn't overwrite the type unless we actually need to auto-detect the
binding type.
This was particularly wrong when we reinterpret-cast some typed memory region
(such as a stack-local variable's address) to something else while operating on
the store.
---
So, in this new version, I'm proposing to do auto-detection only if the type
was null.
---
I haven't done any measurements yet, but I'm still curious about what you think
about this.
TODO: Update the summary&title accordingly, if we agree with this direction.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D136603/new/
https://reviews.llvm.org/D136603
Files:
clang/lib/StaticAnalyzer/Core/RegionStore.cpp
clang/test/Analysis/ptr-arith.cpp
clang/test/Analysis/svalbuilder-float-cast.c
Index: clang/test/Analysis/svalbuilder-float-cast.c
===================================================================
--- clang/test/Analysis/svalbuilder-float-cast.c
+++ clang/test/Analysis/svalbuilder-float-cast.c
@@ -1,16 +1,22 @@
// RUN: %clang_analyze_cc1 -analyzer-checker debug.ExprInspection -Wno-deprecated-non-prototype -verify %s
void clang_analyzer_denote(int, const char *);
void clang_analyzer_express(int);
+void clang_analyzer_dump(int);
+void clang_analyzer_dump_ptr(int *);
void SymbolCast_of_float_type_aux(int *p) {
+ clang_analyzer_dump_ptr(p); // expected-warning {{&x}}
+ clang_analyzer_dump(*p); // expected-warning {{Unknown}}
+ // Storing to the memory region of 'float x' as 'int' will
+ // materialize a fresh conjured symbol to regain accuracy.
*p += 0;
- // FIXME: Ideally, all unknown values should be symbolicated.
- clang_analyzer_denote(*p, "$x"); // expected-warning{{Not a symbol}}
+ clang_analyzer_dump_ptr(p); // expected-warning {{&x}}
+ clang_analyzer_dump(*p); // expected-warning {{conj_$0{int}}
+ clang_analyzer_denote(*p, "$x");
*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}}
+ clang_analyzer_express(*p); // expected-warning{{$x + 1}}
}
void SymbolCast_of_float_type(void) {
Index: clang/test/Analysis/ptr-arith.cpp
===================================================================
--- clang/test/Analysis/ptr-arith.cpp
+++ clang/test/Analysis/ptr-arith.cpp
@@ -1,4 +1,5 @@
-// RUN: %clang_analyze_cc1 -Wno-unused-value -std=c++14 -analyzer-checker=core,debug.ExprInspection,alpha.core.PointerArithm -verify %s
+// RUN: %clang_analyze_cc1 -Wno-unused-value -std=c++14 -verify %s -triple x86_64-pc-linux-gnu \
+// RUN: -analyzer-checker=core,debug.ExprInspection,alpha.core.PointerArithm
template <typename T> void clang_analyzer_dump(T);
@@ -141,3 +142,22 @@
return bits->b; // no-warning
}
} // namespace Bug_55934
+
+void LValueToRValueBitCast_dumps(void *p, char (*array)[8]) {
+ clang_analyzer_dump(p);
+ clang_analyzer_dump(array);
+ // expected-warning@-2 {{&SymRegion{reg_$0<void * p>}}}
+ // expected-warning@-2 {{&SymRegion{reg_$1<char (*)[8] array>}}}
+ clang_analyzer_dump((unsigned long)p);
+ clang_analyzer_dump(__builtin_bit_cast(unsigned long, p));
+ // expected-warning@-2 {{&SymRegion{reg_$0<void * p>} [as 64 bit integer]}}
+ // expected-warning@-2 {{&SymRegion{reg_$0<void * p>} [as 64 bit integer]}}
+ clang_analyzer_dump((unsigned long)array);
+ clang_analyzer_dump(__builtin_bit_cast(unsigned long, array));
+ // expected-warning@-2 {{&SymRegion{reg_$1<char (*)[8] array>} [as 64 bit integer]}}
+ // expected-warning@-2 {{&SymRegion{reg_$1<char (*)[8] array>} [as 64 bit integer]}}
+}
+
+unsigned long ptr_arithmetic(void *p) {
+ return __builtin_bit_cast(unsigned long, p) + 1; // no-crash
+}
Index: clang/lib/StaticAnalyzer/Core/RegionStore.cpp
===================================================================
--- clang/lib/StaticAnalyzer/Core/RegionStore.cpp
+++ clang/lib/StaticAnalyzer/Core/RegionStore.cpp
@@ -1414,19 +1414,20 @@
return UnknownVal();
}
- if (!isa<TypedValueRegion>(MR)) {
- if (T.isNull()) {
- if (const TypedRegion *TR = dyn_cast<TypedRegion>(MR))
- T = TR->getLocationType()->getPointeeType();
- else if (const SymbolicRegion *SR = dyn_cast<SymbolicRegion>(MR))
- T = SR->getPointeeStaticType();
- }
- assert(!T.isNull() && "Unable to auto-detect binding type!");
- assert(!T->isVoidType() && "Attempting to dereference a void pointer!");
+ // Auto-detect the binding type.
+ if (T.isNull()) {
+ if (const auto *TVR = dyn_cast<TypedValueRegion>(MR))
+ T = TVR->getValueType();
+ else if (const auto *TR = dyn_cast<TypedRegion>(MR))
+ T = TR->getLocationType()->getPointeeType();
+ else if (const auto *SR = dyn_cast<SymbolicRegion>(MR))
+ T = SR->getPointeeStaticType();
+ }
+ assert(!T.isNull() && "Unable to auto-detect binding type!");
+ assert(!T->isVoidType() && "Attempting to dereference a void pointer!");
+
+ if (!isa<TypedValueRegion>(MR))
MR = GetElementZeroRegion(cast<SubRegion>(MR), T);
- } else {
- T = cast<TypedValueRegion>(MR)->getValueType();
- }
// FIXME: Perhaps this method should just take a 'const MemRegion*' argument
// instead of 'Loc', and have the other Loc cases handled at a higher level.
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
