target/i386: fxtract, fscale fixes

[PATCH 0/5] target/i386: fxtract, fscale fixes

Posted by Joseph Myers 3 years, 12 months ago

Among the various bugs in the x87 floating-point emulation that show
up through a combination of glibc testing and code inspection, there
are several in the implementations of the fxtract and fscale
instructions.  This series fixes those bugs.

Bugs in other instructions, and bugs relating to floating-point
exceptions and flag setting, will be addressed separately.  In
particular, while some of these patches add code that sets exception
flags in the softfloat state, it's generally the case that the x87
emulation ignores exceptions in that state rather than propagating
them to the status word (and to generating traps where appropriate).
I intend to address that missing propagation of exceptions in a
subsequent patch series; until it's addressed, the code setting
exceptions won't actually do anything useful.  (There is also code in
the x87 emulation, including that of fscale, that would result in
spurious exceptions being set from a naive propagation of exceptions
from the softfloat state, and thus will need updating to avoid
propagating inappropriate exceptions when such propagation is
implemented.)

Joseph Myers (5):
  target/i386: implement special cases for fxtract
  target/i386: fix fscale handling of signaling NaN
  target/i386: fix fscale handling of invalid exponent encodings
  target/i386: fix fscale handling of infinite exponents
  target/i386: fix fscale handling of rounding precision

 target/i386/fpu_helper.c           |  59 +++++++++++++-
 tests/tcg/i386/test-i386-fscale.c  | 108 ++++++++++++++++++++++++++
 tests/tcg/i386/test-i386-fxtract.c | 120 +++++++++++++++++++++++++++++
 3 files changed, 285 insertions(+), 2 deletions(-)
 create mode 100644 tests/tcg/i386/test-i386-fscale.c
 create mode 100644 tests/tcg/i386/test-i386-fxtract.c

-- 
2.17.1


-- 
Joseph S. Myers
joseph@codesourcery.com

Re: [PATCH 0/5] target/i386: fxtract, fscale fixes

Posted by Paolo Bonzini 3 years, 11 months ago

On 07/05/20 02:42, Joseph Myers wrote:
> Among the various bugs in the x87 floating-point emulation that show
> up through a combination of glibc testing and code inspection, there
> are several in the implementations of the fxtract and fscale
> instructions.  This series fixes those bugs.
> 
> Bugs in other instructions, and bugs relating to floating-point
> exceptions and flag setting, will be addressed separately.  In
> particular, while some of these patches add code that sets exception
> flags in the softfloat state, it's generally the case that the x87
> emulation ignores exceptions in that state rather than propagating
> them to the status word (and to generating traps where appropriate).
> I intend to address that missing propagation of exceptions in a
> subsequent patch series; until it's addressed, the code setting
> exceptions won't actually do anything useful.  (There is also code in
> the x87 emulation, including that of fscale, that would result in
> spurious exceptions being set from a naive propagation of exceptions
> from the softfloat state, and thus will need updating to avoid
> propagating inappropriate exceptions when such propagation is
> implemented.)
> 
> Joseph Myers (5):
>   target/i386: implement special cases for fxtract
>   target/i386: fix fscale handling of signaling NaN
>   target/i386: fix fscale handling of invalid exponent encodings
>   target/i386: fix fscale handling of infinite exponents
>   target/i386: fix fscale handling of rounding precision
> 
>  target/i386/fpu_helper.c           |  59 +++++++++++++-
>  tests/tcg/i386/test-i386-fscale.c  | 108 ++++++++++++++++++++++++++
>  tests/tcg/i386/test-i386-fxtract.c | 120 +++++++++++++++++++++++++++++
>  3 files changed, 285 insertions(+), 2 deletions(-)
>  create mode 100644 tests/tcg/i386/test-i386-fscale.c
>  create mode 100644 tests/tcg/i386/test-i386-fxtract.c
> 

Queued, thanks.

Paolo

Re: [PATCH 0/5] target/i386: fxtract, fscale fixes

