Re: [Pixman] [PATCH] pixman-combine: Fix wrong value of RB_MASK_PLUS_ONE.
On Thu, Feb 20, 2020 at 6:35 AM Shiyou Yin wrote: > Will this patch be merged? Yes, pushed. Thanks! ___ Pixman mailing list Pixman@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/pixman
Re: [Pixman] [PATCH] pixman-combine: Fix wrong value of RB_MASK_PLUS_ONE.
Will this patch be merged? >-Original Message- >From: pixman-boun...@lists.freedesktop.org >[mailto:pixman-boun...@lists.freedesktop.org] On Behalf >Of Shiyou Yin >Sent: Monday, February 10, 2020 5:50 PM >To: 'Søren Sandmann' >Cc: 'pixman' >Subject: Re: [Pixman] [PATCH] pixman-combine: Fix wrong value of >RB_MASK_PLUS_ONE. > >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:soren.sandm...@gmail.com] >>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?? >> >>The macro first detects the overflow shifting this value by 8 and >>masking to 0x00ff00ff: >> >>0001 >> >>Then it subtracts this from the RB_MASK_PLUS_ONE value: >> >>with bug: 1000 - 0001 = 0fff >>with bug fix: 0100 - 0001 = 00ff >> >>The macro then ors this value with the original sum: >> >>with bug:0fff | 01?? = 0fff >>with bug fix:00ff | 01?? = 01ff >> >>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 wrote: >>> >>> >-Original Message- >>> >From: Matt Turner [mailto:matts...@gmail.com] >>> >Sent: Sunday, February 9, 2020 7:01 AM >>> >To: Yin Shiyou >>> >Cc: pixman@lists.freedesktop.org >>> >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 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 0x1100 >>> >> +#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 >>> Pixman@lists.freedesktop.org >>> https://lists.freedesktop.org/mailman/listinfo/pixman > >___ >Pixman mailing list >Pixman@lists.freedesktop.org >https://lists.freedesktop.org/mailman/listinfo/pixman ___ Pixman mailing list Pixman@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/pixman
Re: [Pixman] [PATCH] pixman-combine: Fix wrong value of RB_MASK_PLUS_ONE.
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:soren.sandm...@gmail.com] >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?? > >The macro first detects the overflow shifting this value by 8 and >masking to 0x00ff00ff: > >0001 > >Then it subtracts this from the RB_MASK_PLUS_ONE value: > >with bug: 1000 - 0001 = 0fff >with bug fix: 0100 - 0001 = 00ff > >The macro then ors this value with the original sum: > >with bug:0fff | 01?? = 0fff >with bug fix:00ff | 01?? = 01ff > >But then it masks the result with 00ff00ff, and so both with and >without the bug, the final result is the same: > >00ff > >which is why the bug doesn't affect the calculation. > > >Søren > > >On Sun, Feb 9, 2020 at 4:26 AM Shiyou Yin wrote: >> >> >-Original Message- >> >From: Matt Turner [mailto:matts...@gmail.com] >> >Sent: Sunday, February 9, 2020 7:01 AM >> >To: Yin Shiyou >> >Cc: pixman@lists.freedesktop.org >> >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 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 0x1100 >> >> +#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 >> Pixman@lists.freedesktop.org >> https://lists.freedesktop.org/mailman/listinfo/pixman ___ Pixman mailing list Pixman@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/pixman
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?? The macro first detects the overflow shifting this value by 8 and masking to 0x00ff00ff: 0001 Then it subtracts this from the RB_MASK_PLUS_ONE value: with bug: 1000 - 0001 = 0fff with bug fix: 0100 - 0001 = 00ff The macro then ors this value with the original sum: with bug:0fff | 01?? = 0fff with bug fix:00ff | 01?? = 01ff But then it masks the result with 00ff00ff, and so both with and without the bug, the final result is the same: 00ff which is why the bug doesn't affect the calculation. Søren On Sun, Feb 9, 2020 at 4:26 AM Shiyou Yin wrote: > > >-Original Message- > >From: Matt Turner [mailto:matts...@gmail.com] > >Sent: Sunday, February 9, 2020 7:01 AM > >To: Yin Shiyou > >Cc: pixman@lists.freedesktop.org > >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 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 0x1100 > >> +#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 > Pixman@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/pixman ___ Pixman mailing list Pixman@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/pixman
Re: [Pixman] [PATCH] pixman-combine: Fix wrong value of RB_MASK_PLUS_ONE.
>-Original Message- >From: Matt Turner [mailto:matts...@gmail.com] >Sent: Sunday, February 9, 2020 7:01 AM >To: Yin Shiyou >Cc: pixman@lists.freedesktop.org >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 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 0x1100 >> +#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 Pixman@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/pixman
Re: [Pixman] [PATCH] pixman-combine: Fix wrong value of RB_MASK_PLUS_ONE.
On Mon, Feb 3, 2020 at 1:56 AM Yin Shiyou 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 0x1100 > +#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? ___ Pixman mailing list Pixman@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/pixman
