| Age | Commit message (Collapse) | Author | Files | Lines |
|---|
|
(#161370)
Downstream, some change triggered an investigation if we could move a
checker callback from check::PostCall to eval::Call. After a lengthy
investigation that lead to ExprEngine::VisitCXXNewExpr we realized that
CXXNewExprs only trigger a PreCall and PostCall, but never an EvalCall.
It also had a FIXME that maybe it should trigger it.
Remember, it called `defaultEvalCall` which either inlines or
conservatively evaluates aka. invalidates the call. But never probes the
checker eval-calls to see if any would step in.
After implementing the changes to trigger the eval call for the
checkers, I realized that it doesn't really make sense because we are
eval-calling user-provided functions, that we can't be really sure about
their semantics, thus there is no generic way to properly implement the
eval call callback.
This touches on an important point. It only ever makes sense to eval
call functions that has a clear spec. such as standard functions, as
implementing the callback would prevent the inlining of that function,
risking regressing analysis quality if the implemented model is not
complete/correct enough.
As a conclusion, I opted for not exposing the eval call event to
checkers, in other words, keep everything as-is, but document my
journey.
CPP-6585
|
|
not pointers (#160511)
https://github.com/llvm/llvm-project/blob/main/clang/lib/StaticAnalyzer/Checkers/CStringChecker.cpp#L675-L678
mistakenly assumes that target expressions are of pointer type.
`CheckOverlap` has multiple call sites, most of which do not verify this
assumption. Therefore, the simplest solution is to verify it just before
that point.
|
|
transformation directive and "looprange" clause (#139293)
This change implements the fuse directive, `#pragma omp fuse`, as specified in the OpenMP 6.0, along with the `looprange` clause in clang.
This change also adds minimal stubs so flang keeps compiling (a full implementation in flang of this directive is still pending).
---------
Co-authored-by: Roger Ferrer Ibanez <roger.ferrer@bsc.es>
|
|
This PR uses the VFS to check `.model` files in the Clang static
analyzer to match the compiler's behavior for other input files.
|
|
@implementation is seen (#159947)
A @interface declaration with a raw pointer @property does not
necessarily mean it synthesizes ivar of that type. To determine whether
such a synthesis happens or not, we must wait for @implementation to
appear. So this PR makes the checker only validate @property then.
|
|
This change adds two new properties to each `result` object in the SARIF
log:
`partialFingerprints`: Contains the "issue hash" that the analyzer
already generates for each result, which can help identify a result
across runs even if surrounding code changes.
`hostedViewUri`: If running with `-analyzer-format=sarif-html`, this
property will now be emitted with the `file:` URL of the generated HTML
report for that result.
Along the way, I discovered an existing bug where the HTML diagnostic
consumer does not record the path to the generated report if another
compilation already created that report. This caused both the SARIF and
Plist consumers to be missing the link to the file in all but one of the
compilations in case of a warning in a header file. I added a new test
to ensure that the generated SARIF for each compilation contains the
property.
Finally, I made a few changes to the `normalize_sarif` processing in the
tests. I switched to `sed` to allow substitutions. The normalization now
removes directory components from `file:` URLs, replaces the `length`
property of the source file with a constant `-1`, and puts placeholders
in the values of the `version` properties rather than just deleting
them. The URL transformation in particular lets us verify that the right
filename is generated for each HTML report.
Fixes #158159
rdar://160410408
|
|
|
|
This PR ensures that the Clang static analyzer loads the config file
through the properly-configured VFS rather than through the bare real
file system. This enables correctly going through VFS overlays, unifying
the behavior with the rest of the compiler.
|
|
|
|
Add the support for OSObjectPtr, which behaves like RetainPtr.
|
|
std::ranges::all_of as [[clang::noescape]] (#158419)
The checker already had std::ranges hard-coded to treat its arguments as
[[clang::oescape]] but the fact std::ranges::all_of is implemented as a
struct instead of a function confused the checker and resuled in a
superflous warning being emitted for std::ranges::all_of.
This PR adds the support for recognizing DeclRefExpr which appears as a
callee in VisitCallExpr and generalizes the check in
shouldTreatAllArgAsNoEscape to walk up the decl contexts to find the
target namespaces such as std::ranges:: or a namespace and a function
like WTF::switchOn.
|
|
When `-analyzer-output=sarif-html` is specified, the analyzer was
reporting each warning to the console three times. This is because the
code to create the diagnostic consumers for `sarif-html` was calling the
code for `sarif` and `html` separately, each of which also creates its
own console text consumer. Then the `sarif-html` code itself created a
third.
The fix is to factor out the creation of the SARIF and HTML consumers
from the text consumers, so `sarif-html` just calls the code to create
the SARIF and HTML consumers without the text consumers.
The same fix applies for `plist-html`.
I've updated one of the SARIF tests to specify `sarif-html`. This test
would fail in the regular `-verify` validation due to the triplicated
warnings, but now passes with my fix.
Fixes #158103
rdar://160383710
|
|
The underflow reports of checker security.ArrayBound previously
displayed the (negative) byte offset of the accessed location; but those
numbers were a bit hard to decipher (especially when the elements were
large structs), so this commit replaces the byte offset with an index
value (if the offset is an integer multiple of the size of the accessed
element).
This change only affects the content of the messages; the checker will
find the same issues before and after this commit.
|
|
This PR is a part of the effort to make the VFS used in the compiler
more explicit and consistent.
Instead of creating the VFS deep within the compiler (in
`CompilerInstance::createFileManager()`), clients are now required to
explicitly call `CompilerInstance::createVirtualFileSystem()` and
provide the base VFS from the outside.
This PR also helps in breaking up the dependency cycle where creating a
properly configured `DiagnosticsEngine` requires a properly configured
VFS, but creating properly configuring a VFS requires the
`DiagnosticsEngine`.
Both `CompilerInstance::create{FileManager,Diagnostics}()` now just use
the VFS already in `CompilerInstance` instead of taking one as a
parameter, making the VFS consistent across the instance sub-object.
|
|
singleton. (#158012)
|
|
treated as unsafe if it's a template parameter (#157993)
When a template class takes Ref<T> as a template parameter and this
template parameter is used as the return value of a member function, the
return value can be treated as unsafe (i.e. emits a false positive). The
issue was caused by getCanonicalType sometimes converting Ref<T> to T.
Workaround this problem by avoid emitting a warning when the original,
non-canonical type is a safe pointer type.
|
|
Like other functions which results in abort, treat asm brk instruction
as trivial.
|
|
(#157629)
This PR adds the support for treating a function return value to be safe
if the function is annotated with NS_RETURNS_RETAINED or
CF_RETURNS_RETAINED.
|
|
Previously the checker `security.VAList` only tracked the set of the
inintialized `va_list` objects; this commit replaces this with a mapping
that can distinguish the "uninitialized" `va_list` objects from the
"already released" ones. Moreover, a new "unknown" state is introduced
to replace the slightly hacky solutions that checked the `Symbolic`
nature of the region.
In addition to sligthly improving the messages, this commit also
prepares the ground for a follow-up change that would introduce an
"indeterminate" state (which needs `va_end` but cannot be otherwise
used) to model the requirements of SEI CERT rule MSC39-C, which states:
> The va_list may be passed as an argument to another function, but
> calling va_arg() within that function causes the va_list to have an
> indeterminate value in the calling function. As a result, attempting
> to read variable arguments without reinitializing the va_list can have
> unexpected behavior.
|
|
(#157480)
Tldr;
We can't unconditionally trivially copy empty classes because that would
clobber the stored entries in the object that the optimized empty class
overlaps with.
This regression was introduced by #115918, which introduced other
clobbering issues, like the handling of `[[no_unique_address]]` fields
in #137252.
Read issue #157467 for the detailed explanation, but in short, I'd
propose reverting the original patch because these was a lot of problems
with it for arguably not much gain.
In particular, that patch was motivated by unifying the handling of
classes so that copy events would be triggered for a class no matter if
it had data members or not.
So in hindsight, it was not worth it.
I plan to backport this to clang-21 as well, and mention in the release
notes that this should fix the regression from clang-20.
PS: Also an interesting read [D43714](https://reviews.llvm.org/D43714)
in hindsight.
Fixes #157467
CPP-6574
|
|
This PR makes WebKit checkers treat NULL, 0, and nil like nullptr in
various places.
|
|
no-escape argument (#155025)
Fix a bug that webkit.UncountedLambdaCapturesChecker was erroneously
emitting a warning for a DeclRefExpr which is passed in as an argument
to a no-escape function argument. The bug was caused by findLambdaInArg
not adding DeclRefExpr to the ignored set even when a lambda was
identified as an argument.
|
|
```
clang/lib/StaticAnalyzer/Core/RegionStore.cpp: warning: bitwise operation between different enumeration types ('Kind' and '(anonymous namespace)::BindingKey::(unnamed enum at clang/lib/StaticAnalyzer/Core/RegionStore.cpp)') is deprecated [-Wdeprecated-anon-enum-enum-conversion]
XX | : P(r, k | Symbolic), Data(reinterpret_cast<uintptr_t>(Base)) {
| ~ ^ ~~~~~~~~
1 warning generated.
```
|
|
temporary object as an argument. (#155033)
Since a temporary object lives until the end of the statement, it's safe
to pass such an object as a function argument without explicitly
creating a CheckedRef/CheckedPtr in stack.
|
|
...to follow the capitalization style that was already applied within
the file by recent commit a80c393a9c498279a1ec9fd630535b9ff139b49f.
|
|
Previously the analyzer had an undocumented top-level checker group
called `valist` which offered several checkers to detect use of
uninitialized `va_list` objects and leaks of `va_list`s.
As the responsibilities of these checkers were messily intertwined and
`va_list` is a rarely used language feature, this commit simplifies the
situation by consolidating these checkers into a single checker which
will be called `security.VAList`.
Note that I'm choosing the capitalization `VAList` to be consistent with
the example of the AST node type `VAArgExpr`. I updated many variable
names to ensure that `ValistChecker.cpp` uses this spelling everywhere
(in CamelCased names). I'm planning to rename `ValistChecker.cpp` to
`VAListChecker.cpp` in a follow-up commit.
This commit also adds documentation for this checker in checkers.rst.
Among the test files I preserved the existing separation but I
eliminated some duplicated cases now that there is no way to separately
enable the old sub-checkers.
For the background of this change see also the discourse thread
https://discourse.llvm.org/t/clean-up-valist-checkers/85277/3
|
|
This pull improves the handling of placement new in`PointerArith`,
fixing one family of false positives, and one of negatives:
### False Positives
```cpp
Buffer buffer;
int* array = new (&buffer) int[10];
++array; // there should be no warning
```
The code above should flag the memory region `buffer` as reinterpreted,
very much as `reinterpret_cast` would do. Note that in this particular
case the placement new is inlined so the engine can track that `*array`
points to the same region as `buffer`.
This is no-op if the placement new is opaque.
### False Negatives
```cpp
Buffer buffer;
int* array = new (&buffer) int;
++array; // there should be a warning
```
In this case, there is an implicit cast to `void*` when calling
placement new. The memory region was marked as reinterpreted, and
therefore later pointer arithmetic will not raise. I have added a
condition to not consider a cast to `void*` as a reinterpretation, as an
array of voids does not make much sense.
There are still some limitations, of course. For starters, if a single
`int` is created in place of an array of `unsigned char` of exactly the
same size, it will still be considered as an array. A convoluted example
to make the point that I think it makes sense *not* to raise in this
situation is in the test `checkPlacementNewSlices`.
CPP-6868
|
|
Signature:
```c
size_t strxfrm(char *dest, const char *src, size_t n);
```
The modeling covers:
* `src` can never be null
* `dest` can be null only if n is 0, and then the return value is some unspecified positive integer
* `src` and `dest` must not overlap
* `dest` must have at least `n` bytes of capacity
* The return value can either be:
- `< n`, and the contents of the buffer pointed by `dest` is invalidated
- `>= n`, and the contents of the buffer pointed by `dest` is marked as undefined
CPP-6854
|
|
temporary objects (#152751)
Fixes PR60896 - false positive leak reports in various smart pointer scenarios including temporaries, inheritance, direct constructor calls, and mixed ownership patterns. Previously, the analyzer had no smart pointer handling in
`checkPostCall`, causing it to report false positive leaks for memory properly managed by smart pointers while missing legitimate raw pointer leaks.
---------
Co-authored-by: Balazs Benics <benicsbalazs@gmail.com>
Co-authored-by: Donát Nagy <donat.nagy@ericsson.com>
|
|
In CallAndMessageChecker the initialization of bug types was highly
obfuscated (even compared to other `mutable std::unique_ptr` hacks).
This commit cleans up this situation and removes a totally superfluous
hidded 'modeling' sub-checker that did not have any role apart from
obstructing the normal initialization of bug types.
(Note that if we need to reintroduce CallAndMessageModeling in the
future, we can do it cleanly within the CheckerFamily framework, so we
wouldn't need to re-obfuscate the bug type initialization.)
This change is mostly non-functional, the only visible change is the
removal of the hidden modeling checker.
|
|
When calculating the offset of a FieldRegion, we need to find out which
field index the given field refers to.
Previously, if for some reason the field was not found, then the `Idx`
passed to `Layout.getFieldOffset` was out of bounds and caused undefined
behavior when dereferenced an out of bounds element in
`ASTVector::FieldOffsets::operator[]`, which asserts this in debug
builds, but exposes the undefined behavior in release builds.
In this patch, I refactored how we enumerate the fields, and gracefully
handle the scenario where the field is not found.
That case is still bad, but at least it should not expose the undefined
behavior in release builds, and should assert earlier in debug builds
than before.
The motivational case was transformed into a regression test, that would
fail if no canonicalization would happen when creating a FieldRegion.
This was reduced from a production crash.
In the test case, due to how modules work, there would be multiple
copies of the same template specialization in the AST. This could lead
into inconsistent state when the FieldRegion's Decl was different to the
RecordDecl's field - because one referred to the first and the other to
the second. This made `calculateOffset` fail to compute the field index,
triggering the undefined behavior in production.
While this inconsistency gets fixed now, I think the assertion is still
warranted to avoid potential undefined behavior in release builds.
CPP-6691,CPP-6849
Co-authored-by: Marco Borgeaud <marco.borgeaud@sonarsource.com>
|
|
|
|
This PR is part of an effort to remove file system usage from the
command line parsing code. The reason for that is that it's impossible
to do file system access correctly without a configured VFS, and the VFS
can only be configured after the command line is parsed. I don't want to
intertwine command line parsing and VFS configuration, so I decided to
perform the file system access after the command line is parsed and the
VFS is configured - ideally right before the file system entity is used
for the first time.
This patch delays checking that `model-path` is an existing directory.
|
|
This reintroduces `Type.h`, having earlier been renamed to `TypeBase.h`,
as a redirection to `TypeBase.h`, and redirects most users to include
the former instead.
This is a preparatory patch for being able to provide inline definitions
for `Type` methods which would otherwise cause a circular dependency
with `Decl{,CXX}.h`.
Doing these operations into their own NFC patch helps the git rename
detection logic work, preserving the history.
This patch makes clang just a little slower to build (~0.17%), just
because it makes more code indirectly include `DeclCXX.h`.
|
|
This is a preparatory patch, to be able to provide inline definitions
for `Type` functions which depend on `Decl{,CXX}.h`. As the latter also
depends on `Type.h`, this would not be possible without some
reorganizing.
Splitting this rename into its own patch allows git to track this as a
rename, and preserve all git history, and not force any code
reformatting.
A later NFC patch will reintroduce `Type.h` as redirection to
`TypeBase.h`, rewriting most places back to directly including `Type.h`
instead of `TypeBase.h`, leaving only a handful of places where this is
necessary.
Then yet a later patch will exploit this by making more stuff inline.
|
|
|
|
TagDecls (#155463)
And make use of those.
These changes are split from prior PR #155028, in order to decrease the
size of that PR and facilitate review.
|
|
This changes a bunch of places which use getAs<TagType>, including
derived types, just to obtain the tag definition.
This is preparation for #155028, offloading all the changes that PR used
to introduce which don't depend on any new helpers.
|
|
cstring checker (#153498)
Prevent an assertion failure in the cstring checker when library
functions like memcpy are defined with non-default address spaces.
Adds a test for this case.
|
|
This patch replaces SmallSet<T *, N> with SmallPtrSet<T *, N>. Note
that SmallSet.h "redirects" SmallSet to SmallPtrSet for pointer
element types:
template <typename PointeeType, unsigned N>
class SmallSet<PointeeType*, N> : public SmallPtrSet<PointeeType*, N>
{};
We only have 30 instances that rely on this "redirection", with about
half of them under clang/. Since the redirection doesn't improve
readability, this patch replaces SmallSet with SmallPtrSet for pointer
element types.
I'm planning to remove the redirection eventually.
|
|
Addresses
https://github.com/llvm/llvm-project/pull/83027#discussion_r2264102109
We can only reach this part by a non-null `Ptr`, which also implies a
dereference of `PtrExpr`. Consequently, `PtrExpr` cannot be null here so
the check is pointless.
|
|
This is a major change on how we represent nested name qualifications in
the AST.
* The nested name specifier itself and how it's stored is changed. The
prefixes for types are handled within the type hierarchy, which makes
canonicalization for them super cheap, no memory allocation required.
Also translating a type into nested name specifier form becomes a no-op.
An identifier is stored as a DependentNameType. The nested name
specifier gains a lightweight handle class, to be used instead of
passing around pointers, which is similar to what is implemented for
TemplateName. There is still one free bit available, and this handle can
be used within a PointerUnion and PointerIntPair, which should keep
bit-packing aficionados happy.
* The ElaboratedType node is removed, all type nodes in which it could
previously apply to can now store the elaborated keyword and name
qualifier, tail allocating when present.
* TagTypes can now point to the exact declaration found when producing
these, as opposed to the previous situation of there only existing one
TagType per entity. This increases the amount of type sugar retained,
and can have several applications, for example in tracking module
ownership, and other tools which care about source file origins, such as
IWYU. These TagTypes are lazily allocated, in order to limit the
increase in AST size.
This patch offers a great performance benefit.
It greatly improves compilation time for
[stdexec](https://github.com/NVIDIA/stdexec). For one datapoint, for
`test_on2.cpp` in that project, which is the slowest compiling test,
this patch improves `-c` compilation time by about 7.2%, with the
`-fsyntax-only` improvement being at ~12%.
This has great results on compile-time-tracker as well:
This patch also further enables other optimziations in the future, and
will reduce the performance impact of template specialization resugaring
when that lands.
It has some other miscelaneous drive-by fixes.
About the review: Yes the patch is huge, sorry about that. Part of the
reason is that I started by the nested name specifier part, before the
ElaboratedType part, but that had a huge performance downside, as
ElaboratedType is a big performance hog. I didn't have the steam to go
back and change the patch after the fact.
There is also a lot of internal API changes, and it made sense to remove
ElaboratedType in one go, versus removing it from one type at a time, as
that would present much more churn to the users. Also, the nested name
specifier having a different API avoids missing changes related to how
prefixes work now, which could make existing code compile but not work.
How to review: The important changes are all in
`clang/include/clang/AST` and `clang/lib/AST`, with also important
changes in `clang/lib/Sema/TreeTransform.h`.
The rest and bulk of the changes are mostly consequences of the changes
in API.
PS: TagType::getDecl is renamed to `getOriginalDecl` in this patch, just
for easier to rebasing. I plan to rename it back after this lands.
Fixes #136624
Fixes https://github.com/llvm/llvm-project/issues/43179
Fixes https://github.com/llvm/llvm-project/issues/68670
Fixes https://github.com/llvm/llvm-project/issues/92757
|
|
This patch improves MallocChecker to detect use-after-free bugs when
a freed structure's field is passed by address (e.g., `&ptr->field`).
Previously, MallocChecker would miss such cases, as it only checked the
top-level symbol of argument values.
This patch analyzes the base region of arguments and extracts the
symbolic region (if any), allowing UAF detection even for field address
expressions.
Fixes #152446
|
|
Binding a value to location can happen when a new value is created or
when and existing value is updated. This modification exposes whether
the value binding happens at a declaration.
This helps simplify the hacky logic of the BindToImmutable checker.
|
|
This commit converts RetainCountChecker to the new checker family
framework that was introduced in the commit
6833076a5d9f5719539a24e900037da5a3979289
This commit also performs some minor cleanup around the parts that had
to be changed, but lots of technical debt still remains in this old
codebase.
|
|
CStringChecker had an AdditionOverflow bug type which was intended for a
situation where the analyzer concludes that the addition of two
size/length values overflows `size_t`.
I strongly suspect that the analyzer could emit bugs of this type in
certain complex corner cases (e.g. due to inaccurate cast modeling), but
these reports would be all false positives because in the real world the
sum of two size/length values is always far below SIZE_MAX. (Although
note that there was no test where the analyzer emitted a bug with this
type.)
To simplify the code (and perhaps eliminate false positives), I
eliminated this bug type and replaced code that emits it by a simple
`addSink()` call (because we still want to get rid of the execution
paths where the analyzer has invalid assumptions).
|
|
(#151908)
We sometimes don't know if the operand of [[assume]] is true/false, and
that's okay. We can just ignore the attribute in that case.
If we wanted something more fancy, we could bring the assumption to the
constraints, but dropping them should be just as fine for now.
Fixes #151854
|
|
This adds alpha.core.StoreToImmutable, a new alpha checker that detects
writes
to immutable memory regions, implementing part of SEI CERT Rule ENV30-C.
The
original proposal only handled global const variables, but this
implementation
extends it to also detect writes to:
- Local const variables
- String literals
- Const parameters and struct members
- Const arrays and pointers to const data
This checker is the continuation of the work started by zukatsinadze.
Discussion: https://reviews.llvm.org/D124244
|
|
This commit handles the following types:
- clang::ExternalASTSource
- clang::TargetInfo
- clang::ASTContext
- clang::SourceManager
- clang::FileManager
Part of cleanup #151026
|
|
sizeof was handled correctly, but __datasizeof and _Countof were not.
Fixes #151711
|
