From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 3429 invoked by alias); 4 Jun 2014 11:04:33 -0000 Mailing-List: contact gcc-patches-help@gcc.gnu.org; run by ezmlm Precedence: bulk List-Id: List-Archive: List-Post: List-Help: Sender: gcc-patches-owner@gcc.gnu.org Received: (qmail 3417 invoked by uid 89); 4 Jun 2014 11:04:32 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-1.7 required=5.0 tests=AWL,BAYES_00,FREEMAIL_FROM,RCVD_IN_DNSWL_LOW,SPF_PASS autolearn=ham version=3.3.2 X-HELO: mail-we0-f179.google.com Received: from mail-we0-f179.google.com (HELO mail-we0-f179.google.com) (74.125.82.179) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with (AES128-SHA encrypted) ESMTPS; Wed, 04 Jun 2014 11:04:31 +0000 Received: by mail-we0-f179.google.com with SMTP id q59so8122588wes.24 for ; Wed, 04 Jun 2014 04:04:28 -0700 (PDT) MIME-Version: 1.0 X-Received: by 10.180.183.131 with SMTP id em3mr4139077wic.56.1401879867863; Wed, 04 Jun 2014 04:04:27 -0700 (PDT) Received: by 10.194.219.97 with HTTP; Wed, 4 Jun 2014 04:04:27 -0700 (PDT) In-Reply-To: <002701cf7fe1$a7c7c4d0$f7574e70$@arm.com> References: <001d01cf7fba$259d63b0$70d82b10$@arm.com> <002701cf7fe1$a7c7c4d0$f7574e70$@arm.com> Date: Wed, 04 Jun 2014 11:04:00 -0000 Message-ID: Subject: Re: [PATCH] Fix PR61306: improve handling of sign and cast in bswap From: Richard Biener To: "Thomas Preud'homme" Cc: GCC Patches Content-Type: text/plain; charset=UTF-8 X-IsSubscribed: yes X-SW-Source: 2014-06/txt/msg00333.txt.bz2 On Wed, Jun 4, 2014 at 12:42 PM, Thomas Preud'homme 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. >