[PATCH] D137948: [clang][dataflow] Add widening API and implement it for built-in boolean model.

[Yitzhak Mandelbaum via Phabricator via cfe-commits]

ymandel marked 3 inline comments as done and 2 inline comments as done.
ymandel added inline comments.


================
Comment at: clang/include/clang/Analysis/FlowSensitive/DataflowAnalysis.h:68
+/// `LatticeT` can optionally provide the following members:
+///  * `bool widen(const LatticeT &Previous)` - chooses a lattice element that
+///     approximates the join of this element with the argument. Widen is called
----------------
xazax.hun wrote:
> We should document what is the return value for. Also I see `LatticeJoinEffect` instead of bool in the code, but I might be confused.
No, you're right. :) I've completely rewritten the comments -- they were outdated and wrong. They now match what we have in TypeErasedDatafllowAnalysis. Most importantly, we've dropped any mention of defaulting to `join` which is wrong both in fact and conceptually.   Our targeted use of widen means it should not default to `join` -- that would be pointless, since it's prerequisites would guarantee that `join(prev, cur) = cur`, making the join pointless. It instead defaults to an equivalence check.


================
Comment at: clang/include/clang/Analysis/FlowSensitive/DataflowAnalysis.h:108
 
+  LatticeJoinEffect widenTypeErased(TypeErasedLattice &Current,
+                                    const TypeErasedLattice &Previous) final {
----------------
xazax.hun wrote:
> I wonder if `LatticeJoinEffect` is the right name if we also use this for widening. (Also, are we in the process of removing these?)
I had the same thought. How about just `LatticeEffect`? Open to other suggestions as well. Either way, though, I should update in a separate patch in case that breaks something unexpected.

As for removing -- yes, we should remove from join, because we never use it. But, it actually makes sense here.


================
Comment at: clang/include/clang/Analysis/FlowSensitive/DataflowEnvironment.h:227
+  /// Widens the environment point-wise, using `PrevEnv` as needed to inform the
+  /// approximation. by taking the intersection of storage locations and values
+  /// that are stored in them. Distinct values that are assigned to the same
----------------
li.zhe.hua wrote:
> 
I actually just deleted most of the comment -- I don't think the details were helpful.


================
Comment at: clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp:374-378
+  Environment WidenedEnv(*DACtx);
+
+  WidenedEnv.CallStack = CallStack;
+  WidenedEnv.ReturnLoc = ReturnLoc;
+  WidenedEnv.ThisPointeeLoc = ThisPointeeLoc;
----------------
xazax.hun wrote:
> Shouldn't we have a simple copy ctor from PrefEnv that could populate all these fields?
> 
> Also, this makes me wonder if we actually want to move some of these out from environment, since these presumably would not change between basic blocks.
> Shouldn't we have a simple copy ctor from PrefEnv that could populate all these fields?
We *do* have such a copy constructor. The problem is that it's overkill because we want to build LocToVal ourselves by point-wise widening of the elements. For that matter, I wish we could share, rather than copy, `DeclToLoc` and `ExprToLoc` but I don't know of any way to do that. Alternatively: is it possible that we can update in place and don't need a separate `WidenedEnv`?

> Also, this makes me wonder if we actually want to move some of these out from environment, since these presumably would not change between basic blocks.
Agreed. Added a fixme with an associated issue to track.




Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D137948