* include/fpu/softfloat.h (floatx80_invalid_encoding): Handle m68k specific infinity pattern.

Commit Message

Pierre Muller Sept. 17, 2019, 8:04 p.m. UTC

Hello,

  I submitted September 12. a bug report about wrong handling of
infinity values for m68k emulator.

https://bugs.launchpad.net/qemu/+bug/1843651

  The analysis I made in the bug report is wrong, because
I did not know that m68k FPU really uses 80-bit representations internally.
  It thus seems now to me that the m68k specific floatx80_infinity_low
corresponds to a different internal encoding of infinity on m68k floating
point units, different from the one of the usual x87 floating point unit.
  The main problem still remains that the function floatx80_invalid_encoding
does not properly handle those m68k infinity patterns.

  The patch below seems to fix the reported problem on current git.

Pierre Muller
Member of core development team of Free Pascal compiler


From e053d3d07b1951faf0b4637ce1ec4194b8d952e5 Mon Sep 17 00:00:00 2001
From: Pierre Muller <[e-mail]>
Date: Thu, 12 Sep 2019 08:10:48 +0000
Subject: [PATCH]        * include/fpu/softfloat.h (floatx80_invalid_encoding):
 Handle         m68k specific infinity pattern.

---
 include/fpu/softfloat.h | 7 +++++++
 1 file changed, 7 insertions(+)

--
2.20.1

Comments

Thomas Huth Sept. 18, 2019, 7:26 a.m. UTC | #1

On 17/09/2019 22.04, Pierre Muller wrote:
> 
> Hello,
> 
>   I submitted September 12. a bug report about wrong handling of
> infinity values for m68k emulator.
> 
> https://bugs.launchpad.net/qemu/+bug/1843651
> 
>   The analysis I made in the bug report is wrong, because
> I did not know that m68k FPU really uses 80-bit representations internally.
>   It thus seems now to me that the m68k specific floatx80_infinity_low
> corresponds to a different internal encoding of infinity on m68k floating
> point units, different from the one of the usual x87 floating point unit.
>   The main problem still remains that the function floatx80_invalid_encoding
> does not properly handle those m68k infinity patterns.
> 
>   The patch below seems to fix the reported problem on current git.
> 
> Pierre Muller
> Member of core development team of Free Pascal compiler
> 
> 
> From e053d3d07b1951faf0b4637ce1ec4194b8d952e5 Mon Sep 17 00:00:00 2001
> From: Pierre Muller <[e-mail]>
> Date: Thu, 12 Sep 2019 08:10:48 +0000
> Subject: [PATCH]        * include/fpu/softfloat.h (floatx80_invalid_encoding):
>  Handle         m68k specific infinity pattern.

 Hi Pierre,

thanks a lot for the patch! But please note that the QEMU project has
some constraints for patches that have to be followed before a patch can
be applied.

Most important one: You need to provide a "Signed-off-by:" line in the
patch description to make sure that you agree with the Developer
Certificate Of Origin. See this URL for more details:

 https://wiki.qemu.org/Contribute/SubmitAPatch

Then it would be nice if you add some proper commit message to the patch
(something similar to the comment that you've added to the source code
would do the job, I guess).

And finally, please note that qemu-devel is a high traffic mailing list.
When sending patches, you best make sure to put some maintainers on CC:,
or your patch might get lost in the high traffic. You can either have a
look at the MAINTAINERS file in the main directory, or use the
scripts/get_maintainers.pl script on your patch to see who should be put
on CC:.

 Thanks,
  Thomas


