rapp-dev
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Rapp-dev] Have an extra look at commit e4a5, fixing a bug exposed b


From: Johan Almbladh
Subject: Re: [Rapp-dev] Have an extra look at commit e4a5, fixing a bug exposed by fixing MAX
Date: Wed, 15 Dec 2010 00:39:04 +0100

Good catch! The function sets up a a buffer for a 1x1 image with the necessary padding to run the filter and exhaustively test all possible combinations of source image values with variations in the least significant bits. I think that the original code tried to allocate exacty the space we need but somehow got it wrong. The original line where the src buffer is set looks suspicious. Your solution seem to fix that.

/Johan

On Sun, Dec 12, 2010 at 3:50 PM, Hans-Peter Nilsson <address@hidden> wrote:
While looking into fixing and adjusting the morph bin overlap
checks mentioned previously, I noticed that the MAX macro in the
test-suite was... MIN.  Thankfully, there was only one use of
MAX that mattered.  Unthankfully, that was buggy code, which
failed when MAX was fixed.  I think I've got it, and I tried to
improve the comments, ever so slightly.  Please double-check.

If you think you have improvements, please run it through
valgrind first ("valgrind test/rapptest", that is), and try all
combinations with/without
--enable-backends=none 'CC=gcc -m32' --enable-debug
(The 'CC=gcc -m32' if you're on x86_64, to shrink rapp_alignment.)
Using --enable-static --disable-shared will probably help your
gdb session.  And please be explicit in comments what assumptions
the code has regarding filter size and padding need.  Dumbing down
your coding helps others debug it.

Thanks.

PS. There'll hopefully be a release today.

>From e4a530fa390e8f99b0f8fe813b5b3800b7669797 Mon Sep 17 00:00:00 2001
From: Hans-Peter Nilsson <address@hidden>
Date: Sun, 12 Dec 2010 07:27:29 +0100
Subject: [PATCH] testsuite: fix local macro MAX and exposed bugs falling out from fixing it.

       * rapp_test_filter.c (rapp_test_prec_driver): Correct buffer
       calculations.  Assert that buffers are at least the used size.
       * rapp_test_util.h (MAX): Correct.
---
 test/rapp_test_filter.c |   19 +++++++++++++++----
 test/rapp_test_util.h   |    2 +-
 2 files changed, 16 insertions(+), 5 deletions(-)

diff --git a/test/rapp_test_filter.c b/test/rapp_test_filter.c
index b18347a..b22d0ce 100644
--- a/test/rapp_test_filter.c
+++ b/test/rapp_test_filter.c
@@ -225,17 +225,28 @@ rapp_test_prec_driver(int (*func)(), void (*ref)(),
 {
    float    val = 0.0f;                           /* Reference value      */
    int      dim = 3*rapp_alignment;               /* Source buffer dim    */
-    size_t alloc_size = (height + 1) * dim;
+
+    /**
+     *  The tested filters (or to be precise, the reference functions) don't
+     *  read more than half the MAX(width, height) above the image and no
+     *  more than half the MAX(width, height) horizontally.
+     */
+    int   maxref = MAX(width, height) / 2;
+    size_t alloc_size = (height + maxref) * dim + rapp_align(maxref);
    uint8_t *sbuf = rapp_malloc(alloc_size, 0);
    uint8_t *dst = rapp_malloc(alloc_size, 0);     /* Destination buffer   */
    uint8_t *src = ""                               /* Source buffer, padded */
-         &sbuf[dim * (1 + MAX(height, width) / 2) + rapp_alignment];
-    uint8_t *pad =                                 /* Source padding start */
-        &src[-(width / 2 + dim * (height / 2))];
+        &sbuf[dim * maxref + rapp_align(maxref)];
+    uint8_t *pad = &src[-(maxref + dim * maxref)]; /* Source padding start */
    long     size = 1 << (bits*width*height);      /* Num of combinations  */
    long     code;
    int ret;

+    /* Check that src is within the allocated sbuf size. */
+    assert(src < sbuf + alloc_size);
+    /* Check that the max written index of pad[] is within sbuf. */
+    assert(sbuf <= pad &&
+           pad + (height - 1) * dim + width - 1 < sbuf + alloc_size);
    assert((unsigned)bits*width*height < 8*sizeof size);
    assert((unsigned)width < rapp_alignment * 2);
    memset(sbuf, 0, alloc_size);
diff --git a/test/rapp_test_util.h b/test/rapp_test_util.h
index e152f27..97399e2 100644
--- a/test/rapp_test_util.h
+++ b/test/rapp_test_util.h
@@ -62,7 +62,7 @@ extern "C" {
 *  Standard MAX().
 */
 #undef  MAX
-#define MAX(a, b) ((a) < (b) ? (a) : (b))
+#define MAX(a, b) ((a) > (b) ? (a) : (b))

 /**
 *  Standard CLAMP().
--
1.7.2.3




reply via email to

[Prev in Thread] Current Thread [Next in Thread]