Page MenuHomePhabricator

[analyzer] Re-enable the "System is over constrained" assertion on optimized builds.
Needs RevisionPublic

Authored by NoQ on Tue, Jan 22, 11:53 AM.

Details

Summary

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.

Diff Detail

Event Timeline

NoQ created this revision.Tue, Jan 22, 11:53 AM
Szelethus accepted this revision.EditedWed, Jan 23, 12:24 AM

Hmmm, came across this one in the not too distant future, and always wondered how painful that performance hit would be that even Release+Asserts should be spared from it. I think 5% performance hit, if asserts are enabled, is an acceptable tradeoff, if this check is crucial.

This revision is now accepted and ready to land.Wed, Jan 23, 12:24 AM

And if we find out that this is far too painful, we can always put it back anyways. Doubt that'll happen though.

a_sidorin accepted this revision.Wed, Jan 23, 1:32 PM

Oh, I remember how much pain have debug mode-only assertions caused.

george.karpenkov requested changes to this revision.Wed, Jan 23, 1:34 PM

We would start getting crashes from TrustNonnullChecker if we enable it. Shouldn't those be fixed first?
An input which crashes with this assertion is test/Analysis/trustnonnullchecker_test.m.

This revision now requires changes to proceed.Wed, Jan 23, 1:34 PM
NoQ added a subscriber: alexfh.Wed, Jan 23, 1:44 PM

I think there are much more problems that we will find this way, starting from

long foo(long x) {
  unsigned y = (unsigned)x;
  if (y <= -1)
    return 0;

  return (y + 2) < 2; // crash :(
}

I don't mind delaying this patch until at least some of them are sorted out.


+@alexfh because he seems to enjoy running us with assertions :)