BugReporter is used to process ALL bug reports. By using a shared map,
we are having mappings from different PathDiagnosticPieces to LocationContexts
well beyond the point where we are processing a given report. This
state is inherently error prone, and is analogous to using a global
variable. Instead, just create a temporary map, one per report,
and when we are done with it we throw it away. No extra state.
llvm-svn: 180974
...and don't consider '0' to be a null pointer constant if it's the
initializer for a float!
Apparently null pointer constant evaluation looks through both
MaterializeTemporaryExpr and ImplicitCastExpr, so we have to be more
careful about types in the callers. For RegionStore this just means giving
up a little more; for ExprEngine this means handling the
MaterializeTemporaryExpr case explicitly.
Follow-up to r180894.
llvm-svn: 180944
It is unfortunate that we have to mark these exceptions in multiple places.
This was already in CallEvent. I suppose it does let us be more precise
about saying /which/ arguments have their retain counts invalidated -- the
connection's is still valid even though the context object's isn't -- but
we're not tracking the retain count of XPC objects anyway.
<rdar://problem/13783514>
llvm-svn: 180904
Previously, this was scattered across Environment (literal expressions),
ExprEngine (default arguments), and RegionStore (global constants). The
former special-cased several kinds of simple constant expressions, while
the latter two deferred to the AST's constant evaluator.
Now, these are all unified as SValBuilder::getConstantVal(). To keep
Environment fast, the special cases for simple constant expressions have
been left in, but the main benefits are that (a) unusual constants like
ObjCStringLiterals now work as default arguments and global constant
initializers, and (b) we're not duplicating code between ExprEngine and
RegionStore.
This actually caught a bug in our test suite, which is awesome: we stop
tracking allocated memory if it's passed as an argument along with some
kind of callback, but not if the callback is 0. We were testing this in
a case where the callback parameter had a default value, but that value
was 0. After this change, the analyzer now (correctly) flags that as a
leak!
<rdar://problem/13773117>
llvm-svn: 180894
This goes with r178516, which instructed the analyzer not to inline the
constructors and destructors of C++ container classes. This goes a step
further and does the same thing for iterators, so that the analyzer won't
falsely decide we're trying to construct an iterator pointing to a
nonexistent element.
The heuristic for determining whether something is an iterator is the
presence of an 'iterator_category' member. This is controlled under the
same -analyzer-config option as container constructor/destructor inlining:
'c++-container-inlining'.
<rdar://problem/13770187>
llvm-svn: 180890
This doesn't appear to be the cause of the slowdown. I'll have to try a
manual bisect to see if there's really anything there, or if it's just
the bot itself taking on additional load. Meanwhile, this change helps
with correctness.
This changes an assertion and adds a test case, then re-applies r180638,
which was reverted in r180714.
<rdar://problem/13296133> and PR15863
llvm-svn: 180864
Much of this patch outside of PathDiagnostics.h are just minor
syntactic changes due to the return type for operator* and the like
changing for the iterator, so the real focus should be on
PathPieces itself.
This change is motivated so that we can do efficient insertion
and removal of individual pieces from within a PathPiece, just like
this was a kind of "IR" for static analyzer diagnostics. We
currently implement path transformations by iterating over an
entire PathPiece and making a copy. This isn't very natural for
some algorithms.
We use an ilist here instead of std::list because we want operations
to rip out/insert nodes in place, just like IR manipulation. This
isn't being used yet, but opens the door for more powerful
transformation algorithms on diagnostic paths.
llvm-svn: 180741
This seems to be causing quite a slowdown on our internal analyzer bot,
and I'm not sure why. Needs further investigation.
This reverts r180638 / 9e161ea981f22ae017b6af09d660bfc3ddf16a09.
llvm-svn: 180714
In an Objective-C for-in loop "for (id element in collection) {}", the loop
will run 0 times if the collection is nil. This is because the for-in loop
is implemented using a protocol method that returns 0 when there are no
elements to iterate, and messages to nil will result in a 0 return value.
At some point we may want to actually model this message send, but for now
we may as well get the nil case correct, and avoid the false positives that
would come with this case.
<rdar://problem/13744632>
llvm-svn: 180639
Casts to bool (and _Bool) are equivalent to checks against zero,
not truncations to 1 bit or 8 bits.
This improved reasoning does cause a change in the behavior of the alpha
BoolAssignment checker. Previously, this checker complained about statements
like "bool x = y" if 'y' was known not to be 0 or 1. Now it does not, since
that conversion is well-defined. It's hard to say what the "best" behavior
here is: this conversion is safe, but might be better written as an explicit
comparison against zero.
More usefully, besides improving our model of booleans, this fixes spurious
warnings when returning the address of a local variable cast to bool.
<rdar://problem/13296133>
llvm-svn: 180638
We get a CallEnter with a null expression, when processing a destructor. All other users of
CallEnter::getCallExpr work fine with null as return value.
(Addresses PR15832, Thanks to Jordan for reducing the test case!)
llvm-svn: 180234
- If only partial invalidators exist and there are no full invalidators in @implementation, report every ivar that has
not been invalidated. (Previously, we reported the first Ivar in the list, which could actually have been invalidated
by a partial invalidator. The code assumed you cannot have only partial invalidators.)
- Do not report missing invalidation method declaration if a partial invalidation method declaration exists.
llvm-svn: 180170
The uniqueing location is the location which is part of the hash used to determine if two reports are
the same. This is used by the CmpRuns.py script to compare two analyzer runs and determine which
warnings are new.
llvm-svn: 180166
The 2 functions were computing the same location using different logic (each one had edge case bugs that the other
one did not). Refactor them to rely on the same logic.
The location of the warning reported in text/command line output format will now match that of the plist file.
There is one change in the plist output as well. When reporting an error on a BinaryOperator, we use the location of the
operator instead of the beginning of the BinaryOperator expression. This matches our output on command line and
looks better in most cases.
llvm-svn: 180165
The analyzer represents all pointer-to-pointer bitcasts the same way, but
this can be problematic if an implicit base cast gets layered on top of a
manual base cast (performed with reinterpret_cast instead of static_cast).
Fix this (and avoid a valid assertion) by looking through cast regions.
Using reinterpret_cast this way is only valid if the base class is at the
same offset as the derived class; this is checked by -Wreinterpret-base-class.
In the interest of performance, the analyzer doesn't repeat this check
anywhere; it will just silently do the wrong thing (use the wrong offsets
for fields of the base class) if the user code is wrong.
PR15394
llvm-svn: 180052
Add a CXXDefaultInitExpr, analogous to CXXDefaultArgExpr, and use it both in
CXXCtorInitializers and in InitListExprs to represent a default initializer.
There's an additional complication here: because the default initializer can
refer to the initialized object via its 'this' pointer, we need to make sure
that 'this' points to the right thing within the evaluation.
llvm-svn: 179958
Introduce a new helper function, which computes the first symbolic region in
the base region chain. The corresponding symbol has been used for assuming that
a pointer is null. Now, it will also be used for checking if it is null.
This ensures that we are tracking a null pointer correctly in the BugReporter.
llvm-svn: 179916
The analyzer uses LazyCompoundVals to represent rvalues of aggregate types,
most importantly structs and arrays. This allows us to efficiently copy
around an entire struct, rather than doing a memberwise load every time a
struct rvalue is encountered. This can also keep memory usage down by
allowing several structs to "share" the same snapshotted bindings.
However, /lookup/ through LazyCompoundVals can be expensive, especially
since they can end up chaining back to the original value. While we try
to reuse LazyCompoundVals whenever it's safe, and cache information about
this transitivity, the fact is it's sometimes just not a good idea to
perpetuate LazyCompoundVals -- the tradeoffs just aren't worth it.
This commit changes RegionStore so that binding a LazyCompoundVal to struct
will do a memberwise copy if the struct is simple enough. Today's definition
of "simple enough" is "up to N scalar members" (see below), but that could
easily be changed in the future. This is enough to bring the test case in
PR15697 back down to a manageable analysis time (within 20% of its original
time, in an unfair test where the new analyzer is not compiled with LTO).
The actual value of "N" is controlled by a new -analyzer-config option,
'region-store-small-struct-limit'. It defaults to "2", meaning structs with
zero, one, or two scalar members will be considered "simple enough" for
this code path.
It's worth noting that a more straightforward implementation would do this
on load, not on bind, and make use of the structure we already have for this:
CompoundVal. A long time ago, this was actually how RegionStore modeled
aggregate-to-aggregate copies, but today it's only used for compound literals.
Unfortunately, it seems that we've special-cased LazyCompoundVal in certain
places (such as liveness checks) but failed to similarly special-case
CompoundVal in all of them. Until we're confident that CompoundVal is
handled properly everywhere, this solution is safer, since the entire
optimization is just an implementation detail of RegionStore.
<rdar://problem/13599304>
llvm-svn: 179767
A C++ overloaded operator may be implemented as an instance method, and
that instance method may be called on an rvalue object, which has no
associated region. The analyzer handles this by creating a temporary region
just for the evaluation of this call; however, it is possible that /by
creating the region/, the analyzer ends up in a previously-explored state.
In this case we don't need to continue along this path.
This doesn't actually show any behavioral change now, but it starts being
used with the next commit and prevents an assertion failure there.
llvm-svn: 179766
In the committed example, we now see a note that tells us when the pointer
was assumed to be null.
This is the only case in which getDerefExpr returned null (failed to get
the dereferenced expr) throughout our regression tests. (There were multiple
occurrences of this one.)
llvm-svn: 179736
We always register the visitor on a node in which the value we are tracking is live and constrained. However,
the visitation can restart at a node, later on the path, in which the value is under constrained because
it is no longer live. Previously, we just silently stopped tracking in that case.
llvm-svn: 179731
This was slightly tricky because BlockDecls don't currently store an
inferred return type. However, we can rely on the fact that blocks with
inferred return types will have return statements that match the inferred
type.
<rdar://problem/13665798>
llvm-svn: 179699
This is an opt-in tweak for leak diagnostics to reference the allocation
site if the diagnostic consumer only wants a pithy amount of information,
and not the entire path.
This is a strawman enhancement that I expect to see some experimentation
with over the next week, and can go away if we don't want it.
Currently it is only used by RetainCountChecker, but could be used
by MallocChecker if and when we decide this should stay in.
llvm-svn: 179634
during checker registration. There are no immediate clients of this,
but this provides a way for checkers to query the options table
at startup instead.
llvm-svn: 179626
When computing the value of ?: expression, we rely on the last expression in
the previous basic block to be the resulting value of the expression. This is
not the case for binary "?:" operator (GNU extension) in C++. As the last
basic block has the expression for the condition subexpression, which is an
R-value, whereas the true subexpression is the L-value.
Note the operator evaluation just happens to work in C since the true
subexpression is an R-value (like the condition subexpression). CFG is the
same in C and C++ case, but the AST nodes are different, which the LValue to
Rvalue conversion happening after the BinaryConditionalOperator evaluation.
Changed the logic to only use the last expression from the predecessor only
if it matches either true or false subexpression. Note, the logic needed
fortification anyway: L and R were passed but not even used by the function.
Also, change the conjureSymbolVal to correctly compute the type, when the
expression is an LG-value.
llvm-svn: 179574
While we don't do anything intelligent with pointers-to-members today,
it's perfectly legal to need a temporary of pointer-to-member type to, say,
pass by const reference. Tweak an assertion to allow this.
PR15742 and PR15747
llvm-svn: 179563
Structs and arrays can take advantage of the single top-level global
symbol optimization (described in the previous commit) just as well
as scalars.
No intended behavioral change.
llvm-svn: 179555
Now that we're invalidating global regions properly, we want to continue
taking advantage of a particular optimization: if all global regions are
invalidated together, we can represent the bindings of each region with
a "derived region value" symbol. Essentially, this lazily links each
global region with a single symbol created at invalidation time, rather
than binding each region with a new symbolic value.
We used to do this, but haven't been for a while; the previous commit
re-enabled this code path, and this handles the fallout.
<rdar://problem/13464044>
llvm-svn: 179554
This fixes a regression where a call to a function we can't reason about
would not actually invalidate global regions that had explicit bindings.
void test_that_now_works() {
globalInt = 42;
clang_analyzer_eval(globalInt == 42); // expected-warning{{TRUE}}
invalidateGlobals();
clang_analyzer_eval(globalInt == 42); // expected-warning{{UNKNOWN}}
}
This has probably been around since the initial "cluster" refactoring of
RegionStore, if not longer.
<rdar://problem/13464044>
llvm-svn: 179553
There are few cases where we can track the region, but cannot print the note,
which makes the testing limited. (Though, I’ve tested this manually by making
all regions non-printable.) Even though the applicability is limited now, the enhancement
will be more relevant as we start tracking more regions.
llvm-svn: 179396
Before:
1. Calling 'foo'
2. Doing something interesting
3. Returning from 'foo'
4. Some kind of error here
After:
1. Calling 'foo'
2. Doing something interesting
3. Returning from 'foo'
4. Some kind of error here
The location of the note is already in the caller, not the callee, so this
just brings the "depth" attribute in line with that.
This only affects plist diagnostic consumers (i.e. Xcode). It's necessary
for Xcode to associate the control flow arrows with the right stack frame.
<rdar://problem/13634363>
llvm-svn: 179351
In this code
int getZero() {
return 0;
}
void test() {
int problem = 1 / getZero(); // expected-warning {{Division by zero}}
}
we generate these arrows:
+-----------------+
| v
int problem = 1 / getZero();
^ |
+---+
where the top one represents the control flow up to the first call, and the
bottom one represents the flow to the division.* It turns out, however, that
we were generating the top arrow twice, as if attempting to "set up context"
after we had already returned from the call. This resulted in poor
highlighting in Xcode.
* Arguably the best location for the division is the '/', but that's a
different problem.
<rdar://problem/13326040>
llvm-svn: 179350
The heuristic here (proposed by Jordan) is that, usually, if a leak is due to an early exit from init, the allocation site will be
a call to alloc. Note that in other cases init resets self to [super init], which becomes the allocation site of the object.
llvm-svn: 179221
Previously, the analyzer used isIntegerType() everywhere, which uses the C
definition of "integer". The C++ predicate with the same behavior is
isIntegerOrUnscopedEnumerationType().
However, the analyzer is /really/ using this to ask if it's some sort of
"integrally representable" type, i.e. it should include C++11 scoped
enumerations as well. hasIntegerRepresentation() sounds like the right
predicate, but that includes vectors, which the analyzer represents by its
elements.
This commit audits all uses of isIntegerType() and replaces them with the
general isIntegerOrEnumerationType(), except in some specific cases where
it makes sense to exclude scoped enumerations, or any enumerations. These
cases now use isIntegerOrUnscopedEnumerationType() and getAs<BuiltinType>()
plus BuiltinType::isInteger().
isIntegerType() is hereby banned in the analyzer - lib/StaticAnalysis and
include/clang/StaticAnalysis. :-)
Fixes real assertion failures. PR15703 / <rdar://problem/12350701>
llvm-svn: 179081
Test that the path notes do not change. I don’t think we should print a note on escape.
Also, I’ve removed a check that assumed that the family stored in the RefStete could be
AF_None and added an assert in the constructor.
llvm-svn: 179075
This is important because sometimes two nodes are identical, except the
second one is a sink.
This bug has probably been around for a while, but it wouldn't have been an
issue in the old report graph algorithm. I'm ashamed to say I actually looked
at this the first time around and thought it would never be a problem...and
then didn't include an assertion to back that up.
PR15684
llvm-svn: 178944
As mentioned in the previous commit message, the use-after-free and
double-free warnings for 'delete' are worth enabling even while the
leak warnings still have false positives.
llvm-svn: 178891
This splits the leak-checking part of alpha.cplusplus.NewDelete into a
separate user-level checker, alpha.cplusplus.NewDeleteLeaks. All the
difficult false positives we've seen with the new/delete checker have been
spurious leak warnings; the use-after-free warnings and mismatched
deallocator warnings, while rare, have always been valid.
<rdar://problem/6194569>
llvm-svn: 178890
The statement passed to isTrackedFamily() might be a user defined function calling malloc; in this case we got AF_NONE family for this function.
Now the allocation family is derived from Sym, that holds a family of a real allocator.
This commit is also a movement towards getting rid of tracking memory allocating by unknown means.
llvm-svn: 178834
This fixes an issue pointed to by Jordan: if unix.Malloc and unix.MismatchedDeallocator are both on, then we end up still tracking leaks of memory allocated by new.
Moved the guards right before emitting the bug reports to unify and simplify the logic of handling of multiple checkers. Now all the checkers perform their checks regardless of if they were enabled, or not, and it is decided just before the emitting of the report, if it should be emitted. (idea from Anna).
Additional changes:
improved test coverage for checker correlations;
refactoring: BadDealloc -> MismatchedDealloc
llvm-svn: 178814
This turns on not only destructor inlining, but inlining of constructors
for types with non-trivial destructors. Per r178516, we will still not
inline the constructor or destructor of anything that looks like a
container unless the analyzer-config option 'c++-container-inlining' is
set to 'true'.
In addition to the more precise path-sensitive model, this allows us to
catch simple smart pointer issues:
#include <memory>
void test() {
std::auto_ptr<int> releaser(new int[4]);
} // memory allocated with 'new[]' should not be deleted with 'delete'
<rdar://problem/12295363>
llvm-svn: 178805
...and add a new test case.
I thought this was broken, but it isn't; refactoring and reformatting anyway
so that I don't make the same mistake again. No functionality change.
llvm-svn: 178799
Improvement of r178684 and r178685.
Jordan has pointed out that I should not rely on the value of the condition to know which expression branch
has been taken. It will not work in cases the branch condition is an unknown value (ex: we do not track the constraints for floats).
The better way of doing this would be to find out if the current node is the right or left successor of the node
that has the ternary operator as a terminator (which is how this is done in other places, like ConditionBRVisitor).
llvm-svn: 178701
The lifetime of a temporary can be extended when it is immediately bound
to a local reference:
const Value &MyVal = Value("temporary");
In this case, the temporary object's lifetime is extended for the entire
scope of the reference; at the end of the scope it is destroyed.
The analyzer was modeling this improperly in two ways:
- Since we don't model temporary constructors just yet, we create a fake
temporary region when it comes time to "materialize" a temporary into
a real object (lvalue). This wasn't taking base casts into account when
the bindings being materialized was Unknown; now it always respects base
casts except when the temporary region is itself a pointer.
- When actually destroying the region, the analyzer did not actually load
from the reference variable -- it was basically destroying the reference
instead of its referent. Now it does do the load.
This will be more useful whenever we finally start modeling temporaries,
or at least those that get bound to local reference variables.
<rdar://problem/13552274>
llvm-svn: 178697
1) Look for the node where the condition expression is live when checking if
it is constrained to true or false.
2) Fix a bug in ProgramState::isNull, which was masking the problem. When
the expression is not a symbol (,which is the case when it is Unknown) return
unconstrained value, instead of value constrained to “false”!
(Thankfully other callers of isNull have not been effected by the bug.)
llvm-svn: 178684
- Find the correct region to represent the first array element when
constructing a CXXConstructorCall.
- If the array is trivial, model the copy with a primitive load/store.
- Don't warn about the "uninitialized" subscript in the AST -- we don't use
the helper variable that Sema provides.
<rdar://problem/13091608>
llvm-svn: 178602
Refactor invalidateRegions to take SVals instead of Regions as input and teach RegionStore
about processing LazyCompoundVal as a top-level “escaping” value.
This addresses several false positives that get triggered by the NewDelete checker, but the
underlying issue is reproducible with other checkers as well (for example, MallocChecker).
llvm-svn: 178518
This is a heuristic to make up for the fact that the analyzer doesn't
model C++ containers very well. One example is modeling that
'std::distance(I, E) == 0' implies 'I == E'. In the future, it would be
nice to model this explicitly, but for now it just results in a lot of
false positives.
The actual heuristic checks if the base type has a member named 'begin' or
'iterator'. If so, we treat the constructors and destructors of that type
as opaque, rather than inlining them.
This is intended to drastically reduce the number of false positives
reported with experimental destructor support turned on. We can tweak the
heuristic in the future, but we'd rather err on the side of false negatives
for now.
<rdar://problem/13497258>
llvm-svn: 178516
Certain properties of a function can determine ahead of time whether or not
the function is inlineable, such as its kind, its signature, or its
location. We can cache this value in the FunctionSummaries map to avoid
rechecking these static properties for every call.
Note that the analyzer may still decide not to inline a specific call to
a function because of the particular dynamic properties of the call along
the current path.
No intended functionality change.
llvm-svn: 178515
The summaries lasted for the lifetime of the map anyway; no reason to
include an extra allocation.
Also, use SmallBitVector instead of BitVector to track the visited basic
blocks -- most functions will have less than 64 basic blocks -- and
use bitfields for the other fields to reduce the size of the structure.
No functionality change.
llvm-svn: 178514
This is controlled by the 'suppress-c++-stdlib' analyzer-config flag.
It is currently off by default.
This is more suppression than we'd like to do, since obviously there can
be user-caused issues within 'std', but it gives us the option to wield
a large hammer to suppress false positives the user likely can't work
around.
llvm-svn: 178513
Evaluating a C++ new expression now includes generating an intermediate
ExplodedNode, and this node could very well represent a previously-
reachable state in the ExplodedGraph. If so, we can short-circuit the
rest of the evaluation.
Caught by the assertion a few lines later.
<rdar://problem/13510065>
llvm-svn: 178401
We can check if the receiver is nil in the node that corresponds to the StmtPoint of the message send.
At that point, the receiver is guaranteed to be live. We will find at least one unreclaimed node due to
my previous commit (look for StmtPoint instead of PostStmt) and the fact that the nil receiver nodes are tagged.
+ a couple of extra tests.
llvm-svn: 178381
trackNullOrUndefValue tries to find the first node that matches the statement it is tracking.
Since we collect PostStmt nodes (in node reclamation), none of those might be on the
current path, so relax the search to look for any StmtPoint.
llvm-svn: 178380
Add a new callback that notifies checkers when a const pointer escapes. Currently, this only works
for const pointers passed as a top level parameter into a function. We need to differentiate the const
pointers escape from regular escape since the content pointed by const pointer will not change;
if it’s a file handle, a file cannot be closed; but delete is allowed on const pointers.
This should suppress several false positives reported by the NewDelete checker on llvm codebase.
llvm-svn: 178310
We should only suppress a bug report if the IDCed or null returned nil value is directly related to the value we are warning about. This was
not the case for nil receivers - we would suppress a bug report that had an IDCed nil receiver on the path regardless of how it’s
related to the warning.
1) Thread EnableNullFPSuppression parameter through the visitors to differentiate between tracking the value which
is directly responsible for the bug and other values that visitors are tracking (ex: general tracking of nil receivers).
2) in trackNullOrUndef specifically address the case when a value of the message send is nil due to the receiver being nil.
llvm-svn: 178309
+ Improved display names for allocators and deallocators
The checker checks if a deallocation function matches allocation one. ('free' for 'malloc', 'delete' for 'new' etc.)
llvm-svn: 178250
These types will not have a CXXConstructExpr to do the initialization for
them. Previously we just used a simple call to ProgramState::bindLoc, but
that doesn't trigger proper checker callbacks (like pointer escape).
Found by Anton Yartsev.
llvm-svn: 178160
The visitor should look for the PreStmt node as the receiver is nil in the PreStmt and this is the node. Also, tag the nil
receiver nodes with a special tag for consistency.
llvm-svn: 178152
Register the nil tracking visitors with the region and refactor trackNullOrUndefValue a bit.
Also adds the cast and paren stripping before checking if the value is an OpaqueValueExpr
or ExprWithCleanups.
llvm-svn: 178093
This addresses an undefined value false positive from concreteOffsetBindingIsInvalidatedBySymbolicOffsetAssignment.
Fixes PR14877; radar://12991168.
llvm-svn: 177905
These aren't generated by default, but they are needed when either side of
the comparison is tainted.
Should fix our internal buildbot.
llvm-svn: 177846
In C, comparisons between signed and unsigned numbers are always done in
unsigned-space. Thus, we should know that "i >= 0U" is always true, even
if 'i' is signed. Similarly, "u >= 0" is also always true, even though '0'
is signed.
Part of <rdar://problem/13239003> (false positives related to std::vector)
llvm-svn: 177806
For two concrete locations, we were producing another concrete location and
then casting it to an integer. We should just create a nonloc::ConcreteInt
to begin with.
No functionality change.
llvm-svn: 177805
We can support the full range of comparison operations between two locations
by canonicalizing them as subtraction, as in the previous commit.
This won't work (well) if either location includes an offset, or (again)
if the comparisons are not consistent about which region comes first.
<rdar://problem/13239003>
llvm-svn: 177803
Canonicalizing these two forms allows us to better model containers like
std::vector, which use "m_start != m_finish" to implement empty() but
"m_finish - m_start" to implement size(). The analyzer should have a
consistent interpretation of these two symbolic expressions, even though
it's not properly reasoning about either one yet.
The other unfortunate thing is that while the size() expression will only
ever be written "m_finish - m_start", the comparison may be written
"m_finish == m_start" or "m_start == m_finish". Right now the analyzer does
not attempt to canonicalize those two expressions, since it doesn't know
which length expression to pick. Doing this correctly will probably require
implementing unary minus as a new SymExpr kind (<rdar://problem/12351075>).
For now, the analyzer inverts the order of arguments in the comparison to
build the subtraction, on the assumption that "begin() != end()" is
written more often than "end() != begin()". This is purely speculation.
<rdar://problem/13239003>
llvm-svn: 177801
We just treat this as opaque symbols, but even that allows us to handle
simple cases where the same condition is tested twice. This is very common
in the STL, which means that any project using the STL gets spurious errors.
Part of <rdar://problem/13239003>.
llvm-svn: 177800
The algorithm used here was ridiculously slow when a potential back-edge
pointed to a node that already had a lot of successors. The previous commit
makes this feature unnecessary anyway.
This reverts r177468 / f4cf6b10f863b9bc716a09b2b2a8c497dcc6aa9b.
Conflicts:
lib/StaticAnalyzer/Core/BugReporter.cpp
llvm-svn: 177765
For a given bug equivalence class, we'd like to emit the report with the
shortest path. So far to do this we've been trimming the ExplodedGraph to
only contain relevant nodes, then doing a reverse BFS (starting at all the
error nodes) to find the shortest paths from the root. However, this is
fairly expensive when we are suppressing many bug reports in the same
equivalence class.
r177468-9 tried to solve this problem by breaking cycles during graph
trimming, then updating the BFS priorities after each suppressed report
instead of recomputing the whole thing. However, breaking cycles is not
a cheap operation because an analysis graph minus cycles is still a DAG,
not a tree.
This fix changes the algorithm to do a single forward BFS (starting from the
root) and to use that to choose the report with the shortest path by looking
at the error nodes with the lowest BFS priorities. This was Anna's idea, and
has the added advantage of requiring no update step: we can just pick the
error node with the next lowest priority to produce the next bug report.
<rdar://problem/13474689>
llvm-svn: 177764
This fixes some mistaken condition logic in RegionStore that caused
global variables to be invalidated when /any/ region was invalidated,
rather than only as part of opaque function calls. This was only
being used by CStringChecker, and so users will now see that strcpy()
and friends do not invalidate global variables.
Also, add a test case we don't handle properly: explicitly-assigned
global variables aren't being invalidated by opaque calls. This is
being tracked by <rdar://problem/13464044>.
llvm-svn: 177572
Due to improper modelling of copy constructors (specifically, their
const reference arguments), we were producing spurious leak warnings
for allocated memory stored in structs. In order to silence this, we
decided to consider storing into a struct to be the same as escaping.
However, the previous commit has fixed this issue and we can now properly
distinguish leaked memory that happens to be in a struct from a buffer
that escapes within a struct wrapper.
Originally applied in r161511, reverted in r174468.
<rdar://problem/12945937>
llvm-svn: 177571
In this case, the value of 'x' may be changed after the call to indirectAccess:
struct Wrapper {
int *ptr;
};
void indirectAccess(const Wrapper &w);
void test() {
int x = 42;
Wrapper w = { x };
clang_analyzer_eval(x == 42); // TRUE
indirectAccess(w);
clang_analyzer_eval(x == 42); // UNKNOWN
}
This is important for modelling return-by-value objects in C++, to show
that the contents of the struct are escaping in the return copy-constructor.
<rdar://problem/13239826>
llvm-svn: 177570
This is a bit of old code trying to deal with the fact that functions that
take pointers often use them to access an entire array via pointer
arithmetic. However, RegionStore already conservatively assumes you can use
pointer arithmetic to access any part of a region.
Some day we may want to go back to handling this specifically for calls,
but we can do that in the future.
No functionality change.
llvm-svn: 177569
With the assurance that the trimmed graph does not contain cycles,
this patch is safe (with a few tweaks), and provides the performance
boost it was intended to.
Part of performance work for <rdar://problem/13433687>.
llvm-svn: 177469
Having a trimmed graph with no cycles (a DAG) is much more convenient for
trying to find shortest paths, which is exactly what BugReporter needs to do.
Part of the performance work for <rdar://problem/13433687>.
llvm-svn: 177468
This fixes a crash when analyzing LLVM that was exposed by r177220 (modeling of
trivial copy/move assignment operators).
When we look up a lazy binding for “Builder”, we see the direct binding of Loc at offset 0.
Previously, we believed the binding, which led to a crash. Now, we do not believe it as
the types do not match.
llvm-svn: 177453
The whole reason we were doing a BFS in the first place is because an
ExplodedGraph can have cycles. Unfortunately, my removeErrorNode "update"
doesn't work at all if there are cycles.
I'd still like to be able to avoid doing the BFS every time, but I'll come
back to it later.
This reverts r177353 / 481fa5071c203bc8ba4f88d929780f8d0f8837ba.
llvm-svn: 177448
Splitting the graph trimming and the path-finding (r177216) already
recovered quite a bit of performance lost to increased suppression.
We can still do better by also performing the reverse BFS up front
(needed for shortest-path-finding) and only walking the shortest path
for each report. This does mean we have to walk back up the path and
invalidate all the BFS numbers if the report turns out to be invalid,
but it's probably still faster than redoing the full BFS every time.
More performance work for <rdar://problem/13433687>
llvm-svn: 177353
r175234 allowed the analyzer to model trivial copy/move constructors as
an aggregate bind. This commit extends that to trivial assignment
operators as well. Like the last commit, one of the motivating factors here
is not warning when the right-hand object is partially-initialized, which
can have legitimate uses.
<rdar://problem/13405162>
llvm-svn: 177220
When we generate a path diagnostic for a bug report, we have to take the
full ExplodedGraph and limit it down to a single path. We do this in two
steps: "trimming", which limits the graph to all the paths that lead to
this particular bug, and "creating the report graph", which finds the
shortest path in the trimmed path to any error node.
With BugReporterVisitor false positive suppression, this becomes more
expensive: it's possible for some paths through the trimmed graph to be
invalid (i.e. likely false positives) but others to be valid. Therefore
we have to run the visitors over each path in the graph until we find one
that is valid, or until we've ruled them all out. This can become quite
expensive.
This commit separates out graph trimming from creating the report graph,
performing the first only once per bug equivalence class and the second
once per bug report. It also cleans up that portion of the code by
introducing some wrapper classes.
This seems to recover most of the performance regression described in my
last commit.
<rdar://problem/13433687>
llvm-svn: 177216
...in favor of this typedef:
typedef llvm::DenseMap<const ExplodedNode *, const ExplodedNode *>
InterExplodedGraphMap;
Use this everywhere the previous class and typedef were used.
Took the opportunity to ArrayRef-ize ExplodedGraph::trim while I'm at it.
No functionality change.
llvm-svn: 177215
I removed this check in the recursion->iteration commit, but forgot that
generatePathDiagnostic may be called multiple times if there are multiple
PathDiagnosticConsumers.
llvm-svn: 177214
Fixes a FIXME, improves dead symbol collection, suppresses a false positive,
which resulted from reusing the same symbol twice for simulation of 2 calls to the same function.
Fixing this lead to 2 possible false negatives in CString checker. Since the checker is still alpha and
the solution will not require revert of this commit, move the tests to a FIXME section.
llvm-svn: 177206
The previous generatePathDiagnostic() was intended to be tail-recursive,
restarting and trying again if a report was marked invalid. However:
(1) this leaked all the cloned visitors, which weren't being deleted, and
(2) this wasn't actually tail-recursive because some local variables had
non-trivial destructors.
This was causing us to overflow the stack on inputs with large numbers of
reports in the same equivalence class, such as sqlite3.c. Being iterative
at least prevents us from blowing out the stack, but doesn't solve the
performance issue: suppressing thousands (yes, thousands) of paths in the
same equivalence class is expensive. I'm looking into that now.
<rdar://problem/13423498>
llvm-svn: 177189
We discovered that sqlite3.c currently has 2600 reports in a single
equivalence class; it would be good to know if this is a recent
development or what.
(For the curious, the different reports in an equivalence class represent
the same bug found along different paths. When we're suppressing false
positives, we need to go through /every/ path to make sure there isn't a
valid path to a bug. This is a flaw in our after-the-fact suppression,
made worse by the fact that that function isn't particularly optimized.)
llvm-svn: 177188
In the test case below, the value V is not constrained to 0 in ErrorNode but it is in node N.
So we used to fail to register the Suppression visitor.
We also need to change the way we determine that the Visitor should kick in because the node N belongs to
the ExplodedGraph and might not be on the BugReporter path that the visitor sees. Instead of trying to match the node,
turn on the visitor when we see the last node in which the symbol is ‘0’.
llvm-svn: 177121
When BugReporter tracks C++ references involved in a null pointer violation, we
want to differentiate between a null reference and a reference to a null pointer. In the
first case, we want to track the region for the reference location; in the second, we want
to track the null pointer.
In addition, the core creates CXXTempObjectRegion to represent the location of the
C++ reference, so teach FindLastStoreBRVisitor about it.
This helps null pointer suppression to kick in.
(Patch by Anna and Jordan.)
llvm-svn: 176969
r176737 fixed bugreporter::trackNullOrUndefValue to find nodes for an lvalue
even if the rvalue node had already been collected. This commit extends that
to call statement nodes as well, so that if a call is contained within
implicit casts we can still track the return value.
No test case because node reclamation is extremely finicky (dependent on
how the AST and CFG are built, and then on our current reclamation rules,
and /then/ on how many nodes were generated by the analyzer core and the
current set of checkers). I consider this a low-risk change, though, and
it will only happen in cases of reclamation when the rvalue node isn't
available.
<rdar://problem/13340764>
llvm-svn: 176829
The visitor used to assume that the value it’s tracking is null in the first node it examines. This is not true.
If we are registering the Suppress Inlined Defensive checks visitor while traversing in another visitor
(such as FindlastStoreVisitor). When we restart with the IDC visitor, the invariance of the visitor does
not hold since the symbol we are tracking no longer exists at that point.
I had to pass the ErrorNode when creating the IDC visitor, because, in some cases, node N is
neither the error node nor will be visible along the path (we had not finalized the path at that point
and are dealing with ExplodedGraph.)
We should revisit the other visitors which might not be aware that they might get nodes, which are
later in path than the trigger point.
This suppresses a number of inline defensive checks in JavaScriptCore.
llvm-svn: 176756
Previously, MallocChecker's pointer escape check and its post-call state
update for Objective-C method calls had a fair amount duplicated logic
and not-entirely-consistent checks. This commit restructures all this to
be more consistent and possibly allow us to be more aggressive in warning
about double-frees.
New policy (applies to system header methods only):
(1) If this is a method we know about, model it as taking/holding ownership
of the passed-in buffer.
(1a) ...unless there's a "freeWhenDone:" parameter with a zero (NO) value.
(2) If there's a "freeWhenDone:" parameter (but it's not a method we know
about), treat the buffer as escaping if the value is non-zero (YES) and
non-escaping if it's zero (NO).
(3) If the first selector piece ends with "NoCopy" (but it's not a method we
know about and there's no "freeWhenDone:" parameter), treat the buffer
as escaping.
The reason that (2) and (3) don't explicitly model the ownership transfer is
because we can't be sure that they will actually free the memory using free(),
and we wouldn't want to emit a spurious "mismatched allocator" warning
(coming in Anton's upcoming patch). In the future, we may have an idea of a
"generic deallocation", i.e. we assume that the deallocator is correct but
still continue tracking the region so that we can warn about double-frees.
Patch by Anton Yartsev, with modifications from me.
llvm-svn: 176744
r176010 introduced the notion of "interesting" lvalue expressions, whose
nodes are guaranteed never to be reclaimed by the ExplodedGraph. This was
used in bugreporter::trackNullOrUndefValue to find the region that contains
the null or undef value being tracked.
However, the /rvalue/ nodes (i.e. the loads from these lvalues that produce
a null or undef value) /are/ still being reclaimed, and if we couldn't
find the node for the rvalue, we just give up. This patch changes that so
that we look for the node for either the rvalue or the lvalue -- preferring
the former, since it lets us fall back to value-only tracking in cases
where we can't get a region, but allowing the latter as well.
<rdar://problem/13342842>
llvm-svn: 176737
Previously, ReturnVisitor waited to suppress a null return path until it
had found the inlined "return" statement. Now, it checks up front whether
the return value was NULL, and suppresses the warning right away if so.
We still have to wait until generating the path notes to invalidate the bug
report, or counter-suppression will never be triggered. (Counter-suppression
happens while generating path notes, but the generation won't happen for
reports already marked invalid.)
This isn't actually an issue today because we never reclaim nodes for
top-level statements (like return statements), but it could be an issue
some day in the future. (But, no expected behavioral change and no new
test case.)
llvm-svn: 176736
Warn about null pointer dereference earlier when a reference to a null pointer is
passed in a call. The idea is that even though the standard might allow this, reporting
the issue earlier is better for diagnostics (the error is reported closer to the place where
the pointer was set to NULL). This also simplifies analyzer’s diagnostic logic, which has
to track “where the null came from”. As a consequence, some of our null pointer
warning suppression mechanisms started triggering more often.
TODO: Change the name of the file and class to reflect the new check.
llvm-svn: 176612
Officially in the C++ standard, a null reference cannot exist. However,
it's still very easy to create one:
int &getNullRef() {
int *p = 0;
return *p;
}
We already check that binds to reference regions don't create null references.
This patch checks that we don't create null references by returning, either.
<rdar://problem/13364378>
llvm-svn: 176601
The second modification does not lead to any visible result, but, theoretically, is what we should
have been looking at to begin with since we are checking if the node was assumed to be null in
an inlined function.
llvm-svn: 176576
We weren't treating a cf_audited_transfer CFRetain as returning +1 because
its name doesn't contain "Create" or "Copy". Oops! Fortunately, the
standard definitions of these functions are not marked audited.
<rdar://problem/13339601>
llvm-svn: 176463
Inlining brought a few "null pointer use" false positives, which occur because
the callee defensively checks if a pointer is NULL, whereas the caller knows
that the pointer cannot be NULL in the context of the given call.
This is a first attempt to silence these warnings by tracking the symbolic value
along the execution path in the BugReporter. The new visitor finds the node
in which the symbol was first constrained to NULL. If the node belongs to
a function on the active stack, the warning is reported, otherwise, it is
suppressed.
There are several areas for follow up work, for example:
- How do we differentiate the cases where the first check is followed by
another one, which does happen on the active stack?
Also, this only silences a fraction of null pointer use warnings. For example, it
does not do anything for the cases where NULL was assigned inside a callee.
llvm-svn: 176402
Previously we were assuming that we'd never ask for the sub-region bindings
of a bitfield, since a bitfield cannot have subregions. However,
unification of code paths has made that assumption invalid. While we could
take advantage of this by just checking for the single possible binding,
it's probably better to do the right thing, so that if/when we someday
support unions we'll do the right thing there, too.
This fixes a handful of false positives in analyzing LLVM.
<rdar://problem/13325522>
llvm-svn: 176388
Most map types have an operator[] that inserts a new element if the key
isn't found, then returns a reference to the value slot so that you can
assign into it. However, if the value type is a pointer, it will be
initialized to null. This is usually no problem.
However, if the user /knows/ the map contains a value for a particular key,
they may just use it immediately:
// From ClangSACheckersEmitter.cpp
recordGroupMap[group]->Checkers
In this case the analyzer reports a null dereference on the path where the
key is not in the map, even though the user knows that path is impossible
here. They could silence the warning by adding an assertion, but that means
splitting up the expression and introducing a local variable. (Note that
the analyzer has no way of knowing that recordGroupMap[group] will return
the same reference if called twice in a row!)
We already have logic that says a null dereference has a high chance of
being a false positive if the null came from an inlined function. This
patch simply extends that to references whose rvalues are null as well,
silencing several false positives in LLVM.
<rdar://problem/13239854>
llvm-svn: 176371
By returning the (key, value) binding pairs, we save lookups afterwards.
This also enables further work later on.
No functionality change.
llvm-svn: 176230
Consider this case:
int *p = 0;
p = getPointerThatMayBeNull();
*p = 1;
If we inline 'getPointerThatMayBeNull', we might know that the value of 'p'
is NULL, and thus emit a null pointer dereference report. However, we
usually want to suppress such warnings as error paths, and we do so by using
FindLastStoreBRVisitor to see where the NULL came from. In this case, though,
because 'p' was NULL both before and after the assignment, the visitor
would decide that the "last store" was the initialization, not the
re-assignment.
This commit changes FindLastStoreBRVisitor to consider all PostStore nodes
that assign to this region. This still won't catches changes made directly
by checkers if they re-assign the same value, but it does handle the common
case in user-written code and will trigger ReturnVisitor's suppression
machinery as expected.
<rdar://problem/13299738>
llvm-svn: 176201
This enables constructor inlining for types with non-trivial destructors.
The plan is to enable destructor inlining within the next month, but that
needs further verification.
<rdar://problem/12295329>
llvm-svn: 176200
This potentially reduces a performance optimization of throwing away
PreStmtPurgeDeadSymbols nodes. I'll investigate the performance impact
soon and see if we need something better.
llvm-svn: 176149
This is essentially the same problem as r174031: a lazy binding for the first
field of a struct may stomp on an existing default binding for the
entire struct. Because of the way RegionStore is set up, we can't help
but lose the top-level binding, but then we need to make sure that accessing
one of the other fields doesn't come back as Undefined.
In this case, RegionStore is now correctly detecting that the lazy binding
we have isn't the right type, but then failing to follow through on the
implications of that: we don't know anything about the other fields in the
aggregate. This fix adds a test when searching for other kinds of default
values to see if there's a lazy binding we rejected, and if so returns
a symbolic value instead of Undefined.
The long-term fix for this is probably a new Store model; see
<rdar://problem/12701038>.
Fixes <rdar://problem/13292559>.
llvm-svn: 176144
Fixes PR15358 and <rdar://problem/13295437>.
Along the way, shorten path diagnostics that say "Variable 'x'" to just
be "'x'". By the context, it is obvious that we have a variable,
and so this just consumes text space.
llvm-svn: 176115
Normally, we need to look through derived-to-base casts when creating
temporary object regions (added in r175854). However, if the temporary
is a pointer (rather than a struct/class instance), we need to /preserve/
the base casts that have been applied.
This also ensures that we really do create a new temporary region when
we need to: MaterializeTemporaryExpr and lvalue CXXDefaultArgExprs.
Fixes PR15342, although the test case doesn't include the crash because
I couldn't isolate it.
llvm-svn: 176069
With the new support for trivial copy constructors, we are not always
consistent about whether a CXXTempObjectRegion gets reused or created
from scratch, which affects whether qualifiers are preserved. However,
we probably don't care anyway.
This also switches to using the current PrintingPolicy for the type,
which means C++ types don't get a spurious 'struct' prefix anymore.
llvm-svn: 176068
This addresses a case when we inline a wrong method due to incorrect
dynamic type inference. Specifically, when user code contains a method from init
family, which creates an instance of another class.
Use hasRelatedResultType() to find out if our inference rules should be triggered.
llvm-svn: 176054
These nodes are never consulted by any analyzer client code, so they are
used only for machinery for removing dead bindings. Once successor nodes
are generated they can be safely removed.
This greatly reduces the amount of nodes that are generated in some case,
lowering the memory regression when analyzing Sema.cpp introduced by
r176010 from 14% to 2%.
llvm-svn: 176050
r175026 added support for default values, but didn't take reference
parameters into account, which expect the default argument to be an
lvalue. Use createTemporaryRegionIfNeeded if we can evaluate the default
expr as an rvalue but the expected result is an lvalue.
Fixes the most recent report of PR12915. The original report predates
default argument support, so that can't be it.
llvm-svn: 176042
While RegionStore checks to make sure casts on TypedValueRegions are valid,
it does not do the same for SymbolicRegions, which do not have perfect type
info anyway. Additionally, MemRegion::getAsOffset does not take a
ProgramState, so it can't use dynamic type info to determine a better type
for the regions. (This could also be dangerous if the type of a super-region
changes!)
Account for this by checking that a base object region is valid on top of a
symbolic region, and falling back to "symbolic offset" mode if not.
Fixes PR15345.
llvm-svn: 176034
This was triggering assertion failures when analyzing the LLVM codebase. This
is fallout from r175988.
I've got delta chewing away on a test case, but I wanted the fix to go
in now.
llvm-svn: 176011
r175988 modified the ExplodedGraph trimming algorithm to retain all
nodes for "lvalue" expressions. This patch refines that notion to
only "interesting" expressions that would be used for diagnostics.
llvm-svn: 176010
This required more changes than I originally expected:
- ObjCIvarRegion implements "canPrintPretty" et al
- DereferenceChecker indicates the null pointer source is an ivar
- bugreporter::trackNullOrUndefValue() uses an alternate algorithm
to compute the location region to track by scouring the ExplodedGraph.
This allows us to get the actual MemRegion for variables, ivars,
fields, etc. We only hand construct a VarRegion for C++ references.
- ExplodedGraph no longer drops nodes for expressions that are marked
'lvalue'. This is to facilitate the logic in the previous bullet.
This may lead to a slight increase in size in the ExplodedGraph,
which I have not measured, but it is likely not to be a big deal.
I have validated each of the changed plist output.
Fixes <rdar://problem/12114812>
llvm-svn: 175988
Use Optional<CFG*> where invalid states were needed previously. In the one case
where that's not possible (beginAutomaticObjDtorsInsert) just use a dummy
CFGAutomaticObjDtor.
Thanks for the help from Jordan Rose & discussion/feedback from Ted Kremenek
and Doug Gregor.
Post commit code review feedback on r175796 by Ted Kremenek.
llvm-svn: 175938
This Decl shouldn't be the canonical Decl; it should be the Decl used by
the CXXBaseSpecifier in the subclass. Unfortunately, that means continuing
to throw getCanonicalDecl() on all comparisons.
This fixes MemRegion::getAsOffset's use of ASTRecordLayout when redeclarations
are involved.
llvm-svn: 175913
Previously, we had the decisions about inlining spread out
over multiple functions.
In addition to the refactor, this commit ensures
that we will always inline BodyFarm functions as long as the Decl
is available. This fixes false positives due to those functions
not being inlined when no or minimal inlining is enabled such (as
shallow mode).
llvm-svn: 175857
This is a follow-up to r175830, which made sure a temporary object region
created for, say, a struct rvalue matched up with the initial bindings
being stored into it. This does the same for the case in which the AST
actually tells us that we need to create a temporary via a
MaterializeObjectExpr. I've unified the two code paths and moved a static
helper function onto ExprEngine.
This also caused a bit of test churn, causing us to go back to describing
temporary regions without a 'const' qualifier. This seems acceptable; it's
our behavior from a few months ago.
<rdar://problem/13265460> (part 2)
llvm-svn: 175854
When creating a temporary region (say, when a struct rvalue is used as
the base of a member expr), make sure we account for any derived-to-base
casts. We don't actually record these in the LazyCompoundVal that
represents the rvalue, but we need to make sure that the temporary region
we're creating (a) matches the bindings, and (b) matches its expression.
Most of the time this will do exactly the same thing as before, but it
fixes spurious "garbage value" warnings introduced in r175234 by the use
of lazy bindings to model trivial copy constructors.
<rdar://problem/13265460>
llvm-svn: 175830
This allows MemRegion and MemRegionManager to avoid asking over and over
again whether an class is a virtual base or a non-virtual base.
Minor optimization/cleanup; no functionality change.
llvm-svn: 175716
- When deciding if we can reuse a lazy binding, make sure to check if there
are additional bindings in the sub-region.
- When reading from a lazy binding, don't accidentally strip off casts or
base object regions. This slows down lazy binding reading a bit but is
necessary for type sanity when treating one class as another.
A bit of minor refactoring allowed these two checks to be unified in a nice
early-return-using helper function.
<rdar://problem/13239840>
llvm-svn: 175703
RegionStoreManager::getInterestingValues() returns a pointer to a
std::vector that lives inside a DenseMap, which is constructed on demand.
However, constructing one such value can lead to constructing another
value, which will invalidate the reference created earlier.
Fixed by delaying the new entry creation until the function returns.
llvm-svn: 175582
If a base object is at a 0 offset, RegionStoreManager may find a lazy
binding for the entire object, then try to attach a FieldRegion or
grandparent CXXBaseObjectRegion on top of that (skipping the intermediate
region). We now preserve as many layers of base object regions necessary
to make the types match.
<rdar://problem/13239840>
llvm-svn: 175556
This just adds a very simple check that if a DerivedToBase CastExpr is
operating on a value with known C++ object type, and that type is not the
base type specified in the AST, then the cast is invalid and we should
return UnknownVal.
In the future, perhaps we can have a checker that specifies that this is
illegal, but we still shouldn't assert even if the user turns that checker
off.
PR14872
llvm-svn: 175239
...after a host of optimizations related to the use of LazyCompoundVals
(our implementation of aggregate binds).
Originally applied in r173951.
Reverted in r174069 because it was causing hangs.
Re-applied in r174212.
Reverted in r174265 because it was /still/ causing hangs.
If this needs to be reverted again it will be punted to far in the future.
llvm-svn: 175234
Previously, we were scanning the current store. Now, we properly scan the
store that the LazyCompoundVal came from, which may have very different
live symbols.
llvm-svn: 175232
Previously, whenever we had a LazyCompoundVal, we crawled through the
entire store snapshot looking for bindings within the LCV's region. Now, we
just ask for the subregion bindings of the lazy region and only visit those.
This is an optimization (so no test case), but it may allow us to clean up
more dead bindings than we were previously.
llvm-svn: 175230
This is going to be used in the next commit.
While I'm here, tighten up assumptions about symbolic offset
BindingKeys, and make offset calculation explicitly handle all
MemRegion kinds.
No functionality change.
llvm-svn: 175228
In C++, constants captured by lambdas (and blocks) are not actually stored
in the closure object, since they can be expanded at compile time. In this
case, they will have no binding when we go to look them up. Previously,
RegionStore thought they were uninitialized stack variables; now, it checks
to see if they are a constant we know how to evaluate, using the same logic
as r175026.
This particular code path is only for scalar variables. Constant arrays and
structs are still unfortunately unhandled; we'll need a stronger solution
for those.
This may have a small performance impact, but only for truly-undefined
local variables, captures in a non-inlined block, and non-constant globals.
Even then, in the non-constant case we're only doing a quick type check.
<rdar://problem/13105553>
llvm-svn: 175194
Previously, we were handling only simple integer constants for globals and
the smattering of implicitly-valued expressions handled by Environment for
default arguments. Now, we can use any integer constant expression that
Clang can evaluate, in addition to everything we handled before.
PR15094 / <rdar://problem/12830437>
llvm-svn: 175026
The missing definition check should be in the same category as the
missing ivar validation - in this case, the intent is to invalidate in
the given class, as described in the declaration, but the implementation
does not perform the invalidation. Whereas the MissingInvalidationMethod
checker checks the cases where the method intention is not to
invalidate. The second checker has potential to have a much higher false
positive rate.
llvm-svn: 174787
The new annotation allows having methods that only partially invalidate
IVars and might not be called from the invalidation methods directly
(instead, are guaranteed to be called before the invalidation occurs).
The checker is going to trust the programmer to call the partial
invalidation method before the invalidator.This is common in cases when
partial object tear down happens before the death of the object.
llvm-svn: 174779
The malloc checker will now catch the case when a previously malloc'ed
region is freed, but the pointer passed to free does not point to the
start of the allocated memory. For example:
int *p1 = malloc(sizeof(int));
p1++;
free(p1); // warn
From the "memory.LeakPtrValChanged enhancement to unix.Malloc" entry
in the list of potential checkers.
A patch by Branden Archer!
llvm-svn: 174678
The checkPointerEscape callback previously did not specify how a
pointer escaped. This change includes an enum which describes the
different ways a pointer may escape. This enum is passed to the
checkPointerEscape callback when a pointer escapes. If the escape
is due to a function call, the call is passed. This changes
previous behavior where the call is passed as NULL if the escape
was due to indirectly invalidating the region the pointer referenced.
A patch by Branden Archer!
llvm-svn: 174677
This patch makes sure that we do not reinitialize static globals when
the function is called more than once along a path. The motivation is
code with initialization patterns that rely on 2 static variables, where
one of them has an initializer while the other does not. Currently, we
reset the static variables with initializers on every visit to the
function along a path.
llvm-svn: 174676
This is a "quick fix".
The underlining issue is that when a const pointer to a struct is passed
into a function, we do not invalidate the pointer fields. This results
in false positives that are common in C++ (since copy constructors are
prevalent). (Silences two llvm false positives.)
llvm-svn: 174468
Ted Kremenek