New issue
Advanced search Search tips

Issue 613003 link

Starred by 2 users

Issue metadata

Status: Assigned
Owner:
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 3
Type: Bug



Sign in to add a comment

Use intrinsics in base/numerics

Project Member Reported by jsc...@chromium.org, May 18 2016

Issue description

Clang has a nice set of intrinsics for efficiently detecting overflow in math operations (e.g. __builtin_add_overflow). Implementing this may require reconsidering how the state is currently handled, because the intrinsics don't expose the direction of overflow. However, the CheckedNumeric public API doesn't expose that information either, so there's a lot of latitude in changing our implementation.
 
Project Member

Comment 1 by bugdroid1@chromium.org, May 21 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/819c82686cc9d3ffb14db3f9926f92dcb50ab910

commit 819c82686cc9d3ffb14db3f9926f92dcb50ab910
Author: jschuh <jschuh@chromium.org>
Date: Sat May 21 01:39:03 2016

Simplify validity detection in SafeNumerics tests

Intrinsics don't identify overflow direction, and we're not relying on
it anyway. So, it's easiest to remove it piece by piece.

BUG=613003

Review-Url: https://codereview.chromium.org/2003693002
Cr-Commit-Position: refs/heads/master@{#395224}

[modify] https://crrev.com/819c82686cc9d3ffb14db3f9926f92dcb50ab910/base/numerics/safe_numerics_unittest.cc

Project Member

Comment 2 by bugdroid1@chromium.org, Nov 4 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/428cf9473ad24fc6e026ebe8fe12f0181c9c35d4

commit 428cf9473ad24fc6e026ebe8fe12f0181c9c35d4
Author: jschuh <jschuh@chromium.org>
Date: Fri Nov 04 21:16:21 2016

Remove usage time.cc of CheckedNumeric::validity()

Fix the API abuse while preserving existing dubious behavior.

BUG=613003
TBR=brettw@chromium.org

Review-Url: https://codereview.chromium.org/2472983003
Cr-Commit-Position: refs/heads/master@{#430018}

[modify] https://crrev.com/428cf9473ad24fc6e026ebe8fe12f0181c9c35d4/base/time/time.cc
[modify] https://crrev.com/428cf9473ad24fc6e026ebe8fe12f0181c9c35d4/base/time/time.h

Project Member

Comment 3 by bugdroid1@chromium.org, Nov 5 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/12d6cf3734f7f1cd64cf52812644a6a49b4da1fe

commit 12d6cf3734f7f1cd64cf52812644a6a49b4da1fe
Author: jschuh <jschuh@chromium.org>
Date: Sat Nov 05 04:23:16 2016

Simplify Checked arithmetic functions in base/numerics

Starting the process of restructuring to match the GCC and
Clang intrinsics. This step is mostly just replacing
RangeConstraint with bool.

BUG=613003

Review-Url: https://codereview.chromium.org/2472933003
Cr-Commit-Position: refs/heads/master@{#430150}

[modify] https://crrev.com/12d6cf3734f7f1cd64cf52812644a6a49b4da1fe/base/numerics/safe_math.h
[modify] https://crrev.com/12d6cf3734f7f1cd64cf52812644a6a49b4da1fe/base/numerics/safe_math_impl.h

Project Member

Comment 4 by bugdroid1@chromium.org, Nov 8 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/216b62a9bb39c9b6e5936d90b8085c300843bd9c

commit 216b62a9bb39c9b6e5936d90b8085c300843bd9c
Author: jschuh <jschuh@chromium.org>
Date: Tue Nov 08 03:53:08 2016

Make Checked* functions better align with GCC/Clang intrinsics

This switches around the return types and out params.

BUG=613003

Review-Url: https://codereview.chromium.org/2480953002
Cr-Commit-Position: refs/heads/master@{#430511}

[modify] https://crrev.com/216b62a9bb39c9b6e5936d90b8085c300843bd9c/base/numerics/safe_math.h
[modify] https://crrev.com/216b62a9bb39c9b6e5936d90b8085c300843bd9c/base/numerics/safe_math_impl.h
[modify] https://crrev.com/216b62a9bb39c9b6e5936d90b8085c300843bd9c/base/numerics/safe_numerics_unittest.cc

Project Member

Comment 6 by bugdroid1@chromium.org, Nov 18 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/990079ab09e7785132dfdd3021c748c878c9a551

commit 990079ab09e7785132dfdd3021c748c878c9a551
Author: jschuh <jschuh@chromium.org>
Date: Fri Nov 18 12:58:21 2016

Use GCC/Clang builtins for Checked(Add|Sub|Mul)

BUG=613003

Review-Url: https://codereview.chromium.org/2509233003
Cr-Commit-Position: refs/heads/master@{#433184}

[modify] https://crrev.com/990079ab09e7785132dfdd3021c748c878c9a551/base/numerics/safe_math_impl.h

Comment 7 by jsc...@chromium.org, Dec 10 2016

Still need to fix the runtime dependency to get this working on clang. They have a bug open for it here: https://llvm.org/bugs/show_bug.cgi?id=16404

In the interim, it looks like we can explicitly link against: libclang_rt.builtins-${ARCH}

Comment 8 by jsc...@chromium.org, Dec 11 2016

The other approach is explicitly passing this switch to clang: -rtlib=compiler-rt
Cc: scottmg@chromium.org
Comment #7 and #8 have the details I uncovered.
Cc: -scottmg@chromium.org
Components: -Security Internals>BaseNumerics

Sign in to add a comment