Last Comment Bug 759208 - CheckedInt.h depends on undefined value of signed arithmetic
: CheckedInt.h depends on undefined value of signed arithmetic
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: MFBT (show other bugs)
: unspecified
: x86 Mac OS X
: -- normal (vote)
: mozilla15
Assigned To: Rafael Ávila de Espíndola (:espindola) (not reading bugmail)
:
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-05-28 15:19 PDT by Rafael Ávila de Espíndola (:espindola) (not reading bugmail)
Modified: 2012-05-30 08:15 PDT (History)
1 user (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Use unsigned operations (2.84 KB, patch)
2012-05-28 15:19 PDT, Rafael Ávila de Espíndola (:espindola) (not reading bugmail)
jacob.benoit.1: review+
Details | Diff | Splinter Review

Description Rafael Ávila de Espíndola (:espindola) (not reading bugmail) 2012-05-28 15:19:11 PDT
Created attachment 627798 [details] [diff] [review]
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) 2012-05-28 15:57:56 PDT
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 (⌚ UTC+1/+2) 2012-05-29 00:01:09 PDT
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) 2012-05-29 05:16:17 PDT
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) 2012-05-29 09:49:45 PDT
https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=98410387837c
Comment 5 Ed Morley [:emorley] 2012-05-30 08:15:56 PDT
https://hg.mozilla.org/mozilla-central/rev/98410387837c

Note You need to log in before you can comment on or make changes to this bug.