> ---
>  include/fpu/softfloat.h | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/include/fpu/softfloat.h b/include/fpu/softfloat.h
> index ecb8ba0114..538c35767e 100644
> --- a/include/fpu/softfloat.h
> +++ b/include/fpu/softfloat.h
> @@ -685,10 +685,17 @@ static inline int floatx80_is_any_nan(floatx80 a)
>  | pseudo-infinities and un-normal numbers. It does not include
>  | pseudo-denormals, which must still be correctly handled as inputs even
>  | if they are never generated as outputs.
> +| As m68k uses a different pattern for infinity, thus an additional test
> +| for valid infinity value must be added for m68k CPU.
>  *----------------------------------------------------------------------------*/
>  static inline bool floatx80_invalid_encoding(floatx80 a)
>  {
> +#if defined (TARGET_M68K)
> +    return ((a.low & (1ULL << 63)) == 0 && (a.high & 0x7FFF) != 0)
> +           && (! floatx80_is_infinity(a));
> +#else
>      return (a.low & (1ULL << 63)) == 0 && (a.high & 0x7FFF) != 0;
> +#endif
>  }
> 
>  #define floatx80_zero make_floatx80(0x0000, 0x0000000000000000LL)
> --
> 2.20.1
> 
>

Pierre Muller Sept. 18, 2019, 8:20 a.m. UTC | #2

Hi Thomas,

  I tried to use git format-patch -s below,
and change the commit message that appears below:


muller@gcc123:~/gnu/qemu/qemu$ git format-patch -s a017dc6d43aaa4ffc7be40ae3adee4086be9cec2^
0001-Fix-floatx80_invalid_encoding-function-for-m68k-cpu.patch
muller@gcc123:~/gnu/qemu/qemu$ cat 0001-Fix-floatx80_invalid_encoding-function-for-m68k-cpu.patch
From a017dc6d43aaa4ffc7be40ae3adee4086be9cec2 Mon Sep 17 00:00:00 2001
From: Pierre Muller <[e-mail]>
Date: Wed, 18 Sep 2019 08:04:19 +0000
Subject: [PATCH]    Fix floatx80_invalid_encoding function for m68k cpu

    As m68k accepts different patterns for infinity,
    and additional test for valid infinity must be added
    for m68k cpu target.

Signed-off-by: Pierre Muller <[e-mail]>
---
 include/fpu/softfloat.h | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/include/fpu/softfloat.h b/include/fpu/softfloat.h
index ecb8ba0114..dea24384e9 100644
--- a/include/fpu/softfloat.h
+++ b/include/fpu/softfloat.h
@@ -685,10 +685,17 @@ static inline int floatx80_is_any_nan(floatx80 a)
 | pseudo-infinities and un-normal numbers. It does not include
 | pseudo-denormals, which must still be correctly handled as inputs even
 | if they are never generated as outputs.
+| As m68k accepts different patterns for infinity, thus an additional test
+| for valid infinity value must be added for m68k CPU.
 *----------------------------------------------------------------------------*/
 static inline bool floatx80_invalid_encoding(floatx80 a)
 {
+#if defined (TARGET_M68K)
+    return ((a.low & (1ULL << 63)) == 0 && (a.high & 0x7FFF) != 0)
+           && (! floatx80_is_infinity(a));
+#else
     return (a.low & (1ULL << 63)) == 0 && (a.high & 0x7FFF) != 0;
+#endif
 }

 #define floatx80_zero make_floatx80(0x0000, 0x0000000000000000LL)
--
2.20.1

I hope this is correct according to the guidelines.


Le 18/09/2019 à 09:26, Thomas Huth a écrit :
> On 17/09/2019 22.04, Pierre Muller wrote:
>>
>> Hello,
....
> 
>  Hi Pierre,
> 
> thanks a lot for the patch! But please note that the QEMU project has
> some constraints for patches that have to be followed before a patch can
> be applied.
> 
> Most important one: You need to provide a "Signed-off-by:" line in the
> patch description to make sure that you agree with the Developer
> Certificate Of Origin. See this URL for more details:
> 
>  https://wiki.qemu.org/Contribute/SubmitAPatch

   I tried to follow this guide.

> Then it would be nice if you add some proper commit message to the patch
> (something similar to the comment that you've added to the source code
> would do the job, I guess).

  I hope the above is OK.

