diff mbox series

[5/7] riscv: Fix feenvupdate with FE_DFL_ENV (BZ 31022)

Message ID 20231103122416.2724355-6-adhemerval.zanella@linaro.org
State Superseded
Headers show
Series Multiple floating-point environment fixes | expand

Commit Message

Adhemerval Zanella Nov. 3, 2023, 12:24 p.m. UTC 
libc_feupdateenv_riscv should check for FE_DFL_ENV, similar to
libc_fesetenv_riscv.

Also extend the test-fenv.c to test fenvupdate.

Checked on riscv under qemu-system.
---
 math/test-fenv.c                 | 132 ++++++++++++++++++++++++++++---
 sysdeps/riscv/rvf/fenv_private.h |   6 +-
 2 files changed, 127 insertions(+), 11 deletions(-)

Comments

Bruno Haible Nov. 6, 2023, 12:21 a.m. UTC | #1 
This code gave me puzzles:

  long int env = (long int) envp - (long int) FE_DFL_ENV;
  if (env != 0)
    env = *envp;

* What is the intent of the code?
* Can signed integer overflow happen in the first line? (It cannot, due to
  the value of FE_DFL_ENV being -1 and envp being otherwise aligned mod 4.
  But one should not have to think that far.)
* It assumes that intptr_t is the same as 'long int'. Which is not
  universally true.

I would suggest to replace the function with this code:

libc_feupdateenv_riscv (const fenv_t *envp)
{
  long int env = (envp != FE_DFL_ENV ? *envp : 0);

  _FPU_SETCW (env | riscv_getflags ());
}

* It is clearer.
* It does not make assumptions regarding FE_DFL_ENV or alignment.
* It produces the exact same code; see attached foo.c and foo.s,
  generated by "riscv64-linux-gnu-gcc -O2 -S foo.c".

Probably the idiom that you copied was meant to allow the use of a
conditional load instruction. But I would avoid such micro-optimizations
and instead rely on the compiler to choose the best instructions for a
task.

Bruno
.file	"foo.c"
	.option pic
	.text
	.align	1
	.globl	libc_feupdateenv_riscv_az
	.type	libc_feupdateenv_riscv_az, @function
libc_feupdateenv_riscv_az:
	li	a5,-1
	li	a4,0
	beq	a0,a5,.L2
	lwu	a4,0(a0)
.L2:
#APP
# 9 "foo.c" 1
	frflags a5
# 0 "" 2
#NO_APP
	sext.w	a5,a5
	or	a5,a5,a4
#APP
# 20 "foo.c" 1
	fssr a5
# 0 "" 2
#NO_APP
	ret
	.size	libc_feupdateenv_riscv_az, .-libc_feupdateenv_riscv_az
	.align	1
	.globl	libc_feupdateenv_riscv_bh
	.type	libc_feupdateenv_riscv_bh, @function
libc_feupdateenv_riscv_bh:
	li	a5,-1
	li	a4,0
	beq	a0,a5,.L6
	lwu	a4,0(a0)
.L6:
#APP
# 9 "foo.c" 1
	frflags a5
# 0 "" 2
#NO_APP
	sext.w	a5,a5
	or	a5,a5,a4
#APP
# 28 "foo.c" 1
	fssr a5
# 0 "" 2
#NO_APP
	ret
	.size	libc_feupdateenv_riscv_bh, .-libc_feupdateenv_riscv_bh
	.ident	"GCC: (Ubuntu 11.4.0-1ubuntu1~22.04) 11.4.0"
	.section	.note.GNU-stack,"",@progbits
Bruno Haible Nov. 6, 2023, 12:24 a.m. UTC | #2 
> +feupdateenv_single_test (const char *test_name, int fe_exc,
> +			 fexcept_t exception)

The 3rd argument type should be 'int', not 'fexcept_t'.

