Re: [PATCH v2 1/7] powerpc: Do not raise exception traps for fesetexcept/fesetexceptflag (BZ 30988)

[Carlos O'Donell <carlos@redhat.com>]

On 11/27/23 08:46, Adhemerval Zanella Netto wrote:
> Hi Carlos,
> 
> Updated patch below:

LGTM, but because this is submitted under the existing thread patchwork doesn't
consider this a submission.

Reviewed-by: Carlos O'Donell <carlos@redhat.com>
 
> --
> 
> From c10d1d59321d3fce0c532304b428dcef3c8cbb3a Mon Sep 17 00:00:00 2001
> From: Adhemerval Zanella <adhemerval.zanella@linaro.org>
> Date: Tue, 24 Oct 2023 08:37:14 -0300
> Subject: [PATCH 1/7] powerpc: Do not raise exception traps for
>  fesetexcept/fesetexceptflag (BZ 30988)
> 
> According to ISO C23 (7.6.4.4), fesetexcept is supposed to set
> floating-point exception flags without raising a trap (unlike
> feraiseexcept, which is supposed to raise a trap if feenableexcept was
> called with the appropriate argument).
> 
> This is a side-effect of how we implement the GNU extension
> feenableexcept, where feenableexcept/fesetenv/fesetmode/feupdateenv
> might issue prctl (PR_SET_FPEXC, PR_FP_EXC_PRECISE) depending of the
> argument.  And on PR_FP_EXC_PRECISE, setting a floating-point exception
> flag triggers a trap.
> 
> To make the both functions follow the C23, fesetexcept and
> fesetexceptflag now fail if the argument may trigger a trap.
> 
> The math tests now check for an value different than 0, instead
> of bail out as unsupported for EXCEPTION_SET_FORCES_TRAP.
> 
> Checked on powerpc64le-linux-gnu.
> ---
>  math/test-fesetexcept-traps.c      | 24 ++++++++++++++++--------
>  math/test-fexcept-traps.c          | 16 +++++++++-------
>  sysdeps/powerpc/fpu/fesetexcept.c  |  5 +++++
>  sysdeps/powerpc/fpu/fsetexcptflg.c |  9 ++++++++-
>  4 files changed, 38 insertions(+), 16 deletions(-)
> 
> diff --git a/math/test-fesetexcept-traps.c b/math/test-fesetexcept-traps.c
> index 71b6e45b33..7efcd0343a 100644
> --- a/math/test-fesetexcept-traps.c
> +++ b/math/test-fesetexcept-traps.c
> @@ -39,16 +39,24 @@ do_test (void)
>        return result;
>      }
>  
> -  if (EXCEPTION_SET_FORCES_TRAP)
> -    {
> -      puts ("setting exceptions traps, cannot test on this architecture");
> -      return 77;
> -    }
> -  /* Verify fesetexcept does not cause exception traps.  */
> +  /* Verify fesetexcept does not cause exception traps.  For architectures
> +     where setting the exception might result in traps the function should
> +     return a nonzero value.  */

OK.

>    ret = fesetexcept (FE_ALL_EXCEPT);
> +
> +  _Static_assert (!(EXCEPTION_SET_FORCES_TRAP && !EXCEPTION_TESTS(float)),
> +		  "EXCEPTION_SET_FORCES_TRAP only makes sense if the "
> +		  "architecture suports exceptions");

OK. Good _Static_assert to avoid machine misconfiguration.

> +
>    if (ret == 0)
> -    puts ("fesetexcept (FE_ALL_EXCEPT) succeeded");
> -  else
> +    {
> +      if (EXCEPTION_SET_FORCES_TRAP)
> +	{
> +	  puts ("unexpected fesetexcept success");
> +	  result = 1;
> +	}
> +    }
> +  else if (!EXCEPTION_SET_FORCES_TRAP)

OK. Tests both sides pass/fail. I like this version.

>      {
>        puts ("fesetexcept (FE_ALL_EXCEPT) failed");
>        if (EXCEPTION_TESTS (float))
> diff --git a/math/test-fexcept-traps.c b/math/test-fexcept-traps.c
> index 9701c3c320..998c241058 100644
> --- a/math/test-fexcept-traps.c
> +++ b/math/test-fexcept-traps.c
> @@ -63,14 +63,16 @@ do_test (void)
>        result = 1;
>      }
>  
> -  if (EXCEPTION_SET_FORCES_TRAP)
> -    {
> -      puts ("setting exceptions traps, cannot test on this architecture");
> -      return 77;
> -    }
> -  /* The test is that this does not cause exception traps.  */
> +  /* The test is that this does not cause exception traps.  For architectures
> +     where setting the exception might result in traps the function should
> +     return a nonzero value.  */

OK.

>    ret = fesetexceptflag (&saved, FE_ALL_EXCEPT);
> -  if (ret != 0)
> +
> +  _Static_assert (!(EXCEPTION_SET_FORCES_TRAP && !EXCEPTION_TESTS(float)),
> +		  "EXCEPTION_SET_FORCES_TRAP only makes sense if the "
> +		  "architecture suports exceptions");

OK.

> +
> +  if (ret != 0 && !EXCEPTION_SET_FORCES_TRAP)
>      {
>        puts ("fesetexceptflag failed");
>        result = 1;

OK.

> diff --git a/sysdeps/powerpc/fpu/fesetexcept.c b/sysdeps/powerpc/fpu/fesetexcept.c
> index 609a148a95..2850156d3a 100644
> --- a/sysdeps/powerpc/fpu/fesetexcept.c
> +++ b/sysdeps/powerpc/fpu/fesetexcept.c
> @@ -31,6 +31,11 @@ fesetexcept (int excepts)
>  	    & FE_INVALID_SOFTWARE));
>    if (n.l != u.l)
>      {
> +      if (n.l & fenv_exceptions_to_reg (excepts))
> +	/* Setting the exception flags may trigger a trap.  ISO C 23 § 7.6.4.4
> +	    does not allow it.   */
> +	return -1;
> +
>        fesetenv_register (n.fenv);
>  
>        /* Deal with FE_INVALID_SOFTWARE not being implemented on some chips.  */
> diff --git a/sysdeps/powerpc/fpu/fsetexcptflg.c b/sysdeps/powerpc/fpu/fsetexcptflg.c
> index 2b22f913c0..6517e8ea03 100644
> --- a/sysdeps/powerpc/fpu/fsetexcptflg.c
> +++ b/sysdeps/powerpc/fpu/fsetexcptflg.c
> @@ -44,7 +44,14 @@ __fesetexceptflag (const fexcept_t *flagp, int excepts)
>       This may cause floating-point exceptions if the restored state
>       requests it.  */
>    if (n.l != u.l)
> -    fesetenv_register (n.fenv);
> +    {
> +      if (n.l & fenv_exceptions_to_reg (excepts))
> +	/* Setting the exception flags may trigger a trap.  ISO C 23 § 7.6.4.4
> +	    does not allow it.   */
> +	return -1;
> +
> +      fesetenv_register (n.fenv);
> +    }
>  
>    /* Deal with FE_INVALID_SOFTWARE not being implemented on some chips.  */
>    if (flag & FE_INVALID)

-- 
Cheers,
Carlos.