> And finally, please note that qemu-devel is a high traffic mailing list.
> When sending patches, you best make sure to put some maintainers on CC:,
> or your patch might get lost in the high traffic. You can either have a
> look at the MAINTAINERS file in the main directory, or use the
> scripts/get_maintainers.pl script on your patch to see who should be put
> on CC:.

Thus I took the liberty to use 'Reply to all' as you have
added several persons which are listed using
./scripts/get_maintainer.pl -f  include/fpu/softfloat.h
and Laurent who is more specifically involved in m68k support.

Please let me know if more is needed to get this patch accepted.

Pierre Muller

Alex Bennée Sept. 18, 2019, 9:59 a.m. UTC | #3

Pierre Muller <[e-mail]> writes:

>   Hi Thomas,
>
>   I tried to use git format-patch -s below,
> and change the commit message that appears below:
>
>
> muller@gcc123:~/gnu/qemu/qemu$ git format-patch -s a017dc6d43aaa4ffc7be40ae3adee4086be9cec2^
> 0001-Fix-floatx80_invalid_encoding-function-for-m68k-cpu.patch
> muller@gcc123:~/gnu/qemu/qemu$ cat
> 0001-Fix-floatx80_invalid_encoding-function-for-m68k-cpu.patch

It's best to send the patches directly (i.e. don't include them in a
larger email). This is because when maintainers apply the email they end
up with a bunch of additional stuff and the corrupting of subject line.

> From a017dc6d43aaa4ffc7be40ae3adee4086be9cec2 Mon Sep 17 00:00:00 2001
> From: Pierre Muller <[e-mail]>
> Date: Wed, 18 Sep 2019 08:04:19 +0000
> Subject: [PATCH]    Fix floatx80_invalid_encoding function for m68k cpu
>
>     As m68k accepts different patterns for infinity,
>     and additional test for valid infinity must be added
>     for m68k cpu target.
>
> Signed-off-by: Pierre Muller <[e-mail]>
> ---
>  include/fpu/softfloat.h | 7 +++++++
>  1 file changed, 7 insertions(+)
>
> diff --git a/include/fpu/softfloat.h b/include/fpu/softfloat.h
> index ecb8ba0114..dea24384e9 100644
> --- a/include/fpu/softfloat.h
> +++ b/include/fpu/softfloat.h
> @@ -685,10 +685,17 @@ static inline int floatx80_is_any_nan(floatx80 a)
>  | pseudo-infinities and un-normal numbers. It does not include
>  | pseudo-denormals, which must still be correctly handled as inputs even
>  | if they are never generated as outputs.
> +| As m68k accepts different patterns for infinity, thus an additional test
> +| for valid infinity value must be added for m68k CPU.
>  *----------------------------------------------------------------------------*/
>  static inline bool floatx80_invalid_encoding(floatx80 a)
>  {
> +#if defined (TARGET_M68K)
> +    return ((a.low & (1ULL << 63)) == 0 && (a.high & 0x7FFF) != 0)
> +           && (! floatx80_is_infinity(a));
> +#else
>      return (a.low & (1ULL << 63)) == 0 && (a.high & 0x7FFF) != 0;
> +#endif
>  }

As most of the test is the same we could rewrite this to:

 bool invalid = (a.low & (1ULL << 63)) == 0 && (a.high & 0x7FFF) != 0;
 #if defined (TARGET_M68K)
 invalid &= !floatx80_is_infinity(a)
 #endif
 return invalid;

The compiler should be able to simplify the logic from there.

Do we have any test cases that we could add to tests/tcg/m68k?

>
>  #define floatx80_zero make_floatx80(0x0000, 0x0000000000000000LL)


--
Alex Bennée

Laurent Vivier Sept. 18, 2019, 10:08 a.m. UTC | #4