The type fexcept_t is only for use with the functions fegetexceptflag,
fesetexceptflag, fetestexceptflag. It would be perfectly reasonable for
a platform to define fexcept_t to 'unsigned char', regardless of the
values of the FE_* constants.

> +update_single_exc (const char *test_name, const fenv_t *envp, int fe_exc,
> +                  int fe_exc_clear, fexcept_t exception)

Likewise.

Bruno
Adhemerval Zanella Nov. 6, 2023, 11:28 a.m. UTC | #3 
On 05/11/23 21:21, Bruno Haible wrote:
> This code gave me puzzles:
> 
>   long int env = (long int) envp - (long int) FE_DFL_ENV;
>   if (env != 0)
>     env = *envp;
> 
> * What is the intent of the code?
> * Can signed integer overflow happen in the first line? (It cannot, due to
>   the value of FE_DFL_ENV being -1 and envp being otherwise aligned mod 4.
>   But one should not have to think that far.)
> * It assumes that intptr_t is the same as 'long int'. Which is not
>   universally true.
> 
> I would suggest to replace the function with this code:
> 
> libc_feupdateenv_riscv (const fenv_t *envp)
> {
>   long int env = (envp != FE_DFL_ENV ? *envp : 0);
> 
>   _FPU_SETCW (env | riscv_getflags ());
> }
> 
> * It is clearer.
> * It does not make assumptions regarding FE_DFL_ENV or alignment.
> * It produces the exact same code; see attached foo.c and foo.s,
>   generated by "riscv64-linux-gnu-gcc -O2 -S foo.c".

Indeed I was not sure why libc_fesetenv_riscv used this specific idiom
to check whether the input is FE_DFL_ENV.  I will change to this, it
is clear.

> 
> Probably the idiom that you copied was meant to allow the use of a
> conditional load instruction. But I would avoid such micro-optimizations
> and instead rely on the compiler to choose the best instructions for a
> task.
> 
> Bruno
Adhemerval Zanella Nov. 6, 2023, 11:32 a.m. UTC | #4 
On 05/11/23 21:24, Bruno Haible wrote:
>> +feupdateenv_single_test (const char *test_name, int fe_exc,
>> +			 fexcept_t exception)
> 
> The 3rd argument type should be 'int', not 'fexcept_t'.
> 
> The type fexcept_t is only for use with the functions fegetexceptflag,
> fesetexceptflag, fetestexceptflag. It would be perfectly reasonable for
> a platform to define fexcept_t to 'unsigned char', regardless of the
> values of the FE_* constants.

Ok.

> 
>> +update_single_exc (const char *test_name, const fenv_t *envp, int fe_exc,
>> +                  int fe_exc_clear, fexcept_t exception)
> 
> Likewise.

Ok.
Szabolcs Nagy Jan. 9, 2024, 1:44 p.m. UTC | #5 
The 11/03/2023 09:24, Adhemerval Zanella wrote:
> libc_feupdateenv_riscv should check for FE_DFL_ENV, similar to
> libc_fesetenv_riscv.
> 
> Also extend the test-fenv.c to test fenvupdate.
> 
> Checked on riscv under qemu-system.

the test fails on aarch64 when trapping is supported.

