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

[Stanislav Gatev via Phabricator via cfe-commits]

sgatev marked 3 inline comments as done.
sgatev added a comment.

I also agree that the current approach isn't robust. I think that a proper solution would involve patching the CFG because in some cases it seems to be incorrect. For example, the call to `qux` is incorrectly deemed to be unreachable in the CFG of the following code:

  int foo();
  
  class Fatal {
   public:
    ~Fatal() __attribute__((noreturn));
    int bar();
    int baz();
  };
  
  void qux();
  
  void target(bool b1, bool b2) {
    int value = b1 ? foo() : (b2 ? Fatal().bar() : Fatal().baz());
    qux();
  }

I'd be happy to look into a solution for this as a follow up. I guess it would have an impact on other users of the CFG too.



================
Comment at: clang/include/clang/Analysis/FlowSensitive/ControlFlowContext.h:50
+  std::unique_ptr<CFG> Cfg;
+  llvm::DenseMap<const Stmt *, const CFGBlock *> StmtToBlock;
+};
----------------
gribozavr2 wrote:
> xazax.hun wrote:
> > There is a special class for this at `clang/lib/Analysis/CFGStmtMap.cpp`. That also does some special handling of the terminators. I wonder if we need to do something similar here (or just use that class as is?).
> The only downside I see is that CFGStmtMap needs a ParentMap, which is not cheap to make, but we don't need. Maybe we need to make it optional in CFGStmtMap?
The main reason for me not to use `CFGStmtMap` right away is that based on some tests it seems to behave differently compared to what's implemented here. I plan to look into that in detail and see if there's an opportunity to replace the logic here. It shouldn't be too difficult to do that in a follow up as `getStmtToBlock` is used in a single place.


================
Comment at: clang/include/clang/Analysis/FlowSensitive/TypeErasedDataflowAnalysis.h:103
 std::vector<llvm::Optional<TypeErasedDataflowAnalysisState>>
-runTypeErasedDataflowAnalysis(const CFG &Cfg,
+runTypeErasedDataflowAnalysis(const ControlFlowContext &Ctx,
                               TypeErasedDataflowAnalysis &Analysis,
----------------
gribozavr2 wrote:
> I'd suggest a more verbose name like `CFCtx`, since `Ctx` is most often used in Clang for `ASTContext`. (Here and elsewhere in the patch.)
Replaced `Ctx` with `CFCtx` across the patch.


================
Comment at: clang/lib/Analysis/FlowSensitive/TypeErasedDataflowAnalysis.cpp:51
+  if (Block.getTerminator().isTemporaryDtorsBranch()) {
+    // The first successor of a block with a temporary destructor terminator is
+    // the block that evaluates the destructor. If that block has a noreturn
----------------
xazax.hun wrote:
> This comment might be a bit hard to understand without a code example. Admittedly, including an example here could get very chatty, but we could refer to the name of the corresponding test.
> 
> I wonder whether naming the blocks like `The first successor of block 'A'` would make this clearer.
I updated the comment and added more tests. Hopefully, that would be good enough for now and we can replace all of this with a more principled solution shortly.


================
Comment at: clang/unittests/Analysis/FlowSensitive/TypeErasedDataflowAnalysisTest.cpp:47
+    auto Ctx = ControlFlowContext::build(nullptr, Body, Result.Context);
+    assert(Ctx);
 
----------------
gribozavr2 wrote:
> Use `llvm::cantFail` to unwrap the `Expected`?
Much better. Thanks!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D116022