AuroraMiddleware/skia2
1
0
Fork 0
Code Issues Pull Requests Projects Releases Wiki Activity

Revert of Fix UB in SkDivBits (patchset #2 id:10002 of https://codereview.chromium.org/1455163004/ )

Browse Source 
Reason for revert:
Need to reland with #define guards for tiny layout test changes. (Yikes!)

Original issue's description:
> Fix UB in SkDivBits
>
> 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.
>
> BUG=skia:3562
>
> Committed: https://skia.googlesource.com/skia/+/988adddd48322bfa3e3cb0c017cfce71fbbf1123

TBR=caryclark@google.com,mtklein@chromium.org
NOPRESUBMIT=true
NOTREECHECKS=true
NOTRY=true
BUG=skia:3562

Review URL: https://codereview.chromium.org/1457863002
This commit is contained in:
mtklein 2015-11-18 14:01:07 -08:00 committed by Commit bot
parent a445cbccef
commit c8e8fc6e98
4 changed files with 23 additions and 26 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

2
BUILD.public
View File

@ -439,7 +439,7 @@ cc_test(
"--nogpu",
"--verbose",
# TODO(mtklein): maybe investigate why these fail?
"--match ~FontMgr ~Scalar ~Canvas ~Codec_stripes ~Codec_Dimensions ~Codec ~Stream ~skps ~RecordDraw_TextBounds",
"--match ~FontMgr ~Scalar ~Canvas ~Codec_stripes ~Codec_Dimensions ~Codec ~Stream ~skps ~Math ~RecordDraw_TextBounds",
"--resourcePath %s/resources" % BASE_DIR,
"--images %s/resources" % BASE_DIR,
],

38
src/core/SkMath.cpp
View File

@ -45,32 +45,21 @@ int SkCLZ_portable(uint32_t x) {
///////////////////////////////////////////////////////////////////////////////
#define DIVBITS_ITER(n) \
case n: \
if ((numer = (numer << 1) - denom) >= 0) \
result |= 1 << (n - 1); else numer += denom
#define DIVBITS_ITER(k) \
case k: \
if (numer*2 >= denom) { \
numer = numer*2 - denom; \
result |= 1 << (k-1); \
} else { \
numer *= 2; \
}
// NOTE: if you're thinking of editing this method, consider replacing it with
// a simple shift and divide. This legacy method predated reliable hardware division.
int32_t SkDivBits(int32_t n, int32_t d, int shift_bias) {
SkASSERT(d != 0);
if (n == 0) {
int32_t SkDivBits(int32_t numer, int32_t denom, int shift_bias) {
SkASSERT(denom != 0);
if (numer == 0) {
return 0;
}
// Make numer and denom positive, and sign hold the resulting sign
// We'll be left-shifting numer, so it's important it's a uint32_t.
// We put denom in a uint32_t just to keep things simple.
int32_t sign = SkExtractSign(n ^ d);
uint32_t numer = SkAbs32(n);
uint32_t denom = SkAbs32(d);
// It's probably a bug to use n or d below here.
// make numer and denom positive, and sign hold the resulting sign
int32_t sign = SkExtractSign(numer ^ denom);
numer = SkAbs32(numer);
denom = SkAbs32(denom);
int nbits = SkCLZ(numer) - 1;
int dbits = SkCLZ(denom) - 1;
@ -89,9 +78,10 @@ int32_t SkDivBits(int32_t n, int32_t d, int shift_bias) {
SkFixed result = 0;
// do the first one
if (numer >= denom) {
numer -= denom;
if ((numer -= denom) >= 0) {
result = 1;
} else {
numer += denom;
}
// Now fall into our switch statement if there are more bits to compute

4
tools/dm_flags.json
View File

@ -1221,7 +1221,9 @@
"_",
"image",
"_",
"interlaced3.png"
"interlaced3.png",
"--match",
"~Math"
],
"Test-Ubuntu-GCC-GCE-CPU-AVX2-x86_64-Release-Valgrind": [
"--matrix",

5
tools/dm_flags.py
View File

@ -169,6 +169,11 @@ def get_args(bot):
if 'Valgrind' in bot: # skia:3021
match.append('~Threaded')
# skia:3562
if ('TSAN' in bot or
'Test-Mac10.8-Clang-MacMini4.1-CPU-SSE4-x86_64-Release' in bot):
match.append('~Math')
if 'GalaxyS3' in bot: # skia:1699
match.append('~WritePixels')