> +++ b/math/test-fenv.c
> @@ -196,6 +196,30 @@ set_single_exc (const char *test_name, int fe_exc, fexcept_t exception)
>    feclearexcept (exception);
>    test_exceptions (str, ALL_EXC ^ fe_exc, 0);
>  }
> +
> +static void
> +update_single_exc (const char *test_name, const fenv_t *envp, int fe_exc,
> +		   int fe_exc_clear, fexcept_t exception)
> +{
> +  char str[200];
> +  /* The standard allows the inexact exception to be set together with the
> +     underflow and overflow exceptions.  So ignore the inexact flag if the
> +     others are raised.  */
> +  int ignore_inexact = (fe_exc & (UNDERFLOW_EXC | OVERFLOW_EXC)) != 0;
> +
> +  strcpy (str, test_name);
> +  strcat (str, ": set flag, with rest not set");
> +  feclearexcept (FE_ALL_EXCEPT);
> +  feraiseexcept (exception);

this relies on disabled trapping.

> +  feupdateenv (envp);
> +  test_exceptions (str, fe_exc, ignore_inexact);
> +
> +  strcpy (str, test_name);
> +  strcat (str, ": clear flag, rest also unset");
> +  feclearexcept (exception);
> +  feupdateenv (envp);
> +  test_exceptions (str, fe_exc_clear, ignore_inexact);
> +}
...
>  /* Test that program aborts with no masked interrupts */
>  static void
> -feenv_nomask_test (const char *flag_name, int fe_exc)
> +feenv_nomask_test (const char *flag_name, int fe_exc, int (*func)(const fenv_t *))
>  {
>  # if defined FE_NOMASK_ENV
>    int status;
>    pid_t pid;
>  
>    if (!EXCEPTION_ENABLE_SUPPORTED (FE_ALL_EXCEPT)
> -      && fesetenv (FE_NOMASK_ENV) != 0)
> +      && func (FE_NOMASK_ENV) != 0)

trapping is enabled here on targets where trapping is optional.

> +static void
> +feupdate_single_test (const char *flag_name, int fe_exc)
> +{
> +  feenv_nomask_test (flag_name, fe_exc, feupdateenv);
> +  feenv_mask_test (flag_name, fe_exc, feupdateenv);
> +}

trapping is still on after this function.

