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
Summary: Removed the `GDM` checking what could prevent reports made by this visitor. Now we rely on constraint changes instead.
Reviewers: NoQ, george.karpenkov
Reviewed By: NoQ
Subscribers: 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: 356318
RegionStore now knows how to bind a nonloc::CompoundVal that represents the
value of an aggregate initializer when it has its initial segment of sub-values
correspond to base classes.
Additionally, fixes the crash from pr40022.
Differential Revision: https://reviews.llvm.org/D59054
llvm-svn: 356222
For a rather short code snippet, if debug.ReportStmts (added in this patch) was
enabled, a bug reporter visitor crashed:
struct h {
operator int();
};
int k() {
return h();
}
Ultimately, this originated from PathDiagnosticLocation::createMemberLoc, as it
didn't handle the case where it's MemberExpr typed parameter returned and
invalid SourceLocation for MemberExpr::getMemberLoc. The solution was to find
any related valid SourceLocaion, and Stmt::getBeginLoc happens to be just that.
Differential Revision: https://reviews.llvm.org/D58777
llvm-svn: 356161
Checking whether two regions are the same is a partially decidable problem:
either we know for sure that they are the same or we cannot decide. A typical
case for this are the symbolic regions based on conjured symbols. Two
different conjured symbols are either the same or they are different. Since
we cannot decide this and want to reduce false positives as much as possible
we exclude these regions whenever checking whether two containers are the
same at iterator mismatch check.
Differential Revision: https://reviews.llvm.org/D53754
llvm-svn: 356049
Buildbot breaks when LLVm is compiled with memory sanitizer.
WARNING: MemorySanitizer: use-of-uninitialized-value
#0 0xa3d16d8 in getMacroNameAndPrintExpansion(blahblah)
lib/StaticAnalyzer/Core/PlistDiagnostics.cpp:903:11
llvm-svn: 355911
When there is a functor-like macro which is passed as parameter to another
"function" macro then its parameters are not listed at the place of expansion:
#define foo(x) int bar() { return x; }
#define hello(fvar) fvar(0)
hello(foo)
int main() { 1 / bar(); }
Expansion of hello(foo) asserted Clang, because it expected an l_paren token in
the 3rd line after "foo", since it is a function-like token.
Patch by Tibor Brunner!
Differential Revision: https://reviews.llvm.org/D57893
llvm-svn: 355903
In the commited testfile, macro expansion (the one implemented for the plist
output) runs into an infinite recursion. The issue originates from the algorithm
being faulty, as in
#define value REC_MACRO_FUNC(value)
the "value" is being (or at least attempted) expanded from the same macro.
The solved this issue by gathering already visited macros in a set, which does
resolve the crash, but will result in an incorrect macro expansion, that would
preferably be fixed down the line.
Patch by Tibor Brunner!
Differential Revision: https://reviews.llvm.org/D57891
llvm-svn: 355705
Asserting on invalid input isn't very nice, hence the patch to emit an error
instead.
This is the first of many patches to overhaul the way we handle checker options.
Differential Revision: https://reviews.llvm.org/D57850
llvm-svn: 355704
In D55734, we implemented a far more general way of describing taint propagation
rules for functions, like being able to specify an unlimited amount of
source and destination parameters. Previously, we didn't have a particularly
elegant way of expressing the propagation rules for functions that always return
(either through an out-param or return value) a tainted value. In this patch,
we model these functions similarly to other ones, by assigning them a
TaintPropagationRule that describes that they "create a tainted value out of
nothing".
The socket C function is somewhat special, because for certain parameters (for
example, if we supply localhost as parameter), none of the out-params should
be tainted. For this, we added a general solution of being able to specify
custom taint propagation rules through function pointers.
Patch by Gábor Borsik!
Differential Revision: https://reviews.llvm.org/D59055
llvm-svn: 355703
Summary:
When comparing a symbolic region and a constant, the constant would be
widened or truncated to the width of a void pointer, meaning that the
constant could be incorrectly truncated when handling symbols for
non-default address spaces. In the attached test case this resulted in a
false positive since the constant was truncated to zero. To fix this,
widen/truncate the constant to the width of the symbol expression's
type.
This commit does not consider non-symbolic regions as I'm not sure how
to generalize getting the type there.
This fixes PR40814.
Reviewers: NoQ, zaks.anna, george.karpenkov
Reviewed By: NoQ
Subscribers: xazax.hun, baloghadamsoftware, szepet, a.sidorin, mikhail.ramalho, Szelethus, donat.nagy, dkrupp, jdoerfert, Charusso, cfe-commits
Tags: #clang
Differential Revision: https://reviews.llvm.org/D58665
llvm-svn: 355592
This patch includes the necessary code for converting between a fixed point type and integer.
This also includes constant expression evaluation for conversions with these types.
Differential Revision: https://reviews.llvm.org/D56900
llvm-svn: 355462
The gets function has no SrcArgs. Because the default value for isTainted was
false, it didn't mark its DstArgs as tainted.
Patch by Gábor Borsik!
Differential Revision: https://reviews.llvm.org/D58828
llvm-svn: 355396
Under the term "subchecker", I mean checkers that do not have a checker class on
their own, like unix.MallocChecker to unix.DynamicMemoryModeling.
Since a checker object was required in order to retrieve checker options,
subcheckers couldn't possess options on their own.
This patch is also an excuse to change the argument order of getChecker*Option,
it always bothered me, now it resembles the actual command line argument
(checkername:option=value).
Differential Revision: https://reviews.llvm.org/D57579
llvm-svn: 355297
#define f(y) x
#define x f(x)
int main() { x; }
This example results a compilation error since "x" in the first line was not
defined earlier. However, the macro expression printer goes to an infinite
recursion on this example.
Patch by Tibor Brunner!
Differential Revision: https://reviews.llvm.org/D57892
llvm-svn: 354806
Add more "consuming" functions. For now only vm_deallocate() was supported.
Add a non-zero value that isn't an error; this value is -305 ("MIG_NO_REPLY")
and it's fine to deallocate data when you are returning this error.
Make sure that the mig_server_routine annotation is inherited.
rdar://problem/35380337
Differential Revision: https://reviews.llvm.org/D58397
llvm-svn: 354643
When a MIG server routine argument is released in an automatic destructor,
the Static Analyzer thinks that this happens after the return statement, and so
the violation of the MIG convention doesn't happen.
Of course, it doesn't quite work that way, so this is a false negative.
Add a hack that makes the checker double-check at the end of function
that no argument was released when the routine fails with an error.
rdar://problem/35380337
Differential Revision: https://reviews.llvm.org/D58392
llvm-svn: 354642
Add a BugReporterVisitor for highlighting the events of deallocating a
parameter. All such events are relevant to the emitted report (as long as the
report is indeed emitted), so all of them will get highlighted.
Add a trackExpressionValue visitor for highlighting where does the error return
code come from.
Do not add a trackExpressionValue visitor for highlighting how the deallocated
argument(s) was(were) copied around. This still remains to be implemented.
rdar://problem/35380337
Differential Revision: https://reviews.llvm.org/D58368
llvm-svn: 354641
r354530 has added a new function/block/message attribute "mig_server_routine"
that attracts compiler's attention to functions that need to follow the MIG
server routine convention with respect to deallocating out-of-line data that
was passed to them as an argument.
Teach the checker to identify MIG routines by looking at this attribute,
rather than by making heuristic-based guesses.
rdar://problem/35380337
Differential Revision: https://reviews.llvm.org/58366
llvm-svn: 354638
This checker detects use-after-free bugs in (various forks of) the Mach kernel
that are caused by errors in MIG server routines - functions called remotely by
MIG clients. The MIG convention forces the server to only deallocate objects
it receives from the client when the routine is executed successfully.
Otherwise, if the server routine exits with an error, the client assumes that
it needs to deallocate the out-of-line data it passed to the server manually.
This means that deallocating such data within the MIG routine and then returning
a non-zero error code is always a dangerous use-after-free bug.
rdar://problem/35380337
Differential Revision: https://reviews.llvm.org/D57558
llvm-svn: 354635
FindLastStoreBRVisitor tries to find the first node in the exploded graph where
the current value was assigned to a region. This node is called the "store
site". It is identified by a pair of Pred and Succ nodes where Succ already has
the binding for the value while Pred does not have it. However the visitor
mistakenly identifies a node pair as the store site where the value is a
`LazyCompoundVal` and `Pred` does not have a store yet but `Succ` has it. In
this case the `LazyCompoundVal` is different in the `Pred` node because it also
contains the store which is different in the two nodes. This error may lead to
crashes (a declaration is cast to a parameter declaration without check) or
misleading bug path notes.
In this patch we fix this problem by checking for unequal `LazyCompoundVals`: if
their region is equal, and their store is the same as the store of their nodes
we consider them as equal when looking for the "store site". This is an
approximation because we do not check for differences of the subvalues
(structure members or array elements) in the stores.
Differential Revision: https://reviews.llvm.org/D58067
llvm-svn: 353943
There are certain unsafe or deprecated (since C11) buffer handling
functions which should be avoided in safety critical code. They
could cause buffer overflows. A new checker,
'security.insecureAPI.DeprecatedOrUnsafeBufferHandling' warns for
every occurrence of such functions (unsafe or deprecated printf,
scanf family, and other buffer handling functions, which now have
a secure variant).
Patch by Dániel Kolozsvári!
Differential Revision: https://reviews.llvm.org/D35068
llvm-svn: 353698
oth strlcat and strlcpy cut off their safe bound for the argument value
at sizeof(destination). There's no need to subtract 1 in only one
of these cases.
Differential Revision: https://reviews.llvm.org/D57981
rdar://problem/47873212
llvm-svn: 353583
Now, instead of passing the reference to a shared_ptr, we pass the shared_ptr instead.
I've also removed the check if Z3 is present in CreateZ3ConstraintManager as this function already calls CreateZ3Solver that performs the exactly same check.
Differential Revision: https://reviews.llvm.org/D54976
llvm-svn: 353371
This patch moves the ConstraintSMT definition to the SMTConstraintManager header to make it easier to move the Z3 backend around.
We achieve this by not using shared_ptr anymore, as llvm::ImmutableSet doesn't seem to like it.
The solver specific exprs and sorts are cached in the Z3Solver object now and we move pointers to those objects around.
As a nice side-effect, SMTConstraintManager doesn't have to be a template anymore. Yay!
Differential Revision: https://reviews.llvm.org/D54975
llvm-svn: 353370
Memory region that correspond to a variable is identified by the variable's
declaration and, in case of local variables, the stack frame it belongs to.
The declaration needs to be canonical, otherwise we'd have two different
memory regions that correspond to the same variable.
Fix such bug for global variables with forward declarations and assert
that no other problems of this kind happen.
Differential Revision: https://reviews.llvm.org/D57619
llvm-svn: 353353
This reverts commit r341722.
The "postponed" mechanism turns out to be necessary in order to handle
situations when a symbolic region is only kept alive by implicit bindings
in the Store. Otherwise the region is never scanned by the Store's worklist
and the binding gets dropped despite being live, as demonstrated
by the newly added tests.
Differential Revision: https://reviews.llvm.org/D57554
llvm-svn: 353350
This patch is an implementation of the ideas discussed on the mailing list[1].
The idea is to somewhat heuristically guess whether the field that was confirmed
to be uninitialized is actually guarded with ifs, asserts, switch/cases and so
on. Since this is a syntactic check, it is very much prone to drastically
reduce the amount of reports the checker emits. The reports however that do not
get filtered out though have greater likelihood of them manifesting into actual
runtime errors.
[1] http://lists.llvm.org/pipermail/cfe-dev/2018-September/059255.html
Differential Revision: https://reviews.llvm.org/D51866
llvm-svn: 352959
Having an incorrect type for a cast causes the checker to incorrectly
dismiss the operation under ARC, leading to a false positive
use-after-release on the test.
rdar://47709885
Differential Revision: https://reviews.llvm.org/D57557
llvm-svn: 352824
This builtin has the same UI as __builtin_object_size, but has the
potential to be evaluated dynamically. It is meant to be used as a
drop-in replacement for libraries that use __builtin_object_size when
a dynamic checking mode is enabled. For instance,
__builtin_object_size fails to provide any extra checking in the
following function:
void f(size_t alloc) {
char* p = malloc(alloc);
strcpy(p, "foobar"); // expands to __builtin___strcpy_chk(p, "foobar", __builtin_object_size(p, 0))
}
This is an overflow if alloc < 7, but because LLVM can't fold the
object size intrinsic statically, it folds __builtin_object_size to
-1. With __builtin_dynamic_object_size, alloc is passed through to
__builtin___strcpy_chk.
rdar://32212419
Differential revision: https://reviews.llvm.org/D56760
llvm-svn: 352665
Provide a more powerful and at the same time more readable way of specifying
taint propagation rules for known functions within the checker.
Now it should be possible to specify an unlimited amount of source and
destination parameters for taint propagation.
No functional change intended just yet.
Patch by Gábor Borsik!
Differential Revision: https://reviews.llvm.org/D55734
llvm-svn: 352572
Track them for ISL/OS objects by default, and for NS/CF under a flag.
rdar://47536377
Differential Revision: https://reviews.llvm.org/D57356
llvm-svn: 352534
That weakens inner invariants, but allows the class to be more generic,
allowing usage in situations where the call expression is not known (or
should not matter).
Differential Revision: https://reviews.llvm.org/D57344
llvm-svn: 352531
When a function takes the address of a field the analyzer will no longer
assume that the function will change other fields of the enclosing structs.
Differential Revision: https://reviews.llvm.org/D57230
llvm-svn: 352473
This patch effectively fixes the almost decade old checker naming issue.
The solution is to assert when CheckerManager::getChecker is called on an
unregistered checker, and assert when CheckerManager::registerChecker is called
on a checker that is already registered.
Differential Revision: https://reviews.llvm.org/D55429
llvm-svn: 352292
Unfortunately, up until now, the fact that certain checkers depended on one
another was known, but how these actually unfolded was hidden deep within the
implementation. For example, many checkers (like RetainCount, Malloc or CString)
modelled a certain functionality, and exposed certain reportable bug types to
the user. For example, while MallocChecker models many many different types of
memory handling, the actual "unix.MallocChecker" checker the user was exposed to
was merely and option to this modeling part.
Other than this being an ugly mess, this issue made resolving the checker naming
issue almost impossible. (The checker naming issue being that if a checker
registered more than one checker within its registry function, both checker
object recieved the same name) Also, if the user explicitly disabled a checker
that was a dependency of another that _was_ explicitly enabled, it implicitly,
without "telling" the user, reenabled it.
Clearly, changing this to a well structured, declarative form, where the
handling of dependencies are done on a higher level is very much preferred.
This patch, among the detailed things later, makes checkers declare their
dependencies within the TableGen file Checkers.td, and exposes the same
functionality to plugins and statically linked non-generated checkers through
CheckerRegistry::addDependency. CheckerRegistry now resolves these dependencies,
makes sure that checkers are added to CheckerManager in the correct order,
and makes sure that if a dependency is disabled, so will be every checker that
depends on it.
In detail:
* Add a new field to the Checker class in CheckerBase.td called Dependencies,
which is a list of Checkers.
* Move unix checkers before cplusplus, as there is no forward declaration in
tblgen :/
* Add the following new checkers:
- StackAddrEscapeBase
- StackAddrEscapeBase
- CStringModeling
- DynamicMemoryModeling (base of the MallocChecker family)
- IteratorModeling (base of the IteratorChecker family)
- ValistBase
- SecuritySyntaxChecker (base of bcmp, bcopy, etc...)
- NSOrCFErrorDerefChecker (base of NSErrorChecker and CFErrorChecker)
- IvarInvalidationModeling (base of IvarInvalidation checker family)
- RetainCountBase (base of RetainCount and OSObjectRetainCount)
* Clear up and registry functions in MallocChecker, happily remove old FIXMEs.
* Add a new addDependency function to CheckerRegistry.
* Neatly format RUN lines in files I looked at while debugging.
Big thanks to Artem Degrachev for all the guidance through this project!
Differential Revision: https://reviews.llvm.org/D54438
llvm-svn: 352287
My last patch, D56989, moved the validation of whether a checker exists into
its constructor, but we do support statically linked (and non-plugin) checkers
that were do not have an entry in Checkers.td. However, the handling of this
happens after the creation of the CheckerRegistry object.
This patch fixes this bug by moving even this functionality into
CheckerRegistry's constructor.
llvm-svn: 352284
I added a new enum to CheckerInfo, so we can easily track whether the check is
explicitly enabled, explicitly disabled, or isn't specified in this regard.
Checkers belonging in the latter category may be implicitly enabled through
dependencies in the followup patch. I also made sure that this is done within
CheckerRegisty's constructor, leading to very significant simplifications in
its query-like methods.
Differential Revision: https://reviews.llvm.org/D56989
llvm-svn: 352282
Since pretty much all methods of CheckerRegistry has AnalyzerOptions as an
argument, it makes sense to just simply require it in it's constructor.
Differential Revision: https://reviews.llvm.org/D56988
llvm-svn: 352279
The actual implementation of unix.API features a dual-checker: two checkers in
one, even though they don't even interact at all. Split them up, as this is a
problem for establishing dependencies.
I added no new code at all, just merely moved it around.
Since the plist files change (and that's a benefit!) this patch isn't NFC.
Differential Revision: https://reviews.llvm.org/D55425
llvm-svn: 352278
Introduce the boolean ento::shouldRegister##CHECKERNAME(const LangOptions &LO)
function very similarly to ento::register##CHECKERNAME. This will force every
checker to implement this function, but maybe it isn't that bad: I saw a lot of
ObjC or C++ specific checkers that should probably not register themselves based
on some LangOptions (mine too), but they do anyways.
A big benefit of this is that all registry functions now register their checker,
once it is called, registration is guaranteed.
This patch is a part of a greater effort to reinvent checker registration, more
info here: D54438#1315953
Differential Revision: https://reviews.llvm.org/D55424
llvm-svn: 352277
As noted in https://bugs.llvm.org/show_bug.cgi?id=36651, the specialization for
isPodLike<std::pair<...>> did not match the expectation of
std::is_trivially_copyable which makes the memcpy optimization invalid.
This patch renames the llvm::isPodLike trait into llvm::is_trivially_copyable.
Unfortunately std::is_trivially_copyable is not portable across compiler / STL
versions. So a portable version is provided too.
Note that the following specialization were invalid:
std::pair<T0, T1>
llvm::Optional<T>
Tests have been added to assert that former specialization are respected by the
standard usage of llvm::is_trivially_copyable, and that when a decent version
of std::is_trivially_copyable is available, llvm::is_trivially_copyable is
compared to std::is_trivially_copyable.
As of this patch, llvm::Optional is no longer considered trivially copyable,
even if T is. This is to be fixed in a later patch, as it has impact on a
long-running bug (see r347004)
Note that GCC warns about this UB, but this got silented by https://reviews.llvm.org/D50296.
Differential Revision: https://reviews.llvm.org/D54472
llvm-svn: 351701
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
Add a defensive check against an invalid destructor in the CFG.
Unions with fields with destructors have their own destructor implicitly
deleted. Due to a bug in the CFG we're still trying to evaluate them
at the end of the object's lifetime and crash because we are unable
to find the destructor's declaration.
rdar://problem/47362608
Differential Revision: https://reviews.llvm.org/D56899
llvm-svn: 351610
This is especially crucial for reports related to use-after-move of
standard library objects.
rdar://problem/47338505
Differential Revision: https://reviews.llvm.org/D56824
llvm-svn: 351500
SymbolReaper now realizes that our liveness analysis isn't sharp enough
to discriminate between liveness of, say, variables and their fields.
Surprisingly, this didn't quite work before: having a variable live only
through Environment (eg., calling a C++ method on a local variable
as the last action ever performed on that variable) would not keep the
region value symbol of a field of that variable alive.
It would have been broken in the opposite direction as well, but both
Environment and RegionStore use the scanReachableSymbols mechanism for finding
live symbols regions within their values, and due to that they accidentally
end up marking the whole chain of super-regions as live when at least one
sub-region is known to be live.
It is now a direct responsibility of SymbolReaper to maintain this invariant,
and a unit test was added in order to make sure it stays that way.
Differential Revision: https://reviews.llvm.org/D56632
rdar://problem/46914108
llvm-svn: 351499
This is not NFC strictly speaking, since it unifies CleanupAttr handling,
so that out parameters now also understand it.
Differential Revision: https://reviews.llvm.org/D56759
llvm-svn: 351394
Summary:
https://reviews.llvm.org/D54862 removed the usages of `ASTContext&` from
within the `CXXMethodDecl::getThisType` method. Remove the parameter
altogether, as well as all usages of it. This does not result in any
functional change because the parameter was unused since
https://reviews.llvm.org/D54862.
Test Plan: check-clang
Reviewers: akyrtzi, mikael
Reviewed By: mikael
Subscribers: mehdi_amini, dexonsmith, cfe-commits
Differential Revision: https://reviews.llvm.org/D56509
llvm-svn: 350914
Several headers would fail to compile if other headers were not previously
included. The usual issue is that a class is forward declared, but the
full definition is needed. The requirement for the definition is use of
isa/dyn_cast or calling functions of pointer-packed data types such as
DenseMap or PointerIntPair. Add missing includes to these headers.
SVals.h required an out-of-line method definition in the .cpp file to avoid
circular inclusion of headers with BasicValueFactory.h
llvm-svn: 350913
We need to be able to emit the diagnostic at PreImplicitCall,
and the patch implements this functionality.
However, for now the need for emitting such diagnostics is not all that great:
it is only necessary to not crash when emitting a false positive due to an
unrelated issue of having dead symbol collection not working properly.
Coming up with a non-false-positive test seems impossible with the current
set of checkers, though it is likely to be needed for good things as well
in the future.
Differential Revision: https://reviews.llvm.org/D56042
rdar://problem/46911462
llvm-svn: 350907
The complicated machinery for passing the summary log around is actually
only used for one thing! To figure out whether the "dealloc" message was
sent.
Since I have tried to extend it for other uses and failed (it's actually
very hard to use), I think it's much better to simply use a tag and
remove the summary log altogether.
Differential Revision: https://reviews.llvm.org/D56228
llvm-svn: 350864
Make sure all checks for attributes go through a centralized function,
which checks whether attribute handling is enabled, and performs
validation. The type of the attribute is returned.
Sadly, metaprogramming is required as attributes have no sensible static
getters.
Differential Revision: https://reviews.llvm.org/D56222
llvm-svn: 350862
Summary: The LocationE parameter of evalStore is documented as "The location expression that is stored to". When storing from an increment / decrement operator this was not satisfied. In user code this causes an inconsistency between the SVal and Stmt parameters of checkLocation.
Reviewers: NoQ, dcoughlin, george.karpenkov
Reviewed By: NoQ
Subscribers: xazax.hun, baloghadamsoftware, szepet, a.sidorin, mikhail.ramalho, Szelethus, donat.nagy, dkrupp, cfe-commits
Differential Revision: https://reviews.llvm.org/D55701
llvm-svn: 350528
Previously, argument effects were stored in a method variable, which was
effectively global.
The global state was reset at each (hopefully) entrance point to the
summary construction,
and every function could modify it.
Differential Revision: https://reviews.llvm.org/D56036
llvm-svn: 350057
This patch is a different approach to landing the reverted r349701.
It is expected to have the same object (memory region) treated as if it has
different types in different program points. The correct behavior for
RegionStore when an object is stored as an object of type T1 but loaded as
an object of type T2 is to store the object as if it has type T1 but cast it
to T2 during load.
Note that the cast here is some sort of a "reinterpret_cast" (even in C). For
instance, if you store an integer and load a float, you won't get your integer
represented as a float; instead, you will get garbage.
Admit that we cannot perform the cast and return an unknown value.
Differential Revision: https://reviews.llvm.org/D55875
rdar://problem/45062567
llvm-svn: 349984
The fix done in D55465 did not previously apply when the function was inlined.
rdar://46889541
Differential Revision: https://reviews.llvm.org/D55976
llvm-svn: 349876
Previously, we were not printing a note at all if at least one of the parameters was not annotated.
rdar://46888422
Differential Revision: https://reviews.llvm.org/D55972
llvm-svn: 349875
If it ends with "Retain" like CFRetain and returns a CFTypeRef like CFRetain,
then it is not necessarily a CFRetain. But it is indeed true that these two
return something retained.
Differential Revision: https://reviews.llvm.org/D55907
rdar://problem/39390714
llvm-svn: 349862
This adds anchors to all of the documented checks so that you can directly link to a check by a stable name. This is useful because the SARIF file format has a field for specifying a URI to documentation for a rule and some viewers, like CodeSonar, make use of this information. These links are then exposed through the SARIF exporter.
llvm-svn: 349812
This reverts commit r349701.
The patch was incorrect. The whole point of CastRetrievedVal()
is to handle the case in which the type from which the cast is made
(i.e., the "type" of value `V`) has nothing to do with the type of
the region it was loaded from (i.e., `R->getValueType()`).
Differential Revision: https://reviews.llvm.org/D55875
rdar://problem/45062567
llvm-svn: 349798
Replace multiple comparisons of getOS() value with FreeBSD, NetBSD,
OpenBSD and DragonFly with matching isOS*BSD() methods. This should
improve the consistency of coding style without changing the behavior.
Direct getOS() comparisons were left whenever used in switch or switch-
like context.
Differential Revision: https://reviews.llvm.org/D55916
llvm-svn: 349752
It is expected to have the same object (memory region) treated as if it has
different types in different program points. The correct behavior for
RegionStore when an object is stored as an object of type T1 but loaded as
an object of type T2 is to store the object as if it has type T1 but cast it
to T2 during load.
Note that the cast here is some sort of a "reinterpret_cast" (even in C). For
instance, if you store a float and load an integer, you won't have your float
rounded to an integer; instead, you will have garbage.
Admit that we cannot perform the cast as long as types we're dealing with are
non-trivial (neither integers, nor pointers).
Of course, if the cast is not necessary (eg, T1 == T2), we can still load the
value just fine.
Differential Revision: https://reviews.llvm.org/D55875
rdar://problem/45062567
llvm-svn: 349701
Static Analyzer processes the program function-by-function, sometimes diving
into other functions ("inlining" them). When an object is returned from an
inlined function, Return Value Optimization is modeled, and the returned object
is constructed at its return location directly.
When an object is returned from the function from which the analysis has started
(the top stack frame of the analysis), the return location is unknown. Model it
with a SymbolicRegion based on a conjured symbol that is specifically tagged for
that purpose, because this is generally the correct way to symbolicate
unknown locations in Static Analyzer.
Fixes leak false positives when an object is returned from top frame in C++17:
objects that are put into a SymbolicRegion-based memory region automatically
"escape" and no longer get reported as leaks. This only applies to C++17 return
values with destructors, because it produces a redundant CXXBindTemporaryExpr
in the call site, which confuses our liveness analysis. The actual fix
for liveness analysis is still pending, but it is no longer causing problems.
Additionally, re-enable temporary destructor tests in C++17.
Differential Revision: https://reviews.llvm.org/D55804
rdar://problem/46217550
llvm-svn: 349696
It turns out that it's not all that uncommon to have a C++ override of, say,
memcpy that receives a structure (or two) by reference (or by value, if it's
being copied from) and copies memory from it (or into it, if it's passed
by reference). In this case the argument will be of structure type (recall that
expressions of reference type do not exist: instead, C++ classifies expressions
into prvalues and lvalues and xvalues).
In this scenario we crash because we are trying to assume that, say,
a memory region is equal to an empty CompoundValue (the non-lazy one; this is
what makeZeroVal() return for compound types and it represents prvalue of
an object that is initialized with an empty initializer list).
Add defensive checks.
Differential Revision: https://reviews.llvm.org/D55873
rdar://problem/45366551
llvm-svn: 349682
Accidentally commited earlier with the same commit title, but really it
should've been
"Revert rC349283 '[analyzer][MallocChecker] Improve warning messages on double-delete errors'"
llvm-svn: 349344
Re-using a moved-from local variable is most likely a bug because there's
rarely a good motivation for not introducing a separate variable instead.
We plan to keep emitting such warnings by default.
Introduce a flag that allows disabling warnings on local variables that are
not of a known move-unsafe type. If it doesn't work out as we expected,
we'll just flip the flag.
We still warn on move-unsafe objects and unsafe operations on known move-safe
objects.
Differential Revision: https://reviews.llvm.org/D55730
llvm-svn: 349327
This re-applies commit r349226 that was reverted in r349233 due to failures
on clang-x64-windows-msvc.
Specify enum type as unsigned for use in bit field. Otherwise overflows
may cause UB.
Differential Revision: https://reviews.llvm.org/D55388
llvm-svn: 349326
StaticAnalyzer uses the CFG-based RelaxedLiveVariables analysis in order to,
in particular, figure out values of which expressions are still needed.
When the expression becomes "dead", it is garbage-collected during
the dead binding scan.
Expressions that constitute branches/bodies of control flow statements,
eg. `E1' in `if (C1) E1;' but not `E2' in `if (C2) { E2; }', were kept alive
for too long. This caused false positives in MoveChecker because it relies
on cleaning up loop-local variables when they go out of scope, but some of those
live-for-too-long expressions were keeping a reference to those variables.
Fix liveness analysis to correctly mark these expressions as dead.
Add a debug checker, debug.DumpLiveStmts, in order to test expressions liveness.
Differential Revision: https://reviews.llvm.org/D55566
llvm-svn: 349320
This patch merely reorganizes some things, and features no functional change.
In detail:
* Provided documentation, or moved existing documentation in more obvious
places.
* Added dividers. (the //===----------===// thing).
* Moved getAllocationFamily, printAllocDeallocName, printExpectedAllocName and
printExpectedDeallocName in the global namespace on top of the file where
AllocationFamily is declared, as they are very strongly related.
* Moved isReleased and MallocUpdateRefState near RefState's definition for the
same reason.
* Realloc modeling was very poor in terms of variable and structure naming, as
well as documentation, so I renamed some of them and added much needed docs.
* Moved function IdentifierInfos to a separate struct, and moved isMemFunction,
isCMemFunction adn isStandardNewDelete inside it. This makes the patch affect
quite a lot of lines, should I extract it to a separate one?
* Moved MallocBugVisitor out of MallocChecker.
* Preferred switches to long else-if branches in some places.
* Neatly organized some RUN: lines.
Differential Revision: https://reviews.llvm.org/D54823
llvm-svn: 349281
Now that CheckerRegistry lies in Frontend, we can finally eliminate
ClangCheckerRegistry. Fortunately, this also provides us with a
DiagnosticsEngine, so I went ahead and removed some parameters from it's
methods.
Differential Revision: https://reviews.llvm.org/D54437
llvm-svn: 349280
ClangCheckerRegistry is a very non-obvious, poorly documented, weird concept.
It derives from CheckerRegistry, and is placed in lib/StaticAnalyzer/Frontend,
whereas it's base is located in lib/StaticAnalyzer/Core. It was, from what I can
imagine, used to circumvent the problem that the registry functions of the
checkers are located in the clangStaticAnalyzerCheckers library, but that
library depends on clangStaticAnalyzerCore. However, clangStaticAnalyzerFrontend
depends on both of those libraries.
One can make the observation however, that CheckerRegistry has no place in Core,
it isn't used there at all! The only place where it is used is Frontend, which
is where it ultimately belongs.
This move implies that since
include/clang/StaticAnalyzer/Checkers/ClangCheckers.h only contained a single function:
class CheckerRegistry;
void registerBuiltinCheckers(CheckerRegistry ®istry);
it had to re purposed, as CheckerRegistry is no longer available to
clangStaticAnalyzerCheckers. It was renamed to BuiltinCheckerRegistration.h,
which actually describes it a lot better -- it does not contain the registration
functions for checkers, but only those generated by the tblgen files.
Differential Revision: https://reviews.llvm.org/D54436
llvm-svn: 349275
Renaming collectCheckers to getEnabledCheckers
Changing the functionality to acquire all enabled checkers, rather then collect
checkers for a specific CheckerOptInfo (for example, collecting all checkers for
{ "core", true }, which meant enabling all checkers from the core package, which
was an unnecessary complication).
Removing CheckerOptInfo, instead of storing whether the option was claimed via a
field, we handle errors immediately, as getEnabledCheckers can now access a
DiagnosticsEngine. Realize that the remaining information it stored is directly
accessible through AnalyzerOptions.CheckerControlList.
Fix a test with -analyzer-disable-checker -verify accidentally left in.
llvm-svn: 349274
Right now they report to have one parameter with null decl,
because initializing an ArrayRef of pointers with a nullptr
yields an ArrayRef to an array of one null pointer.
Fixes a crash in the OSObject section of RetainCountChecker.
Differential Revision: https://reviews.llvm.org/D55671
llvm-svn: 349229
The checker wasn't prepared to see the dealloc message sent to the class itself
rather than to an instance, as if it was +dealloc.
Additionally, it wasn't prepared for pure-unknown or undefined self values.
The new guard covers that as well, but it is annoying to test because
both kinds of values shouldn't really appear and we generally want to
get rid of all of them (by modeling unknown values with symbols and
by warning on use of undefined values before they are used).
The CHECK: directive for FileCheck at the end of the test looks useless,
so i removed it.
Differential Revision: https://reviews.llvm.org/D55680
llvm-svn: 349228
Use trackExpressionValue() (previously known as trackNullOrUndefValue())
to track index value in the report, so that the user knew
what Static Analyzer thinks the index is.
Additionally, implement printState() to help debugging the checker later.
Differential Revision: https://reviews.llvm.org/D55458
llvm-svn: 349227
Calling operator*() or operator->() on a null STL smart pointer is
undefined behavior.
Smart pointers are specified to become null after being moved from.
So we can't warn on arbitrary method calls, but these two operators
definitely make no sense.
The new bug is fatal because it's an immediate UB,
unlike other use-after-move bugs.
The work on a more generic null smart pointer dereference checker
is still pending.
Differential Revision: https://reviews.llvm.org/D55388
llvm-svn: 349226
Some C++ standard library classes provide additional guarantees about their
state after move. Suppress warnings on such classes until a more precise
behavior is implemented. Warnings for locals are not suppressed anyway
because it's still most likely a bug.
Differential Revision: https://reviews.llvm.org/D55307
llvm-svn: 349191
If a moved-from object is passed into a conservatively evaluated function
by pointer or by reference, we assume that the function may reset its state.
Make sure it doesn't apply to const pointers and const references. Add a test
that demonstrates that it does apply to rvalue references.
Additionally, make sure that the object is invalidated when its contents change
for reasons other than invalidation caused by evaluating a call conservatively.
In particular, when the object's fields are manipulated directly, we should
assume that some sort of reset may be happening.
Differential Revision: https://reviews.llvm.org/D55289
llvm-svn: 349190
Functional changes include:
* The run.files property is now an array instead of a mapping.
* fileLocation objects now have a fileIndex property specifying the array index into run.files.
* The resource.rules property is now an array instead of a mapping.
* The result object was given a ruleIndex property that is an index into the resource.rules array.
* rule objects now have their "id" field filled out in addition to the name field.
* Updated the schema and spec version numbers to 11-28.
llvm-svn: 349188
- explicit_bzero has limited scope/usage only for security/crypto purposes but is non-optimisable version of memset/0 and bzero.
- explicit_memset has similar signature and semantics as memset but is also a non-optimisable version.
Reviewers: NoQ
Reviewed By: NoQ
Differential Revision: https://reviews.llvm.org/D54592
llvm-svn: 348884
Memoization dose not seem to be necessary, as other statement visitors
run just fine without it,
and in fact seems to be causing memory corruptions.
Just removing it instead of investigating the root cause.
rdar://45945002
Differential Revision: https://reviews.llvm.org/D54921
llvm-svn: 348822
This is currently a diagnostics, but might be upgraded to an error in the future,
especially if we introduce os_return_on_success attributes.
rdar://46359592
Differential Revision: https://reviews.llvm.org/D55530
llvm-svn: 348820
Escaping to void * / uint64_t / others non-OSObject * should stop tracking,
as such functions can have heterogeneous semantics depending on context,
and can not always be annotated.
rdar://46439133
Differential Revision: https://reviews.llvm.org/D55465
llvm-svn: 348675
Allow enabling and disabling tracking of ObjC/CF objects
separately from tracking of OS objects.
Differential Revision: https://reviews.llvm.org/D55400
llvm-svn: 348638
The option has no tests, is not used anywhere, and is actually
incorrect: it prints the line number without the reference to a file,
which can be outright incorrect.
Differential Revision: https://reviews.llvm.org/D55385
llvm-svn: 348637
Summary:
With a new switch we may be able to print to stderr if a new TU is being loaded
during CTU. This is very important for higher level scripts (like CodeChecker)
to be able to parse this output so they can create e.g. a zip file in case of
a Clang crash which contains all the related TU files.
Reviewers: xazax.hun, Szelethus, a_sidorin, george.karpenkov
Subscribers: whisperity, baloghadamsoftware, szepet, rnkovacs, a.sidorin, mikhail.ramalho, donat.nagy, dkrupp,
Differential Revision: https://reviews.llvm.org/D55135
llvm-svn: 348594
Previously, the iterator range checker only warned upon dereferencing of
iterators outside their valid range as well as increments and decrements of
out-of-range iterators where the result remains out-of-range. However, the C++
standard is more strict than this: decrementing begin() or incrementing end()
results in undefined behaviour even if the iterator is not dereferenced
afterwards. Coming back to the range once out-of-range is also undefined.
This patch corrects the behaviour of the iterator range checker: warnings are
given for any operation whose result is ahead of begin() or past the end()
(which is the past-end iterator itself, thus now we are speaking of past
past-the-end).
Differential Revision: https://reviews.llvm.org/D53812
llvm-svn: 348245
If an iterator is represented by a derived C++ class but its comparison operator
is for its base the iterator checkers cannot recognize the iterators compared.
This results in false positives in very straightforward cases (range error when
dereferencing an iterator after disclosing that it is equal to the past-the-end
iterator).
To overcome this problem we always use the region of the topmost base class for
iterators stored in a region. A new method called getMostDerivedObjectRegion()
was added to the MemRegion class to get this region.
Differential Revision: https://reviews.llvm.org/D54466
llvm-svn: 348244
Includes "resize" and "shrink" because they can reset the object to a known
state in certain circumstances.
Differential Revision: https://reviews.llvm.org/D54563
llvm-svn: 348235
The warning piece traditionally describes the bug itself, i.e.
"The bug is a _____", eg. "Attempt to delete released memory",
"Resource leak", "Method call on a moved-from object".
Event pieces produced by the visitor are usually in a present tense, i.e.
"At this moment _____": "Memory is released", "File is closed",
"Object is moved".
Additionally, type information is added into the event pieces for STL objects
(in order to highlight that it is in fact an STL object), and the respective
event piece now mentions that the object is left in an unspecified state
after it was moved, which is a vital piece of information to understand the bug.
Differential Revision: https://reviews.llvm.org/D54560
llvm-svn: 348229
In general case there use-after-move is not a bug. It depends on how the
move-constructor or move-assignment is implemented.
In STL, the convention that applies to most classes is that the move-constructor
(-assignment) leaves an object in a "valid but unspecified" state. Using such
object without resetting it to a known state first is likely a bug. Objects
Local value-type variables are special because due to their automatic lifetime
there is no intention to reuse space. If you want a fresh object, you might
as well make a new variable, no need to move from a variable and than re-use it.
Therefore, it is not always a bug, but it is obviously easy to suppress when it
isn't, and in most cases it indeed is - as there's no valid intention behind
the intentional use of a local after move.
This applies not only to local variables but also to parameter variables,
not only of value type but also of rvalue reference type (but not to lvalue
references).
Differential Revision: https://reviews.llvm.org/D54557
llvm-svn: 348210
The checker had extra code to clean up memory regions that were sticking around
in the checker without ever being cleaned up due to the bug that was fixed in
r347953. Because of that, if a region was moved from, then became dead,
and then reincarnated, there were false positives.
Why regions are even allowed to reincarnate is a separate story. Luckily, this
only happens for local regions that don't produce symbols when loaded from.
No functional change intended. The newly added test demonstrates that even
though no cleanup is necessary upon destructor calls, the early return
cannot be removed. It was not failing before the patch.
Differential Revision: https://reviews.llvm.org/D54372
llvm-svn: 348208
This follows the Static Analyzer's tradition to name checkers after
things in which they find bugs, not after bugs they find.
Differential Revision: https://reviews.llvm.org/D54556
llvm-svn: 348201
This continues the work that was started in r342313, which now gets applied to
object-under-construction tracking in C++. Makes it possible to debug
temporaries by dumping exploded graphs again.
Differential Revision: https://reviews.llvm.org/D54459
llvm-svn: 348200
Buildbot failures were caused by an unrelated UB that was introduced in r347943
and fixed in r347970.
Also the revision was incorrectly specified as r344580 during revert.
Differential Revision: https://reviews.llvm.org/D54017
llvm-svn: 348188
It seems the two failing tests can be simply fixed after r348037
Fix 3 cases in Analysis/builtin-functions.cpp
Delete the bad CodeGen/builtin-constant-p.c for now
llvm-svn: 348053
Kept the "indirect_builtin_constant_p" test case in test/SemaCXX/constant-expression-cxx1y.cpp
while we are investigating why the following snippet fails:
extern char extern_var;
struct { int a; } a = {__builtin_constant_p(extern_var)};
llvm-svn: 348039
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
From what I can see, this should be the last patch needed to replicate macro
argument expansions.
Differential Revision: https://reviews.llvm.org/D52988
llvm-svn: 348025
During the review of D41938 a condition check with an early exit accidentally
slipped into a branch, leaving the other branch unprotected. This may result in
an assertion later on. This hotfix moves this contition check outside of the
branch.
Differential Revision: https://reviews.llvm.org/D55051
llvm-svn: 347981
Don't generate a checker-tagged node unconditionally on the first
checkDeadSymbols callback when no pointers are tracked.
This is a tiny performance optimization; it may change the behavior slightly
by making Static Analyzer bail out on max-nodes one node later (which is good)
but any test would either break for no good reason or become useless
every time someone sneezes.
Differential Revision: https://reviews.llvm.org/D54013
llvm-svn: 347955
The checker suppresses warnings on paths on which a nonnull value is assumed
to be nullable. This probably deserves a warning, but it's a separate story.
Now, because dead symbol collection fires in pretty random moments,
there sometimes was a situation when dead symbol collection fired after
computing a parameter but before actually evaluating call enter into the
function, which triggered the suppression when the argument was null
in the first place earlier than the obvious warning for null-to-nonnull
was emitted, causing false negatives.
Only trigger the suppression for symbols, not for concrete values.
It is impossible to constrain a concrete value post-factum because
it is impossible to constrain a concrete value at all.
This covers all the necessary cases because by the time we reach the call,
symbolic values should be either not constrained to null, or already collapsed
into concrete null values. Which in turn happens because they are passed through
the Store, and the respective collapse is implemented as part of getSVal(),
which is also weird.
Differential Revision: https://reviews.llvm.org/D54017
llvm-svn: 347954
It's an old bug that consists in stale references to symbols remaining in the
GDM if they disappear from other program state sections as a result of any
operation that isn't the actual dead symbol collection. The most common example
here is:
FILE *fp = fopen("myfile.txt", "w");
fp = 0; // leak of file descriptor
In this example the leak were not detected previously because the symbol
disappears from the public part of the program state due to evaluating
the assignment. For that reason the checker never receives a notification
that the symbol is dead, and never reports a leak.
This patch not only causes leak false negatives, but also a number of other
problems, including false positives on some checkers.
What's worse, even though the program state contains a finite number of symbols,
the set of symbols that dies is potentially infinite. This means that is
impossible to compute the set of all dead symbols to pass off to the checkers
for cleaning up their part of the GDM.
No longer compute the dead set at all. Disallow iterating over dead symbols.
Disallow querying if any symbols are dead. Remove the API for marking symbols
as dead, as it is no longer necessary. Update checkers accordingly.
Differential Revision: https://reviews.llvm.org/D18860
llvm-svn: 347953
The "free" call frees the object immediately, ignoring the reference count.
Sadly, it is actually used in a few places, so we need to model it.
Differential Revision: https://reviews.llvm.org/D55092
llvm-svn: 347950
Move visitors to the implementation file, move a complicated logic into
a function.
Differential Revision: https://reviews.llvm.org/D55036
llvm-svn: 347946
Attempt to get a fully qualified name from AST if an SVal corresponding
to the object is not available.
Differential Revision: https://reviews.llvm.org/D55034
llvm-svn: 347944
If the object is a temporary, and there is no variable it binds to,
let's at least print out the object name in order to help differentiate
it from other temporaries.
rdar://45175098
Differential Revision: https://reviews.llvm.org/D55033
llvm-svn: 347943
Summary: Left only the constructors that are actually required, and marked the move constructors as deleted. They are not used anymore and we were never sure they've actually worked correctly.
Reviewers: george.karpenkov, NoQ
Reviewed By: george.karpenkov
Subscribers: xazax.hun, baloghadamsoftware, szepet, a.sidorin, Szelethus, donat.nagy, dkrupp
Differential Revision: https://reviews.llvm.org/D54974
llvm-svn: 347777
This was reverted in r347656 due to me thinking it caused a miscompile of
Chromium. Turns out it was the Chromium code that was broken.
llvm-svn: 347756
This caused a miscompile in Chrome (see crbug.com/908372) that's
illustrated by this small reduction:
static bool f(int *a, int *b) {
return !__builtin_constant_p(b - a) || (!(b - a));
}
int arr[] = {1,2,3};
bool g() {
return f(arr, arr + 3);
}
$ clang -O2 -S -emit-llvm a.cc -o -
g() should return true, but after r347417 it became false for some reason.
This also reverts the follow-up commits.
r347417:
> Re-Reinstate 347294 with a fix for the failures.
>
> Don't try to emit a scalar expression for a non-scalar argument to
> __builtin_constant_p().
>
> Third time's a charm!
r347446:
> The result of is.constant() is unsigned.
r347480:
> A __builtin_constant_p() returns 0 with a function type.
r347512:
> isEvaluatable() implies a constant context.
>
> Assume that we're in a constant context if we're asking if the expression can
> be compiled into a constant initializer. This fixes the issue where a
> __builtin_constant_p() in a compound literal was diagnosed as not being
> constant, even though it's always possible to convert the builtin into a
> constant.
r347531:
> A "constexpr" is evaluated in a constant context. Make sure this is reflected
> if a __builtin_constant_p() is a part of a constexpr.
llvm-svn: 347656
Summary:
A __builtin_constant_p may end up with a constant after inlining. Use
the is.constant intrinsic if it's a variable that's in a context where
it may resolve to a constant, e.g., an argument to a function after
inlining.
Reviewers: rsmith, shafik
Subscribers: jfb, kristina, cfe-commits, nickdesaulniers, jyknight
Differential Revision: https://reviews.llvm.org/D54355
llvm-svn: 347294
CheckerOptInfo feels very much out of place in CheckerRegistration.cpp, so I
moved it to CheckerRegistry.h.
Differential Revision: https://reviews.llvm.org/D54397
llvm-svn: 347157
Especially with pointees, a lot of meaningless reports came from uninitialized
regions that were already reported. This is fixed by storing all reported fields
to the GDM.
Differential Revision: https://reviews.llvm.org/D51531
llvm-svn: 347153
Extend the alpha.core.Conversion checker to handle implicit converions
where a too large integer value is converted to a floating point type. Each
floating point type has a range where it can exactly represent all integers; we
emit a warning when the integer value is above this range. Although it is
possible to exactly represent some integers which are outside of this range
(those that are divisible by a large enough power of 2); we still report cast
involving those, because their usage may lead to bugs. (For example, if 1<<24
is stored in a float variable x, then x==x+1 holds.)
Patch by: Donát Nagy!
Differential Revision: https://reviews.llvm.org/D52730
llvm-svn: 347006
With z3-4.8.1:
../tools/clang/lib/StaticAnalyzer/Core/Z3ConstraintManager.cpp:49:40: error:
'Z3_get_error_msg_ex' was not declared in this scope
../tools/clang/lib/StaticAnalyzer/Core/Z3ConstraintManager.cpp:49:40: note:
suggested alternative: 'Z3_get_error_msg'
Formerly used Z3_get_error_msg_ex() as one could find in z3-4.7.1 states:
"Retained function name for backwards compatibility within v4.1"
And it is implemented only as a forwarding call:
return Z3_get_error_msg(c, err);
Differential Revision: https://reviews.llvm.org/D54391
llvm-svn: 346635
Summary:
Compound literals, enums, file-scoped arrays, etc. require their
initializers and size specifiers to be constant. Wrap the initializer
expressions in a ConstantExpr so that we can easily check for this later
on.
Reviewers: rsmith, shafik
Reviewed By: rsmith
Subscribers: cfe-commits, jyknight, nickdesaulniers
Differential Revision: https://reviews.llvm.org/D53921
llvm-svn: 346455
One of the reasons why AnalyzerOptions is so chaotic is that options can be
retrieved from the command line whenever and wherever. This allowed for some
options to be forgotten for a looooooong time. Have you ever heard of
"region-store-small-struct-limit"? In order to prevent this in the future, I'm
proposing to restrict AnalyzerOptions' interface so that only checker options
can be retrieved without special getters. I would like to make every option be
accessible only through a getter, but checkers from plugins are a thing, so I'll
have to figure something out for that.
This also forces developers who'd like to add a new option to register it
properly in the .def file.
This is done by
* making the third checker pointer parameter non-optional, and checked by an
assert to be non-null.
* I added new, but private non-checkers option initializers, meant only for
internal use,
* Renamed these methods accordingly (mind the consistent name for once with
getBooleanOption!):
- getOptionAsString -> getCheckerStringOption,
- getOptionAsInteger -> getCheckerIntegerOption
* The 3 functions meant for initializing data members (with the not very
descriptive getBooleanOption, getOptionAsString and getOptionAsUInt names)
were renamed to be overloads of the getAndInitOption function name.
* All options were in some way retrieved via getCheckerOption. I removed it, and
moved the logic to getStringOption and getCheckerStringOption. This did cause
some code duplication, but that's the only way I could do it, now that checker
and non-checker options are separated. Note that the non-checker version
inserts the new option to the ConfigTable with the default value, but the
checker version only attempts to find already existing entries. This is how
it always worked, but this is clunky and I might end reworking that too, so we
can eventually get a ConfigTable that contains the entire configuration of the
analyzer.
Differential Revision: https://reviews.llvm.org/D53483
llvm-svn: 346113
Windows buildbots break with the previous commit '[analyzer][PlistMacroExpansion]
Part 2.: Retrieving the macro name and primitive expansion'. This patch attempts
to solve this issue.
llvm-svn: 346112
This patch adds a couple new functions to acquire the macro's name, and also
expands it, although it doesn't expand the arguments, as seen from the test files
Differential Revision: https://reviews.llvm.org/D52794
llvm-svn: 346095
This exposes a (known) CodeGen bug: it can't cope with emitting lvalue
expressions that denote non-odr-used but usable-in-constant-expression
variables. See PR39528 for a testcase.
Reverted for now until that issue can be fixed.
llvm-svn: 346065
Interestingly, this many year old (when I last looked I remember 2010ish)
checker was committed without any tests, so I thought I'd implement them, but I
was shocked to see how I barely managed to get it working. The code is severely
outdated, I'm not even sure it has ever been used, so I'd propose to move it
back into alpha, and possibly even remove it.
Differential Revision: https://reviews.llvm.org/D53856
llvm-svn: 345990
I'm in the process of refactoring AnalyzerOptions. The main motivation behind
here is to emit warnings if an invalid -analyzer-config option is given from the
command line, and be able to list them all.
In this patch, I'm moving all analyzer options to a def file, and move 2 enums
to global namespace.
Differential Revision: https://reviews.llvm.org/D53277
llvm-svn: 345986
I'm in the process of refactoring AnalyzerOptions. The main motivation behind
here is to emit warnings if an invalid -analyzer-config option is given from
the command line, and be able to list them all.
In this patch, I found some flags that should've been used as checker options,
or have absolutely no mention of in AnalyzerOptions, or are nonexistent.
- NonLocalizedStringChecker now uses its "AggressiveReport" flag as a checker
option
- lib/StaticAnalyzer/Frontend/ModelInjector.cpp now accesses the "model-path"
option through a getter in AnalyzerOptions
- -analyzer-config path-diagnostics-alternate=false is not a thing, I removed it,
- lib/StaticAnalyzer/Checkers/AllocationDiagnostics.cpp and
lib/StaticAnalyzer/Checkers/AllocationDiagnostics.h are weird, they actually
only contain an option getter. I deleted them, and fixed RetainCountChecker
to get it's "leak-diagnostics-reference-allocation" option as a checker option,
- "region-store-small-struct-limit" has a proper getter now.
Differential Revision: https://reviews.llvm.org/D53276
llvm-svn: 345985
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
SARIF allows you to export descriptions about rules that are present in the SARIF log. Expose the help text table generated into Checkers.inc as the rule's "full description" and export all of the rules present in the analysis output. This information is useful for analysis result viewers like CodeSonar.
llvm-svn: 345874
This removes the Step property (which can be calculated by consumers trivially), and updates the schema and version numbers accordingly.
llvm-svn: 345823
MallocChecker no longer thinks that operator delete() that accepts the size of
the object to delete (available since C++14 or under -fsized-deallocation)
is some weird user-defined operator. Instead, it handles it like normal delete.
Additionally, it exposes a regression in NewDelete-intersections.mm's
testStandardPlacementNewAfterDelete() test, where the diagnostic is delayed
from before the call of placement new into the code of placement new
in the header. This happens because the check for pass-into-function-after-free
for placement arguments is located in checkNewAllocator(), which happens after
the allocator is inlined, which is too late. Move this use-after-free check
into checkPreCall instead, where it works automagically because the guard
that prevents it from working is useless and can be removed as well.
This commit causes regressions under -analyzer-config
c++-allocator-inlining=false but this option is essentially unsupported
because the respective feature has been enabled by default quite a while ago.
Differential Revision: https://reviews.llvm.org/D53543
llvm-svn: 345802
Trusting summaries of inlined code would require a more thorough work,
as the current approach was causing too many false positives, as the new
example in test. The culprit lies in the fact that we currently escape
all variables written into a field (but not passed off to unknown
functions!), which can result in inconsistent behavior.
rdar://45655344
Differential Revision: https://reviews.llvm.org/D53902
llvm-svn: 345746
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
We haven't supported compiling ObjC1 for a long time (and never will again), so
there isn't any reason to keep these separate. This patch replaces
LangOpts::ObjC1 and LangOpts::ObjC2 with LangOpts::ObjC.
Differential revision: https://reviews.llvm.org/D53547
llvm-svn: 345637
This allows users to specify SARIF (https://github.com/oasis-tcs/sarif-spec) as the output from the clang static analyzer so that the results can be read in by other tools, such as extensions to Visual Studio and VSCode, as well as static analyzers like CodeSonar.
llvm-svn: 345628
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.
llvm-svn: 345562
The existing padding checker skips classes that have any base classes.
This patch allows the checker to traverse very simple cases:
classes that have no fields and have exactly one base class.
This is important mostly in the case of array declarations.
Patch by Max Bernstein!
Test plan: make check-all
Differential revision: https://reviews.llvm.org/D53206
llvm-svn: 345558
This has been a long time coming. Note the usage of AnalyzerOptions: I'll need
it for D52742, and added it in rC343620. The main motivation for this was that
I'll need to add yet another parameter to every single function, and some
functions would reach their 10th parameter with that change.
llvm-svn: 345531
Nodes which have only one predecessor and only one successor can not
always be hidden, even if all states are the same.
An additional condition is needed: the predecessor may have only one successor.
This can be seen on this example:
```
A
/ \
B C
\ /
D
```
Nodes B and C can not be hidden even if all nodes in the graph have the
same state.
Differential Revision: https://reviews.llvm.org/D53735
llvm-svn: 345341
Previously, OSDynamicCast was modeled as an identity.
This is not correct: the output of OSDynamicCast may be zero even if the
input was not zero (if the class is not of desired type), and thus the
modeling led to false positives.
Instead, we are doing eager state split:
in one branch, the returned value is identical to the input parameter,
and in the other branch, the returned value is zero.
This patch required a substantial refactoring of canEval infrastructure,
as now it can return different function summaries, and not just true/false.
rdar://45497400
Differential Revision: https://reviews.llvm.org/D53624
llvm-svn: 345338
Summary:
This patch moves the last method in `Z3ConstraintManager` to `SMTConstraintManager`: `canReasonAbout()`.
The `canReasonAbout()` method checks if a given `SVal` can be encoded in SMT. I've added a new method to the SMT API to return true if a solver can encode floating-point arithmetics and it was enough to make `canReasonAbout()` solver independent.
As an annoying side-effect, `Z3ConstraintManager` is pretty empty now and only (1) creates the Z3 solver object by calling `CreateZ3Solver()` and (2) instantiates `SMTConstraintManager`. Maybe we can get rid of this class altogether in the future: a `CreateSMTConstraintManager()` method that does (1) and (2) and returns the constraint manager object?
Reviewers: george.karpenkov, NoQ
Reviewed By: george.karpenkov
Subscribers: mehdi_amini, xazax.hun, szepet, a.sidorin, dexonsmith, Szelethus, donat.nagy, dkrupp
Differential Revision: https://reviews.llvm.org/D53694
llvm-svn: 345284
Summary:
Getting an `APSInt` from the model always returned an unsigned integer because of the unused parameter.
This was not breaking any test case because no code relies on the actual value of the integer returned here, but rather it is only used to check if a symbol has more than one solution in `getSymVal`.
Reviewers: NoQ, george.karpenkov
Reviewed By: george.karpenkov
Subscribers: xazax.hun, szepet, a.sidorin, Szelethus, donat.nagy, dkrupp
Differential Revision: https://reviews.llvm.org/D53637
llvm-svn: 345283
trackNullOrUndefValue is a long and confusing name,
and it does not actually reflect what the function is doing.
Give a function a new name, with a relatively clear semantics.
Also remove some dead code.
Differential Revision: https://reviews.llvm.org/D52758
llvm-svn: 345064
This patch is a part of https://reviews.llvm.org/D48456 in an attempt to split
the casting logic up into smaller patches. This contains the code for casting
from fixed point types to boolean types.
Differential Revision: https://reviews.llvm.org/D53308
llvm-svn: 345063
As rightly pointed out by @NoQ, nonloc::LazyCompoundVals were only used to acquire a constructed object's region, which isn't what LazyCompoundVal was made for.
Differential Revision: https://reviews.llvm.org/D51300
llvm-svn: 344879
I'm in the process of refactoring AnalyzerOptions. The main motivation behind
here is to emit warnings if an invalid -analyzer-config option is given from the
command line, and be able to list them all.
This first NFC patch contains small modifications to make AnalyzerOptions.cpp a
little more consistent.
Differential Revision: https://reviews.llvm.org/D53274
llvm-svn: 344870
The GDMIndex functions return a pointer that's used as a key for looking up
data, but addresses of local statics defined in header files aren't the same
across shared library boundaries and the result is that analyzer plugins
can't access this data.
Event types are uniqued by using the addresses of a local static defined
in a header files, but it isn't the same across shared library boundaries
and plugins can't currently handle ImplicitNullDerefEvents.
Patches by Joe Ranieri!
Differential Revision: https://reviews.llvm.org/D52905
Differential Revision: https://reviews.llvm.org/D52906
llvm-svn: 344823
In C++17, when class C has large alignment value, a special case of
overload resolution rule kicks in for expression new C that causes the aligned
version of operator new() to be called. The aligned new has two arguments:
size and alignment. However, the new-expression has only one "argument":
the construct-expression for C(). This causes a false positive in
core.CallAndMessage's check for matching number of arguments and number
of parameters.
Update CXXAllocatorCall, which is a CallEvent sub-class for operator new calls
within new-expressions, so that the number of arguments always matched
the number of parameters.
rdar://problem/44738501
Differential Revision: https://reviews.llvm.org/D52957
llvm-svn: 344539
We don't need a separate node for every symbol, because whenever the first
symbol leaks, a bug is emitted, the analysis is sinked, and the checker
callback immediately returns due to State variable turning into null,
so we never get to see the second leaking symbol.
Additionally, we are no longer able to break normal analysis while experimenting
with debug dumps.
Differential Revision: https://reviews.llvm.org/D52804
llvm-svn: 344538
This patch is a part of https://reviews.llvm.org/D48456 in an attempt to
split them up. This contains the code for casting between fixed point types
and other fixed point types.
The method for converting between fixed point types is based off the convert()
method in APFixedPoint.
Differential Revision: https://reviews.llvm.org/D50616
llvm-svn: 344530
Summary:
Enhanced support for Z3 in the cmake configuration of clang; now it is possible to specify any arbitrary Z3 install prefix (CLANG_ANALYZER_Z3_PREFIX) to cmake with lib (or bin) and include folders. Before the patch only in cmake default locations
were searched (https://cmake.org/cmake/help/v3.4/command/find_path.html).
Specifying any CLANG_ANALYZER_Z3_PREFIX will force also CLANG_ANALYZER_BUILD_Z3 to ON.
Removed also Z3 4.5 version requirement since it was not checked, and now Clang works with Z3 4.7
Reviewers: NoQ, george.karpenkov, mikhail.ramalho
Reviewed By: george.karpenkov
Subscribers: rnkovacs, NoQ, esteffin, george.karpenkov, delcypher, ddcc, mgorny, xazax.hun, szepet, a.sidorin, Szelethus
Tags: #clang
Differential Revision: https://reviews.llvm.org/D50818
llvm-svn: 344464
For now, tresting the cast as a no-op, and disregarding the case where
the output becomes null due to the type mismatch.
rdar://45174557
Differential Revision: https://reviews.llvm.org/D53156
llvm-svn: 344311
I've added a new functionality, the checker is now able to
detect and report fields pointing to themselves. I figured
this would fit well into the checker as there's no reason
for a pointer to point to itself instead of being nullptr.
Differential Revision: https://reviews.llvm.org/D51305
llvm-svn: 344242
Doesn't do much despite sounding quite bad, but fixes an exotic test case where
liveness of a nonloc::LocAsInteger array index is now evaluated correctly.
Differential Revision: https://reviews.llvm.org/D52667
llvm-svn: 343631
I intend to add a new flag macro-expnasions-as-events, and unfortunately
I'll only be able to convert the macro piece into an event one once I'm
about to emit it, due to the lack of an avaible Preprocessor object in
the BugReporter.
Differential Revision: https://reviews.llvm.org/D52787
llvm-svn: 343620
Summary:
Several improvements in preparation for the new backends.
Refactoring:
- Removed duplicated methods `fromBoolean`, `fromAPSInt`, `fromInt` and `fromAPFloat`. The methods `mkBoolean`, `mkBitvector` and `mkFloat` are now used instead.
- The names of the functions that convert BVs to FPs were swapped (`mkSBVtoFP`, `mkUBVtoFP`, `mkFPtoSBV`, `mkFPtoUBV`).
- Added a couple of comments in function calls.
Crosscheck encoding:
- Changed how constraints are encoded in the refutation manager so it doesn't start with (false OR ...). This change introduces one duplicated line (see file `BugReporterVisitors.cpp`, the `SMTConv::getRangeExpr is called twice, so I can remove this change if the duplication is a problem.
Reviewers: george.karpenkov, NoQ
Reviewed By: george.karpenkov
Subscribers: xazax.hun, szepet, a.sidorin, Szelethus
Differential Revision: https://reviews.llvm.org/D52365
llvm-svn: 343581
This is patch is a preparation for the proposed inclusion of macro expansions in the plist output.
Differential Revision: https://reviews.llvm.org/D52735
llvm-svn: 343511
Dumping graphs instead of opening them is often very useful,
e.g. for transfer or converting to SVG.
Basic sanity check for generated exploded graphs.
Differential Revision: https://reviews.llvm.org/D52637
llvm-svn: 343352
Commit r340984 causes a crash when a pointer to a completely unrelated type
UnrelatedT (eg., opaque struct pattern) is being casted from base class BaseT to
derived class DerivedT, which results in an ill-formed region
Derived{SymRegion{$<UnrelatedT x>}, DerivedT}.
Differential Revision: https://reviews.llvm.org/D52189
llvm-svn: 343051
Tests introduced in r329780 was disabled in r342317 because these tests
were accidentally testing dump infrastructure, when all they cared about was
how symbols relate to each other. So when dump infrastructure changed,
tests became annoying to maintain.
Add a new feature to ExprInspection: clang_analyzer_denote() and
clang_analyzer_explain(). The former adds a notation to a symbol, the latter
expresses another symbol in terms of previously denoted symbols.
It's currently a bit wonky - doesn't print parentheses and only supports
denoting atomic symbols. But it's even more readable that way.
Differential Revision: https://reviews.llvm.org/D52133
llvm-svn: 343048
Combine the two constructor overrides into a single ArrayRef constructor
to allow easier brace initializations and simplify how the respective field
is used internally.
Differential Revision: https://reviews.llvm.org/D51390
llvm-svn: 343037
When a checker maintains a program state trait that isn't a simple list/set/map, but is a combination of multiple lists/sets/maps (eg., a multimap - which may be implemented as a map from something to set of something), ProgramStateManager only contains the factory for the trait itself. All auxiliary lists/sets/maps need a factory to be provided by the checker, which is annoying.
So far two checkers wanted a multimap, and both decided to trick the
ProgramStateManager into keeping the auxiliary factory within itself
by pretending that it's some sort of trait they're interested in,
but then never using this trait but only using the factory.
Make this trick legal. Define a convenient macro.
One thing that becomes apparent once all pieces are put together is that
these two checkers are in fact using the same factory, because the type that
identifies it, ImmutableMap<const MemRegion *, ImmutableSet<SymbolRef>>,
is the same. This situation is different from two checkers registering similar
primitive traits.
Differential Revision: https://reviews.llvm.org/D51388
llvm-svn: 343035
This patch is a band-aid. A proper solution would be too change
trackNullOrUndefValue to only try to dereference the pointer when it is
relevant to the problem.
Differential Revision: https://reviews.llvm.org/D52435
llvm-svn: 342920
Assuming strlcat is used with strlcpy we check as we can if the last argument does not equal os not larger than the buffer.
Advising the proper usual pattern.
Reviewers: george.karpenkov, NoQ, MaskRay
Reviewed By: MaskRay
Differential Revision: https://reviews.llvm.org/D49722
llvm-svn: 342832
Modify the RetainCountChecker to perform state "adjustments" in
checkEndFunction, as performing work in PreStmt<ReturnStmt> does not
work with destructors.
The previous version made an implicit assumption that no code runs
after the return statement is executed.
rdar://43945028
Differential Revision: https://reviews.llvm.org/D52338
llvm-svn: 342770
Kristof Umann