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

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

diff --git a/BUILD.public b/BUILD.public
index e76d879..a029a9f 100644 (file)
--- a/BUILD.public
+++ b/BUILD.public
@@ -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,
     ],

diff --git a/src/core/SkMath.cpp b/src/core/SkMath.cpp
index c5d468d..af93d7e 100644 (file)
--- a/src/core/SkMath.cpp
+++ b/src/core/SkMath.cpp
@@ -45,32 +45,21 @@ int SkCLZ_portable(uint32_t x) {
 
 ///////////////////////////////////////////////////////////////////////////////
 
-
-#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) {
+#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) {
         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

diff --git a/tools/dm_flags.json b/tools/dm_flags.json
index 3c55786..be6bc3d 100644 (file)
--- a/tools/dm_flags.json
+++ b/tools/dm_flags.json
@@ -1221,7 +1221,9 @@
     "_", 
     "image", 
     "_", 
-    "interlaced3.png"
+    "interlaced3.png", 
+    "--match", 
+    "~Math"
   ], 
   "Test-Ubuntu-GCC-GCE-CPU-AVX2-x86_64-Release-Valgrind": [
     "--matrix", 

diff --git a/tools/dm_flags.py b/tools/dm_flags.py
index 5341b22..7c90189 100755 (executable)
--- a/tools/dm_flags.py
+++ b/tools/dm_flags.py
@@ -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')