> +static void
> +feupdateenv_single_test (const char *test_name, int fe_exc,
> +			 fexcept_t exception)
> +{
> +  char str[100];
> +  fenv_t env;
> +  int res;
> +
> +  snprintf (str, sizeof str, "feupdateenv %s and FL_DFL_ENV", test_name);
> +  update_single_exc (str, FE_DFL_ENV, fe_exc, NO_EXC, exception);

this relies on disabled trapping.

> +static void
> +feupdateenv_tests (void)
> +{
...
> +#ifdef FE_OVERFLOW
> +  feupdate_single_test ("FE_OVERFLOW", FE_OVERFLOW);
> +#endif
> +
> +#ifdef FE_DIVBYZERO
> +  feupdateenv_single_test ("DIVBYZERO", DIVBYZERO_EXC, FE_DIVBYZERO);
> +#endif

this test fails because previous enables trapping mode.

fwiw a feupdateenv (FE_DFL_ENV); in between the tests
makes math/test-fenv pass.
Adhemerval Zanella Jan. 9, 2024, 6:16 p.m. UTC | #6 
On 09/01/24 10:44, Szabolcs Nagy wrote:
> The 11/03/2023 09:24, Adhemerval Zanella wrote:
>> libc_feupdateenv_riscv should check for FE_DFL_ENV, similar to
>> libc_fesetenv_riscv.
>>
>> Also extend the test-fenv.c to test fenvupdate.
>>
>> Checked on riscv under qemu-system.
> 
> the test fails on aarch64 when trapping is supported.
> 
>> +++ b/math/test-fenv.c
>> @@ -196,6 +196,30 @@ set_single_exc (const char *test_name, int fe_exc, fexcept_t exception)
>>    feclearexcept (exception);
>>    test_exceptions (str, ALL_EXC ^ fe_exc, 0);
>>  }
>> +
>> +static void
>> +update_single_exc (const char *test_name, const fenv_t *envp, int fe_exc,
>> +		   int fe_exc_clear, fexcept_t exception)
>> +{
>> +  char str[200];
>> +  /* The standard allows the inexact exception to be set together with the
>> +     underflow and overflow exceptions.  So ignore the inexact flag if the
>> +     others are raised.  */
>> +  int ignore_inexact = (fe_exc & (UNDERFLOW_EXC | OVERFLOW_EXC)) != 0;
>> +
>> +  strcpy (str, test_name);
>> +  strcat (str, ": set flag, with rest not set");
>> +  feclearexcept (FE_ALL_EXCEPT);
>> +  feraiseexcept (exception);
> 
> this relies on disabled trapping.
> 
>> +  feupdateenv (envp);
>> +  test_exceptions (str, fe_exc, ignore_inexact);
>> +
>> +  strcpy (str, test_name);
>> +  strcat (str, ": clear flag, rest also unset");
>> +  feclearexcept (exception);
>> +  feupdateenv (envp);
>> +  test_exceptions (str, fe_exc_clear, ignore_inexact);
>> +}
> ...
>>  /* Test that program aborts with no masked interrupts */
>>  static void
>> -feenv_nomask_test (const char *flag_name, int fe_exc)
>> +feenv_nomask_test (const char *flag_name, int fe_exc, int (*func)(const fenv_t *))
>>  {
>>  # if defined FE_NOMASK_ENV
>>    int status;
>>    pid_t pid;
>>  
>>    if (!EXCEPTION_ENABLE_SUPPORTED (FE_ALL_EXCEPT)
>> -      && fesetenv (FE_NOMASK_ENV) != 0)
>> +      && func (FE_NOMASK_ENV) != 0)
> 
> trapping is enabled here on targets where trapping is optional.
> 
>> +static void
>> +feupdate_single_test (const char *flag_name, int fe_exc)
>> +{
>> +  feenv_nomask_test (flag_name, fe_exc, feupdateenv);
>> +  feenv_mask_test (flag_name, fe_exc, feupdateenv);
>> +}
> 
> trapping is still on after this function.
> 
>> +static void
>> +feupdateenv_single_test (const char *test_name, int fe_exc,
>> +			 fexcept_t exception)
>> +{
>> +  char str[100];
>> +  fenv_t env;
>> +  int res;
>> +
>> +  snprintf (str, sizeof str, "feupdateenv %s and FL_DFL_ENV", test_name);
>> +  update_single_exc (str, FE_DFL_ENV, fe_exc, NO_EXC, exception);
> 
> this relies on disabled trapping.
> 
>> +static void
>> +feupdateenv_tests (void)
>> +{
> ...
>> +#ifdef FE_OVERFLOW
>> +  feupdate_single_test ("FE_OVERFLOW", FE_OVERFLOW);
>> +#endif
>> +
>> +#ifdef FE_DIVBYZERO
>> +  feupdateenv_single_test ("DIVBYZERO", DIVBYZERO_EXC, FE_DIVBYZERO);
>> +#endif
> 
> this test fails because previous enables trapping mode.
> 
> fwiw a feupdateenv (FE_DFL_ENV); in between the tests
> makes math/test-fenv pass.

I will take a look, thanks for the heads up.
diff mbox series

Patch

diff --git a/math/test-fenv.c b/math/test-fenv.c
index 0af7141ba7..734e0a219d 100644
--- a/math/test-fenv.c
+++ b/math/test-fenv.c
@@ -196,6 +196,30 @@  set_single_exc (const char *test_name, int fe_exc, fexcept_t exception)
   feclearexcept (exception);
   test_exceptions (str, ALL_EXC ^ fe_exc, 0);
 }
+
+static void
+update_single_exc (const char *test_name, const fenv_t *envp, int fe_exc,
+		   int fe_exc_clear, fexcept_t exception)
+{
+  char str[200];
+  /* The standard allows the inexact exception to be set together with the
+     underflow and overflow exceptions.  So ignore the inexact flag if the
+     others are raised.  */
+  int ignore_inexact = (fe_exc & (UNDERFLOW_EXC | OVERFLOW_EXC)) != 0;
+
+  strcpy (str, test_name);
+  strcat (str, ": set flag, with rest not set");
+  feclearexcept (FE_ALL_EXCEPT);
+  feraiseexcept (exception);
+  feupdateenv (envp);
+  test_exceptions (str, fe_exc, ignore_inexact);
+
+  strcpy (str, test_name);
+  strcat (str, ": clear flag, rest also unset");
+  feclearexcept (exception);
+  feupdateenv (envp);
+  test_exceptions (str, fe_exc_clear, ignore_inexact);
+}
 #endif
 
 static void
@@ -233,22 +257,32 @@  fe_tests (void)
 }
 
 #if FE_ALL_EXCEPT
+static const char *
+funcname (int (*func)(const fenv_t *))
+{
+  if (func == fesetenv)
+    return "fesetenv";
+  else if (func == feupdateenv)
+    return "feupdateenv";
+  __builtin_unreachable ();
+}
+
 /* Test that program aborts with no masked interrupts */
 static void
-feenv_nomask_test (const char *flag_name, int fe_exc)
+feenv_nomask_test (const char *flag_name, int fe_exc, int (*func)(const fenv_t *))
 {
 # if defined FE_NOMASK_ENV
   int status;
   pid_t pid;
 
   if (!EXCEPTION_ENABLE_SUPPORTED (FE_ALL_EXCEPT)
-      && fesetenv (FE_NOMASK_ENV) != 0)
+      && func (FE_NOMASK_ENV) != 0)
     {
       printf ("Test: not testing FE_NOMASK_ENV, it isn't implemented.\n");
       return;
     }
 
-  printf ("Test: after fesetenv (FE_NOMASK_ENV) processes will abort\n");
+  printf ("Test: after %s (FE_NOMASK_ENV) processes will abort\n", funcname (func));
   printf ("      when feraiseexcept (%s) is called.\n", flag_name);
   pid = fork ();
   if (pid == 0)
@@ -295,12 +329,12 @@  feenv_nomask_test (const char *flag_name, int fe_exc)
 
 /* Test that program doesn't abort with default environment */
 static void
-feenv_mask_test (const char *flag_name, int fe_exc)
+feenv_mask_test (const char *flag_name, int fe_exc, int (*func)(const fenv_t *))
 {
   int status;
   pid_t pid;
 
-  printf ("Test: after fesetenv (FE_DFL_ENV) processes will not abort\n");
+  printf ("Test: after %s (FE_DFL_ENV) processes will not abort\n", funcname (func));
   printf ("      when feraiseexcept (%s) is called.\n", flag_name);
   pid = fork ();
   if (pid == 0)
@@ -313,7 +347,7 @@  feenv_mask_test (const char *flag_name, int fe_exc)
       setrlimit (RLIMIT_CORE, &core_limit);
 #endif
 
-      fesetenv (FE_DFL_ENV);
+      func (FE_DFL_ENV);
       feraiseexcept (fe_exc);
       exit (2);
     }
@@ -615,10 +649,18 @@  feenable_test (const char *flag_name, int fe_exc)
 static void
 fe_single_test (const char *flag_name, int fe_exc)
 {
-  feenv_nomask_test (flag_name, fe_exc);
-  feenv_mask_test (flag_name, fe_exc);
+  feenv_nomask_test (flag_name, fe_exc, fesetenv);
+  feenv_mask_test (flag_name, fe_exc, fesetenv);
   feenable_test (flag_name, fe_exc);
 }
+
+
+static void
+feupdate_single_test (const char *flag_name, int fe_exc)
+{
+  feenv_nomask_test (flag_name, fe_exc, feupdateenv);
+  feenv_mask_test (flag_name, fe_exc, feupdateenv);
+}
 #endif
 
 
@@ -646,6 +688,73 @@  feenv_tests (void)
   fesetenv (FE_DFL_ENV);
 }
 
