[PATCH] D116022: [clang][dataflow] Add support for terminating virtual destructors

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

xazax.hun added a comment.

I am looking at the CFG and wonder if this change is what we what to do in the first place:

  [B7 (ENTRY)]
     Succs (1): B6
  
   [B1]
     1: 0
     2: (void)[B1.1] (CStyleCastExpr, ToVoid, void)
     Preds (2): B2(Unreachable) B3
     Succs (1): B0
  
   [B2 (NORETURN)]
     1: ~Fatal() (Temporary object destructor)
     Preds (1): B3
     Succs (1): B0
  
   [B3]
     1: [B6.2] ? [B4.3] : [B5.5]
     2: int value = b ? foo() : Fatal().bar();
     T: (Temp Dtor) [B5.2]
     Preds (2): B4 B5
     Succs (2): B2 B1
  
   [B4]
     1: foo
     2: [B4.1] (ImplicitCastExpr, FunctionToPointerDecay, int (*)(void))
     3: [B4.2]()
     Preds (1): B6
     Succs (1): B3
  
   [B5]
     1: Fatal() (CXXConstructExpr, [B5.2], [B5.3], class Fatal)
     2: [B5.1] (BindTemporary)
     3: [B5.2]
     4: [B5.3].bar
     5: [B5.4]()
     Preds (1): B6
     Succs (1): B3
  
   [B6]
     1: b
     2: [B6.1] (ImplicitCastExpr, LValueToRValue, _Bool)
     T: [B6.2] ? ... : ...
     Preds (1): B7
     Succs (2): B4 B5
  
   [B0 (EXIT)]
     Preds (2): B1 B2

To summarize the problem, and let me know if I'm wrong:

- In the CFG the noreturn block works as expected, its only successor is the exit block, this is great
- The source of the problem is that we have a join node, where we do a merge BEFORE reaching the noreturn block, so we end up propagating the states from the noreturn block.
- This code tries to pattern match this particular pattern in the CFG and introduce a small degree of local path-sensitivity.

How resilient is this pattern matching? Will this work if we swap the branches of the ternary operator? Will this work if there are multiple dtors in either/both branches? Will this work if there are dtors coming from the condition? Will this work of I start nesting ternary operators? I think this needs more coverage.

Also, I wonder if concentrating on noreturn is the right approach here. The root cause of this problem has relatively little to do with noreturn, and we could probably come up with a poorly behaving analysis even without having a single noreturn destructor. The root cause is the overly eager merge in the CFG.
Some alternative approaches:

- Duplicate the B3 <https://reviews.llvm.org/B3> block to avoid having a join node in the CFG.
- Do a proper pattern matching, match blocks containing the dtor calls with the branches of the ternary, and only propagate the states from the corresponding branches. I.e., do the correct state propagation even when we have no noreturn dtors.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D116022