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

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

xazax.hun added inline comments.


================
Comment at: clang/include/clang/Analysis/FlowSensitive/DataflowAnalysis.h:114
   bool isEqualTypeErased(const TypeErasedLattice &E1,
                          const TypeErasedLattice &E2) final {
     const Lattice &L1 = llvm::any_cast<const Lattice &>(E1.Value);
----------------
Maybe this is another case where we want to mark the whole class `final` instead of the individual methods? It is fine to address this in a separate PR.


================
Comment at: clang/include/clang/Analysis/FlowSensitive/DataflowAnalysis.h:135
+  // between overloads.
+  struct Rank1 {};
+  struct Rank0 : Rank1 {};
----------------
Is there ever a reason to instantiate `Rank1`? If no, should we do something (like make its ctor private and friends with `Rank0`?). Although possibly not important enough to worth the hassle.


================
Comment at: clang/include/clang/Analysis/FlowSensitive/DataflowAnalysis.h:108
 
+  LatticeJoinEffect widenTypeErased(TypeErasedLattice &Current,
+                                    const TypeErasedLattice &Previous) final {
----------------
ymandel wrote:
> 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.
`LatticeEffect` sounds good to me.


================
Comment at: clang/include/clang/Analysis/FlowSensitive/DataflowEnvironment.h:465
+  // FIXME: move the fields `CallStack`, `ReturnLoc` and `ThisPointeeLoc` into a
+  // separate call-context object, shared between environment in the same call.
+  // https://github.com/llvm/llvm-project/issues/59005
----------------



================
Comment at: clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp:424-425
+  }
+  if (WidenedEnv.DeclToLoc.size() != PrevEnv.DeclToLoc.size() ||
+      WidenedEnv.ExprToLoc.size() != PrevEnv.ExprToLoc.size() ||
+      WidenedEnv.LocToVal.size() != PrevEnv.LocToVal.size() ||
----------------
Isn't some of these tautologically false, as we only read from them?


================
Comment at: clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp:374-378
+  Environment WidenedEnv(*DACtx);
+
+  WidenedEnv.CallStack = CallStack;
+  WidenedEnv.ReturnLoc = ReturnLoc;
+  WidenedEnv.ThisPointeeLoc = ThisPointeeLoc;
----------------
ymandel wrote:
> 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.
> 
> 
> For that matter, I wish we could share, rather than copy, DeclToLoc and ExprToLoc.

One way to maximize sharing (and making copies free) is to use the immutable datastructures in LLVM: https://github.com/llvm/llvm-project/blob/main/llvm/include/llvm/ADT/ImmutableMap.h

Unfortunately, these have their own costs so it might not be easy to decide when it is worth to use them. The Clang Static Analyzer was designed from the ground up with immutable data structures in mind. 

On the other hand, I see that you want a more ad-hoc/opportunistic sharing. And I actually wonder if we need any sharing at all. If we are 100% sure we will do the widening, do we actually need to preserve the previous environment? Couldn't we "consume" it and move certain fields out instead of doing the more expensive copy operation when it makes sense?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D137948