[Spice-devel] [spice-common PATCH v1 9/12] clang-noise

[Victor Toso]

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