[Spice-devel] [PATCH v2 2/2] Avoid clang warnings on casts with stricter alignment requirements
Christophe de Dinechin
christophe at dinechin.org
Thu May 18 13:24:41 UTC 2017
> On 18 May 2017, at 12:48, Frediano Ziglio <fziglio at redhat.com> wrote:
>
>>
>> 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.
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
More information about the Spice-devel
mailing list