[Spice-devel] [PATCH spice-gtk v5 2/5] Avoid clang warnings on casts with stricter alignment requirements
Frediano Ziglio
fziglio at redhat.com
Tue May 30 13:49:40 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.
>
> Signed-off-by: Christophe de Dinechin <dinechin at redhat.com>
> ---
> spice-common | 2 +-
> src/channel-cursor.c | 6 +++---
> src/channel-display-mjpeg.c | 2 +-
> src/continuation.h | 6 ++++--
> src/decode-glz-tmpl.c | 2 +-
> src/spice-channel.c | 14 +++++++++-----
> 6 files changed, 19 insertions(+), 13 deletions(-)
>
> diff --git a/spice-common b/spice-common
> index af682b1..692d303 160000
> --- a/spice-common
> +++ b/spice-common
> @@ -1 +1 @@
> -Subproject commit af682b1b06dea55007d9aa7c37cd443e4349e43f
> +Subproject commit 692d303c8b1e2da7a47da713ad499f57ed5e6b5e
> diff --git a/src/channel-cursor.c b/src/channel-cursor.c
> index cddba03..85160eb 100644
> --- a/src/channel-cursor.c
> +++ b/src/channel-cursor.c
> @@ -422,7 +422,7 @@ static display_cursor *set_cursor(SpiceChannel *channel,
> SpiceCursor *scursor)
> memcpy(cursor->data, data, size);
> for (i = 0; i < hdr->width * hdr->height; i++) {
> pix_mask = get_pix_mask(data, size, i);
> - if (pix_mask && *((guint32*)data + i) == 0xffffff) {
> + if (pix_mask && *(SPICE_ALIGNED_CAST(guint32 *, data) + i) ==
> 0xffffff) {
> cursor->data[i] = get_pix_hack(i, hdr->width);
> } else {
> cursor->data[i] |= (pix_mask ? 0 : 0xff000000);
Here cursor->data[i] can be used which is aligned.
> @@ -432,7 +432,7 @@ static display_cursor *set_cursor(SpiceChannel *channel,
> SpiceCursor *scursor)
> case SPICE_CURSOR_TYPE_COLOR16:
> for (i = 0; i < hdr->width * hdr->height; i++) {
> pix_mask = get_pix_mask(data, size, i);
> - pix = *((guint16*)data + i);
> + pix = *(SPICE_ALIGNED_CAST(guint16 *, data) + i);
> if (pix_mask && pix == 0x7fff) {
> cursor->data[i] = get_pix_hack(i, hdr->width);
> } else {
actually this is unaligned, the data came from network buffer.
> @@ -446,7 +446,7 @@ static display_cursor *set_cursor(SpiceChannel *channel,
> SpiceCursor *scursor)
> for (i = 0; i < hdr->width * hdr->height; i++) {
> pix_mask = get_pix_mask(data, size + (sizeof(uint32_t) << 4),
> i);
> int idx = (i & 1) ? (data[i >> 1] & 0x0f) : ((data[i >> 1] &
> 0xf0) >> 4);
> - pix = *((uint32_t*)(data + size) + idx);
> + pix = *(SPICE_UNALIGNED_CAST(uint32_t *, (data + size)) + idx);
> if (pix_mask && pix == 0xffffff) {
> cursor->data[i] = get_pix_hack(i, hdr->width);
> } else {
Maybe the palette can be copied with a memcpy?
Looks like this code is also little endian dependent.
> diff --git a/src/channel-display-mjpeg.c b/src/channel-display-mjpeg.c
> index 3ae9d21..ee33b01 100644
> --- a/src/channel-display-mjpeg.c
> +++ b/src/channel-display-mjpeg.c
> @@ -151,7 +151,7 @@ static gboolean mjpeg_decoder_decode_frame(gpointer
> video_decoder)
> #ifndef JCS_EXTENSIONS
> {
> uint8_t *s = lines[0];
> - uint32_t *d = (uint32_t *)s;
> + uint32_t *d = SPICE_ALIGNED_CAST(uint32_t *, s);
>
> if (back_compat) {
> for (unsigned int j = lines_read * width; j > 0; ) {
> diff --git a/src/continuation.h b/src/continuation.h
> index 675a257..d1fd137 100644
> --- a/src/continuation.h
> +++ b/src/continuation.h
> @@ -21,6 +21,7 @@
> #ifndef _CONTINUATION_H_
> #define _CONTINUATION_H_
>
> +#include "spice-common.h"
> #include <stddef.h>
> #include <ucontext.h>
> #include <setjmp.h>
> @@ -48,8 +49,9 @@ int cc_release(struct continuation *cc);
> int cc_swap(struct continuation *from, struct continuation *to);
>
> #define offset_of(type, member) ((unsigned long)(&((type *)0)->member))
> -#define container_of(obj, type, member) \
> - (type *)(((char *)obj) - offset_of(type, member))
> +#define container_of(obj, type, member) \
> + SPICE_ALIGNED_CAST(type *, \
> + (((char *)obj) - offset_of(type, member)))
>
> #endif
> /*
No reason to check, just add the void* cast.
> diff --git a/src/decode-glz-tmpl.c b/src/decode-glz-tmpl.c
> index b337a8b..76d832c 100644
> --- a/src/decode-glz-tmpl.c
> +++ b/src/decode-glz-tmpl.c
> @@ -178,7 +178,7 @@ static size_t FNAME(decode)(SpiceGlzDecoderWindow
> *window,
> uint64_t image_id, SpicePalette *plt)
> {
> uint8_t *ip = in_buf;
> - OUT_PIXEL *out_pix_buf = (OUT_PIXEL *)out_buf;
> + OUT_PIXEL *out_pix_buf = SPICE_ALIGNED_CAST(OUT_PIXEL *, out_buf);
> OUT_PIXEL *op = out_pix_buf;
> OUT_PIXEL *op_limit = out_pix_buf + size;
>
> diff --git a/src/spice-channel.c b/src/spice-channel.c
> index 3b6231e..6c52547 100644
> --- a/src/spice-channel.c
> +++ b/src/spice-channel.c
> @@ -1312,6 +1312,7 @@ static void spice_channel_send_link(SpiceChannel
> *channel)
> {
> SpiceChannelPrivate *c = channel->priv;
> uint8_t *buffer, *p;
> + uint32_t *caps;
> int protocol, i;
>
> c->link_hdr.magic = SPICE_MAGIC;
> @@ -1356,14 +1357,15 @@ static void spice_channel_send_link(SpiceChannel
> *channel)
> memcpy(p, &c->link_hdr, sizeof(c->link_hdr)); p += sizeof(c->link_hdr);
> memcpy(p, &c->link_msg, sizeof(c->link_msg)); p += sizeof(c->link_msg);
>
> + // Need this to avoid error with -Wcast-align
> + caps = SPICE_UNALIGNED_CAST(uint32_t *,p);
> for (i = 0; i < c->common_caps->len; i++) {
> - *(uint32_t *)p = GUINT32_TO_LE(g_array_index(c->common_caps,
> uint32_t, i));
> - p += sizeof(uint32_t);
> + *caps++ = GUINT32_TO_LE(g_array_index(c->common_caps, uint32_t, i));
> }
> for (i = 0; i < c->caps->len; i++) {
> - *(uint32_t *)p = GUINT32_TO_LE(g_array_index(c->caps, uint32_t, i));
> - p += sizeof(uint32_t);
> + *caps++ = GUINT32_TO_LE(g_array_index(c->caps, uint32_t, i));
> }
> + p = (uint8_t *) caps;
> CHANNEL_DEBUG(channel, "channel type %d id %d num common caps %u num
> caps %u",
> c->channel_type,
> c->channel_id,
another way is to align caps_offset.
> @@ -1887,6 +1889,7 @@ static gboolean
> spice_channel_recv_link_msg(SpiceChannel *channel)
> SpiceChannelPrivate *c;
> int rc, num_caps, i;
> uint32_t *caps, num_channel_caps, num_common_caps;
> + uint8_t *caps_src;
> SpiceChannelEvent event = SPICE_CHANNEL_ERROR_LINK;
>
> g_return_val_if_fail(channel != NULL, FALSE);
> @@ -1926,7 +1929,8 @@ static gboolean
> spice_channel_recv_link_msg(SpiceChannel *channel)
> /* see original spice/client code: */
> /* g_return_if_fail(c->peer_msg + c->peer_msg->caps_offset *
> sizeof(uint32_t) > c->peer_msg + c->peer_hdr.size); */
>
> - caps = (uint32_t *)((uint8_t *)c->peer_msg +
> GUINT32_FROM_LE(c->peer_msg->caps_offset));
> + caps_src = (uint8_t *)c->peer_msg +
> GUINT32_FROM_LE(c->peer_msg->caps_offset);
> + caps = SPICE_UNALIGNED_CAST(uint32_t *, caps_src);
>
> g_array_set_size(c->remote_common_caps, num_common_caps);
> for (i = 0; i < num_common_caps; i++, caps++) {
A memcpy before the conversion would solve any cases.
Frediano
More information about the Spice-devel
mailing list