This reverts commit 19e21887eb. I
accidentally landed the non-final version of the patch that used
decomposition declarations (not yet usable in LLVM/Clang source).
A truth assignment to atomic boolean values which satisfy `Constraints` will be returned if found by the solver.
This gives us more information which can be helpful for debugging or constructing warning messages.
Reviewed By: hlopko, gribozavr2, sgatev
Differential Revision: https://reviews.llvm.org/D129180
Treat `std::nullptr_t` as a regular scalar type to avoid tripping
assertions when analyzing code that uses `std::nullptr_t`.
Differential Revision: https://reviews.llvm.org/D129097
This patch deletes the now-unused `SourceLocationsLattice` class, along with its containing files and surrounding helper functions and tests.
Reviewed By: xazax.hun, ymandel, sgatev, gribozavr2
Differential Revision: https://reviews.llvm.org/D128448
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
When a `nullptr` is assigned to a pointer variable, it is wrapped in a `ImplicitCastExpr` with cast kind `CK_NullTo(Member)Pointer`. This patch assigns singleton pointer values representing null to these expressions.
For each pointee type, a singleton null `PointerValue` is created and stored in the `NullPointerVals` map of the `DataflowAnalysisContext` class. The pointee type is retrieved from the implicit cast expression, and used to initialise the `PointeeLoc` field of the `PointerValue`. The `PointeeLoc` created is not mapped to any `Value`, reflecting the absence of value indicated by null pointers.
Reviewed By: gribozavr2, sgatev, xazax.hun
Differential Revision: https://reviews.llvm.org/D128056
This patch introduces `buildAndSubstituteFlowCondition` - given a flow condition token, this function returns the expression of constraints defining the flow condition, with values substituted where specified.
As an example:
Say we have tokens `FC1`, `FC2`, `FC3`:
```
FlowConditionConstraints: {
FC1: C1,
FC2: C2,
FC3: (FC1 v FC2) ^ C3,
}
```
`buildAndSubstituteFlowCondition(FC3, /*Substitutions:*/{{C1 -> C1'}})`
returns a value corresponding to `(C1' v C2) ^ C3`.
Note:
This function returns the flow condition expressed directly as its constraints, which differs to how we currently represent the flow condition as a token bound to a set of constraints and dependencies. Making the representation consistent may be an option to consider in the future.
Depends On D128357
Reviewed By: gribozavr2, xazax.hun
Differential Revision: https://reviews.llvm.org/D128363
`createStorageLocation` in `DataflowEnvironment` is now a trivial wrapper around the logic in `DataflowAnalysisContext`.
Additionally, `getObjectFields` and `getFieldsFromClassHierarchy` (required for the implementation of `createStorageLocation`) are also moved to `DataflowAnalysisContext`.
Reviewed By: gribozavr2, sgatev
Differential Revision: https://reviews.llvm.org/D128359
`equivalentBoolValues` compares equivalence between two booleans. The current implementation does not consider constraints imposed by flow conditions on the booleans and its subvalues.
Depends On D128520
Reviewed By: gribozavr2, xazax.hun
Differential Revision: https://reviews.llvm.org/D128521
Given a set of `Constraints`, `querySolver` adds common background information across queries (`TrueVal` is always true and `FalseVal` is always false) and passes the query to the solver.
`checkUnsatisfiable` is a simple wrapper around `querySolver` for checking that the solver returns an unsatisfiable result.
Depends On D128519
Reviewed By: gribozavr2, xazax.hun
Differential Revision: https://reviews.llvm.org/D128520
To keep functionality of creating boolean expressions in a consistent location.
Depends On D128357
Reviewed By: gribozavr2, sgatev, xazax.hun
Differential Revision: https://reviews.llvm.org/D128519
A flow condition is represented with an atomic boolean token, and it is bound to a set of constraints: `(FC <=> C1 ^ C2 ^ ...)`. \
This was internally represented as `(FC v !C1 v !C2 v ...) ^ (C1 v !FC) ^ (C2 v !FC) ^ ...` and tracked by 2 maps:
- `FlowConditionFirstConjunct` stores the first conjunct `(FC v !C1 v !C2 v ...)`
- `FlowConditionRemainingConjuncts` stores the remaining conjuncts `(C1 v !FC) ^ (C2 v !FC) ^ ...`
This patch simplifies the tracking of the constraints by using a single `FlowConditionConstraints` map which stores `(C1 ^ C2 ^ ...)`, eliminating the use of two maps.
Reviewed By: gribozavr2, sgatev, xazax.hun
Differential Revision: https://reviews.llvm.org/D128357
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 precedes a future patch to make PointeeLoc for PointerValue possibly empty (for nullptr), by using a pointer instead of a reference type.
ReferenceValue should maintain a non-empty PointeeLoc reference.
Reviewed By: gribozavr2
Differential Revision: https://reviews.llvm.org/D127312
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
This is part of the implementation of the dataflow analysis framework.
See "[RFC] A dataflow analysis framework for Clang AST" on cfe-dev.
Differential Revision: https://reviews.llvm.org/D120495
Reviewed-by: ymandel, xazax.hun
Currently, we assert that `CXXCtorInitializer`s are field initializers. Replace
the assertion with an early return. Otherwise, we crash every time we process a
constructor with a non-field (e.g. base class) initializer.
Differential Revision: https://reviews.llvm.org/D126419
The API for `AggregateStorageLocation` does not allow for missing fields (it asserts). Therefore, it is incorrect to filter out any fields at location-creation time which may be accessed by the code. Currently, we limit filtering to private, base-calss fields on the assumption that those can never be accessed. However, `friend` declarations can invalidate that assumption, thereby breaking our invariants.
This patch removes said field filtering to avoid violating the invariant of "no missing fields" for `AggregateStorageLocation`.
Differential Revision: https://reviews.llvm.org/D126420
When constructing the `Environment`, the `this` pointee is established
for a `CXXMethodDecl` by looking at its parent. However, inside of
lambdas, a `CXXThisExpr` refers to the captured `this` coming from the
enclosing member function.
When establishing the `this` pointee for a function, we check whether
the function is a lambda, and check for an enclosing member function
to establish the `this` pointee storage location.
Differential Revision: https://reviews.llvm.org/D126413
Support for unions is incomplete (per 99f7d55e) and the `this` pointee
storage location is not set for unions. The assert in
`VisitCXXThisExpr` is then guaranteed to trigger when analyzing member
functions of a union.
This commit changes the assert to an early-return. Any expression may
be undefined, and so having a value for the `CXXThisExpr` is not a
postcondition of the transfer function.
Differential Revision: https://reviews.llvm.org/D126405
Ignore `MemberLocToStruct` in environment comparison. As an ancillary data
structure, including it is redundant. We also can generate environments which
differ in their `MemberLocToStruct` but are otherwise equivalent.
Differential Revision: https://reviews.llvm.org/D126314
Currently, the maximum number of iterations of the loop for finding the fixpoint
of the dataflow analysis is set at 2^16. When things go wrong in an analysis,
this can be far too large. This patch changes the limit to be proportional to
the size of the CFG, which will generally be far smaller than 2^16 (while still
maintaining 2^16 as the absolute limit).
Differential Revision: https://reviews.llvm.org/D126316
Sub-expressions that are logical operators are not spelled out
separately in basic blocks, so we need to manually visit them when we
encounter them. We do this in both the `TerminatorVisitor`
(conditionally) and the `TransferVisitor` (unconditionally), which can
cause cause an expression to be visited twice when the binary
operators are nested 2+ times.
This changes the visit in `TransferVisitor` to check if it has been
evaluated before trying to visit the sub-expression.
Differential Revision: https://reviews.llvm.org/D125821
Weaken the guard for whether a sub-expression has been evaluated to
only check for the storage location, instead of checking for the
value. It should be sufficient to check for the storage location, as
we don't necessarily guarantee that a value will be set for the
location (although this is currently true right now).
Differential Revision: https://reviews.llvm.org/D125823
This check verifies the safety of access to `std::optional` and related
types (including `absl::optional`). It is based on a corresponding Clang
Dataflow Analysis, which does most of the work. This check merely runs it and
converts its findings into diagnostics.
Differential Revision: https://reviews.llvm.org/D121120
A follow-up to 62b2a47 to centralize the logic that skips expressions
that the CFG does not emit. This allows client code to avoid
sprinkling this logic everywhere.
Add redirects in the transfer function to similarly skip such
expressions by forwarding the visit to the sub-expression.
Differential Revision: https://reviews.llvm.org/D124965
Kazu Hirata