From e47f1a7c741c0ea209f7a41358557e66f27ef6a3 Mon Sep 17 00:00:00 2001 From: mtklein Date: Fri, 20 Nov 2015 13:58:18 -0800 Subject: [PATCH] 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. Review URL: https://codereview.chromium.org/1455163004 --- BUILD.public | 2 +- include/core/SkFixed.h | 9 ++++++++- tools/dm_flags.json | 4 +--- tools/dm_flags.py | 5 ----- 4 files changed, 10 insertions(+), 10 deletions(-) diff --git a/BUILD.public b/BUILD.public index a029a9fa02..e76d879bce 100644 --- 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 ~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/include/core/SkFixed.h b/include/core/SkFixed.h index 48284effda..fefa718d0f 100644 --- a/include/core/SkFixed.h +++ b/include/core/SkFixed.h @@ -82,7 +82,14 @@ typedef int32_t SkFixed; #define SkFixedAbs(x) SkAbs32(x) #define SkFixedAve(a, b) (((a) + (b)) >> 1) -#define SkFixedDiv(numer, denom) SkDivBits(numer, denom, 16) +// Blink layout tests are baselined to Clang optimizing through undefined behavior in SkDivBits. +#if defined(SK_SUPPORT_LEGACY_DIVBITS_UB) + #define SkFixedDiv(numer, denom) SkDivBits(numer, denom, 16) +#else + // TODO(reed): this clamp shouldn't be needed. Use SkToS32(). + #define SkFixedDiv(numer, denom) \ + SkTPin(((int64_t)numer << 16) / denom, SK_MinS32, SK_MaxS32) +#endif ////////////////////////////////////////////////////////////////////////////////////////////////////// // Now look for ASM overrides for our portable versions (should consider putting this in its own file) diff --git a/tools/dm_flags.json b/tools/dm_flags.json index be6bc3d747..3c557865dc 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 7c901891ca..5341b221da 100755 --- a/tools/dm_flags.py +++ b/tools/dm_flags.py @@ -169,11 +169,6 @@ 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')