[PATCH] D116596: [clang][dataflow] Add transfer functions for assignment

[Stanislav Gatev via Phabricator via cfe-commits]

sgatev added inline comments.


================
Comment at: clang/include/clang/Analysis/FlowSensitive/DataflowEnvironment.h:34
+/// storage locations or values.
+enum class SkipPast {
+  /// No indirections should be skipped past.
----------------
xazax.hun wrote:
> I am just wondering if this is the right level of abstraction. Maybe users do think of this in terms of skipping references? 
> Or would they think in terms of value categories? E.g., having `getLValue` vs `getRValue`. I think we can leave this as is for now, I just like the idea of exploring alternatives.
Absolutely! It makes sense to me to consider this (and other) alternatives so I added a FIXME as a reminder.


================
Comment at: clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp:95
+StorageLocation &Environment::createStorageLocation(const Expr &E) {
+  // Evaluated expressions are always assigned the same storage locations to
+  // ensure that the environment stabilizes across loop iterations. Storage
----------------
xazax.hun wrote:
> What is the model for expressions that evaluate to different locations in every loop iterations, e.g.:
> ```
> for(int *p = array; p != array + size; ++p)
>   *p; // The dereference expression
> ```
> 
> Here, having `*p` always evaluate to the same location is not correct, we'd probably need a way to widen this. 
In the current model, the storage location for `p` is stable and the value that is assigned to it changes. So, in one iteration the value assigned to the storage location for `p` is `val1` which is a `PointerValue` that points to `loc1` and in another iteration the value assigned to the storage location for `p` is `val2` which is a `PointerValue` that points to `loc2`. The storage location for `*p` is also stable and the value that is assigned to it is a `ReferenceValue` that points to either `loc1` or `loc2`. I implemented this and added a test.


================
Comment at: clang/lib/Analysis/FlowSensitive/Transfer.cpp:46
+
+      Env.setValue(*LHSLoc, *RHSVal);
+    }
----------------
xazax.hun wrote:
> I think you probably also want to set the correct location for the whole expression. Or is that handled somewhere else?
You're right. I updated the code and added a test.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D116596