759208 - CheckedInt.h depends on undefined value of signed arithmetic

Status: RESOLVED FIXED

(Core :: MFBT, defect)

Description

[Rafael Ávila de Espíndola (:espindola) (not reading bugmail)]

Attachment: Use unsigned operations

This was found by clang:

https://tbpl.mozilla.org/php/getParsedLog.php?id=12136397&tree=Try&full=1#error0

When trying to find out if a+b or a-b is valid, CheckedInt.h would use the computed result of a+b (or a-b), but that value is undefined for signed types.

Comment 1

[Benoit Jacob [:bjacob] (mostly away)]

Comment on attachment 627798 [details] [diff] [review]
Use unsigned operations

Review of attachment 627798 [details] [diff] [review]:
-----------------------------------------------------------------

::: mfbt/CheckedInt.h
@@ +319,4 @@
>  
>  template<typename T>
>  inline bool
> +IsAddValid(T x, T y)

Passing the result was an optimization, but a minor and premature one, and this is expensive enough to make the cost of this operation negligible.

@@ +328,5 @@
>    // if T was a small type like int8_t, so we explicitly cast to T.
> +
> +  typename UnsignedType<T>::Type ux = x;
> +  typename UnsignedType<T>::Type uy = y;
> +  typename UnsignedType<T>::Type result = ux + uy;

If MSVC gives you compiler errors with that, try making a typedef:
typename UnsignedType<T>::Type UType;
UType ux = x;

Comment 2

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Comment on attachment 627798 [details] [diff] [review]
Use unsigned operations

Review of attachment 627798 [details] [diff] [review]:
-----------------------------------------------------------------

::: mfbt/CheckedInt.h
@@ +680,4 @@
>    T y = rhs.mValue;                                                   \
>    T result = x OP y;                                                  \
>    T isOpValid                                                         \
> +      = detail::Is##NAME##Valid(x, y);                        \

Line up the \. (This can become one line now, I think.)

Comment 3

[Rafael Ávila de Espíndola (:espindola) (not reading bugmail)]

I have pushed to try:

https://tbpl.mozilla.org/?tree=Try&rev=93040f4eb3e0

and will push to mozilla-inbound if that is green.

Comment 4

[Rafael Ávila de Espíndola (:espindola) (not reading bugmail)]

https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=98410387837c

Comment 5

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/98410387837c