[Spice-devel] [PATCH v2 2/2] Avoid clang warnings on casts with stricter alignment requirements
Frediano Ziglio
fziglio at redhat.com
Thu May 18 10:48:32 UTC 2017
>
> From: Christophe de Dinechin <dinechin at redhat.com>
>
> For example, something like this:
> uint8_t *p8;
> uint32_t *p32 = (uint32_t *) p8;
>
> generates a warning like this:
> spice-channel.c:1350:10: error: cast from 'uint8_t *' (aka 'unsigned char
> *') to
> 'uint32_t *' (aka 'unsigned int *') increases required alignment from 1
> to
> 4 [-Werror,-Wcast-align]
>
> The warning indicates that we end up with a pointer to data that
> should be 4-byte aligned, but its value may be misaligned. On x86,
> this does not make much of a difference, except a relatively minor
> performance penalty. However, on platforms such as ARM, misaligned
> accesses are emulated by the kernel, and support for them is optional.
> So we may end up with a fault.
>
> The intent of the fix here is to make it easy to identify and rework
> places where actual mis-alignment occurs. Wherever casts raise the warning,
> they are replaced with a macro:
>
> - SPICE_ALIGNED_CAST(type, value) casts value to type, and indicates that
> we believe the resulting pointer is aligned. If it is not, a warning will
> be issued.
>
> - SPICE_UNALIGNED_CAST(type, value) casts value to type, and indicates that
> we believe the resulting pointer is not always aligned
>
> Any code using SPICE_UNALIGNED_CAST may need to be revisited in order
> to improve performance, e.g. by using memcpy.
>
> There are normally no warnings for SPICE_UNALIGNED_CAST, but it is possible
> to emit debug messages for mis-alignment in SPICE_UNALIGNED_CAST.
> Set environment variable SPICE_DEBUG_ALIGNMENT to activate this check.
>
> All run-time checks are disabled when the NDEBUG macro is set
> (like for the assert() macro from <assert.h>).
>
> Signed-off-by: Christophe de Dinechin <dinechin at redhat.com>
> ---
> common/canvas_base.c | 14 +++++++++-----
> common/mem.c | 24 ++++++++++++++++++++++++
> common/mem.h | 39 +++++++++++++++++++++++++++++++++++++--
> common/sw_canvas.c | 10 ++++++----
> 4 files changed, 76 insertions(+), 11 deletions(-)
>
> diff --git a/common/canvas_base.c b/common/canvas_base.c
> index 8f653e0..5815e9c 100644
> --- a/common/canvas_base.c
> +++ b/common/canvas_base.c
> @@ -389,7 +389,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,
> + SPICE_UNALIGNED_CAST(uint32_t
> *,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");
> @@ -523,8 +523,8 @@ static void canvas_fix_alignment(uint8_t *bits,
> uint8_t *dest = bits;
> for (row = height - 1; row > 0; --row) {
> uint32_t *dest_aligned, *dest_misaligned;
> - dest_aligned = (uint32_t *)(dest + stride_pixman*row);
> - dest_misaligned = (uint32_t*)(dest + stride_encoded*row);
> + dest_aligned = SPICE_ALIGNED_CAST(uint32_t *,dest +
> stride_pixman*row);
> + dest_misaligned = SPICE_UNALIGNED_CAST(uint32_t*,dest +
> stride_encoded*row);
> memmove(dest_aligned, dest_misaligned, stride_encoded);
> }
> }
Here I would just declare dest_aligned and dest_misaligned as void * or uint8_t *,
memmove cope with any alignment.
> @@ -1889,7 +1889,8 @@ 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 = SPICE_ALIGNED_CAST(uint32_t *,
> +
> quic_data->chunks->chunk[quic_data->current_chunk].data);
> return quic_data->chunks->chunk[quic_data->current_chunk].len >> 2;
> }
>
> @@ -1945,6 +1946,7 @@ static void canvas_mask_pixman(CanvasBase *canvas,
> int needs_invert;
> pixman_region32_t mask_region;
> uint32_t *mask_data;
> + uint8_t *mask_data_src;
> int mask_x, mask_y;
> int mask_width, mask_height, mask_stride;
> pixman_box32_t extents;
> @@ -2006,7 +2008,9 @@ 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_src = (uint8_t *)mask_data + mask_stride * extents.y1 +
> extents.x1 / 32;
> + mask_data = SPICE_UNALIGNED_CAST(uint32_t *, mask_data_src);
> +
> mask_x -= extents.x1;
> mask_y -= extents.y1;
> mask_width = extents.x2 - extents.x1;
> diff --git a/common/mem.c b/common/mem.c
> index e430b5d..5b15bb6 100644
> --- a/common/mem.c
> +++ b/common/mem.c
> @@ -294,3 +294,27 @@ size_t spice_buffer_remove(SpiceBuffer *buffer, size_t
> len)
> buffer->offset -= len;
> return len;
> }
> +
> +void spice_alignment_warning(const char *loc, void *p, unsigned sz)
> +{
> + static const char *last_loc = NULL;
> + if (loc != last_loc) {
> + last_loc = loc;
> + spice_log(SPICE_LOG_DOMAIN, SPICE_LOG_LEVEL_WARNING, loc,
> __FUNCTION__,
> + "Misaligned access at %p, alignment %u", p, sz);
> + }
> +}
> +
> +void spice_alignment_debug(const char *loc, void *p, unsigned sz)
> +{
> + static const char *last_loc = NULL;
> + static int debug_alignment = -1;
> + if (debug_alignment < 0) {
> + debug_alignment = getenv("SPICE_DEBUG_ALIGNMENT") != NULL;
> + }
> + if (debug_alignment && loc != last_loc) {
> + last_loc = loc;
> + spice_log(SPICE_LOG_DOMAIN, SPICE_LOG_LEVEL_DEBUG, loc,
> __FUNCTION__,
> + "Expected misaligned access at %p, alignment %u", p, sz);
> + }
> +}
> diff --git a/common/mem.h b/common/mem.h
> index 857e8b0..f8b7e7d 100644
> --- a/common/mem.h
> +++ b/common/mem.h
> @@ -19,6 +19,7 @@
> #ifndef _H_MEM
> #define _H_MEM
>
> +#include "log.h"
> #include <stdlib.h>
> #include <spice/macros.h>
>
> @@ -116,7 +117,7 @@ size_t spice_strnlen(const char *str, size_t max_len);
> __p; \
> }))
> # define _SPICE_RENEW(struct_type, mem, n_structs, func) \
> - (struct_type *) (__extension__ ({ \
> + (struct_type *) (__extension__ ({ \
> size_t __n = (size_t) (n_structs); \
> size_t __s = sizeof (struct_type); \
> void *__p = (void *) (mem); \
spurious space change
> @@ -132,7 +133,6 @@ size_t spice_strnlen(const char *str, size_t max_len);
> #else
>
> /* Unoptimized version: always call the _n() function. */
> -
> #define _SPICE_NEW(struct_type, n_structs, func) \
> ((struct_type *) spice_##func##_n ((n_structs), sizeof (struct_type)))
> #define _SPICE_RENEW(struct_type, mem, n_structs, func) \
spurious change
> @@ -140,6 +140,41 @@ size_t spice_strnlen(const char *str, size_t max_len);
>
> #endif
>
> +/* Cast to a type with stricter alignment constraints (to build with clang)
> */
> +extern void spice_alignment_warning(const char *loc, void *p, unsigned sz);
> +extern void spice_alignment_debug(const char *loc, void *p, unsigned sz);
> +
> +static inline void *spice_alignment_check(const char *loc,
> + void *ptr, unsigned sz)
> +{
> +#ifndef NDEBUG
I like the debug idea but I think that currently NDEBUG is never defined.
> + if (G_UNLIKELY(((uintptr_t) ptr & (sz-1U)) != 0))
> + spice_alignment_warning(loc, ptr, sz);
> +#endif // NDEBUG
> + return ptr;
> +
> +}
> +
> +static inline void *spice_alignment_weak_check(const char *loc,
> + void *ptr, unsigned sz)
> +{
> +#ifndef NDEBUG
> + if (G_UNLIKELY(((uintptr_t) ptr & (sz-1U)) != 0))
I think should be == 0 here.
> + spice_alignment_debug(loc, ptr, sz);
> +#endif // NDEBUG
> + return ptr;
> +
> +}
> +#define SPICE_ALIGNED_CAST(type, value) \
> + ((type)spice_alignment_check(SPICE_STRLOC, \
> + (void *)(value), \
> + __alignof(*((type)0))))
> +
> +#define SPICE_UNALIGNED_CAST(type, value) \
> + ((type)spice_alignment_weak_check(SPICE_STRLOC, \
> + (void *)(value), \
> + __alignof(*((type)0))))
> +
really I don't understand the reason for this check.
> #define spice_new(struct_type, n_structs) _SPICE_NEW(struct_type, n_structs,
> malloc)
> #define spice_new0(struct_type, n_structs) _SPICE_NEW(struct_type,
> n_structs, malloc0)
> #define spice_renew(struct_type, mem, n_structs) _SPICE_RENEW(struct_type,
> mem, n_structs, realloc)
> diff --git a/common/sw_canvas.c b/common/sw_canvas.c
> index 531d608..e727090 100644
> --- a/common/sw_canvas.c
> +++ b/common/sw_canvas.c
> @@ -355,8 +355,8 @@ static void clear_dest_alpha(pixman_image_t *dest,
> }
>
> stride = pixman_image_get_stride(dest);
> - data = (uint32_t *) (
> - (uint8_t *)pixman_image_get_data(dest) + y * stride + 4 * x);
> + data = SPICE_ALIGNED_CAST(uint32_t *,
> + (uint8_t *)pixman_image_get_data(dest) + y *
> stride + 4 * x);
>
> if ((*data & 0xff000000U) == 0xff000000U) {
> spice_pixman_fill_rect_rop(dest,
> @@ -1010,7 +1010,7 @@ static void canvas_put_image(SpiceCanvas *spice_canvas,
> src = pixman_image_create_bits(PIXMAN_x8r8g8b8,
> src_width,
> src_height,
> - (uint32_t*)src_data,
> + SPICE_ALIGNED_CAST(uint32_t*,src_data),
> src_stride);
>
>
> @@ -1265,7 +1265,9 @@ 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 *) data,
> stride);
> + width, height,
> + SPICE_ALIGNED_CAST(uint32_t *,data),
> + stride);
>
> return canvas_create_common(image, format,
> bits_cache,
Frediano
More information about the Spice-devel
mailing list