[clang][dataflow] Change PruneTriviallyFalseEdges for building CFG

[kinu]

Summary

Keeping this false could end up with extra iterations on a lot of loops
that aren't real ones (e.g. they could be a do-while-false for macros),
and makes the analyses very slow.

This patch changes the default for
CFG::BuildOptions.PruneTriviallyFalseEdges to true to avoid it.

[gribozavr2]

Is it testable? For example, can we show that we don't compute the program state for the program point "p" here:

void f() {
  while (true) {
  }
  /*p*/
}

[kinu]

Add a while-true test

[kinu]

In D149640#4311889, @gribozavr2 wrote:

Is it testable? For example, can we show that we don't compute the program state for the program point "p" here:

I added a simple test, how does it look?

[mboehme]

In D149640#4311963, @kinu wrote:

In D149640#4311889, @gribozavr2 wrote:

Is it testable? For example, can we show that we don't compute the program state for the program point "p" here:

I added a simple test, how does it look?

Do we even need to add a new test for this?

This change breaks the existing test TransferTest.VarDeclInDoWhile, which tests essentially the same scenario, so it's probably sufficient to just update that test?

[ymandel]

Thank you!

[kinu]

Changing the existing test to reflect the flag change

[gribozavr2]

clang/unittests/Analysis/FlowSensitive/TransferTest.cpp

2641 ↗

(On Diff #518702)

Could you update the test name to reflect what the test is checking now? (the comment that you added below)

[mboehme]

clang/unittests/Analysis/FlowSensitive/TransferTest.cpp

2673 ↗

(On Diff #518702)

It's unfortuante that all of these checks have gone away. I think the test was actually trying to test something.

I'd suggest checking the environment at two different places:

void target(int *Foo) {
  do {
    int Bar = *Foo;
    // [[in_loop]]
  } while (true);
  (void)0;
  // [[after_loop]]
}

You can keep the existing checks for the in_loop environment and verify that Results doesn't actually contain an environment for after_loop.

[kinu]

(comment only)

clang/unittests/Analysis/FlowSensitive/TransferTest.cpp

2673 ↗

(On Diff #518702)

Wdyt if we change this to exercise do { } while (false) instead (with the checks that we already have), and add a simple while (true) {}?

[kinu]

Updated changes further not to lose the existing checks with the do-while test.

Also restored the simplest test that exercises the while-true one.

[ymandel]

LGTM

[mboehme]

clang/unittests/Analysis/FlowSensitive/TransferTest.cpp

2690 ↗

(On Diff #518762)

This comment looks stale?

2673 ↗

(On Diff #518702)

Thanks, good idea!

[kinu]

Updated the stale comment.

[kinu]

clang/unittests/Analysis/FlowSensitive/TransferTest.cpp

2690 ↗

(On Diff #518762)

Good catch, updated!

clang/lib/Analysis/FlowSensitive/ControlFlowContext.cpp

static llvm::BitVector findReachableBlocks(const CFG &Cfg) {
   }
 
   return BlockReachable;
 }
 
 llvm::Expected<ControlFlowContext>
 ControlFlowContext::build(const Decl *D, Stmt &S, ASTContext &C) {
   CFG::BuildOptions Options;
-  Options.PruneTriviallyFalseEdges = false;
+  Options.PruneTriviallyFalseEdges = true;
   Options.AddImplicitDtors = true;
   Options.AddTemporaryDtors = true;
   Options.AddInitializers = true;
   Options.AddCXXDefaultInitExprInCtors = true;
 
   // Ensure that all sub-expressions in basic blocks are evaluated.
   Options.setAllAlwaysAdd();