Re: [PULL 092/126] hw/display/sm501: Add fallbacks to pixman routines

[Peter Maydell]

On Mon, 27 Feb 2023 at 14:38, Philippe Mathieu-Daudé <philmd@linaro.org> wrote:
>
> From: BALATON Zoltan <balaton@eik.bme.hu>
>
> Pixman may return false if it does not have a suitable implementation.
> Add fallbacks to handle such cases.
>
> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
> Reported-by: Rene Engel <ReneEngel80@emailn.de>
> Tested-by: Rene Engel <ReneEngel80@emailn.de>
> Message-Id: 
> <20ed9442a0146238254ccc340c0d1efa226c6356.1677445307.git.balaton@eik.bme.hu>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>

Hi; Coverity points out what may be a bug in this code
(CID 1508621):

> @@ -868,13 +891,19 @@ static void sm501_2d_operation(SM501State *s)
>              color = cpu_to_le16(color);
>          }
>
> -        if (width == 1 && height == 1) {
> -            unsigned int i = (dst_x + dst_y * dst_pitch) * bypp;
> -            stn_he_p(&s->local_mem[dst_base + i], bypp, color);
> -        } else {
> -            pixman_fill((uint32_t *)&s->local_mem[dst_base],
> -                        dst_pitch * bypp / sizeof(uint32_t),
> -                        8 * bypp, dst_x, dst_y, width, height, color);
> +        if ((width == 1 && height == 1) ||
> +            !pixman_fill((uint32_t *)&s->local_mem[dst_base],
> +                         dst_pitch * bypp / sizeof(uint32_t), 8 * bypp,
> +                         dst_x, dst_y, width, height, color)) {
> +            /* fallback when pixman failed or we don't want to call it */
> +            uint8_t *d = s->local_mem + dst_base;
> +            unsigned int x, y, i;
> +            for (y = 0; y < height; y++, i += dst_pitch * bypp) {

In this loop, the on-every-iteration expression updates i...

> +                i = (dst_x + (dst_y + y) * dst_pitch) * bypp;

...but the first statement in the loop unconditionally sets i,
so whatever value the loop iteration expression calculated is ignored.

Should the iteration expression just be deleted, or should the
statement in the loop be updating i rather than just setting it?

> +                for (x = 0; x < width; x++, i += bypp) {
> +                    stn_he_p(&d[i], bypp, color);
> +                }
> +            }

thanks
-- PMM