+#if FE_ALL_EXCEPT
+static void
+feupdateenv_single_test (const char *test_name, int fe_exc,
+			 fexcept_t exception)
+{
+  char str[100];
+  fenv_t env;
+  int res;
+
+  snprintf (str, sizeof str, "feupdateenv %s and FL_DFL_ENV", test_name);
+  update_single_exc (str, FE_DFL_ENV, fe_exc, NO_EXC, exception);
+
+  feraiseexcept (FE_ALL_EXCEPT);
+  res = fegetenv (&env);
+  if (res != 0)
+    {
+      printf ("fegetenv failed: %d\n", res);
+      ++count_errors;
+      return;
+    }
+
+  snprintf (str, sizeof str, "feupdateenv %s and FE_ALL_EXCEPT", test_name);
+  update_single_exc (str, &env, ALL_EXC, ALL_EXC, exception);
+}
+#endif
+
+static void
+feupdateenv_tests (void)
+{
+  /* We might have some exceptions still set.  */
+  feclearexcept (FE_ALL_EXCEPT);
+
+#ifdef FE_DIVBYZERO
+  feupdate_single_test ("FE_DIVBYZERO", FE_DIVBYZERO);
+#endif
+#ifdef FE_INVALID
+  feupdate_single_test ("FE_INVALID", FE_INVALID);
+#endif
+#ifdef FE_INEXACT
+  feupdate_single_test ("FE_INEXACT", FE_INEXACT);
+#endif
+#ifdef FE_UNDERFLOW
+  feupdate_single_test ("FE_UNDERFLOW", FE_UNDERFLOW);
+#endif
+#ifdef FE_OVERFLOW
+  feupdate_single_test ("FE_OVERFLOW", FE_OVERFLOW);
+#endif
+
+#ifdef FE_DIVBYZERO
+  feupdateenv_single_test ("DIVBYZERO", DIVBYZERO_EXC, FE_DIVBYZERO);
+#endif
+#ifdef FE_INVALID
+  feupdateenv_single_test ("INVALID", INVALID_EXC, FE_INVALID);
+#endif
+#ifdef FE_INEXACT
+  feupdateenv_single_test ("INEXACT", INEXACT_EXC, FE_INEXACT);
+#endif
+#ifdef FE_UNDERFLOW
+  feupdateenv_single_test ("UNDERFLOW", UNDERFLOW_EXC, FE_UNDERFLOW);
+#endif
+#ifdef FE_OVERFLOW
+  feupdateenv_single_test ("OVERFLOW", OVERFLOW_EXC, FE_OVERFLOW);
+#endif
+
+  feupdateenv (FE_DFL_ENV);
+}
+
 
 static void
 feholdexcept_tests (void)
