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