[Spice-devel] [PATCH spice-gtk v7 2/4] Avoid clang warnings on casts with stricter alignment requirements
Christophe de Dinechin
christophe at dinechin.org
Thu Jun 8 15:09:15 UTC 2017
> 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.
- 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?
>
> 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?
>
>> 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.
>
>> 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.
>
>> 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
> _______________________________________________
> 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/20170608/53a9fba2/attachment-0001.html>
More information about the Spice-devel
mailing list