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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
