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

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

[Joseph Myers]

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

[PATCH 1/5] target/i386: implement special cases for fxtract

[Joseph Myers]

The implementation of the fxtract instruction treats all nonzero
operands as normal numbers, so yielding incorrect results for invalid
formats, infinities, NaNs and subnormal and pseudo-denormal operands.
Implement appropriate handling of all those cases.

Signed-off-by: Joseph Myers <joseph@codesourcery.com>
---
 target/i386/fpu_helper.c           |  25 +++++-
 tests/tcg/i386/test-i386-fxtract.c | 120 +++++++++++++++++++++++++++++
 2 files changed, 144 insertions(+), 1 deletion(-)
 create mode 100644 tests/tcg/i386/test-i386-fxtract.c

diff --git a/target/i386/fpu_helper.c b/target/i386/fpu_helper.c
index 792a128a6d..71a696a863 100644
--- a/target/i386/fpu_helper.c
+++ b/target/i386/fpu_helper.c
@@ -767,10 +767,33 @@ void helper_fxtract(CPUX86State *env)
                            &env->fp_status);
         fpush(env);
         ST0 = temp.d;
+    } else if (floatx80_invalid_encoding(ST0)) {
+        float_raise(float_flag_invalid, &env->fp_status);
+        ST0 = floatx80_default_nan(&env->fp_status);
+        fpush(env);
+        ST0 = ST1;
+    } else if (floatx80_is_any_nan(ST0)) {
+        if (floatx80_is_signaling_nan(ST0, &env->fp_status)) {
+            float_raise(float_flag_invalid, &env->fp_status);
+            ST0 = floatx80_silence_nan(ST0, &env->fp_status);
+        }
+        fpush(env);
+        ST0 = ST1;
+    } else if (floatx80_is_infinity(ST0)) {
+        fpush(env);
+        ST0 = ST1;
+        ST1 = floatx80_infinity;
     } else {
         int expdif;
 
-        expdif = EXPD(temp) - EXPBIAS;
+        if (EXPD(temp) == 0) {
+            int shift = clz64(temp.l.lower);
+            temp.l.lower <<= shift;
+            expdif = 1 - EXPBIAS - shift;
+            float_raise(float_flag_input_denormal, &env->fp_status);
+        } else {
+            expdif = EXPD(temp) - EXPBIAS;
+        }
         /* DP exponent bias */
         ST0 = int32_to_floatx80(expdif, &env->fp_status);
         fpush(env);
diff --git a/tests/tcg/i386/test-i386-fxtract.c b/tests/tcg/i386/test-i386-fxtract.c
new file mode 100644
index 0000000000..64fd93d333
--- /dev/null
+++ b/tests/tcg/i386/test-i386-fxtract.c
@@ -0,0 +1,120 @@
+/* Test fxtract instruction.  */
+
+#include <stdint.h>
+#include <stdio.h>
+
+union u {
+    struct { uint64_t sig; uint16_t sign_exp; } s;
+    long double ld;
+};
+
+volatile union u ld_pseudo_m16382 = { .s = { UINT64_C(1) << 63, 0 } };
+volatile union u ld_invalid_1 = { .s = { 1, 1234 } };
+volatile union u ld_invalid_2 = { .s = { 0, 1234 } };
+volatile union u ld_invalid_3 = { .s = { 0, 0x7fff } };
+volatile union u ld_invalid_4 = { .s = { (UINT64_C(1) << 63) - 1, 0x7fff } };
+
+volatile long double ld_sig, ld_exp;
+
+int isnan_ld(long double x)
+{
+  union u tmp = { .ld = x };
+  return ((tmp.s.sign_exp & 0x7fff) == 0x7fff &&
+          (tmp.s.sig >> 63) != 0 &&
+          (tmp.s.sig << 1) != 0);
+}
+
+int issignaling_ld(long double x)
+{
+    union u tmp = { .ld = x };
+    return isnan_ld(x) && (tmp.s.sig & UINT64_C(0x4000000000000000)) == 0;
+}
+
+int main(void)
+{
+    int ret = 0;
+    __asm__ volatile ("fxtract" : "=t" (ld_sig), "=u" (ld_exp) : "0" (2.5L));
+    if (ld_sig != 1.25L || ld_exp != 1.0L) {
+        printf("FAIL: fxtract 2.5\n");
+        ret = 1;
+    }
+    __asm__ volatile ("fxtract" : "=t" (ld_sig), "=u" (ld_exp) : "0" (0.0L));
+    if (ld_sig != 0.0L || __builtin_copysignl(1.0L, ld_sig) != 1.0L ||
+        ld_exp != -__builtin_infl()) {
+        printf("FAIL: fxtract 0.0\n");
+        ret = 1;
+    }
+    __asm__ volatile ("fxtract" : "=t" (ld_sig), "=u" (ld_exp) : "0" (-0.0L));
+    if (ld_sig != -0.0L || __builtin_copysignl(1.0L, ld_sig) != -1.0L ||
+        ld_exp != -__builtin_infl()) {
+        printf("FAIL: fxtract -0.0\n");
+        ret = 1;
+    }
+    __asm__ volatile ("fxtract" : "=t" (ld_sig), "=u" (ld_exp) :
+                      "0" (__builtin_infl()));
+    if (ld_sig != __builtin_infl() || ld_exp != __builtin_infl()) {
+        printf("FAIL: fxtract inf\n");
+        ret = 1;
+    }
+    __asm__ volatile ("fxtract" : "=t" (ld_sig), "=u" (ld_exp) :
+                      "0" (-__builtin_infl()));
+    if (ld_sig != -__builtin_infl() || ld_exp != __builtin_infl()) {
+        printf("FAIL: fxtract -inf\n");
+        ret = 1;
+    }
+    __asm__ volatile ("fxtract" : "=t" (ld_sig), "=u" (ld_exp) :
+                      "0" (__builtin_nanl("")));
+    if (!isnan_ld(ld_sig) || issignaling_ld(ld_sig) ||
+        !isnan_ld(ld_exp) || issignaling_ld(ld_exp)) {
+        printf("FAIL: fxtract qnan\n");
+        ret = 1;
+    }
+    __asm__ volatile ("fxtract" : "=t" (ld_sig), "=u" (ld_exp) :
+                      "0" (__builtin_nansl("")));
+    if (!isnan_ld(ld_sig) || issignaling_ld(ld_sig) ||
+        !isnan_ld(ld_exp) || issignaling_ld(ld_exp)) {
+        printf("FAIL: fxtract snan\n");
+        ret = 1;
+    }
+    __asm__ volatile ("fxtract" : "=t" (ld_sig), "=u" (ld_exp) :
+                      "0" (0x1p-16445L));
+    if (ld_sig != 1.0L || ld_exp != -16445.0L) {
+        printf("FAIL: fxtract subnormal\n");
+        ret = 1;
+    }
+    __asm__ volatile ("fxtract" : "=t" (ld_sig), "=u" (ld_exp) :
+                      "0" (ld_pseudo_m16382.ld));
+    if (ld_sig != 1.0L || ld_exp != -16382.0L) {
+        printf("FAIL: fxtract pseudo\n");
+        ret = 1;
+    }
+    __asm__ volatile ("fxtract" : "=t" (ld_sig), "=u" (ld_exp) :
+                      "0" (ld_invalid_1.ld));
+    if (!isnan_ld(ld_sig) || issignaling_ld(ld_sig) ||
+        !isnan_ld(ld_exp) || issignaling_ld(ld_exp)) {
+        printf("FAIL: fxtract invalid 1\n");
+        ret = 1;
+    }
+    __asm__ volatile ("fxtract" : "=t" (ld_sig), "=u" (ld_exp) :
+                      "0" (ld_invalid_2.ld));
+    if (!isnan_ld(ld_sig) || issignaling_ld(ld_sig) ||
+        !isnan_ld(ld_exp) || issignaling_ld(ld_exp)) {
+        printf("FAIL: fxtract invalid 2\n");
+        ret = 1;
+    }
+    __asm__ volatile ("fxtract" : "=t" (ld_sig), "=u" (ld_exp) :
+                      "0" (ld_invalid_3.ld));
+    if (!isnan_ld(ld_sig) || issignaling_ld(ld_sig) ||
+        !isnan_ld(ld_exp) || issignaling_ld(ld_exp)) {
+        printf("FAIL: fxtract invalid 3\n");
+        ret = 1;
+    }
+    __asm__ volatile ("fxtract" : "=t" (ld_sig), "=u" (ld_exp) :
+                      "0" (ld_invalid_4.ld));
+    if (!isnan_ld(ld_sig) || issignaling_ld(ld_sig) ||
+        !isnan_ld(ld_exp) || issignaling_ld(ld_exp)) {
+        printf("FAIL: fxtract invalid 4\n");
+        ret = 1;
+    }
+    return ret;
+}
-- 
2.17.1


-- 
Joseph S. Myers
joseph@codesourcery.com

[PATCH 2/5] target/i386: fix fscale handling of signaling NaN

[Joseph Myers]

The implementation of the fscale instruction returns a NaN exponent
unchanged.  Fix it to return a quiet NaN when the provided exponent is
a signaling NaN.

Signed-off-by: Joseph Myers <joseph@codesourcery.com>
---
 target/i386/fpu_helper.c          |  4 ++++
 tests/tcg/i386/test-i386-fscale.c | 37 +++++++++++++++++++++++++++++++
 2 files changed, 41 insertions(+)
 create mode 100644 tests/tcg/i386/test-i386-fscale.c

diff --git a/target/i386/fpu_helper.c b/target/i386/fpu_helper.c
index 71a696a863..60012c405c 100644
--- a/target/i386/fpu_helper.c
+++ b/target/i386/fpu_helper.c
@@ -970,6 +970,10 @@ void helper_fscale(CPUX86State *env)
 {
     if (floatx80_is_any_nan(ST1)) {
         ST0 = ST1;
+        if (floatx80_is_signaling_nan(ST0, &env->fp_status)) {
+            float_raise(float_flag_invalid, &env->fp_status);
+            ST0 = floatx80_silence_nan(ST0, &env->fp_status);
+        }
     } else {
         int n = floatx80_to_int32_round_to_zero(ST1, &env->fp_status);
         ST0 = floatx80_scalbn(ST0, n, &env->fp_status);
diff --git a/tests/tcg/i386/test-i386-fscale.c b/tests/tcg/i386/test-i386-fscale.c
new file mode 100644
index 0000000000..aecac5125f
--- /dev/null
+++ b/tests/tcg/i386/test-i386-fscale.c
@@ -0,0 +1,37 @@
+/* Test fscale instruction.  */
+
+#include <stdint.h>
+#include <stdio.h>
+
+union u {
+    struct { uint64_t sig; uint16_t sign_exp; } s;
+    long double ld;
+};
+
+volatile long double ld_res;
+
+int isnan_ld(long double x)
+{
+  union u tmp = { .ld = x };
+  return ((tmp.s.sign_exp & 0x7fff) == 0x7fff &&
+          (tmp.s.sig >> 63) != 0 &&
+          (tmp.s.sig << 1) != 0);
+}
+
+int issignaling_ld(long double x)
+{
+    union u tmp = { .ld = x };
+    return isnan_ld(x) && (tmp.s.sig & UINT64_C(0x4000000000000000)) == 0;
+}
+
+int main(void)
+{
+    int ret = 0;
+    __asm__ volatile ("fscale" : "=t" (ld_res) :
+                      "0" (2.5L), "u" (__builtin_nansl("")));
+    if (!isnan_ld(ld_res) || issignaling_ld(ld_res)) {
+        printf("FAIL: fscale snan\n");
+        ret = 1;
+    }
+    return ret;
+}
-- 
2.17.1


-- 
Joseph S. Myers
joseph@codesourcery.com

[PATCH 3/5] target/i386: fix fscale handling of invalid exponent encodings

[Joseph Myers]

The fscale implementation does not check for invalid encodings in the
exponent operand, thus treating them like INT_MIN (the value returned
for invalid encodings by floatx80_to_int32_round_to_zero).  Fix it to
treat them similarly to signaling NaN exponents, thus generating a
quiet NaN result.

Signed-off-by: Joseph Myers <joseph@codesourcery.com>
---
 target/i386/fpu_helper.c          |  5 ++++-
 tests/tcg/i386/test-i386-fscale.c | 29 +++++++++++++++++++++++++++++
 2 files changed, 33 insertions(+), 1 deletion(-)

diff --git a/target/i386/fpu_helper.c b/target/i386/fpu_helper.c
index 60012c405c..7709af8fdd 100644
--- a/target/i386/fpu_helper.c
+++ b/target/i386/fpu_helper.c
@@ -968,7 +968,10 @@ void helper_frndint(CPUX86State *env)
 
 void helper_fscale(CPUX86State *env)
 {
-    if (floatx80_is_any_nan(ST1)) {
+    if (floatx80_invalid_encoding(ST1)) {
+        float_raise(float_flag_invalid, &env->fp_status);
+        ST0 = floatx80_default_nan(&env->fp_status);
+    } else if (floatx80_is_any_nan(ST1)) {
         ST0 = ST1;
         if (floatx80_is_signaling_nan(ST0, &env->fp_status)) {
             float_raise(float_flag_invalid, &env->fp_status);
diff --git a/tests/tcg/i386/test-i386-fscale.c b/tests/tcg/i386/test-i386-fscale.c
index aecac5125f..b65a055d0a 100644
--- a/tests/tcg/i386/test-i386-fscale.c
+++ b/tests/tcg/i386/test-i386-fscale.c
@@ -8,6 +8,11 @@ union u {
     long double ld;
 };
 
+volatile union u ld_invalid_1 = { .s = { 1, 1234 } };
+volatile union u ld_invalid_2 = { .s = { 0, 1234 } };
+volatile union u ld_invalid_3 = { .s = { 0, 0x7fff } };
+volatile union u ld_invalid_4 = { .s = { (UINT64_C(1) << 63) - 1, 0x7fff } };
+
 volatile long double ld_res;
 
 int isnan_ld(long double x)
@@ -33,5 +38,29 @@ int main(void)
         printf("FAIL: fscale snan\n");
         ret = 1;
     }
+    __asm__ volatile ("fscale" : "=t" (ld_res) :
+                      "0" (2.5L), "u" (ld_invalid_1.ld));
+    if (!isnan_ld(ld_res) || issignaling_ld(ld_res)) {
+        printf("FAIL: fscale invalid 1\n");
+        ret = 1;
+    }
+    __asm__ volatile ("fscale" : "=t" (ld_res) :
+                      "0" (2.5L), "u" (ld_invalid_2.ld));
+    if (!isnan_ld(ld_res) || issignaling_ld(ld_res)) {
+        printf("FAIL: fscale invalid 2\n");
+        ret = 1;
+    }
+    __asm__ volatile ("fscale" : "=t" (ld_res) :
+                      "0" (2.5L), "u" (ld_invalid_3.ld));
+    if (!isnan_ld(ld_res) || issignaling_ld(ld_res)) {
+        printf("FAIL: fscale invalid 3\n");
+        ret = 1;
+    }
+    __asm__ volatile ("fscale" : "=t" (ld_res) :
+                      "0" (2.5L), "u" (ld_invalid_4.ld));
+    if (!isnan_ld(ld_res) || issignaling_ld(ld_res)) {
+        printf("FAIL: fscale invalid 4\n");
+        ret = 1;
+    }
     return ret;
 }
-- 
2.17.1


-- 
Joseph S. Myers
joseph@codesourcery.com

[PATCH 4/5] target/i386: fix fscale handling of infinite exponents

[Joseph Myers]

The fscale implementation passes infinite exponents through to generic
code that rounds the exponent to a 32-bit integer before using
floatx80_scalbn.  In round-to-nearest mode, and ignoring exceptions,
this works in many cases.  But it fails to handle the special cases of
scaling 0 by a +Inf exponent or an infinity by a -Inf exponent, which
should produce a NaN, and because it produces an inexact result for
finite nonzero numbers being scaled, the result is sometimes incorrect
in other rounding modes.  Add appropriate handling of infinite
exponents to produce a NaN or an appropriately signed exact zero or
infinity as a result.

Signed-off-by: Joseph Myers <joseph@codesourcery.com>
---
 target/i386/fpu_helper.c          | 22 ++++++++++++++++++++++
 tests/tcg/i386/test-i386-fscale.c | 29 +++++++++++++++++++++++++++++
 2 files changed, 51 insertions(+)

diff --git a/target/i386/fpu_helper.c b/target/i386/fpu_helper.c
index 7709af8fdd..d4c15728e1 100644
--- a/target/i386/fpu_helper.c
+++ b/target/i386/fpu_helper.c
@@ -977,6 +977,28 @@ void helper_fscale(CPUX86State *env)
             float_raise(float_flag_invalid, &env->fp_status);
             ST0 = floatx80_silence_nan(ST0, &env->fp_status);
         }
+    } else if (floatx80_is_infinity(ST1) &&
+               !floatx80_invalid_encoding(ST0) &&
+               !floatx80_is_any_nan(ST0)) {
+        if (floatx80_is_neg(ST1)) {
+            if (floatx80_is_infinity(ST0)) {
+                float_raise(float_flag_invalid, &env->fp_status);
+                ST0 = floatx80_default_nan(&env->fp_status);
+            } else {
+                ST0 = (floatx80_is_neg(ST0) ?
+                       floatx80_chs(floatx80_zero) :
+                       floatx80_zero);
+            }
+        } else {
+            if (floatx80_is_zero(ST0)) {
+                float_raise(float_flag_invalid, &env->fp_status);
+                ST0 = floatx80_default_nan(&env->fp_status);
+            } else {
+                ST0 = (floatx80_is_neg(ST0) ?
+                       floatx80_chs(floatx80_infinity) :
+                       floatx80_infinity);
+            }
+        }
     } else {
         int n = floatx80_to_int32_round_to_zero(ST1, &env->fp_status);
         ST0 = floatx80_scalbn(ST0, n, &env->fp_status);
diff --git a/tests/tcg/i386/test-i386-fscale.c b/tests/tcg/i386/test-i386-fscale.c
index b65a055d0a..b953e7c563 100644
--- a/tests/tcg/i386/test-i386-fscale.c
+++ b/tests/tcg/i386/test-i386-fscale.c
@@ -31,6 +31,7 @@ int issignaling_ld(long double x)
 
 int main(void)
 {
+    short cw;
     int ret = 0;
     __asm__ volatile ("fscale" : "=t" (ld_res) :
                       "0" (2.5L), "u" (__builtin_nansl("")));
@@ -62,5 +63,33 @@ int main(void)
         printf("FAIL: fscale invalid 4\n");
         ret = 1;
     }
+    __asm__ volatile ("fscale" : "=t" (ld_res) :
+                      "0" (0.0L), "u" (__builtin_infl()));
+    if (!isnan_ld(ld_res) || issignaling_ld(ld_res)) {
+        printf("FAIL: fscale 0 up inf\n");
+        ret = 1;
+    }
+    __asm__ volatile ("fscale" : "=t" (ld_res) :
+                      "0" (__builtin_infl()), "u" (-__builtin_infl()));
+    if (!isnan_ld(ld_res) || issignaling_ld(ld_res)) {
+        printf("FAIL: fscale inf down inf\n");
+        ret = 1;
+    }
+    /* Set round-downward.  */
+    __asm__ volatile ("fnstcw %0" : "=m" (cw));
+    cw = (cw & ~0xc00) | 0x400;
+    __asm__ volatile ("fldcw %0" : : "m" (cw));
+    __asm__ volatile ("fscale" : "=t" (ld_res) :
+                      "0" (1.0L), "u" (__builtin_infl()));
+    if (ld_res != __builtin_infl()) {
+        printf("FAIL: fscale finite up inf\n");
+        ret = 1;
+    }
+    __asm__ volatile ("fscale" : "=t" (ld_res) :
+                      "0" (-1.0L), "u" (-__builtin_infl()));
+    if (ld_res != -0.0L || __builtin_copysignl(1.0L, ld_res) != -1.0L) {
+        printf("FAIL: fscale finite down inf\n");
+        ret = 1;
+    }
     return ret;
 }
-- 
2.17.1


-- 
Joseph S. Myers
joseph@codesourcery.com

[PATCH 5/5] target/i386: fix fscale handling of rounding precision

[Joseph Myers]

The fscale implementation uses floatx80_scalbn for the final scaling
operation.  floatx80_scalbn ends up rounding the result using the
dynamic rounding precision configured for the FPU.  But only a limited
set of x87 floating-point instructions are supposed to respect the
dynamic rounding precision, and fscale is not in that set.  Fix the
implementation to save and restore the rounding precision around the
call to floatx80_scalbn.

Signed-off-by: Joseph Myers <joseph@codesourcery.com>
---
 target/i386/fpu_helper.c          |  3 +++
 tests/tcg/i386/test-i386-fscale.c | 13 +++++++++++++
 2 files changed, 16 insertions(+)

diff --git a/target/i386/fpu_helper.c b/target/i386/fpu_helper.c
index d4c15728e1..0c3fce933c 100644
--- a/target/i386/fpu_helper.c
+++ b/target/i386/fpu_helper.c
@@ -1001,7 +1001,10 @@ void helper_fscale(CPUX86State *env)
         }
     } else {
         int n = floatx80_to_int32_round_to_zero(ST1, &env->fp_status);
+        signed char save = env->fp_status.floatx80_rounding_precision;
+        env->fp_status.floatx80_rounding_precision = 80;
         ST0 = floatx80_scalbn(ST0, n, &env->fp_status);
+        env->fp_status.floatx80_rounding_precision = save;
     }
 }
 
