[Spice-devel] [PATCH spice-gtk v7 2/4] Avoid clang warnings on casts with stricter alignment requirements

[Frediano Ziglio]

> > On 8 Jun 2017, at 12:17, 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 older 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 runtime
> > 
> 
> > > 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.
> > 
> 

> > > Runtime warnings are enabled by the --enable-alignment-checks configure
> > 
> 
> > > option (or #define SPICE_DEBUG_ALIGNMENT).
> > 
> 

> > > Signed-off-by: Christophe de Dinechin < dinechin at redhat.com >
> > 
> 

> > Honestly I would start nacking this patch.
> 

> > Or you disable the warning or you fix it where possible and leave
> 
> > the warning for a time when we'll really need to fix it.
> 

> Just to clarify, I have already done both.

> - The warning is only enabled if you have —enable-alignment-checks. It’s in
> the check-in comment above:

> > > Runtime warnings are enabled by the --enable-alignment-checks configure
> > 
> 
> > > option (or #define SPICE_DEBUG_ALIGNMENT).
> > 
> 

> So the macros no longer have any cost in the normal case, as you wanted.

If is fixed I don't see the point of having any cost even if --enable-alignment-checks is used. 

> - The patch 4/4 is dedicated to fixing some “easy” cases. I understand from
> your comments here and in response to 4/4 that you’d rather have the
> “automatic” changes (adding a void *) and the non-automatic ones (e.g.
> replacing a loop with a memcpy) in the same patch. Is that the general
> feeling for everybody?

I'd like to have fixes before. 

> > Frediano
> 

> > > ---
> > 
> 
> > > configure.ac | 8 ++++++++
> > 
> 
> > > spice-common | 2 +-
> > 
> 
> > > src/channel-cursor.c | 4 ++--
> > 
> 
> > > src/channel-display-mjpeg.c | 2 +-
> > 
> 
> > > src/channel-main.c | 2 +-
> > 
> 
> > > src/continuation.h | 6 ++++--
> > 
> 
> > > src/decode-glz-tmpl.c | 2 +-
> > 
> 
> > > src/spice-channel.c | 14 +++++++++-----
> > 
> 
> > > 8 files changed, 27 insertions(+), 13 deletions(-)
> > 
> 

> > > diff --git a/configure.ac b/configure.ac
> > 
> 
> > > index 62acafc..caa289a 100644
> > 
> 
> > > --- a/configure.ac
> > 
> 
> > > +++ b/configure.ac
> > 
> 
> > > @@ -565,6 +565,14 @@ else
> > 
> 
> > > SPICE_WARNING([No D-Bus support, desktop integration and USB redirection
> > 
> 
> > > may not work properly])
> > 
> 
> > > fi
> > 
> 

> > > +AC_ARG_ENABLE([alignment-checks],
> > 
> 
> > > + AS_HELP_STRING([--enable-alignment-checks],
> > 
> 
> > > + [Enable runtime checks for cast alignment
> > 
> 
> > > @<:@default=no@:>@]),
> > 
> 
> > > + [],
> > 
> 
> > > + enable_alignment_checks="no")
> > 
> 
> > > +AS_IF([test "x$enable_alignment_checks" = "xyes"],
> > 
> 
> > > + [AC_DEFINE([SPICE_DEBUG_ALIGNMENT], 1, [Enable runtime checks for cast
> > 
> 
> > > alignment])])
> > 
> 
> > > +
> > 
> 
> > > SPICE_CHECK_LZ4
> > 
> 

> > > dnl
> > 
> 
> > > ===========================================================================
> > 
> 
> > > diff --git a/spice-common b/spice-common
> > 
> 
> > > index af682b1..8cbfedc 160000
> > 
> 
> > > --- a/spice-common
> > 
> 
> > > +++ b/spice-common
> > 
> 
> > > @@ -1 +1 @@
> > 
> 
> > > -Subproject commit af682b1b06dea55007d9aa7c37cd443e4349e43f
> > 
> 
> > > +Subproject commit 8cbfedc08fdf6be68cabd5afd781f61ae01b577b
> > 
> 
> > > diff --git a/src/channel-cursor.c b/src/channel-cursor.c
> > 
> 
> > > index 558c920..80d2410 100644
> > 
> 
> > > --- a/src/channel-cursor.c
> > 
> 
> > > +++ b/src/channel-cursor.c
> > 
> 
> > > @@ -433,7 +433,7 @@ static display_cursor *set_cursor(SpiceChannel
> > > *channel,
> > 
> 
> > > SpiceCursor *scursor)
> > 
> 
> > > size /= 2u;
> > 
> 
> > > 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 {
> > 
> 

> > we know this is not aligned, either fix or ignore the warning!
> 

> Hmmm. For some reason, I goofed up, and the fix (marking it UNALIGNED) was
> integrated in 4/4 instead of 2/4.

> > > @@ -447,7 +447,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 {
> > 
> 

> > Did you revert the memcpy ??
> 

> No, it’s in 4/4. I separated the “fixes” from the “detection”.

> > > 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; ) {
> > 
> 

> > Here we are not sure, the warning is fine.
> 

> > > diff --git a/src/channel-main.c b/src/channel-main.c
> > 
> 
> > > index 29dd42d..1202d13 100644
> > 
> 
> > > --- a/src/channel-main.c
> > 
> 
> > > +++ b/src/channel-main.c
> > 
> 
> > > @@ -1872,7 +1872,7 @@ static void
> > 
> 
> > > main_agent_handle_xfer_status(SpiceMainChannel *channel,
> > 
> 
> > > _("The spice agent reported an error
> > 
> 
> > > during the file transfer"));
> > 
> 
> > > break;
> > 
> 
> > > case VD_AGENT_FILE_XFER_STATUS_NOT_ENOUGH_SPACE: {
> > 
> 
> > > - uint64_t *free_space = (uint64_t *)(msg->data);
> > 
> 
> > > + uint64_t *free_space = SPICE_ALIGNED_CAST(uint64_t *, msg->data);
> > 
> 
> > > gchar *free_space_str = g_format_size(*free_space);
> > 
> 
> > > gchar *file_size_str =
> > 
> 
> > > g_format_size(spice_file_transfer_task_get_total_bytes(xfer_task));
> > 
> 
> > > error = g_error_new(SPICE_CLIENT_ERROR, SPICE_CLIENT_ERROR_FAILED,
> > 
> 

> > This should be fixed.
> 

> This one is new code since last time I submitted. How should it be fixed? Or
> are you saying that we know data is unaligned? To me, the structure looks
> like:

> typedef struct SPICE_ATTR_PACKED VDAgentFileXferStatusMessage {
> uint32_t id;
> uint32_t result;
> /* Used to send additional data for detailed error messages
> * to clients with VD_AGENT_CAP_FILE_XFER_DETAILED_ERRORS capability.
> * Type of data varies with the result:
> * result : data type
> * VD_AGENT_FILE_XFER_STATUS_NOT_ENOUGH_SPACE : uint64_t
> */
> uint8_t data[0];
> } VDAgentFileXferStatusMessage;

> So at the moment, it seems that data is uint64_t aligned. Do I read
> incorrectly?

Yes, and I find really bad you don't see the problem. 

> > > 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
> > 
> 
> > > /*
> > 
> 

> > as already explained cast to void* before char*.
> 

> That’s what SPICE_ALIGNED_CAST does. Do you object to documenting that we
> widen the alignment on purpose here? Or are you still put off by a runtime
> check that is no longer there?

> void * does not document anything. I don’t like it.

In this case we know it's aligned, the problem is that the macro should cost 0 in all cases. 
If you need to check for alignment because you don't understand when is surely aligned it 
means you are not trusting the code you are writing. Or I'm entirely not understanding 
the purpose of these macros (beside doing cast and silencing the warnings). 
Or maybe I lost some changes in the macros? 

> > > 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;
> > 
> 

> > no need to check anything
> 

> Still need to silence the compiler and document we do this on purpose.

Yes, maybe I miss the macro change 

> > > diff --git a/src/spice-channel.c b/src/spice-channel.c
> > 
> 
> > > index 77ac9cd..3b6d839 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;
> > 
> 
> > > SpiceLinkMess link_msg;
> > 
> 

> > > @@ -1357,14 +1358,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, &link_msg, sizeof(link_msg)); p += sizeof(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,
> > 
> 

> > why did you revert the fix? It's hard to review patches if you keep
> 
> > going back and forth.
> 

> No, as for the others, the fix is in patch 4/4.

> > > @@ -1888,6 +1890,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);
> > 
> 
> > > @@ -1927,7 +1930,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++) {
> > 
> 

> > same
> 

> Checked that it is in 4/4.

Frediano 
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/spice-devel/attachments/20170608/72e7ad5c/attachment-0001.html>