Le 18/09/2019 à 11:59, Alex Bennée a écrit :
> 
> Pierre Muller <[e-mail]> writes:
> 
>>   Hi Thomas,
>>
>>   I tried to use git format-patch -s below,
>> and change the commit message that appears below:
>>
>>
>> muller@gcc123:~/gnu/qemu/qemu$ git format-patch -s a017dc6d43aaa4ffc7be40ae3adee4086be9cec2^
>> 0001-Fix-floatx80_invalid_encoding-function-for-m68k-cpu.patch
>> muller@gcc123:~/gnu/qemu/qemu$ cat
>> 0001-Fix-floatx80_invalid_encoding-function-for-m68k-cpu.patch
> 
> It's best to send the patches directly (i.e. don't include them in a
> larger email). This is because when maintainers apply the email they end
> up with a bunch of additional stuff and the corrupting of subject line.
> 
>> From a017dc6d43aaa4ffc7be40ae3adee4086be9cec2 Mon Sep 17 00:00:00 2001
>> From: Pierre Muller <[e-mail]>
>> Date: Wed, 18 Sep 2019 08:04:19 +0000
>> Subject: [PATCH]    Fix floatx80_invalid_encoding function for m68k cpu
>>
>>     As m68k accepts different patterns for infinity,
>>     and additional test for valid infinity must be added
>>     for m68k cpu target.
>>
>> Signed-off-by: Pierre Muller <[e-mail]>
>> ---
>>  include/fpu/softfloat.h | 7 +++++++
>>  1 file changed, 7 insertions(+)
>>
>> diff --git a/include/fpu/softfloat.h b/include/fpu/softfloat.h
>> index ecb8ba0114..dea24384e9 100644
>> --- a/include/fpu/softfloat.h
>> +++ b/include/fpu/softfloat.h
>> @@ -685,10 +685,17 @@ static inline int floatx80_is_any_nan(floatx80 a)
>>  | pseudo-infinities and un-normal numbers. It does not include
>>  | pseudo-denormals, which must still be correctly handled as inputs even
>>  | if they are never generated as outputs.
>> +| As m68k accepts different patterns for infinity, thus an additional test
>> +| for valid infinity value must be added for m68k CPU.
>>  *----------------------------------------------------------------------------*/
>>  static inline bool floatx80_invalid_encoding(floatx80 a)
>>  {
>> +#if defined (TARGET_M68K)
>> +    return ((a.low & (1ULL << 63)) == 0 && (a.high & 0x7FFF) != 0)
>> +           && (! floatx80_is_infinity(a));
>> +#else
>>      return (a.low & (1ULL << 63)) == 0 && (a.high & 0x7FFF) != 0;
>> +#endif
>>  }
> 
> As most of the test is the same we could rewrite this to:
> 
>  bool invalid = (a.low & (1ULL << 63)) == 0 && (a.high & 0x7FFF) != 0;
>  #if defined (TARGET_M68K)
>  invalid &= !floatx80_is_infinity(a)
>  #endif
>  return invalid;
> 
> The compiler should be able to simplify the logic from there.
> 
> Do we have any test cases that we could add to tests/tcg/m68k?
> 
>>
>>  #define floatx80_zero make_floatx80(0x0000, 0x0000000000000000LL)
> 

I think patch from Pierre doesn't treat all the problem and don't rely 
on any specification.

I proposed a patch years ago that is closer to the hardware behaviour:

/*----------------------------------------------------------------------------
| Return whether the given value is an invalid floatx80 encoding.
| Invalid floatx80 encodings arise when the integer bit is not set, but
| the exponent is not zero. The only times the integer bit is permitted to
| be zero is in subnormal numbers and the value zero.
| This includes what the Intel software developer's manual calls pseudo-NaNs,
| pseudo-infinities and un-normal numbers. It does not include
| pseudo-denormals, which must still be correctly handled as inputs even
| if they are never generated as outputs.
*----------------------------------------------------------------------------*/
static inline bool floatx80_invalid_encoding(floatx80 a)
{
#if defined(TARGET_M68K)
    /*-------------------------------------------------------------------------
    |  M68000 FAMILY PROGRAMMER’S REFERENCE MANUAL
    |  1.6.2 Denormalized Numbers
    |  Since the extended-precision data format has an explicit integer bit,
    |  a number can be formatted with a nonzero exponent, less than the maximum
    |  value, and a zero integer bit.  The IEEE 754 standard does not define a
    |  zero integer bit. Such a number is an unnormalized number. Hardware does
    |  not directly support denormalized and unnormalized numbers, but
    |  implicitly supports them by trapping them as unimplemented data types,
    |  allowing efficient conversion in software.
    *------------------------------------------------------------------------*/
    return 0;
#else
    return (a.low & (1ULL << 63)) == 0 && (a.high & 0x7FFF) != 0;
#endif
}

