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
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
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
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
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
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
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
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
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
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 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
This is a more natural order of evaluation, and it is very important
for visualization in the static analyzer. Within Xcode, the arrows
will not jump from right to left, which looks very visually jarring.
It also provides a more natural location for dataflow-based diagnostics.
Along the way, we found a case in the analyzer diagnostics where we
needed to indicate that a variable was "captured" by a block.
-fsyntax-only timings on sqlite3.c show no visible performance change,
although this is just one test case.
Fixes <rdar://problem/13016513>
llvm-svn: 174447
...again. The problem has not been fixed and our internal buildbot is still
getting hangs.
This reverts r174212, originally applied in r173951, then reverted in r174069.
Will not re-apply until the entire project analyzes successfully on my
local machine.
llvm-svn: 174265
Inlining these functions is essential for correctness. We often have
cases where we do not inline calls. For example, the shallow mode and
when reanalyzing previously inlined ObjC methods as top level.
llvm-svn: 174245
This allows us to keep from chaining LazyCompoundVals in cases like this:
CGRect r = CGRectMake(0, 0, 640, 480);
CGRect r2 = r;
CGRect r3 = r2;
Previously we only made this optimization if the struct did not begin with
an aggregate member, to make sure that we weren't picking up an LCV for
the first field of the struct. But since LazyCompoundVals are typed, we can
make that inference directly by comparing types.
This is a pure optimization; the test changes are to guard against possible
future regressions.
llvm-svn: 174211
It's causing hangs on our internal analyzer buildbot. Will restore after
investigating.
This reverts r173951 / baa7ca1142990e1ad6d4e9d2c73adb749ff50789.
llvm-svn: 174069
This is a hack to work around the fact that we don't track extents for our
default bindings:
CGPoint p;
p.x = 0.0;
p.y = 0.0;
rectParam.origin = p;
use(rectParam.size); // warning: uninitialized value in rectParam.size.width
In this case, the default binding for 'p' gets copied into 'rectParam',
because the 'origin' field is at offset 0 within CGRect. From then on,
rectParam's old default binding (in this case a symbol) is lost.
This patch silences the warning by pretending that lazy bindings are never
made from uninitialized memory, but not only is that not true, the original
default binding is still getting overwritten (see FIXME test cases).
The long-term solution is tracked in <rdar://problem/12701038>
PR14765 and <rdar://problem/12875012>
llvm-svn: 174031
positives.
The includeSuffix was only set on the first iteration through the
function, resulting in invalid regions being produced by getLazyBinding
(ex: zoomRegion.y).
llvm-svn: 174016
Redefine the shallow mode to inline all functions for which we have a
definite definition (ipa=inlining). However, only inline functions that
are up to 4 basic blocks large and cut the max exploded nodes generated
per top level function in half.
This makes shallow faster and allows us to keep inlining small
functions. For example, we would keep inlining wrapper functions and
constructors/destructors.
With the new shallow, it takes 104s to analyze sqlite3, whereas
the deep mode is 658s and previous shallow is 209s.
llvm-svn: 173958
This is faster for the analyzer to process than inlining the constructor
and performing a member-wise copy, and it also solves the problem of
warning when a partially-initialized POD struct is copied.
Before:
CGPoint p;
p.x = 0;
CGPoint p2 = p; <-- assigned value is garbage or undefined
After:
CGPoint p;
p.x = 0;
CGPoint p2 = p; // no-warning
This matches our behavior in C, where we don't see a field-by-field copy.
<rdar://problem/12305288>
llvm-svn: 173951
When the analyzer sees an initializer, it checks if the initializer
contains a CXXConstructExpr. If so, it trusts that the CXXConstructExpr
does the necessary work to initialize the object, and performs no further
initialization.
This patch looks through any implicit wrapping expressions like
ExprWithCleanups to find the CXXConstructExpr inside.
Fixes PR15070.
llvm-svn: 173557
The idea is to introduce a higher level "user mode" option for
different use scenarios. For example, if one wants to run the analyzer
for a small project each time the code is built, they would use
the "shallow" mode.
The user mode option will influence the default settings for the
lower-level analyzer options. For now, this just influences the ipa
modes, but we plan to find more optimal settings for them.
llvm-svn: 173386
The idea is to eventually place all analyzer options under
"analyzer-config". In addition, this lays the ground for introduction of
a high-level analyzer mode option, which will influence the
default setting for IPAMode.
llvm-svn: 173385
Before:
Calling implicit default constructor for 'Foo' (where Foo is constructed)
Entered call from 'test' (at "=default" or 'Foo' declaration)
Calling default constructor for 'Bar' (at "=default" or 'Foo' declaration)
After:
Calling implicit default constructor for 'Foo' (where Foo is constructed)
Calling default constructor for 'Bar' (at "=default" or 'Foo' declaration)
This only affects the plist diagnostics; this note is never shown in the
other diagnostics.
llvm-svn: 172915
Suppress the warning by just not emitting the report. The sink node
would get generated, which is fine since we did reach a bad state.
Motivation
Due to the way code is structured in some of these macros, we do not
reason correctly about it and report false positives. Specifically, the
following loop reports a use-after-free. Because of the way the code is
structured inside of the macro, the analyzer assumes that the list can
have cycles, so you end up with use-after-free in the loop, that is
safely deleting elements of the list. (The user does not have a way to
teach the analyzer about shape of data structures.)
SLIST_FOREACH_SAFE(item, &ctx->example_list, example_le, tmpitem) {
if (item->index == 3) { // if you remove each time, no complaints
assert((&ctx->example_list)->slh_first == item);
SLIST_REMOVE(&ctx->example_list, item, example_s, example_le);
free(item);
}
}
llvm-svn: 172883
The issue here is that if we have 2 leaks reported at the same line for
which we cannot print the corresponding region info, they will get
treated as the same by issue_hash+description. We need to AUGMENT the
issue_hash with the allocation info to differentiate the two issues.
Add the "hash" (offset from the beginning of a function) representing
allocation site to solve the issue.
We might want to generalize solution in the future when we decide to
track more than just the 2 locations from the diagnostics.
llvm-svn: 171825
Instead of using several callbacks to identify the pointer escape event,
checkers now can register for the checkPointerEscape.
Converted the Malloc checker to use the new callback.
SimpleStreamChecker will be converted next.
llvm-svn: 170625
performance heuristic
After inlining a function with more than 13 basic blocks 32 times, we
are not going to inline it anymore. The idea is that inlining large
functions leads to drastic performance implications. Since the function
has already been inlined, we know that we've analyzed it in many
contexts.
The following metrics are used:
- Large function is a function with more than 13 basic blocks (we
should switch to another metric, like cyclomatic complexity)
- We consider that we've inlined a function many times if it's been
inlined 32 times. This number is configurable with -analyzer-config
max-times-inline-large=xx
This heuristic addresses a performance regression introduced with
inlining on one benchmark. The analyzer on this benchmark became 60
times slower with inlining turned on. The heuristic allows us to analyze
it in 24% of the time. The performance improvements on the other
benchmarks I've tested with are much lower - under 10%, which is
expected.
llvm-svn: 170361
We don't handle array destructors correctly yet, but we now apply the same
hack (explicitly destroy the first element, implicitly invalidate the rest)
for multidimensional arrays that we already use for linear arrays.
<rdar://problem/12858542>
llvm-svn: 170000
top level.
This heuristic is already turned on for non-ObjC methods
(inlining-mode=noredundancy). If a method has been previously analyzed,
while being inlined inside of another method, do not reanalyze it as top
level.
This commit applies it to ObjCMethods as well. The main caveat here is
that to catch the retain release errors, we are still going to reanalyze
all the ObjC methods but without inlining turned on.
Gives 21% performance increase on one heavy ObjC benchmark, which
suffered large performance regressions due to ObjC inlining.
llvm-svn: 169639
This is the case where the analyzer tries to print out source locations
for code within a synthesized function body, which of course does not have
a valid source location. The previous fix attempted to do this during
diagnostic path pruning, but some diagnostics have pruning disabled, and
so any diagnostic with a path that goes through a synthesized body will
either hit an assertion or emit invalid output.
<rdar://problem/12657843> (again)
llvm-svn: 169631
This reduces analysis time by 1.2% on one test case (Objective-C), but
also cleans up some of the code conceptually as well. We can possible
just make RegionBindingsRef -> RegionBindings, but I wanted to stage
things.
After this, we should revisit Jordan's optimization of not canonicalizing
the immutable AVL trees for the cluster bindings as well.
llvm-svn: 169571
Previously we would search for the last statement, then back up to the
entrance of the block that contained that statement. Now, while we're
scanning for the statement, we just keep track of which blocks are being
exited (in reverse order).
llvm-svn: 169526
This doesn't seem to make much of a difference in practice, but it does
have the potential to avoid a trip through the constraint manager.
llvm-svn: 169524
Whenever we touch a single bindings cluster multiple times, we can delay
canonicalizing it until the final access. This has some interesting
implications, in particular that we shouldn't remove an /empty/ cluster
from the top-level map until canonicalization.
This is good for a 2% speedup or so on the test case in
<rdar://problem/12810842>
llvm-svn: 169523
This feature was probably intended to improve diagnostics, but was currently
only used when dumping the Environment. It shows what location a given value
was loaded from, e.g. when evaluating an LValueToRValue cast.
llvm-svn: 169522
uncovered.
This required manually correcting all of the incorrect main-module
headers I could find, and running the new llvm/utils/sort_includes.py
script over the files.
I also manually added quite a few missing headers that were uncovered by
shuffling the order or moving headers up to be main-module-headers.
llvm-svn: 169237
The stop-gap here is to just drop such objects when processing the InitListExpr.
We still need a better solution.
Fixes <rdar://problem/12755044>.
llvm-svn: 168757
This was also covered by <rdar://problem/12753384>. The static analyzer
evaluates a CXXConstructExpr within an initializer expression and
RegionStore doesn't know how to handle the resulting CXXTempObjectRegion
that gets created. We need a better solution than just dropping the
value, but we need to better understand how to implement the right
semantics here.
Thanks to Jordan for his help diagnosing the behavior here.
llvm-svn: 168741
The AllocaRegion did not have the superRegion (based on LocationContext)
as part of it's hash. As a consequence, the AllocaRegions from
different frames were uniqued to be the same region.
llvm-svn: 168599
In code like this:
void foo() {
bar();
baz();
}
...the location for the call to 'bar()' was being used as a backup location
for the call to 'baz()'. This is fine unless the call to 'bar()' is deemed
uninteresting and that part of the path deleted.
(This looks like a logic error as well, but in practice the only way 'baz()'
could have an invalid location is if the entire body of 'foo()' is
synthesized, meaning the call to 'bar()' will be using the location of the
call to 'foo()' anyway. Nevertheless, the new version better matches the
intent of the code.)
Found by Matt Beaumont-Gay using ASan. Thanks, Matt!
llvm-svn: 168080
This fixes a few cases where we'd emit path notes like this:
+---+
1| v
p = malloc(len);
^ |2
+---+
In general this should make path notes more consistent and more correct,
especially in cases where the leak happens on the false branch of an if
that jumps directly to the end of the function. There are a couple places
where the leak is reported farther away from the cause; these are usually
cases where there are several levels of nested braces before the end of
the function. This still matches our current behavior for when there /is/
a statement after all the braces, though.
llvm-svn: 168070
This allows us to properly remove dead bindings at the end of the top-level
stack frame, using the ReturnStmt, if there is one, to keep the return value
live. This in turn removes the need for a check::EndPath callback in leak
checkers.
This does cause some changes in the path notes for leak checkers. Previously,
a leak would be reported at the location of the closing brace in a function.
Now, it gets reported at the last statement. This matches the way leaks are
currently reported for inlined functions, but is less than ideal for both.
llvm-svn: 168066
We do this by using the "most recent" good location: if a synthesized
function 'A' calls another function 'B', the path notes for the call to 'B'
will be placed at the same location as the path note for calling 'A'.
Similarly, the call to 'A' will have a note saying "Entered call from...",
and now we just don't emit that (since the user doesn't have a body to look
at anyway).
Previously, we were doing this for the "Calling..." notes, but not for the
"Entered call from..." or "Returning to caller". This caused a crash when
the path entered and then exiting a call within a synthesized body.
<rdar://problem/12657843>
llvm-svn: 168019
conditions.
The adjustment is needed only in case of dynamic dispatch performed by
the analyzer - when the runtime declaration is different from the static
one.
Document this explicitly in the code (by adding a helper). Also, use
canonical Decls to avoid matching against the case where the definition
is different from found declaration.
This fix suppresses the testcase I added in r167762, so add another
testcase to make sure we do test commit r167762.
llvm-svn: 167780
Suppresses a leak false positive (radar://12663777).
In addition, we'll need to rewrite the adjustReturnValue() method not to
return UnknownVal by default, but rather assert in cases we cannot
handle. To make it possible, we need to correctly handle some of the
edge cases we already know about.
llvm-svn: 167762
Previously, RegionStore was being VERY conservative in saying that because
p[i].x and p[i].y have a concrete base region of 'p', they might overlap.
Now, we check the chain of fields back up to the base object and check if
they match.
This only kicks in when dealing with symbolic offset regions because
RegionStore's "base+offset" representation of concrete offset regions loses
all information about fields. In cases where all offsets are concrete
(s.x and s.y), RegionStore will already do the right thing, but mixing
concrete and symbolic offsets can cause bindings to be invalidated that
are known to not overlap (e.g. p[0].x and p[i].y).
This additional refinement is tracked by <rdar://problem/12676180>.
<rdar://problem/12530149>
llvm-svn: 167654
As Anna pointed out, ProgramStateTrait.h is a relatively obscure header,
and checker writers may not know to look there to add their own custom
state.
The base macro that specializes the template remains in ProgramStateTrait.h
(REGISTER_TRAIT_WITH_PROGRAMSTATE), which allows the analyzer core to keep
using it.
llvm-svn: 167385
This will simplify checkers that need to register for leaks. Currently,
they have to register for both: check dead and check end of path.
I've modified the SymbolReaper to consider everything on the stack dead
if the input StackLocationContext is 0.
(This is a bit disruptive, so I'd like to flash out all the issues
asap.)
llvm-svn: 167352
These are CallEvent-equivalents of helpers already accessible in
CheckerContext, as part of making it easier for new checkers to be written
using CallEvent rather than raw CallExprs.
llvm-svn: 167338
Also, move the REGISTER_*_WITH_PROGRAMSTATE macros to ProgramStateTrait.h.
This doesn't get rid of /all/ explicit uses of ProgramStatePartialTrait,
but it does get a lot of them.
llvm-svn: 167276
Previously, every call to a ConstraintManager's isNull would do a full
assumeDual to test feasibility. Now, ConstraintManagers can override
checkNull if they have a cheaper way to do the same thing.
RangeConstraintManager can do this in less than half the work.
<rdar://problem/12608209>
llvm-svn: 167138
Our one basic suppression heuristic is to assume that functions do not
usually return NULL. However, when one of the arguments is NULL it is
suddenly much more likely that NULL is a valid return value. In this case,
we don't suppress the report here, but we do attach /another/ visitor to
go find out if this NULL argument also comes from an inlined function's
error path.
This new behavior, controlled by the 'avoid-suppressing-null-argument-paths'
analyzer-config option, is turned off by default. Turning it on produced
two false positives and no new true positives when running over LLVM/Clang.
This is one of the possible refinements to our suppression heuristics.
<rdar://problem/12350829>
llvm-svn: 166941
Additionally, don't collect PostStore nodes -- they are often used in
path diagnostics.
Previously, we tried to track null arguments in the same way as any other
null values, but in many cases the necessary nodes had already been
collected (a memory optimization in ExplodedGraph). Now, we fall back to
using the value of the argument at the time of the call, which may not
always match the actual contents of the region, but often will.
This is a precursor to improving our suppression heuristic.
<rdar://problem/12350829>
llvm-svn: 166940
path notes for cases where a value may be assumed to be null, etc.
Instead of having redundant diagnostics, do a pass over the generated
PathDiagnostic pieces and remove notes from TrackConstraintBRVisitor
that are already covered by ConditionBRVisitor, whose notes tend
to be better.
Fixes <rdar://problem/12252783>
llvm-svn: 166728
After every 1000 CFGElements processed, the ExplodedGraph trims out nodes
that satisfy a number of criteria for being "boring" (single predecessor,
single successor, and more). Rather than controlling this with a cc1 option,
which can only disable this behavior, we now have an analyzer-config option,
'graph-trim-interval', which can change this interval from 1000 to something
else. Setting the value to 0 disables reclamation.
The next commit relies on this behavior to actually test anything.
llvm-svn: 166528
This is actually required by the C++ standard in
[basic.stc.dynamic.allocation]p3:
If an allocation function declared with a non-throwing
exception-specification fails to allocate storage, it shall return a
null pointer. Any other allocation function that fails to allocate
storage shall indicate failure only by throwing an exception of a type
that would match a handler of type std::bad_alloc.
We don't bother checking for the specific exception type, but just go off
the operator new prototype. This should help with a certain class of lazy
initalization false positives.
<rdar://problem/12115221>
llvm-svn: 166363
This actually looks through several kinds of expression, such as
OpaqueValueExpr and ExprWithCleanups. The idea is that binding and lookup
should be consistent, and so if the environment needs to be modified later,
the code doing the modification will not have to manually look through these
"transparent" expressions to find the real binding to change.
This is necessary for proper updating of struct rvalues as described in
the previous commit.
llvm-svn: 166121
In C++, rvalues that need to have their address taken (for example, to be
passed to a function by const reference) will be wrapped in a
MaterializeTemporaryExpr, which lets CodeGen know to create a temporary
region to store this value. However, MaterializeTemporaryExprs are /not/
created when a method is called on an rvalue struct, even though the 'this'
pointer needs a valid value. CodeGen works around this by creating a
temporary region anyway; now, so does the analyzer.
The analyzer also does this when accessing a field of a struct rvalue.
This is a little unfortunate, since the rest of the struct will soon be
thrown away, but it does make things consistent with the rest of the
analyzer.
This allows us to bring back the assumption that all known 'this' values
are Locs. This is a revised version of r164828-9, reverted in r164876-7.
<rdar://problem/12137950>
llvm-svn: 166120
This was only used by OSAtomicChecker and makes it more
difficult to update values for expressions that the environment
may look through instead (it's not the same as IgnoreParens).
With this gone, we can have bindExpr bind to the inner
expression that getSVal will find.
Groundwork for <rdar://problem/12137950>
llvm-svn: 165866
I believe the removed assert in CheckerManager says it best:
InlineCall is a special hacky callback to allow intrusive
evaluation of the call (which simulates inlining). It is
currently only used by OSAtomicChecker and should go away
at some point.
OSAtomicChecker has gone away; inlineCall can now go away as well!
llvm-svn: 165865
This time, actually uncomment the code that's supposed to fix the problem.
This reverts r165671 / 8ceb837585ed973dc36fba8dfc57ef60fc8f2735.
llvm-svn: 165676
Author: Jordan Rose <jordan_rose@apple.com>
Date: Wed Oct 10 21:31:21 2012 +0000
[analyzer] Treat fields of unions as having symbolic offsets.
This allows only one field to be active at a time in RegionStore.
This isn't quite the correct behavior for unions, but it at least
would handle the case of "value goes in, value comes out" from the
same field.
RegionStore currently has a number of places where any access to a union
results in UnknownVal being returned. However, it is clearly missing
some cases, or the original issue wouldn't have occurred. It is probably
now safe to remove those changes, but that's a potentially destabilizing
change that should wait for more thorough testing.
Fixes PR14054.
git-svn-id: https://llvm.org/svn/llvm-project/cfe/trunk@165660 91177308-0d34-0410-b5e6-96231b3b80d8
This reverts commit cf9030e480f77ab349672f00ad302e216c26c92c.
llvm-svn: 165671
This allows only one field to be active at a time in RegionStore.
This isn't quite the correct behavior for unions, but it at least
would handle the case of "value goes in, value comes out" from the
same field.
RegionStore currently has a number of places where any access to a union
results in UnknownVal being returned. However, it is clearly missing
some cases, or the original issue wouldn't have occurred. It is probably
now safe to remove those changes, but that's a potentially destabilizing
change that should wait for more thorough testing.
Fixes PR14054.
llvm-svn: 165660
Some implicit statements, such as the implicit 'self' inserted for "free"
Objective-C ivar access, have invalid source locations. If one of these
statements is the location where an issue is reported, we'll now look at
the enclosing statements for a valid source location.
<rdar://problem/12446776>
llvm-svn: 165354
In C++, overriding virtual methods are allowed to specify a covariant
return type -- that is, if the return type of the base method is an
object pointer type (or reference type), the overriding method's return
type can be a pointer to a subclass of the original type. The analyzer
was failing to take this into account when devirtualizing a method call,
and anything that relied on the return value having the proper type later
would crash.
In Objective-C, overriding methods are allowed to specify ANY return type,
meaning we can NEVER be sure that devirtualizing will give us a "safe"
return value. Of course, a program that does this will most likely crash
at runtime, but the analyzer at least shouldn't crash.
The solution is to check and see if the function/method being inlined is
the function that static binding would have picked. If not, check that
the return value has the same type. If the types don't match, see if we
can fix it with a derived-to-base cast (the C++ case). If we can't,
return UnknownVal to avoid crashing later.
<rdar://problem/12409977>
llvm-svn: 165079
These functions are store-agnostic, and would benefit from information in
DynamicTypeInfo but gain nothing from the store type.
No intended functionality change.
llvm-svn: 165078
table, making it printable with the ConfigDump checker. Along the
way, fix a really serious bug where the value was getting parsed
from the string in code that was in an assert() call. This means
in a Release-Asserts build this code wouldn't work as expected.
llvm-svn: 165041
By analogy with C structs, this seems to be legal, if probably discouraged.
It's only if the ivar is read from or written to that there's a problem.
Running a program that gets the "address" of an instance variable does in
fact return the offset when the base "object" is nil.
This isn't a full revert because r164442 includes some diagnostic tweaks
as well; those have been kept.
This partially reverts r164442 / 08965091770c9b276c238bac2f716eaa4da2dca4.
llvm-svn: 164960
The original intent of this commit was to catch potential null dereferences
early, but it breaks the common "home-grown offsetof" idiom (PR13927):
(((struct Foo *)0)->member - ((struct foo *)0))
As it turns out, this appears to be legal in C, per a footnote in
C11 6.5.3.2: "Thus, &*E is equivalent to E (even if E is a null pointer)".
In C++ this issue is still open:
http://www.open-std.org/jtc1/sc22/wg21/docs/cwg_active.html#232
We'll just have to make sure we have good path notes in the future.
This reverts r164441 / 9be016dcd1ca3986873a7b66bd4bc027309ceb59.
llvm-svn: 164958
string in the config table so that it can be dumped as part of the
config dumper. Add a test to show that these options are sticking
and can be cross-checked using FileCheck.
llvm-svn: 164954
This is related to but not blocked by <rdar://problem/12137950>
("Return-by-value structs do not have associated regions")
This reverts r164875 / 3278d41e17749dbedb204a81ef373499f10251d7.
llvm-svn: 164952
It is possible and valid to have a state manager and associated objects
without having a SubEngine or checkers.
Patch by Olaf Krzikalla!
llvm-svn: 164947
Previously the analyzer treated all inlined constructors like lvalues,
setting the value of the CXXConstructExpr to the newly-constructed
region. However, some CXXConstructExprs behave like rvalues -- in
particular, the implicit copy constructor into a pass-by-value argument.
In this case, we want only the /contents/ of a temporary object to be
passed, so that we can use the same "copy each argument into the
parameter region" algorithm that we use for scalar arguments.
This may change when we start modeling destructors of temporaries,
but for now this is the last part of <rdar://problem/12137950>.
llvm-svn: 164830
An rvalue has no address, but calling a C++ member function requires a
'this' pointer. This commit makes the analyzer create a temporary region
in which to store the struct rvalue and use as a 'this' pointer whenever
a member function is called on an rvalue, which is essentially what
CodeGen does.
More of <rdar://problem/12137950>. The last part is tracking down the
C++ FIXME in array-struct-region.cpp.
llvm-svn: 164829
Struct rvalues are represented in the analyzer by CompoundVals,
LazyCompoundVals, or plain ConjuredSymbols -- none of which have associated
regions. If the entire structure is going to persist, this is not a
problem -- either the rvalue will be assigned to an existing region, or
a MaterializeTemporaryExpr will be present to create a temporary region.
However, if we just need a field from the struct, we need to create the
temporary region ourselves.
This is inspired by the way CodeGen handles calls to temporaries;
support for that in the analyzer is coming next.
Part of <rdar://problem/12137950>
llvm-svn: 164828
Previously, we'd just keep constraints around forever, which means we'd
never be able to merge paths that differed only in constraints on dead
symbols.
Because we now allow constraints on symbolic expressions, not just single
symbols, this requires changing SymExpr::symbol_iterator to include
intermediate symbol nodes in its traversal, not just the SymbolData leaf
nodes.
This depends on the previous commit to be correct. Originally applied in
r163444, reverted in r164275, now being re-applied.
llvm-svn: 164622
No tests, but this allows the optimization of removing dead constraints.
We can then add tests that we don't do this prematurely.
<rdar://problem/12333297>
Note: the added FIXME to investigate SymbolRegionValue liveness is
tracked by <rdar://problem/12368183>. This patch does not change the
existing behavior.
llvm-svn: 164621
This is a heuristic intended to greatly reduce the number of false
positives resulting from inlining, particularly inlining of generic,
defensive C++ methods that live in header files. The suppression is
triggered in the cases where we ask to track where a null pointer came
from, and it turns out that the source of the null pointer was an inlined
function call.
This change brings the number of bug reports in LLVM from ~1500 down to
around ~300, a much more manageable number. Yes, some true positives may
be hidden as well, but from what I looked at the vast majority of silenced
reports are false positives, and many of the true issues found by the
analyzer are still reported.
I'm hoping to improve this heuristic further by adding some exceptions
next week (cases in which a bug should still be reported).
llvm-svn: 164449
Before, PathDiagnosticConsumers that did not support actual path output
would (sensibly) cause the generation of the full path to be skipped.
However, BugReporterVisitors may want to see the path in order to mark a
BugReport as invalid.
Now, even for a path generation scheme of 'None' we will still create a
trimmed graph and walk backwards through the bug path, doing no work other
than passing the nodes to the BugReporterVisitors. This isn't cheap, but
it's necessary to properly do suppression when the first path consumer does
not support path notes.
In the future, we should try only generating the path and visitor-provided
path notes once, or at least only creating the trimmed graph once.
llvm-svn: 164447
This is intended to allow visitors to make decisions about whether a
BugReport is likely a false positive. Currently there are no visitors
making use of this feature, so there are no tests.
When a BugReport is marked invalid, the invalidator must provide a key
that identifies the invaliation (intended to be the visitor type and a
context pointer of some kind). This allows us to reverse the decision
later on. Being able to reverse a decision about invalidation gives us more
flexibility, and allows us to formulate conditions like "this report is
invalid UNLESS the original argument is 'foo'". We can use this to
fine-tune our false-positive suppression (coming soon).
llvm-svn: 164446
Rather than saying "Null pointer value stored to 'foo'", we now say
"Passing null pointer value via Nth parameter 'foo'", which is much better.
The note is also now on the argument expression as well, rather than the
entire call.
This paves the way for continuing to track arguments back to their sources.
<rdar://problem/12211490>
llvm-svn: 164444
Like with struct fields, we want to catch cases like this early,
so that we can produce better diagnostics and path notes:
PointObj *p = nil;
int *px = &p->_x; // should warn here
*px = 1;
llvm-svn: 164442
We want to catch cases like this early, so that we can produce better
diagnostics and path notes:
Point *p = 0;
int *px = &p->x; // should warn here
*px = 1;
llvm-svn: 164441
their implementations are unavailable. Start by simulating dispatch_sync().
This change is largely a bunch of plumbing around something very simple. We
use AnalysisDeclContext to conjure up a fake function body (using the
current ASTContext) when one does not exist. This is controlled
under the analyzer-config option "faux-bodies", which is off by default.
The plumbing in this patch is largely to pass the necessary machinery
around. CallEvent needs the AnalysisDeclContextManager to get
the function definition, as one may get conjured up lazily.
BugReporter and PathDiagnosticLocation needed to be relaxed to handle
invalid locations, as the conjured body has no real source locations.
We do some primitive recovery in diagnostic generation to generate
some reasonable locations (for arrows and events), but it can be
improved.
llvm-svn: 164339
While we definitely want this optimization in the future, we're not
currently handling constraints on symbolic /expressions/ correctly.
These should stay live even if the SymExpr itself is no longer referenced
because could recreate an identical SymExpr later. Only once the SymExpr
can no longer be recreated -- i.e. a component symbol is dead -- can we
safely remove the constraints on it.
This liveness issue is tracked by <rdar://problem/12333297>.
This reverts r163444 / 24c7f98828e039005cff3bd847e7ab404a6a09f8.
llvm-svn: 164275
in ObjCMethods.
Extend FunctionTextRegion to represent ObjC methods as well as
functions. Note, it is not clear what type ObjCMethod region should
return. Since the type of the FunctionText region is not currently used,
defer solving this issue.
llvm-svn: 164046
Anna Zaks