diff --git a/tests/tcg/i386/test-i386-fscale.c b/tests/tcg/i386/test-i386-fscale.c
index b953e7c563..d23b3cfeec 100644
--- a/tests/tcg/i386/test-i386-fscale.c
+++ b/tests/tcg/i386/test-i386-fscale.c
@@ -8,6 +8,8 @@ union u {
     long double ld;
 };
 
+volatile long double ld_third = 1.0L / 3.0L;
+volatile long double ld_four_thirds = 4.0L / 3.0L;
 volatile union u ld_invalid_1 = { .s = { 1, 1234 } };
 volatile union u ld_invalid_2 = { .s = { 0, 1234 } };
 volatile union u ld_invalid_3 = { .s = { 0, 0x7fff } };
@@ -91,5 +93,16 @@ int main(void)
         printf("FAIL: fscale finite down inf\n");
         ret = 1;
     }
+    /* Set round-to-nearest with single-precision rounding.  */
+    cw = cw & ~0xf00;
+    __asm__ volatile ("fldcw %0" : : "m" (cw));
+    __asm__ volatile ("fscale" : "=t" (ld_res) :
+                      "0" (ld_third), "u" (2.0L));
+    cw = cw | 0x300;
+    __asm__ volatile ("fldcw %0" : : "m" (cw));
+    if (ld_res != ld_four_thirds) {
+        printf("FAIL: fscale single-precision\n");
+        ret = 1;
+    }
     return ret;
 }
