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
Review URL: https://codereview.chromium.org/1455163004
diff --git a/BUILD.public b/BUILD.public
index 12c1a5d..5cd9e45 100644
--- a/BUILD.public
+++ b/BUILD.public
@@ -443,7 +443,7 @@
"--nogpu",
"--verbose",
# TODO(mtklein): maybe investigate why these fail?
- "--match ~FontMgr ~Scalar ~Canvas ~Codec_stripes ~Codec_Dimensions ~Codec ~Stream ~skps ~Math ~RecordDraw_TextBounds",
+ "--match ~FontMgr ~Scalar ~Canvas ~Codec_stripes ~Codec_Dimensions ~Codec ~Stream ~skps ~RecordDraw_TextBounds",
"--resourcePath %s/resources" % BASE_DIR,
"--images %s/resources" % BASE_DIR,
],
diff --git a/src/core/SkMath.cpp b/src/core/SkMath.cpp
index af93d7e..c5d468d 100644
--- a/src/core/SkMath.cpp
+++ b/src/core/SkMath.cpp
@@ -45,21 +45,32 @@
///////////////////////////////////////////////////////////////////////////////
-#define DIVBITS_ITER(n) \
- case n: \
- if ((numer = (numer << 1) - denom) >= 0) \
- result |= 1 << (n - 1); else numer += denom
-int32_t SkDivBits(int32_t numer, int32_t denom, int shift_bias) {
- SkASSERT(denom != 0);
- if (numer == 0) {
+#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) {
return 0;
}
- // make numer and denom positive, and sign hold the resulting sign
- int32_t sign = SkExtractSign(numer ^ denom);
- numer = SkAbs32(numer);
- denom = SkAbs32(denom);
+ // 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.
int nbits = SkCLZ(numer) - 1;
int dbits = SkCLZ(denom) - 1;
@@ -78,10 +89,9 @@
SkFixed result = 0;
// do the first one
- if ((numer -= denom) >= 0) {
+ if (numer >= denom) {
+ numer -= denom;
result = 1;
- } else {
- numer += denom;
}
// Now fall into our switch statement if there are more bits to compute
diff --git a/tools/dm_flags.json b/tools/dm_flags.json
index be6bc3d..3c55786 100644
--- a/tools/dm_flags.json
+++ b/tools/dm_flags.json
@@ -1221,9 +1221,7 @@
"_",
"image",
"_",
- "interlaced3.png",
- "--match",
- "~Math"
+ "interlaced3.png"
],
"Test-Ubuntu-GCC-GCE-CPU-AVX2-x86_64-Release-Valgrind": [
"--matrix",
diff --git a/tools/dm_flags.py b/tools/dm_flags.py
index 7c90189..5341b22 100755
--- a/tools/dm_flags.py
+++ b/tools/dm_flags.py
@@ -169,11 +169,6 @@
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')
