Jakub Jelinek <[email protected]> wrote:
> On Tue, Nov 10, 2020 at 04:48:10PM -0700, Jeff Law via Gcc-patches wrote:
>> > @@ -486,10 +425,10 @@
>> > SItype
>> > __bswapsi2 (SItype u)
>> > {
>> > - return ((((u) & 0xff000000) >> 24)
>> > - | (((u) & 0x00ff0000) >> 8)
>> > - | (((u) & 0x0000ff00) << 8)
>> > - | (((u) & 0x000000ff) << 24));
>> > + return ((((u) & 0xff000000u) >> 24)
>> > + | (((u) & 0x00ff0000u) >> 8)
>> > + | (((u) & 0x0000ff00u) << 8)
>> > + | (((u) & 0x000000ffu) << 24));
>>
>> What's the point of this change? I'm not sure how the signedness of the
>> constant really matters here.
>
> Note 0xff000000 is implicitly 0xff000000U because it doesn't fit into signed
> int, and that is the only one where the logical vs. arithmetic right shift
> really matters for correct behavior.
Ouch: that's but not the point here; what matters is the undefined behaviour of
((u) & 0x000000ff) << 24
0x000000ff is a signed int, so (u) & 0x000000ff is signed too -- and producing
a negative value (or overflow) from the left-shift of a signed int, i.e.
shifting into (or beyond) the sign bit, is undefined behaviour!
JFTR: both -fsanitize=signed-integer-overflow and -fsanitize=undefined fail
to catch this BUGBUGBUG, which surfaces on i386 and AMD64 with -O1 or
-O0!
Stefan Kanthak
PS: even worse, -fsanitize=signed-integer-overflow fails to catch 1 << 31
or 128 << 24!
