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

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

mboehme marked 3 inline comments as done.
mboehme added a comment.

In D150653#4345771 <https://reviews.llvm.org/D150653#4345771>, @ymandel wrote:

> I'm a little confused by one of the points in the description:
>
>> `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`.
>
> Since values are immutable, we (intentionally) share them between different storage locations. So, it makes sense that a single value could be associated with multiple storage locations. Now, there's a bug in that values aren't really immutable since you can update the fields, so we have a false sharing bug in the framework, but maybe that only applies to glvalues?

Yes, values are and should definitely be shared between different storage locations.

The comment in the RFC is specifically in the context of the "fake" storage locations that we create for prvalues (since `setValueStrict()` only deals with prvalues). I call these storage locations "fake" because a prvalue doesn't actually have a storage location. During the review of the RFC, one of the reviewers asked whether we should guarantee that a given `Value`, when associated with a prvalue `Expr`, should always use the same "fake" storage location, instead of creating a new storage location every time. The idea was that this might be required for convergence. This argument made sense to me at the time, but having looked at what the existing code does, almost every place that associates a `Value` with a prvalue expression unconditionally create a new "fake" storage location. So this implies to me that creating durable fake storage locations isn't necessary.

Does this make sense?



================
Comment at: clang/include/clang/Analysis/FlowSensitive/DataflowEnvironment.h:273
   ///
+  /// This function is deprecated; prefer `setStorageLocationStrict()`.
+  /// For details, see https://discourse.llvm.org/t/70086.
----------------
ymandel wrote:
> Use LLVM_DEPRECATED? here and below.
There are some buildbots that are configured to produce an error (instead of a warning) when an `LLVM_DEPRECATED` function is used. (I've unfortunately broken builds more than once because of this.)

The functions that are being deprecated in this patch are still being used in many places, so we can only deprecate them with a comment, but not with `LLVM_DEPRECATED`.


================
Comment at: clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp:644
+StorageLocation *Environment::getStorageLocationStrict(const Expr &E) const {
+  assert(E.isGLValue());
+  StorageLocation *Loc = getStorageLocation(E, SkipPast::None);
----------------
sammccall wrote:
> why does it make sense to allow setStorageLocationStrict but not getStorageLocationStrict for builtin functions?
Thanks for pointing this out -- this was an inconsistency that I just missed (and obviously there aren't any tests that exercise this case for `getStorageLocationStrict()`).

I've changed this (in comments and code) to be consistent with `setStorageLocationStrict()`.


================
Comment at: clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp:703
+  assert(E.isPRValue());
+  assert(!isa<ReferenceValue>(Val));
+
----------------
sammccall wrote:
> this isn't documented, should it be?
Yes! Done.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D150653/new/

https://reviews.llvm.org/D150653