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
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
In the current implementation, we run visitors until the fixed point is
reached.
That is, if a visitor adds another visitor, the currently processed path
is destroyed, all diagnostics is discarded, and it is regenerated again,
until it's no longer modified.
This pattern has a few negative implications:
- This loop does not even guarantee to terminate.
E.g. just imagine two visitors bouncing a diagnostics around.
- Performance-wise, e.g. for sqlite3 all visitors are being re-run at
least 10 times for some bugs.
We have already seen a few reports where it leads to timeouts.
- If we want to add more computationally intense visitors, this will
become worse.
- From architectural standpoint, the current layout requires copying
visitors, which is conceptually wrong, and can be annoying (e.g. no
unique_ptr on visitors allowed).
The proposed change is a much simpler architecture: the outer loop
processes nodes upwards, and whenever the visitor is added it only
processes current nodes and above, thus guaranteeing termination.
Differential Revision: https://reviews.llvm.org/D47856
llvm-svn: 335666
BugReporter.cpp is already severely overloaded, and those dump methods
are on PathDiagnostics and should belong in the corresponding
implementation file.
Differential Revision: https://reviews.llvm.org/D48035
llvm-svn: 334541
getEndPath is a problematic API, because it's not clear when it's called
(hint: not always at the end of the path), it crashes at runtime with
more than one non-nullptr returning implementation, and diagnostics
internal depend on it being called at some exact place.
However, most visitors don't actually need that: all they want is a
function consistently called after all nodes are traversed, to perform
finalization and to decide whether invalidation is needed.
Differential Revision: https://reviews.llvm.org/D48042
llvm-svn: 334540
Once we removed AlternateExtensive, I've looked closer into the
difference between Minimal and Extensive, and turns out, the difference
was not that large.
Differential Revision: https://reviews.llvm.org/D47756
llvm-svn: 334525
Rename AlternateExtensive to Extensive.
In 2013, five years ago, we have switched to AlternateExtensive
diagnostics by default, and Extensive was available under unused,
undocumented flag.
This change remove the flag, renames the Alternate
diagnostic to Extensive (as it's no longer Alternate), and ports the
test.
Differential Revision: https://reviews.llvm.org/D47670
llvm-svn: 334524
Summary: This is a prototype of a bug reporter visitor that invalidates bug reports by re-checking constraints of certain states on the bug path using the Z3 constraint manager backend. The functionality is available under the `crosscheck-with-z3` analyzer config flag.
Reviewers: george.karpenkov, NoQ, dcoughlin, rnkovacs
Reviewed By: george.karpenkov
Subscribers: rnkovacs, NoQ, george.karpenkov, dcoughlin, xbolva00, ddcc, mikhail.ramalho, MTC, fhahn, whisperity, baloghadamsoftware, szepet, a.sidorin, gsd, dkrupp, xazax.hun, cfe-commits
Differential Revision: https://reviews.llvm.org/D45517
llvm-svn: 333903
This is similar to the LLVM change https://reviews.llvm.org/D46290.
We've been running doxygen with the autobrief option for a couple of
years now. This makes the \brief markers into our comments
redundant. Since they are a visual distraction and we don't want to
encourage more \brief markers in new code either, this patch removes
them all.
Patch produced by
for i in $(git grep -l '\@brief'); do perl -pi -e 's/\@brief //g' $i & done
for i in $(git grep -l '\\brief'); do perl -pi -e 's/\\brief //g' $i & done
Differential Revision: https://reviews.llvm.org/D46320
llvm-svn: 331834
When a '>>' token is split into two '>' tokens (in C++11 onwards), or (as an
extension) when we do the same for other tokens starting with a '>', we can't
just use a location pointing to the first '>' as the location of the split
token, because that would result in our miscomputing the length and spelling
for the token. As a consequence, for example, a refactoring replacing 'A<X>'
with something else would sometimes replace one character too many, and
similarly diagnostics highlighting a template-id source range would highlight
one character too many.
Fix this by creating an expansion range covering the first character of the
'>>' token, whose spelling is '>'. For this to work, we generalize the
expansion range of a macro FileID to be either a token range (the common case)
or a character range (used in this new case).
llvm-svn: 331155
removeUnneededCalls() is responsible for removing path diagnostic pieces within
functions that don't contain "interesting" events. It makes bug reports
much tidier.
When a stack frame is known to be interesting, the function doesn't descend
into it to prune anything within it, even other callees that are totally boring.
Fix the function to prune boring callees in interesting stack frames.
Differential Revision: https://reviews.llvm.org/D45117
llvm-svn: 329102
r327219 added wrappers to std::sort which randomly shuffle the container before
sorting. This will help in uncovering non-determinism caused due to undefined
sorting order of objects having the same key.
To make use of that infrastructure we need to invoke llvm::sort instead of
std::sort.
llvm-svn: 328636
When viewing the report in the collapsed mode the label signifying where
did the execution go is often necessary for properly understanding the
context.
Differential Revision: https://reviews.llvm.org/D43145
llvm-svn: 325975
HTML diagnostics can be an overwhelming blob of pages of code.
This patch adds a checkbox which filters this list down to only the
lines *relevant* to the counterexample by e.g. skipping branches which
analyzer has assumed to be infeasible at a time.
The resulting amount of output is much smaller, and often fits on one
screen, and also provides a much more readable diagnostics.
Differential Revision: https://reviews.llvm.org/D41378
llvm-svn: 322612
This addresses an issue introduced in r183451: since
`removePiecesWithInvalidLocations` is called *after* `adjustCallLocations`,
it is not necessary, and in fact harmful, to have this assertion in
adjustCallLocations.
Addresses rdar://36170689
Differential Revision: https://reviews.llvm.org/D41680
llvm-svn: 321682
The bugreporter::trackNullOrUndefValue() mechanism contains a system of bug
reporter visitors that recursively call each other in order to track where a
null or undefined value came from, where each visitor represents a particular
tracking mechanism (track how the value was stored, track how the value was
returned from a function, track how the value was constrained to null, etc.).
Each visitor is only added once per value it needs to track. Almost. One
exception from this rule would be FindLastStoreBRVisitor that has two operation
modes: it contains a flag that indicates whether null stored values should be
suppressed. Two instances of FindLastStoreBRVisitor with different values of
this flag are considered to be different visitors, so they can be added twice
and produce the same diagnostic twice. This was indeed the case in the affected
test.
With the current logic of this whole machinery, such duplication seems
unavoidable. We should be able to safely add visitors with different flag
values without constructing duplicate diagnostic pieces. Hence the effort
in this commit to de-duplicate diagnostics regardless of what visitors
have produced them.
Differential Revision: https://reviews.llvm.org/D41258
llvm-svn: 321135
Because since r308957 the suppress-on-sink feature contains its own
mini-analysis, it also needs to become aware that C++ unhandled exceptions
cause sinks. Unfortunately, for now we treat all exceptions as unhandled in
the analyzer, so suppress-on-sink needs to do the same.
rdar://problem/28157554
Differential Revision: https://reviews.llvm.org/D35674
llvm-svn: 308961
If a certain memory leak (or other similar bug) found by the analyzer is known
to be happening only before abnormal termination of the program ("sink", eg.
assertion failure in the code under analysis, or another bug that introduces
undefined behavior), such leak warning is discarded. However, if the analysis
has never reaches completion (due to complexity of the code), it may be
failing to notice the sink.
This commit further extends the partial solution introduced in r290341 to cover
cases when a complicated control flow occurs before encountering a no-return
statement (which anyway inevitably leads to such statement(s)) by traversing
the respective section of the CFG in a depth-first manner. A complete solution
still seems elusive.
rdar://problem/28157554
Differential Revision: https://reviews.llvm.org/D35673
llvm-svn: 308957
In plist output mode with alternate path diagnostics, when entering a function,
we draw an arrow from the caller to the beginning of the callee's declaration.
Upon exiting, however, we draw the arrow from the last statement in the
callee function. The former makes little sense when the declaration is
not a definition, i.e. has no body, which may happen in case the body
is coming from a body farm, eg. Objective-C autosynthesized property accessor.
Differential Revision: https://reviews.llvm.org/D33671
llvm-svn: 304713
