Traditionally, clang-tidy uses the term check, and the analyzer uses checker,
but in the very early years, this wasn't the case, and code originating from the
early 2010's still incorrectly refer to checkers as checks.
This patch attempts to hunt down most of these, aiming to refer to checkers as
checkers, but preserve references to callback functions (like checkPreCall) as
checks.
Differential Revision: https://reviews.llvm.org/D67140
llvm-svn: 371760
At this point the PathDiagnostic, PathDiagnosticLocation, PathDiagnosticPiece
structures no longer rely on anything specific to Static Analyzer, so we can
move them out of it for everybody to use.
PathDiagnosticConsumers are still to be handed off.
Differential Revision: https://reviews.llvm.org/D67419
llvm-svn: 371661
This method of PathDiagnostic is a part of Static Analyzer's particular
path diagnostic construction scheme. As such, it doesn't belong to
the PathDiagnostic class, but to the Analyzer.
Differential Revision: https://reviews.llvm.org/D67418
llvm-svn: 371660
These static functions deal with ExplodedNodes which is something we don't want
the PathDiagnostic interface to know anything about, as it's planned to be
moved out of libStaticAnalyzerCore.
Differential Revision: https://reviews.llvm.org/D67382
llvm-svn: 371659
That's one of the few random entities in the PathDiagnostic interface that
are specific to the Static Analyzer. By moving them out we could let
everybody use path diagnostics without linking against Static Analyzer.
Differential Revision: https://reviews.llvm.org/D67381
llvm-svn: 371658
Checkers are now required to specify whether they're creating a
path-sensitive report or a path-insensitive report by constructing an
object of the respective type.
This makes BugReporter more independent from the rest of the Static Analyzer
because all Analyzer-specific code is now in sub-classes.
Differential Revision: https://reviews.llvm.org/D66572
llvm-svn: 371450
Allow attaching fixit hints to Static Analyzer BugReports.
Fixits are attached either to the bug report itself or to its notes
(path-sensitive event notes or path-insensitive extra notes).
Add support for fixits in text output (including the default text output that
goes without notes, as long as the fixit "belongs" to the warning).
Add support for fixits in the plist output mode.
Implement a fixit for the path-insensitive DeadStores checker. Only dead
initialization warning is currently covered.
Implement a fixit for the path-sensitive VirtualCall checker when the virtual
method is not pure virtual (in this case the "fix" is to suppress the warning
by qualifying the call).
Both fixits are under an off-by-default flag for now, because they
require more careful testing.
Differential Revision: https://reviews.llvm.org/D65182
llvm-svn: 371257
We defined (on the mailing list and here on phabricator) 2 different cases where
retrieving information about a control dependency condition is very important:
* When the condition's last write happened in a different stack frame
* When the collapse point of the condition (when we can constrain it to be
true/false) didn't happen in the actual condition.
It seems like we solved this problem with the help of expression value tracking,
and have started working on better diagnostics notes about this process.
Expression value tracking is nothing more than registering a variety of visitors
to construct reports about it. Each of the registered visitors (ReturnVisitor,
FindLastStoreVisitor, NoStoreFuncVisitor, etc) have something to go by: a
MemRegion, an SVal, an ExplodedNode, etc. For this reason, better explaining a
last write is super simple, we can always just pass on some more information to
the visitor in question (as seen in D65575).
ConditionBRVisitor is a different beast, as it was built for a different
purpose. It is responsible for constructing events at, well, conditions, and is
registered only once, and isn't a part of the "expression value tracking
family". Unfortunately, it is also the visitor to tinker with for constructing
better diagnostics about the collapse point problem.
This creates a need for alternative way to communicate with ConditionBRVisitor
that a specific condition is being tracked for for the reason of being a control
dependency. Since at almost all PathDiagnosticEventPiece construction the
visitor checks interestingness, it makes sense to pair interestingness with a
reason as to why we marked an entity as such.
Differential Revision: https://reviews.llvm.org/D65723
llvm-svn: 369583
Summary:
This patch introduces a new `analyzer-config` configuration:
`-analyzer-config silence-checkers`
which could be used to silence the given checkers.
It accepts a semicolon separated list, packed into quotation marks, e.g:
`-analyzer-config silence-checkers="core.DivideZero;core.NullDereference"`
It could be used to "disable" core checkers, so they model the analysis as
before, just if some of them are too noisy it prevents to emit reports.
This patch also adds support for that new option to the scan-build.
Passing the option `-disable-checker core.DivideZero` to the scan-build
will be transferred to `-analyzer-config silence-checkers=core.DivideZero`.
Reviewed By: NoQ, Szelethus
Differential Revision: https://reviews.llvm.org/D66042
llvm-svn: 369078
Now that we've moved to C++14, we no longer need the llvm::make_unique
implementation from STLExtras.h. This patch is a mechanical replacement
of (hopefully) all the llvm::make_unique instances across the monorepo.
Differential revision: https://reviews.llvm.org/D66259
llvm-svn: 368942
Well, what is says on the tin I guess!
Some more changes:
* Move isInevitablySinking() from BugReporter.cpp to CFGBlock's interface
* Rename and move findBlockForNode() from BugReporter.cpp to
ExplodedNode::getCFGBlock()
Differential Revision: https://reviews.llvm.org/D65287
llvm-svn: 368836
I feel this is kinda important, because in a followup patch I'm adding different
kinds of interestingness, and propagating the correct kind in BugReporter.cpp is
just one less thing to worry about.
Differential Revision: https://reviews.llvm.org/D65578
llvm-svn: 368755
Apparently this does literally nothing.
When you think about this, it makes sense. If something is really important,
we're tracking it anyways, and that system is sophisticated enough to mark
actually interesting statements as such. I wouldn't say that it's even likely
that subexpressions are also interesting (array[10 - x + x]), so I guess even
if this produced any effects, its probably undesirable.
Differential Revision: https://reviews.llvm.org/D65487
llvm-svn: 368752
In D65379, I briefly described the construction of bug paths from an
ExplodedGraph. This patch is about refactoring the code processing the bug path
into a bug report.
A part of finding a valid bug report was running all visitors on the bug path,
so we already have a (possibly empty) set of diagnostics for each ExplodedNode
in it.
Then, for each diagnostic consumer, we construct non-visitor diagnostic pieces.
* We first construct the final diagnostic piece (the warning), then
* We start ascending the bug path from the error node's predecessor (since the
error node itself was used to construct the warning event). For each node
* We check the location (whether its a CallEnter, CallExit) etc. We simultaneously
keep track of where we are with the execution by pushing CallStack when we see a
CallExit (keep in mind that everything is happening in reverse!), popping it
when we find a CallEnter, compacting them into a single PathDiagnosticCallEvent.
void f() {
bar();
}
void g() {
f();
error(); // warning
}
=== The bug path ===
(root) -> f's CallEnter -> bar() -> f's CallExit -> (error node)
=== Constructed report ===
f's CallEnter -> bar() -> f's CallExit
^ /
\ V
(root) ---> f's CallEvent --> (error node)
* We also keep track of different PathPieces different location contexts
* (CallEvent::path in the above example has f's LocationContext, while the
CallEvent itself is in g's context) in a LocationContextMap object. Construct
whatever piece, if any, is needed for the note.
* If we need to generate edges (or arrows) do so. Make sure to also connect
these pieces with the ones that visitors emitted.
* Clean up the constructed PathDiagnostic by making arrows nicer, pruning
function calls, etc.
So I complained about mile long function invocations with seemingly the same
parameters being passed around. This problem, as I see it, a natural candidate
for creating classes and tying them all together.
I tried very hard to make the implementation feel natural, like, rolling off the
tongue. I introduced 2 new classes: PathDiagnosticBuilder (I mean, I kept the
name but changed almost everything in it) contains every contextual information
(owns the bug path, the diagnostics constructed but the visitors, the BugReport
itself, etc) needed for constructing a PathDiagnostic object, and is pretty much
completely immutable. BugReportContruct is the object containing every
non-contextual information (the PathDiagnostic object we're constructing, the
current location in the bug path, the location context map and the call stack I
meantioned earlier), and is passed around all over the place as a single entity
instead of who knows how many parameters.
I tried to used constness, asserts, limiting visibility of fields to my
advantage to clean up the code big time and dramatically improve safety. Also,
whenever I found the code difficult to understand, I added comments and/or
examples.
Here's a complete list of changes and my design philosophy behind it:
* Instead of construcing a ReportInfo object (added by D65379) after finding a
valid bug report, simply return an optional PathDiagnosticBuilder object straight
away. Move findValidReport into the class as a static method. I find
GRBugReporter::generatePathDiagnostics a joy to look at now.
* Rename generatePathDiagnosticForConsumer to generate (maybe not needed, but
felt that way in the moment) and moved it to PathDiagnosticBuilder. If we don't
need to generate diagnostics, bail out straight away, like we always should have.
After that, construct a BugReportConstruct object, leaving the rest of the logic
untouched.
* Move all static methods that would use contextual information into
PathDiagnosticBuilder, reduce their parameter count drastically by simply
passing around a BugReportConstruct object.
* Glance at the code I removed: Could you tell what the original
PathDiagnosticBuilder::LC object was for? It took a gooood long while for me to
realize that nothing really. It is always equal with the LocationContext
associated with our current position in the bug path. Remove it completely.
* The original code contains the following expression quite a bit:
LCM[&PD.getActivePath()], so what does it mean? I said that we collect the
contexts associated with different PathPieces, but why would we ever modify that,
shouldn't it be set? Well, theoretically yes, but in the implementation, the
address of PathDiagnostic::getActivePath doesn't change if we move to an outer,
previously unexplored function. Add both descriptive method names and
explanations to BugReportConstruct to help on this.
* Add plenty of asserts, both for safety and as a poor man's documentation.
Differential Revision: https://reviews.llvm.org/D65484
llvm-svn: 368737
When I'm new to a file/codebase, I personally find C++'s strong static type
system to be a great aid. BugReporter.cpp is still painful to read however:
function calls are made with mile long parameter lists, seemingly all of them
taken with a non-const reference/pointer. This patch fixes nothing but this:
make a few things const, and hammer it until it compiles.
Differential Revision: https://reviews.llvm.org/D65382
llvm-svn: 368735
find clang/ -type f -exec sed -i 's/std::shared_ptr<PathDiagnosticPiece>/PathDiagnosticPieceRef/g' {} \;
git diff -U3 --no-color HEAD^ | clang-format-diff-6.0 -p1 -i
Just as C++ is meant to be refactored, right?
Differential Revision: https://reviews.llvm.org/D65381
llvm-svn: 368717
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:
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
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
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
to reflect the new license.
We understand that people may be surprised that we're moving the header
entirely to discuss the new license. We checked this carefully with the
Foundation's lawyer and we believe this is the correct approach.
Essentially, all code in the project is now made available by the LLVM
project under our new license, so you will see that the license headers
include that license only. Some of our contributors have contributed
code under our old license, and accordingly, we have retained a copy of
our old license notice in the top-level files in each project and
repository.
llvm-svn: 351636
In earlier patches regarding AnalyzerOptions, a lot of effort went into
gathering all config options, and changing the interface so that potential
misuse can be eliminited.
Up until this point, AnalyzerOptions only evaluated an option when it was
querried. For example, if we had a "-no-false-positives" flag, AnalyzerOptions
would store an Optional field for it that would be None up until somewhere in
the code until the flag's getter function is called.
However, now that we're confident that we've gathered all configs, we can
evaluate off of them before analysis, so we can emit a error on invalid input
even if that prticular flag will not matter in that particular run of the
analyzer. Another very big benefit of this is that debug.ConfigDumper will now
show the value of all configs every single time.
Also, almost all options related class have a similar interface, so uniformity
is also a benefit.
The implementation for errors on invalid input will be commited shorty.
Differential Revision: https://reviews.llvm.org/D53692
llvm-svn: 348031
This patch should not introduce any behavior changes. It consists of
mostly one of two changes:
1. Replacing fall through comments with the LLVM_FALLTHROUGH macro
2. Inserting 'break' before falling through into a case block consisting
of only 'break'.
We were already using this warning with GCC, but its warning behaves
slightly differently. In this patch, the following differences are
relevant:
1. GCC recognizes comments that say "fall through" as annotations, clang
doesn't
2. GCC doesn't warn on "case N: foo(); default: break;", clang does
3. GCC doesn't warn when the case contains a switch, but falls through
the outer case.
I will enable the warning separately in a follow-up patch so that it can
be cleanly reverted if necessary.
Reviewers: alexfh, rsmith, lattner, rtrieu, EricWF, bollu
Differential Revision: https://reviews.llvm.org/D53950
llvm-svn: 345882
This is the first part of the implementation of the inclusion of macro
expansions into the plist output. It adds a new flag that adds a new
"macro_expansions" entry to each report that has PathDiagnosticPieces that were
expanded from a macro. While there's an entry for each macro expansion, both
the name of the macro and what it expands to is missing, and will be implemented
in followup patches.
Differential Revision: https://reviews.llvm.org/D52742
llvm-svn: 345724
A ConstantExpr class represents a full expression that's in a context where a
constant expression is required. This class reflects the path the evaluator
took to reach the expression rather than the syntactic context in which the
expression occurs.
In the future, the class will be expanded to cache the result of the evaluated
expression so that it's not needlessly re-evaluated
Reviewed By: rsmith
Differential Revision: https://reviews.llvm.org/D53475
llvm-svn: 345692
If the non-sink report is generated at the exit node, it will be
suppressed by the current functionality in isInevitablySinking, as it
only checks the successors of the block, but not the block itself.
The bug shows up in RetainCountChecker checks.
Differential Revision: https://reviews.llvm.org/D52284
llvm-svn: 342766
Kristof Umann