But in fact I think this kind of number should raise an exception.

I tried to do this in:

https://github.com/vivier/qemu-m68k/commit/5bd7742437a3c770373734add5ab452e8df4e621

but it needs more work and for the moment doesn't fix the problem Pierre is trying to fix.

Thanks,
Laurent

[e-mail] Sept. 18, 2019, 10:26 a.m. UTC | #5

Patchew URL: https://patchew.org/QEMU/[e-mail]/



Hi,

This series seems to have some coding style problems. See output below for
more information:

Subject: [Qemu-devel] [PATCH] * include/fpu/softfloat.h (floatx80_invalid_encoding): Handle m68k specific infinity pattern.
Message-id: [e-mail]
Type: series

=== TEST SCRIPT BEGIN ===
#!/bin/bash
git rev-parse base > /dev/null || exit 0
git config --local diff.renamelimit 0
git config --local diff.renames True
git config --local diff.algorithm histogram
./scripts/checkpatch.pl --mailback base..
=== TEST SCRIPT END ===

Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
From https://github.com/patchew-project/qemu
 * [new tag]         patchew/[e-mail] -> patchew/[e-mail]
 * [new tag]         patchew/[e-mail] -> patchew/[e-mail]
Switched to a new branch 'test'
0e0d352 * include/fpu/softfloat.h (floatx80_invalid_encoding): Handle m68k specific infinity pattern.

=== OUTPUT BEGIN ===
ERROR: space prohibited between function name and open parenthesis '('
#48: FILE: include/fpu/softfloat.h:693:
+#if defined (TARGET_M68K)

ERROR: space prohibited after that '!' (ctx:BxW)
#50: FILE: include/fpu/softfloat.h:695:
+           && (! floatx80_is_infinity(a));
                ^

ERROR: Missing Signed-off-by: line(s)

total: 3 errors, 0 warnings, 17 lines checked

Commit 0e0d352d93d2 (* include/fpu/softfloat.h (floatx80_invalid_encoding): Handle m68k specific infinity pattern.) has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.
=== OUTPUT END ===

Test command exited with code: 1


The full log is available at
http://patchew.org/logs/[e-mail]/testing.checkpatch/?type=message.
---
Email generated automatically by Patchew [https://patchew.org/].
Please send your feedback to [e-mail]

Patch

diff --git a/include/fpu/softfloat.h b/include/fpu/softfloat.h
index ecb8ba0114..538c35767e 100644
--- a/include/fpu/softfloat.h
+++ b/include/fpu/softfloat.h
@@ -685,10 +685,17 @@  static inline int floatx80_is_any_nan(floatx80 a)
 | pseudo-infinities and un-normal numbers. It does not include
 | pseudo-denormals, which must still be correctly handled as inputs even
 | if they are never generated as outputs.
+| As m68k uses a different pattern for infinity, thus an additional test
+| for valid infinity value must be added for m68k CPU.
 *----------------------------------------------------------------------------*/
 static inline bool floatx80_invalid_encoding(floatx80 a)
 {
+#if defined (TARGET_M68K)
+    return ((a.low & (1ULL << 63)) == 0 && (a.high & 0x7FFF) != 0)
+           && (! floatx80_is_infinity(a));
+#else
     return (a.low & (1ULL << 63)) == 0 && (a.high & 0x7FFF) != 0;
+#endif
 }

 #define floatx80_zero make_floatx80(0x0000, 0x0000000000000000LL)