xazax.hun created this revision.
xazax.hun added reviewers: dergachev.a, george.karpenkov, Szelethus.
xazax.hun added a project: clang.
Herald added subscribers: gamesh411, dkrupp, donat.nagy, mikhail.ramalho,
a.sidorin, rnkovacs, szepet, baloghadamsoftware, whisperity.
This is a patch for the following discussion on the mailing list:
http://clang-developers.42468.n3.nabble.com/analyzer-Toning-down-invalidations-td4058816.html
The consensus is, while this approach might temporarily increase the false
positive rate a bit systems of mutually-canceling bugs are worth untangling.
Most of the extra results are not false positive due to the less invalidation
but other reasons, so we could focus on those problems instead of them being
hidden.
Another consideration is that we actually introduce a new class of false
positives, when a function is doing an offset trickery with fields. I think
such functions should have a special annotation to suppress such false
positives.
There are a few questions regarding that:
- What should be the spelling of such an annotation?
- How to handle indirect calls? Even if we were in an ideal world where all the
callees are annotated, we might not know who the actual callee is (function
pointers, virtual calls etc). So what should we do? Less or more invalidation
for all indirect calls or have a separate mechanism to let the user define at
the call site how to handle a specific call?
Some other ideas from mailing Artem on the mailing list:
- Relaxing the C++ container inlining heuristic, i.e. replacing it with
visitor-based suppressions, so that to still enjoy the benefits of inlining.
This will also likely to result in less invalidation, but it could have severe
effect on how and where we spend our budget (given the complexity of STL
implementations for performance tuning).
- It shouldn't be all that hard to model extents of bindings within
RegionStore, so that bindings to sub-structures didn't overwrite bindings to
super-structures simply because they have the same base region and the same
offset. The only problem here is to model extents of *integers* because we
don't represent casts as part of SymbolRefs. All other sorts of SVals have
well-defined extents (including, say, lazy compound values).
Repository:
rC Clang
https://reviews.llvm.org/D57230
Files:
lib/StaticAnalyzer/Core/CallEvent.cpp
test/Analysis/call-invalidation.cpp
test/Analysis/cxx-uninitialized-object.cpp
test/Analysis/malloc.c
test/Analysis/taint-generic.c
test/Analysis/taint-tester.c
Index: test/Analysis/taint-tester.c
===================================================================
--- test/Analysis/taint-tester.c
+++ test/Analysis/taint-tester.c
@@ -51,7 +51,7 @@
scanf("%d", &xy.y);
scanf("%d", &xy.x);
int tx = xy.x; // expected-warning + {{tainted}}
- int ty = xy.y; // FIXME: This should be tainted as well.
+ int ty = xy.y; // expected-warning + {{tainted}}
char ntz = xy.z;// no warning
// Now, scanf scans both.
scanf("%d %d", &xy.y, &xy.x);
Index: test/Analysis/taint-generic.c
===================================================================
--- test/Analysis/taint-generic.c
+++ test/Analysis/taint-generic.c
@@ -231,6 +231,7 @@
int sock = socket(AF_INET, SOCK_STREAM, 0);
read(sock, &tainted.y, sizeof(tainted.y));
+ tainted.x = 0;
// FIXME: overlapping regions aren't detected by isTainted yet
__builtin_memcpy(buffer, tainted.y, tainted.x);
}
Index: test/Analysis/malloc.c
===================================================================
--- test/Analysis/malloc.c
+++ test/Analysis/malloc.c
@@ -1758,8 +1758,8 @@
void testConstEscapeThroughAnotherField() {
struct IntAndPtr s;
s.p = malloc(sizeof(int));
- constEscape(&(s.x)); // could free s->p!
-} // no-warning
+ constEscape(&(s.x));
+} // expected-warning {{Potential leak of memory pointed to by 's.p'}}
// PR15623
int testNoCheckerDataPropogationFromLogicalOpOperandToOpResult(void) {
Index: test/Analysis/cxx-uninitialized-object.cpp
===================================================================
--- test/Analysis/cxx-uninitialized-object.cpp
+++ test/Analysis/cxx-uninitialized-object.cpp
@@ -358,7 +358,7 @@
void wontInitialize(const T &);
class PassingToUnknownFunctionTest1 {
- int a, b;
+ int a, b; // expected-note{{uninitialized field 'this->b'}}
public:
PassingToUnknownFunctionTest1() {
@@ -368,8 +368,7 @@
}
PassingToUnknownFunctionTest1(int) {
- mayInitialize(a);
- // All good!
+ mayInitialize(a); // expected-warning{{1 uninitialized field at the end of the constructor call}}
}
PassingToUnknownFunctionTest1(int, int) {
Index: test/Analysis/call-invalidation.cpp
===================================================================
--- test/Analysis/call-invalidation.cpp
+++ test/Analysis/call-invalidation.cpp
@@ -143,7 +143,7 @@
// modify a mutable member variable through const pointer.
clang_analyzer_eval(s1.z == 1); // expected-warning{{TRUE}}
useAnything(&(s1.y));
- clang_analyzer_eval(s1.x == 1); // expected-warning{{UNKNOWN}}
+ clang_analyzer_eval(s1.x == 1); // expected-warning{{TRUE}}
}
Index: lib/StaticAnalyzer/Core/CallEvent.cpp
===================================================================
--- lib/StaticAnalyzer/Core/CallEvent.cpp
+++ lib/StaticAnalyzer/Core/CallEvent.cpp
@@ -303,11 +303,23 @@
for (unsigned Idx = 0, Count = getNumArgs(); Idx != Count; ++Idx) {
// Mark this region for invalidation. We batch invalidate regions
// below for efficiency.
- if (PreserveArgs.count(Idx))
- if (const MemRegion *MR = getArgSVal(Idx).getAsRegion())
- ETraits.setTrait(MR->getBaseRegion(),
- RegionAndSymbolInvalidationTraits::TK_PreserveContents);
- // TODO: Factor this out + handle the lower level const pointers.
+ if (const MemRegion *MR = getArgSVal(Idx).getAsRegion()) {
+ bool UseBaseRegion = true;
+ if (const auto *FR = MR->getAs<FieldRegion>()) {
+ if (const auto *TVR = FR->getSuperRegion()->getAs<TypedValueRegion>()) {
+ if (!TVR->getValueType()->isUnionType()) {
+ ETraits.setTrait(MR, RegionAndSymbolInvalidationTraits::
+ TK_DoNotInvalidateSuperRegion);
+ UseBaseRegion = false;
+ }
+ }
+ }
+ // todo: factor this out + handle the lower level const pointers.
+ if (PreserveArgs.count(Idx))
+ ETraits.setTrait(
+ UseBaseRegion ? MR->getBaseRegion() : MR,
+ RegionAndSymbolInvalidationTraits::TK_PreserveContents);
+ }
ValuesToInvalidate.push_back(getArgSVal(Idx));
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
