> On 18 May 2017, at 12:48, Frediano Ziglio <fziglio@xxxxxxxxxx> wrote: > >> >> From: Christophe de Dinechin <dinechin@xxxxxxxxxx> >> >> 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@xxxxxxxxxx> >> --- >> 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. I was assuming the original code was intentionally indicating that the target was 32-bit words. I think void * loses that information. > >> @@ -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 This is realigning the \ with the others, which are all aligned but this one. I’ll put it with the other stylistic changes in patch 1/2. > >> @@ -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 Removing an empty line after comment which did not exist for other comments in the files of this directory. I’ll put it in patch 1/2. > >> @@ -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. I read that package maintainers often build the released version by doing ./configure CFLAGS=-DNDEBUG, and that it’s considered “principle of least surprise” to use NDEBUG rather than something package-specific for that purpose. That being said, I checked our .spec file, and I don’t see NDEBUG being set in there. That would be the logical next step. > >> + 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. LGTM. I mask the low bits. So if it’s misaligned, it is not zero. In that case, I emit the warning. > >> + 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. It’s mostly a reminder that at some point, I need to revisit the code that uses the pointer, and then, I need to activate debug to know if the hypothesis that it’s misaligned is valid. I’ll put this one under a special #if, because you apparently feel strongly about the check not being in the code, and I feel strongly about being able to reactivate the instrumentation easily when I need it. > >> #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 _______________________________________________ Spice-devel mailing list Spice-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/spice-devel