Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(809)

Issue 1455163004: Fix UB in SkDivBits (Closed)

Created:
5 years, 1 month ago by mtklein_C
Modified:
5 years, 1 month ago
Reviewers:
mtklein, caryclark
CC:
reviews_skia.org
Base URL:
https://skia.googlesource.com/skia.git@master
Target Ref:
refs/heads/master
Project:
skia
Visibility:
Public.

Description

Fix UB in SkDivBits This used to: DIVBITS_ITER was shifting bits up into the sign bit, which is a no-no. This turns numer into a uint32_t to make those defined, and adds a few notes. x >= 0 is always true for unsigned x, so we needed a few small logic refactors. Instead it now: Only call SkDivBits if the old behavior is required. Usually, just do the divide with /. BUG=skia:3562 Committed: https://skia.googlesource.com/skia/+/988adddd48322bfa3e3cb0c017cfce71fbbf1123 Committed: https://skia.googlesource.com/skia/+/6c7b104b4c08ae2332a6ce3c8c906da4e8c51e5f TBR=reed@google.com No API change. Committed: https://skia.googlesource.com/skia/+/e47f1a7c741c0ea209f7a41358557e66f27ef6a3

Patch Set 1 #

Patch Set 2 : re-enable Math test #

Patch Set 3 : guard #

Patch Set 4 : SK_SUPPORT_LEGACY_DIVBITS_UB #

Patch Set 5 : Hook in SkFixed instead. #

Patch Set 6 : () #

Patch Set 7 : pin #

Unified diffs Side-by-side diffs Delta from patch set Stats (+10 lines, -10 lines) Patch
M BUILD.public View 1 2 1 chunk +1 line, -1 line 0 comments Download
M include/core/SkFixed.h View 1 2 3 4 5 6 1 chunk +8 lines, -1 line 0 comments Download
M tools/dm_flags.json View 1 1 chunk +1 line, -3 lines 0 comments Download
M tools/dm_flags.py View 1 1 chunk +0 lines, -5 lines 0 comments Download

Messages

Total messages: 40 (20 generated)
commit-bot: I haz the power
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1455163004/10002 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1455163004/10002
5 years, 1 month ago (2015-11-18 16:48:22 UTC) #3
mtklein_C
5 years, 1 month ago (2015-11-18 16:54:28 UTC) #5
caryclark
lgtm
5 years, 1 month ago (2015-11-18 16:59:08 UTC) #7
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1455163004/10002 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1455163004/10002
5 years, 1 month ago (2015-11-18 17:55:53 UTC) #9
commit-bot: I haz the power
Committed patchset #2 (id:10002) as https://skia.googlesource.com/skia/+/988adddd48322bfa3e3cb0c017cfce71fbbf1123
5 years, 1 month ago (2015-11-18 17:56:30 UTC) #10
mtklein
A revert of this CL (patchset #2 id:10002) has been created in https://codereview.chromium.org/1457863002/ by mtklein@google.com. ...
5 years, 1 month ago (2015-11-18 22:00:33 UTC) #11
commit-bot: I haz the power
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1455163004/30001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1455163004/30001
5 years, 1 month ago (2015-11-19 17:11:01 UTC) #13
commit-bot: I haz the power
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1455163004/50001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1455163004/50001
5 years, 1 month ago (2015-11-19 17:19:21 UTC) #15
commit-bot: I haz the power
Dry run: This issue passed the CQ dry run.
5 years, 1 month ago (2015-11-19 17:34:58 UTC) #17
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1455163004/50001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1455163004/50001
5 years, 1 month ago (2015-11-19 17:40:07 UTC) #20
commit-bot: I haz the power
Committed patchset #4 (id:50001) as https://skia.googlesource.com/skia/+/6c7b104b4c08ae2332a6ce3c8c906da4e8c51e5f
5 years, 1 month ago (2015-11-19 17:40:50 UTC) #21
stephana
A revert of this CL (patchset #4 id:50001) has been created in https://codereview.chromium.org/1467513002/ by stephana@google.com. ...
5 years, 1 month ago (2015-11-20 15:13:13 UTC) #22
f(malita)
On 2015/11/20 15:13:13, stephana wrote: > A revert of this CL (patchset #4 id:50001) has ...
5 years, 1 month ago (2015-11-20 15:29:06 UTC) #23
commit-bot: I haz the power
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1455163004/90001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1455163004/90001
5 years, 1 month ago (2015-11-20 19:07:04 UTC) #26
commit-bot: I haz the power
Dry run: Try jobs failed on following builders: Test-Ubuntu-GCC-GCE-CPU-AVX2-x86_64-Debug-Trybot on client.skia (JOB_FAILED, http://build.chromium.org/p/client.skia/builders/Test-Ubuntu-GCC-GCE-CPU-AVX2-x86_64-Debug-Trybot/builds/4327)
5 years, 1 month ago (2015-11-20 19:13:37 UTC) #28
commit-bot: I haz the power
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1455163004/110001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1455163004/110001
5 years, 1 month ago (2015-11-20 19:24:38 UTC) #30
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1455163004/110001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1455163004/110001
5 years, 1 month ago (2015-11-20 19:44:09 UTC) #34
commit-bot: I haz the power
Try jobs failed on following builders: skia_presubmit-Trybot on client.skia.fyi (JOB_FAILED, http://build.chromium.org/p/client.skia.fyi/builders/skia_presubmit-Trybot/builds/4138)
5 years, 1 month ago (2015-11-20 19:45:44 UTC) #36
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1455163004/110001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1455163004/110001
5 years, 1 month ago (2015-11-20 19:48:39 UTC) #39
commit-bot: I haz the power
5 years, 1 month ago (2015-11-20 21:58:22 UTC) #40
Message was sent while issue was closed.
Committed patchset #7 (id:110001) as
https://skia.googlesource.com/skia/+/e47f1a7c741c0ea209f7a41358557e66f27ef6a3

Powered by Google App Engine
This is Rietveld 408576698