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

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

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.

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).