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

[Carlos O'Donell <carlos@redhat.com>]

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

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++;
 	}
       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?

-- 
Cheers,
Carlos.