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

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added inline comments.


================
Comment at: clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp:1750
+  //                       [B1] -> [B2] -> [B3] -> [sink]
+  // assert(A && B || C);    \       \       \
+  //                          -------------------> [go on with the execution]
----------------
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*


================
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!
----------------
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.


================
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__));
----------------
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).


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

https://reviews.llvm.org/D65287