[PATCH] D121455: [clang][dataflow] Add support for nested composite bool expressions

[Stanislav Gatev via Phabricator via cfe-commits]

sgatev added inline comments.


================
Comment at: clang/lib/Analysis/FlowSensitive/Transfer.cpp:516
+    // assigned to it.
+    Visit(&SubExpr);
+    if (auto *Val = dyn_cast_or_null<BoolValue>(
----------------
xazax.hun wrote:
> sgatev wrote:
> > xazax.hun wrote:
> > > Could you elaborate on when would this happen? I'd expect the traversal to always visit the predecessor basic blocks first and within a basic block always visit subexpressions first. So I'd be quite surprised if there is a subexpression we did not visit.
> > From what I've seen, logic operators influence the structure of the CFG through additional basic blocks and terminators, but their sub-expression operators are not added directly in the basic blocks.
> > 
> > For example:
> > ```
> > void test(bool a, bool b, bool c) {
> >     bool d = a && (b || c);
> > }
> > ```
> > 
> > results in:
> > ```
> > 
> > void test(bool a, bool b, bool c)
> >  [B5 (ENTRY)]
> >    Succs (1): B4
> > 
> >  [B1]
> >    1: [B4.2] && ([B3.2] || [B2.2])
> >    2: bool d = a && (b || c);
> >    Preds (3): B2 B3 B4
> >    Succs (1): B0
> > 
> >  [B2]
> >    1: c
> >    2: [B2.1] (ImplicitCastExpr, LValueToRValue, _Bool)
> >    Preds (1): B3
> >    Succs (1): B1
> > 
> >  [B3]
> >    1: b
> >    2: [B3.1] (ImplicitCastExpr, LValueToRValue, _Bool)
> >    T: [B3.2] || ...
> >    Preds (1): B4
> >    Succs (2): B1 B2
> > 
> >  [B4]
> >    1: a
> >    2: [B4.1] (ImplicitCastExpr, LValueToRValue, _Bool)
> >    T: [B4.2] && ...
> >    Preds (1): B5
> >    Succs (2): B3 B1
> > 
> >  [B0 (EXIT)]
> >    Preds (1): B1
> > ```
> > 
> > So, when we evaluate `a && (b || c)` in `B1`, the sub-expression `b || c` has not been evaluated yet. I updated the comment in the code to make that more clear.
> I understand the structure of the CFG and also understand that certain subexpressions are in different basic blocks. But I still don't understand why would `b || c` be not evaluated when we evaluate `a && (b || c)`.
> 
> The operator `&&` in your example is evaluated in `B1`. The operator `||` is evaluated in `B3`. `B3` is a predecessor of `B1`, so if we process the CFG in reverse-post order, we should visit `B3` before `B1`. 
> 
> I am pretty sure if the traversal is well-written we should have the `||` evaluated before we visit `B1`.
> 
> I suspect that something different might be going on. Is it possible that you want to evaluate the `&&` in `B4`?
> 
> Note that `&&` is a terminator there because of the short-circuiting. So at that point we should NOT ask for the value of `||`.
> The operator `||` is evaluated in `B3`.

I don't think that's the case.  Similar to your observation about `&&` being a terminator in `B4`, I believe that `||` is a terminator in `B3`.

I interpret `B3` as the "`a` is true" branch. It still doesn't contain information about `b` and `c` which might be subject to short-circuiting.

Does that make sense?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D121455