[PATCH] D150653: [clang][dataflow] Add `Strict` versions of `Value` and `StorageLocation` accessors.

[Martin Böhme via Phabricator via cfe-commits]

mboehme created this revision.
Herald added subscribers: martong, xazax.hun.
Herald added a reviewer: NoQ.
Herald added a project: All.
mboehme requested review of this revision.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

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()`.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D150653

Files:
  clang/include/clang/Analysis/FlowSensitive/DataflowEnvironment.h
  clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp
  clang/lib/Analysis/FlowSensitive/Transfer.cpp
  clang/lib/Analysis/FlowSensitive/TypeErasedDataflowAnalysis.cpp

-------------- next part --------------
A non-text attachment was scrubbed...
Name: D150653.522514.patch
Type: text/x-patch
Size: 10925 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20230516/a6c97449/attachment-0001.bin>