-- 
2.17.1


-- 
Joseph S. Myers
joseph@codesourcery.com

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

[no-reply]

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

[Joseph Myers]

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: [PATCH 0/5] target/i386: fxtract, fscale fixes

[Richard Henderson]

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~

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

[Joseph Myers]

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

[Paolo Bonzini]

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

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

[Philippe Mathieu-Daudé]

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
> 

Re: [PATCH 1/5] target/i386: implement special cases for fxtract

[Alex Bennée]

Joseph Myers <joseph@codesourcery.com> writes:

> The implementation of the fxtract instruction treats all nonzero
> operands as normal numbers, so yielding incorrect results for invalid
> formats, infinities, NaNs and subnormal and pseudo-denormal operands.
> Implement appropriate handling of all those cases.
>
> Signed-off-by: Joseph Myers <joseph@codesourcery.com>
> ---
>  target/i386/fpu_helper.c           |  25 +++++-
>  tests/tcg/i386/test-i386-fxtract.c | 120 +++++++++++++++++++++++++++++
>  2 files changed, 144 insertions(+), 1 deletion(-)
>  create mode 100644 tests/tcg/i386/test-i386-fxtract.c
>

For test tests:

Acked-by: Alex Bennée <alex.bennee@linaro.org>

-- 
Alex Bennée

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

