On 8 Jun 2017, at 12:17, Frediano Ziglio <fziglio@xxxxxxxxxx> wrote:
From: Christophe de Dinechin <dinechin@xxxxxxxxxx>
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@xxxxxxxxxx>
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 anythingStill 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++) {
sameChecked that it is in 4/4.
Frediano
_______________________________________________ Spice-devel mailing list Spice-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/spice-devel