color_match in dlls/d3d8/tests/visual.c may not do what one would think

[Gerald Pfeifer]

Hi Henri, hi everyone,

earlier this year via

   commit 2f6fbdec8caee311e2fb85b2ddc9fbd8a9afaea5
   Author: H. Verbeet <hverbeet(a)gmail.com>
   Date:   Sat May 24 10:33:36 2008 +0200

       d3d8: Test our texop implementation.

we go the following function in dlls/d3d8/tests/visual.c:

   static BOOL color_match(D3DCOLOR c1, D3DCOLOR c2, BYTE max_diff)
   {
       if (abs((c1 & 0xff) - (c2 & 0xff)) > max_diff) return FALSE;
       c1 >>= 8; c2 >>= 8;
       if (abs((c1 & 0xff) - (c2 & 0xff)) > max_diff) return FALSE;
       c1 >>= 8; c2 >>= 8;
       if (abs((c1 & 0xff) - (c2 & 0xff)) > max_diff) return FALSE;
       c1 >>= 8; c2 >>= 8;
       if (abs((c1 & 0xff) - (c2 & 0xff)) > max_diff) return FALSE;
       return TRUE;
   }

Intuitively it's clear what this does, however there's a little
challenge: c1 and c2 are unsigned integers (D3DCOLOR being DWORD
being unsigned long), and so are c1 & 0xff and c2 & 0xff.

Substraction of two unsigned integer types is done in unsigned
math, so we may be seeing wrapping, but never a negative number
as a result -- and abs() is a noop.

Gerald

[Ken Thomases]

On Nov 4, 2018, at 3:56 PM, Gerald Pfeifer <gerald(a)pfeifer.com> wrote:
> Hi Henri, hi everyone,
>
> earlier this year via

Uh, that's earlier this century, but not this year or even this decade. ;)
> commit 2f6fbdec8caee311e2fb85b2ddc9fbd8a9afaea5
>   Author: H. Verbeet <hverbeet(a)gmail.com>
>   Date:   Sat May 24 10:33:36 2008 +0200
>
> d3d8: Test our texop implementation.

The code in question is still the same, though.

-Ken

[Henri Verbeet]

On Mon, 5 Nov 2018 at 01:26, Gerald Pfeifer <gerald(a)pfeifer.com> wrote:
> Hi Henri, hi everyone,
>
> earlier this year via
>
> commit 2f6fbdec8caee311e2fb85b2ddc9fbd8a9afaea5
>    Author: H. Verbeet <hverbeet(a)gmail.com>
>    Date:   Sat May 24 10:33:36 2008 +0200
>
> d3d8: Test our texop implementation.
>
> we go the following function in dlls/d3d8/tests/visual.c:
>
> static BOOL color_match(D3DCOLOR c1, D3DCOLOR c2, BYTE max_diff)
>    {
>        if (abs((c1 & 0xff) - (c2 & 0xff)) > max_diff) return FALSE;
>        c1 >>= 8; c2 >>= 8;
>        if (abs((c1 & 0xff) - (c2 & 0xff)) > max_diff) return FALSE;
>        c1 >>= 8; c2 >>= 8;
>        if (abs((c1 & 0xff) - (c2 & 0xff)) > max_diff) return FALSE;
>        c1 >>= 8; c2 >>= 8;
>        if (abs((c1 & 0xff) - (c2 & 0xff)) > max_diff) return FALSE;
>        return TRUE;
>    }
>
> Intuitively it's clear what this does, however there's a little
> challenge: c1 and c2 are unsigned integers (D3DCOLOR being DWORD
> being unsigned long), and so are c1 & 0xff and c2 & 0xff.
>
> Substraction of two unsigned integer types is done in unsigned
> math, so we may be seeing wrapping, but never a negative number
> as a result -- and abs() is a noop.

As Ken pointed out, the code has been in the tree a little longer than
this year. As for the issue you point out, it's true that the function
assumes sizeof(DWORD) == sizeof(int), and a two's complement system.
I.e., in two's complement 0u - 1u == ~0u == -1, etc. I don't think
those are unreasonable assumptions. Perhaps the function has room for
improvement, but I'd prefer to not simply sprinkle some casts on top.

[Gerald Pfeifer]

On Tue, 6 Nov 2018, Henri Verbeet wrote:
> > static BOOL color_match(D3DCOLOR c1, D3DCOLOR c2, BYTE max_diff)
> >    {
> >        if (abs((c1 & 0xff) - (c2 & 0xff)) > max_diff) return FALSE;
> > Substraction of two unsigned integer types is done in unsigned
> > math, so we may be seeing wrapping, but never a negative number
> > as a result -- and abs() is a noop.
> > As Ken pointed out, the code has been in the tree a little longer than
> > this year. As for the issue you point out, it's true that the function
> > assumes sizeof(DWORD) == sizeof(int), and a two's complement system.
> > I.e., in two's complement 0u - 1u == ~0u == -1, etc. I don't think
> > those are unreasonable assumptions. Perhaps the function has room for
> > improvement, but I'd prefer to not simply sprinkle some casts on top.

Thanks for looking into this and the explanation.  Since we are in 
unsigned land, the abs() really is a noop, so at a minimum should 
be removed?  I'll submit a patch in a minute.

Gerald

[Henri Verbeet]

On Wed, 7 Nov 2018 at 03:40, Gerald Pfeifer <gerald(a)pfeifer.com> wrote:
> Thanks for looking into this and the explanation.  Since we are in
> unsigned land, the abs() really is a noop, so at a minimum should
> be removed?  I'll submit a patch in a minute.

No, there's a type conversion when passing the unsigned value to
abs(). I.e., abs(~0u) == 1.