[PATCH] D65287: [analyzer][CFG] Don't track the condition of asserts

[Kristóf Umann via Phabricator via cfe-commits]

Szelethus added inline comments.


================
Comment at: clang/lib/Analysis/CFG.cpp:5634
+  // immediately caught.
+  if (std::any_of(Blk->begin(), Blk->end(), [](const CFGElement &Elm) {
+        if (Optional<CFGStmt> StmtElm = Elm.getAs<CFGStmt>())
----------------
xazax.hun wrote:
> I am wondering, should we actually scan the whole `CFGBlock` for `ThrowExpr`s? Shouldn't `Throw` be the terminator? (In case we can get that easily) In case we are afraid of seeing lifetime end or other blocks AFTER throw, maybe it is more efficient to start scanning from the last element ad early return on the first non-throw stmt?
I just moved this code around, I'd be happy to tinker with this later maybe in a followup patch. To confidently make a change like this would require me to write a bunch more tests and study how the block is built, and I'm not even terribly familiar with how exceptions work -- would you mind if I delayed this?


================
Comment at: clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp:1750
+  //                       [B1] -> [B2] -> [B3] -> [sink]
+  // assert(A && B || C);    \       \       \
+  //                          -------------------> [go on with the execution]
----------------
NoQ wrote:
> xazax.hun wrote:
> > baloghadamsoftware wrote:
> > > xazax.hun wrote:
> > > > I wonder if the CFG is correct for your example. If A evaluates to false, we still check C before the sink. If A evaluates to true we still check B before going on. But maybe it is just late for me :)
> > > I think an arrow from `[B1]` to `[B3]` is missing for the `A == false` case.
> > I am still not sure I like this picture. Looking at [B1] I had the impression it has 3 successors which is clearly not the case. 
> *confirms that 3 successors is 1 successor too many*
Alright. I think I got it now (eventually).


================
Comment at: clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp:1753-1755
+  // It so happens that CFGBlock::getTerminatorCondition returns 'A' for block
+  // B1, 'A && B' for B2, and 'A && B || C' for B3. Let's check whether we
+  // reached the end of the condition!
----------------
NoQ wrote:
> Szelethus wrote:
> > NoQ wrote:
> > > Szelethus wrote:
> > > > NoQ wrote:
> > > > > Clever trick, but why not match for logical operators directly? Something like this:
> > > > > ```lang=c++
> > > > > if (auto B = dyn_cast<BinaryOperator>(OuterCond))
> > > > >   if (B->isLogicalOp())
> > > > >     return isAssertlikeBlock(Else, Context);
> > > > > ```
> > > > What about `assert(a + b && "Shouldn't be zero!");`?
> > > Mmm, could you elaborate? >.<
> > ```lang=c++
> > if (auto B = dyn_cast<BinaryOperator>(OuterCond))
> >   if (B->isLogicalOp())
> >     return isAssertlikeBlock(Else, Context);
> > ```
> > We don't need `isLogicalOp()` here.
> > 
> > Also, I should probably have a testcase for the elvis operator (`?:`).
> Mmm, i'm still confused. The `a + b` in your example doesn't appear as an else-block terminator anywhere. And i don't see why would `a + b` behave differently compared to a simple `a`, while i do see why would, say, `a && b` behave differently.
Right. I take it all back. But I guess we might as well just `assert` that it's a logical operator, if it has 2 successors.


================
Comment at: clang/test/Analysis/track-control-dependency-conditions.cpp:469-471
+extern void __assert_fail (__const char *__assertion, __const char *__file,
+                           unsigned int __line, __const char *__function)
+__attribute__ ((__noreturn__));
----------------
NoQ wrote:
> Szelethus wrote:
> > Szelethus wrote:
> > > NoQ wrote:
> > > > Szelethus wrote:
> > > > > NoQ wrote:
> > > > > > I'm pretty sure you can define this function only once.
> > > > > Note that it is in a namespace!
> > > > I mean, why is it in a namespace? Why not just define it once for the whole file? It's not like you're gonna be trying out different variants of `__assert_fail`.
> > > Yea, good point.
> > Actually, I kinda like how a single namespace is a single test case, an all-in-one package. Do you insist?
> Mmm, i guess it's more about `__assert_fail` never actually residing in a namespace in real life (being a C function at best).
I could actually just use `[[noreturn]] void halt();`, but this looks cooler. :)


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

https://reviews.llvm.org/D65287