Hi Johannes
On Tue, Aug 8, 2017 at 1:19 PM, Johannes Schindelin <
[email protected]> 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
[email protected]
http://lists.busybox.net/mailman/listinfo/busybox
