Closed Bug 1356936 Opened 3 years ago Closed 2 years ago

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

Categories

(Core :: MFBT, enhancement)

enhancement
Not set

Tracking

()

RESOLVED FIXED
mozilla58
Tracking Status
firefox55 --- affected
firefox58 --- fixed

People

(Reporter: mats, Assigned: mats)

Details

Attachments

(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).

https://clang.llvm.org/docs/LanguageExtensions.html#checked-arithmetic-builtins
https://gcc.gnu.org/onlinedocs/gcc/Integer-Overflow-Builtins.html
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 http://clang.llvm.org/docs/LanguageExtensions.html#builtin-macros 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 http://clang.llvm.org/docs/LanguageExtensions.html#langext-feature-check and http://clang.llvm.org/docs/LanguageExtensions.html#checked-arithmetic-builtins 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)
> +MOZ_CHECKEDINT_BASIC_BINARY_OPERATOR(Div, /)
> +MOZ_CHECKEDINT_BASIC_BINARY_OPERATOR(Mod, %)

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 mpalmgren@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/55541ed45819
Use __builtin_add/sub/mul_overflow() where available to optimize CheckedInt.  r=waldo,ehsan
https://hg.mozilla.org/mozilla-central/rev/55541ed45819
Status: NEW → RESOLVED
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.