[Spice-devel] [PATCH spice-gtk 1/4] Avoid clang warnings on casts with stricter alignment requirements
Frediano Ziglio
fziglio at redhat.com
Thu May 11 08:37:52 UTC 2017
>
>
> > On 11 May 2017, at 10:17, Frediano Ziglio <fziglio at redhat.com> wrote:
> >
> >>
> >> 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]
> >>
> >> Adding an intermediate cast to (void *) silences the warning.
> >>
> >> 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/decode-glz-tmpl.c | 2 +-
> >> src/spice-channel.c | 14 +++++++++-----
> >> 5 files changed, 15 insertions(+), 11 deletions(-)
> >>
> >> diff --git a/spice-common b/spice-common
> >> index af682b1..1239c82 160000
> >> --- a/spice-common
> >> +++ b/spice-common
> >> @@ -1 +1 @@
> >> -Subproject commit af682b1b06dea55007d9aa7c37cd443e4349e43f
> >> +Subproject commit 1239c82c54dee0244cca262cca0aa21071b23e24
> >> diff --git a/src/channel-cursor.c b/src/channel-cursor.c
> >> index 609243b..51c6c2e 100644
> >> --- a/src/channel-cursor.c
> >> +++ b/src/channel-cursor.c
> >> @@ -340,7 +340,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);
> >> @@ -350,7 +350,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 {
> >> @@ -364,7 +364,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_ALIGNED_CAST(uint32_t*,(data + size)) + idx);
> >> if (pix_mask && pix == 0xffffff) {
> >> cursor->data[i] = get_pix_hack(i, hdr->width);
> >> } else {
> >
> > This is not fine. The palette at the end of the image (this is a 4 bit
> > image)
> > is not garantee to be 4 byte aligned.
>
> OK. This would show up as a warning in the logs, but I guess I would need to
> test color cursor changes to see it, right?
>
actually you need a 4 bit cursor, not sure if windows use these anymore...
surely with an old windows version you'll get these. I supposed is harder
to find on X.
>
> >
> >> diff --git a/src/channel-display-mjpeg.c b/src/channel-display-mjpeg.c
> >> index 3ae9d21..17c0f4f 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; ) {
> >
> > Not really sure, lines are returned by jpeg_read_scanlines, not sure
> > the output alignment.
>
> I prefer to keep “aligned” by default. If there is a warning showing up in
> the logs, we can switch it to unaligned.
>
Well.. no as actually are just macros silencing a possible problem would
be better if this could be unaligned so a grep would help the real
porting (if needed) in the future.
> >
> >> diff --git a/src/decode-glz-tmpl.c b/src/decode-glz-tmpl.c
> >> index b337a8b..7695a28 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..88d0567 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 *p32;
> >> 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
> >> + p32 = SPICE_UNALIGNED_CAST(uint32_t *,p);
> >
> >
> > absolutely false! This is SURELY not aligned, link_msg is aligned to 2
> > bytes.
>
> This is why it is marked as “unaligned”. This one did indeed show up in my
> experiments.
>
Though were all "aligned"... my mistake. Yes, there are right.
Just had coffee, should help :-)
> >
> >> 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);
> >> + *p32++ = 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);
> >> + *p32++ = GUINT32_TO_LE(g_array_index(c->caps, uint32_t, i));
> >> }
> >> + p = (uint8_t *) p32;
> >> CHANNEL_DEBUG(channel, "channel type %d id %d num common caps %u num
> >> caps
> >> %u",
> >> c->channel_type,
> >> c->channel_id,
> >> @@ -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);
> >>
> >
> > same!
>
> Are you saying this one is guaranteed to be unaligned? If so, that’s the
> correct cast.
>
> >
> >> g_array_set_size(c->remote_common_caps, num_common_caps);
> >> for (i = 0; i < num_common_caps; i++, caps++) {
> >
I cannot find these macros defined in spice-gtk. Where are they?
Frediano
More information about the Spice-devel
mailing list