From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:36965) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZtHN9-0002wx-6V for qemu-devel@nongnu.org; Mon, 02 Nov 2015 10:50:18 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1ZtHN4-0002Kw-Uk for qemu-devel@nongnu.org; Mon, 02 Nov 2015 10:50:15 -0500 Received: from mx1.redhat.com ([209.132.183.28]:42711) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZtHN4-0002Km-Ps for qemu-devel@nongnu.org; Mon, 02 Nov 2015 10:50:10 -0500 References: <1446473134-4330-1-git-send-email-pbonzini@redhat.com> <563777D5.6050000@redhat.com> From: Paolo Bonzini Message-ID: <5637862D.3060206@redhat.com> Date: Mon, 2 Nov 2015 16:50:05 +0100 MIME-Version: 1.0 In-Reply-To: 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: Peter Maydell Cc: Blue Swirl , Mark Cave-Ayland , QEMU Developers On 02/11/2015 16:13, 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? I don't know... That manual even says that GSR has no bit defined above bit 6 (where scale_factor is 3 to 6). It's possible that a newer processor defines a 5-bit scale factor; I honestly have no idea. Paolo