Posted by no-reply@patchew.org 3 years, 12 months ago

Patchew URL: https://patchew.org/QEMU/alpine.DEB.2.21.2005070038550.18350@digraph.polyomino.org.uk/



Hi,

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

Message-id: alpine.DEB.2.21.2005070038550.18350@digraph.polyomino.org.uk
Subject: [PATCH 0/5] target/i386: fxtract, fscale fixes
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 ===

Switched to a new branch 'test'
ef3dfb7 target/i386: fix fscale handling of rounding precision
0ef4ac9 target/i386: fix fscale handling of infinite exponents
9c12341 target/i386: fix fscale handling of invalid exponent encodings
aac0b0b target/i386: fix fscale handling of signaling NaN
69eed0b target/i386: implement special cases for fxtract

=== OUTPUT BEGIN ===
1/5 Checking commit 69eed0bcaaaf (target/i386: implement special cases for fxtract)
WARNING: added, moved or deleted file(s), does MAINTAINERS need updating?
#55: 
new file mode 100644

ERROR: Use of volatile is usually wrong, please add a comment
#70: FILE: tests/tcg/i386/test-i386-fxtract.c:11:
+volatile union u ld_pseudo_m16382 = { .s = { UINT64_C(1) << 63, 0 } };

ERROR: Use of volatile is usually wrong, please add a comment
#71: FILE: tests/tcg/i386/test-i386-fxtract.c:12:
+volatile union u ld_invalid_1 = { .s = { 1, 1234 } };

ERROR: Use of volatile is usually wrong, please add a comment
#72: FILE: tests/tcg/i386/test-i386-fxtract.c:13:
+volatile union u ld_invalid_2 = { .s = { 0, 1234 } };

ERROR: Use of volatile is usually wrong, please add a comment
#73: FILE: tests/tcg/i386/test-i386-fxtract.c:14:
+volatile union u ld_invalid_3 = { .s = { 0, 0x7fff } };

ERROR: Use of volatile is usually wrong, please add a comment
#74: FILE: tests/tcg/i386/test-i386-fxtract.c:15:
+volatile union u ld_invalid_4 = { .s = { (UINT64_C(1) << 63) - 1, 0x7fff } };

ERROR: Use of volatile is usually wrong, please add a comment
#76: FILE: tests/tcg/i386/test-i386-fxtract.c:17:
+volatile long double ld_sig, ld_exp;

ERROR: spaces required around that '-' (ctx:VxV)
#139: FILE: tests/tcg/i386/test-i386-fxtract.c:80:
+                      "0" (0x1p-16445L));
                                ^

total: 7 errors, 1 warnings, 154 lines checked

Patch 1/5 has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.

2/5 Checking commit aac0b0b6881b (target/i386: fix fscale handling of signaling NaN)
WARNING: added, moved or deleted file(s), does MAINTAINERS need updating?
#30: 
new file mode 100644

ERROR: Use of volatile is usually wrong, please add a comment
#45: FILE: tests/tcg/i386/test-i386-fscale.c:11:
+volatile long double ld_res;

total: 1 errors, 1 warnings, 47 lines checked

Patch 2/5 has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.

3/5 Checking commit 9c123418e935 (target/i386: fix fscale handling of invalid exponent encodings)
ERROR: Use of volatile is usually wrong, please add a comment
#40: FILE: tests/tcg/i386/test-i386-fscale.c:11:
+volatile union u ld_invalid_1 = { .s = { 1, 1234 } };

ERROR: Use of volatile is usually wrong, please add a comment
#41: FILE: tests/tcg/i386/test-i386-fscale.c:12:
+volatile union u ld_invalid_2 = { .s = { 0, 1234 } };

ERROR: Use of volatile is usually wrong, please add a comment
#42: FILE: tests/tcg/i386/test-i386-fscale.c:13:
+volatile union u ld_invalid_3 = { .s = { 0, 0x7fff } };

