[Spice-devel] [PATCH v3 2/2] Avoid clang warnings on casts with stricter alignment requirements
Christophe de Dinechin
christophe at dinechin.org
Thu May 18 13:24:53 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. This runtime check is disabled if NDEBUG is defined
(much like assert() as defined in <assert.h>)
- 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
by configuring with CFLAGS=-DSPICE_DEBUG_ALIGNMENT.
Signed-off-by: Christophe de Dinechin <dinechin at redhat.com>
---
common/canvas_base.c | 14 +++++++++-----
common/mem.c | 23 +++++++++++++++++++++++
common/mem.h | 40 ++++++++++++++++++++++++++++++++++++++++
common/sw_canvas.c | 11 ++++++-----
4 files changed, 78 insertions(+), 10 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);
}
}
@@ -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..5ce6874 100644
--- a/common/mem.c
+++ b/common/mem.c
@@ -294,3 +294,26 @@ 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);
+ }
+}
+
+
+#ifdef SPICE_DEBUG_ALIGNMENT
+void spice_alignment_debug(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_DEBUG, loc, __FUNCTION__,
+ "Expected misaligned access at %p, alignment %u", p, sz);
+ }
+}
+#endif // SPICE_DEBUG_ALIGNMENT
diff --git a/common/mem.h b/common/mem.h
index cf24d0a..91d49ce 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>
@@ -139,6 +140,45 @@ size_t spice_strnlen(const char *str, size_t max_len);
#endif
+/* Cast to a type with stricter alignment constraints (to build with clang) */
+#define SPICE_ALIGNED_CAST(type, value) \
+ ((type)spice_alignment_check(SPICE_STRLOC, \
+ (void *)(value), \
+ __alignof(*((type)0))))
+
+extern void spice_alignment_warning(const char *loc, void *p, unsigned sz);
+static inline void *spice_alignment_check(const char *loc,
+ void *ptr, unsigned sz)
+{
+#ifndef NDEBUG
+ if (G_UNLIKELY(((uintptr_t) ptr & (sz-1U)) != 0))
+ spice_alignment_warning(loc, ptr, sz);
+#endif // NDEBUG
+ return ptr;
+
+}
+
+/* Misaligned cast to a type with stricter alignment */
+#ifndef SPICE_DEBUG_ALIGNMENT
+#define SPICE_UNALIGNED_CAST(type, value) ((type)(void *)(value))
+
+#else // SPICE_DEBUG_ALIGNMENT
+#define SPICE_UNALIGNED_CAST(type, value) \
+ ((type)spice_alignment_weak_check(SPICE_STRLOC, \
+ (void *)(value), \
+ __alignof(*((type)0))))
+
+static inline void *spice_alignment_weak_check(const char *loc,
+ void *ptr, unsigned sz)
+{
+ extern void spice_alignment_debug(const char *loc, void *p, unsigned sz);
+ if (G_UNLIKELY(((uintptr_t) ptr & (sz-1U)) != 0))
+ spice_alignment_debug(loc, ptr, sz);
+ return ptr;
+
+}
+#endif // SPICE_DEBUG_ALIGNMENT
+
#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 f9bd99c..c347a56 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);
@@ -1264,9 +1264,10 @@ 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,
--
2.11.0 (Apple Git-81)
More information about the Spice-devel
mailing list