From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:44272) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZtvZJ-0001dW-Uk for qemu-devel@nongnu.org; Wed, 04 Nov 2015 05:45:30 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1ZtvZF-0006xe-Mo for qemu-devel@nongnu.org; Wed, 04 Nov 2015 05:45:29 -0500 Received: from mail-wi0-x22d.google.com ([2a00:1450:400c:c05::22d]:38707) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZtvZF-0006xX-HE for qemu-devel@nongnu.org; Wed, 04 Nov 2015 05:45:25 -0500 Received: by wicll6 with SMTP id ll6so29220141wic.1 for ; Wed, 04 Nov 2015 02:45:24 -0800 (PST) Sender: Paolo Bonzini References: <1446473134-4330-1-git-send-email-pbonzini@redhat.com> <563777D5.6050000@redhat.com> <5639DA14.3020507@twiddle.net> From: Paolo Bonzini Message-ID: <5639E1C2.80902@redhat.com> Date: Wed, 4 Nov 2015 11:45:22 +0100 MIME-Version: 1.0 In-Reply-To: <5639DA14.3020507@twiddle.net> 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: Richard Henderson , Peter Maydell Cc: Blue Swirl , Mark Cave-Ayland , QEMU Developers On 04/11/2015 11:12, Richard Henderson wrote: > On 11/02/2015 04:13 PM, Peter Maydell wrote: >> On 2 November 2015 at 14:48, Paolo Bonzini wrote: >>> >>> >>> On 02/11/2015 15:09, Peter Maydell wrote: >>>>>> diff --git a/target-sparc/vis_helper.c b/target-sparc/vis_helper.c >>>>>> index 383cc8b..45fc7db 100644 >>>>>> --- a/target-sparc/vis_helper.c >>>>>> +++ b/target-sparc/vis_helper.c >>>>>> @@ -447,7 +447,7 @@ uint32_t helper_fpackfix(uint64_t gsr, >>>>>> uint64_t rs2) >>>>>> for (word = 0; word < 2; word++) { >>>>>> uint32_t val; >>>>>> int32_t src = rs2 >> (word * 32); >>>>>> - int64_t scaled = src << scale; >>>>>> + int64_t scaled = (int64_t)src << scale; >>>>>> int64_t from_fixed = scaled >> 16; >>>> This will now shift left into the sign bit of a signed integer, >>>> which is undefined behaviour. >>> >>> Why "now"? It would have done the same before. >> >> True, but I was reviewing the new code rather than the >> code you were taking away :-) >> >> Incidentally, that manual says the fpackfix and fpack32 insns >> use a 4 bit GSR.scale_factor value, but our code is masking >> by 0x1f in helper_fpack32 and helper_fpackfix. Which is right? > > The 2011 manual has 5 bits for fpack32 and fpackfix; fpack16 uses only 4 > bits. > > 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. Paolo