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

[Adhemerval Zanella Netto <adhemerval.zanella@linaro.org>]

On 23/11/23 18:47, Carlos O'Donell wrote:
> On 11/6/23 15:46, Adhemerval Zanella Netto wrote:
>>
>>
>> On 06/11/23 14:56, Adhemerval Zanella Netto wrote:
>>>
>>>
>>> On 06/11/23 14:38, Carlos O'Donell wrote:
>>>> On 11/6/23 12:11, Adhemerval Zanella Netto wrote:
>>>>>
>>>>>
>>>>> On 06/11/23 14:02, Carlos O'Donell wrote:
>>>>>> On 11/6/23 11:50, Adhemerval Zanella Netto wrote:
>>>>>>>
>>>>>>>
>>>>>>> On 06/11/23 13:08, Carlos O'Donell wrote:
>>>>>>>> On 11/6/23 08:27, Adhemerval Zanella wrote:
>>>>>>>>> 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.
>>>>>>>>
>>>>>>>> OK. I reviewed ISO C 2x (n3096), and I agree this is permissible and preferable.
>>>>>>>>
>>>>>>>>>
>>>>>>>>> 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.
>>>>>>>>
>>>>>>>> Changes test from UNSUPPORTED to PASS when we should test more now that with
>>>>>>>> C2x we're saying the behaviour will result in a non-zero return... then we
>>>>>>>> should test for that.
>>>>>>>>
>>>>>>>>> ---
>>>>>>>>>  math/test-fesetexcept-traps.c      | 11 ++++-------
>>>>>>>>>  math/test-fexcept-traps.c          | 11 ++++-------
>>>>>>>>>  sysdeps/powerpc/fpu/fesetexcept.c  |  5 +++++
>>>>>>>>>  sysdeps/powerpc/fpu/fsetexcptflg.c |  9 ++++++++-
>>>>>>>>>  4 files changed, 21 insertions(+), 15 deletions(-)
>>>>>>>>>
>>>>>>>>> diff --git a/math/test-fesetexcept-traps.c b/math/test-fesetexcept-traps.c
>>>>>>>>> index 71b6e45b33..96f6c4752f 100644
>>>>>>>>> --- a/math/test-fesetexcept-traps.c
>>>>>>>>> +++ b/math/test-fesetexcept-traps.c
>>>>>>>>> @@ -39,16 +39,13 @@ 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.  */
>>>>>>>>>    ret = fesetexcept (FE_ALL_EXCEPT);
>>>>>>>>>    if (ret == 0)
>>>>>>>>
>>>>>>>> We can check for a non-zero return if EXCEPTION_SET_FORCES_TRAP?
>>>>>>>>
>>>>>>>> e.g.
>>>>>>>>
>>>>>>>>   if (!EXCEPTION_SET_FORCES_TRAP)
>>>>>>>>     { 
>>>>>>>>       if (ret == 0)
>>>>>>>>         puts ("fesetexcept (FE_ALL_EXCEPT) succeeded");
>>>>>>>>       else
>>>>>>>>         /* fail */
>>>>>>>>     }
>>>>>>>>   else
>>>>>>>>     {
>>>>>>>>       if (ret == 0)
>>>>>>>>         /* fail */
>>>>>>>>       else
>>>>>>>>         /* pass */
>>>>>>>>     }
>>>>>>>
>>>>>>> The '!EXCEPTION_SET_FORCES_TRAP && ret == 0' or 'EXCEPTION_SET_FORCES_TRAP && ret == 1'
>>>>>>> checks are not really meaningful: either the function succeeds and return 0, or it fails
>>>>>>> for some reason.  And for failure, EXCEPTION_SET_FORCES_TRAP really means an expected 
>>>>>>> failure.
>>>>>>
>>>>>> Sure.
>>>>>>
>>>>>>> So if the function succeeds and no trap is generated (which terminates the process
>>>>>>> as default on Linux) we are fine.  Otherwise, it check if the failure is expected
>>>>>>> (EXCEPTION_SET_FORCES_TRAP).
>>>>>>>
>>>>>>
>>>>>> So we go from UNSUPPORTED to... ?
>>>>>>
>>>>>
>>>>> I though about that, but the test also checks fegetexceptflag (a better option would
>>>>> to split the test in two, so only the fesetexceptflag is unsupported on ppc32).
>>>>>
>>>>
>>>> Perhaps the best option is to just keep the UNSUPPORTED status?
>>>>
>>>
>>> Fair enough.
>>
>> Revising the patch, I recalled that I explicitly removed the UNSUPPORTED
>> so the test can now check if the fesetexcept does fails with -1 for 
>> !EXCEPTION_SET_FORCES_TRAP.  I am not sure if adding it back is an improvement,
>> it means that it won't actually check if BZ#30988 is really fixed.
>  
> My apologies that we have gone around in a circle.
> 
> Let me start again.
> 
> And for the public record and your review I'll write down my assumptions.
> 
> (a) Previously calling fesetexcept() (ISO/IEC 60559) or fesetexceptflag() (ISO C)
>     on POWER would raise a trap because the hardware can only raise the flag if
>     it *also* forces a trap.
> 
> (b) In Bug 30988 (a) is raised as an ISO/IEC 60559 and ISO C conformance issue.
>     And the fix is to return an error from fesetexcept() or fesetexceptflag() if
>     the hardware cannot raise a flag without *also* forcing a trap (which fails
>     to comply with the standard definition).
> 
> (c) In your patch 1/7 you want to remove the "return 77;" for the
>     EXCEPTION_SET_FORCES_TRAP path because it can now be tested.
> 
> Given (c) my expectation is that we *actively* test for the failure.
> 
> Your test changes look they will cause POWER to now fail the test, particularly
> since 'EXCEPTION_TESTS (float)' for POWER will always be true because we want
> to test exceptions (it's just that our expectations are different).

It won't fail on powerpc (I actually tested using the gcc compile farm), because
EXCEPTION_TESTS (float) won't be checked:

  volatile double a = 1.0;
  volatile double b = a + a;
  math_force_eval (b);					 // It will trigger the exception
  volatile long double al = 1.0L;
  volatile long double bl = al + al;
  math_force_eval (bl);

  if (ret == 0)						 // ret will -1 here (this very fix)
    puts ("fesetexcept (FE_ALL_EXCEPT) succeeded");
  else if (!EXCEPTION_SET_FORCES_TRAP)			 // EXCEPTION_SET_FORCES_TRAP is set to 1
    {
      puts ("fesetexcept (FE_ALL_EXCEPT) failed");
      if (EXCEPTION_TESTS (float))
        {
          puts ("failure of fesetexcept was unexpected");
          result = 1;
        }
      else
        puts ("failure of fesetexcept OK");
    }

> 
> Let me sketch out what I was expecting for both test cases:
> 
> diff --git a/math/test-fesetexcept-traps.c b/math/test-fesetexcept-traps.c
> index 71b6e45b33..5ea295a5b8 100644
> --- a/math/test-fesetexcept-traps.c
> +++ b/math/test-fesetexcept-traps.c
> @@ -23,46 +23,97 @@
>  static int
>  do_test (void)
>  {
> -  int result = 0;
> +  int errors = 0;
> +  int ret;
>  
>    fedisableexcept (FE_ALL_EXCEPT);
> -  int ret = feenableexcept (FE_ALL_EXCEPT);
> +  ret = feenableexcept (FE_ALL_EXCEPT);
>    if (!EXCEPTION_ENABLE_SUPPORTED (FE_ALL_EXCEPT) && (ret == -1))
>      {
> -      puts ("feenableexcept (FE_ALL_EXCEPT) not supported, cannot test");
> +      puts ("UNSUPPORTED: feenableexcept (FE_ALL_EXCEPT) not supported, cannot test");
>        return 77;
>      }
>    else if (ret != 0)
>      {
> -      puts ("feenableexcept (FE_ALL_EXCEPT) failed");
> -      result = 1;
> -      return result;
> +      puts ("FAIL: feenableexcept (FE_ALL_EXCEPT)");
> +      errors++;
> +      return errors;
>      }
>  
> -  if (EXCEPTION_SET_FORCES_TRAP)
> +  if (!EXCEPTION_SET_FORCES_TRAP)
>      {
> -      puts ("setting exceptions traps, cannot test on this architecture");
> -      return 77;
> +      /* Verify fesetexcept does not cause exception traps.  */
> +      ret = fesetexcept (FE_ALL_EXCEPT);
> +      if (ret == 0)
> +	puts ("PASS: fesetexcept (FE_ALL_EXCEPT)");
> +      else
> +        {
> +	  /* Some architectures are expected to fail.  */
> +	  if (EXCEPTION_TESTS (float))
> +	    puts ("PASS: fesetexcept (FE_ALL_EXCEPT) "
> +		  "failed as expected because testing is disabled");
> +	  else
> +	    {
> +	      puts ("FAIL: fesetexcept (FE_ALL_EXCEPT)");
> +	      errors++;
> +	    }
> +	}
> +      ret = feclearexcept (FE_ALL_EXCEPT);
> +      if (ret == 0)
> +	puts ("PASS: feclearexcept (FE_ALL_EXCEPT)");
> +      else
> +	{
> +	  /* Some architectures are expected to fail.  */
> +	  if (EXCEPTION_TESTS (float))
> +	    {
> +	      puts ("PASS: feclearexcept (FE_ALL_EXCEPT) "
> +		    "failed as expected because testing is disabled");
> +	    }
> +	  else
> +	    {
> +	      puts ("FAIL: feclearexcept (FE_ALL_EXCEPT) failed");
> +	      errors++;
> +	    }
> +	}
>      }
> -  /* Verify fesetexcept does not cause exception traps.  */
> -  ret = fesetexcept (FE_ALL_EXCEPT);
> -  if (ret == 0)
> -    puts ("fesetexcept (FE_ALL_EXCEPT) succeeded");
>    else
>      {
> -      puts ("fesetexcept (FE_ALL_EXCEPT) failed");
> -      if (EXCEPTION_TESTS (float))
> +      /* Verify fesetexcept fails because the hardware cannot set the
> +	 exceptions without also raising them.  */
> +      ret = fesetexcept (FE_ALL_EXCEPT);
> +      if (ret == 0)
>  	{
> -	  puts ("failure of fesetexcept was unexpected");
> -	  result = 1;
> +	  puts ("FAIL: fesetexcept (FE_ALL_EXCEPT) succeeded unexpectedly");
> +	  errors++;
>  	}

I think this is essentially what you think my proposed change is incomplete,
I assume that EXCEPTION_SET_FORCES_TRAP is a hit since I think it might be
possible that either kernel might paper over this limitation (by some instruction
emulation to hide the exception signal) or a new chip revision might eventually
fix it (as i686 did with SSE2).

Maybe it would be better to assume that EXCEPTION_SET_FORCES_TRAP is a failure
expectation and trigger a regression is function succeeds.

>        else
> -	puts ("failure of fesetexcept OK");
> +	{
> +	  if (EXCEPTION_TESTS (float))
> +	    puts ("PASS: fesetexcept (FE_ALL_EXCEPT) "
> +		  "failed as expected because testing is disabled");
> +	  else
> +	    puts ("PASS: fesetexcept (FE_ALL_EXCEPT) failed as expected");
> +	}
> +      ret = feclearexcept (FE_ALL_EXCEPT);
> +      if (ret == 0)
> +	puts ("PASS: feclearexcept (FE_ALL_EXCEPT)");
> +      else
> +	{
> +	  /* Some architectures are expected to fail.  */
> +	  if (EXCEPTION_TESTS (float))
> +	    {
> +	      puts ("PASS: feclearexcept (FE_ALL_EXCEPT) "
> +		    "failed as expected because testing is disabled");
> +	    }
> +	  else
> +	    {
> +	      puts ("FAIL: feclearexcept (FE_ALL_EXCEPT) failed");
> +	      errors++;
> +	    }
> +	}
>      }
> -  feclearexcept (FE_ALL_EXCEPT);
>  
> -  return result;
> +  return errors;
>  }
>  
> -#define TEST_FUNCTION do_test ()
> -#include "../test-skeleton.c"
> +#include <support/test-driver.c>
> ---
> 
> My point is that by changing the implementation we need to test a whole
> different set of conditions now and the test needs expanding, likewise
> with test-fexcept-traps.c.
> 
> We need two testing paths with different expectations?

No really, the whole point of the test is to check:

    int exc_before = fegetexcept ();
    ret = fesetexcept (FE_ALL_EXCEPT);
    int exc_after = fegetexcept ();

Will not change the exception mask (exc_before == exc_after) *and* not generate
any trap (which you abort the process).  Also, for i686 we need to trigger some
math operations after the fesetexcept to check no exception will be triggered.

Now, if ret is 0 everything works as expected.  If ret is -1, it would depend
whether the architecture has EXCEPTION_SET_FORCES_TRAP: 

   * if is not set, it will depend whether the architectures allows setting
     the exception for the specific float type (EXCEPTION_TESTS (float), which
     is expanded to the constants defined by math-tests-exceptions.h).  Some
     architectures does not support exceptions at all (riscv), or it depends
     of the ABI (arc, arm, loongarch, and ork1 in soft-fp mode).

   * if it is set (powerpc and i386/x87) it means that there is no extra
     checks requires, since the failure for these architectures *is*
     expected.

So assuming EXCEPTION_SET_FORCES_TRAP is a hard indication, I think this
below would be suffice:

  if (ret == 0)
    puts ("fesetexcept (FE_ALL_EXCEPT) succeeded");
  else if (!EXCEPTION_SET_FORCES_TRAP)
    {
      puts ("fesetexcept (FE_ALL_EXCEPT) failed");
      if (EXCEPTION_TESTS (float))
        {
          puts ("failure of fesetexcept was unexpected");
          result = 1;
        }
      else
        puts ("failure of fesetexcept OK");
    }
  else
    {
      if (ret == 0)
        puts ("unexpected fesetexcept success");
      result = ret != -1;
    }