Don't enable c++-temp-dtor-inlining by default yet, due to this reference
counting pointe problem.
Otherwise the new mode seems stable and allows us to incrementally fix C++
problems in much less hacky ways.
Differential Revision: https://reviews.llvm.org/D43804
llvm-svn: 326461
Originally submitted as r326323 and r326324.
Reverted in r326432.
Reverting the commit was a mistake.
The breakage was due to invalid build files in our internal buildsystem,
CMakeLists did not have any cyclic dependencies.
llvm-svn: 326439
Also revert "[analyzer] Fix a compiler warning"
This reverts commits r326323 and r326324.
Reason: the commits introduced a cyclic dependency in the build graph.
This happens to work with cmake, but breaks out internal integrate.
llvm-svn: 326432
So I wrote a clang-tidy check to lint out redundant `isa`, `cast`, and
`dyn_cast`s for fun. This is a portion of what it found for clang; I
plan to do similar cleanups in LLVM and other subprojects when I find
time.
Because of the volume of changes, I explicitly avoided making any change
that wasn't highly local and obviously correct to me (e.g. we still have
a number of foo(cast<Bar>(baz)) that I didn't touch, since overloading
is a thing and the cast<Bar> did actually change the type -- just up the
class hierarchy).
I also tried to leave the types we were cast<>ing to somewhere nearby,
in cases where it wasn't locally obvious what we were dealing with
before.
llvm-svn: 326416
This is a security check that warns when both PROT_WRITE and PROT_EXEC are
set during mmap(). If mmap()ed memory is both writable and executable, it makes
it easier for the attacker to execute arbitrary code when contents of this
memory are compromised. Some applications require such mmap()s though, such as
different sorts of JIT.
Re-applied after a revert in r324167.
Temporarily stays in the alpha package because it needs a better way of
determining macro values that are not immediately available in the AST.
Patch by David Carlier!
Differential Revision: https://reviews.llvm.org/D42645
llvm-svn: 326405
The aim of this patch is to be minimal to enable incremental development of
the feature on the top of the tree. This patch should be an NFC when the
feature is turned off. It is turned off by default and still considered as
experimental.
Technical details are available in the EuroLLVM Talk:
http://llvm.org/devmtg/2017-03//2017/02/20/accepted-sessions.html#7
Note that the initial prototype was done by A. Sidorin et al.: http://lists.llvm.org/pipermail/cfe-dev/2015-October/045730.html
Contributions to the measurements and the new version of the code: Peter Szecsi, Zoltan Gera, Daniel Krupp, Kareem Khazem.
Differential Revision: https://reviews.llvm.org/D30691
llvm-svn: 326323
When a class forgets to initialize a field in the constructor, and then gets
copied around, a warning is emitted that the value assigned to a specific field
is undefined.
When the copy/move constructor is implicit (not written out in the code) but not
trivial (is not a trivial memory copy, eg. because members have an explicit copy
constructor), the body of such constructor is auto-generated in the AST.
In this case the checker's warning message is squeezed at the top of
the class declaration, and it gets hard to guess which field is at fault.
Fix the warning message to include the name of the field.
Differential Revision: https://reviews.llvm.org/D43798
llvm-svn: 326258
Throw away MallocChecker warnings that occur after releasing a pointer within a
destructor (or its callees) after performing C11 atomic fetch_add or fetch_sub
within that destructor (or its callees).
This is an indication that the destructor's class is likely a
reference-counting pointer. The analyzer is not able to understand that the
original reference count is usually large enough to avoid most use-after-frees.
Even when the smart pointer is a local variable, we still have these false
positives that this patch suppresses, because the analyzer doesn't currently
support atomics well enough.
Differential Revision: https://reviews.llvm.org/D43791
llvm-svn: 326249
The SVal for any empty C++ object is an UnknownVal. Because RegionStore does
not have binding extents, binding an empty object to an UnknownVal may
potentially overwrite existing bindings at the same offset.
Therefore, when performing a trivial copy of an empty object, don't try to
take the value of the object and bind it to the copy. Doing nothing is accurate
enough, and it doesn't screw any existing bindings.
Differential Revision: https://reviews.llvm.org/D43714
llvm-svn: 326247
Sometimes it is not known at compile time which temporary objects will be
constructed, eg. 'x ? A() : B()' or 'C() || D()'. In this case we track which
temporary was constructed to know how to properly call the destructor.
Once the construction context for temporaries was introduced, we moved the
tracking code to the code that investigates the construction context.
Bring back the old mechanism because construction contexts are not always
available yet - eg. in the case where a temporary is constructed without a
constructor expression, eg. returned from a function by value. The mechanism
should still go away eventually.
Additionally, fix a bug in the temporary cleanup code for the case when
construction contexts are not available, which could lead to temporaries
staying in the program state and increasing memory consumption.
Differential Revision: https://reviews.llvm.org/D43666
llvm-svn: 326246
If a variable or an otherwise a concrete typed-value region is being
placement-new'ed into, its dynamic type may change in arbitrary manners. And
when the region is used, there may be a third type that's different from both
the static and the dynamic type. It cannot be *completely* different from the
dynamic type, but it may be a base class of the dynamic type - and in this case
there isn't (and shouldn't be) any indication anywhere in the AST that there is
a derived-to-base cast from the dynamic type to the third type.
Perform a generic cast (evalCast()) from the third type to the dynamic type
in this case. From the point of view of the SVal hierarchy, this would have
produced non-canonical SVals if we used such generic cast in the normal case,
but in this case there doesn't seem to be a better option.
Differential Revision: https://reviews.llvm.org/D43659
llvm-svn: 326245
Automatic destructors are missing in the CFG in situations like
const int &x = C().x;
For now it's better to disable construction inlining, because inlining
constructors while doing nothing on destructors is very bad.
Differential Revision: https://reviews.llvm.org/D43689
llvm-svn: 326240
ConstructionContext is moved into a separate translation unit and is separated
into multiple classes. The "old" "raw" ConstructionContext is renamed into
ConstructionContextLayer - which corresponds to the idea of building the context
gradually layer-by-layer, but it isn't easy to use in the clients. Once
CXXConstructExpr is reached, layers that we've gathered so far are transformed
into the actual, "new-style" "flat" ConstructionContext, which is put into the
CFGConstructor element and has no layers whatsoever (until it actually needs
them, eg. aggregate initialization). The new-style ConstructionContext is
instead presented as a variety of sub-classes that enumerate different ways of
constructing an object in C++. There are 5 of these supported for now,
which is around a half of what needs to be supported.
The layer-by-layer buildup process is still a little bit weird, but it hides
all the weirdness in one place, that sounds like a good thing.
Differential Revision: https://reviews.llvm.org/D43533
llvm-svn: 326238
This patch uses the reference to MaterializeTemporaryExpr stored in the
construction context since r326014 in order to model that expression correctly.
When modeling MaterializeTemporaryExpr, instead of copying the raw memory
contents from the sub-expression's rvalue to a completely new temporary region,
that we conjure up for the lack of better options, we now have the better
option to recall the region into which the object was originally constructed
and declare that region to be the value of the expression, which is semantically
correct.
This only works when the construction context is available, which is worked on
independently.
The temporary region's liveness (in the sense of removeDeadBindings) is extended
until the MaterializeTemporaryExpr is resolved, in order to keep the store
bindings around, because it wouldn't be referenced from anywhere else in the
program state.
Differential Revision: https://reviews.llvm.org/D43497
llvm-svn: 326236
Fixes https://bugs.llvm.org/show_bug.cgi?id=36474
In general, getSVal API should be changed so that it does not crash on
some non-obvious conditions.
It should either be updated to require a type, or to return Optional<SVal>.
Differential Revision: https://reviews.llvm.org/D43801
llvm-svn: 326233
See D42775 for discussion. Turns out, just exploring nodes which
weren't explored first is not quite enough, as e.g. the first quick
traversal resulting in a report can mark everything as "visited", and
then subsequent traversals of the same region will get all the pitfalls
of DFS.
Priority queue-based approach in comparison shows much greater
increase in coverage and even performance, without sacrificing memory.
Differential Revision: https://reviews.llvm.org/D43354
llvm-svn: 326136
Bison/YACC generated files result in a very large number of (presumably)
false positives from the analyzer.
These false positives are "true" in a sense of the information analyzer
sees: assuming that the lexer can return any token at any point a number
of uninitialized reads does occur.
(naturally, the analyzer can not capture a complex invariant that
certain tokens can only occur under certain conditions).
Current fix simply stops analysis on those files.
I have examined a very large number of such auto-generated files, and
they do all start with such a comment.
Conversely, user code is very unlikely to contain such a comment.
rdar://33608161
Differential Revision: https://reviews.llvm.org/D43421
llvm-svn: 326135
Addresses https://bugs.llvm.org/show_bug.cgi?id=36206
rdar://37159026
A proper fix would be much harder, and would involve changing the
appropriate code in ExprEngine to be aware of the size limitations of
the type used for addressing.
Differential Revision: https://reviews.llvm.org/D43218
llvm-svn: 326122
The assertion gets exposed when changing the exploration order.
This is a quick hacky fix, but the intention is that if the nodes do
merge, it should not matter which predecessor should be traverse.
A proper fix would be not to traverse predecessors at all, as all
information relevant for any decision should be avilable locally.
rdar://37540480
Differential Revision: https://reviews.llvm.org/D42773
llvm-svn: 325977
In the wild, many cases of null pointer dereference, or uninitialized
value read occur because the value was meant to be initialized by the
inlined function, but did not, most often due to error condition in the
inlined function.
This change highlights the return branch taken by the inlined function,
in order to help user understand the error report and see why the value
was uninitialized.
rdar://36287652
Differential Revision: https://reviews.llvm.org/D41848
llvm-svn: 325976
When viewing the report in the collapsed mode the label signifying where
did the execution go is often necessary for properly understanding the
context.
Differential Revision: https://reviews.llvm.org/D43145
llvm-svn: 325975
The checker marks the locations where the analyzer creates sinks. However, it
can happen that the sink was created because of a loop which does not contain
condition statement, only breaks in the body. The exhausted block is the block
which should contain the condition but empty, in this case.
This change only emits this marking in order to avoid the undefined behavior.
Differential Revision: https://reviews.llvm.org/D42266
llvm-svn: 325693
Array destructors, like constructors, need to be called for each element of the
array separately. We do not have any mechanisms to do this in the analyzer,
so for now all we do is evaluate a single constructor or destructor
conservatively and give up. It automatically causes the necessary invalidation
and pointer escape for the whole array, because this is how RegionStore works.
Implement this conservative behavior for temporary destructors. This fixes the
crash on the provided test.
Differential Revision: https://reviews.llvm.org/D43149
llvm-svn: 325286
Temporary destructors fire at the end of the full-expression. It is reasonable
to attach the path note for entering/leaving the temporary destructor to its
CXXBindTemporaryExpr. This would not affect lifetime-extended temporaries with
their automatic destructors which aren't temporary destructors.
The path note may be confusing in the case of destructors after elidable copy
constructors.
Differential Revision: https://reviews.llvm.org/D43144
llvm-svn: 325284
Inline them if possible - a separate flag is added to control this.
The whole thing is under the cfg-temporary-dtors flag, off by default so far.
Temporary destructors are called at the end of full-expression. If the
temporary is lifetime-extended, automatic destructors kick in instead,
which are not addressed in this patch, and normally already work well
modulo the overally broken support for lifetime extension.
The patch operates by attaching the this-region to the CXXBindTemporaryExpr in
the program state, and then recalling it during destruction that was triggered
by that CXXBindTemporaryExpr. It has become possible because
CXXBindTemporaryExpr is part of the construction context since r325210.
Differential revision: https://reviews.llvm.org/D43104
llvm-svn: 325282
Don't look at the parent statement to figure out if the cxx-allocator-inlining
flag should kick in and prevent us from inlining the constructor within
a new-expression. We now have construction contexts for that purpose.
llvm-svn: 325278
Since r325210, in cfg-temporary-dtors mode, we can rely on the CFG to tell us
that we're indeed constructing a temporary, so we can trivially construct a
temporary region and inline the constructor.
Much like r325202, this is only done under the off-by-default
cfg-temporary-dtors flag because the temporary destructor, even if available,
will not be inlined and won't have the correct object value (target region).
Unless this is fixed, it is quite unsafe to inline the constructor.
If the temporary is lifetime-extended, the destructor would be an automatic
destructor, which would be evaluated with a "correct" target region - modulo
the series of incorrect relocations performed during the lifetime extension.
It means that at least, values within the object are guaranteed to be properly
escaped or invalidated.
Differential Revision: https://reviews.llvm.org/D43062
llvm-svn: 325211
EvalCallOptions were introduced in r324018 for allowing various parts of
ExprEngine to notify the inlining mechanism, while preparing for evaluating a
function call, of possible difficulties with evaluating the call that they
foresee. Then mayInlineCall() would still be a single place for making the
decision.
Use that mechanism for destructors as well - pass the necessary flags from the
CFG-element-specific destructor handlers.
Part of this patch accidentally leaked into r324018, which led into a change in
tests; this change is reverted now, because even though the change looked
correct, the underlying behavior wasn't. Both of these commits were not intended
to introduce any function changes otherwise.
Differential Revision: https://reviews.llvm.org/D42991
llvm-svn: 325209
This only affects the cfg-temporary-dtors mode - in this mode we begin inlining
constructors that are constructing function return values. These constructors
have a correct construction context since r324952.
Because temporary destructors are not only never inlined, but also don't have
the correct target region yet, this change is not entirely safe. But this
will be fixed in the subsequent commits, while this stays off behind the
cfg-temporary-dtors flag.
Lifetime extension for return values is still not modeled correctly.
Differential Revision: https://reviews.llvm.org/D42875
llvm-svn: 325202
In CFG, every DeclStmt has exactly one decl, which is always a variable.
It is also pointless to check that the initializer is the constructor because
that's how construction contexts work now.
llvm-svn: 325201
See reviews.llvm.org/M1 for evaluation, and
lists.llvm.org/pipermail/cfe-dev/2018-January/056718.html for
discussion.
Differential Revision: https://reviews.llvm.org/D42775
llvm-svn: 324956
Massive false positives were known to be caused by continuing the analysis
after a destructor with a noreturn attribute has been executed in the program
but not modeled in the analyzer due to being missing in the CFG.
Now that work is being done on enabling the modeling of temporary constructors
and destructors in the CFG, we need to make sure that the heuristic that
suppresses these false positives keeps working when such modeling is disabled.
In particular, different code paths open up when the corresponding constructor
is being inlined during analysis.
Differential Revision: https://reviews.llvm.org/D42779
llvm-svn: 324802
The analyzer was relying on peeking the next CFG element during analysis
whenever it was trying to figure out what object is being constructed
by a given constructor. This information is now available in the current CFG
element in all cases that were previously supported by the analyzer,
so no complicated lookahead is necessary anymore.
No functional change intended - the context in the CFG should for now be
available if and only if it was previously discoverable via CFG lookahead.
Differential Revision: https://reviews.llvm.org/D42721
llvm-svn: 324800
This expression may or may not be evaluated in compile time, so tracking the
result symbol is of potential interest. However, run-time offsetof is not yet
supported by the analyzer, so for now this callback is only there to assist
future implementation.
Patch by Henry Wong!
Differential Revision: https://reviews.llvm.org/D42300
llvm-svn: 324790
