From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:35996) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Zu0Zs-0007ee-BM for qemu-devel@nongnu.org; Wed, 04 Nov 2015 11:06:25 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Zu0Zn-00063a-Bk for qemu-devel@nongnu.org; Wed, 04 Nov 2015 11:06:24 -0500 Received: from mx1.redhat.com ([209.132.183.28]:45420) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Zu0Zn-00063W-6E for qemu-devel@nongnu.org; Wed, 04 Nov 2015 11:06:19 -0500 References: <1446473134-4330-1-git-send-email-pbonzini@redhat.com> <563777D5.6050000@redhat.com> <5639DA14.3020507@twiddle.net> <5639E1C2.80902@redhat.com> <5639E691.4050203@twiddle.net> <5639FE12.9000901@redhat.com> <87pozpluy7.fsf@blackfin.pond.sub.org> From: Paolo Bonzini Message-ID: <563A2CF5.5020103@redhat.com> Date: Wed, 4 Nov 2015 17:06:13 +0100 MIME-Version: 1.0 In-Reply-To: <87pozpluy7.fsf@blackfin.pond.sub.org> Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH] target-sparc: fix 32-bit truncation in fpackfix List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Markus Armbruster Cc: Blue Swirl , Peter Maydell , Mark Cave-Ayland , QEMU Developers , Richard Henderson On 04/11/2015 15:07, Markus Armbruster wrote: > Paolo Bonzini writes: > >> On 04/11/2015 12:05, Richard Henderson wrote: >>> On 11/04/2015 11:45 AM, Paolo Bonzini wrote: >>>>>>>>>> int32_t src = rs2 >> (word * 32); >>>>>>>>>> - int64_t scaled = src << scale; >>>>>>>>>> + int64_t scaled = (int64_t)src << scale; >>>>>>>>>> int64_t from_fixed = scaled >> 16; >>> ... >>>>> >>>>> I do think we'd be better served by casting to uint64_t on that line. >>>>> Note that fpackfix requires the same correction. And it wouldn't hurt >>>>> to cast to uint32_t in fpack16, lest we anger the self-same shifting >>>>> gods. >>>> >>>> Hmmm.. say src = -0x80000000, scale = 1; >>>> >>>> scaled = (uint64_t)-0x8000000 << 1 = 0xffffffff00000000 >>>> from_fixed = 0xffffffff00000000 >> 16 = 0x0000ffffffff0000 >>>> >>>> Now from_fixed is positive and you get 32767 instead of -32768. In >>>> other words, we would have to cast to uint64_t on the scaled assignment, >>>> and back to int64_t on the from_fixed assignment. I must be >>>> misunderstanding your suggestion. >>> >>> int64_t scaled = (uint64_t)src << scale; >>> >>> I.e. one explicit conversion and one implicit conversion. >> >> That does the job, but it also does look like a typo... > > Make the implicit conversion explicit then. Sorry, but I'll say it again: there's _no way_ that a sane compiler will _ever_ use this particular bit of undefined behavior. I'm generally against uglifying the code to placate ubsan, but especially so in this case: it is not common code and it would only affect people running fpackfix under ubsan. Paolo