That's great, the analysis is very detailed. Macro RB_MASK_PLUS_ONE is mainly used to saturate the 16-bits data to 0xff. In function UN8_rb_ADD_UN8_rb, it can’t affect the result in the end.
>-----Original Message----- >From: Søren Sandmann [mailto:[email protected]] >Sent: Sunday, February 9, 2020 11:44 PM >To: Shiyou Yin >Cc: Matt Turner; pixman >Subject: Re: [Pixman] [PATCH] pixman-combine: Fix wrong value of >RB_MASK_PLUS_ONE. > >Hi, > >The reason this bug doesn't show up in the test suite is that it >doesn't have an effect on the output. The purpose of the >UN8_rb_ADD_UN8_rb macro is to add two numbers like these: > > 00aa00bb > 00cc00dd > >together, but to saturate aa+cc and bb+dd to 0xff instead of allowing >them to overflow. Let's pretend that bb and dd are both zero and focus >on aa+cc. If that addition does in fact overflow (ie., if aa+cc > >0xff), the result looks like this: > > 01??0000 > >The macro first detects the overflow shifting this value by 8 and >masking to 0x00ff00ff: > > 00010000 > >Then it subtracts this from the RB_MASK_PLUS_ONE value: > > with bug: 10000000 - 00010000 = 0fff0000 > with bug fix: 01000000 - 00010000 = 00ff0000 > >The macro then ors this value with the original sum: > > with bug: 0fff0000 | 01??0000 = 0fff0000 > with bug fix: 00ff0000 | 01??0000 = 01ff0000 > >But then it masks the result with 00ff00ff, and so both with and >without the bug, the final result is the same: > > 00ff0000 > >which is why the bug doesn't affect the calculation. > > >Søren > > >On Sun, Feb 9, 2020 at 4:26 AM Shiyou Yin <[email protected]> wrote: >> >> >-----Original Message----- >> >From: Matt Turner [mailto:[email protected]] >> >Sent: Sunday, February 9, 2020 7:01 AM >> >To: Yin Shiyou >> >Cc: [email protected] >> >Subject: Re: [Pixman] [PATCH] pixman-combine: Fix wrong value of >> >RB_MASK_PLUS_ONE. >> > >> >On Mon, Feb 3, 2020 at 1:56 AM Yin Shiyou <[email protected]> wrote: >> >> >> >> --- >> >> pixman/pixman-combine32.h | 2 +- >> >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> >> >> diff --git a/pixman/pixman-combine32.h b/pixman/pixman-combine32.h >> >> index cdd56a6..59bb247 100644 >> >> --- a/pixman/pixman-combine32.h >> >> +++ b/pixman/pixman-combine32.h >> >> @@ -12,7 +12,7 @@ >> >> #define RB_MASK 0xff00ff >> >> #define AG_MASK 0xff00ff00 >> >> #define RB_ONE_HALF 0x800080 >> >> -#define RB_MASK_PLUS_ONE 0x10000100 >> >> +#define RB_MASK_PLUS_ONE 0x1000100 >> > >> > >> >Thanks. The patch looks correct, but obviously nothing in the test >> >suite is failing. How did you discover this? Does this patch fix >> >something for you? >> >> I just found this mistake while analyzing macro 'UN8_rb_ADD_UN8_rb' >> and functions called this macro. I can't verify this error with existing >> test suite, it all passed no matter Fix it or not. >> >> _______________________________________________ >> Pixman mailing list >> [email protected] >> https://lists.freedesktop.org/mailman/listinfo/pixman _______________________________________________ Pixman mailing list [email protected] https://lists.freedesktop.org/mailman/listinfo/pixman
