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 13:22, Carlos O'Donell wrote:
> On 11/24/23 07:28, Adhemerval Zanella Netto wrote:
>> 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)
> 
> OK. Agreed.
> 
>>     puts ("fesetexcept (FE_ALL_EXCEPT) succeeded");
>>   else if (!EXCEPTION_SET_FORCES_TRAP)			 // EXCEPTION_SET_FORCES_TRAP is set to 1
> 
> OK. Agreed.
> 
>>     {
>>       puts ("fesetexcept (FE_ALL_EXCEPT) failed");
>>       if (EXCEPTION_TESTS (float))
>>         {
>>           puts ("failure of fesetexcept was unexpected");
>>           result = 1;
> 
> Where do we set EXCEPTION_TESTS (float) to zero for POWER?
> 
> sysdeps/generic/math-tests-exceptions.h:#define EXCEPTION_TESTS_float	1
> sysdeps/generic/math-tests-exceptions.h:#define EXCEPTION_TESTS_double	1
> sysdeps/generic/math-tests-exceptions.h:#define EXCEPTION_TESTS_long_double	1
> sysdeps/generic/math-tests-exceptions.h:#define EXCEPTION_TESTS_float128	1

We don't, powerpc does support exceptions.  The issues is a powerpc limitation
that Bruno has pointed out in BZ 30988:

  setting a floating-point exception flag triggers a trap, when traps are enabled
  for the particular exception and globally for the thread (via
  prctl (PR_SET_FPEXC, PR_FP_EXC_PRECISE)).

It is because feenableexcept on powerpc enables the PR_FP_EXC_PRECISE mode.

> 
> 
>>         }
>>       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.
> 
> Correct, if the function succeeds then it is a failure, it's likely someone broke
> the conditional and now we have a function that is back to raising traps by
> accident like it was before. It is a regression of bug 30988 if it succeeds.

Agreed.

> 
>>
>>>        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).
> 
> Agreed.
> 
>>
>>    * 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.
> 
> Agreed. Though EXCEPTION_TESTS is still relevant here to avoid regression.
> 
>>
>> 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;
>>     }
>>
> 
> Pasted below from downthread correction:
> 
>> Oops, the above does not make sense:
>>
>>   if (ret == 0)
>>     {
>>       if (EXCEPTION_SET_FORCES_TRAP)
>>         {
>>           puts ("unexpected fesetexcept success");
>>           result = 1;
> 
> Yes, this catches a POWER target regression for bug 30988.
> 
> For the sake of completeness and the use of internal macro APIs
> it is conceivable that EXCEPTION_TESTS could be used to check if the
> test should even be checked (like my suggestion does).> 
> I consider it a simplification that you are applying target
> knowledge from other files in the tree to skip that check
> i.e. you know there is no EXCEPTION_SET_FORCES_TRAP true target that
> is also EXCEPTION_TESTS true target.
> 
> Is it correct to apply that simplification to this code?
> 
> Or should the code handle both EXCEPTION_SET_FORCES_TRAP and
> EXCEPTION_TESTS permutations in a generic fashion?

I think the simplification applies here, because the issue is a 
powerpc/x87 architecture limitation here setting the floating-point 
register status will trigger a floating point exception (x87 would 
trigger in the next floating point operation, but it is essentially 
the same issue).

So the fesetexcept/fesetexceptflag would either:

  1. Raise a floating point exception, aborting the testcase (current
     code).
  2. Fail where it should not.
  3. Rail where it should (powerpc/x87).
  4. Succeeds.

So 1. and 2. are considered a regression, where 3. and 4. is the
expected result.

> 
>>         }
>>     }
>>   else if (!EXCEPTION_SET_FORCES_TRAP)
> 
> OK. This is all other architecture failure paths.
> 
>>     {
>>       puts ("fesetexcept (FE_ALL_EXCEPT) failed");
> 
> OK.
> 
>>       if (EXCEPTION_TESTS (float))
>>         {
>>           puts ("failure of fesetexcept was unexpected");
>>           result = 1;
> 
> OK. This is the failure path for all targets that can do these operations.
> 
>>         }
>>       else
>>         puts ("failure of fesetexcept OK");
> 
> OK. Because it shouldn't be tested e.g. np-fpu targets.
> 
>>     }
> 
> 
>