Message-ID: <532BE51E.7060208@ilande.co.uk>
Date: Fri, 21 Mar 2014 07:07:10 +0000
From: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
MIME-Version: 1.0
References: <532B94FF.4090809@caramail.com>
In-Reply-To: <532B94FF.4090809@caramail.com>
Content-Type: text/plain; charset=ISO-8859-1; format=flowed
Content-Transfer-Encoding: 7bit
Subject: Re: [Qemu-devel] [PATCH v3] sparc : 32bits integer division overflow
To: Olivier Danet <odanet@caramail.com>
Cc: Blue Swirl <blauwirbel@gmail.com>, qemu-devel <qemu-devel@nongnu.org>, Richard Henderson <rth@twiddle.net>

On 21/03/14 01:25, Olivier Danet wrote:

> The signed integer division -0x8000_0000_0000_0000 / -1 must be handled
> separately to avoid an overflow on the QEMU host.
>
> Negative overflow must be a negative number for correct sign
> extension in Sparc64 mode. Use<stdint.h>  constants.
>
> Signed-off-by: Olivier Danet<odanet@caramail.com>
> ---
>   target-sparc/helper.c | 17 ++++++++++-------
>   1 file changed, 10 insertions(+), 7 deletions(-)
>
> diff --git a/target-sparc/helper.c b/target-sparc/helper.c
> index f3c7fbf..ae7740b 100644
> --- a/target-sparc/helper.c
> +++ b/target-sparc/helper.c
> @@ -85,8 +85,8 @@ static target_ulong helper_udiv_common(CPUSPARCState *env, target_ulong a,
>       }
>
>       x0 = x0 / x1;
> -    if (x0>  0xffffffff) {
> -        x0 = 0xffffffff;
> +    if (x0>  UINT32_MAX) {
> +        x0 = UINT32_MAX;
>           overflow = 1;
>       }
>
> @@ -122,12 +122,15 @@ static target_ulong helper_sdiv_common(CPUSPARCState *env, target_ulong a,
>       if (x1 == 0) {
>           cpu_restore_state(CPU(cpu), GETPC());
>           helper_raise_exception(env, TT_DIV_ZERO);
> -    }
> -
> -    x0 = x0 / x1;
> -    if ((int32_t) x0 != x0) {
> -        x0 = x0<  0 ? 0x80000000 : 0x7fffffff;
> +    } else if (x1 == -1&&  x0 == INT64_MIN) {
> +        x0 = INT32_MAX;
>           overflow = 1;
> +    } else {
> +        x0 = x0 / x1;
> +        if ((int32_t) x0 != x0) {
> +            x0 = x0<  0 ? INT32_MIN : INT32_MAX;
> +            overflow = 1;
> +        }
>       }
>
>       if (cc) {

Hi Olivier,

This basic patch looks good to me. My only comment is that I suspect for 
bisection purposes it may be better to split this into 2 patches - one 
to perform the conversion of all existing constants to INT*_MAX and 
INT*_MIN, and then a second to add your change to prevent the crash. 
I'll let Richard have the final say though.

Having said that, I will definitely give it a test over the next couple 
of days when I get a moment.


ATB,

Mark.