Hey,
Thanks for taking time to review this.
On Thu, Aug 06, 2015 at 09:46:29AM -0400, Frediano Ziglio wrote:
> >
> > ---
> > common/canvas_base.c | 12 ++++++------
> > common/sw_canvas.c | 6 +++---
> > 2 files changed, 9 insertions(+), 9 deletions(-)
> >
> > diff --git a/common/canvas_base.c b/common/canvas_base.c
> > index 6f48340..4351334 100644
> > --- a/common/canvas_base.c
> > +++ b/common/canvas_base.c
> > @@ -396,7 +396,7 @@ static pixman_image_t *canvas_get_quic(CanvasBase
> > *canvas, SpiceImage *image,
> > quic_data->current_chunk = 0;
> >
> > if (quic_decode_begin(quic_data->quic,
> > - (uint32_t *)image->u.quic.data->chunk[0].data,
> > + (uint32_t *)(void
> > *)image->u.quic.data->chunk[0].data,
> > image->u.quic.data->chunk[0].len >> 2,
> > &type, &width, &height) == QUIC_ERROR) {
> > spice_warning("quic decode begin failed");
>
> This is fine
>
> > @@ -583,7 +583,7 @@ static pixman_image_t *canvas_get_lz4(CanvasBase *canvas,
> > SpiceImage *image)
> >
> > do {
> > // Read next compressed block
> > - enc_size = ntohl(*((uint32_t *)data));
> > + enc_size = ntohl(*((uint32_t *)(void *)data));
> > data += 4;
> > dec_size = LZ4_decompress_safe_continue(stream, (const char *) data,
> > (char *) dest, enc_size,
> > available);
>
> Here data can be not aligned.
Not sure about this one, this data is also SpiceChunk->data so I'd
expect that the memory is aligned.
>
> > @@ -607,8 +607,8 @@ static pixman_image_t *canvas_get_lz4(CanvasBase *canvas,
> > SpiceImage *image)
> > }
> > for (row = height - 1; row > 0; --row) {
> > uint32_t *dest_aligned, *dest_misaligned;
> > - dest_aligned = (uint32_t *)(dest + stride_abs*row);
> > - dest_misaligned = (uint32_t*)(dest + stride_encoded*row);
> > + dest_aligned = (uint32_t *)(void *)(dest + stride_abs*row);
> > + dest_misaligned = (uint32_t*)(void *)(dest +
> > stride_encoded*row);
> > memmove(dest_aligned, dest_misaligned, stride_encoded);
> > }
> > }
>
> Here just declare dest_aligned and dest_misaligned as void * instead
>
Sure
> > @@ -1919,7 +1919,7 @@ static int quic_usr_more_space(QuicUsrContext *usr,
> > uint32_t **io_ptr, int rows_
> > }
> > quic_data->current_chunk++;
> >
> > - *io_ptr = (uint32_t
> > *)quic_data->chunks->chunk[quic_data->current_chunk].data;
> > + *io_ptr = (uint32_t *)(void
> > *)quic_data->chunks->chunk[quic_data->current_chunk].data;
> > return quic_data->chunks->chunk[quic_data->current_chunk].len >> 2;
> > }
> >
>
> fine
>
> > @@ -2066,7 +2066,7 @@ static void canvas_mask_pixman(CanvasBase *canvas,
> > /* round down X to even 32 pixels (i.e. uint32_t) */
> > extents.x1 = extents.x1 & ~(0x1f);
> >
> > - mask_data = (uint32_t *)((uint8_t *)mask_data + mask_stride * extents.y1
> > + extents.x1 / 32);
> > + mask_data = (uint32_t *)(void *)((uint8_t *)mask_data + mask_stride *
> > extents.y1 + extents.x1 / 32);
> > mask_x -= extents.x1;
> > mask_y -= extents.y1;
> > mask_width = extents.x2 - extents.x1;
>
> This can produce not aligned pointer.
>
> > diff --git a/common/sw_canvas.c b/common/sw_canvas.c
> > index 7d67ca5..bb353c1 100644
> > --- a/common/sw_canvas.c
> > +++ b/common/sw_canvas.c
> > @@ -357,7 +357,7 @@ static void clear_dest_alpha(pixman_image_t *dest,
> > }
> >
> > stride = pixman_image_get_stride(dest);
> > - data = (uint32_t *) (
> > + data = (uint32_t *)(void *) (
> > (uint8_t *)pixman_image_get_data(dest) + y * stride + 4 * x);
> >
> > if ((*data & 0xff000000U) == 0xff000000U) {
>
> Here is fine, the compiler cannot know that stride is a multiple of 4.
>
> > @@ -1012,7 +1012,7 @@ static void canvas_put_image(SpiceCanvas *spice_canvas,
> > src = pixman_image_create_bits(PIXMAN_x8r8g8b8,
> > src_width,
> > src_height,
> > - (uint32_t*)src_data,
> > + (uint32_t*)(void *)src_data,
> > src_stride);
> >
> >
>
> Should be fine but I would change function (canvas_put_image) pointer to uint32_t instead.
That would mean changing the SpiceCanvas->put_image and the respective
implementations on gl_canvas and gdi_canvas.
>
> > @@ -1276,7 +1276,7 @@ SpiceCanvas *canvas_create_for_data(int width, int
> > height, uint32_t format,
> > pixman_image_t *image;
> >
> > image = pixman_image_create_bits(spice_surface_format_to_pixman
> > (format),
> > - width, height, (uint32_t *)data,
> > stride);
> > + width, height, (uint32_t *)(void
> > *)data, stride);
> >
> > return canvas_create_common(image, format
> > , bits_cache
>
> Not sure if this is aligned.
>
> > --
> > 2.4.3
> >
>
> You could define some define which do the conversion, hide the warning but make explicit the unaligned issue, something like
>
> #define UNALIGNED_CAST(type, ptr) ((type)(void*)(ptr))
>
> so in the future if we need to support such architectures we can just use a grep.
>
> Frediano
Sure, I'm using two defines, SPICE_ALIGNED_CAST for false-positive and
SPICE_UNALIGNED_CAST for the ones that could lead to problems.
toso
_______________________________________________
Spice-devel mailing list
Spice-devel@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/spice-devel