If we get an item from a dictionary, we know that the item is non-null
if and only if the key is non-null.
This patch is a rather hacky way to record this implication, because
some logic needs to be duplicated from the solver.
And yet, it's pretty simple, performant, and works.
Other possible approaches:
- Record the implication, in future rely on Z3 to pick it up.
- Generalize the current code and move it to the constraint manager.
rdar://34990742
Differential Revision: https://reviews.llvm.org/D50124
llvm-svn: 339482
Lambdas can affect static locals even without an explicit capture.
rdar://39537031
Differential Revision: https://reviews.llvm.org/D50368
llvm-svn: 339459
This patch fixed an issue where the dynamic type of pointer/reference
object was known by the analyzer, but wasn't obtained in the checker,
which resulted in false negatives. This should also increase reliability
of the checker, as derefencing is always done now according to the
dynamic type (even if that happens to be the same as the static type).
Special thanks to Artem Degrachev for setting me on the right track.
Differential Revision: https://reviews.llvm.org/D49199
llvm-svn: 339240
As of now, all constructor calls are ignored that are being called
by a constructor. The point of this was not to analyze the fields
of an object, so an uninitialized field wouldn't be reported
multiple times.
This however introduced false negatives when the two constructors
were in no relation to one another -- see the test file for a neat
example for this with singletons. This patch aims so fix this issue.
Differential Revision: https://reviews.llvm.org/D48436
llvm-svn: 339237
Summary:
The loop-widening code processes c++ methods looking for `this` pointers. In
the case of static methods (which do not have `this` pointers), an assertion
was triggering. This patch avoids trying to process `this` pointers for
static methods, and thus avoids triggering the assertion .
Reviewers: dcoughlin, george.karpenkov, NoQ
Reviewed By: NoQ
Subscribers: NoQ, xazax.hun, szepet, a.sidorin, mikhail.ramalho, cfe-commits
Differential Revision: https://reviews.llvm.org/D50408
llvm-svn: 339201
Even for a checker being in alpha, some reports about pointees held so little
value to the user that it's safer to disable pointer/reference chasing for now.
It can be enabled with a new flag, in which case checker should function as it
has always been. This can be set with `CheckPointeeInitialization`.
Differential Revision: https://reviews.llvm.org/D49438
llvm-svn: 339135
Some checkers require ASTContext. Having it in the constructor saves a
lot of boilerplate of having to pass it around.
Differential Revision: https://reviews.llvm.org/D50111
llvm-svn: 339079
For InnerPointerChecker to function properly, both the checker itself
and parts of MallocChecker that handle relevant use-after-free problems
need to be turned on. So far, the latter part has been developed within
MallocChecker's NewDelete sub-checker, often causing warnings to appear
under that name. This patch defines a new CheckKind within MallocChecker
for the inner pointer checking functionality, so that the correct name
is displayed in warnings and in the ExplodedGraph.
Tested on clang-tidy.
Differential Review: https://reviews.llvm.org/D50211
llvm-svn: 339067
Objects local to a function are destroyed right after the statement returning
(part of) them is executed in the analyzer. This patch enables MallocChecker to
warn in these cases.
Differential Revision: https://reviews.llvm.org/D49361
llvm-svn: 338780
The CoreEngine only gives us a ReturnStmt if the last element in the
CFGBlock is a CFGStmt, otherwise the ReturnStmt is nullptr.
This patch adds support for the case when the last element is a
CFGAutomaticObjDtor, by returning its TriggerStmt as a ReturnStmt.
Differential Revision: https://reviews.llvm.org/D49811
llvm-svn: 338777
Newly added methods allow reasoning about the stack frame of the call (as
opposed to the stack frame on which the call was made, which was always
available) - obtain the stack frame context, obtain parameter regions - even if
the call is not going to be (or was not) inlined, i.e. even if the analysis
has never actually entered the stack frame.
Differential Revision: https://reviews.llvm.org/D49715
llvm-svn: 338474
Because of incomplete support for CXXDefaultArgExpr, we cannot yet commit to
asserting that the same destructor won't be elided twice.
Suppress the assertion failure for now. Proper support is still an open problem.
Differential Revision: https://reviews.llvm.org/D49213
llvm-svn: 338441
This is a refactoring patch; no functional change intended.
The common part of ConstructionContextLayer and ConstructedObjectKey is
factored out into a new structure, ConstructionContextItem.
Various sub-kinds of ConstructionContextItem are enumerated in order to
provide richer information about construction contexts.
Differential Revision: https://reviews.llvm.org/D49210.
llvm-svn: 338439
In r330377 and r338425 we have already identified what constitutes function
argument constructors and added stubs in order to prevent confusing them
with other temporary object constructors.
Now we implement a ConstructionContext sub-class to carry all the necessary
information about the construction site, namely call expression and argument
index.
On the analyzer side, the patch interacts with the recently implemented
pre-C++17 copy elision support in an interesting manner. If on the CFG side we
didn't find a construction context for the elidable constructor, we build
the CFG as if the elidable constructor is not elided, and the non-elided
constructor within it is a simple temporary. But the same problem may occur
in the analyzer: if the elidable constructor has a construction context but
the analyzer doesn't implement such context yet, the analyzer should also
try to skip copy elision and still inline the non-elided temporary constructor.
This was implemented by adding a "roll back" mechanism: when elision fails,
roll back the changes and proceed as if it's a simple temporary. The approach
is wonky, but i'm fine with that as long as it's merely a defensive mechanism
that should eventually go away once all construction contexts become supported.
Differential Revision: https://reviews.llvm.org/D48681.
llvm-svn: 338436
This fix is similar to r337769 and addresses a regression caused by r337167.
When an operation between a nonloc::LocAsInteger and a non-pointer symbol
is performed, the LocAsInteger-specific part of information is lost.
When the non-pointer symbol is collapsing into a constant, we cannot easily
re-evaluate the result, because we need to recover the missing
LocAsInteger-specific information (eg., integer type, or the very fact that
this pointer was at some point converted to an integer).
Add one more defensive check to prevent crashes on trying to simplify a
SymSymExpr with different Loc-ness of operands.
Differential Revision:
llvm-svn: 338420
When emitting a bug report, it is important to highlight which argument of the
call-expression is causing the problem.
Before:
warning: Null pointer argument in call to string comparison function
strcmp(a, b);
^~~~~~~~~~~~
After:
warning: Null pointer argument in call to string comparison function
strcmp(a, b);
^ ~
Affects other output modes as well, not just text.
Differential Revision: https://reviews.llvm.org/D50028
llvm-svn: 338333
Do not warn when the other message-send-expression is correctly wrapped
in a different autorelease pool.
Differential Revision: https://reviews.llvm.org/D49921
llvm-svn: 338314
After cleaning up program state maps in `checkDeadSymbols()`,
a transition should be added to generate the new state.
Differential Revision: https://reviews.llvm.org/D47417
llvm-svn: 338263
According to the standard, pointers referring to the elements of a
`basic_string` may be invalidated if they are used as an argument to
any standard library function taking a reference to non-const
`basic_string` as an argument. This patch makes InnerPointerChecker warn
for these cases.
Differential Revision: https://reviews.llvm.org/D49656
llvm-svn: 338259
The analyzer may consider a container region as dead while it still has live
iterators. We must defer deletion of the data belonging to such containers
until all its iterators are dead as well to be able to compare the iterator
to the begin and the end of the container which is stored in the container
data.
Differential Revision: https://reviews.llvm.org/D48427
llvm-svn: 338234
The note is added in the following situation:
- We are throwing a nullability-related warning on an IVar
- The path goes through a method which *could have* (syntactically
determined) written into that IVar, but did not
rdar://42444460
Differential Revision: https://reviews.llvm.org/D49689
llvm-svn: 338149
Summary:
This patch replaces the current method of getting an `APSInt` from Z3's model by calling generic API method `getBitvector` instead of `Z3_get_numeral_uint64`.
By calling `getBitvector`, there's no need to handle bitvectors with bit width == 128 separately.
And, as a bonus, clang now compiles correctly with Z3 4.7.1.
Reviewers: NoQ, george.karpenkov
Reviewed By: george.karpenkov
Subscribers: xazax.hun, szepet, a.sidorin
Differential Revision: https://reviews.llvm.org/D49818
llvm-svn: 338020
Summary:
Update the documentation of all the classes introduced with the new generic SMT API, most of them were referencing Z3 and how previous operations were being done (like including the context as parameter in a few methods).
Renamed the following methods, so it's clear that the operate on bitvectors:
*`mkSignExt` -> `mkBVSignExt`
*`mkZeroExt` -> `mkBVZeroExt`
*`mkExtract` -> `mkBVExtract`
*`mkConcat` -> `mkBVConcat`
Removed the unecessary methods:
* `getDataExpr`: it was an one line method that called `fromData`
* `mkBitvector(const llvm::APSInt Int)`: it was not being used anywhere
Reviewers: NoQ, george.karpenkov
Reviewed By: george.karpenkov
Subscribers: xazax.hun, szepet, a.sidorin
Differential Revision: https://reviews.llvm.org/D49799
llvm-svn: 337954
Summary:
The macro was manually expanded in the Z3 backend and this patch adds it back.
Adding the expanded code is dangerous as the macro may change in the future and the expanded code might be left outdated.
Reviewers: NoQ, george.karpenkov
Reviewed By: george.karpenkov
Subscribers: xazax.hun, szepet, a.sidorin
Differential Revision: https://reviews.llvm.org/D49769
llvm-svn: 337923
Summary:
Third patch in the refactoring series, to decouple the SMT Solver from the Refutation Manager (1st: D49668, 2nd: D49767).
The refutation API in the `SMTConstraintManager` was a hack to allow us to create an SMT solver and verify the constraints; it was conceptually wrong from the start. Now, we don't actually need to use the `SMTConstraintManager` and can create an SMT object directly, add the constraints and check them.
While updating the Falsification visitor, I inlined the two functions that were used to collect the constraints and add them to the solver.
As a result of this patch, we could move the SMT API elsewhere and as it's not really dependent on the CSA anymore. Maybe we can create a new dir (utils/smt) for Z3 and future solvers?
Reviewers: NoQ, george.karpenkov
Reviewed By: george.karpenkov
Subscribers: xazax.hun, szepet, a.sidorin
Differential Revision: https://reviews.llvm.org/D49768
llvm-svn: 337922
Summary:
This is the second part of D49668, and moves all the code that's not specific to a ConstraintManager to SMTSolver.
No functional change intended.
Reviewers: NoQ, george.karpenkov
Reviewed By: george.karpenkov
Subscribers: xazax.hun, szepet, a.sidorin
Differential Revision: https://reviews.llvm.org/D49767
llvm-svn: 337921
Summary:
This patch changes how the SMT bug refutation runs in an equivalent bug report class.
Now, all other visitor are executed until they find a valid bug or mark all bugs as invalid. When the one valid bug is found (and crosscheck is enabled), the SMT refutation checks the satisfiability of this single bug.
If the bug is still valid after checking with Z3, it is returned and a bug report is created. If the bug is found to be invalid, the next bug report in the equivalent class goes through the same process, until we find a valid bug or all bugs are marked as invalid.
Massive speedups when verifying redis/src/rax.c, from 1500s to 10s.
Reviewers: NoQ, george.karpenkov
Reviewed By: george.karpenkov
Subscribers: xazax.hun, szepet, a.sidorin
Differential Revision: https://reviews.llvm.org/D49693
llvm-svn: 337920
Summary:
This patch moves a lot of code from `Z3ConstraintManager` to `SMTConstraintManager`, leaving only the necessary:
* `canReasonAbout` which returns if a Solver can handle a given `SVal` (should be moved to `SMTSolver` in the future).
* `removeDeadBindings`, `assumeExpr` and `print`: methods that need to use `ConstraintZ3Ty`, can probably be moved to `SMTConstraintManager` in the future.
The patch creates a new file, `SMTConstraintManager.cpp` with the moved code. Conceptually, this is move in the right direction and needs further improvements: `SMTConstraintManager` still does a lot of things that are not required by a `ConstraintManager`.
We ought to move the unrelated to `SMTSolver` and remove everything that's not related to a `ConstraintManager`. In particular, we could remove `addRangeConstraints` and `isModelFeasible`, and make the refutation manager create an Z3Solver directly.
Reviewers: NoQ, george.karpenkov
Reviewed By: george.karpenkov
Subscribers: mgorny, xazax.hun, szepet, a.sidorin
Differential Revision: https://reviews.llvm.org/D49668
llvm-svn: 337919
Summary:
Created new SMT generic API.
Small changes to `Z3ConstraintManager` because of the new generic objects (`SMTSort` and `SMTExpr`) returned by `SMTSolver`.
Reviewers: george.karpenkov, NoQ
Reviewed By: george.karpenkov
Subscribers: mgorny, xazax.hun, szepet, a.sidorin
Differential Revision: https://reviews.llvm.org/D49495
llvm-svn: 337918
Summary:
New base class for all future SMT Exprs.
No major changes except moving `areEquivalent` and `getFloatSemantics` outside of `Z3Expr` to keep the class minimal.
Reviewers: NoQ, george.karpenkov
Reviewed By: george.karpenkov
Subscribers: xazax.hun, szepet, a.sidorin
Differential Revision: https://reviews.llvm.org/D49551
llvm-svn: 337917
Summary:
New base class for all future SMT sorts.
The only change is that the class implements methods `isBooleanSort()`, `isBitvectorSort()` and `isFloatSort()` so it doesn't rely on `Z3`'s enum.
Reviewers: NoQ, george.karpenkov
Reviewed By: george.karpenkov
Subscribers: xazax.hun, szepet, a.sidorin
Differential Revision: https://reviews.llvm.org/D49550
llvm-svn: 337916
Summary:
Although it is a big patch, the changes are simple:
1. There is one `Z3_Context` now, member of the `SMTConstraintManager` class.
2. `Z3Expr`, `Z3Sort`, `Z3Model` and `Z3Solver` are constructed with a reference to the `Z3_Context` in `SMTConstraintManager`.
3. All static functions are now members of `Z3Solver`, e.g, the `SMTConstraintManager` now calls `Solver.fromBoolean(false)` instead of `Z3Expr::fromBoolean(false)`.
Most of the patch only move stuff around except:
1. New method `Z3Sort MkSort(const QualType &Ty, unsigned BitWidth)`, that creates a sort based on the `QualType` and its width. Used to simplify the `fromData` method.
Unfortunate consequence of this patch:
1. `getInterpretation` was moved from `Z3Model` class to `Z3Solver`, because it needs to create a `Z3Sort` before returning the interpretation. This can be fixed by changing both `toAPFloat` and `toAPSInt` by removing the dependency of `Z3Sort` (it's only used to check which Sort was created and to retrieve the type width).
Reviewers: NoQ, george.karpenkov, ddcc
Reviewed By: george.karpenkov
Subscribers: xazax.hun, szepet, a.sidorin
Differential Revision: https://reviews.llvm.org/D49236
llvm-svn: 337915
Summary:
This patch creates `SMTContext` which will wrap a specific SMT context, through `SMTSolverContext`.
The templated `SMTSolverContext` class it's a simple wrapper around a SMT specific context (currently only used in the Z3 backend), while `Z3Context` inherits `SMTSolverContext<Z3_context>` and implements solver specific operations like initialization and destruction of the context.
This separation was done because:
1. We might want to keep one single context, shared across different `SMTConstraintManager`s. It can be achieved by constructing a `SMTContext`, through a function like `CreateSMTContext(Z3)`, `CreateSMTContext(BOOLECTOR)`, etc. The rest of the CSA only need to know about `SMTContext`, so maybe it's a good idea moving `SMTSolverContext` to a separate header in the future.
2. Any generic SMT operation will only require one `SMTSolverContext`object, which can access the specific context by calling `getContext()`.
Reviewers: NoQ, george.karpenkov
Reviewed By: george.karpenkov
Subscribers: xazax.hun, szepet, a.sidorin
Differential Revision: https://reviews.llvm.org/D49233
llvm-svn: 337914
Reka Kovacs