From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:55468) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZuGbL-0006Qy-ER for qemu-devel@nongnu.org; Thu, 05 Nov 2015 04:13:00 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1ZuGbG-0000EU-GD for qemu-devel@nongnu.org; Thu, 05 Nov 2015 04:12:59 -0500 Received: from mail-wi0-x22f.google.com ([2a00:1450:400c:c05::22f]:36092) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZuGbG-0000EP-AV for qemu-devel@nongnu.org; Thu, 05 Nov 2015 04:12:54 -0500 Received: by wimw2 with SMTP id w2so5704257wim.1 for ; Thu, 05 Nov 2015 01:12:53 -0800 (PST) Sender: Paolo Bonzini 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> <563A968D.705@ilande.co.uk> From: Paolo Bonzini Message-ID: <563B1D91.5010701@redhat.com> Date: Thu, 5 Nov 2015 10:12:49 +0100 MIME-Version: 1.0 In-Reply-To: <563A968D.705@ilande.co.uk> Content-Type: text/plain; charset=utf-8 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: Mark Cave-Ayland , Richard Henderson , Peter Maydell Cc: Blue Swirl , QEMU Developers On 05/11/2015 00:36, Mark Cave-Ayland wrote: > On 04/11/15 11: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. > > I suspect Richard knows more about this part of SPARC emulation than I > do, so I'd be fine with a solution similar to the above if everyone > agress. Let me know if you need me to send a SPARC pull request, > although it will probably be quicker coming from Paolo/Richard at the > moment. All solutions work. You have to tell us which you prefer among /* Has undefined behavior (though no compiler uses it) */ int64_t scaled = (int64_t)src << scale; /* Seems like a typo */ int64_t scaled = (uint64_t)src << scale; /* Ugly code */ int64_t scaled = (uint64_t)(int64_t)src << scale; Paolo