ERROR: Use of volatile is usually wrong, please add a comment
#43: FILE: tests/tcg/i386/test-i386-fscale.c:14:
+volatile union u ld_invalid_4 = { .s = { (UINT64_C(1) << 63) - 1, 0x7fff } };

total: 4 errors, 0 warnings, 51 lines checked

Patch 3/5 has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.

4/5 Checking commit 0ef4ac9a50a5 (target/i386: fix fscale handling of infinite exponents)
5/5 Checking commit ef3dfb7e7c89 (target/i386: fix fscale handling of rounding precision)
ERROR: Use of volatile is usually wrong, please add a comment
#41: FILE: tests/tcg/i386/test-i386-fscale.c:11:
+volatile long double ld_third = 1.0L / 3.0L;

ERROR: Use of volatile is usually wrong, please add a comment
#42: FILE: tests/tcg/i386/test-i386-fscale.c:12:
+volatile long double ld_four_thirds = 4.0L / 3.0L;

total: 2 errors, 0 warnings, 34 lines checked

Patch 5/5 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/alpine.DEB.2.21.2005070038550.18350@digraph.polyomino.org.uk/testing.checkpatch/?type=message.
---
Email generated automatically by Patchew [https://patchew.org/].
Please send your feedback to patchew-devel@redhat.com

Re: [PATCH 0/5] target/i386: fxtract, fscale fixes

Posted by Joseph Myers 3 years, 12 months ago

On Thu, 7 May 2020, no-reply@patchew.org wrote:

> === OUTPUT BEGIN ===
> 1/5 Checking commit 69eed0bcaaaf (target/i386: implement special cases for fxtract)
> WARNING: added, moved or deleted file(s), does MAINTAINERS need updating?

I don't think any MAINTAINERS update is needed for a new testcase in an 
existing directory.

> ERROR: Use of volatile is usually wrong, please add a comment

I think the justification for volatile in such testcase code is obvious 
without comments in individual cases - to avoid any code movement or 
optimization that might break what the tests are intending to test (these 
tests are making heavy use of mixed C and inline asm to test how emulated 
instructions behave, including on input representations that are not valid 
long double values in the ABI and with the rounding precision changed 
behind the compiler's back).  I think making everything possibly relevant 
volatile in these tests is better than trying to produce a fragile 
argument that in fact certain data does not need to be volatile to avoid 
problematic code movement.

> ERROR: spaces required around that '-' (ctx:VxV)
> #139: FILE: tests/tcg/i386/test-i386-fxtract.c:80:
> +                      "0" (0x1p-16445L));
>                                 ^

No, this is a C99 hex float contstant, not a subtraction.  There are 
already such constants in tests/tcg/multiarch/float_helpers.c and 
tests/tcg/multiarch/float_madds.c at least, so I assume they are OK in 
QEMU floating-point tests and this style checker should not be objecting 
to them.

-- 
Joseph S. Myers
joseph@codesourcery.com

Re: [Patchew-devel] [PATCH 0/5] target/i386: fxtract, fscale fixes

Posted by Richard Henderson 3 years, 12 months ago

On 5/7/20 7:57 AM, Joseph Myers wrote:
> On Thu, 7 May 2020, no-reply@patchew.org wrote:
> 
>> === OUTPUT BEGIN ===
>> 1/5 Checking commit 69eed0bcaaaf (target/i386: implement special cases for fxtract)
>> WARNING: added, moved or deleted file(s), does MAINTAINERS need updating?
> 
> I don't think any MAINTAINERS update is needed for a new testcase in an 
> existing directory.
> 
>> ERROR: Use of volatile is usually wrong, please add a comment
> 
> I think the justification for volatile in such testcase code is obvious 
> without comments in individual cases - to avoid any code movement or 
> optimization that might break what the tests are intending to test (these 
> tests are making heavy use of mixed C and inline asm to test how emulated 
> instructions behave, including on input representations that are not valid 
> long double values in the ABI and with the rounding precision changed 
> behind the compiler's back).  I think making everything possibly relevant 
> volatile in these tests is better than trying to produce a fragile 
> argument that in fact certain data does not need to be volatile to avoid 
> problematic code movement.
> 
>> ERROR: spaces required around that '-' (ctx:VxV)
>> #139: FILE: tests/tcg/i386/test-i386-fxtract.c:80:
>> +                      "0" (0x1p-16445L));
>>                                 ^
> 
> No, this is a C99 hex float contstant, not a subtraction.  There are 
> already such constants in tests/tcg/multiarch/float_helpers.c and 
> tests/tcg/multiarch/float_madds.c at least, so I assume they are OK in 
> QEMU floating-point tests and this style checker should not be objecting 
> to them.
> 

Correct, these are all false positives.


r~

_______________________________________________
Patchew-devel mailing list
Patchew-devel@redhat.com
https://www.redhat.com/mailman/listinfo/patchew-devel

Re: [PATCH 0/5] target/i386: fxtract, fscale fixes

Posted by Philippe Mathieu-Daudé 3 years, 11 months ago

Cc'ing FPU emulation maintainers too.

On 5/7/20 2:42 AM, Joseph Myers wrote:
> Among the various bugs in the x87 floating-point emulation that show
> up through a combination of glibc testing and code inspection, there
> are several in the implementations of the fxtract and fscale
> instructions.  This series fixes those bugs.
> 
> Bugs in other instructions, and bugs relating to floating-point
> exceptions and flag setting, will be addressed separately.  In
> particular, while some of these patches add code that sets exception
> flags in the softfloat state, it's generally the case that the x87
> emulation ignores exceptions in that state rather than propagating
> them to the status word (and to generating traps where appropriate).
> I intend to address that missing propagation of exceptions in a
> subsequent patch series; until it's addressed, the code setting
> exceptions won't actually do anything useful.  (There is also code in
> the x87 emulation, including that of fscale, that would result in
> spurious exceptions being set from a naive propagation of exceptions
> from the softfloat state, and thus will need updating to avoid
> propagating inappropriate exceptions when such propagation is
> implemented.)
> 
> Joseph Myers (5):
>    target/i386: implement special cases for fxtract
>    target/i386: fix fscale handling of signaling NaN
>    target/i386: fix fscale handling of invalid exponent encodings
>    target/i386: fix fscale handling of infinite exponents
>    target/i386: fix fscale handling of rounding precision
> 
>   target/i386/fpu_helper.c           |  59 +++++++++++++-
>   tests/tcg/i386/test-i386-fscale.c  | 108 ++++++++++++++++++++++++++
>   tests/tcg/i386/test-i386-fxtract.c | 120 +++++++++++++++++++++++++++++
>   3 files changed, 285 insertions(+), 2 deletions(-)
>   create mode 100644 tests/tcg/i386/test-i386-fscale.c
>   create mode 100644 tests/tcg/i386/test-i386-fxtract.c
>

Ping Re: [PATCH 0/5] target/i386: fxtract, fscale fixes

Posted by Joseph Myers 3 years, 11 months ago

Ping for this patch series 
<https://lists.gnu.org/archive/html/qemu-devel/2020-05/msg01465.html>.

Although my three patch series so far for floatx80 and i386 floating-point 
instructions fixes are independent of each other, it's likely future patch 
series in this area will depend on some of the previous patch series.

-- 
Joseph S. Myers
joseph@codesourcery.com

Re: Ping Re: [PATCH 0/5] target/i386: fxtract, fscale fixes

Posted by Paolo Bonzini 3 years, 11 months ago

On 14/05/20 20:25, Joseph Myers wrote:
> Ping for this patch series 
> <https://lists.gnu.org/archive/html/qemu-devel/2020-05/msg01465.html>.
> 
> Although my three patch series so far for floatx80 and i386 floating-point 
> instructions fixes are independent of each other, it's likely future patch 
> series in this area will depend on some of the previous patch series.

Sorry, I'm lagging behind on my QEMU reviews.  I'll get to it tomorrow
or next week.

Thanks,

Paolo