Re: [RFC v2 1/2] compiler: use compiler to detect integer overflows

[Linus Torvalds]

On Wed, Nov 26, 2014 at 5:37 PM, Sasha Levin <sasha.levin@xxxxxxxxxx> wrote:
>
> #define IS_UNSIGNED(A) (((typeof(A))-1) >= 0)
> #define TYPE_MAX(A) ((typeof(A))(~0U>>1))
> #define TYPE_MIN(A) (-TYPE_MAX(A) - 1)
> #define check_add_overflow(A, B)                                        \
> ({                                                                      \
>         typeof(A) __a = (A);                                            \
>         typeof(B) __b = (B);                                            \
>         typeof(sizeof(__a) > sizeof(__b) ? __a : __b) __min, __max;     \

That doesn't do what you think it does. The "typeof()" on a ternary
operator will not pick the type of the selected side, it will pick the
normal C integer promotion type of the right-hand sides. The actual
_conditional_ matters not at all for the final type.

Which actually ends up being the thing you want in this case, but the
thing is, it's much better written as

    typeof(__a + __b) __min, __max;

and leave it at that. That way __min and __max have the type of the
integer promotion of A and B.

>         if (IS_UNSIGNED(__a) || IS_UNSIGNED(__b))                       \
>                 0;                                                      \

And again, that's wrong for two reasons. I think you're trying to
"return" 0 from the statement expression, but that's not how it works.

Also, the logic itself is also wrong. If the smaller type is unsigned,
that doesn't help.

But once again, the C integer type promotion DTRT, and you should just then do

      if (IS_UNSIGNED(__a+__b))

but as mentioned, that "if (x) 0;" is not how you'd return 0 anyway.
You'd have to make it part of the next expression.


>         __min = TYPE_MIN(__min);                                        \
>         __max = TYPE_MAX(__max);                                        \
>         (((__a > 0) && (typeof(__max))__b > (__max - ((typeof(__max))__a)))  ||\
>                 ((__a < 0) && (typeof(__max))__b < (__min - ((typeof(__max))__a))));\
> })

.. which I didn't actually validate. And I suspect gcc won't be good
enough to optimize, so it probably generates horrendous code.

And the thing is, I think it's just *wrong* to do "overflow in signed
type". The code that does it shouldn't be helped to do it, it should
be fixed to use an unsigned type.

In other words - in this case, the lofft_t should probably just be a u64.

                   Linus
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/