@@ -766,13 +875,14 @@  initial_tests (void)
 #endif
 }
 
-int
-main (void)
+static int
+do_test (void)
 {
   initial_tests ();
   fe_tests ();
   feenv_tests ();
   feholdexcept_tests ();
+  feupdateenv_tests ();
 
   if (count_errors)
     {
@@ -782,3 +892,5 @@  main (void)
   printf ("\n All tests passed successfully.\n");
   return 0;
 }
+
+#include <support/test-driver.c>
diff --git a/sysdeps/riscv/rvf/fenv_private.h b/sysdeps/riscv/rvf/fenv_private.h
index 40e23661b7..c91d871160 100644
--- a/sysdeps/riscv/rvf/fenv_private.h
+++ b/sysdeps/riscv/rvf/fenv_private.h
@@ -123,7 +123,11 @@  libc_feupdateenv_test_riscv (const fenv_t *envp, int ex)
 static __always_inline void
 libc_feupdateenv_riscv (const fenv_t *envp)
 {
-  _FPU_SETCW (*envp | riscv_getflags ());
+  long int env = (long int) envp - (long int) FE_DFL_ENV;
+  if (env != 0)
+    env = *envp;
+
+  _FPU_SETCW (env | riscv_getflags ());
 }
 
 #define libc_feupdateenv  libc_feupdateenv_riscv