This commit relands the patches for implementing P0960R3 and P1975R0,
which describe initializing aggregates via a parenthesized list.
The relanded commits are:
* 40c52159d3 - P0960R3 and P1975R0: Allow initializing aggregates from
a parenthesized list of values
* c77a91bb7b - Remove overly restrictive aggregate paren init logic
* 32d7aae04f - Fix a clang crash on invalid code in C++20 mode
This patch also fixes a crash in the original implementation.
Previously, if the input tried to call an implicitly deleted copy or
move constructor of a union, we would then try to initialize the union
by initializing it's first element with a reference to a union. This
behavior is incorrect (we should fail to initialize) and if the type of
the first element has a constructor with a single template typename
parameter, then Clang will explode. This patch fixes that issue by
checking that constructor overload resolution did not result in a
deleted function before attempting parenthesized aggregate
initialization.
Additionally, this patch also includes D140159, which contains some
minor fixes made in response to code review comments in the original
implementation that were made after that patch was submitted.
Co-authored-by: Sheng <ox59616e@gmail.com>
Fixes#54040, Fixes#59675
Reviewed By: ilya-biryukov
Differential Revision: https://reviews.llvm.org/D141546
When the engine processes a store to a variable, it will eventually call
`ExprEngine::processPointerEscapedOnBind()`. This function is supposed to
invalidate (put the given locations to an escape list) the locations
which we cannot reason about.
Unfortunately, local static variables are also put into this list.
This patch relaxes the guard condition, so that beyond stack variables,
static local variables are also ignored.
Differential Revision: https://reviews.llvm.org/D139534
The case of NULL stream passed to stream functions was reported by StreamChecker.
The same condition is checked already by StdLibraryFunctionsChecker and it is
enough to check at one place. The StreamChecker stops now analysis if a passed NULL
stream is encountered but generates no report.
This change removes a dependency between StdCLibraryFunctionArgs checker and
StreamChecker. There is now no more specific message reported by StreamChecker,
the previous weak-dependency is not needed. And StreamChecker can be used
without StdCLibraryFunctions checker or its ModelPOSIX option.
Reviewed By: Szelethus
Differential Revision: https://reviews.llvm.org/D137790
The stream handling functions `ftell`, `rewind`, `fgetpos`, `fsetpos`
are evaluated in the checker more exactly than before.
New tests are added to test behavior of the checker together with
StdLibraryFunctionsChecker. The option ModelPOSIX of that checker
affects if (most of) the stream functions are recognized, and checker
StdLibraryFunctionArgs generates warnings if constraints for arguments
are not satisfied. The state of `errno` is set by StdLibraryFunctionsChecker
too for every case in the stream functions.
StreamChecker works with the stream state only, does not set the errno state,
and is not dependent on other checkers.
Reviewed By: Szelethus
Differential Revision: https://reviews.llvm.org/D140395
Additional stream handling functions are added.
These are partially evaluated by StreamChecker, result of the addition is
check for more preconditions and construction of success and failure branches
with specific errno handling.
Reviewed By: Szelethus
Differential Revision: https://reviews.llvm.org/D140387
This feature causes clang to crash when compiling Chrome - see
https://crbug.com/1405031 and
https://github.com/llvm/llvm-project/issues/59675
Revert "[clang] Fix a clang crash on invalid code in C++20 mode."
This reverts commit 32d7aae04f.
Revert "[clang] Remove overly restrictive aggregate paren init logic"
This reverts commit c77a91bb7b.
Revert "[clang][C++20] P0960R3 and P1975R0: Allow initializing aggregates from a parenthesized list of values"
This reverts commit 40c52159d3.
Revert "Fix lldb option handling since e953ae5bbc (part 2)"
Revert "Fix lldb option handling since e953ae5bbc313fd0cc980ce021d487e5b5199ea4"
GCC build hangs on this bot https://lab.llvm.org/buildbot/#/builders/37/builds/19104
compiling CMakeFiles/obj.clangBasic.dir/Targets/AArch64.cpp.d
The bot uses GNU 11.3.0, but I can reproduce locally with gcc (Debian 12.2.0-3) 12.2.0.
This reverts commit caa713559b.
This reverts commit 06b90e2e9c.
This reverts commit e953ae5bbc.
This is a fairly large changeset, but it can be broken into a few
pieces:
- `llvm/Support/*TargetParser*` are all moved from the LLVM Support
component into a new LLVM Component called "TargetParser". This
potentially enables using tablegen to maintain this information, as
is shown in https://reviews.llvm.org/D137517. This cannot currently
be done, as llvm-tblgen relies on LLVM's Support component.
- This also moves two files from Support which use and depend on
information in the TargetParser:
- `llvm/Support/Host.{h,cpp}` which contains functions for inspecting
the current Host machine for info about it, primarily to support
getting the host triple, but also for `-mcpu=native` support in e.g.
Clang. This is fairly tightly intertwined with the information in
`X86TargetParser.h`, so keeping them in the same component makes
sense.
- `llvm/ADT/Triple.h` and `llvm/Support/Triple.cpp`, which contains
the target triple parser and representation. This is very intertwined
with the Arm target parser, because the arm architecture version
appears in canonical triples on arm platforms.
- I moved the relevant unittests to their own directory.
And so, we end up with a single component that has all the information
about the following, which to me seems like a unified component:
- Triples that LLVM Knows about
- Architecture names and CPUs that LLVM knows about
- CPU detection logic for LLVM
Given this, I have also moved `RISCVISAInfo.h` into this component, as
it seems to me to be part of that same set of functionality.
If you get link errors in your components after this patch, you likely
need to add TargetParser into LLVM_LINK_COMPONENTS in CMake.
Differential Revision: https://reviews.llvm.org/D137838
This avoids the continuous API churn when upgrading things to use
std::optional and makes trivial string replace upgrades possible.
I tested this with GCC 7.5, the oldest supported GCC I had around.
Differential Revision: https://reviews.llvm.org/D140332
std::optional::value() has undesired exception checking semantics and is
unavailable in older Xcode (see _LIBCPP_AVAILABILITY_BAD_OPTIONAL_ACCESS). The
call sites block std::optional migration.
This makes `ninja clang` work in the absence of llvm::Optional::value.
value() has undesired exception checking semantics and calls
__throw_bad_optional_access in libc++. Moreover, the API is unavailable without
_LIBCPP_NO_EXCEPTIONS on older Mach-O platforms (see
_LIBCPP_AVAILABILITY_BAD_OPTIONAL_ACCESS).
This fixes clang.
This patch implements P0960R3, which allows initialization of aggregates
via parentheses.
As an example:
```
struct S { int i, j; };
S s1(1, 1);
int arr1[2](1, 2);
```
This patch also implements P1975R0, which fixes the wording of P0960R3
for single-argument parenthesized lists so that statements like the
following are allowed:
```
S s2(1);
S s3 = static_cast<S>(1);
S s4 = (S)1;
int (&&arr2)[] = static_cast<int[]>(1);
int (&&arr3)[2] = static_cast<int[2]>(1);
```
This patch was originally authored by @0x59616e and completed by
@ayzhao.
Fixes#54040, Fixes#54041
Co-authored-by: Sheng <ox59616e@gmail.com>
Full write up : https://discourse.llvm.org/t/c-20-rfc-suggestion-desired-regarding-the-implementation-of-p0960r3/63744
Reviewed By: ilya-biryukov
Differential Revision: https://reviews.llvm.org/D129531
The checker applies constraints in a sequence and adds new nodes for these states.
If a constraint violation is found this sequence should be stopped with a sink
(error) node. Instead the `generateErrorNode` did add a new error node as a new
branch that is parallel to the other node sequence, the other branch was not
stopped and analysis was continuing on that invalid branch.
To add an error node after any previous node a new version of `generateErrorNode`
is needed, this function is added here and used by `StdLibraryFunctionsChecker`.
The added test executes a situation where the checker adds a number of
constraints before it finds a constraint violation.
Reviewed By: NoQ
Differential Revision: https://reviews.llvm.org/D137722
The NullabilityChecker has a very early policy decision that non-inlined
property accesses will be inferred as returning nonnull, despite nullability
annotations to the contrary. This decision eliminates false positives related
to very common code patterns that look like this:
if (foo.prop) {
[bar doStuffWithNonnull:foo.prop];
}
While this probably represents a correct nil-check, the analyzer can't
determine correctness without gaining visibility into the property
implementation.
Unfortunately, inferring nullable properties as nonnull comes at the cost of
significantly reduced code coverage. My goal here is to enable detection of
many property-related nullability violations without a large increase
in false positives.
The approach is to introduce a heuristic: after accessing the value of
a property, if the analyzer at any time proves that the property value is
nonnull (which would happen in particular due to a nil-check conditional),
then subsequent property accesses on that code path will be *inferred*
as nonnull. This captures the pattern described above, which I believe
to be the dominant source of false positives in real code.
https://reviews.llvm.org/D131655
With this patch, the solver can infer results for not equal (!=) operator
over Ranges as well. This also fixes the issue of comparison between
different types, by first converting the RangeSets to the resulting type,
which then can be used for comparisons.
Patch by Manas.
Reviewed By: steakhal
Differential Revision: https://reviews.llvm.org/D112621
This patch mechanically replaces None with std::nullopt where the
compiler would warn if None were deprecated. The intent is to reduce
the amount of manual work required in migrating from Optional to
std::optional.
This is part of an effort to migrate from llvm::Optional to
std::optional:
https://discourse.llvm.org/t/deprecating-llvm-optional-x-hasvalue-getvalue-getvalueor/63716
Mixing LLVM and Clang address spaces can result in subtle bugs, and there
is no need for this hook to use the LLVM IR level address spaces.
Most of this change is just replacing zero with LangAS::Default,
but it also allows us to remove a few calls to getTargetAddressSpace().
This also removes a stale comment+workaround in
CGDebugInfo::CreatePointerLikeType(): ASTContext::getTypeSize() does
return the expected size for ReferenceType (and handles address spaces).
Differential Revision: https://reviews.llvm.org/D138295
By default, clang assumes that all trailing array objects could be a
FAM. So, an array of undefined size, size 0, size 1, or even size 42 is
considered as FAMs for optimizations at least.
One needs to override the default behavior by supplying the
`-fstrict-flex-arrays=<N>` flag, with `N > 0` value to reduce the set of
FAM candidates. Value `3` is the most restrictive and `0` is the most
permissive on this scale.
0: all trailing arrays are FAMs
1: only incomplete, zero and one-element arrays are FAMs
2: only incomplete, zero-element arrays are FAMs
3: only incomplete arrays are FAMs
If the user is happy with consdering single-element arrays as FAMs, they
just need to remove the
`consider-single-element-arrays-as-flexible-array-members` from the
command line.
Otherwise, if they don't want to recognize such cases as FAMs, they
should specify `-fstrict-flex-arrays` anyway, which will be picked up by
CSA.
Any use of the deprecated analyzer-config value will trigger a warning
explaining what to use instead.
The `-analyzer-config-help` is updated accordingly.
Depends on D138657
Reviewed By: xazax.hun
Differential Revision: https://reviews.llvm.org/D138659
Casting a pointer to a suitably large integral type by reinterpret-cast
should result in the same value as by using the `__builtin_bit_cast()`.
The compiler exploits this: https://godbolt.org/z/zMP3sG683
However, the analyzer does not bind the same symbolic value to these
expressions, resulting in weird situations, such as failing equality
checks and even results in crashes: https://godbolt.org/z/oeMP7cj8q
Previously, in the `RegionStoreManager::getBinding()` even if `T` was
non-null, we replaced it with `TVR->getValueType()` in case the `MR` was
`TypedValueRegion`.
It doesn't make much sense to auto-detect the type if the type is
already given. By not doing the auto-detection, we would just do the
right thing and perform the load by that type.
This means that we will cast the value to that type.
So, in this patch, I'm proposing to do auto-detection only if the type
was null.
Here is a snippet of code, annotated by the previous and new dump values.
`LocAsInteger` should wrap the `SymRegion`, since we want to load the
address as if it was an integer.
In none of the following cases should type auto-detection be triggered,
hence we should eventually reach an `evalCast()` to lazily cast the loaded
value into that type.
```lang=C++
void LValueToRValueBitCast_dumps(void *p, char (*array)[8]) {
clang_analyzer_dump(p); // remained: &SymRegion{reg_$0<void * p>}
clang_analyzer_dump(array); // remained: {{&SymRegion{reg_$1<char (*)[8] array>}
clang_analyzer_dump((unsigned long)p);
// remained: {{&SymRegion{reg_$0<void * p>} [as 64 bit integer]}}
clang_analyzer_dump(__builtin_bit_cast(unsigned long, p)); <--------- change #1
// previously: {{&SymRegion{reg_$0<void * p>}}}
// now: {{&SymRegion{reg_$0<void * p>} [as 64 bit integer]}}
clang_analyzer_dump((unsigned long)array); // remained: {{&SymRegion{reg_$1<char (*)[8] array>} [as 64 bit integer]}}
clang_analyzer_dump(__builtin_bit_cast(unsigned long, array)); <--------- change #2
// previously: {{&SymRegion{reg_$1<char (*)[8] array>}}}
// now: {{&SymRegion{reg_$1<char (*)[8] array>} [as 64 bit integer]}}
}
```
Reviewed By: xazax.hun
Differential Revision: https://reviews.llvm.org/D136603
Refactor StaticAnalyzer to use clang::SarifDocumentWriter for
serializing sarif diagnostics.
Uses clang::SarifDocumentWriter to generate SARIF output in the
StaticAnalyzer.
Various bugfixes are also made to clang::SarifDocumentWriter.
Summary of changes:
clang/lib/Basic/Sarif.cpp:
* Fix bug in adjustColumnPos introduced from prev move, it now uses
FullSourceLoc::getDecomposedExpansionLoc which provides the correct
location (in the presence of macros) instead of
FullSourceLoc::getDecomposedLoc.
* Fix createTextRegion so that it handles caret ranges correctly,
this should bring it to parity with the previous implementation.
clang/test/Analysis/diagnostics/Inputs/expected-sarif:
* Update the schema URL to the offical website
* Add the emitted defaultConfiguration sections to all rules
* Annotate results with the "level" property
clang/lib/StaticAnalyzer/Core/SarifDiagnostics.cpp:
* Update SarifDiagnostics class to hold a clang::SarifDocumentWriter
that it uses to convert diagnostics to SARIF.
We now skip the destruction of array elements for `delete[] p`,
if the value of `p` is UnknownVal and does not have corresponding region.
This eliminate the crash in `getDynamicElementCount` on that
region and matches the behavior for deleting the array of
non-constant range.
Reviewed By: isuckatcs
Differential Revision: https://reviews.llvm.org/D136671
This revision fixes typos where there are 2 consecutive words which are
duplicated. There should be no code changes in this revision (only
changes to comments and docs). Do let me know if there are any
undesirable changes in this revision. Thanks.
This was done as a test for D137302 and it makes sense to push these changes
Reviewed By: shafik
Differential Revision: https://reviews.llvm.org/D137491
The -fstrict-flex-arrays=3 is the most restrictive type of flex arrays.
No number, including 0, is allowed in the FAM. In the cases where a "0"
is used, the resulting size is the same as if a zero-sized object were
substituted.
This is needed for proper _FORTIFY_SOURCE coverage in the Linux kernel,
among other reasons. So while the only reason for specifying a
zero-length array at the end of a structure is for specify a FAM,
treating it as such will cause _FORTIFY_SOURCE not to work correctly;
__builtin_object_size will report -1 instead of 0 for a destination
buffer size to keep any kernel internals from using the deprecated
members as fake FAMs.
For example:
struct broken {
int foo;
int fake_fam[0];
struct something oops;
};
There have been bugs where the above struct was created because "oops"
was added after "fake_fam" by someone not realizing. Under
__FORTIFY_SOURCE, doing:
memcpy(p->fake_fam, src, len);
raises no warnings when __builtin_object_size(p->fake_fam, 1) returns -1
and may stomp on "oops."
Omitting a warning when using the (invalid) zero-length array is how GCC
treats -fstrict-flex-arrays=3. A warning in that situation is likely an
irritant, because requesting this option level is explicitly requesting
this behavior.
Link: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=101836
Differential Revision: https://reviews.llvm.org/D134902
Discourse mail:
https://discourse.llvm.org/t/analyzer-why-do-we-suck-at-modeling-c-dynamic-memory/65667
malloc() returns a piece of uninitialized dynamic memory. We were (almost)
always able to model this behaviour. Its C++ counterpart, operator new is a
lot more complex, because it allows for initialization, the most complicated of which is the usage of constructors.
We gradually became better in modeling constructors, but for some reason, most
likely for reasons lost in history, we never actually modeled the case when the
memory returned by operator new was just simply uninitialized. This patch
(attempts) to fix this tiny little error.
Differential Revision: https://reviews.llvm.org/D135375
constraints
In this patch I add a new NoteTag for each applied argument constraint.
This way, any other checker that reports a bug - where the applied
constraint is relevant - will display the corresponding note. With this
change we provide more information for the users to understand some
bug reports easier.
Differential Revision: https://reviews.llvm.org/D101526
Reviewed By: NoQ
It turns out we can reach the `Init.castAs<nonlock::CompoundVal>()`
expression with other kinds of SVals. Such as by `nonloc::ConcreteInt`
in this example: https://godbolt.org/z/s4fdxrcs9
```lang=C++
int buffer[10];
void b();
void top() {
b(&buffer);
}
void b(int *c) {
*c = 42; // would crash
}
```
In this example, we try to store `42` to the `Elem{buffer, 0}`.
This situation can appear if the CallExpr refers to a function
declaration without prototype. In such cases, the engine will pick the
redecl of the referred function decl which has function body, hence has
a function prototype.
This weird situation will have an interesting effect to the AST, such as
the argument at the callsite will miss a cast, which would cast the
`int (*)[10]` expression into `int *`, which means that when we evaluate
the `*c = 42` expression, we want to bind `42` to an array, causing the
crash.
Look at the AST of the callsite with and without the function prototype:
https://godbolt.org/z/Gncebcbdb
The only difference is that without the proper function prototype, we
will not have the `ImplicitCastExpr` `BitCasting` from `int (*)[10]`
to `int *` to match the expected type of the parameter declaration.
In this patch, I'm proposing to emit a cast in the mentioned edge-case,
to bind the argument value of the expected type to the parameter.
I'm only proposing this if the runtime definition has exactly the same
number of parameters as the callsite feeds it by arguments.
If that's not the case, I believe, we are better off by binding `Unknown`
to those parameters.
Reviewed By: martong
Differential Revision: https://reviews.llvm.org/D136162
Previously, `LazyCompoundVal` bindings to subregions referred by
`LazyCopoundVals`, were not marked as //lazily copied//.
This change returns `LazyCompoundVals` from `getInterestingValues()`,
so their regions can be marked as //lazily copied// in `RemoveDeadBindingsWorker::VisitBinding()`.
Depends on D134947
Authored by: Tomasz Kamiński <tomasz.kamiński@sonarsource.com>
Reviewed By: martong
Differential Revision: https://reviews.llvm.org/D135136
To illustrate our current understanding, let's start with the following program:
https://godbolt.org/z/33f6vheh1
```lang=c++
void clang_analyzer_printState();
struct C {
int x;
int y;
int more_padding;
};
struct D {
C c;
int z;
};
C foo(D d, int new_x, int new_y) {
d.c.x = new_x; // B1
assert(d.c.x < 13); // C1
C c = d.c; // L
assert(d.c.y < 10); // C2
assert(d.z < 5); // C3
d.c.y = new_y; // B2
assert(d.c.y < 10); // C4
return c; // R
}
```
In the code, we create a few bindings to subregions of root region `d` (`B1`, `B2`), a constrain on the values (`C1`, `C2`, ….), and create a `lazyCompoundVal` for the part of the region `d` at point `L`, which is returned at point `R`.
Now, the question is which of these should remain live as long the return value of the `foo` call is live. In perfect a word we should preserve:
# only the bindings of the subregions of `d.c`, which were created before the copy at `L`. In our example, this includes `B1`, and not `B2`. In other words, `new_x` should be live but `new_y` shouldn’t.
# constraints on the values of `d.c`, that are reachable through `c`. This can be created both before the point of making the copy (`L`) or after. In our case, that would be `C1` and `C2`. But not `C3` (`d.z` value is not reachable through `c`) and `C4` (the original value of`d.c.y` was overridden at `B2` after the creation of `c`).
The current code in the `RegionStore` covers the use case (1), by using the `getInterestingValues()` to extract bindings to parts of the referred region present in the store at the point of copy. This also partially covers point (2), in case when constraints are applied to a location that has binding at the point of the copy (in our case `d.c.x` in `C1` that has value `new_x`), but it fails to preserve the constraints that require creating a new symbol for location (`d.c.y` in `C2`).
We introduce the concept of //lazily copied// locations (regions) to the `SymbolReaper`, i.e. for which a program can access the value stored at that location, but not its address. These locations are constructed as a set of regions referred to by `lazyCompoundVal`. A //readable// location (region) is a location that //live// or //lazily copied// . And symbols that refer to values in regions are alive if the region is //readable//.
For simplicity, we follow the current approach to live regions and mark the base region as //lazily copied//, and consider any subregions as //readable//. This makes some symbols falsy live (`d.z` in our example) and keeps the corresponding constraints alive.
The rename `Regions` to `LiveRegions` inside `RegionStore` is NFC change, that was done to make it clear, what is difference between regions stored in this two sets.
Regression Test: https://reviews.llvm.org/D134941
Co-authored-by: Balazs Benics <benicsbalazs@gmail.com>
Reviewed By: martong, xazax.hun
Differential Revision: https://reviews.llvm.org/D134947
This is a change to how we represent type subsitution in the AST.
Instead of only storing the replaced type, we track the templated
entity we are substituting, plus an index.
We modify MLTAL to track the templated entity at each level.
Otherwise, it's much more expensive to go from the template parameter back
to the templated entity, and not possible to do in some cases, as when
we instantiate outer templates, parameters might still reference the
original entity.
This also allows us to very cheaply lookup the templated entity we saw in
the naming context and find the corresponding argument it was replaced
from, such as for implementing template specialization resugaring.
Signed-off-by: Matheus Izvekov <mizvekov@gmail.com>
Differential Revision: https://reviews.llvm.org/D131858
The Clang Static Analyzer will crash on this code:
```lang=C++
struct Box {
int value;
};
template <Box V> int get() {
return V.value;
}
template int get<Box{-1}>();
```
https://godbolt.org/z/5Yb1sMMMb
The problem is that we don't account for encountering `TemplateParamObjectDecl`s
within the `DeclRefExpr` handler in the `ExprEngine`.
IMO we should create a new memregion for representing such template
param objects, to model their language semantics.
Such as:
- it should have global static storage
- for two identical values, their addresses should be identical as well
http://eel.is/c%2B%2Bdraft/temp.param#8
I was thinking of introducing a `TemplateParamObjectRegion` under `DeclRegion`
for this purpose. It could have `TemplateParamObjectDecl` as a field.
The `TemplateParamObjectDecl::getValue()` returns `APValue`, which might
represent multiple levels of structures, unions and other goodies -
making the transformation from `APValue` to `SVal` a bit complicated.
That being said, for now, I think having `Unknowns` for such cases is
definitely an improvement to crashing, hence I'm proposing this patch.
Reviewed By: xazax.hun
Differential Revision: https://reviews.llvm.org/D135763
Before this change, the `NoReturnFunctionChecker` was missing function pointers
with a `[[noreturn]]` attribute, while `CFG` was constructed taking that into
account, which leads CSA to take impossible paths. The reason was that the
`NoReturnFunctionChecker` was looking for the attribute in the type of the
entire call expression rather than the type of the function being called.
This change makes the `[[noreturn]]` attribute of a function pointer visible
to `NoReturnFunctionChecker`. This leads to a more coherent behavior of the
CSA on the AST involving.
Reviewed By: xazax.hun
Differential Revision: https://reviews.llvm.org/D135682
ProcessMemberDtor(), ProcessDeleteDtor(), and ProcessAutomaticObjDtor():
Fix static analyzer warnings with suspicious dereference of pointer
'Pred' in function call before NULL checks - NFCI
Differential Revision: https://reviews.llvm.org/D135290
In the context of caching clang invocations it is important to emit diagnostics in deterministic order;
the same clang invocation should result in the same diagnostic output.
rdar://100336989
Differential Revision: https://reviews.llvm.org/D135118
In case when the prvalue is returned from the function (kind is one
of `SimpleReturnedValueKind`, `CXX17ElidedCopyReturnedValueKind`),
then it construction happens in context of the caller.
We pass `BldrCtx` explicitly, as `currBldrCtx` will always refer to callee
context.
In the following example:
```
struct Result {int value; };
Result create() { return Result{10}; }
int accessValue(Result r) { return r.value; }
void test() {
for (int i = 0; i < 2; ++i)
accessValue(create());
}
```
In case when the returned object was constructed directly into the
argument to a function call `accessValue(create())`, this led to
inappropriate value of `blockCount` being used to locate parameter region,
and as a consequence resulting object (from `create()`) was constructed
into a different region, that was later read by inlined invocation of
outer function (`accessValue`).
This manifests itself only in case when calling block is visited more
than once (loop in above example), as otherwise there is no difference
in `blockCount` value between callee and caller context.
This happens only in case when copy elision is disabled (before C++17).
Reviewed By: NoQ
Differential Revision: https://reviews.llvm.org/D132030
showBRParamDiagnostics assumed stores happen only via function parameters while that
can also happen via implicit parameters like 'self' or 'this'.
The regression test caused a failed assert in the original cast to ParmVarDecl.
Differential Revision: https://reviews.llvm.org/D133815
Most of the state traits used for non-POD array evaluation were
only cleaned up if the ctors/dtors were inlined, since the cleanup
happened in ExprEngine::processCallExit(). This patch makes sure
they are removed even if said functions are not inlined.
Differential Revision: https://reviews.llvm.org/D133643
By this change the `exploded-graph-rewriter` will display the class kind
of the expression of the environment entry. It makes easier to decide if
the given entry corresponds to the lvalue or to the rvalue of some
expression.
It turns out the rewriter already had support for visualizing it, but
probably was never actually used?
Reviewed By: martong
Differential Revision: https://reviews.llvm.org/D132109
`LazyCompoundVals` should only appear as `default` bindings in the
store. This fixes the second case in this patch-stack.
Depends on: D132142
Reviewed By: xazax.hun
Differential Revision: https://reviews.llvm.org/D132143
It turns out that in certain cases `SymbolRegions` are wrapped by
`ElementRegions`; in others, it's not. This discrepancy can cause the
analyzer not to recognize if the two regions are actually referring to
the same entity, which then can lead to unreachable paths discovered.
Consider this example:
```lang=C++
struct Node { int* ptr; };
void with_structs(Node* n1) {
Node c = *n1; // copy
Node* n2 = &c;
clang_analyzer_dump(*n1); // lazy...
clang_analyzer_dump(*n2); // lazy...
clang_analyzer_dump(n1->ptr); // rval(n1->ptr): reg_$2<int * SymRegion{reg_$0<struct Node * n1>}.ptr>
clang_analyzer_dump(n2->ptr); // rval(n2->ptr): reg_$1<int * Element{SymRegion{reg_$0<struct Node * n1>},0 S64b,struct Node}.ptr>
clang_analyzer_eval(n1->ptr != n2->ptr); // UNKNOWN, bad!
(void)(*n1);
(void)(*n2);
}
```
The copy of `n1` will insert a new binding to the store; but for doing
that it actually must create a `TypedValueRegion` which it could pass to
the `LazyCompoundVal`. Since the memregion in question is a
`SymbolicRegion` - which is untyped, it needs to first wrap it into an
`ElementRegion` basically implementing this untyped -> typed conversion
for the sake of passing it to the `LazyCompoundVal`.
So, this is why we have `Element{SymRegion{.}, 0,struct Node}` for `n1`.
The problem appears if the analyzer evaluates a read from the expression
`n1->ptr`. The same logic won't apply for `SymbolRegionValues`, since
they accept raw `SubRegions`, hence the `SymbolicRegion` won't be
wrapped into an `ElementRegion` in that case.
Later when we arrive at the equality comparison, we cannot prove that
they are equal.
For more details check the corresponding thread on discourse:
https://discourse.llvm.org/t/are-symbolicregions-really-untyped/64406
---
In this patch, I'm eagerly wrapping each `SymbolicRegion` by an
`ElementRegion`; basically canonicalizing to this form.
It seems reasonable to do so since any object can be thought of as a single
array of that object; so this should not make much of a difference.
The tests also underpin this assumption, as only a few were broken by
this change; and actually fixed a FIXME along the way.
About the second example, which does the same copy operation - but on
the heap - it will be fixed by the next patch.
Reviewed By: martong
Differential Revision: https://reviews.llvm.org/D132142
If an object has a trivial copy/move constructor, it's not inlined
on invocation but a trivial copy is performed instead. This patch
handles trivial copies in the bug reporter by matching the field
regions of the 2 objects involved in the copy/move construction,
and tracking the appropriate region further. This patch also
introduces some support for tracking values in initializer lists.
Differential Revision: https://reviews.llvm.org/D131262
This patch introduces a new checker, called NewArraySize checker,
which detects if the expression that yields the element count of
the array in new[], results in an Undefined value.
Differential Revision: https://reviews.llvm.org/D131299
This patch dumps every state trait in the egraph. Also
the empty state traits are no longer dumped, instead
they are treated as null by the egraph rewriter script,
which solves reverse compatibility issues.
Differential Revision: https://reviews.llvm.org/D131187
Some of the code used in StdLibraryFunctionsChecker is applicable to
other checkers, this is put into common functions. Errno related
parts of the checker are simplified and renamed. Documentations in
errno_modeling functions are updated.
This change makes it available to have more checkers that perform
modeling of some standard functions. These can set the errno state
with common functions and the bug report messages (note tags) can
look similar.
Reviewed By: steakhal, martong
Differential Revision: https://reviews.llvm.org/D131879
leaking in ARC mode
When ARC (automatic reference count) is enabled, (objective-c) block
objects are automatically retained and released thus they do not leak.
Without ARC, they still can leak from an expiring stack frame like
other stack variables.
With this commit, the static analyzer now puts a block object in an
"unknown" region if ARC is enabled because it is up to the
implementation to choose whether to put the object on stack initially
(then move to heap when needed) or in heap directly under ARC.
Therefore, the `StackAddrEscapeChecker` has no need to know
specifically about ARC at all and it will not report errors on objects
in "unknown" regions.
Reviewed By: NoQ (Artem Dergachev)
Differential Revision: https://reviews.llvm.org/D131009
Prior to this patch when the analyzer encountered a non-POD 0 length array,
it still invoked the constructor for 1 element, which lead to false positives.
This patch makes sure that we no longer construct any elements when we see a
0 length array.
Differential Revision: https://reviews.llvm.org/D131501
The constructors of non-POD array elements are evaluated under
certain conditions. This patch makes sure that in such cases
we also evaluate the destructors.
Differential Revision: https://reviews.llvm.org/D130737
Dead store detection automatically checks that an expression is a
CXXConstructor and skips it because of potential side effects. In C++17,
with guaranteed copy elision, this check can fail because we actually
receive the implicit cast of a CXXConstructor.
Most checks in the dead store analysis were already stripping all casts
and parenthesis and those that weren't were either forgotten (like the
constructor) or would not suffer from it, so this patch proposes to
factorize the stripping.
It has an impact on where the dead store warning is reported in the case
of an explicit cast, from
auto a = static_cast<B>(A());
^~~~~~~~~~~~~~~~~~~
to
auto a = static_cast<B>(A());
^~~
which we think is an improvement.
Patch By: frederic-tingaud-sonarsource
Reviewed By: steakhal
Differential Revision: https://reviews.llvm.org/D126534
This patch makes it possible for lambdas, implicit copy/move ctors
and structured bindings to handle non-POD multidimensional arrays.
Differential Revision: https://reviews.llvm.org/D131840
Prior to this patch we handled lambda captures based on their
initializer expression, which resulted in pattern matching. With
C++17 copy elision the initializer expression can be anything,
and this approach proved to be fragile and a source of crashes.
This patch removes pattern matching and only checks whether the
object is under construction or not.
Differential Revision: https://reviews.llvm.org/D131944
This patch adds a ProgramPointTag to the EpsilonPoint created
before we replay a call without inlining.
Differential Revision: https://reviews.llvm.org/D132246
Inside `ExprEngine::VisitLambdaExpr()` we wasn't prepared for a
copy elided initialized capture's `InitExpr`. This patch teaches
the analyzer how to handle such situation.
Differential Revision: https://reviews.llvm.org/D131784
Support for functions wmempcpy, wmemmove, wmemcmp is added to the checker.
The same tests are copied that exist for the non-wide versions, with
non-wide functions and character types changed to the wide version.
Reviewed By: martong
Differential Revision: https://reviews.llvm.org/D130470
This completes the implementation of P1091R3 and P1381R1.
This patch allow the capture of structured bindings
both for C++20+ and C++17, with extension/compat warning.
In addition, capturing an anonymous union member,
a bitfield, or a structured binding thereof now has a
better diagnostic.
We only support structured bindings - as opposed to other kinds
of structured statements/blocks. We still emit an error for those.
In addition, support for structured bindings capture is entirely disabled in
OpenMP mode as this needs more investigation - a specific diagnostic indicate the feature is not yet supported there.
Note that the rest of P1091R3 (static/thread_local structured bindings) was already implemented.
at the request of @shafik, i can confirm the correct behavior of lldb wit this change.
Fixes https://github.com/llvm/llvm-project/issues/54300
Fixes https://github.com/llvm/llvm-project/issues/54300
Fixes https://github.com/llvm/llvm-project/issues/52720
Reviewed By: aaron.ballman
Differential Revision: https://reviews.llvm.org/D122768
This completes the implementation of P1091R3 and P1381R1.
This patch allow the capture of structured bindings
both for C++20+ and C++17, with extension/compat warning.
In addition, capturing an anonymous union member,
a bitfield, or a structured binding thereof now has a
better diagnostic.
We only support structured bindings - as opposed to other kinds
of structured statements/blocks. We still emit an error for those.
In addition, support for structured bindings capture is entirely disabled in
OpenMP mode as this needs more investigation - a specific diagnostic indicate the feature is not yet supported there.
Note that the rest of P1091R3 (static/thread_local structured bindings) was already implemented.
at the request of @shafik, i can confirm the correct behavior of lldb wit this change.
Fixes https://github.com/llvm/llvm-project/issues/54300
Fixes https://github.com/llvm/llvm-project/issues/54300
Fixes https://github.com/llvm/llvm-project/issues/52720
Reviewed By: aaron.ballman
Differential Revision: https://reviews.llvm.org/D122768
In ExprEngine::bindReturnValue() we cast an SVal to DefinedOrUnknownSVal,
however this SVal can also be Undefined, which leads to an assertion failure.
Fixes: #56873
Differential Revision: https://reviews.llvm.org/D130974
I went over the output of the following mess of a command:
(ulimit -m 2000000; ulimit -v 2000000; git ls-files -z |
parallel --xargs -0 cat | aspell list --mode=none --ignore-case |
grep -E '^[A-Za-z][a-z]*$' | sort | uniq -c | sort -n |
grep -vE '.{25}' | aspell pipe -W3 | grep : | cut -d' ' -f2 | less)
and proceeded to spend a few days looking at it to find probable typos
and fixed a few hundred of them in all of the llvm project (note, the
ones I found are not anywhere near all of them, but it seems like a
good start).
Differential Revision: https://reviews.llvm.org/D130827
Without this patch, clang will not wrap in an ElaboratedType node types written
without a keyword and nested name qualifier, which goes against the intent that
we should produce an AST which retains enough details to recover how things are
written.
The lack of this sugar is incompatible with the intent of the type printer
default policy, which is to print types as written, but to fall back and print
them fully qualified when they are desugared.
An ElaboratedTypeLoc without keyword / NNS uses no storage by itself, but still
requires pointer alignment due to pre-existing bug in the TypeLoc buffer
handling.
---
Troubleshooting list to deal with any breakage seen with this patch:
1) The most likely effect one would see by this patch is a change in how
a type is printed. The type printer will, by design and default,
print types as written. There are customization options there, but
not that many, and they mainly apply to how to print a type that we
somehow failed to track how it was written. This patch fixes a
problem where we failed to distinguish between a type
that was written without any elaborated-type qualifiers,
such as a 'struct'/'class' tags and name spacifiers such as 'std::',
and one that has been stripped of any 'metadata' that identifies such,
the so called canonical types.
Example:
```
namespace foo {
struct A {};
A a;
};
```
If one were to print the type of `foo::a`, prior to this patch, this
would result in `foo::A`. This is how the type printer would have,
by default, printed the canonical type of A as well.
As soon as you add any name qualifiers to A, the type printer would
suddenly start accurately printing the type as written. This patch
will make it print it accurately even when written without
qualifiers, so we will just print `A` for the initial example, as
the user did not really write that `foo::` namespace qualifier.
2) This patch could expose a bug in some AST matcher. Matching types
is harder to get right when there is sugar involved. For example,
if you want to match a type against being a pointer to some type A,
then you have to account for getting a type that is sugar for a
pointer to A, or being a pointer to sugar to A, or both! Usually
you would get the second part wrong, and this would work for a
very simple test where you don't use any name qualifiers, but
you would discover is broken when you do. The usual fix is to
either use the matcher which strips sugar, which is annoying
to use as for example if you match an N level pointer, you have
to put N+1 such matchers in there, beginning to end and between
all those levels. But in a lot of cases, if the property you want
to match is present in the canonical type, it's easier and faster
to just match on that... This goes with what is said in 1), if
you want to match against the name of a type, and you want
the name string to be something stable, perhaps matching on
the name of the canonical type is the better choice.
3) This patch could expose a bug in how you get the source range of some
TypeLoc. For some reason, a lot of code is using getLocalSourceRange(),
which only looks at the given TypeLoc node. This patch introduces a new,
and more common TypeLoc node which contains no source locations on itself.
This is not an inovation here, and some other, more rare TypeLoc nodes could
also have this property, but if you use getLocalSourceRange on them, it's not
going to return any valid locations, because it doesn't have any. The right fix
here is to always use getSourceRange() or getBeginLoc/getEndLoc which will dive
into the inner TypeLoc to get the source range if it doesn't find it on the
top level one. You can use getLocalSourceRange if you are really into
micro-optimizations and you have some outside knowledge that the TypeLocs you are
dealing with will always include some source location.
4) Exposed a bug somewhere in the use of the normal clang type class API, where you
have some type, you want to see if that type is some particular kind, you try a
`dyn_cast` such as `dyn_cast<TypedefType>` and that fails because now you have an
ElaboratedType which has a TypeDefType inside of it, which is what you wanted to match.
Again, like 2), this would usually have been tested poorly with some simple tests with
no qualifications, and would have been broken had there been any other kind of type sugar,
be it an ElaboratedType or a TemplateSpecializationType or a SubstTemplateParmType.
The usual fix here is to use `getAs` instead of `dyn_cast`, which will look deeper
into the type. Or use `getAsAdjusted` when dealing with TypeLocs.
For some reason the API is inconsistent there and on TypeLocs getAs behaves like a dyn_cast.
5) It could be a bug in this patch perhaps.
Let me know if you need any help!
Signed-off-by: Matheus Izvekov <mizvekov@gmail.com>
Differential Revision: https://reviews.llvm.org/D112374
This patch introduces a new `ConstructionContext` for
lambda capture. This `ConstructionContext` allows the
analyzer to construct the captured object directly into
it's final region, and makes it possible to capture
non-POD arrays.
Differential Revision: https://reviews.llvm.org/D129967
This patch introduces the evaluation of ArrayInitLoopExpr
in case of structured bindings and implicit copy/move
constructor. The idea is to call the copy constructor for
every element in the array. The parameter of the copy
constructor is also manually selected, as it is not a part
of the CFG.
Differential Revision: https://reviews.llvm.org/D129496
Support for functions wmemcpy, wcslen, wcsnlen is added to the checker.
Documentation and tests are updated and extended with the new functions.
Reviewed By: martong
Differential Revision: https://reviews.llvm.org/D130091
Summary: Get rid of explicit function splitting in favor of specifically designed Visitor. Move logic from a family of `evalCastKind` and `evalCastSubKind` helper functions to `SValVisitor`.
Differential Revision: https://reviews.llvm.org/D130029
Some code [0] consider that trailing arrays are flexible, whatever their size.
Support for these legacy code has been introduced in
f8f6324983 but it prevents evaluation of
__builtin_object_size and __builtin_dynamic_object_size in some legit cases.
Introduce -fstrict-flex-arrays=<n> to have stricter conformance when it is
desirable.
n = 0: current behavior, any trailing array member is a flexible array. The default.
n = 1: any trailing array member of undefined, 0 or 1 size is a flexible array member
n = 2: any trailing array member of undefined or 0 size is a flexible array member
This takes into account two specificities of clang: array bounds as macro id
disqualify FAM, as well as non standard layout.
Similar patch for gcc discuss here: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=101836
[0] https://docs.freebsd.org/en/books/developers-handbook/sockets/#sockets-essential-functions
Summary: Introduce a new function 'clang_analyzer_value'. It emits a report that in turn prints a RangeSet or APSInt associated with SVal. If there is no associated value, prints "n/a".
Summary: Sorted some handler-functions into more appropriate visitor functions of the SymbolicRangeInferrer.
- Spread `getRangeForNegatedSub` body over several visitor functions: `VisitSymExpr`, `VisitSymIntExpr`, `VisitSymSymExpr`.
- Moved `getRangeForComparisonSymbol` from `infer` to `VisitSymSymExpr`.
Differential Revision: https://reviews.llvm.org/D129678
This reverts commit 7c51f02eff because it
stills breaks the LLDB tests. This was re-landed without addressing the
issue or even agreement on how to address the issue. More details and
discussion in https://reviews.llvm.org/D112374.
Without this patch, clang will not wrap in an ElaboratedType node types written
without a keyword and nested name qualifier, which goes against the intent that
we should produce an AST which retains enough details to recover how things are
written.
The lack of this sugar is incompatible with the intent of the type printer
default policy, which is to print types as written, but to fall back and print
them fully qualified when they are desugared.
An ElaboratedTypeLoc without keyword / NNS uses no storage by itself, but still
requires pointer alignment due to pre-existing bug in the TypeLoc buffer
handling.
---
Troubleshooting list to deal with any breakage seen with this patch:
1) The most likely effect one would see by this patch is a change in how
a type is printed. The type printer will, by design and default,
print types as written. There are customization options there, but
not that many, and they mainly apply to how to print a type that we
somehow failed to track how it was written. This patch fixes a
problem where we failed to distinguish between a type
that was written without any elaborated-type qualifiers,
such as a 'struct'/'class' tags and name spacifiers such as 'std::',
and one that has been stripped of any 'metadata' that identifies such,
the so called canonical types.
Example:
```
namespace foo {
struct A {};
A a;
};
```
If one were to print the type of `foo::a`, prior to this patch, this
would result in `foo::A`. This is how the type printer would have,
by default, printed the canonical type of A as well.
As soon as you add any name qualifiers to A, the type printer would
suddenly start accurately printing the type as written. This patch
will make it print it accurately even when written without
qualifiers, so we will just print `A` for the initial example, as
the user did not really write that `foo::` namespace qualifier.
2) This patch could expose a bug in some AST matcher. Matching types
is harder to get right when there is sugar involved. For example,
if you want to match a type against being a pointer to some type A,
then you have to account for getting a type that is sugar for a
pointer to A, or being a pointer to sugar to A, or both! Usually
you would get the second part wrong, and this would work for a
very simple test where you don't use any name qualifiers, but
you would discover is broken when you do. The usual fix is to
either use the matcher which strips sugar, which is annoying
to use as for example if you match an N level pointer, you have
to put N+1 such matchers in there, beginning to end and between
all those levels. But in a lot of cases, if the property you want
to match is present in the canonical type, it's easier and faster
to just match on that... This goes with what is said in 1), if
you want to match against the name of a type, and you want
the name string to be something stable, perhaps matching on
the name of the canonical type is the better choice.
3) This patch could exposed a bug in how you get the source range of some
TypeLoc. For some reason, a lot of code is using getLocalSourceRange(),
which only looks at the given TypeLoc node. This patch introduces a new,
and more common TypeLoc node which contains no source locations on itself.
This is not an inovation here, and some other, more rare TypeLoc nodes could
also have this property, but if you use getLocalSourceRange on them, it's not
going to return any valid locations, because it doesn't have any. The right fix
here is to always use getSourceRange() or getBeginLoc/getEndLoc which will dive
into the inner TypeLoc to get the source range if it doesn't find it on the
top level one. You can use getLocalSourceRange if you are really into
micro-optimizations and you have some outside knowledge that the TypeLocs you are
dealing with will always include some source location.
4) Exposed a bug somewhere in the use of the normal clang type class API, where you
have some type, you want to see if that type is some particular kind, you try a
`dyn_cast` such as `dyn_cast<TypedefType>` and that fails because now you have an
ElaboratedType which has a TypeDefType inside of it, which is what you wanted to match.
Again, like 2), this would usually have been tested poorly with some simple tests with
no qualifications, and would have been broken had there been any other kind of type sugar,
be it an ElaboratedType or a TemplateSpecializationType or a SubstTemplateParmType.
The usual fix here is to use `getAs` instead of `dyn_cast`, which will look deeper
into the type. Or use `getAsAdjusted` when dealing with TypeLocs.
For some reason the API is inconsistent there and on TypeLocs getAs behaves like a dyn_cast.
5) It could be a bug in this patch perhaps.
Let me know if you need any help!
Signed-off-by: Matheus Izvekov <mizvekov@gmail.com>
Differential Revision: https://reviews.llvm.org/D112374
Introducing the support for evaluating the constructor
of every element in an array. The idea is to record the
index of the current array member being constructed and
create a loop during the analysis. We looping over the
same CXXConstructExpr as many times as many elements
the array has.
Differential Revision: https://reviews.llvm.org/D127973
In method `TypeRetrievingVisitor::VisitConcreteInt`, `ASTContext::getIntTypeForBitwidth` is used to get the type for `ConcreteInt`s.
However, the getter in ASTContext cannot handle the boolean type with the bit width of 1, which will make method `SVal::getType` return a Null `Type`.
In this patch, a check for this case is added to fix this problem by returning the bool type directly when the bit width is 1.
Differential Revision: https://reviews.llvm.org/D129737
CStringChecker is using getByteLength to get the length of a string
literal. For targets where a "char" is 8-bits, getByteLength() and
getLength() will be equal for a C string, but for targets where a "char"
is 16-bits getByteLength() returns the size in octets.
This is verified in our downstream target, but we have no way to add a
test case for this case since there is no target supporting 16-bit
"char" upstream. Since this cannot have a test case, I'm asserted this
change is "correct by construction", and visually inspected to be
correct by way of the following example where this was found.
The case that shows this fails using a target with 16-bit chars is here.
getByteLength() for the string literal returns 4, which fails when
checked against "char x[4]". With the change, the string literal is
evaluated to a size of 2 which is a correct number of "char"'s for a
16-bit target.
```
void strcpy_no_overflow_2(char *y) {
char x[4];
strcpy(x, "12"); // with getByteLength(), returns 4 using 16-bit chars
}
```
This change exposed that embedded nulls within the string are not
handled. This is documented as a FIXME for a future fix.
```
void strcpy_no_overflow_3(char *y) {
char x[3];
strcpy(x, "12\0");
}
```
Reviewed By: martong
Differential Revision: https://reviews.llvm.org/D129269
This reverts commit bdc6974f92 because it
breaks all the LLDB tests that import the std module.
import-std-module/array.TestArrayFromStdModule.py
import-std-module/deque-basic.TestDequeFromStdModule.py
import-std-module/deque-dbg-info-content.TestDbgInfoContentDequeFromStdModule.py
import-std-module/forward_list.TestForwardListFromStdModule.py
import-std-module/forward_list-dbg-info-content.TestDbgInfoContentForwardListFromStdModule.py
import-std-module/list.TestListFromStdModule.py
import-std-module/list-dbg-info-content.TestDbgInfoContentListFromStdModule.py
import-std-module/queue.TestQueueFromStdModule.py
import-std-module/stack.TestStackFromStdModule.py
import-std-module/vector.TestVectorFromStdModule.py
import-std-module/vector-bool.TestVectorBoolFromStdModule.py
import-std-module/vector-dbg-info-content.TestDbgInfoContentVectorFromStdModule.py
import-std-module/vector-of-vectors.TestVectorOfVectorsFromStdModule.py
https://green.lab.llvm.org/green/view/LLDB/job/lldb-cmake/45301/
Without this patch, clang will not wrap in an ElaboratedType node types written
without a keyword and nested name qualifier, which goes against the intent that
we should produce an AST which retains enough details to recover how things are
written.
The lack of this sugar is incompatible with the intent of the type printer
default policy, which is to print types as written, but to fall back and print
them fully qualified when they are desugared.
An ElaboratedTypeLoc without keyword / NNS uses no storage by itself, but still
requires pointer alignment due to pre-existing bug in the TypeLoc buffer
handling.
Signed-off-by: Matheus Izvekov <mizvekov@gmail.com>
Differential Revision: https://reviews.llvm.org/D112374
Depends on D128068.
Added a new test code that fails an assertion in the baseline.
That is because `getAPSIntType` works only with integral types.
Differential Revision: https://reviews.llvm.org/D126779
In `RegionStore::getBinding` we call `evalCast` unconditionally to align
the stored value's type to the one that is being queried. However, the
stored type might be the same, so we may end up having redundant
`SymbolCasts` emitted.
The solution is to check whether the `to` and `from` type are the same
in `makeNonLoc`.
Note, we can't just do type equivalence check at the beginning of `evalCast`
because when `evalCast` is called from `getBinding` then the original type
(`OriginalTy`) is not set, so one operand is missing for the comparison. In
`evalCastSubKind(nonloc::SymbolVal)` when the original type is not set,
we get the `from` type via `SymbolVal::getType()`.
Differential Revision: https://reviews.llvm.org/D128068
This patch gives basic parsing and semantic support for
"parallel masked taskloop simd" construct introduced in
OpenMP 5.1 (section 2.16.10)
Differential Revision: https://reviews.llvm.org/D128946
This patch gives basic parsing and semantic support for
"parallel masked taskloop" construct introduced in
OpenMP 5.1 (section 2.16.9)
Differential Revision: https://reviews.llvm.org/D128834
Instead of dumping the string literal (which
quotes it and escape every non-ascii symbol),
we can use the content of the string when it is a
8 byte string.
Wide, UTF-8/UTF-16/32 strings are still completely
escaped, until we clarify how these entities should
behave (cf https://wg21.link/p2361).
`FormatDiagnostic` is modified to escape
non printable characters and invalid UTF-8.
This ensures that unicode characters, spaces and new
lines are properly rendered in static messages.
This make clang more consistent with other implementation
and fixes this tweet
https://twitter.com/jfbastien/status/1298307325443231744 :)
Of note, `PaddingChecker` did print out new lines that were
later removed by the diagnostic printing code.
To be consistent with its tests, the new lines are removed
from the diagnostic.
Unicode tables updated to both use the Unicode definitions
and the Unicode 14.0 data.
U+00AD SOFT HYPHEN is still considered a print character
to match existing practices in terminals, in addition of
being considered a formatting character as per Unicode.
Reviewed By: aaron.ballman, #clang-language-wg
Differential Revision: https://reviews.llvm.org/D108469
The case when the bound variable is reference type in a
BindingDecl wasn't handled, which lead to false positives.
Differential Revision: https://reviews.llvm.org/D128716
This patch gives basic parsing and semantic support for
"masked taskloop simd" construct introduced in OpenMP 5.1 (section 2.16.8)
Differential Revision: https://reviews.llvm.org/D128693
Instead of dumping the string literal (which
quotes it and escape every non-ascii symbol),
we can use the content of the string when it is a
8 byte string.
Wide, UTF-8/UTF-16/32 strings are still completely
escaped, until we clarify how these entities should
behave (cf https://wg21.link/p2361).
`FormatDiagnostic` is modified to escape
non printable characters and invalid UTF-8.
This ensures that unicode characters, spaces and new
lines are properly rendered in static messages.
This make clang more consistent with other implementation
and fixes this tweet
https://twitter.com/jfbastien/status/1298307325443231744 :)
Of note, `PaddingChecker` did print out new lines that were
later removed by the diagnostic printing code.
To be consistent with its tests, the new lines are removed
from the diagnostic.
Unicode tables updated to both use the Unicode definitions
and the Unicode 14.0 data.
U+00AD SOFT HYPHEN is still considered a print character
to match existing practices in terminals, in addition of
being considered a formatting character as per Unicode.
Reviewed By: aaron.ballman, #clang-language-wg
Differential Revision: https://reviews.llvm.org/D108469
This patch gives basic parsing and semantic support for "masked taskloop"
construct introduced in OpenMP 5.1 (section 2.16.7)
Differential Revision: https://reviews.llvm.org/D128478
Some code [0] consider that trailing arrays are flexible, whatever their size.
Support for these legacy code has been introduced in
f8f6324983 but it prevents evaluation of
__builtin_object_size and __builtin_dynamic_object_size in some legit cases.
Introduce -fstrict-flex-arrays=<n> to have stricter conformance when it is
desirable.
n = 0: current behavior, any trailing array member is a flexible array. The default.
n = 1: any trailing array member of undefined, 0 or 1 size is a flexible array member
n = 2: any trailing array member of undefined or 0 size is a flexible array member
n = 3: any trailing array member of undefined size is a flexible array member (strict c99 conformance)
Similar patch for gcc discuss here: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=101836
[0] https://docs.freebsd.org/en/books/developers-handbook/sockets/#sockets-essential-functions
Introducing structured binding to data members and more.
To handle binding to arrays, ArrayInitLoopExpr is also
evaluated, which enables the analyzer to store information
in two more cases. These are:
- when a lambda-expression captures an array by value
- in the implicit copy/move constructor for a class
with an array member
Differential Revision: https://reviews.llvm.org/D126613
The functions 'mkdir', 'mknod', 'mkdirat', 'mknodat' return 0 on success
and -1 on failure. The checker modeled these functions with a >= 0
return value on success which is changed to 0 only. This fix makes
ErrnoChecker work better for these functions.
Reviewed By: steakhal
Differential Revision: https://reviews.llvm.org/D127277
This updates StdLibraryFunctionsChecker to set the state of 'errno'
by using the new errno_modeling functionality.
The errno value is set in the PostCall callback. Setting it in call::Eval
did not work for some reason and then every function should be
EvalCallAsPure which may be bad to do. Now the errno value and state
is not allowed to be checked in any PostCall checker callback because
it is unspecified if the errno was set already or will be set later
by this checker.
Reviewed By: martong, steakhal
Differential Revision: https://reviews.llvm.org/D125400
Extend checker 'ErrnoModeling' with a state of 'errno' to indicate
the importance of the 'errno' value and how it should be used.
Add a new checker 'ErrnoChecker' that observes use of 'errno' and
finds possible wrong uses, based on the "errno state".
The "errno state" should be set (together with value of 'errno')
by other checkers (that perform modeling of the given function)
in the future. Currently only a test function can set this value.
The new checker has no user-observable effect yet.
Reviewed By: martong, steakhal
Differential Revision: https://reviews.llvm.org/D122150
If a lazyCompoundVal to a struct is bound to the store, there is a policy which decides
whether a copy gets created instead.
This patch introduces a similar policy for arrays, which is required to model structured
binding to arrays without false negatives.
Differential Revision: https://reviews.llvm.org/D128064
The arithmetic restriction seems to be artificial.
The comment below seems to be stale.
Thus, we remove both.
Depends on D127306.
Reviewed By: martong
Differential Revision: https://reviews.llvm.org/D127763
Previously, system globals were treated as immutable regions, unless it
was the `errno` which is known to be frequently modified.
D124244 wants to add a check for stores to immutable regions.
It would basically turn all stores to system globals into an error even
though we have no reason to believe that those mutable sys globals
should be treated as if they were immutable. And this leads to
false-positives if we apply D124244.
In this patch, I'm proposing to treat mutable sys globals actually
mutable, hence allocate them into the `GlobalSystemSpaceRegion`, UNLESS
they were declared as `const` (and a primitive arithmetic type), in
which case, we should use `GlobalImmutableSpaceRegion`.
In any other cases, I'm using the `GlobalInternalSpaceRegion`, which is
no different than the previous behavior.
---
In the tests I added, only the last `expected-warning` was different, compared to the baseline.
Which is this:
```lang=C++
void test_my_mutable_system_global_constraint() {
assert(my_mutable_system_global > 2);
clang_analyzer_eval(my_mutable_system_global > 2); // expected-warning {{TRUE}}
invalidate_globals();
clang_analyzer_eval(my_mutable_system_global > 2); // expected-warning {{UNKNOWN}} It was previously TRUE.
}
void test_my_mutable_system_global_assign(int x) {
my_mutable_system_global = x;
clang_analyzer_eval(my_mutable_system_global == x); // expected-warning {{TRUE}}
invalidate_globals();
clang_analyzer_eval(my_mutable_system_global == x); // expected-warning {{UNKNOWN}} It was previously TRUE.
}
```
---
Unfortunately, the taint checker will be also affected.
The `stdin` global variable is a pointer, which is assumed to be a taint
source, and the rest of the taint propagation rules will propagate from
it.
However, since mutable variables are no longer treated immutable, they
also get invalidated, when an opaque function call happens, such as the
first `scanf(stdin, ...)`. This would effectively remove taint from the
pointer, consequently disable all the rest of the taint propagations
down the line from the `stdin` variable.
All that said, I decided to look through `DerivedSymbol`s as well, to
acquire the memregion in that case as well. This should preserve the
previously existing taint reports.
Reviewed By: martong
Differential Revision: https://reviews.llvm.org/D127306
Initially, I thought there is some fundamental bug here by not using the
bool fields, but it turns out D55425 split this checker into two
separate ones; making these fields dead.
Depends on D127836, which uncovered this issue.
Reviewed By: martong
Differential Revision: https://reviews.llvm.org/D127838
The `Profile` function was incorrectly implemented.
The `StreamErrorState` has an implicit `bool` conversion operator, which
will result in a different hash than faithfully hashing the raw value of
the enum.
I don't have a test for it, since it seems difficult to find one.
Even if we would have one, any change in the hashing algorithm would
have a chance of breaking it, so I don't think it would justify the
effort.
Depends on D127836, which uncovered this issue by marking the related
`Profile` function dead.
Reviewed By: martong, balazske
Differential Revision: https://reviews.llvm.org/D127839
Thanks @kazu for helping me clean these parts in D127799.
I'm leaving the dump methods, along with the unused visitor handlers and
the forwarding methods.
The dead parts actually helped to uncover two bugs, to which I'm going
to post separate patches.
Reviewed By: martong
Differential Revision: https://reviews.llvm.org/D127836
This is an initial step of removing the SimpleSValBuilder abstraction. The SValBuilder alone should be enough.
Reviewed By: martong
Differential Revision: https://reviews.llvm.org/D126127
This change specializes the LLVM RTTI mechanism for SVals.
After this change, we can use the well-known `isa`, `cast`, `dyn_cast`.
Examples:
// SVal V = ...;
// Loc MyLoc = ...;
bool IsInteresting = isa<loc::MemRegionVal, loc::GotoLabel>(MyLoc);
auto MRV = cast<loc::MemRegionVal>(MyLoc);
Optional<loc::MemRegionVal> MaybeMRV = dyn_cast<loc::MemRegionVal>(V)
The current `SVal::getAs` and `castAs` member functions are redundant at
this point, but I believe that they are still handy.
The member function version is terse and reads left-to-right, which IMO
is a great plus. However, we should probably add a variadic `isa` member
function version to have the same casting API in both cases.
Thanks for the extensive TMP help @bzcheeseman!
Reviewed By: bzcheeseman
Differential Revision: https://reviews.llvm.org/D125709
I'm trying to remove unused options from the `Analyses.def` file, then
merge the rest of the useful options into the `AnalyzerOptions.def`.
Then make sure one can set these by an `-analyzer-config XXX=YYY` style
flag.
Then surface the `-analyzer-config` to the `clang` frontend;
After all of this, we can pursue the tablegen approach described
https://discourse.llvm.org/t/rfc-tablegen-clang-static-analyzer-engine-options-for-better-documentation/61488
In this patch, I'm proposing flag deprecations.
We should support deprecated analyzer flags for exactly one release. In
this case I'm planning to drop this flag in `clang-16`.
In the clang frontend, now we won't pass this option to the cc1
frontend, rather emit a warning diagnostic reminding the users about
this deprecated flag, which will be turned into error in clang-16.
Unfortunately, I had to remove all the tests referring to this flag,
causing a mass change. I've also added a test for checking this warning.
I've seen that `scan-build` also uses this flag, but I think we should
remove that part only after we turn this into a hard error.
Reviewed By: martong
Differential Revision: https://reviews.llvm.org/D126215
I'm trying to remove unused options from the `Analyses.def` file, then
merge the rest of the useful options into the `AnalyzerOptions.def`.
Then make sure one can set these by an `-analyzer-config XXX=YYY` style
flag.
Then surface the `-analyzer-config` to the `clang` frontend;
After all of this, we can pursue the tablegen approach described
https://discourse.llvm.org/t/rfc-tablegen-clang-static-analyzer-engine-options-for-better-documentation/61488
In this patch, I'm proposing flag deprecations.
We should support deprecated analyzer flags for exactly one release. In
this case I'm planning to drop this flag in `clang-16`.
In the clang frontend, now we won't pass this option to the cc1
frontend, rather emit a warning diagnostic reminding the users about
this deprecated flag, which will be turned into error in clang-16.
Unfortunately, I had to remove all the tests referring to this flag,
causing a mass change. I've also added a test for checking this warning.
I've seen that `scan-build` also uses this flag, but I think we should
remove that part only after we turn this into a hard error.
Reviewed By: martong
Differential Revision: https://reviews.llvm.org/D126215
I've faced crashes in the past multiple times when some
`check::EndAnalysis` callback caused some crash.
It's really anoying that it doesn't tell which function triggered this
callback.
This patch adds the well-known trace for that situation as well.
Example:
1. <eof> parser at end of file
2. While analyzing stack:
#0 Calling test11
Note that this does not have tests.
I've considered `unittests` for this purpose, by using the
`ASSERT_DEATH()` similarly how we check double eval called functions in
`ConflictingEvalCallsTest.cpp`, however, that the testsuite won't invoke
the custom handlers. Only the message of the `llvm_unreachable()` will
be printed. Consequently, it's not applicable for us testing this
feature.
I've also considered using an end-to-end LIT test for this.
For that, we would need to somehow overload the `clang_analyzer_crash()`
`ExprInspection` handler, to get triggered by other events than the
`EvalCall`. I'm not saying that we could not come up with a generic way
of causing crash in a specific checker callback, but I'm not sure if
that would worth the effort.
Reviewed By: martong
Differential Revision: https://reviews.llvm.org/D127389
Depends on D126560. `getKnownValue` has been changed by the parent patch
in a way that simplification was removed. This is not correct when the
function is called by the Checkers. Thus, a new internal function is
introduced, `getConstValue`, which simply queries the constraint manager.
This `getConstValue` is used internally in the `SimpleSValBuilder` when a
binop is evaluated, this way we avoid the recursion into the `Simplifier`.
Differential Revision: https://reviews.llvm.org/D127285
A crash was seen in CastValueChecker due to a null pointer dereference.
The fix uses QualType::getAsString to avoid the null dereference
when a CXXRecordDecl cannot be obtained. A small reproducer is added,
and cast value notes LITs are updated for the new debug messages.
Reviewed By: steakhal
Differential Revision: https://reviews.llvm.org/D127105
Aligned with the measures we had in D124674, this condition seems to be
unlikely.
Nevertheless, I've made some new measurments with stats just for this,
and data confirms this is indeed unlikely.
Differential Revision: https://reviews.llvm.org/D127190
Assume functions might recurse (see `reAssume` or `tryRearrange`).
During the recursion, the State might not change anymore, that means we
reached a fixpoint. In this patch, we avoid infinite recursion of assume
calls by checking already visited States on the stack of assume function
calls. This patch renders the previous "workaround" solution (D47155)
unnecessary. Note that this is not an NFC patch. If we were to limit the
maximum stack depth of the assume calls to 1 then would it be equivalent
with the previous solution in D47155.
Additionally, in D113753, we simplify the symbols right at the beginning
of evalBinOpNN. So, a call to `simplifySVal` in `getKnownValue` (added
in D51252) is no longer needed.
Fixes https://github.com/llvm/llvm-project/issues/55851
Differential Revision: https://reviews.llvm.org/D126560
I'm also hoisting common code from the existing specializations into a
common trait impl to reduce code duplication.
Reviewed By: martong
Differential Revision: https://reviews.llvm.org/D126801
Make the SimpleSValBuilder to be able to look up and use a constraint
for an operand of a SymbolCast, when the operand is constrained to a
const value.
This part of the SValBuilder is responsible for constant folding. We
need this constant folding, so the engine can work with less symbols,
this way it can be more efficient. Whenever a symbol is constrained with
a constant then we substitute the symbol with the corresponding integer.
If a symbol is constrained with a range, then the symbol is kept and we
fall-back to use the range based constraint manager, which is not that
efficient. This patch is the natural extension of the existing constant
folding machinery with the support of SymbolCast symbols.
Differential Revision: https://reviews.llvm.org/D126481
This reverts commit 3988bd1398.
Did not build on this bot:
https://lab.llvm.org/buildbot#builders/215/builds/6372
/usr/include/c++/9/bits/predefined_ops.h:177:11: error: no match for call to
‘(llvm::less_first) (std::pair<long unsigned int, llvm::bolt::BinaryBasicBlock*>&, const std::pair<long unsigned int, std::nullptr_t>&)’
177 | { return bool(_M_comp(*__it, __val)); }
One could reuse this functor instead of rolling out your own version.
There were a couple other cases where the code was similar, but not
quite the same, such as it might have an assertion in the lambda or other
constructs. Thus, I've not touched any of those, as it might change the
behavior in some way.
As per https://discourse.llvm.org/t/submitting-simple-nfc-patches/62640/3?u=steakhal
Chris Lattner
> LLVM intentionally has a “yes, you can apply common sense judgement to
> things” policy when it comes to code review. If you are doing mechanical
> patches (e.g. adopting less_first) that apply to the entire monorepo,
> then you don’t need everyone in the monorepo to sign off on it. Having
> some +1 validation from someone is useful, but you don’t need everyone
> whose code you touch to weigh in.
Differential Revision: https://reviews.llvm.org/D126068
This patch annotates the most important analyzer function APIs.
Also adds a couple of assertions for uncovering any potential issues
earlier in the constructor; in those cases, the member functions were
already dereferencing the members unconditionally anyway.
Measurements showed no performance impact, nor crashes.
Reviewed By: martong
Differential Revision: https://reviews.llvm.org/D126198
This patch adds a new descendant to the SymExpr hierarchy. This way, now
we can assign constraints to symbolic unary expressions. Only the unary
minus and bitwise negation are handled.
Differential Revision: https://reviews.llvm.org/D125318
Depends on D124758. That patch introduced serious regression in the run-time in
some special cases. This fixes that.
Differential Revision: https://reviews.llvm.org/D126406
Fixes https://github.com/llvm/llvm-project/issues/55546
The assertion mentioned in the issue is triggered because an
inconsistency is formed in the Sym->Class and Class->Sym relations. A
simpler but similar inconsistency is demonstrated here:
https://reviews.llvm.org/D114887 .
Previously in `removeMember`, we didn't remove the old symbol's
Sym->Class relation. Back then, we explained it with the following two
bullet points:
> 1) This way constraints for the old symbol can still be found via it's
> equivalence class that it used to be the member of.
> 2) Performance and resource reasons. We can spare one removal and thus one
> additional tree in the forest of `ClassMap`.
This patch do remove the old symbol's Sym->Class relation in order to
keep the Sym->Class relation consistent with the Class->Sym relations.
Point 2) above has negligible performance impact, empirical measurements
do not show any noticeable difference in the run-time. Point 1) above
seems to be a not well justified statement. This is because we cannot
create a new symbol that would be equal to the old symbol after the
simplification had happened. The reason for this is that the SValBuilder
uses the available constant constraints for each sub-symbol.
Differential Revision: https://reviews.llvm.org/D126281
Depends on D125892. There might be efficiency and performance
implications by using a lambda. Thus, I am going to conduct measurements
to see if there is any noticeable impact.
I've been thinking about two more alternatives:
1) Make `assumeDualImpl` a variadic template and (perfect) forward the
arguments for the used `assume` function.
2) Use a macros.
I have concerns though, whether these alternatives would deteriorate the
readability of the code.
Differential Revision: https://reviews.llvm.org/D125954
Depends on D124758. This is the very same thing we have done for
assumeDual, but this time we do it for assumeInclusiveRange. This patch
is basically a no-brainer copy of that previous patch.
Differential Revision:
https://reviews.llvm.org/D125892
Most clients only used these methods because they wanted to be able to
extend or truncate to the same bit width (which is a no-op). Now that
the standard zext, sext and trunc allow this, there is no reason to use
the OrSelf versions.
The OrSelf versions additionally have the strange behaviour of allowing
extending to a *smaller* width, or truncating to a *larger* width, which
are also treated as no-ops. A small amount of client code relied on this
(ConstantRange::castOp and MicrosoftCXXNameMangler::mangleNumber) and
needed rewriting.
Differential Revision: https://reviews.llvm.org/D125557
This new CTU implementation is the natural extension of the normal single TU
analysis. The approach consists of two analysis phases. During the first phase,
we do a normal single TU analysis. During this phase, if we find a foreign
function (that could be inlined from another TU) then we don’t inline that
immediately, we rather mark that to be analysed later.
When the first phase is finished then we start the second phase, the CTU phase.
In this phase, we continue the analysis from that point (exploded node)
which had been enqueued during the first phase. We gradually extend the
exploded graph of the single TU analysis with the new node that was
created by the inlining of the foreign function.
We count the number of analysis steps of the first phase and we limit the
second (ctu) phase with this number.
This new implementation makes it convenient for the users to run the
single-TU and the CTU analysis in one go, they don't need to run the two
analysis separately. Thus, we name this new implementation as "onego" CTU.
Discussion:
https://discourse.llvm.org/t/rfc-much-faster-cross-translation-unit-ctu-analysis-implementation/61728
Differential Revision: https://reviews.llvm.org/D123773
In some rare cases the type of an SVal might be interesting.
This introspection function exposes this information in tests.
Reviewed By: martong
Differential Revision: https://reviews.llvm.org/D125532
BoolAssignment checker is now taint-aware and warns if a tainted value is
assigned.
Original author: steakhal
Reviewed By: martong
Differential Revision: https://reviews.llvm.org/D125360
This PR changes the `SymIntExpr` so the expression that uses a
negative value as `RHS`, for example: `x +/- (-N)`, is modeled as
`x -/+ N` instead.
This avoids producing a very large `RHS` when the symbol is cased to
an unsigned number, and as consequence makes the value more robust in
presence of casts.
Note that this change is not applied if `N` is the lowest negative
value for which negation would not be representable.
Reviewed By: steakhal
Patch By: tomasz-kaminski-sonarsource!
Differential Revision: https://reviews.llvm.org/D124658
Summary:
By evaluating both children states, now we are capable of discovering
infeasible parent states. In this patch, `assume` is implemented in the terms
of `assumeDuali`. This might be suboptimal (e.g. where there are adjacent
assume(true) and assume(false) calls, next patches addresses that). This patch
fixes a real CRASH.
Fixes https://github.com/llvm/llvm-project/issues/54272
Differential Revision:
https://reviews.llvm.org/D124758
In some cases a parent State is already infeasible, but we recognize
this only if an additonal constraint is added. This patch is the first
of a series to address this issue. In this patch `assumeDual` is changed
to clone the parent State but with an `Infeasible` flag set, and this
infeasible-parent is returned both for the true and false case. Then
when we add a new transition in the exploded graph and the destination
is marked as infeasible, the node will be a sink node.
Related bug:
https://github.com/llvm/llvm-project/issues/50883
Actually, this patch does not solve that bug in the solver, rather with
this patch we can handle the general parent-infeasible cases.
Next step would be to change the State API and require all checkers to
use the `assume*Dual` API and deprecate the simple `assume` calls.
Hopefully, the next patch will introduce `assumeInBoundDual` and will
solve the CRASH we have here:
https://github.com/llvm/llvm-project/issues/54272
Differential Revision: https://reviews.llvm.org/D124674
This patch restores the symmetry between how operator new and operator delete
are handled by also inlining the content of operator delete when possible.
Patch by Fred Tingaud.
Reviewed By: martong
Differential Revision: https://reviews.llvm.org/D124845
It seems like multiple users are affected by a crash introduced by this
commit, thus I'm reverting it for the time being.
Read more about the found reproducers at Phabricator.
Differential Revision: https://reviews.llvm.org/D124658
This reverts commit f0d6cb4a5c.
There are many more instances of this pattern, but I chose to limit this change to .rst files (docs), anything in libcxx/include, and string literals. These have the highest chance of being seen by end users.
Reviewed By: #libc, Mordante, martong, ldionne
Differential Revision: https://reviews.llvm.org/D124708
This PR changes the `SymIntExpr` so the expression that uses a
negative value as `RHS`, for example: `x +/- (-N)`, is modeled as
`x -/+ N` instead.
This avoids producing a very large `RHS` when the symbol is cased to
an unsigned number, and as consequence makes the value more robust in
presence of casts.
Note that this change is not applied if `N` is the lowest negative
value for which negation would not be representable.
Reviewed By: steakhal
Patch By: tomasz-kaminski-sonarsource!
Differential Revision: https://reviews.llvm.org/D124658
Region store was not able to see through this case to the actual
initialized value of STRUCT ff. This change addresses this case by
getting the direct binding. This was found and debugged in a downstream
compiler, with debug guidance from @steakhal. A positive and negative
test case is added.
The specific case where this issue was exposed.
typedef struct {
int a:1;
int b[2];
} STRUCT;
int main() {
STRUCT ff = {0};
STRUCT* pff = &ff;
int a = ((int)pff + 1);
return a;
}
Reviewed By: steakhal, martong
Differential Revision: https://reviews.llvm.org/D124349
This is an extension to diff D99260. This adds an additional exception
for `std::__addressof` in `InnerPointerChecker`.
Patch By alishuja (Ali Shuja Siddiqui)!
Reviewed By: martong, alishuja
Differential Revision: https://reviews.llvm.org/D109467
Essentially, having a default member initializer for a constant member
does not necessarily imply the member will have the given default value.
Remove part of a2e053638b ([analyzer] Treat more const variables and
fields as known contants., 2018-05-04).
Fix#47878
Reviewed By: r.stahl, steakhal
Differential Revision: https://reviews.llvm.org/D124621
Remove unnecessary conversion to Optional<> and incorrect assumption
that BindExpr can return a null state.
Reviewed By: steakhal
Differential Revision: https://reviews.llvm.org/D124681
Historically, exploded graph dumps were disabled in non-debug builds.
It was done so probably because a regular user should not dump the
internal representation of the analyzer anyway and the dump methods
might introduce unnecessary binary size overhead.
It turns out some of the users actually want to dump this.
Note that e.g. `LiveExpressionsDumper`, `LiveVariablesDumper`,
`ControlDependencyTreeDumper` etc. worked previously, and they are
unaffected by this change.
However, `CFGViewer` and `CFGDumper` still won't work for a similar
reason. AFAIK only these two won't work after this change.
Addresses #53873
---
**baseline**
| binary | size | size after strip |
| clang | 103M | 83M |
| clang-tidy | 67M | 54M |
**after this change**
| binary | size | size after strip |
| clang | 103M | 84M |
| clang-tidy | 67M | 54M |
CMake configuration:
```
cmake -S llvm -GNinja -DBUILD_SHARED_LIBS=OFF -DCMAKE_BUILD_TYPE=Release
-DCMAKE_CXX_COMPILER=clang++ -DCMAKE_C_COMPILER=clang
-DLLVM_ENABLE_ASSERTIONS=OFF -DLLVM_USE_LINKER=lld
-DLLVM_ENABLE_DUMP=OFF -DLLVM_ENABLE_PROJECTS="clang;clang-tools-extra"
-DLLVM_ENABLE_Z3_SOLVER=ON -DLLVM_TARGETS_TO_BUILD="X86"
```
Built by `clang-14.0.0`.
Reviewed By: martong
Differential Revision: https://reviews.llvm.org/D124442
We ignored the cast if the enum was scoped.
This is bad since there is no implicit conversion from the scoped enum to the corresponding underlying type.
The fix is basically: isIntegralOrEnumerationType() -> isIntegralOr**Unscoped**EnumerationType()
This materialized in crashes on analyzing the LLVM itself using the Z3 refutation.
Refutation synthesized the given Z3 Binary expression (`BO_And` of `unsigned char` aka. 8 bits
and an `int` 32 bits) with the wrong bitwidth in the end, which triggered an assert.
Now, we evaluate the cast according to the standard.
This bug could have been triggered using the Z3 CM according to
https://bugs.llvm.org/show_bug.cgi?id=44030Fixes#47570#43375
Reviewed By: martong
Differential Revision: https://reviews.llvm.org/D85528
`g_memdup()` allocates and copies memory, thus we should not assume that
the returned memory region is uninitialized because it might not be the
case.
PS: It would be even better to copy the bindings to mimic the actual
content of the buffer, but this works too.
Fixes#53617
Reviewed By: martong
Differential Revision: https://reviews.llvm.org/D124436
The patch is straightforward except the tiny fix in BugReporterVisitors.cpp
that suppresses a default note for "Assuming pointer value is null" when
a note tag from the checker is present. This is probably the right thing to do
but also definitely not a complete solution to the problem of different sources
of path notes being unaware of each other, which is a large and annoying issue
that we have to deal with. Note tags really help there because they're nicely
introspectable. The problem is demonstrated by the newly added getenv() test.
Differential Revision: https://reviews.llvm.org/D122285
In the following example:
int va_list_get_int(va_list *va) {
return va_arg(*va, int); // FP
}
The `*va` expression will be something like `Element{SymRegion{va}, 0, va_list}`.
We use `ElementRegions` for representing the result of the dereference.
In this case, the `IsSymbolic` was set to `false` in the
`getVAListAsRegion()`.
Hence, before checking if the memregion is a SymRegion, we should take
the base of that region.
Analogously to the previous example, one can craft other cases:
struct MyVaList {
va_list l;
};
int va_list_get_int(struct MyVaList va) {
return va_arg(va.l, int); // FP
}
But it would also work if the `va_list` would be in the base or derived
part of a class. `ObjCIvarRegions` are likely also susceptible.
I'm not explicitly demonstrating these cases.
PS: Check the `MemRegion::getBaseRegion()` definition.
Fixes#55009
Reviewed By: xazax.hun
Differential Revision: https://reviews.llvm.org/D124239
This change adds an option to detect all null dereferences for
non-default address spaces, except for address spaces 256, 257 and 258.
Those address spaces are special since null dereferences are not errors.
All address spaces can be considered (except for 256, 257, and 258) by
using -analyzer-config
core.NullDereference:DetectAllNullDereferences=true. This option is
false by default, retaining the original behavior.
A LIT test was enhanced to cover this case, and the rst documentation
was updated to describe this behavior.
Reviewed By: steakhal
Differential Revision: https://reviews.llvm.org/D122841
A recent review emphasized the preference to use DefaultBool instead of
bool for checker options. This change is a NFC and cleans up some of the
instances where bool was used, and could be changed to DefaultBool.
Reviewed By: steakhal
Differential Revision: https://reviews.llvm.org/D123464
Under the hood this prints the same as `QualType::getAsString()` but cuts out the middle-man when that string is sent to another raw_ostream.
Also cleaned up all the call sites where this occurs.
Reviewed By: aaron.ballman
Differential Revision: https://reviews.llvm.org/D123926
WG14 has elected to remove support for K&R C functions in C2x. The
feature was introduced into C89 already deprecated, so after this long
of a deprecation period, the committee has made an empty parameter list
mean the same thing in C as it means in C++: the function accepts no
arguments exactly as if the function were written with (void) as the
parameter list.
This patch implements WG14 N2841 No function declarators without
prototypes (http://www.open-std.org/jtc1/sc22/wg14/www/docs/n2841.htm)
and WG14 N2432 Remove support for function definitions with identifier
lists (http://www.open-std.org/jtc1/sc22/wg14/www/docs/n2432.pdf).
It also adds The -fno-knr-functions command line option to opt into
this behavior in other language modes.
Differential Revision: https://reviews.llvm.org/D123955
Summary: Handle casts for ranges working similarly to APSIntType::apply function but for the whole range set. Support promotions, truncations and conversions.
Example:
promotion: char [0, 42] -> short [0, 42] -> int [0, 42] -> llong [0, 42]
truncation: llong [4295033088, 4295033130] -> int [65792, 65834] -> short [256, 298] -> char [0, 42]
conversion: char [-42, 42] -> uint [0, 42]U[4294967254, 4294967295] -> short[-42, 42]
Differential Revision: https://reviews.llvm.org/D103094
I recently evaluated ~150 of bug reports on open source projects relating to my
GSoC'19 project, which was about tracking control dependencies that were
relevant to a bug report.
Here is what I found: when the condition is a function call, the extra notes
were almost always unimportant, and often times intrusive:
void f(int *x) {
x = nullptr;
if (alwaysTrue()) // We don't need a whole lot of explanation
// here, the function name is good enough.
*x = 5;
}
It almost always boiled down to a few "Returning null pointer, which participates
in a condition later", or similar notes. I struggled to find a single case
where the notes revealed anything interesting or some previously hidden
correlation, which is kind of the point of condition tracking.
This patch checks whether the condition is a function call, and if so, bails
out.
The argument against the patch is the popular feedback we hear from some of our
users, namely that they can never have too much information. I was specifically
fishing for examples that display best that my contribution did more good than
harm, so admittedly I set the bar high, and one can argue that there can be
non-trivial trickery inside functions, and function names may not be that
descriptive.
My argument for the patch is all those reports that got longer without any
notable improvement in the report intelligibility. I think the few exceptional
cases where this patch would remove notable information are an acceptable
sacrifice in favor of more reports being leaner.
Differential Revision: https://reviews.llvm.org/D116597
Do import the definition of objects from a foreign translation unit if that's type is const and trivial.
Differential Revision: https://reviews.llvm.org/D122805
This change fixes an assert that occurs in the SMT layer when refuting a
finding that uses pointers of two different sizes. This was found in a
downstream build that supports two different pointer sizes, The CString
Checker was attempting to compute an overlap for the 'to' and 'from'
pointers, where the pointers were of different sizes.
In the downstream case where this was found, a specialized memcpy
routine patterned after memcpy_special is used. The analyzer core hits
on this builtin because it matches the 'memcpy' portion of that builtin.
This cannot be duplicated in the upstream test since there are no
specialized builtins that match that pattern, but the case does
reproduce in the accompanying LIT test case. The amdgcn target was used
for this reproducer. See the documentation for AMDGPU address spaces here
https://llvm.org/docs/AMDGPUUsage.html#address-spaces.
The assert seen is:
`*Solver->getSort(LHS) == *Solver->getSort(RHS) && "AST's must have the same sort!"'
Ack to steakhal for reviewing the fix, and creating the test case.
Reviewed By: steakhal
Differential Revision: https://reviews.llvm.org/D118050
clang: <root>/clang/lib/StaticAnalyzer/Core/SimpleSValBuilder.cpp:727:
void assertEqualBitWidths(clang::ento::ProgramStateRef,
clang::ento::Loc, clang::ento::Loc): Assertion `RhsBitwidth ==
LhsBitwidth && "RhsLoc and LhsLoc bitwidth must be same!"'
This change adjusts the bitwidth of the smaller operand for an evalBinOp
as a result of a comparison operation. This can occur in the specific
case represented by the test cases for a target with different pointer
sizes.
Reviewed By: NoQ
Differential Revision: https://reviews.llvm.org/D122513
Adds basic parsing/sema/serialization support for the
#pragma omp target parallel loop directive.
Differential Revision: https://reviews.llvm.org/D122359
This change fixes a crash in RangedConstraintManager.cpp:assumeSym due to an
unhandled BO_Div case.
clang: <root>clang/lib/StaticAnalyzer/Core/RangedConstraintManager.cpp:51:
virtual clang::ento::ProgramStateRef
clang::ento::RangedConstraintManager::assumeSym(clang::ento::ProgramStateRef,
clang::ento::SymbolRef, bool):
Assertion `BinaryOperator::isComparisonOp(Op)' failed.
Reviewed By: NoQ
Differential Revision: https://reviews.llvm.org/D122277
This is a NFC refactoring to change makeIntValWithPtrWidth
and remove getZeroWithPtrWidth to use types when forming values to match
pointer widths. Some targets may have different pointer widths depending
upon address space, so this needs to be comprehended.
Reviewed By: steakhal
Differential Revision: https://reviews.llvm.org/D120134
Usages of makeNull need to be deprecated in favor of makeNullWithWidth
for architectures where the pointer size should not be assumed. This can
occur when pointer sizes can be of different sizes, depending on address
space for example. See https://reviews.llvm.org/D118050 as an example.
This was uncovered initially in a downstream compiler project, and
tested through those systems tests.
steakhal performed systems testing across a large set of open source
projects.
Co-authored-by: steakhal
Resolves: https://github.com/llvm/llvm-project/issues/53664
Reviewed By: NoQ, steakhal
Differential Revision: https://reviews.llvm.org/D119601
Few weeks back I was experimenting with reading the uninitialized values from src , which is actually a bug but the CSA seems to give up at that point . I was curious about that and I pinged @steakhal on the discord and according to him this seems to be a genuine issue and needs to be fix. So I goes with fixing this bug and thanks to @steakhal who help me creating this patch. This feature seems to break some tests but this was the genuine problem and the broken tests also needs to fix in certain manner. I add a test but yeah we need more tests,I'll try to add more tests.Thanks
Reviewed By: steakhal, NoQ
Differential Revision: https://reviews.llvm.org/D120489
Few weeks back I was experimenting with reading the uninitialized values from src , which is actually a bug but the CSA seems to give up at that point . I was curious about that and I pinged @steakhal on the discord and according to him this seems to be a genuine issue and needs to be fix. So I goes with fixing this bug and thanks to @steakhal who help me creating this patch. This feature seems to break some tests but this was the genuine problem and the broken tests also needs to fix in certain manner. I add a test but yeah we need more tests,I'll try to add more tests.Thanks
Reviewed By: steakhal, NoQ
Differential Revision: https://reviews.llvm.org/D120489
