C++: Handle overflow for upperbound

[andersfugmann]

SimpleRangeAnalysis::getGuardedUpperBound may calculate wrong upper bounds when overflows are involved.
This PR change the said predicate to not require that no overflows exists in the guard expression.

[andersfugmann]

This predicate looks at any child of guard including expressions that does not affect guardVa.

I was considering extending this to:
not exists(Expr e | e = guard.getAChild+() and e = guardVa.getParent*() | convertedExprMightOverflow(e))

but I was unable to construct an expression where this had any effect.

[MathiasVP]

It's a bit scary that we have to exclude overflows in any child of the guard. This is not something we've had to do before, I think. I would have hoped that we could fix it inside upperBoundFromGuard somehow. Is the real problem that we are overflowing in linearAccessImpl?

[jbj]

I have the same concerns as @MathiasVP. I'd also like to think a bit more about the root cause, but at least we have this PR if we don't find a better solution before Monday.

[andersfugmann]

I agree there might be a better solution for this, but I'm unsure on where we should add this.

The predicate getTruncatedUpperBounds does handle the overflows correctly, but the addition of predicate getGuardedUpperBound was add specifically to handle cases where getTruncatedUpperBounds would widen the result inside a loop and hence miss obvious cases.

[jbj]

Whenever we use transitive closures like this, we should double-check tuple counts and performance. But we might change the whole approach, so we don't need to do that now.

[jbj]

Anders and I looked at the tuple counts together on the Asterisk project. The TC gets compiled to a boundedFastTC with a good restriction on the starting points. It produces 34M (compressed) rows in 7MB, which is not alarming, and DCA shows no measurable impact.

[jbj]

I have the same concerns as @MathiasVP. I'd also like to think a bit more about the root cause, but at least we have this PR if we don't find a better solution before Monday.

[jbj]

I was able to construct a test case to show that the unsound treatment of overflow was not introduced with #6568 but existed already:

void test_overflow() {
  unsigned int x = 0xffFFffFF;
  if ((x + 256) <= 512) {
    out(x); // range analysis says upper bound is 256
  }
}

The existing problem is much harder to solve than the amplification of it that got introduced with #6568. That's because we haven't calculated overflow information yet at the point where we determine what to do with guard phi nodes. The only general solution I can think of is to make linearAccessImpl much more conservative around unsigned arithmetic.

[jbj]

This PR might not be the ideal solution, but I think it's the most low-risk solution we can merge this morning, before branching rc/3.3. It's been validated with DCA and by looking at tuple counts, and all alternatives we've considered would either involve a lot more code or would remove a lot more good results.