| Message ID | 20231106132713.953501-2-adhemerval.zanella@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | Multiple floating-point environment fixes | expand |
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 */ } > puts ("fesetexcept (FE_ALL_EXCEPT) succeeded"); > - else > + else if (!EXCEPTION_SET_FORCES_TRAP) > { > puts ("fesetexcept (FE_ALL_EXCEPT) failed"); > if (EXCEPTION_TESTS (float)) > diff --git a/math/test-fexcept-traps.c b/math/test-fexcept-traps.c > index 9701c3c320..9b8f583ae6 100644 > --- a/math/test-fexcept-traps.c > +++ b/math/test-fexcept-traps.c > @@ -63,14 +63,11 @@ do_test (void) > result = 1; > } > > - if (EXCEPTION_SET_FORCES_TRAP) > - { > - puts ("setting exceptions traps, cannot test on this architecture"); > - return 77; > - } > - /* The test is that this does not cause exception traps. */ > + /* The test is that this does not cause exception traps. For architectures > + where setting the exception might result in traps the function should > + return a nonzero value. */ > ret = fesetexceptflag (&saved, FE_ALL_EXCEPT); > - if (ret != 0) > + if (ret != 0 && !EXCEPTION_SET_FORCES_TRAP) Likewise. We can test more now. > { > puts ("fesetexceptflag failed"); > result = 1; > diff --git a/sysdeps/powerpc/fpu/fesetexcept.c b/sysdeps/powerpc/fpu/fesetexcept.c > index 609a148a95..2850156d3a 100644 > --- a/sysdeps/powerpc/fpu/fesetexcept.c > +++ b/sysdeps/powerpc/fpu/fesetexcept.c > @@ -31,6 +31,11 @@ fesetexcept (int excepts) > & FE_INVALID_SOFTWARE)); > if (n.l != u.l) > { > + if (n.l & fenv_exceptions_to_reg (excepts)) > + /* Setting the exception flags may trigger a trap. ISO C 23 § 7.6.4.4 > + does not allow it. */ > + return -1; > + > fesetenv_register (n.fenv); > > /* Deal with FE_INVALID_SOFTWARE not being implemented on some chips. */ > diff --git a/sysdeps/powerpc/fpu/fsetexcptflg.c b/sysdeps/powerpc/fpu/fsetexcptflg.c > index 2b22f913c0..6517e8ea03 100644 > --- a/sysdeps/powerpc/fpu/fsetexcptflg.c > +++ b/sysdeps/powerpc/fpu/fsetexcptflg.c > @@ -44,7 +44,14 @@ __fesetexceptflag (const fexcept_t *flagp, int excepts) > This may cause floating-point exceptions if the restored state > requests it. */ > if (n.l != u.l) > - fesetenv_register (n.fenv); > + { > + if (n.l & fenv_exceptions_to_reg (excepts)) > + /* Setting the exception flags may trigger a trap. ISO C 23 § 7.6.4.4 > + does not allow it. */ > + return -1; > + > + fesetenv_register (n.fenv); > + } > > /* Deal with FE_INVALID_SOFTWARE not being implemented on some chips. */ > if (flag & FE_INVALID)
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).
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... ?
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).
On 06/11/23 14: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). And the proposed changes follow current file idea, maybe the split should be done in a different patch.
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?
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.
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.
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?
On 23/11/23 18:47, Carlos O'Donell wrote: > 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). 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) puts ("fesetexcept (FE_ALL_EXCEPT) succeeded"); else if (!EXCEPTION_SET_FORCES_TRAP) // EXCEPTION_SET_FORCES_TRAP is set to 1 { puts ("fesetexcept (FE_ALL_EXCEPT) failed"); if (EXCEPTION_TESTS (float)) { puts ("failure of fesetexcept was unexpected"); result = 1; } 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. > 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). * 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. 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; }
On 24/11/23 09:28, Adhemerval Zanella Netto wrote: > > > On 23/11/23 18:47, Carlos O'Donell wrote: >> 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). > > 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) > puts ("fesetexcept (FE_ALL_EXCEPT) succeeded"); > else if (!EXCEPTION_SET_FORCES_TRAP) // EXCEPTION_SET_FORCES_TRAP is set to 1 > { > puts ("fesetexcept (FE_ALL_EXCEPT) failed"); > if (EXCEPTION_TESTS (float)) > { > puts ("failure of fesetexcept was unexpected"); > result = 1; > } > 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. > >> 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). > > * 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. > > 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; > } Oops, the above does not make sense: if (ret == 0) { if (EXCEPTION_SET_FORCES_TRAP) { puts ("unexpected fesetexcept success"); result = 1; } } 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"); }
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 > } > 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. > >> 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? > } > } > 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. > }
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. > >> } > > >
On 11/24/23 12:53, Adhemerval Zanella Netto wrote: > 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). In which case I'd rather the test code assert that the two macros can't be used that way in the test with a macro check or static assert that checks 'EXCEPTION_SET_FORCES_TRAP && !FLOATING_TESTS(float)' is true. > 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. 5. Succeed where it should not (powerpc/x87) ? > > So 1. and 2. are considered a regression, where 3. and 4. is the > expected result. ... and 5?
On 24/11/23 15:15, Carlos O'Donell wrote: > On 11/24/23 12:53, Adhemerval Zanella Netto wrote: >> 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). > > In which case I'd rather the test code assert that the two macros can't be > used that way in the test with a macro check or static assert that > checks 'EXCEPTION_SET_FORCES_TRAP && !FLOATING_TESTS(float)' is true. I think you mean EXCEPTION_TESTS (float), and it seems reasonable: _Static_assert (!(EXCEPTION_SET_FORCES_TRAP && !EXCEPTION_TESTS(float)), "EXCEPTION_SET_FORCES_TRAP only makes sense if the " "architecture suports exceptions"); > >> 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. > > 5. Succeed where it should not (powerpc/x87) ? Indeed. > >> >> So 1. and 2. are considered a regression, where 3. and 4. is the >> expected result. > > ... and 5? Yes, and 1., 2., and 5. should be considered a regression.
On 24/11/23 15:46, Adhemerval Zanella Netto wrote: > > > On 24/11/23 15:15, Carlos O'Donell wrote: >> On 11/24/23 12:53, Adhemerval Zanella Netto wrote: >>> 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). >> >> In which case I'd rather the test code assert that the two macros can't be >> used that way in the test with a macro check or static assert that >> checks 'EXCEPTION_SET_FORCES_TRAP && !FLOATING_TESTS(float)' is true. > > I think you mean EXCEPTION_TESTS (float), and it seems reasonable: > > _Static_assert (!(EXCEPTION_SET_FORCES_TRAP && !EXCEPTION_TESTS(float)), > "EXCEPTION_SET_FORCES_TRAP only makes sense if the " > "architecture suports exceptions"); > >> >>> 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. >> >> 5. Succeed where it should not (powerpc/x87) ? > > Indeed. > >> >>> >>> So 1. and 2. are considered a regression, where 3. and 4. is the >>> expected result. >> >> ... and 5? > > Yes, and 1., 2., and 5. should be considered a regression. Hi Carlos, Updated patch below: -- From c10d1d59321d3fce0c532304b428dcef3c8cbb3a Mon Sep 17 00:00:00 2001 From: Adhemerval Zanella <adhemerval.zanella@linaro.org> Date: Tue, 24 Oct 2023 08:37:14 -0300 Subject: [PATCH 1/7] powerpc: Do not raise exception traps for fesetexcept/fesetexceptflag (BZ 30988) 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. 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. --- math/test-fesetexcept-traps.c | 24 ++++++++++++++++-------- math/test-fexcept-traps.c | 16 +++++++++------- sysdeps/powerpc/fpu/fesetexcept.c | 5 +++++ sysdeps/powerpc/fpu/fsetexcptflg.c | 9 ++++++++- 4 files changed, 38 insertions(+), 16 deletions(-) diff --git a/math/test-fesetexcept-traps.c b/math/test-fesetexcept-traps.c index 71b6e45b33..7efcd0343a 100644 --- a/math/test-fesetexcept-traps.c +++ b/math/test-fesetexcept-traps.c @@ -39,16 +39,24 @@ 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); + + _Static_assert (!(EXCEPTION_SET_FORCES_TRAP && !EXCEPTION_TESTS(float)), + "EXCEPTION_SET_FORCES_TRAP only makes sense if the " + "architecture suports exceptions"); + if (ret == 0) - puts ("fesetexcept (FE_ALL_EXCEPT) succeeded"); - else + { + if (EXCEPTION_SET_FORCES_TRAP) + { + puts ("unexpected fesetexcept success"); + result = 1; + } + } + else if (!EXCEPTION_SET_FORCES_TRAP) { puts ("fesetexcept (FE_ALL_EXCEPT) failed"); if (EXCEPTION_TESTS (float)) diff --git a/math/test-fexcept-traps.c b/math/test-fexcept-traps.c index 9701c3c320..998c241058 100644 --- a/math/test-fexcept-traps.c +++ b/math/test-fexcept-traps.c @@ -63,14 +63,16 @@ do_test (void) result = 1; } - if (EXCEPTION_SET_FORCES_TRAP) - { - puts ("setting exceptions traps, cannot test on this architecture"); - return 77; - } - /* The test is that this does not cause exception traps. */ + /* The test is that this does not cause exception traps. For architectures + where setting the exception might result in traps the function should + return a nonzero value. */ ret = fesetexceptflag (&saved, FE_ALL_EXCEPT); - if (ret != 0) + + _Static_assert (!(EXCEPTION_SET_FORCES_TRAP && !EXCEPTION_TESTS(float)), + "EXCEPTION_SET_FORCES_TRAP only makes sense if the " + "architecture suports exceptions"); + + if (ret != 0 && !EXCEPTION_SET_FORCES_TRAP) { puts ("fesetexceptflag failed"); result = 1; diff --git a/sysdeps/powerpc/fpu/fesetexcept.c b/sysdeps/powerpc/fpu/fesetexcept.c index 609a148a95..2850156d3a 100644 --- a/sysdeps/powerpc/fpu/fesetexcept.c +++ b/sysdeps/powerpc/fpu/fesetexcept.c @@ -31,6 +31,11 @@ fesetexcept (int excepts) & FE_INVALID_SOFTWARE)); if (n.l != u.l) { + if (n.l & fenv_exceptions_to_reg (excepts)) + /* Setting the exception flags may trigger a trap. ISO C 23 § 7.6.4.4 + does not allow it. */ + return -1; + fesetenv_register (n.fenv); /* Deal with FE_INVALID_SOFTWARE not being implemented on some chips. */ diff --git a/sysdeps/powerpc/fpu/fsetexcptflg.c b/sysdeps/powerpc/fpu/fsetexcptflg.c index 2b22f913c0..6517e8ea03 100644 --- a/sysdeps/powerpc/fpu/fsetexcptflg.c +++ b/sysdeps/powerpc/fpu/fsetexcptflg.c @@ -44,7 +44,14 @@ __fesetexceptflag (const fexcept_t *flagp, int excepts) This may cause floating-point exceptions if the restored state requests it. */ if (n.l != u.l) - fesetenv_register (n.fenv); + { + if (n.l & fenv_exceptions_to_reg (excepts)) + /* Setting the exception flags may trigger a trap. ISO C 23 § 7.6.4.4 + does not allow it. */ + return -1; + + fesetenv_register (n.fenv); + } /* Deal with FE_INVALID_SOFTWARE not being implemented on some chips. */ if (flag & FE_INVALID)
On 11/27/23 08:46, Adhemerval Zanella Netto wrote: > Hi Carlos, > > Updated patch below: LGTM, but because this is submitted under the existing thread patchwork doesn't consider this a submission. Reviewed-by: Carlos O'Donell <carlos@redhat.com> > -- > > From c10d1d59321d3fce0c532304b428dcef3c8cbb3a Mon Sep 17 00:00:00 2001 > From: Adhemerval Zanella <adhemerval.zanella@linaro.org> > Date: Tue, 24 Oct 2023 08:37:14 -0300 > Subject: [PATCH 1/7] powerpc: Do not raise exception traps for > fesetexcept/fesetexceptflag (BZ 30988) > > 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. > > 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. > --- > math/test-fesetexcept-traps.c | 24 ++++++++++++++++-------- > math/test-fexcept-traps.c | 16 +++++++++------- > sysdeps/powerpc/fpu/fesetexcept.c | 5 +++++ > sysdeps/powerpc/fpu/fsetexcptflg.c | 9 ++++++++- > 4 files changed, 38 insertions(+), 16 deletions(-) > > diff --git a/math/test-fesetexcept-traps.c b/math/test-fesetexcept-traps.c > index 71b6e45b33..7efcd0343a 100644 > --- a/math/test-fesetexcept-traps.c > +++ b/math/test-fesetexcept-traps.c > @@ -39,16 +39,24 @@ 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. */ OK. > ret = fesetexcept (FE_ALL_EXCEPT); > + > + _Static_assert (!(EXCEPTION_SET_FORCES_TRAP && !EXCEPTION_TESTS(float)), > + "EXCEPTION_SET_FORCES_TRAP only makes sense if the " > + "architecture suports exceptions"); OK. Good _Static_assert to avoid machine misconfiguration. > + > if (ret == 0) > - puts ("fesetexcept (FE_ALL_EXCEPT) succeeded"); > - else > + { > + if (EXCEPTION_SET_FORCES_TRAP) > + { > + puts ("unexpected fesetexcept success"); > + result = 1; > + } > + } > + else if (!EXCEPTION_SET_FORCES_TRAP) OK. Tests both sides pass/fail. I like this version. > { > puts ("fesetexcept (FE_ALL_EXCEPT) failed"); > if (EXCEPTION_TESTS (float)) > diff --git a/math/test-fexcept-traps.c b/math/test-fexcept-traps.c > index 9701c3c320..998c241058 100644 > --- a/math/test-fexcept-traps.c > +++ b/math/test-fexcept-traps.c > @@ -63,14 +63,16 @@ do_test (void) > result = 1; > } > > - if (EXCEPTION_SET_FORCES_TRAP) > - { > - puts ("setting exceptions traps, cannot test on this architecture"); > - return 77; > - } > - /* The test is that this does not cause exception traps. */ > + /* The test is that this does not cause exception traps. For architectures > + where setting the exception might result in traps the function should > + return a nonzero value. */ OK. > ret = fesetexceptflag (&saved, FE_ALL_EXCEPT); > - if (ret != 0) > + > + _Static_assert (!(EXCEPTION_SET_FORCES_TRAP && !EXCEPTION_TESTS(float)), > + "EXCEPTION_SET_FORCES_TRAP only makes sense if the " > + "architecture suports exceptions"); OK. > + > + if (ret != 0 && !EXCEPTION_SET_FORCES_TRAP) > { > puts ("fesetexceptflag failed"); > result = 1; OK. > diff --git a/sysdeps/powerpc/fpu/fesetexcept.c b/sysdeps/powerpc/fpu/fesetexcept.c > index 609a148a95..2850156d3a 100644 > --- a/sysdeps/powerpc/fpu/fesetexcept.c > +++ b/sysdeps/powerpc/fpu/fesetexcept.c > @@ -31,6 +31,11 @@ fesetexcept (int excepts) > & FE_INVALID_SOFTWARE)); > if (n.l != u.l) > { > + if (n.l & fenv_exceptions_to_reg (excepts)) > + /* Setting the exception flags may trigger a trap. ISO C 23 § 7.6.4.4 > + does not allow it. */ > + return -1; > + > fesetenv_register (n.fenv); > > /* Deal with FE_INVALID_SOFTWARE not being implemented on some chips. */ > diff --git a/sysdeps/powerpc/fpu/fsetexcptflg.c b/sysdeps/powerpc/fpu/fsetexcptflg.c > index 2b22f913c0..6517e8ea03 100644 > --- a/sysdeps/powerpc/fpu/fsetexcptflg.c > +++ b/sysdeps/powerpc/fpu/fsetexcptflg.c > @@ -44,7 +44,14 @@ __fesetexceptflag (const fexcept_t *flagp, int excepts) > This may cause floating-point exceptions if the restored state > requests it. */ > if (n.l != u.l) > - fesetenv_register (n.fenv); > + { > + if (n.l & fenv_exceptions_to_reg (excepts)) > + /* Setting the exception flags may trigger a trap. ISO C 23 § 7.6.4.4 > + does not allow it. */ > + return -1; > + > + fesetenv_register (n.fenv); > + } > > /* Deal with FE_INVALID_SOFTWARE not being implemented on some chips. */ > if (flag & FE_INVALID)
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) puts ("fesetexcept (FE_ALL_EXCEPT) succeeded"); - else + else if (!EXCEPTION_SET_FORCES_TRAP) { puts ("fesetexcept (FE_ALL_EXCEPT) failed"); if (EXCEPTION_TESTS (float)) diff --git a/math/test-fexcept-traps.c b/math/test-fexcept-traps.c index 9701c3c320..9b8f583ae6 100644 --- a/math/test-fexcept-traps.c +++ b/math/test-fexcept-traps.c @@ -63,14 +63,11 @@ do_test (void) result = 1; } - if (EXCEPTION_SET_FORCES_TRAP) - { - puts ("setting exceptions traps, cannot test on this architecture"); - return 77; - } - /* The test is that this does not cause exception traps. */ + /* The test is that this does not cause exception traps. For architectures + where setting the exception might result in traps the function should + return a nonzero value. */ ret = fesetexceptflag (&saved, FE_ALL_EXCEPT); - if (ret != 0) + if (ret != 0 && !EXCEPTION_SET_FORCES_TRAP) { puts ("fesetexceptflag failed"); result = 1; diff --git a/sysdeps/powerpc/fpu/fesetexcept.c b/sysdeps/powerpc/fpu/fesetexcept.c index 609a148a95..2850156d3a 100644 --- a/sysdeps/powerpc/fpu/fesetexcept.c +++ b/sysdeps/powerpc/fpu/fesetexcept.c @@ -31,6 +31,11 @@ fesetexcept (int excepts) & FE_INVALID_SOFTWARE)); if (n.l != u.l) { + if (n.l & fenv_exceptions_to_reg (excepts)) + /* Setting the exception flags may trigger a trap. ISO C 23 § 7.6.4.4 + does not allow it. */ + return -1; + fesetenv_register (n.fenv); /* Deal with FE_INVALID_SOFTWARE not being implemented on some chips. */ diff --git a/sysdeps/powerpc/fpu/fsetexcptflg.c b/sysdeps/powerpc/fpu/fsetexcptflg.c index 2b22f913c0..6517e8ea03 100644 --- a/sysdeps/powerpc/fpu/fsetexcptflg.c +++ b/sysdeps/powerpc/fpu/fsetexcptflg.c @@ -44,7 +44,14 @@ __fesetexceptflag (const fexcept_t *flagp, int excepts) This may cause floating-point exceptions if the restored state requests it. */ if (n.l != u.l) - fesetenv_register (n.fenv); + { + if (n.l & fenv_exceptions_to_reg (excepts)) + /* Setting the exception flags may trigger a trap. ISO C 23 § 7.6.4.4 + does not allow it. */ + return -1; + + fesetenv_register (n.fenv); + } /* Deal with FE_INVALID_SOFTWARE not being implemented on some chips. */ if (flag & FE_INVALID)