On Wed, Sep 26, 2018 at 7:46 PM Aldy Hernandez <al...@redhat.com> wrote:
>
> As I've said in the PR...
>
> For a 1-bit signed field we are trying to subtract the following ranges:
>
> [0, 0] - VR_VARYING
>
> Mathematically these are:
>
> [MAX, MAX] - [MIN, MAX]
> [0, 0] - [MIN, MAX]
> => [0, 0] - [-1, 0]
>
> For ranges: [a, b] - [c, d] is [a - d, b - c], so combine_bounds() yields:
>
> [0, OVERFLOW]
>
> Then we adjust the result for overflows in
> set_value_range_with_overflow() we transform the above into:
>
> [0, +INF]
>
> And since +INF is 0, we get [0, 0] which is incorrect.
>
> [0, 0] - [MIN, MAX] should have given [MIN, MAX].
>
> 1-bit signed fields are weird in that 0 - (-MIN) is still -MIN. In any
> other world, it is an impossible result. For instance, in an 8-bit
> signed world, 0 - (-128) is invalid.
I think that set_value_range_with_overflow saturates to the wrong end, possibly
because wi::sub computes the wrong overflow?
You can't really argue that 0 - (-128) is "invalid" in any sense since
-128 might
not be represented at runtime so you cannot use overflow semantics to restrict
what is valid and what is not when doing the actual operations.
> One option would be to treat signed bit fields as TYPE_OVERFLOW wraps,
> since they seem to have wrapping properties, but I'm afraid such a heavy
> handed approach would yield latent bugs across the compiler. What VRP
> seems to currently be doing in set_and_canonicalize_value_range(), is
> just special casing the wrapping of 1-bit fields:
>
> /* Wrong order for min and max, to swap them and the VR type we need
> to adjust them. */
> if (tree_int_cst_lt (max, min))
> {
> tree one, tmp;
>
> /* For one bit precision if max < min, then the swapped
> range covers all values, so for VR_RANGE it is varying and
> for VR_ANTI_RANGE empty range, so drop to varying as well. */
> if (TYPE_PRECISION (TREE_TYPE (min)) == 1)
> {
> set_value_range_to_varying (vr);
> return;
> }
> ...
> }
>
> I've done the same in set_value_range_with_overflow().
>
> BTW, I believe this is a latent bug. Had the MINUS_EXPR code been
> handed down [0,0] - [-1,0] instead of [0,0] - VR_VARYING, we would've
> triggered it.
>
> Tested on x86-64 Linux.
>
> OK for trunk?
Still OK.
Thanks,
Richard.
