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

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

On 24/11/23 09:28, Adhemerval Zanella Netto wrote:
> 
> 
> 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;
>     }

Oops, the above does not make sense:

  if (ret == 0)
    {
      if (EXCEPTION_SET_FORCES_TRAP)
        {
          puts ("unexpected fesetexcept success");
          result = 1;
        }
    }
  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");
    }