[Spice-devel] [PATCH spice-gtk 1/4] Avoid clang warnings on casts with stricter alignment requirements
Christophe de Dinechin
dinechin at redhat.com
Thu May 11 08:50:37 UTC 2017
> On 11 May 2017, at 10:37, Frediano Ziglio <fziglio at redhat.com> wrote:
>
>>
>>
>>> 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.
I updated the commit log, because I think there is a misunderstanding. SPICE_ALIGNED_CAST means “I think this should be aligned”. If it is not aligned, then a warning is emitted at runtime. So it is less “silencing” the problem than SPICE_UNALIGNED_CAST, which states that you know this is misaligned anyway.
As for being able to grep to identify code that needs rework, you are right that it may be useful to revisit unaligned cases first. But the aligned
cases do not need to be de-optimized if experiments don’t show that we actually have misaligned pointers.
>
>>>
>>>> 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 :-)
You you you only only only had had had your first first cooo cooo coofffeee toootootootday?
Lucky you!
>
>>>
>>>> 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.
It is not known to be aligned. Not guaranteed to be unaligned. I don’t have a special cast to check that it’s always misaligned ;-)
>>
>>>
>>>> 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?
spice-common. I think I announced it in the envelope mail for the patch. That’s why there is a submodule update. I don’t know if this is the right way to do things for submodule required changes.
>
> Frediano
> _______________________________________________
> Spice-devel mailing list
> Spice-devel at lists.freedesktop.org <mailto:Spice-devel at lists.freedesktop.org>
> https://lists.freedesktop.org/mailman/listinfo/spice-devel <https://lists.freedesktop.org/mailman/listinfo/spice-devel>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/spice-devel/attachments/20170511/4c6b706c/attachment-0001.html>
More information about the Spice-devel
mailing list