Closed Bug 1356936 Opened 3 years ago Closed 2 years ago

Use __builtin_add/sub/mul_overflow() where available to optimize CheckedInt


(Core :: MFBT, enhancement)

Not set



Tracking Status
firefox55 --- affected
firefox58 --- fixed


(Reporter: mats, Assigned: mats)



(2 files, 1 obsolete file)

Clang 3.4 and GCC 5 have builtin functions for overflow checks.
I suspect they are faster than our current CheckedInt overflow checks
(but I haven't measured).
Comment on attachment 8858691 [details] [diff] [review]
Use __builtin_add/sub/mul_overflow() where available to optimize CheckedInt.

Review of attachment 8858691 [details] [diff] [review]:

::: mfbt/CheckedInt.h
@@ +15,5 @@
>  #include "mozilla/IntegerTypeTraits.h"
> +#if (defined(__clang__) &&                                             \
> +     ((__clang_major__ > 3) ||                                         \
> +      (__clang_major__ == 3 && __clang_minor__ >= 4)))                 \

This might seem like the right way to do it, but it's not.  Per these are *marketing* version numbers, and it's not the case that clang version numbers exposed here reflect capabilities in any way.  You should be using feature-testing macros as described at and instead.

@@ +724,5 @@
> +MOZ_CHECKEDINT_BASIC_BINARY_OPERATOR2(Add, +, __builtin_add_overflow)
> +MOZ_CHECKEDINT_BASIC_BINARY_OPERATOR2(Sub, -, __builtin_sub_overflow)
> +MOZ_CHECKEDINT_BASIC_BINARY_OPERATOR2(Mul, *, __builtin_mul_overflow)

How about moving div/mod outside the #ifdef so they're more obviously shared between both cases?
Attachment #8858691 - Flags: review?(jwalden+bmo) → review-
Mats, are you planning to pick this up again?
Flags: needinfo?(mats)
Attached file bug.cpp
It seems GCC has a bug in __builtin_mul_overflow() when compiling
with the -m32 flag, as we do for our Linux/Android 32-bit builds.
The result from this test should be:
# g++ -std=c++11 bug.cpp  && ./a.out
SUCCESS: overflow detected
Result is 0

but when compiled with -m32 it fails to detect the overflow:
# g++ -m32 -std=c++11 bug.cpp  && ./a.out
ERROR: failed to detect overflow!
Result is 0

(g++ (Ubuntu 5.4.0-6ubuntu1~16.04.4) 5.4.0 20160609)
I excluded 32-bit builds for now since it appears the GCC compiler
we use has not implemented these built-ins correctly. (see above)
Attachment #8858691 - Attachment is obsolete: true
Attachment #8910473 - Flags: review?(ehsan)
Looks like it only fails to detect overflow when not optimizing.  Peculiar.  Can you file a GCC bug?
Will do.
Attachment #8910467 - Attachment mime type: text/x-c++src → text/plain
Comment on attachment 8910473 [details] [diff] [review]
Use __builtin_add/sub/mul_overflow() where available to optimize CheckedInt.

Review of attachment 8910473 [details] [diff] [review]:

Thanks a lot!

::: mfbt/CheckedInt.h
@@ +18,5 @@
> +// since "gcc -m32" claims to support these but its implementation is buggy.
> +#if defined(HAVE_64BIT_BUILD)
> +#if defined(__has_builtin)
> +#define MOZ_HAS_BUILTIN_OP_OVERFLOW (__has_builtin(__builtin_add_overflow))
> +#elif defined(__GNUC__)

clang defines __GNUC__ but it currently defines it to 4.  It of course does define __has_builtin, so this branch should never be taken by clang, but nevertheless, do you mind adding a comment to that affect here, or a !defined(__clang__), whichever you prefer please?
Attachment #8910473 - Flags: review?(ehsan) → review+
Pushed by
Use __builtin_add/sub/mul_overflow() where available to optimize CheckedInt.  r=waldo,ehsan
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
You need to log in before you can comment on or make changes to this bug.