[PATCH] D117754: [clang][dataflow] Intersect ExprToLoc when joining environments

[Stanislav Gatev via Phabricator via cfe-commits]

sgatev marked 2 inline comments as done.
sgatev added inline comments.


================
Comment at: clang/lib/Analysis/FlowSensitive/TypeErasedDataflowAnalysis.cpp:49
+  // in tests.
+  std::set<const CFGBlock *> Preds;
   Preds.insert(Block.pred_begin(), Block.pred_end());
----------------
xazax.hun wrote:
> sgatev wrote:
> > xazax.hun wrote:
> > > xazax.hun wrote:
> > > > Are we sure that the memory addresses of CFGBlocks are stable enough for a deterministic order? Alternatively, we could use the block ids for the ordering.
> > > Also, could you describe where the flakiness is originated from? Naively, I'd expect that the order in which we process the predecessors should not change the results of the analysis.
> > You're right, using block ids for ordering is better. I updated the code.
> > 
> > > Also, could you describe where the flakiness is originated from?
> > 
> > Say we have a block `B1` with predecessors `B2` and `B3`. Let the environment of `B2` after evaluating all of its statements is `B2Env = { Expr1 -> Loc1 }` and the environment of `B3` after evaluating all of its statement is `B3Env = { Expr2 -> Loc2 }` where `ExprX -> LocX` refers to a particular mapping of storage locations to expressions.
> > 
> > What we want for the input environment of `B1` is `{}` because `B2Env` and `B3Env` do not contain common assignments of storage locations to expressions. What we got before this patch is either `B2Env.join(B3Env) = { Expr1 -> Loc1 }` or `B3Env.join(B2Env) = { Expr2 -> Loc2 }`.
> > 
> > Without deterministic ordering of predecessors the test that I'm introducing in this patch is flaky.
> > What we got before this patch is either B2Env.join(B3Env) = { Expr1 -> Loc1 } or B3Env.join(B2Env) = { Expr2 -> Loc2 }.
> 
> I think I'm still missing something. With this patch, wouldn't both B2Env.join(B3Env) and B3Env.join(B2Env) produce the empty environment? If that is the case, do we still care about a deterministic order?
> 
> 
> 
That's right. With the patch in `DataflowEnvironment.cpp` the particular order of predecessors doesn't affect the result. However, one of the properties that I'm looking for in tests is being able to remove functionality from the code and have the tests that exercise this functionality fail. This won't necessarily be the case here if the order wasn't deterministic. I don't have a strong preference so please let me know if you have concerns about it. I should also note that we expect all of this to be removed once temporary destructors are handled better in the CFG.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D117754