[Paolo Bonzini]

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 1/5] target/i386: implement special cases for fxtract

[Eduardo Habkost]

On Thu, May 07, 2020 at 12:43:30AM +0000, Joseph Myers wrote:
> The implementation of the fxtract instruction treats all nonzero
> operands as normal numbers, so yielding incorrect results for invalid
> formats, infinities, NaNs and subnormal and pseudo-denormal operands.
> Implement appropriate handling of all those cases.
> 
> Signed-off-by: Joseph Myers <joseph@codesourcery.com>
> ---
>  target/i386/fpu_helper.c           |  25 +++++-
>  tests/tcg/i386/test-i386-fxtract.c | 120 +++++++++++++++++++++++++++++
>  2 files changed, 144 insertions(+), 1 deletion(-)
>  create mode 100644 tests/tcg/i386/test-i386-fxtract.c
> 
> diff --git a/target/i386/fpu_helper.c b/target/i386/fpu_helper.c
> index 792a128a6d..71a696a863 100644
> --- a/target/i386/fpu_helper.c
> +++ b/target/i386/fpu_helper.c
> @@ -767,10 +767,33 @@ void helper_fxtract(CPUX86State *env)
>                             &env->fp_status);
>          fpush(env);
>          ST0 = temp.d;
> +    } else if (floatx80_invalid_encoding(ST0)) {
> +        float_raise(float_flag_invalid, &env->fp_status);
> +        ST0 = floatx80_default_nan(&env->fp_status);
> +        fpush(env);
> +        ST0 = ST1;
> +    } else if (floatx80_is_any_nan(ST0)) {
> +        if (floatx80_is_signaling_nan(ST0, &env->fp_status)) {
> +            float_raise(float_flag_invalid, &env->fp_status);
> +            ST0 = floatx80_silence_nan(ST0, &env->fp_status);
> +        }
> +        fpush(env);
> +        ST0 = ST1;
> +    } else if (floatx80_is_infinity(ST0)) {
> +        fpush(env);
> +        ST0 = ST1;
> +        ST1 = floatx80_infinity;
>      } else {
>          int expdif;
>  
> -        expdif = EXPD(temp) - EXPBIAS;
> +        if (EXPD(temp) == 0) {
> +            int shift = clz64(temp.l.lower);
> +            temp.l.lower <<= shift;

Coverity reports the following.  It looks like a false positive
because floatx80_is_zero() would be true if both EXPD(temp) and
temp.l.lower were zero, but maybe I'm missing something.

________________________________________________________________________________________________________
*** CID 1429970:  Integer handling issues  (BAD_SHIFT)
/target/i386/fpu_helper.c: 922 in helper_fxtract()
916             ST1 = floatx80_infinity;
917         } else {
918             int expdif;
919
920             if (EXPD(temp) == 0) {
921                 int shift = clz64(temp.l.lower);
>>>     CID 1429970:  Integer handling issues  (BAD_SHIFT)
>>>     In expression "temp.l.lower <<= shift", left shifting by more than 63 bits has undefined behavior.  The shift amount, "shift", is 64.
922                 temp.l.lower <<= shift;
923                 expdif = 1 - EXPBIAS - shift;
924                 float_raise(float_flag_input_denormal, &env->fp_status);
925             } else {
926                 expdif = EXPD(temp) - EXPBIAS;
927             }



> +            expdif = 1 - EXPBIAS - shift;
> +            float_raise(float_flag_input_denormal, &env->fp_status);
> +        } else {
> +            expdif = EXPD(temp) - EXPBIAS;
> +        }
>          /* DP exponent bias */
>          ST0 = int32_to_floatx80(expdif, &env->fp_status);
>          fpush(env);

-- 
Eduardo

Re: [PATCH 1/5] target/i386: implement special cases for fxtract

[Joseph Myers]

On Tue, 23 Jun 2020, Eduardo Habkost wrote:

> > +        if (EXPD(temp) == 0) {
> > +            int shift = clz64(temp.l.lower);
> > +            temp.l.lower <<= shift;
> 
> Coverity reports the following.  It looks like a false positive
> because floatx80_is_zero() would be true if both EXPD(temp) and
> temp.l.lower were zero, but maybe I'm missing something.

Yes, that looks like a false positive to me.

-- 
Joseph S. Myers
joseph@codesourcery.com

Re: [PATCH 1/5] target/i386: implement special cases for fxtract

[Peter Maydell]

On Tue, 23 Jun 2020 at 23:01, Joseph Myers <joseph@codesourcery.com> wrote:
>
> On Tue, 23 Jun 2020, Eduardo Habkost wrote:
>
> > > +        if (EXPD(temp) == 0) {
> > > +            int shift = clz64(temp.l.lower);
> > > +            temp.l.lower <<= shift;
> >
> > Coverity reports the following.  It looks like a false positive
> > because floatx80_is_zero() would be true if both EXPD(temp) and
> > temp.l.lower were zero, but maybe I'm missing something.
>
> Yes, that looks like a false positive to me.

Thanks; I've marked it as a fp in the coverity UI.

-- PMM