This is an archive of the discontinued LLVM Phabricator instance.

[analyzer][CFG] Don't track the condition of asserts
ClosedPublic

Authored by Szelethus on Jul 25 2019, 9:07 AM.

Details

Summary

Well, what is says on the tin I guess!

Some more changes:

  • Move isInevitablySinking() from BugReporter.cpp to CFGBlock's interface
  • Rename and move findBlockForNode() from BugReporter.cpp to ExplodedNode::getCFGBlock()
  • Add some testcases, but are these assert implementations esoteric enough?

Diff Detail

Event Timeline

Szelethus created this revision.Jul 25 2019, 9:07 AM
NoQ added a comment.Jul 25 2019, 1:47 PM

I'm sure i've seen something with a do-while(0) loop. Let's also add the following:

#define assert(x) do {                                    \
  if (!x)                                                 \
    __assert_fail (#expr, __FILE__, __LINE__, __func__)   \
} while (0)
clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
1755–1757

Clever trick, but why not match for logical operators directly? Something like this:

if (auto B = dyn_cast<BinaryOperator>(OuterCond))
  if (B->isLogicalOp())
    return isAssertlikeBlock(Else, Context);
clang/test/Analysis/track-control-dependency-conditions.cpp
499–501

I'm pretty sure you can define this function only once.

xazax.hun added inline comments.Jul 29 2019, 9:02 PM
clang/include/clang/Analysis/CFG.h
860

Is it obvious what sink means in CFG's context? Maybe we should mention what our definition of sink is? Think about non-csa devs too :)

clang/lib/Analysis/CFG.cpp
5628

Are there other nodes that are treated as sinks? If so I wonder if there is a way to keep the list updated. Oh, I just saw that this code is just moved around.

clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
1745

Do we consider a block assert like if both branches are inevitable sinking?

1752

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 :)

clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
1745

No, I think (Then->isInevitablySinking() != Else->isInevitablySinking()) would serve better here.

1752

I think an arrow from [B1] to [B3] is missing for the A == false case.

1762

I would avoid using matchers in the core infrastructure. My experience says they are quite expensive to create and destroy.

Szelethus updated this revision to Diff 212803.Aug 1 2019, 7:04 AM
Szelethus marked 10 inline comments as done.

Fixes according to reviewer comments!

Szelethus marked an inline comment as done.Aug 1 2019, 7:05 AM
Szelethus added inline comments.
clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
1755–1757

What about assert(a + b && "Shouldn't be zero!");?

clang/test/Analysis/track-control-dependency-conditions.cpp
499–501

Note that it is in a namespace!

NoQ added inline comments.Aug 7 2019, 3:16 PM
clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
1755–1757

Mmm, could you elaborate? >.<

clang/test/Analysis/track-control-dependency-conditions.cpp
499–501

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.

Szelethus marked 2 inline comments as done.Aug 8 2019, 12:34 AM

After testing this patch out, I'm confident it works pretty well. Take a look at the following two runs: 138 notes -> 20 notes.

clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
1755–1757
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 (?:).

clang/test/Analysis/track-control-dependency-conditions.cpp
499–501

Yea, good point.

Szelethus marked an inline comment as done.Aug 8 2019, 10:41 AM
Szelethus added inline comments.
clang/test/Analysis/track-control-dependency-conditions.cpp
499–501

Actually, I kinda like how a single namespace is a single test case, an all-in-one package. Do you insist?

xazax.hun added inline comments.Aug 8 2019, 1:25 PM
clang/include/clang/Analysis/CFG.h
859

I think the depth-first manner is an implementation detail and the result of this method should not depend on the traversal strategy. I would remove that part from the comment.

clang/lib/Analysis/CFG.cpp
5634

I am wondering, should we actually scan the whole CFGBlock for ThrowExprs? 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?

5649

This should never be null, I think this should be dead code.

clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
1752

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.

NoQ added inline comments.Aug 9 2019, 7:59 PM
clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
1752

*confirms that 3 successors is 1 successor too many*

1755–1757

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.

clang/test/Analysis/track-control-dependency-conditions.cpp
499–501

Mmm, i guess it's more about __assert_fail never actually residing in a namespace in real life (being a C function at best).

Szelethus updated this revision to Diff 214708.Aug 12 2019, 2:27 PM
Szelethus marked 12 inline comments as done.

Addressing reviewer feedback!

clang/lib/Analysis/CFG.cpp
5634

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?

clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
1752

Alright. I think I got it now (eventually).

1755–1757

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.

clang/test/Analysis/track-control-dependency-conditions.cpp
499–501

I could actually just use [[noreturn]] void halt();, but this looks cooler. :)

NoQ added inline comments.Aug 13 2019, 5:12 PM
clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
1752

There shouldn't be a direct edge from [B1] to the sink. If A is true, we proceed to evaluate B. If A is false, we proceed to evaluate C.

1755–1757

Aha, great!

If you have no major objections, could you please accept formally? :)

And I'll promise to draw CFG graphs all day tomorrow as an exercise after reading the order of operator evaluation rules >.<

NoQ accepted this revision.Aug 13 2019, 5:48 PM

Wish granted!

This revision is now accepted and ready to land.Aug 13 2019, 5:48 PM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptAug 14 2019, 5:21 AM
Szelethus marked an inline comment as done.Aug 17 2019, 10:06 AM
Szelethus added inline comments.
cfe/trunk/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
1759 ↗(On Diff #215088)

Okay, this is stupid. Commited rL369195 to turn this into a condition, accompanied with a test case that would cause an assertion failure.