From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:33341) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Ztv3j-0003BJ-84 for qemu-devel@nongnu.org; Wed, 04 Nov 2015 05:12:52 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Ztv3f-000666-O7 for qemu-devel@nongnu.org; Wed, 04 Nov 2015 05:12:51 -0500 Received: from mail-wm0-x22a.google.com ([2a00:1450:400c:c09::22a]:38825) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Ztv3f-000662-HU for qemu-devel@nongnu.org; Wed, 04 Nov 2015 05:12:47 -0500 Received: by wmeg8 with SMTP id g8so37124875wme.1 for ; Wed, 04 Nov 2015 02:12:46 -0800 (PST) Sender: Richard Henderson References: <1446473134-4330-1-git-send-email-pbonzini@redhat.com> <563777D5.6050000@redhat.com> From: Richard Henderson Message-ID: <5639DA14.3020507@twiddle.net> Date: Wed, 4 Nov 2015 11:12:36 +0100 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8; format=flowed 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 , Paolo Bonzini Cc: Blue Swirl , Mark Cave-Ayland , QEMU Developers 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. r~