This serves two purposes:
- Because, today, we only copy the `StructValue`, modifying the destination of
the copy also modifies the source. This is demonstrated by the new checks
added to `CopyConstructor` and `MoveConstructor`, which fail without the
deep copy.
- It lays the groundwork for eliminating the redundancy between
`AggregateStorageLocation` and `StructValue`, which will happen as part of the
ongoing migration to strict handling of value categories (seeo
https://discourse.llvm.org/t/70086 for details). This will involve turning
`StructValue` into essentially just a wrapper for `AggregateStorageLocation`;
under this scheme, the current "shallow" copy (copying a `StructValue` from
one `AggregateStorageLocation` to another) will no longer be possible.
Because we now perform deep copies, tests need to perform a deep equality
comparison instead of just comparing for equal identity of the `StructValue`s.
The new function `recordsEqual()` provides such a deep equality comparison.
Reviewed By: xazax.hun
Differential Revision: https://reviews.llvm.org/D153006
Environments are heavyweight, and copies are observably different from the
original: they introduce new SAT variables, which degrade performance &
debugging. Copies should only be done deliberately, where justified.
Empirically there are several places in the framework where we perform dubious
copies, sometimes entirely accidentally. (see e.g. D153491). Making these
explicit makes this mistake harder.
This patch forces copies to go through fork(), the copy-constructor is private.
This requires changes to existing callsites: some are correct and call fork(),
some are incorrect and are fixed, others are difficult and I left a FIXME.
Differential Revision: https://reviews.llvm.org/D153674
This patch changes the way `Environment::ReturnLoc` is set: Whereas previously it was set by the caller, it is now set by the callee (obviously, as we otherwise would not be able to return references).
The patch also introduces `Environment::ReturnVal`, which is used for non-reference-type return values. This allows these to be handled with the correct value category semantics; see also https://discourse.llvm.org/t/70086, which describes the ongoing migration to strict value category semantics.
Depends On D150776
Reviewed By: ymandel, xazax.hun
Differential Revision: https://reviews.llvm.org/D151194
This is part of the gradual migration to strict handling of value categories, as described in the RFC at https://discourse.llvm.org/t/70086.
This patch migrates some representative calls of the newly deprecated accessors to the new `Strict` functions. Followup patches will migrate the remaining callers. (There are a large number of callers, with some subtlety involved in some of them, so it makes sense to split this up into multiple patches rather than migrating all callers in one go.)
The `Strict` accessors as implemented here have some differences in semantics compared to the semantics originally proposed in the RFC; specifically:
* `setStorageLocationStrict()`: The RFC proposes to create an intermediate
`ReferenceValue` that then refers to the `StorageLocation` for the glvalue.
It turns out though that, even today, most places in the code are not doing
this but are instead associating glvalues directly with their
`StorageLocation`. It therefore didn't seem to make sense to introduce new
`ReferenceValue`s where there were none previously, so I have chosen to
instead make `setStorageLocationStrict()` simply call through to
`setStorageLocation(const Expr &, StorageLocation &)` and merely add the
assertion that the expression must be a glvalue.
* `getStorageLocationStrict()`: The RFC proposes that this should assert that
the storage location for the glvalue expression is associated with an
intermediate `ReferenceValue`, but, as explained, this is often not true.
The current state is inconsistent: Sometimes the intermediate
`ReferenceValue` is there, sometimes it isn't. For this reason,
`getStorageLocationStrict()` skips past a `ReferenceValue` if it is there but
otherwise directly returns the storage location associated with the
expression. This behavior is equivalent to the existing behavior of
`SkipPast::Reference`.
* `setValueStrict()`: The RFC proposes that this should always create the same
`StorageLocation` for a given `Value`, but, in fact, the transfer functions
that exist today don't guarantee this; almost all transfer functions
unconditionally create a new `StorageLocation` when associating an expression
with a `Value`.
There appears to be one special case:
`TerminatorVisitor::extendFlowCondition()` checks whether the expression is
already associated with a `StorageLocation` and, if so, reuses the existing
`StorageLocation` instead of creating a new one.
For this reason, `setValueStrict()` implements this logic (preserve an
existing `StorageLocation`) but makes no attempt to always associate the same
`StorageLocation` with a given `Value`, as nothing in the framework appers to
require this.
As `TerminatorVisitor::extendFlowCondition()` is an interesting special case,
the `setValue()` call there is among the ones that this patch migrates to
`setValueStrict()`.
Reviewed By: sammccall, ymandel, xazax.hun
Differential Revision: https://reviews.llvm.org/D150653
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
This parameter was already a no-op, so removing it doesn't change behavior.
Reviewed By: ymandel
Differential Revision: https://reviews.llvm.org/D150137
This parameter was already a no-op, so removing it doesn't change behavior.
Depends On D149144
Reviewed By: ymandel, xazax.hun, gribozavr2
Differential Revision: https://reviews.llvm.org/D149151
For the wider context of this change, see the RFC at
https://discourse.llvm.org/t/70086.
After this change, global and local variables of reference type are associated
directly with the `StorageLocation` of the referenced object instead of the
`StorageLocation` of a `ReferenceValue`.
Some tests that explicitly check for an existence of `ReferenceValue` for a
variable of reference type have been modified accordingly.
As discussed in the RFC, I have added an assertion to `Environment::join()` to
check that if both environments contain an entry for the same declaration in
`DeclToLoc`, they both map to the same `StorageLocation`. As discussed in
https://discourse.llvm.org/t/70086/5, this also necessitates removing
declarations from `DeclToLoc` when they go out of scope.
In the RFC, I proposed a gradual migration for this change, but it appears
that all of the callers of `Environment::setStorageLocation(const ValueDecl &,
SkipPast` are in the dataflow framework itself, and that there are only a few of
them.
As this is the function whose semantics are changing in a way that callers
potentially need to adapt to, I've decided to change the semantics of the
function directly.
The semantics of `getStorageLocation(const ValueDecl &, SkipPast SP` now no
longer depend on the behavior of the `SP` parameter. (There don't appear to be
any callers that use `SkipPast::ReferenceThenPointer`, so I've added an
assertion that forbids this usage.)
This patch adds a default argument for the `SP` parameter and removes the
explicit `SP` argument at the callsites that are touched by this change. A
followup patch will remove the argument from the remaining callsites,
allowing the `SkipPast` parameter to be removed entirely. (I don't want to do
that in this patch so that semantics-changing changes can be reviewed separately
from semantics-neutral changes.)
Reviewed By: ymandel, xazax.hun, gribozavr2
Differential Revision: https://reviews.llvm.org/D149144
This will eliminate the need for more pass-through APIs. Also replace pass-through usages with this exposure.
Reviewed By: ymandel, gribozavr2, xazax.hun
Differential Revision: https://reviews.llvm.org/D149464
DataflowAnalysisContext has a few too many responsibilities, this narrows them.
It also allows the Arena to be shared with analysis steps, which need to create
Values, without exposing the whole DACtx API (flow conditions etc).
This means Environment no longer needs to proxy all these methods.
(For now it still does, because there are many callsites to update, and maybe
if we separate bool formulas from values we can avoid churning them twice)
In future, if we untangle the concepts of Values from boolean formulas/atoms,
Arena would also be responsible for creating formulas and managing atom IDs.
Differential Revision: https://reviews.llvm.org/D148554
To ensure that we have a pointee for the `PointerValue`, we also create
storage locations for `FunctionDecl`s referenced in the function under analysis.
Reviewed By: gribozavr2
Differential Revision: https://reviews.llvm.org/D148006
This is less verbose than checking for class, struct, and union individually,
and I believe it's also more efficient (not that that should be the overriding
concern).
Reviewed By: sammccall, xazax.hun
Differential Revision: https://reviews.llvm.org/D147603
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
The deduplicated code is moved into initVars().
As an added bonus, pushCallInternal() now also gets the "Add all fields
mentioned in default member initializers" behavior, which apparently had been
added to the Environment ctor but not pushCallInternal().
Reviewed By: xazax.hun, ymandel
Differential Revision: https://reviews.llvm.org/D147326
When building the set of referenced fields for the `DataflowAnalysisContext`,
include fields referenced only in default member initializers. These
initializers are visited in the CFGs of constructors and so the fields must be
included when analysing constructor bodies.
Differential Revision: https://reviews.llvm.org/D144987
Currently, the code assumes that all boolean-typed values are an instance of
`BoolValue` (or its subclasses). Yet, lvalues violate this assumption. This
patch drops the assumption and strengthens the check to confirm the shape of
both values being joined.
The patch also notes as FIXMES a number of problems discovered fixing this bug.
Differential Revision: https://reviews.llvm.org/D141709
There were two (small) bugs causing crashes in the analysis. This patch fixes both of them.
1. An enum value was accessed as a class member. Now, the engine gracefully
ignores such member expressions.
2. Field access in `MemberExpr` of struct/class-typed global variables. Analysis
didn't interpret fields of global vars, because the vars were initialized before
the fields were added to the "allowlist". Now, the allowlist is set _before_
init of globals.
Differential Revision: https://reviews.llvm.org/D141384
This reverts commit 2b1a517a92bfdfa3b692a660e19a2bb22513a567. It's a fix forward
with two memory errors fixed, one of which was the cause of the build breakage
in the buildbots.
Original message:
Previously, the model for structs modeled all fields in a struct when
`createValue` was called for that type. This patch adds a prepass on the
function under analysis to discover the fields referenced in the scope and then
limits modeling to only those fields. This reduces wasted memory usage
(modeling unused fields) which can be important for programs that use large
structs.
Note: This patch obviates the need for https://reviews.llvm.org/D123032.
Previously, the model for structs modeled all fields in a struct when
`createValue` was called for that type. This patch adds a prepass on the
function under analysis to discover the fields referenced in the scope and then
limits modeling to only those fields. This reduces wasted memory usage
(modeling unused fields) which can be important for programss that use large
structs.
Note: This patch obviates the need for https://reviews.llvm.org/D123032.
Differential Revision: https://reviews.llvm.org/D140694
This is a straightfoward way to handle unions in dataflow analysis. Without this change, nullability verification crashes on files that contain unions.
Reviewed By: gribozavr2, ymandel
Differential Revision: https://reviews.llvm.org/D140696
Since now we just ignore all (implicit) integral casts, treating the
resulting value as the same as the underlying value, it could cause
inconsistency between values after `Join` if in some paths the type
doesn't strictly match. This could cause intermittent crashes.
std::optional<bool> o;
int x;
if (o.has_value()) {
x = o.value();
}
Fixes: https://github.com/llvm/llvm-project/issues/59728
Signed-off-by: Jun Zhang <jun@junz.org>
Differential Revision: https://reviews.llvm.org/D140753
Previously, the analysis modeled global variables appearing in the _body_ of
any function (including constructors). But, that misses those appearing in
constructor _initializers_. This patch adds the initializers to the set of
expressions used to determine which globals to model.
Differential Revision: https://reviews.llvm.org/D140501
The comments describing the API for analysis `widen` and the environment `widen`
were overly strict in the preconditions they assumed for the operation. In
particular, both assumed that the previous value preceded the current value in
the relevant ordering. However, that's not generally how widen operators work
and widening itself can violate this property. That is, when the previous value
is the result of a widening, it can easily be "greater" than the current value.
This patch updates the comments to accurately reflect the expectations.
Differential Revision: https://reviews.llvm.org/D140308
Removes an assertion and a useless line. The assertion seems left over from
earlier debugging and the line that follows is a stray line.
Differential Revision: https://reviews.llvm.org/D140306
* Adds API support for widening of lattice elements and environments,
* Updates the algorithm to apply widening where appropriate,
* Implements widening for boolean values. In the process, moves the unsoundness
of comparison from the default implementation of
`Environment::ValueModel::compare` to model-specific handling inside
`DataflowEnvironment::equivalentTo`. This change is intended to clarify
the source and location of unsoundess.
This patch is a replacement for, and was based substantially on, https://reviews.llvm.org/D131645.
Differential Revision: https://reviews.llvm.org/D137948
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
Defines an equivalence relation on the `Value` type to standardize several
places in the code where we replicate the ~same equivalence comparison.
Differential Revision: https://reviews.llvm.org/D135964
Currently, our boolean formulas (`BoolValue`) don't form a lattice, since they
have no Top element. This patch adds such an element, thereby "completing" the
built-in model of bools to be a proper semi-lattice. It still has infinite
height, which is its own problem, but that can be solved separately, through
widening and the like.
Patch 1 for Issue #56931.
Differential Revision: https://reviews.llvm.org/D135397
Extend the context-sensitive analysis to handle a call to a method (of the same
class) from within a method. That, is a member-call expression through `this`.
Differential Revision: https://reviews.llvm.org/D134432
Commit 28bd7945eabdbde2b1fc071ab2f9b78e6e754a1a incidentally fixed the
associated FIXME, but didn't delete it.
Differential Revision: https://reviews.llvm.org/D133588
This patch adds a `Depth` field (default value 2) to `ContextSensitiveOptions`, allowing context-sensitive analysis of functions that call other functions. This also requires replacing the `DeclCtx` field on `Environment` with a `CallString` field that contains a vector of decl contexts, to ensure that the analysis doesn't try to analyze recursive or mutually recursive calls (which would result in a crash, due to the way we handle `StorageLocation`s).
Reviewed By: xazax.hun
Differential Revision: https://reviews.llvm.org/D131809
This patch modifies `Environment`'s `pushCall` method to pass over arguments that are missing storage locations, instead of crashing.
Reviewed By: gribozavr2
Differential Revision: https://reviews.llvm.org/D131600
This patch adds the ability to context-sensitively analyze constructor bodies, by changing `pushCall` to allow both `CallExpr` and `CXXConstructExpr`, and extracting the main context-sensitive logic out of `VisitCallExpr` into a new `transferInlineCall` method which is now also called at the end of `VisitCXXConstructExpr`.
Reviewed By: ymandel, sgatev, xazax.hun
Differential Revision: https://reviews.llvm.org/D131438
