Hi Johannes

On Tue, Aug 8, 2017 at 1:19 PM, Johannes Schindelin <
johannes.schinde...@gmx.de> wrote:

> Hi Emmanuel,
>
> On Mon, 7 Aug 2017, Emmanuel Deloget wrote:
>
> > And yes, its seems that the get_le32() macro in xz_private.h is a bit
> > illegal with respect to strict aliasing, as it casts a uint8_t * into a
> > const uint32_t *.
>
> It would seem so.
>
> Until you realize that it is used only on s->temp.buf (or `s->temp.buf + 4
> * <number>`), and that the `buf` field was carefully placed after two
> fields of type `size_t` (i.e. naturally aligned *at least* to a 32-bit
> boundary).
>
> If you are worried about that magic, you can even mark the `buf` field
> using ALIGN4. But that *still* does not fix GCC's compiler warning. What
> fixes it is my workaround of defining a static ALWAYS_INLINE function.
>
>
​No, I'm not worried by the magic :)

Yet the problem is not the aliasing per see, it's the way the compiler
handles it and what it means to him.

Strict aliasing has been defined as a way to allow the compiler to do some
clever code optimization (mostly reordering). If the compiler knows that
two memory areas are distinct, there is no R/W dependency on these areas
and it can reorder the code as he wants. But when aliasing comes into play,
such optimizations should be forbidden since an undetected R/W dependency
might exists. Yet, since the -fstrict-aliasing is given to him, we
explicitely tell the compiler to force strict aliasing rules and the
compiler ​still apply them -- ditching any unproved R/W dependency in the
process.

Needless to say, this can be the source of hard-to-catch bugs :)

Consider this code:

uint32_t k = 0;
uint8_t *p = (uint8_t *)&k;

k = 0xffffffff;
if (*p == 0xff)
   do_some_magic_stuff();

Without strict aliasing, the compiler must order the code so that *p is
indeed 0xff when the if condition is evaluated because it cannot be sure
that there is a R/W dependency between k and p. With forced strict
aliasing, &k and p are assumed to not point to the same memory area, thus
there cannot be any R/W dependency, so changing k is not supposed to have
any effect on the *p test. As a consequence, the compiler is free to
reorder the code and for exemple do the following:

if (*p = 0xff)
  do_some_magic_stuff();
k = 0xffffffff;

Which, of course, will not work as expected.

Thus, this aliasing problem may have other unintended consequences.
Silencing the warning will hide a possible bug, and this is not a good
thing. It would be far better to fix the aliasing issue correctly.
Similarly, the fact that ALWAY_INLINE seems to hide the warning looks more
like a GCC bug to me than the expected behavior, as ALWAYS_INLINE does not
change the way the compiler plays with the aliasing rules.

Something along the line of (not tested):

#define get_le32(p) \
  ({ \
    union { const uint32_t *x32; const uint8_t *x8; } v = { .x8 = (p), }; \
    le32_to_cpup(v.x32); \
  })

​(for the occurence at xz_private.h@24)

Maybe it's even better to create an aliastype macro looking like:

#define aliasvalue(type, value) \
  ({ \
    union { \
      type *aliased; \
      typeof(value) ​*original; \
    } v = { \
      .original = &(value), \
    }; \
    v.aliased; \
  })

​And then:

#define get_le32(p) le32_to_cpup(aliasvalue(uint32_t, (p)))

Or

#define get_le32(p) (*(aliasvalue(uint32_t,(p))))

I'm not even sure the compiler is not able to fully optimize out this kind
of cod since it will look like a move from one register to another register
to him (my own tests seems to show that the compiler emits optimized code
in this situation).
​
The aliasvalue() macro above is probably not perfect (it might run ok when
p in a scalar or an array of scalar but will break if p is a pointer; not
to mention its horrible name...) so more though might be needed to come up
with a correct version.



> Denys' fix works around the problem, alright, but it also makes the code
> slower. And it is *not* necessary to make the code slower.
>
> Ciao,
> Johannes
>

​Best regards,

-- Emmanuel Deloget​
_______________________________________________
busybox mailing list
busybox@busybox.net
http://lists.busybox.net/mailman/listinfo/busybox

Reply via email to