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

[Gábor Horváth via Phabricator via cfe-commits]

xazax.hun added inline comments.


================
Comment at: clang/include/clang/Analysis/FlowSensitive/DataflowEnvironment.h:36
+/// FIXME: Consider replacing this with a model that is more aligned with C++
+/// value categories.
+enum class SkipPast {
----------------
ymandel wrote:
> I'm not sure that value categories are right here.  The key is that skipping is optional, unlike value categories which are fixed.  I think the problem is just that C++ references are weird (for lack of a technical term). A reference variable exists simultaneously in two different categories -- a pointer and the value it points to. That's what this is trying to capture.
> 
> So, I'd recommend just changing the name to reflect that. WDYT of something like this?
> 
> ```
> /// Indicates how to treat references, if encountered -- as a reference or the value referred to -- when retrieving
> /// storage locations or values.
> enum class RefTreatment {
>    /// Consider the reference itself.
>   AsSelf,
>   /// Consider the referent.
>   AsReferent,
> };
> ```
Yeah, value categories are indeed fixed, I was thinking along the lines of treating expressions as if there was an `LValueToRValue` or `RValueToLValue` conversion. I think some of this problem might come from the ambiguity. E.g., when I want to query the analysis state about the value of `*p`, what do I want? Do I want the value for `*p` as an lvalue or `*p` as an rvalue? I would get a different answer in both cases. If I see `*p` in the source code it is fixed but when I want to query the analysis state it is not.

On the other hand I see that `SkipPast` is used in the transfer functions, and I am wondering if that is justified. According to the language rules, I can never observe the actual value of the reference. I can modify the pointee, (`ref = val`) or create a pointer to the same value (`&ref`), but I cannot actually read or modify what is inside the reference. So I wonder if it might be less error prone if there would be no way in the transfer functions to observe the values of the references either. 

But I do not have a strong feeling about any of this at this point so feel free to leave everything as it is. 

> So, I'd recommend just changing the name to reflect that.

I am not sure how clear `Referent` is. For a non-native speaker `Pointee` might be clearer, although I do see how it can be confusing. 


================
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
----------------
sgatev wrote:
> 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.
In this case, does it really make sense to materialize this mapping? If every subexpression corresponds to a unique location, and always the same location, we could use the subexpression directly as a location. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D116596