`getStorageLocation` may return `nullptr` and this will produce crash
when use `cast`, use `dyn_cast_or_null` instead. I test it locally using
[FTXUI](https://github.com/ArthurSonzogni/FTXUI) and it may be the cause
of issue [issue](https://github.com/llvm/llvm-project/issues/68412), but
I am not sure.
Co-authored-by: huqizhi <huqizhi@836744285@qq.com>
The template is agnostic as to the type used by the list, as long as it
is
compatible with `llvm::move` and `std::back_inserter`. In practice,
we've
encountered analyses which use different types (`llvm::SmallVector` vs
`std::vector`), so it seems preferable to leave this open to the caller.
Instead, map prvalue expressions directly to values in a newly introduced map `Environment::ExprToVal`.
This change introduces an additional member variable in `Environment` but is an overall win:
- It is more conceptually correctly, since prvalues don't have storage
locations.
- It eliminates complexity from
`Environment::setValue(const Expr &E, Value &Val)`.
- It reduces the amount of data stored in `Environment`: A prvalue now has a
single entry in `ExprToVal` instead of one in `ExprToLoc` and one in
`LocToVal`.
- Not allocating `StorageLocation`s for prvalues additionally reduces memory
usage.
This patch is the last step in the migration to strict handling of value categories (see https://discourse.llvm.org/t/70086 for details). The changes here are almost entirely internal to `Environment`.
The only externally observable change is that when associating a `RecordValue` with the location returned by `Environment::getResultObjectLocation()` for a given expression, callers additionally need to associate the `RecordValue` with the expression themselves.
Reviewed By: xazax.hun
Differential Revision: https://reviews.llvm.org/D158977
- Both of these constructs are used to represent structs, classes, and unions;
Clang uses the collective term "record" for these.
- The term "aggregate" in `AggregateStorageLocation` implies that, at some
point, the intention may have been to use it also for arrays, but it don't
think it's possible to use it for arrays. Records and arrays are very
different and therefore need to be modeled differently. Records have a fixed
set of named fields, which can have different type; arrays have a variable
number of elements, but they all have the same type.
- Futhermore, "aggregate" has a very specific meaning in C++
(https://en.cppreference.com/w/cpp/language/aggregate_initialization).
Aggregates of class type may not have any user-declared or inherited
constructors, no private or protected non-static data members, no virtual
member functions, and so on, but we use `AggregateStorageLocations` to model all objects of class type.
In addition, for consistency, we also rename the following:
- `getAggregateLoc()` (in `RecordValue`, formerly known as `StructValue`) to
simply `getLoc()`.
- `refreshStructValue()` to `refreshRecordValue()`
We keep the old names around as deprecated synonyms to enable clients to be migrated to the new names.
Reviewed By: ymandel, xazax.hun
Differential Revision: https://reviews.llvm.org/D156788
For the time being, we're keeping the `Strict` versions around as deprecated synonyms so that clients can be migrated, but these synonyms will be removed soon.
Depends On D156673
Reviewed By: ymandel, xazax.hun
Differential Revision: https://reviews.llvm.org/D156674
In the [value categories RFC](https://discourse.llvm.org/t/70086), I proposed that the end state of the migration should be that `getValue()` should only be legal to call on prvalues.
As a stepping stone, to allow migrating off existing calls to `getValue()`, I proposed introducing `getValueStrict()`, which would already have the new semantics.
However, I've now reconsidered this. Any expression, whether prvalue or glvalue, has a value, so really there isn't any reason to forbid calling `getValue()` on glvalues. I'm therefore removing the deprecation from `getValue()` and transitioning existing `getValueStrict()` calls back to `getValue()`.
The other "strict" accessors are a different case. `setValueStrict()` should only be called on prvalues because glvalues need to have a storage location associated with them; it doesn't make sense to only set a value for them. And, of course, `getStorageLocationStrict()` and `setStorageLocationStrict()` should obviously only be called on glvalues because prvalues don't have storage locations.
Reviewed By: ymandel, xazax.hun
Differential Revision: https://reviews.llvm.org/D155921
Also changes code in the underlying model to fit the type expected by the convenience function.
Differential Revision: https://reviews.llvm.org/D156255
The unchecked-optional-access check identifies attempted value
unwrapping without checking if the value exists. These changes extend
that support to checking folly::Optional.
Reviewed By: gribozavr2
Differential Revision: https://reviews.llvm.org/D155890
After this change, `StructValue` is just a wrapper for an `AggregateStorageLocation`. For the wider context, see https://discourse.llvm.org/t/70086.
## How to review
- Start by looking at the comments added / changed in Value.h, StorageLocation.h,
and DataflowEnvironment.h. This will give you a good overview of the semantic
changes.
- Look at the corresponding .cpp files that implement the semantic changes.
- Transfer.cpp, TypeErasedDataflowAnalysis.cpp, and RecordOps.cpp show how the
core of the framework is affected by the semantic changes.
- UncheckedOptionalAccessModel.cpp shows how this complex model is affected by
the changes.
- Many of the changes in the rest of the patch are mechanical in nature.
Reviewed By: ymandel, xazax.hun
Differential Revision: https://reviews.llvm.org/D155446
The argument and return value of `std::forward` is always a reference, so we can simply forward the storage location.
Depends On D155075
Reviewed By: ymandel, gribozavr2, xazax.hun
Differential Revision: https://reviews.llvm.org/D155202
These are intended to ease an upcoming change that will eliminate the duplication between `AggregateStorageLocation` and `StructValue` (see https://discourse.llvm.org/t/70086 for details), but many of the changes also have value in their own right.
Depends On D154586
Reviewed By: ymandel, gribozavr2
Differential Revision: https://reviews.llvm.org/D154597
While working on the ongoing migration to strict handling of value
categories (see https://discourse.llvm.org/t/70086), I ran into issues related
to losing the value associated with an optional.
This issue is hinted at in the existing comments, but the issue didn't become
sufficiently clear to me from those, so I thought it would be worth capturing
more details, along with ideas for how this issue might be fixed.
Reviewed By: ymandel
Differential Revision: https://reviews.llvm.org/D152369
Prior to this patch, `operator->` was being handled like `operator*`: It was
associating a `Value` of type `T` with the expression result (where `T` is the
template argument of the `optional<T>`). This is correct for `operator*`, which
returns a reference (of some flavor) to `T`, so that the result of the
`CXXOperatorCallExpr` is a glvalue of type `T`. However, `operator*` returns a
`T*`, so the result of the `CXXOperatorCallExpr` is a prvalue `T*`, which should
therefore be associated with `PointerValue` that in turn refers to a `T`.
I noticed this issue while working on the migration to strict handling of
value categories (see https://discourse.llvm.org/t/70086). The current behavior
also seems problematic more generally because it's plausible that the framework
may at some point introduce behavior that assumes an `Expr` of pointer type is
always associated with a `PointerValue`.
As it turns out, this patch fixes an existing FIXME in the test
`OptionalValueInitialization`.
Depends On D150657
Reviewed By: ymandel
Differential Revision: https://reviews.llvm.org/D150775
As a replacement, we provide the accessors `getImplicitObjectLocation()` and
`getBaseObjectLocation()`, which are higher-level constructs that cover the use
cases in which `SkipPast::ReferenceThenPointer` was typically used.
Unfortunately, it isn't possible to use these accessors in
UncheckedOptionalAccessModel.cpp; I've added a FIXME to the code explaining the
details. I initially attempted to resolve the issue as part of this patch, but
it turned out to be non-trivial to fix. Instead, I have therefore added a
lower-level replacement for `SkipPast::ReferenceThenPointer` that is used only
within this file.
The wider context of this change is that `SkipPast` will be going away entirely.
See also the RFC at https://discourse.llvm.org/t/70086.
Reviewed By: ymandel, gribozavr2
Differential Revision: https://reviews.llvm.org/D149838
Adjusts the matchers in the optional model to avoid dependency on internal
implementation details of libc++'s `std::optional`. In the process, factors out
the code to check the name of these types so that it's shared throughout.
Differential Revision: https://reviews.llvm.org/D148377
This patch refines the matching of the relevant optional types to anchor on the
global namespace. Previously, we could match anything with the right name
(e.g. `base::Optional`) even if nested within other namespaces. This over
matching resulted in an assertion violation when _different_ `base::Optional`
was encountered nested inside another namespace.
Fixes issue #57036.
Differential Revision: https://reviews.llvm.org/D148344
The check now understands that calling `std::forward`
will not modify the underlying optional value.
This fixes llvm#59705
Reviewed By: PiotrZSL
Differential Revision: https://reviews.llvm.org/D147383
These methods provide a less verbose way of allocating `StorageLocation`s and
`Value`s than the existing `takeOwnership(make_unique(...))` pattern.
In addition, because allocation of `StorageLocation`s and `Value`s now happens
within the `DataflowAnalysisContext`, the `create<T>()` open up the possibility
of using `BumpPtrAllocator` to allocate these objects if it turns out this
helps performance.
Reviewed By: ymandel, xazax.hun, gribozavr2
Differential Revision: https://reviews.llvm.org/D147302
Currently, the interpretation of `swap` calls in the optional model assumes the
optional arguments are modeled (and therefore have valid storage locations and
values). This assumption is incorrect, for example, in the case of unmodeled
optional fields (which can be missing either value or location). This patch
relaxes these assumptions, to return rather than assert when either argument is
not modeled.
Differential Revision: https://reviews.llvm.org/D142710
This patch includes two related changes:
1. Rewrite `compare` operation to be sound. Current version checks for equality
of `isNonEmptyOptional` on both values, judging the values `Same` when the
results are equal. While that works when both are true, it is problematic when
they are both false, because there are four cases in which that's can occur:
both empty, one empty and one unknown (which is two cases), and both unknown. In
the latter three cases, it is unsound to judge them `Same`. This patch changes
`compare` to explicitly check for case of `both empty` and then judge any other
case `Different`.
2. With the change to `compare`, a number of common cases will no longer
terminate. So, we also implement widening to properly handle those cases and
recover termination.
Drive-by: improve performance of `merge` operation.
Of the new tests, the code before the patch fails
* ReassignValueInLoopToSetUnsafe, and
* ReassignValueInLoopToUnknownUnsafe.
Differential Revision: https://reviews.llvm.org/D140344
Currently, the checker only recognizes the nullopt constructor when it is called
without sugar, resulting in a crash in the (rare) case where it has been wrapped
in sugar. This relaxes the constraint by checking the constructor decl directly
(which always contains the same, desugared form) rather than the construct
expression (where the spelling depends on the context).
Differential Revision: https://reviews.llvm.org/D140921
Previously, in the case of an optional constructed from `nullopt`, we relied on
the value constructed for the `nullopt`. This complicates the implementation and
exposes it to bugs (indeed, one such was found), yet doesn't improve the
engine. Instead, this patch constructs a fresh optional representation, rather
than relying on the underlying nullopt representation.
Differential Revision: https://reviews.llvm.org/D140506
The optional model has an option to ignore optionals accessed through smart
pointer types (other than optional itself). This patch improves this feature in
two ways:
1. We extend support to optionals accessed directly through the smart pointer,
like `ptr->value()`. Previously, support was limited to cases that went through
an intermediate field.
2. We clean up the implementation by removing the option from the analysis,
leaving it only in the diagnostic phase (where it is relevant).
3. Adjusts a test which was misleading in what it was testing.
Differential Revision: https://reviews.llvm.org/D140020
This patch adds interpretation of the overloaded equality and inequality
operators available for the optional types.
Fixes issue #57253.
Differential Revision: https://reviews.llvm.org/D139360
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
Currently, the API for a model's custom value comparison returns a
boolean. Therefore, models cannot distinguish between situations where the
values are recognized by the model and different and those where the values are
just not recognized. This patch changes the return value to a tri-valued enum,
allowing models to express "don't know".
This patch is essentially a NFC -- no practical differences result from this
change in this patch. But, it prepares for future patches (particularly,
upcoming patches for widening) which will take advantage of the new flexibility.
Differential Revision: https://reviews.llvm.org/D137334
- Update `transfer` and `diagnose` to take `const CFGElement *` as input in `Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel`.
- Update `clang-tools-extra/clang-tidy/bugprone/UncheckedOptionalAccessCheck.cpp` accordingly.
- Rename `runDataflowAnalysisOnCFG` to `runDataflowAnalysis` and remove the deprecated `runDataflowAnalysis` (this was only used by the now updated optional check).
Reviewed By: gribozavr2, sgatev
Differential Revision: https://reviews.llvm.org/D133930
Followup to D128352. This patch pulls the `NoopLattice` class out from the `NoopAnalysis.h` test file into its own `NoopLattice.h` source file, and uses it to replace usage of `SourceLocationsLattice` in `UncheckedOptionalAccessModel`.
Reviewed By: ymandel, sgatev, gribozavr2, xazax.hun
Differential Revision: https://reviews.llvm.org/D128356
Followup to D128352. This patch pulls the `NoopLattice` class out from the `NoopAnalysis.h` test file into its own `NoopLattice.h` source file, and uses it to replace usage of `SourceLocationsLattice` in `UncheckedOptionalAccessModel`.
Reviewed By: ymandel, sgatev, gribozavr2, xazax.hun
Differential Revision: https://reviews.llvm.org/D128356
This patch adds an optional `PostVisitStmt` parameter to the `runTypeErasedDataflowAnalysis` function, which does one more pass over all statements in the CFG after a fixpoint is reached. It then defines a `diagnose` method for the optional model in a new `UncheckedOptionalAccessDiagnosis` class, but only integrates that into the tests and not the actual optional check for `clang-tidy`. That will be done in a followup patch.
The primary motivation is to separate the implementation of the unchecked optional access check into two parts, to allow for further refactoring of just the model part later, while leaving the checking part alone. Currently there is duplication between the `transferUnwrapCall` and `diagnoseUnwrapCall` functions, but that will be dealt with in the followup.
Because diagnostics are now all gathered into one collection rather than being populated at each program point like when computing a fixpoint, this patch removes the usage of `Pair` and `UnorderedElementsAre` from the optional model tests, and instead modifies all their expectations to simply check the stringified set of diagnostics against a single string, either `"safe"` or some concatenation of `"unsafe: input.cc:y:x"`. This is not ideal as it loses any connection to the `/*[[check]]*/` annotations in the source strings, but it does still retain the source locations from the diagnostic strings themselves.
Reviewed By: sgatev, gribozavr2, xazax.hun
Differential Revision: https://reviews.llvm.org/D127898
Add support for correlated branches to the std::optional dataflow model.
Differential Revision: https://reviews.llvm.org/D125931
Reviewed-by: ymandel, xazax.hun
We distinguish between the referent location for `ReferenceValue` and pointee location for `PointerValue`. The former must be non-empty but the latter may be empty in the case of a `nullptr`
Reviewed By: gribozavr2, sgatev
Differential Revision: https://reviews.llvm.org/D127745
Currently the unchecked-optional-access model fails on this example:
#include <memory>
#include <optional>
void foo() {
std::unique_ptr<std::optional<float>> x;
*x = std::nullopt;
}
You can verify the failure by saving the file as `foo.cpp` and running this command:
clang-tidy -checks='-*,bugprone-unchecked-optional-access' foo.cpp -- -std=c++17
The failing `assert` is in the `transferAssignment` function of the `UncheckedOptionalAccessModel.cpp` file:
assert(OptionalLoc != nullptr);
The symptom can be treated by replacing that `assert` with an early `return`:
if (OptionalLoc == nullptr)
return;
That would be better anyway since we cannot expect to always cover all possible LHS expressions, but it is out of scope for this patch and left as a followup.
Note that the failure did not occur on this very similar example:
#include <optional>
template <typename T>
struct smart_ptr {
T& operator*() &;
T* operator->();
};
void foo() {
smart_ptr<std::optional<float>> x;
*x = std::nullopt;
}
The difference is caused by the `isCallReturningOptional` matcher, which was previously checking the `functionDecl` of the `callee`. This patch changes it to instead use `hasType` directly on the call expression, fixing the failure for the `std::unique_ptr` example above.
Reviewed By: sgatev
Differential Revision: https://reviews.llvm.org/D127434
This patch adds partial support for tracking (i.e. modeling) the contents of an
optional value. Specifically, it supports tracking the value after it is
accessed. We leave tracking constructed/assigned contents to a future patch.
Differential Revision: https://reviews.llvm.org/D124932
This patch moves the implementation of synthetic properties from the StructValue class into the Value base class so that it can be used across all Value instances.
Reviewed By: gribozavr2, ymandel, sgatev, xazax.hun
Differential Revision: https://reviews.llvm.org/D127196
Previously, type aliases were not handled (and resulted in an assertion
firing). This patch generalizes the model to consider aliases everywhere (a
previous patch already considered aliases for optional-returning functions).
Differential Revision: https://reviews.llvm.org/D126972
