Handle CXXUnresolvedConstructExpr in tryToFindPtrOrigin so that
constructing Ref, RefPtr, CheckedRef, CheckedPtr, ... constructed in
such a way that its type is unresolved at AST level will be still
treated as a safe pointer origin.
Also fix a bug in isPtrOfType that it was not recognizing
DeducedTemplateSpecializationType.
This is the second attempt to bring initial support for [[assume()]] in
the Clang Static Analyzer.
The first attempt (#116462) was reverted in
2b9abf0db2d106c7208b4372e662ef5df869e6f1 due to some weird failure in a
libcxx test involving `#pragma clang loop vectorize(enable)
interleave(enable)`.
The failure could be reduced into:
```c++
template <class ExecutionPolicy>
void transform(ExecutionPolicy) {
#pragma clang loop vectorize(enable) interleave(enable)
for (int i = 0; 0;) { // The DeclStmt of "i" would be added twice in the ThreadSafety analysis.
// empty
}
}
void entrypoint() {
transform(1);
}
```
As it turns out, the problem with the initial patch was this:
```c++
for (const auto *Attr : AS->getAttrs()) {
if (const auto *AssumeAttr = dyn_cast<CXXAssumeAttr>(Attr)) {
Expr *AssumeExpr = AssumeAttr->getAssumption();
if (!AssumeExpr->HasSideEffects(Ctx)) {
childrenBuf.push_back(AssumeExpr);
}
}
// Visit the actual children AST nodes.
// For CXXAssumeAttrs, this is always a NullStmt.
llvm::append_range(childrenBuf, AS->children()); // <--- This was not meant to be part of the "for" loop.
children = childrenBuf;
}
return;
```
The solution was simple. Just hoist it from the loop.
I also had a closer look at `CFGBuilder::VisitAttributedStmt`, where I also spotted another bug.
We would have added the CFG blocks twice if the AttributedStmt would have both the `[[fallthrough]]` and the `[[assume()]]` attributes. With my fix, it will only once add the blocks. Added a regression test for this.
Co-authored-by: Vinay Deshmukh <vinay_deshmukh AT outlook DOT com>
The triggered callbacks for the default copy constructed instance and
the instance used for initialization now behave in the same way. The LHS
already calls checkBind. To keep this consistent, checkLocation is now
triggered accordingly for the RHS.
Further details on the previous discussion:
https://discourse.llvm.org/t/checklocation-for-implicitcastexpr-of-kind-ck-noop/84729
---------
Authored-by: tobias.gruber <tobias.gruber@concentrio.io>
This statement level construct takes no clauses and has no associated
statement, and simply labels a number of array elements as valid for
caching. The implementation here is pretty simple, but it is a touch of
a special case for parsing, so the parsing code reflects that.
Fixes#116444.
Closed#127700 because I accidentally updated it in github UI.
### Current vs expected behavior
Previously, the result of a `CXXNewExpr` was not always list initialized
when using an initializer list.
In this example:
```
struct S { int x; };
void F() {
S *s = new S{1};
delete s;
}
```
there would be a binding of `s` to `compoundVal{1}`, but this isn't used
during later field binding lookup. After this PR, there is instead a
binding of `s->x` to `1`. This is the cause of #116444 since the field
binding lookup returns undefined in some cases currently.
### Changes
This PR swaps around the handling of typed value regions (seems to be
the usual region type when doing non-CXX-new-expr list initialization)
and symbolic regions (the result of the CXX new expr), so that symbolic
regions also get list initialized. In the below snippet, it swaps the
order of the two conditionals.
8529bd7b96/clang/lib/StaticAnalyzer/Core/RegionStore.cpp (L2426-L2448)
### Followup work
This PR only makes CSA do list init for `CXXNewExpr`s. After this, I
would like to make some changes to `RegionStoreMananger::bind` in how it
handles list initialization generally.
I've added some straightforward test cases here for the `new` expr with
a list initializer. I started adding some more before realizing that the
current general (not just `new` expr) list initialization could be
changed to handle more cases like list initialization of unions and
arrays (like https://github.com/llvm/llvm-project/issues/54910). Lmk if
it is preferred to then leave these test cases out for now.
Basically, we may leave the loop because if exhaust the fields, array
elements or other subobjects to initialize.
In that case, the Bindings may be in an exhausted state, thus no further
addBinding calls are allowed.
Let's harden the code by sprinkling some early exists in the recursive
dispatcher functions.
And to actually fix the issue, I added a check guarding the single
unguarded addBinding right after a loop I mentioned.
Fixes#129211
Previously checker objects were created by raw `new` calls, which
necessitated managing and calling their destructors explicitly. This
commit refactors this convoluted logic by introducing `unique_ptr`s that
to manage the ownership of these objects automatically.
This change can be thought of as stand-alone code quality improvement;
but I also have a secondary motivation that I'm planning further changes
in the checker registration/initialization process (to formalize our
tradition of multi-part checker) and this commit "prepares the ground"
for those changes.
A clang user pointed out that messages for the static analyzer undefined
assignment checker use the term ‘garbage’, which might have a negative
connotation to some users. This change updates the messages to use the
term ‘uninitialized’. This is the usual reason why a value is undefined
in the static analyzer and describes the logical error that a programmer
should take action to fix.
Out-of-bounds reads can also produce undefined values in the static
analyzer. The right long-term design is to have to the array bounds
checker cover out-of-bounds reads, so we do not cover that case in the
updated messages. The recent improvements to the array bounds checker
make it a candidate to add to the core set of checkers.
rdar://133418644
This PR adds alpha.webkit.UnretainedLocalVarsChecker by generalizing
RawPtrRefLocalVarsChecker. It checks local variables to NS or CF types
are guarded with a RetainPtr or not. The new checker is effective for NS
and CF types in Objective-C++ code without ARC, and it's effective for
CF types in code with ARC.
In our test pool, the max entry point RT was improved by this change:
1'181 seconds (~19.7 minutes) -> 94 seconds (1.6 minutes)
BTW, the 1.6 minutes is still really bad. But a few orders of magnitude
better than it was before.
This was the most servere RT edge-case as you can see from the numbers.
There are are more known RT bottlenecks, such as:
- Large environment sizes, and `removeDead`. See more about the failed
attempt on improving it at:
https://discourse.llvm.org/t/unsuccessful-attempts-to-fix-a-slow-analysis-case-related-to-removedead-and-environment-size/84650
- Large chunk of time could be spend inside `assume`, to reach a fixed
point. This is something we want to look into a bit later if we have
time.
We have 3'075'607 entry points in our test set.
About 393'352 entry points ran longer than 1 second when measured.
To give a sense of the distribution, if we ignore the slowest 500 entry
points, then the maximum entry point runs for about 14 seconds. These
500 slow entry points are in 332 translation units.
By this patch, out of the slowest 500 entry points, 72 entry points were
improved by at least 10x after this change.
We measured no RT regression on the "usual" entry points.
(The dashed lines represent the maximum of their RT)
CPP-6092
Well, yes. It's not pretty.
At least after this we would have a bit more unique pointers than
before.
This is for fixing the memory leak diagnosed by:
https://lab.llvm.org/buildbot/#/builders/24/builds/5580
And that caused the revert of #127409.
After these uptrs that patch can re-land finally.
In general, if we see an allocation, we associate the immutable memory
space with the constructed memory region.
This works fine if we see the allocation.
However, with symbolic regions it's not great because there we don't
know anything about their memory spaces, thus put them into the Unknown
space.
The unfortunate consequence is that once we learn about some aliasing
with this Symbolic Region, we can't change the memory space to the
deduced one.
In this patch, we open up the memory spaces as a trait, basically
allowing associating a better memory space with a memregion that
was created with the Unknown memory space.
As a side effect, this means that now queriing the memory space of a
region depends on the State, but many places in the analyzer, such as
the Store, doesn't have (and cannot have) access to the State by design.
This means that some uses must solely rely on the memspaces of the
region, but any other users should use the getter taking a State.
Co-authored-by: Balazs Benics <benicsbalazs@gmail.com>
If we were to delay checker constructions after we have a filled
ASTContext, then we could get rid of a bunch of "lazy initializers" in
checkers.
Turns out in the register functions of the checkers we could transfer
the ASTContext and all other things to checkers, so those could benefit
from in-class initializers and const fields.
For example, if a checker would take the ASTContext as the first field,
then the rest of the fields could use it in their in-class initializers,
so the ctor of the checker would only need to set a single field!
This would open uup countless opportunities for cleaning up the
asthetics of our checkers.
I attached a single use-case for the AST and the PP as demonstrating
purposes. You can imagine the rest.
**FYI: This may be a breaking change** to some downstream users that may
had some means to attach different listeners and what not to e.g. the
Preprocessor inside their checker register functions. Since we delay the
calls to these register fns after parsing is already done, they would of
course miss the parsing Preprocessor events.
Like a C++ member variable, every Objective-C++ instance variable must
be a RefPtr, Ref CheckedPtr, or CheckedRef to an object, not a raw
pointer or reference.
Currently there are many casts that are not modeled (i.e. ignored) by
the analyzer, which can cause paradox states (e.g. negative value stored
in `unsigned` variable) and false positive reports from various
checkers, e.g. from `security.ArrayBound`.
Unfortunately this issue is deeply rooted in the architectural
limitations of the analyzer (if we started to model the casts, it would
break other things). For details see the umbrella ticket
https://github.com/llvm/llvm-project/issues/39492
This commit adds an ugly hack in `security.ArrayBound` to silence most
of the false positives caused by this shortcoming of the engine.
Fixes#126884
This merges the functionality of ResolvedUnexpandedPackExpr into
FunctionParmPackExpr. I also added a test to show that
https://github.com/llvm/llvm-project/issues/125103 should be fixed with
this. I put the removal of ResolvedUnexpandedPackExpr in its own commit.
Let me know what you think.
Fixes#125103
this PR close#124474
when calling `read` and `recv` function for a non-block file descriptor
or a invalid file descriptor(`-1`), it will not cause block inside a
critical section.
this commit checks for non-block file descriptor assigned by `open`
function with `O_NONBLOCK` flag.
---------
Co-authored-by: Balazs Benics <benicsbalazs@gmail.com>
Two small stylistic improvements in code that I wrote ~a year ago:
1. fix a typo in a comment; and
2. simplify the code of `tryDividePair` by swapping the true and the
false branches.
This PR reapply https://github.com/llvm/llvm-project/pull/117437.
The issue has been fixed by the 2nd commit, we need to ignore parens in
CXXDefaultArgExpr when build CFG, because CXXDefaultArgExpr::getExpr
stripped off the top level FullExpr and ConstantExpr, ParenExpr may
occurres in the top level.
---------
Signed-off-by: yronglin <yronglin777@gmail.com>
Fixes#123459.
This changes checking of the returned expr to also look for memory
regions whose stack frame context was a child of the current stack frame
context, e.g., for cases like this given in #123459:
```
struct S { int *p; };
S f() {
S s;
{
int a = 1;
s.p = &a;
}
return s;
}
```
In WebKit, it's pretty common to capture "this" and "protectedThis"
where "protectedThis" is a guardian variable of type Ref or RefPtr for
"this". Furthermore, it's common for this "protectedThis" variable from
being passed to an inner lambda by std::move. Recognize this pattern so
that we don't emit warnings for nested inner lambdas.
To recognize this pattern, we introduce a new DenseSet,
ProtectedThisDecls, which contains every "protectedThis" we've
recognized to our subclass of DynamicRecursiveASTVisitor. This set is
now populated in "hasProtectedThis" and "declProtectsThis" uses this
DenseSet to determine a given value declaration constitutes a
"protectedThis" pattern or not.
Because hasProtectedThis and declProtectsThis had to be moved from the
checker class to the visitor class, it's now a responsibility of each
caller of visitLambdaExpr to check whether a given lambda captures
"this" without a "protectedThis" or not.
Finally, this PR improves the code to recognize "protectedThis" pattern
by allowing more nested CXXBindTemporaryExpr, CXXOperatorCallExpr, and
UnaryOperator expressions.
Added the support for annotating a constructor's argument with
[[clang::noescape]].
We explicitly ignore CXXConstructExpr which is visited as a part of
CallExpr so that construction of closures like Function,
CompletionHandler, etc... don't result in a warning.
Implement HLSL Aggregate Splat casting that handles splatting for arrays
and structs, and vectors if splatting from a vec1.
Closes#100609 and Closes#100619
Depends on #118842
This commit adds the new analyzer option
`assume-at-least-one-iteration`, which is `false` by default, but can be
set to `true` to ensure that the analyzer always assumes at least one
iteration in loops.
In some situations this "loop is skipped" execution path is an important
corner case that may evade the notice of the developer and hide
significant bugs -- however, there are also many situations where it's
guaranteed that at least one iteration will happen (e.g. some data
structure is always nonempty), but the analyzer cannot realize this and
will produce false positives when it assumes that the loop is skipped.
This commit refactors some logic around the implementation of the new
feature, but the only functional change is introducing the new analyzer
option. If the new option is left in its default state (false), then the
analysis is functionally equivalent to an analysis done with a version
before this commit.
Allow operator T&() in a member function which returns a const member
variable.
In particular, this will allow UniqueRef::operator T&() and
Ref::operator T&() to be treated as a safe pointer origin when they're
called on a const member.
Reapplying changes from https://github.com/llvm/llvm-project/pull/125638
after buildbot failures.
Buildbot failures fixed in 029e7e98dc9956086adc6c1dfb0c655a273fbee6,
latest commit on this PR. It was a problem with a declared class member
with same name as its type. Sorry!
Fixes https://github.com/llvm/llvm-project/issues/123459.
Previously, when the StackAddrEscapeChecker checked return values, it
did not scan into the structure of the return SVal. Now it does, and we
can catch some more false negatives that were already mocked out in the
tests in addition to those mentioned in
https://github.com/llvm/llvm-project/issues/123459.
The warning message at the moment for these newly caught leaks is not
great. I think they would be better if they had a better trace of why
and how the region leaks. If y'all are happy with these changes, I would
try to improve these warnings and work on normalizing this SVal checking
on the `checkEndFunction` side of the checker also.
Two of the stack address leak test cases now have two warnings, one
warning from return expression checking and another from`
checkEndFunction` `iterBindings` checking. For these two cases, I prefer
the warnings from the return expression checking, but I couldn't figure
out a way to drop the `checkEndFunction` without breaking other
`checkEndFunction` warnings that we do want. Thoughts here?
Previously commit 6e17ed9b04e5523cc910bf171c3122dcc64b86db deleted the
obsolete checker `alpha.security.ArrayBound` which was implemented in
`ArrayBoundChecker.cpp` and renamed the checker
`alpha.security.ArrayBoundV2` to `security.ArrayBound`.
This commit concludes that consolidation by renaming the source file
`ArrayBoundCheckerV2.cpp` to `ArrayBoundChecker.cpp` (which was "freed
up" by the previous commit).
