Page MenuHomePhabricator

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

Authored by Szelethus on Thu, Jul 25, 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

Repository
rL LLVM

Event Timeline

Szelethus created this revision.Thu, Jul 25, 9:07 AM
NoQ added a comment.Thu, Jul 25, 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
1753–1755 ↗(On Diff #211771)

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
469–471 ↗(On Diff #211771)

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

xazax.hun added inline comments.Mon, Jul 29, 9:02 PM
clang/include/clang/Analysis/CFG.h
860 ↗(On Diff #211771)

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 ↗(On Diff #211771)

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
1743 ↗(On Diff #211771)

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

1750 ↗(On Diff #211771)

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
1743 ↗(On Diff #211771)

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

1750 ↗(On Diff #211771)

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

1760 ↗(On Diff #211771)

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.Thu, Aug 1, 7:04 AM
Szelethus marked 10 inline comments as done.

Fixes according to reviewer comments!

Szelethus marked an inline comment as done.Thu, Aug 1, 7:05 AM
Szelethus added inline comments.
clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
1753–1755 ↗(On Diff #211771)

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

clang/test/Analysis/track-control-dependency-conditions.cpp
469–471 ↗(On Diff #211771)

Note that it is in a namespace!

NoQ added inline comments.Wed, Aug 7, 3:16 PM
clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
1753–1755 ↗(On Diff #211771)

Mmm, could you elaborate? >.<

clang/test/Analysis/track-control-dependency-conditions.cpp
469–471 ↗(On Diff #211771)

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.Thu, Aug 8, 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
1753–1755 ↗(On Diff #211771)
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
469–471 ↗(On Diff #211771)

Yea, good point.

Szelethus marked an inline comment as done.Thu, Aug 8, 10:41 AM
Szelethus added inline comments.
clang/test/Analysis/track-control-dependency-conditions.cpp
469–471 ↗(On Diff #211771)

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.Thu, Aug 8, 1:25 PM
clang/include/clang/Analysis/CFG.h
859 ↗(On Diff #212803)

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 ↗(On Diff #212803)

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 ↗(On Diff #212803)

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

clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
1750 ↗(On Diff #211771)

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.Fri, Aug 9, 7:59 PM
clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
1750 ↗(On Diff #211771)

*confirms that 3 successors is 1 successor too many*

1753–1755 ↗(On Diff #211771)

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
469–471 ↗(On Diff #211771)

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.Mon, Aug 12, 2:27 PM
Szelethus marked 12 inline comments as done.

Addressing reviewer feedback!

clang/lib/Analysis/CFG.cpp
5634 ↗(On Diff #212803)

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
1750 ↗(On Diff #211771)

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

1753–1755 ↗(On Diff #211771)

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
469–471 ↗(On Diff #211771)

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

NoQ added inline comments.Tue, Aug 13, 5:12 PM
clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
1750 ↗(On Diff #211771)

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.

1753–1755 ↗(On Diff #211771)

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.Tue, Aug 13, 5:48 PM

Wish granted!

This revision is now accepted and ready to land.Tue, Aug 13, 5:48 PM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptWed, Aug 14, 5:21 AM