On 11 May 2017, at 10:17, Frediano Ziglio <fziglio@xxxxxxxxxx> 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@xxxxxxxxxx>
---
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 harderto 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 wouldbe better if this could be unaligned so a grep would help the realporting (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.
|