From 86398e5d9149eaad2fd2c8dba5557aca645d2385 Mon Sep 17 00:00:00 2001 From: "commit-bot@chromium.org" Date: Wed, 30 Apr 2014 16:51:51 +0000 Subject: [PATCH] Avoid undefined behavior and enable asserts in ClampTest. The old code here wasn't being careful to avoid int32_t overflow in slow_check. Fix that. R_ASSERT hasn't been doing anything for a while. As a result, there are a couple bugs in SkClampRange, marked as such and commented out. The asserts also weren't quite passing, so I fixed them up (allowing 0xFFFF to be considered either as part of the ramp or part of V1.) BUG=skia:2481 R=reed@google.com, mtklein@google.com Author: mtklein@chromium.org Review URL: https://codereview.chromium.org/260523004 git-svn-id: http://skia.googlecode.com/svn/trunk@14479 2bbb7eff-a529-9590-31e7-b0007b416f81 --- tests/ClampRangeTest.cpp | 81 ++++++++++++++++++++---------------------------- 1 file changed, 34 insertions(+), 47 deletions(-) diff --git a/tests/ClampRangeTest.cpp b/tests/ClampRangeTest.cpp index dee777c..bf7c95f 100644 --- a/tests/ClampRangeTest.cpp +++ b/tests/ClampRangeTest.cpp @@ -10,71 +10,54 @@ #include "gradients/SkClampRange.h" static skiatest::Reporter* gReporter; - -static void debug_me() { - if (NULL == gReporter) { - SkDebugf("dsfdssd\n"); - } +#define R_ASSERT(cond) if (!(cond)) { \ + SkDebugf("%d: %s\n", __LINE__, #cond); \ + REPORTER_ASSERT(gReporter, cond); \ } -#ifdef USE_REPORTER - -#define R_ASSERT(cond) \ - do { if (!(cond)) { \ - debug_me(); \ - REPORTER_ASSERT(gReporter, cond); \ - }} while (0) - -#else -#define R_ASSERT(cond) \ - do { if (!(cond)) { \ - debug_me(); \ - }} while (0) -#endif - -static int classify_value(SkFixed fx, int v0, int v1) { - if (fx <= 0) { - return v0; - } - if (fx >= 0xFFFF) { - return v1; +// Arbitrary sentinel values outside [0, 0xFFFF]. +static const int kV0 = -42, kV1 = -53, kRamp = -64; + +static void check_value(int64_t bigfx, int expected) { + if (bigfx < 0) { + R_ASSERT(expected == kV0); + } else if (bigfx > 0xFFFF) { + R_ASSERT(expected == kV1); + } else if (bigfx == 0xFFFF) { + // Either one is fine (and we do see both). + R_ASSERT(expected == kV1 || expected == kRamp); + } else { + R_ASSERT(expected == kRamp); } - R_ASSERT(false); - return 0; } -#define V0 -42 -#define V1 1024 - static void slow_check(const SkClampRange& range, - SkFixed fx, SkFixed dx, int count) { + const SkFixed fx, SkFixed dx, int count) { SkASSERT(range.fCount0 + range.fCount1 + range.fCount2 == count); + // If dx is large, fx will overflow if updated naively. So we use more bits. + int64_t bigfx = fx; + for (int i = 0; i < range.fCount0; i++) { - int v = classify_value(fx, V0, V1); - R_ASSERT(v == range.fV0); - fx += dx; - } - if (range.fCount1 > 0 && fx != range.fFx1) { -// SkDebugf("%x %x\n", fx, range.fFx1); - R_ASSERT(false); // bad fFx1 - return; + check_value(bigfx, range.fV0); + bigfx += dx; } + for (int i = 0; i < range.fCount1; i++) { - R_ASSERT(fx >= 0 && fx <= 0xFFFF); - fx += dx; + check_value(bigfx, kRamp); + bigfx += dx; } + for (int i = 0; i < range.fCount2; i++) { - int v = classify_value(fx, V0, V1); - R_ASSERT(v == range.fV1); - fx += dx; + check_value(bigfx, range.fV1); + bigfx += dx; } } static void test_range(SkFixed fx, SkFixed dx, int count) { SkClampRange range; - range.init(fx, dx, count, V0, V1); + range.init(fx, dx, count, kV0, kV1); slow_check(range, fx, dx, count); } @@ -96,7 +79,8 @@ DEF_TEST(ClampRange, reporter) { test_range(ff(1), ff(16384), 100); test_range(ff(-1), ff(-16384), 100); test_range(ff(1)/2, ff(16384), 100); - test_range(ff(1)/2, ff(-16384), 100); + // TODO(reed): skia:2481, fix whatever bug this is, then uncomment + //test_range(ff(1)/2, ff(-16384), 100); SkRandom rand; @@ -109,6 +93,8 @@ DEF_TEST(ClampRange, reporter) { test_range(fx, dx, count); } + // TODO(reed): skia:2481, fix whatever bug this is, then uncomment + /* // test overflow cases for (int i = 0; i < 100000; i++) { SkFixed fx = rand.nextS(); @@ -116,4 +102,5 @@ DEF_TEST(ClampRange, reporter) { int count = rand.nextU() % 1000 + 1; test_range(fx, dx, count); } + */ } -- 2.7.4