This patch refactors the utility functions and classes around the construction
of a bug path.
At a very high level, this consists of 3 steps:
* For all BugReports in the same BugReportEquivClass, collect all their error
nodes in a set. With that set, create a new, trimmed ExplodedGraph whose leafs
are all error nodes.
* Until a valid report is found, construct a bug path, which is yet another
ExplodedGraph, that is linear from a given error node to the root of the graph.
* Run all visitors on the constructed bug path. If in this process the report
got invalidated, start over from step 2.
Now, to the changes within this patch:
* Do not allow the invalidation of BugReports up to the point where the trimmed
graph is constructed. Checkers shouldn't add bug reports that are known to be
invalid, and should use visitors and argue about the entirety of the bug path if
needed.
* Do not calculate indices. I may be biased, but I personally find code like
this horrible. I'd like to point you to one of the comments in the original code:
SmallVector<const ExplodedNode *, 32> errorNodes;
for (const auto I : bugReports) {
if (I->isValid()) {
HasValid = true;
errorNodes.push_back(I->getErrorNode());
} else {
// Keep the errorNodes list in sync with the bugReports list.
errorNodes.push_back(nullptr);
}
}
Not on my watch. Instead, use a far easier to follow trick: store a pointer to
the BugReport in question, not an index to it.
* Add range iterators to ExplodedGraph's successors and predecessors, and a
visitor range to BugReporter.
* Rename TrimmedGraph to BugPathGetter. Because that is what it has always been:
no sane graph type should store an iterator-like state, or have an interface not
exposing a single graph-like functionalities.
* Rename ReportGraph to BugPathInfo, because it is only a linear path with some
other context.
* Instead of having both and out and in parameter (which I think isn't ever
excusable unless we use the out-param for caching), return a record object with
descriptive getter methods.
* Where descriptive names weren't sufficient, compliment the code with comments.
Differential Revision: https://reviews.llvm.org/D65379
llvm-svn: 368694
The goal of this refactoring effort was to better understand how interestingness
was propagated in BugReporter.cpp, which eventually turned out to be a dead end,
but with such a twist, I wouldn't even want to spoil it ahead of time. However,
I did get to learn a lot about how things are working in there.
In these series of patches, as well as cleaning up the code big time, I invite
you to study how BugReporter.cpp operates, and discuss how we could design this
file to reduce the horrible mess that it is.
This patch reverts a great part of rC162028, which holds the title "Allow
multiple PathDiagnosticConsumers to be used with a BugReporter at the same
time.". This, however doesn't imply that there's any need for multiple "layers"
or stacks of interesting symbols and regions, quite the contrary, I would argue
that we would like to generate the same amount of information for all output
types, and only process them differently.
Differential Revision: https://reviews.llvm.org/D65378
llvm-svn: 368689
Summary:
A condition could be a multi-line expression where we create the highlight
in separated chunks. PathDiagnosticPopUpPiece is not made for that purpose,
it cannot be added to multiple lines because we have only one ending part
which contains all the notes. So that it cannot have multiple endings and
therefore this patch narrows down the ranges of the highlight to the given
interesting variable of the condition. It prevents HTML-breaking injections.
Reviewed By: NoQ
Differential Revision: https://reviews.llvm.org/D65663
llvm-svn: 368382
This patch is a prerequisite for using LangStandard from Driver in
https://reviews.llvm.org/D64793.
It moves LangStandard* and InputKind::Language to Basic. It is mostly
mechanical, with only a few changes of note:
- enum Language has been changed into enum class Language : uint8_t to
avoid a clash between OpenCL in enum Language and OpenCL in enum
LangFeatures and not to increase the size of class InputKind.
- Now that getLangStandardForName, which is currently unused, also checks
both canonical and alias names, I've introduced a helper getLangKind
which factors out a code pattern already used 3 times.
The patch has been tested on x86_64-pc-solaris2.11, sparcv9-sun-solaris2.11,
and x86_64-pc-linux-gnu.
There's a companion patch for lldb which uses LangStandard.h
(https://reviews.llvm.org/D65717).
While polly includes isl which in turn uses InputKind::C, that part of the
code isn't even built inside the llvm tree. I've posted a patch to allow
for both InputKind::C and Language::C upstream
(https://groups.google.com/forum/#!topic/isl-development/6oEvNWOSQFE).
Differential Revision: https://reviews.llvm.org/D65562
llvm-svn: 367864
Summary:
It allows discriminating between stack frames of the same call that is
called multiple times in a loop.
Thanks to Artem Dergachev for the great idea!
Reviewed By: NoQ
Tags: #clang
Differential Revision: https://reviews.llvm.org/D65587
llvm-svn: 367608
While we implemented taint propagation rules for several
builtin/standard functions, there's a natural desire for users to add
such rules to custom functions.
A series of patches will implement an option that allows users to
annotate their functions with taint propagation rules through a YAML
file. This one adds parsing of the configuration file, which may be
specified in the commands line with the analyzer config:
alpha.security.taint.TaintPropagation:Config. The configuration may
contain propagation rules, filter functions (remove taint) and sink
functions (give a warning if it gets a tainted value).
I also added a new header for future checkers to conveniently read YAML
files as checker options.
Differential Revision: https://reviews.llvm.org/D59555
llvm-svn: 367190
Summary:
When cross TU analysis is used it is possible that a macro expansion
is generated for a macro that is defined (and used) in other than
the main translation unit. To get the expansion for it the source
location in the original source file and original preprocessor
is needed.
Reviewers: martong, xazax.hun, Szelethus, ilya-biryukov
Reviewed By: Szelethus
Subscribers: mgorny, NoQ, ilya-biryukov, rnkovacs, dkrupp, Szelethus, gamesh411, cfe-commits
Tags: #clang
Differential Revision: https://reviews.llvm.org/D64638
llvm-svn: 367006
Summary:
Integer Set Library using retain-count based allocation which is not
modeled in MallocChecker.
Reviewed By: NoQ
Tags: #clang
Differential Revision: https://reviews.llvm.org/D64680
llvm-svn: 366391
Summary:
It models the LLVM casts:
- `cast<>`
- `dyn_cast<>`
- `cast_or_null<>`
- `dyn_cast_or_null<>`
It has a very basic support without checking the `classof()` function.
(It reapplies the reverted 'llvm-svn: 365582' patch with proper test file.)
Reviewed By: NoQ
Tags: #clang
Differential Revision: https://reviews.llvm.org/D64374
llvm-svn: 365585
Summary:
It models the LLVM casts:
- `cast<>`
- `dyn_cast<>`
- `cast_or_null<>`
- `dyn_cast_or_null<>`
It has a very basic support without checking the `classof()` function.
Reviewed By: NoQ
Tags: #clang
Differential Revision: https://reviews.llvm.org/D64374
llvm-svn: 365582
This patch is a major part of my GSoC project, aimed to improve the bug
reports of the analyzer.
TL;DR: Help the analyzer understand that some conditions are important,
and should be explained better. If an CFGBlock is a control dependency
of a block where an expression value is tracked, explain the condition
expression better by tracking it.
if (A) // let's explain why we believe A to be true
10 / x; // division by zero
This is an experimental feature, and can be enabled by the
off-by-default analyzer configuration "track-conditions".
In detail:
This idea was inspired by the program slicing algorithm. Essentially,
two things are used to produce a program slice (a subset of the program
relevant to a (statement, variable) pair): data and control
dependencies. The bug path (the linear path in the ExplodedGraph that leads
from the beginning of the analysis to the error node) enables to
analyzer to argue about data dependencies with relative ease.
Control dependencies are a different slice of the cake entirely.
Just because we reached a branch during symbolic execution, it
doesn't mean that that particular branch has any effect on whether the
bug would've occured. This means that we can't simply rely on the bug
path to gather control dependencies.
In previous patches, LLVM's IDFCalculator, which works on a control flow
graph rather than the ExplodedGraph was generalized to solve this issue.
We use this information to heuristically guess that the value of a tracked
expression depends greatly on it's control dependencies, and start
tracking them as well.
After plenty of evaluations this was seen as great idea, but still
lacking refinements (we should have different descriptions about a
conditions value), hence it's off-by-default.
Differential Revision: https://reviews.llvm.org/D62883
llvm-svn: 365207
I intend to improve the analyzer's bug reports by tracking condition
expressions.
01 bool b = messyComputation();
02 int i = 0;
03 if (b) // control dependency of the bug site, let's explain why we assume val
04 // to be true
05 10 / i; // warn: division by zero
I'll detail this heuristic in the followup patch, strictly related to this one
however:
* Create the new ControlDependencyCalculator class that uses llvm::IDFCalculator
to (lazily) calculate control dependencies for Clang's CFG.
* A new debug checker debug.DumpControlDependencies is added for lit tests
* Add unittests
Differential Revision: https://reviews.llvm.org/D62619
llvm-svn: 365197
Transform clang::DominatorTree to be able to also calculate post dominators.
* Tidy up the documentation
* Make it clang::DominatorTree template class (similarly to how
llvm::DominatorTreeBase works), rename it to clang::CFGDominatorTreeImpl
* Clang's dominator tree is now called clang::CFGDomTree
* Clang's brand new post dominator tree is called clang::CFGPostDomTree
* Add a lot of asserts to the dump() function
* Create a new checker to test the functionality
Differential Revision: https://reviews.llvm.org/D62551
llvm-svn: 365028
Add a label to nodes that have a bug report attached or on which
the analysis was generally interrupted.
Fix printing has_report and implement printing is_sink in the graph dumper.
Differential Revision: https://reviews.llvm.org/D64110
llvm-svn: 364992
This commit adds a new builtin, __builtin_bit_cast(T, v), which performs a
bit_cast from a value v to a type T. This expression can be evaluated at
compile time under specific circumstances.
The compile time evaluation currently doesn't support bit-fields, but I'm
planning on fixing this in a follow up (some of the logic for figuring this out
is in CodeGen). I'm also planning follow-ups for supporting some more esoteric
types that the constexpr evaluator supports, as well as extending
__builtin_memcpy constexpr evaluation to use the same infrastructure.
rdar://44987528
Differential revision: https://reviews.llvm.org/D62825
llvm-svn: 364954
Due to RVO the target region of a function that returns an object by
value isn't necessarily a temporary object region; it may be an
arbitrary memory region. In particular, it may be a field of a bigger
object.
Make sure we don't invalidate the bigger object when said function is
evaluated conservatively.
Differential Revision: https://reviews.llvm.org/D63968
llvm-svn: 364870
The NonnullGlobalConstants checker models the rule "it doesn't make sense
to make a constant global pointer and initialize it to null"; it makes sure
that whatever it's initialized with is known to be non-null.
Ironically, annotating the type of the pointer as _Nonnull breaks the checker.
Fix handling of the _Nonnull annotation so that it was instead one more reason
to believe that the value is non-null.
Differential Revision: https://reviews.llvm.org/D63956
llvm-svn: 364869
This patch uses the new CDF_MaybeBuiltin flag to handle C library functions.
It's mostly an NFC/refactoring pass, but it does fix a bug in handling memset()
when it expands to __builtin___memset_chk() because the latter has
one more argument and memset() handling code was trying to match
the exact number of arguments. Now the code is deduplicated and there's
less room for mistakes.
Differential Revision: https://reviews.llvm.org/D62557
llvm-svn: 364868
When matching C standard library functions in the checker, it's easy to forget
that they are often implemented as macros that are expanded to builtins.
Such builtins would have a different name, so matching the callee identifier
would fail, or may sometimes have more arguments than expected, so matching
the exact number of arguments would fail, but this is fine as long as we have
all the arguments that we need in their respective places.
This patch adds a set of flags to the CallDescription class so that to handle
various special matching rules, and adds the first flag into this set,
which enables a more fuzzy matching for functions that
may be implemented as compiler builtins.
Differential Revision: https://reviews.llvm.org/D62556
llvm-svn: 364867
It encapsulates the procedure of figuring out whether a call event
corresponds to a function that's modeled by a checker.
Checker developers no longer need to worry about performance of
lookups into their own custom maps.
Add unittests - which finally test CallDescription itself as well.
Differential Revision: https://reviews.llvm.org/D62441
llvm-svn: 364866
The -analyzer-stats flag now allows you to find out how much time was spent
on AST-based analysis and on path-sensitive analysis and, separately,
on bug visitors, as they're occasionally a performance problem on their own.
The total timer wasn't useful because there's anyway a total time printed out.
Remove it.
Differential Revision: https://reviews.llvm.org/D63227
llvm-svn: 364266
Summary:
After evaluation it would be an Unknown value and tracking would be lost.
Reviewers: NoQ, xazax.hun, ravikandhadai, baloghadamsoftware, Szelethus
Reviewed By: NoQ
Subscribers: szepet, rnkovacs, a.sidorin, mikhail.ramalho, donat.nagy,
dkrupp, cfe-commits
Tags: #clang
Differential Revision: https://reviews.llvm.org/D63720
llvm-svn: 364259
Summary:
- Now we could see the `has_report` property in `trim-egraph` mode.
- This patch also removes the trailing comma after each node.
Reviewers: NoQ
Reviewed By: NoQ
Subscribers: xazax.hun, baloghadamsoftware, szepet, a.sidorin,
mikhail.ramalho, Szelethus, donat.nagy, dkrupp, cfe-commits
Tags: #clang
Differential Revision: https://reviews.llvm.org/D63436
llvm-svn: 364193
Quotes around StringRegions are now escaped and unescaped correctly,
producing valid JSON.
Additionally, add a forgotten escape for Store values.
Differential Revision: https://reviews.llvm.org/D63519
llvm-svn: 363897
Include a unique pointer so that it was possible to figure out if it's
the same cluster in different program states. This allows comparing
dumps of different states against each other.
Differential Revision: https://reviews.llvm.org/D63362
llvm-svn: 363896
Location context ID is a property of the location context, not of an item
within it. It's useful to know the id even when there are no items
in the context, eg. for the purposes of figuring out how did contents
of the Environment for the same location context changed across states.
Differential Revision: https://reviews.llvm.org/D62754
llvm-svn: 363895
This changes the checker callback signature to use the modern, easy to
use interface. Additionally, this unblocks future work on allowing
checkers to implement evalCall() for calls that don't correspond to any
call-expression or require additional information that's only available
as part of the CallEvent, such as C++ constructors and destructors.
Differential Revision: https://reviews.llvm.org/D62440
llvm-svn: 363893
IIG is a replacement for MIG in DriverKit: IIG is autogenerating C++ code.
Suppress dead store warnings on such code, as the tool seems to be producing
them regularly, and the users of IIG are not in position to address these
warnings, as they don't control the autogenerated code. IIG-generated code
is identified by looking at the comments at the top of the file.
Differential Revision: https://reviews.llvm.org/D63118
llvm-svn: 363892
Summary:
This patch applies a change similar to rC363069, but for SARIF files.
The `%diff_sarif` lit substitution invokes `diff` with a non-portable
`-I` option. The intended effect can be achieved by normalizing the
inputs to `diff` beforehand. Such normalization can be done with
`grep -Ev`, which is also used by other tests.
Additionally, this patch updates the SARIF output to have a newline at
the end of the file. This makes it so that the SARIF file qualifies as a
POSIX text file, which increases the consumability of the generated file
in relation to various tools.
Reviewers: NoQ, sfertile, xingxue, jasonliu, daltenty, aaron.ballman
Reviewed By: aaron.ballman
Subscribers: xazax.hun, baloghadamsoftware, szepet, a.sidorin, mikhail.ramalho, Szelethus, donat.nagy, dkrupp, Charusso, jsji, cfe-commits
Tags: #clang
Differential Revision: https://reviews.llvm.org/D62952
llvm-svn: 363822
Often times, when an ArraySubscriptExpr was reported as null or
undefined, the bug report was difficult to understand, because the
analyzer explained why arr[i] has that value, but didn't realize that in
fact i's value is very important as well. This patch fixes this by
tracking the indices of arrays.
Differential Revision: https://reviews.llvm.org/D63080
llvm-svn: 363510
Summary:
When we traversed backwards on ExplodedNodes to see where processed the
given statement we `break` too early. With the current approach we do not
miss the CallExitEnd ProgramPoint which stands for an inlined call.
Reviewers: NoQ, xazax.hun, ravikandhadai, baloghadamsoftware, Szelethus
Reviewed By: NoQ
Subscribers: szepet, rnkovacs, a.sidorin, mikhail.ramalho, donat.nagy,
dkrupp, cfe-commits
Tags: #clang
Differential Revision: https://reviews.llvm.org/D62926
llvm-svn: 363491
nullptr_t does not access memory.
We now reuse CK_NullToPointer to represent a conversion from a glvalue
of type nullptr_t to a prvalue of nullptr_t where necessary.
This reinstates r363337, reverted in r363352.
llvm-svn: 363429
Revert 363340 "Remove unused SK_LValueToRValue initialization step."
Revert 363337 "PR23833, DR2140: an lvalue-to-rvalue conversion on a glvalue of type"
Revert 363295 "C++ DR712 and others: handle non-odr-use resulting from an lvalue-to-rvalue conversion applied to a member access or similar not-quite-trivial lvalue expression."
llvm-svn: 363352
nullptr_t does not access memory.
We now reuse CK_NullToPointer to represent a conversion from a glvalue
of type nullptr_t to a prvalue of nullptr_t where necessary.
This reinstates r345562, reverted in r346065, now that CodeGen's
handling of non-odr-used variables has been fixed.
llvm-svn: 363337
Summary:
As suggested in the review of D62949, this patch updates the plist
output to have a newline at the end of the file. This makes it so that
the plist output file qualifies as a POSIX text file, which increases
the consumability of the generated plist file in relation to various
tools.
Reviewers: NoQ, sfertile, xingxue, jasonliu, daltenty
Reviewed By: NoQ, xingxue
Subscribers: jsji, cfe-commits
Tags: #clang
Differential Revision: https://reviews.llvm.org/D63041
llvm-svn: 362992
Summary:
We're using the clang static analyzer together with a number of
custom analyses in our CI system to ensure that certain invariants
are statiesfied for by the code every commit. Unfortunately, there
currently doesn't seem to be a good way to determine whether any
analyzer warnings were emitted, other than parsing clang's output
(or using scan-build, which then in turn parses clang's output).
As a simpler mechanism, simply add a `-analyzer-werror` flag to CC1
that causes the analyzer to emit its warnings as errors instead.
I briefly tried to have this be `Werror=analyzer` and make it go
through that machinery instead, but that seemed more trouble than
it was worth in terms of conflicting with options to the actual build
and special cases that would be required to circumvent the analyzers
usual attempts to quiet non-analyzer warnings. This is simple and it
works well.
Reviewed-By: NoQ, Szelethusw
Differential Revision: https://reviews.llvm.org/D62885
llvm-svn: 362855
Summary:
This new piece is similar to our macro expansion printing in HTML reports:
On mouse-hover event it pops up on variables. Similar to note pieces it
supports `plist` diagnostics as well.
It is optional, on by default: `add-pop-up-notes=true`.
Extra: In HTML reports `background-color: LemonChiffon` was too light,
changed to `PaleGoldenRod`.
Reviewers: NoQ, alexfh
Reviewed By: NoQ
Subscribers: cfe-commits, gerazo, gsd, george.karpenkov, alexfh, xazax.hun,
baloghadamsoftware, szepet, a.sidorin, mikhail.ramalho,
Szelethus, donat.nagy, dkrupp
Tags: #clang
Differential Revision: https://reviews.llvm.org/D60670
llvm-svn: 362014
The `cplusplus.SelfAssignment` checker has a visitor that is added
to every `BugReport` to mark the to branch of the self assignment
operator with e.g. `rhs == *this` and `rhs != *this`. With the new
`NoteTag` feature this visitor is not needed anymore. Instead the
checker itself marks the two branches using the `NoteTag`s.
Differential Revision: https://reviews.llvm.org/D62479
llvm-svn: 361818
When initialization of virtual base classes is skipped, we now tell the user
about it, because this aspect of C++ isn't very well-known.
The implementation is based on the new "note tags" feature (r358781).
In order to make use of it, allow note tags to produce prunable notes,
and move the note tag factory to CoreEngine.
Differential Revision: https://reviews.llvm.org/D61817
llvm-svn: 361682
This patch adds the run-time CFG branch that would skip initialization of
virtual base classes depending on whether the constructor is called from a
superclass constructor or not. Previously the Static Analyzer was already
skipping virtual base-class initializers in such constructors, but it wasn't
skipping their arguments and their potential side effects, which was causing
pr41300 (and was generally incorrect). The previous skipping behavior is
now replaced with a hard assertion that we're not even getting there due
to how our CFG works.
The new CFG element is under a CFG build option so that not to break other
consumers of the CFG by this change. Static Analyzer support for this change
is implemented.
Differential Revision: https://reviews.llvm.org/D61816
llvm-svn: 361681
Turn it into a variant class instead. This conversion does indeed save some code
but there's a plan to add support for more kinds of terminators that aren't
necessarily based on statements, and with those in mind it becomes more and more
confusing to have CFGTerminators implicitly convertible to a Stmt *.
Differential Revision: https://reviews.llvm.org/D61814
llvm-svn: 361586
Same patch as D62093, but for checker/plugin options, the only
difference being that options for alpha checkers are implicitly marked
as alpha.
Differential Revision: https://reviews.llvm.org/D62093
llvm-svn: 361566
These options are now only visible under
-analyzer-checker-option-help-developer.
Differential Revision: https://reviews.llvm.org/D61839
llvm-svn: 361561
Previously, the only way to display the list of available checkers was
to invoke the analyzer with -analyzer-checker-help frontend flag. This
however wasn't really great from a maintainer standpoint: users came
across checkers meant strictly for development purposes that weren't to
be tinkered with, or those that were still in development. This patch
creates a clearer division in between these categories.
From now on, we'll have 3 flags to display the list checkers. These
lists are mutually exclusive and can be used in any combination (for
example to display both stable and alpha checkers).
-analyzer-checker-help: Displays the list for stable, production ready
checkers.
-analyzer-checker-help-alpha: Displays the list for in development
checkers. Enabling is discouraged
for non-development purposes.
-analyzer-checker-help-developer: Modeling and debug checkers. Modeling
checkers shouldn't be enabled/disabled
by hand, and debug checkers shouldn't
be touched by users.
Differential Revision: https://reviews.llvm.org/D62093
llvm-svn: 361558
Add the new frontend flag -analyzer-checker-option-help to display all
checker/package options.
Differential Revision: https://reviews.llvm.org/D57858
llvm-svn: 361552
This patch refactors begin and end symbol creation by moving symbol
conjuration into the `create...` functions. This way the functions'
responsibilities are clearer and makes possible to add more functions
handling these symbols (e.g. functions for handling the container's
size) without code multiplication.
Differential Revision: https://reviews.llvm.org/D61136
llvm-svn: 361141
Since D57922, the config table contains every checker option, and it's default
value, so having it as an argument for getChecker*Option is redundant.
By the time any of the getChecker*Option function is called, we verified the
value in CheckerRegistry (after D57860), so we can confidently assert here, as
any irregularities detected at this point must be a programmer error. However,
in compatibility mode, verification won't happen, so the default value must be
restored.
This implies something else, other than adding removing one more potential point
of failure -- debug.ConfigDumper will always contain valid values for
checker/package options!
Differential Revision: https://reviews.llvm.org/D59195
llvm-svn: 361042
Validate whether the option exists, and also whether the supplied value is of
the correct type. With this patch, invoking the analyzer should be, at least
in the frontend mode, a lot safer.
Differential Revision: https://reviews.llvm.org/D57860
llvm-svn: 361011
The more entries we have in AnalyzerOptions::ConfigTable, the more helpful
debug.ConfigDumper is. With this patch, I'm pretty confident that it'll now emit
the entire state of the analyzer, minus the frontend flags.
It would be nice to reserve the config table specifically to checker options
only, as storing the regular analyzer configs is kinda redundant.
Differential Revision: https://reviews.llvm.org/D57922
llvm-svn: 361006
Summary:
This patch implements the source location builtins `__builtin_LINE(), `__builtin_FUNCTION()`, `__builtin_FILE()` and `__builtin_COLUMN()`. These builtins are needed to implement [`std::experimental::source_location`](https://rawgit.com/cplusplus/fundamentals-ts/v2/main.html#reflection.src_loc.creation).
With the exception of `__builtin_COLUMN`, GCC also implements these builtins, and Clangs behavior is intended to match as closely as possible.
Reviewers: rsmith, joerg, aaron.ballman, bogner, majnemer, shafik, martong
Reviewed By: rsmith
Subscribers: rnkovacs, loskutov, riccibruno, mgorny, kunitoki, alexr, majnemer, hfinkel, cfe-commits
Differential Revision: https://reviews.llvm.org/D37035
llvm-svn: 360937
The checker was crashing when it was trying to assume a structure
to be null or non-null so that to evaluate the effect of the annotation.
Differential Revision: https://reviews.llvm.org/D61958
llvm-svn: 360790
Suppress MIG checker false positives that occur when the programmer increments
the reference count before calling a MIG destructor, and the MIG destructor
literally boils down to decrementing the reference count.
Differential Revision: https://reviews.llvm.org/D61925
llvm-svn: 360737
When looking for the location context of the call site, unwrap block invocation
contexts because they are attached to the current AnalysisDeclContext
while what we need is the previous AnalysisDeclContext.
Differential Revision: https://reviews.llvm.org/D61545
llvm-svn: 360202
new expression.
This was voted into C++20 as a defect report resolution, so we
retroactively apply it to all prior language modes (though it can never
actually be used before C++11 mode).
llvm-svn: 360006
https://bugs.llvm.org/show_bug.cgi?id=41741
Pretty much the same as D61246 and D61106, this time for __complex__ types. Upon
further investigation, I realized that we should regard all types
Type::isScalarType returns true for as primitive, so I merged
isMemberPointerType(), isBlockPointerType() and isAnyComplexType()` into that
instead.
I also stumbled across yet another bug,
https://bugs.llvm.org/show_bug.cgi?id=41753, but it seems to be unrelated to
this checker.
Differential Revision: https://reviews.llvm.org/D61569
llvm-svn: 359998
During my work on analyzer dependencies, I created a great amount of new
checkers that emitted no diagnostics at all, and were purely modeling some
function or another.
However, the user shouldn't really disable/enable these by hand, hence this
patch, which hides these by default. I intentionally chose not to hide alpha
checkers, because they have a scary enough name, in my opinion, to cause no
surprise when they emit false positives or cause crashes.
The patch introduces the Hidden bit into the TableGen files (you may remember
it before I removed it in D53995), and checkers that are either marked as
hidden, or are in a package that is marked hidden won't be displayed under
-analyzer-checker-help. -analyzer-checker-help-hidden, a new flag meant for
developers only, displays the full list.
Differential Revision: https://reviews.llvm.org/D60925
llvm-svn: 359720
https://bugs.llvm.org/show_bug.cgi?id=41611
Similarly to D61106, the checker ran over an llvm_unreachable for vector types:
struct VectorSizeLong {
VectorSizeLong() {}
__attribute__((__vector_size__(16))) long x;
};
void __vector_size__LongTest() {
VectorSizeLong v;
}
Since, according to my short research,
"The vector_size attribute is only applicable to integral and float scalars,
although arrays, pointers, and function return values are allowed in conjunction
with this construct."
[src: https://gcc.gnu.org/onlinedocs/gcc-4.6.1/gcc/Vector-Extensions.html#Vector-Extensions]
vector types are safe to regard as primitive.
Differential Revision: https://reviews.llvm.org/D61246
llvm-svn: 359539
Currently we always inline functions that have no branches, i.e. have exactly
three CFG blocks: ENTRY, some code, EXIT. This makes sense because when there
are no branches, it means that there's no exponential complexity introduced
by inlining such function. Such functions also don't trigger various fundamental
problems with our inlining mechanism, such as the problem of inlined
defensive checks.
Sometimes the CFG may contain more blocks, but in practice it still has
linear structure because all directions (except, at most, one) of all branches
turned out to be unreachable. When this happens, still treat the function
as "small". This is useful, in particular, for dealing with C++17 if constexpr.
Differential Revision: https://reviews.llvm.org/D61051
llvm-svn: 359531
Don't crash when trying to model a call in which the callee is unknown
in compile time, eg. a pointer-to-member call.
Differential Revision: https://reviews.llvm.org/D61285
llvm-svn: 359530
This patch is more of a fix than a real improvement: in checkPostCall()
we should return immediately after finding the right call and handling
it. This both saves unnecessary processing and double-handling calls by
mistake.
Differential Revision: https://reviews.llvm.org/D61134
llvm-svn: 359283
Because RetainCountChecker has custom "local" reasoning about escapes,
it has a separate facility to deal with tracked symbols at end of analysis
and check them for leaks regardless of whether they're dead or not.
This facility iterates over the list of tracked symbols and reports
them as leaks, but it needs to treat the return value specially.
Some custom allocators tend to return the value with an offset, storing
extra metadata at the beginning of the buffer. In this case the return value
would be a non-base region. In order to avoid false positives, we still need to
find the original symbol within the return value, otherwise it'll be unable
to match it to the item in the list of tracked symbols.
Differential Revision: https://reviews.llvm.org/D60991
llvm-svn: 359263
the assertion is in fact incorrect: there is a cornercase in Objective-C++
in which a C++ object is not constructed with a constructor, but merely
zero-initialized. Namely, this happens when an Objective-C message is sent
to a nil and it is supposed to return a C++ object.
Differential Revision: https://reviews.llvm.org/D60988
llvm-svn: 359262
https://bugs.llvm.org/show_bug.cgi?id=41590
For the following code snippet, UninitializedObjectChecker crashed:
struct MyAtomicInt {
_Atomic(int) x;
MyAtomicInt() {}
};
void entry() {
MyAtomicInt b;
}
The problem was that _Atomic types were not regular records, unions,
dereferencable or primitive, making the checker hit the llvm_unreachable at
lib/StaticAnalyzer/Checkers/UninitializedObject/UninitializedObjectChecker.cpp:347.
The solution is to regard these types as primitive as well. The test case shows
that with this addition, not only are we able to get rid of the crash, but we
can identify x as uninitialized.
Differential Revision: https://reviews.llvm.org/D61106
llvm-svn: 359230
If macro "CHECK_X(x)" expands to something like "if (x != NULL) ...",
the "Assuming..." note no longer says "Assuming 'x' is equal to CHECK_X".
Differential Revision: https://reviews.llvm.org/D59121
llvm-svn: 359037
Summary:
The existing CTU mechanism imports `FunctionDecl`s where the definition is available in another TU. This patch extends that to VarDecls, to bind more constants.
- Add VarDecl importing functionality to CrossTranslationUnitContext
- Import Decls while traversing them in AnalysisConsumer
- Add VarDecls to CTU external mappings generator
- Name changes from "external function map" to "external definition map"
Reviewers: NoQ, dcoughlin, xazax.hun, george.karpenkov, martong
Reviewed By: xazax.hun
Subscribers: Charusso, baloghadamsoftware, mikhail.ramalho, Szelethus, donat.nagy, dkrupp, george.karpenkov, mgorny, whisperity, szepet, rnkovacs, a.sidorin, cfe-commits
Tags: #clang
Differential Revision: https://reviews.llvm.org/D46421
llvm-svn: 358968
A compilation warning was in my previous commit which broke the buildbot
because it is using `-Werror` for compilation. This patch fixes this
issue.
llvm-svn: 358955
Currently iterator checkers record comparison of iterator positions
and process them for keeping track the distance between them (e.g.
whether a position is the same as the end position). However this
makes some processing unnecessarily complex and it is not needed at
all: we only need to keep track between the abstract symbols stored
in these iterator positions. This patch changes this and opens the
path to comparisons to the begin() and end() symbols between the
container (e.g. size, emptiness) which are stored as symbols, not
iterator positions. The functionality of the checker is unchanged.
Differential Revision: https://reviews.llvm.org/D53701
llvm-svn: 358951
When growing a body on a body farm, it's essential to use the same redeclaration
of the function that's going to be used during analysis. Otherwise our
ParmVarDecls won't match the ones that are used to identify argument regions.
This boils down to trusting the reasoning in AnalysisDeclContext. We shouldn't
canonicalize the declaration before farming the body because it makes us not
obey the sophisticated decision-making process of AnalysisDeclContext.
Differential Revision: https://reviews.llvm.org/D60899
llvm-svn: 358946
Stuffing invalid source locations (such as those in functions produced by
body farms) into path diagnostics causes crashes.
Fix a typo in a nearby function name.
Differential Revision: https://reviews.llvm.org/D60808
llvm-svn: 358945
Implement cplusplus.SmartPtrModeling, a new checker that doesn't
emit any warnings but models methods of smart pointers more precisely.
For now the only thing it does is make `(bool) P` return false when `P`
is a freshly moved pointer. This addresses a false positive in the
use-after-move-checker.
Differential Revision: https://reviews.llvm.org/D60796
llvm-svn: 358944
Moved UninitializedObjectChecker from the 'alpha.cplusplus' to the
'optin.cplusplus' package.
Differential Revision: https://reviews.llvm.org/D58573
llvm-svn: 358797
TL;DR:
* Add checker and package options to the TableGen files
* Added a new class called CmdLineOption, and both Package and Checker recieved
a list<CmdLineOption> field.
* Added every existing checker and package option to Checkers.td.
* The CheckerRegistry class
* Received some comments to most of it's inline classes
* Received the CmdLineOption and PackageInfo inline classes, a list of
CmdLineOption was added to CheckerInfo and PackageInfo
* Added addCheckerOption and addPackageOption
* Added a new field called Packages, used in addPackageOptions, filled up in
addPackage
Detailed description:
In the last couple months, a lot of effort was put into tightening the
analyzer's command line interface. The main issue is that it's spectacularly
easy to mess up a lenghty enough invocation of the analyzer, and the user was
given no warnings or errors at all in that case.
We can divide the effort of resolving this into several chapters:
* Non-checker analyzer configurations:
Gather every analyzer configuration into a dedicated file. Emit errors for
non-existent configurations or incorrect values. Be able to list these
configurations. Tighten AnalyzerOptions interface to disallow making such
a mistake in the future.
* Fix the "Checker Naming Bug" by reimplementing checker dependencies:
When cplusplus.InnerPointer was enabled, it implicitly registered
unix.Malloc, which implicitly registered some sort of a modeling checker
from the CStringChecker family. This resulted in all of these checker
objects recieving the name "cplusplus.InnerPointer", making AnalyzerOptions
asking for the wrong checker options from the command line:
cplusplus.InnerPointer:Optimisic
istead of
unix.Malloc:Optimistic.
This was resolved by making CheckerRegistry responsible for checker
dependency handling, instead of checkers themselves.
* Checker options: (this patch included!)
Same as the first item, but for checkers.
(+ minor fixes here and there, and everything else that is yet to come)
There were several issues regarding checker options, that non-checker
configurations didn't suffer from: checker plugins are loaded runtime, and they
could add new checkers and new options, meaning that unlike for non-checker
configurations, we can't collect every checker option purely by generating code.
Also, as seen from the "Checker Naming Bug" issue raised above, they are very
rarely used in practice, and all sorts of skeletons fell out of the closet while
working on this project.
They were extremely problematic for users as well, purely because of how long
they were. Consider the following monster of a checker option:
alpha.cplusplus.UninitializedObject:CheckPointeeInitialization=false
While we were able to verify whether the checker itself (the part before the
colon) existed, any errors past that point were unreported, easily resulting
in 7+ hours of analyses going to waste.
This patch, similarly to how dependencies were reimplemented, uses TableGen to
register checker options into Checkers.td, so that Checkers.inc now contains
entries for both checker and package options. Using the preprocessor,
Checkers.inc is converted into code in CheckerRegistry, adding every builtin
(checkers and packages that have an entry in the Checkers.td file) checker and
package option to the registry. The new addPackageOption and addCheckerOption
functions expose the same functionality to statically-linked non-builtin and
plugin checkers and packages as well.
Emitting errors for incorrect user input, being able to list these options, and
some other functionalies will land in later patches.
Differential Revision: https://reviews.llvm.org/D57855
llvm-svn: 358752
Ideally, there is no reason behind not being able to depend on checkers that
come from a different plugin (or on builtin checkers) -- however, this is only
possible if all checkers are added to the registry before resolving checker
dependencies. Since I used a binary search in my addDependency method, this also
resulted in an assertion failure (due to CheckerRegistry::Checkers not being
sorted), since the function used by plugins to register their checkers
(clang_registerCheckers) calls addDependency.
This patch resolves this issue by only noting which dependencies have to
established when addDependency is called, and resolves them at a later stage
when no more checkers are added to the registry, by which point
CheckerRegistry::Checkers is already sorted.
Differential Revision: https://reviews.llvm.org/D59461
llvm-svn: 358750
Default RegionStore bindings represent values that can be obtained by loading
from anywhere within the region, not just the specific offset within the region
that they are said to be bound to. For example, default-binding a character \0
to an int (eg., via memset()) means that the whole int is 0, not just
that its lower byte is 0.
Even though memset and bzero were modeled this way, it didn't work correctly
when applied to simple variables. Eg., in
int x;
memset(x, 0, sizeof(x));
we did produce a default binding, but were unable to read it later, and 'x'
was perceived as an uninitialized variable even after memset.
At the same time, if we replace 'x' with a variable of a structure or array
type, accessing fields or elements of such variable was working correctly,
which was enough for most cases. So this was only a problem for variables of
simple integer/enumeration/floating-point/pointer types.
Fix loading default bindings from RegionStore for regions of simple variables.
Add a unit test to document the API contract as well.
Differential Revision: https://reviews.llvm.org/D60742
llvm-svn: 358722
There are barely any lines I haven't changed in these files, so I think I could
might as well leave it in an LLVM coding style conforming state. I also renamed
2 functions and moved addDependency out of line to ease on followup patches.
Differential Revision: https://reviews.llvm.org/D59457
llvm-svn: 358676
For the following code snippet:
void builtin_function_call_crash_fixes(char *c) {
__builtin_strncpy(c, "", 6);
__builtin_memset(c, '\0', (0));
__builtin_memcpy(c, c, 0);
}
security.insecureAPI.DeprecatedOrUnsafeBufferHandling caused a regression, as it
didn't recognize functions starting with __builtin_. Fixed exactly that.
I wanted to modify an existing test file, but the two I found didn't seem like
perfect candidates. While I was there, I prettified their RUN: lines.
Differential Revision: https://reviews.llvm.org/D59812
llvm-svn: 358609
Writing stuff into an argument variable is usually equivalent to writing stuff
to a local variable: it will have no effect outside of the function.
There's an important exception from this rule: if the argument variable has
a non-trivial destructor, the destructor would be invoked on
the parent stack frame, exposing contents of the otherwise dead
argument variable to the caller.
If such argument is the last place where a pointer is stored before the function
exits and the function is the one we've started our analysis from (i.e., we have
no caller context for it), we currently diagnose a leak. This is incorrect
because the destructor of the argument still has access to the pointer.
The destructor may deallocate the pointer or even pass it further.
Treat writes into such argument regions as "escapes" instead, suppressing
spurious memory leak reports but not messing with dead symbol removal.
Differential Revision: https://reviews.llvm.org/D60112
llvm-svn: 358321
The idea behind this heuristic is that normally the visitor is there to
inform the user that a certain function may fail to initialize a certain
out-parameter. For system header functions this is usually dictated by the
contract, and it's unlikely that the header function has accidentally
forgot to put the value into the out-parameter; it's more likely
that the user has intentionally skipped the error check.
Warnings on skipped error checks are more like security warnings;
they aren't necessarily useful for all users, and they should instead
be introduced on a per-API basis.
Differential Revision: https://reviews.llvm.org/D60107
llvm-svn: 357810
Requires making the llvm::MemoryBuffer* stored by SourceManager const,
which in turn requires making the accessors for that return const
llvm::MemoryBuffer*s and updating all call sites.
The original motivation for this was to use it and fix the TODO in
CodeGenAction.cpp's ConvertBackendLocation() by using the UnownedTag
version of createFileID, and since llvm::SourceMgr* hands out a const
llvm::MemoryBuffer* this is required. I'm not sure if fixing the TODO
this way actually works, but this seems like a good change on its own
anyways.
No intended behavior change.
Differential Revision: https://reviews.llvm.org/D60247
llvm-svn: 357724
__builtin_constant_p(x) is a compiler builtin that evaluates to 1 when
its argument x is a compile-time constant and to 0 otherwise. In CodeGen
it is simply lowered to the respective LLVM intrinsic. In the Analyzer
we've been trying to delegate modeling to Expr::EvaluateAsInt, which is
allowed to sometimes fail for no apparent reason.
When it fails, let's conservatively return false. Modeling it as false
is pretty much never wrong, and it is only required to return true
on a best-effort basis, which every user should expect.
Fixes VLAChecker false positives on code that tries to emulate
static asserts in C by constructing a VLA of dynamic size -1 under the
assumption that this dynamic size is actually a constant
in the sense of __builtin_constant_p.
Differential Revision: https://reviews.llvm.org/D60110
llvm-svn: 357557
At least gcc 7.4 complained with
../tools/clang/lib/StaticAnalyzer/Checkers/Taint.cpp:26:53: warning: extra ';' [-Wpedantic]
TaintTagType);
^
llvm-svn: 357461
It turns out that SourceManager::isInSystemHeader() crashes when an invalid
source location is passed into it. Invalid source locations are relatively
common: not only they come from body farms, but also, say, any function in C
that didn't come with a forward declaration would have an implicit
forward declaration with invalid source locations.
There's a more comfy API for us to use in the Static Analyzer:
CallEvent::isInSystemHeader(), so just use that.
Differential Revision: https://reviews.llvm.org/D59901
llvm-svn: 357329
It is now an inter-checker communication API, similar to the one that
connects MallocChecker/CStringChecker/InnerPointerChecker: simply a set of
setters and getters for a state trait.
Differential Revision: https://reviews.llvm.org/D59861
llvm-svn: 357326
The transfer function for the CFG element that represents a logical operation
computes the value of the operation and does nothing else. The element
appears after all the short circuit decisions were made, so they don't need
to be made again at this point.
Because our expression evaluation is imprecise, it is often hard to
discriminate between:
(1) we don't know the value of the RHS because we failed to evaluate it
and
(2) we don't know the value of the RHS because it didn't need to be evaluated.
This is hard because it depends on our knowledge about the value of the LHS
(eg., if LHS is true, then RHS in (LHS || RHS) doesn't need to be computed)
but LHS itself may have been evaluated imprecisely and we don't know whether
it is true or not. Additionally, the Analyzer wouldn't necessarily even remember
what the value of the LHS was because theoretically it's not really necessary
to know it for any future evaluations.
In order to work around these issues, the transfer function for logical
operations consists in looking at the ExplodedGraph we've constructed so far
in order to figure out from which CFG direction did we arrive here.
Such post-factum backtracking that doesn't involve looking up LHS and RHS values
is usually possible. However sometimes it fails because when we deduplicate
exploded nodes with the same program point and the same program state we may end
up in a situation when we reached the same program point from two or more
different directions.
By removing the assertion, we admit that the procedure indeed sometimes fails to
work. When it fails, we also admit that we don't know the value of the logical
operator.
Differential Revision: https://reviews.llvm.org/D59857
llvm-svn: 357325
Almost all path-sensitive checkers need to tell the user when something specific
to that checker happens along the execution path but does not constitute a bug
on its own. For instance, a call to operator delete in C++ has consequences
that are specific to a use-after-free bug. Deleting an object is not a bug
on its own, but when the Analyzer finds an execution path on which a deleted
object is used, it'll have to explain to the user when exactly during that path
did the deallocation take place.
Historically such custom notes were added by implementing "bug report visitors".
These visitors were post-processing bug reports by visiting every ExplodedNode
along the path and emitting path notes whenever they noticed that a change that
is relevant to a bug report occurs within the program state. For example,
it emits a "memory is deallocated" note when it notices that a pointer changes
its state from "allocated" to "deleted".
The "visitor" approach is powerful and efficient but hard to use because
such preprocessing implies that the developer first models the effects
of the event (say, changes the pointer's state from "allocated" to "deleted"
as part of operator delete()'s transfer function) and then forgets what happened
and later tries to reverse-engineer itself and figure out what did it do
by looking at the report.
The proposed approach tries to avoid discarding the information that was
available when the transfer function was evaluated. Instead, it allows the
developer to capture all the necessary information into a closure that
will be automatically invoked later in order to produce the actual note.
This should reduce boilerplate and avoid very painful logic duplication.
On the technical side, the closure is a lambda that's put into a special kind of
a program point tag, and a special bug report visitor visits all nodes in the
report and invokes all note-producing closures it finds along the path.
For now it is up to the lambda to make sure that the note is actually relevant
to the report. For instance, a memory deallocation note would be irrelevant when
we're reporting a division by zero bug or if we're reporting a use-after-free
of a different, unrelated chunk of memory. The lambda can figure these thing out
by looking at the bug report object that's passed into it.
A single checker is refactored to make use of the new functionality: MIGChecker.
Its program state is trivial, making it an easy testing ground for the first
version of the API.
Differential Revision: https://reviews.llvm.org/D58367
llvm-svn: 357323
Since rL335814, if the constraint manager cannot find a range set for `A - B`
(where `A` and `B` are symbols) it looks for a range for `B - A` and returns
it negated if it exists. However, if a range set for both `A - B` and `B - A`
is stored then it only returns the first one. If we both use `A - B` and
`B - A`, these expressions behave as two totally unrelated symbols. This way
we miss some useful deductions which may lead to false negatives or false
positives.
This tiny patch changes this behavior: if the symbolic expression the
constraint manager is looking for is a difference `A - B`, it tries to
retrieve the range for both `A - B` and `B - A` and if both exists it returns
the intersection of range `A - B` and the negated range of `B - A`. This way
every time a checker applies new constraints to the symbolic difference or to
its negated it always affects both the original difference and its negated.
Differential Revision: https://reviews.llvm.org/D55007
llvm-svn: 357167
Remove CompilerInstance::VirtualFileSystem and
CompilerInstance::setVirtualFileSystem, instead relying on the VFS in
the FileManager. CompilerInstance and its clients already went to some
trouble to make these match. Now they are guaranteed to match.
As part of this, I added a VFS parameter (defaults to nullptr) to
CompilerInstance::createFileManager, to avoid repeating construction
logic in clients that just wanted to customize the VFS.
https://reviews.llvm.org/D59377
llvm-svn: 357037
r356634 didn't fix all the problems caused by r356222 - even though simple
constructors involving transparent init-list expressions are now evaluated
precisely, many more complicated constructors aren't, for other reasons.
The attached test case is an example of a constructor that will never be
evaluated precisely - simply because there isn't a constructor there (instead,
the program invokes run-time undefined behavior by returning without a return
statement that should have constructed the return value).
Fix another part of the problem for such situations: evaluate transparent
init-list expressions transparently, so that to avoid creating ill-formed
"transparent" nonloc::CompoundVals.
Differential Revision: https://reviews.llvm.org/D59622
llvm-svn: 356969
Summary:
If the constraint information is not changed between two program states the
analyzer has not learnt new information and made no report. But it is
possible to happen because we have no information at all. The new approach
evaluates the condition to determine if that is the case and let the user
know we just `Assuming...` some value.
Reviewers: NoQ, george.karpenkov
Reviewed By: NoQ
Subscribers: llvm-commits, xazax.hun, baloghadamsoftware, szepet, a.sidorin,
mikhail.ramalho, Szelethus, donat.nagy, dkrupp, gsd, gerazo
Tags: #clang, #llvm
Differential Revision: https://reviews.llvm.org/D57410
llvm-svn: 356323
Summary:
Removed the `GDM` checking what could prevent reports made by this visitor.
Now we rely on constraint changes instead.
(It reapplies 356318 with a feature from 356319 because build-bot failure.)
Reviewers: NoQ, george.karpenkov
Reviewed By: NoQ
Subscribers: cfe-commits, jdoerfert, gerazo, xazax.hun, baloghadamsoftware,
szepet, a.sidorin, mikhail.ramalho, Szelethus, donat.nagy, dkrupp
Tags: #clang
Differential Revision: https://reviews.llvm.org/D54811
llvm-svn: 356322
Summary: If the constraint information is not changed between two program states the analyzer has not learnt new information and made no report. But it is possible to happen because we have no information at all. The new approach evaluates the condition to determine if that is the case and let the user know we just 'Assuming...' some value.
Reviewers: NoQ, george.karpenkov
Reviewed By: NoQ
Subscribers: xazax.hun, baloghadamsoftware, szepet, a.sidorin, mikhail.ramalho, Szelethus, donat.nagy, dkrupp, gsd, gerazo
Tags: #clang
Differential Revision: https://reviews.llvm.org/D57410
llvm-svn: 356319
Kristof Umann