[PATCH] D57062: [analyzer] Re-enable the "System is over constrained" assertion on optimized builds.

[Artem Dergachev via Phabricator via cfe-commits]

NoQ created this revision.
NoQ added reviewers: dcoughlin, xazax.hun, a_sidorin, george.karpenkov, rnkovacs, mikhail.ramalho, Szelethus, baloghadamsoftware.
Herald added subscribers: cfe-commits, dkrupp, donat.nagy, a.sidorin, szepet.

This assertion was only enabled in Debug+Asserts, not on Release+Asserts, because it's expensive. Indeed, enabling it on Release+Asserts builds seems to cause varied slowdown, up to roughly ~5%. However, i think it's worth it, because this assertion is very important because it's very fundamental: when it crashes, it means that we've investigated a path that was already //obviously// infeasible, i.e. we could have refuted the path much earlier if we simply looked closer at it with existing solvers.
So, because, testing on large codebases is pretty much always done in Release+Asserts mode, it kinda scares me that we're missing out on such an important test while doing our ultimate real-world testing.

The slowdown is at most a constant factor - like, it roughly doubles the cost of every assume() operation, so it won't cause more than 2x slowdown, and in practice assume() isn't showing up in our profiles too much, so the ~5% number is relatively reliable.
Of course, the behavior of Clang we actually release (i.e., built without asserts) isn't really affected.


Repository:
  rC Clang

https://reviews.llvm.org/D57062

Files:
  include/clang/StaticAnalyzer/Core/PathSensitive/ConstraintManager.h


Index: include/clang/StaticAnalyzer/Core/PathSensitive/ConstraintManager.h
===================================================================
--- include/clang/StaticAnalyzer/Core/PathSensitive/ConstraintManager.h
+++ include/clang/StaticAnalyzer/Core/PathSensitive/ConstraintManager.h
@@ -94,13 +94,7 @@
     // If StTrue is infeasible, asserting the falseness of Cond is unnecessary
     // because the existing constraints already establish this.
     if (!StTrue) {
-#ifndef __OPTIMIZE__
-      // This check is expensive and should be disabled even in Release+Asserts
-      // builds.
-      // FIXME: __OPTIMIZE__ is a GNU extension that Clang implements but MSVC
-      // does not. Is there a good equivalent there?
       assert(assume(State, Cond, false) && "System is over constrained.");
-#endif
       return ProgramStatePair((ProgramStateRef)nullptr, State);
     }
 


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D57062.182951.patch
Type: text/x-patch
Size: 884 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20190122/7bf22fba/attachment.bin>