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

[Adhemerval Zanella Netto]

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.