Re: [PATCH] Fix PR61306: improve handling of sign and cast in bswap

[Richard Biener <richard dot guenther at gmail dot com>]

On Wed, Jun 4, 2014 at 12:42 PM, Thomas Preud'homme
<thomas.preudhomme@arm.com> wrote:
>> From: Richard Biener [mailto:richard.guenther@gmail.com]
>>
>> I'd rather change the comparisons
>>
>> -  if (n->size < (int)sizeof (int64_t))
>> -    n->n &= ((uint64_t)1 << (n->size * BITS_PER_UNIT)) - 1;
>> +  if (bitsize / BITS_PER_UNIT < (int)sizeof (int64_t))
>> +    n->n &= ((uint64_t)1 << bitsize) - 1;
>>
>> to work in bits, thus bitsize < 8 * sizeof (int64_t) (note that using
>> BITS_PER_UNIT is bogus here - you are dealing with 8-bit bytes
>> on the host, not whatever the target uses).  Otherwise it smells
>> like truncation may lose bits (probably not in practice).
>
> Ah yes, right.
>
>>
>> +           /* Sign extension: result is dependent on the value.  */
>> +           if (!TYPE_UNSIGNED (type) && !TYPE_UNSIGNED (n->type)
>> +               && type_size > TYPE_PRECISION (n->type))
>> +             return NULL_TREE;
>>
>> whether it's sign-extended depends solely on the sign of the
>> converted entity, so I'm not sure why you are restricting this
>> to signed n->type.  Consider
>>
>> signed char *p;
>> ((unsigned int)p[0]) << 8 | ((unsigned int)p[1]) << 16
>>
>> the loads are still sign-extended but n->type is unsigned.
>
> Indeed, I understood it for convert_via (the requirement to be
> unsigned) but got it wrong here.
>
>>
>> I'm failing to get why you possibly need two casts ... you should
>> only need one, from the bswap/load result to the final type
>> (zero-extended as you say - so the load type should simply be
>> unsigned which it is already).
>
> Because of the type of the shift constant, the bitwise expression
> is usually of type int. However, if you write a function that does
> a 32 bit load in host endianness (like a 32 bit little endian load on
> x86_64) with a result of size 64 bits, then you need to sign
> extend, since the source type is signed. This is a situation I
> encountered in bfd_getl32 in binutils I think.
>
> Now if you consider bfd_getl16 instead a direct sign extension
> is out of the question. Suppose bfd_getl16 is called to read from
> a memory address that contains 0xff 0xfe. The bitwise expression
> would thus be equivalent to the value 0x0000feff since it's of type
> int. Then after the sign extension to 64bits you'd have
> 0x000000000000feff. But after replacing the bitwise expression
> you end up with a 16bit load into a 16bit SSA variable. If you sign
> extend that directly to 64 bits you'll end up with
> 0xfffffffffffffeff which is wrong. But if you zero extend to an
> int value (the type of the bitwise OR expression) and then sign
> extend to the target type you'll have the correct result.

Err, but if you zero-extend directly to the target type you have the
correct result, too.

> But you're right, we can do simpler by sign extending if
> load size == size of bitwise expression and zero extend else. The
> change of load_type to range_type would still be needed
> because in case of a load + bswap it's better to load in the
> same type as the type of the parameter of bswap. After bswap
> you'd need to convert to a signed or unsigned value according
> to the logic above (load size == size of bitwise expression)
>
> In the original statement, the bitwise OR
> expression would have the 2 bytes of higher weight be 0 while
> the 2 bytes of lower weight would be the value read from
> memory. The result of the sign extension would be
>
>>
>> So I think that the testcase in the patch is fixed already by
>> doing the n->type change (and a proper sign-extension detection).
>
> I don't remember exactly but I think it didn't fix this bug
> (but it is a necessary fix anyway).
>
>>
>> Can you please split that part out?
>
> Sure, that part would need to be applied on gcc 4.9 too. I'll try
> to construct a testcase for that.
>
>>
>> That range_type and convert_via looks wrong and unnecessary to me,
>> and it doesn't look like you have a testcase excercising it?
>
> See above.

But nothing for the testsuite?  The testcase you add fails foul of
sign-extending the loads.

Richard.

>