[Qemu-devel] [PATCH] target-s390x: fix CONVERT TO BINARY (CVD, CVDY)

[Qemu-devel] [PATCH] target-s390x: fix CONVERT TO BINARY (CVD, CVDY)

[Aurelien Jarno]

current_number being shift left by more than 32 bits, we can't use a
simple int. Similarly use an int64_t type for the input binary value,
to not get the -2^31 case wrong. Finally don't initialize shift to 4,
it's already done in the for loop.

Signed-off-by: Aurelien Jarno <aurelien@aurel32.net>
Cc: Alexander Graf <agraf@suse.de>
Cc: Richard Henderson <rth@twiddle.net>
---
 target-s390x/int_helper.c | 9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)

diff --git a/target-s390x/int_helper.c b/target-s390x/int_helper.c
index 2c2b3f6..a46c736 100644
--- a/target-s390x/int_helper.c
+++ b/target-s390x/int_helper.c
@@ -121,11 +121,12 @@ uint64_t HELPER(clz)(uint64_t v)
     return clz64(v);
 }
 
-uint64_t HELPER(cvd)(int32_t bin)
+uint64_t HELPER(cvd)(int32_t reg)
 {
     /* positive 0 */
     uint64_t dec = 0x0c;
-    int shift = 4;
+    int64_t bin = reg;
+    int shift;
 
     if (bin < 0) {
         bin = -bin;
@@ -133,9 +134,7 @@ uint64_t HELPER(cvd)(int32_t bin)
     }
 
     for (shift = 4; (shift < 64) && bin; shift += 4) {
-        int current_number = bin % 10;
-
-        dec |= (current_number) << shift;
+        dec |= (bin % 10) << shift;
         bin /= 10;
     }
 
-- 
2.1.4

Re: [Qemu-devel] [PATCH] target-s390x: fix CONVERT TO BINARY (CVD, CVDY)

[Richard Henderson]

On 06/25/2015 08:16 PM, Aurelien Jarno wrote:
>       for (shift = 4; (shift < 64) && bin; shift += 4) {
> -        int current_number = bin % 10;
> -
> -        dec |= (current_number) << shift;
> +        dec |= (bin % 10) << shift;
>           bin /= 10;
>       }

You've changed from 32-bit division to 64-bit division just to solve a problem 
with the shift.  Better to just change the type of current_number there.


r~

Re: [Qemu-devel] [PATCH] target-s390x: fix CONVERT TO BINARY (CVD, CVDY)

[Aurelien Jarno]

On 2015-06-29 10:28, Richard Henderson wrote:
> On 06/25/2015 08:16 PM, Aurelien Jarno wrote:
> >      for (shift = 4; (shift < 64) && bin; shift += 4) {
> >-        int current_number = bin % 10;
> >-
> >-        dec |= (current_number) << shift;
> >+        dec |= (bin % 10) << shift;
> >          bin /= 10;
> >      }
> 
> You've changed from 32-bit division to 64-bit division just to solve a
> problem with the shift.  Better to just change the type of current_number
> there.

Changing the type of current_number instead of bin would indeed solve
the shift issue, but not the -2^31 case. As we take the absolute value,
we need a 64-bit variable to hold the corresponding 2^31 value.

-- 
Aurelien Jarno                          GPG: 4096R/1DDD8C9B
aurelien@aurel32.net                 http://www.aurel32.net

Re: [Qemu-devel] [PATCH] target-s390x: fix CONVERT TO BINARY (CVD, CVDY)

[Richard Henderson]

On 06/29/2015 09:24 PM, Aurelien Jarno wrote:
> On 2015-06-29 10:28, Richard Henderson wrote:
>> On 06/25/2015 08:16 PM, Aurelien Jarno wrote:
>>>       for (shift = 4; (shift < 64) && bin; shift += 4) {
>>> -        int current_number = bin % 10;
>>> -
>>> -        dec |= (current_number) << shift;
>>> +        dec |= (bin % 10) << shift;
>>>           bin /= 10;
>>>       }
>>
>> You've changed from 32-bit division to 64-bit division just to solve a
>> problem with the shift.  Better to just change the type of current_number
>> there.
>
> Changing the type of current_number instead of bin would indeed solve
> the shift issue, but not the -2^31 case. As we take the absolute value,
> we need a 64-bit variable to hold the corresponding 2^31 value.
>

Ah, true enough.  I suppose adding a 32-bit unsigned variable with which to do 
the division is more trouble than it's worth.

Reviewed-by: Richard Henderson <rth@twiddle.net>


r~

Re: [Qemu-devel] [PATCH] target-s390x: fix CONVERT TO BINARY (CVD, CVDY)

[Alexander Graf]

On 30.06.15 07:53, Richard Henderson wrote:
> On 06/29/2015 09:24 PM, Aurelien Jarno wrote:
>> On 2015-06-29 10:28, Richard Henderson wrote:
>>> On 06/25/2015 08:16 PM, Aurelien Jarno wrote:
>>>>       for (shift = 4; (shift < 64) && bin; shift += 4) {
>>>> -        int current_number = bin % 10;
>>>> -
>>>> -        dec |= (current_number) << shift;
>>>> +        dec |= (bin % 10) << shift;
>>>>           bin /= 10;
>>>>       }
>>>
>>> You've changed from 32-bit division to 64-bit division just to solve a
>>> problem with the shift.  Better to just change the type of
>>> current_number
>>> there.
>>
>> Changing the type of current_number instead of bin would indeed solve
>> the shift issue, but not the -2^31 case. As we take the absolute value,
>> we need a 64-bit variable to hold the corresponding 2^31 value.
>>
> 
> Ah, true enough.  I suppose adding a 32-bit unsigned variable with which
> to do the division is more trouble than it's worth.
> 
> Reviewed-by: Richard Henderson <rth@twiddle.net>

Thanks, applied to s390-next.


Alex