> On 16 May 2017, at 16:51, Christophe Fergeau <cferg...@redhat.com> wrote:
>
> Hey,
>
> On Thu, May 11, 2017 at 12:47:05PM +0200, Christophe de Dinechin wrote:
>> From: Christophe de Dinechin <dinec...@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) is the same as (type) value, but
>> silences the warning (using an intermediate cast to (void *)) and
>> adds a runtime check to verify that the pointer value is actually
>> aligned as expected. If that expectation is violated, a warning
>> is issued.
>>
>> - SPICE_UNALIGNED_CAST(type, value) works like SPICE_ALIGNED_CAST,
>> but asssumes that the value is not known to be aligned. If there
>> is an alignment error, no warning is issued. It is still possible
>> to know if misalignment occurs by defining environment variable
>> SPICE_DEBUG_ALIGNMENT, in which case a debug message is issued for
>> every misaligned pointer.
>
> For some reason, after reading the log, it's still not 100% obvious to
> me that SPICE_ALIGNED_CAST means "I checked, the cast which is being
> done is legit as the pointer has the right aligment", while
> SPICE_UNALIGNED_CAST means "pointer does not have the right alignment,
> fixing might need to be done”.
Actually, both are “legit” on x86, except performance-wise.
Do you have a better name suggestion for naming these macros?
>
>>
>> Signed-off-by: Christophe de Dinechin <dinec...@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..50de5ce 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_UNALIGNED_CAST(uint32_t*,(data + size)) + idx);
>> if (pix_mask && pix == 0xffffff) {
>> cursor->data[i] = get_pix_hack(i, hdr->width);
>> } else {
>> 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; ) {
>> 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);
>
> I don't think the comment is necessary.
> p32 could be named current_cap, caps, or something like this instead
OK. I named it that way because the old pointer was called 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);
>> + *p32++ = GUINT32_TO_LE(g_array_index(c->common_caps, uint32_t, i));
>
> Assignment followed by increment is more readable imo.
OK. What I really wanted to get rid of was the sizeof(uint32_t).
>
>> }
>> 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);
>
> Nit: Sometimes you use a ", " inside SPICE_*_CAST, sometimes just ",”.
Will fix. I normally prefer with space, that’s also the majority style in spice
AFAICT. I will send a new version with your comments. How does it work if
ack-ed?
>
> Overall looks fine to me,
>
> Acked-by: Christophe Fergeau <cferg...@redhat.com
> <mailto:cferg...@redhat.com>>
>
> Christophe
> _______________________________________________
> Spice-devel mailing list
> Spice-devel@lists.freedesktop.org <mailto:Spice-devel@lists.freedesktop.org>
> https://lists.freedesktop.org/mailman/listinfo/spice-devel
> <https://lists.freedesktop.org/mailman/listinfo/spice-devel>
_______________________________________________
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel
