From: Carlos O'Donell <carlos@redhat.com>
To: Adhemerval Zanella Netto <adhemerval.zanella@linaro.org>,
libc-alpha@sourceware.org, Bruno Haible <bruno@clisp.org>
Subject: Re: [PATCH v2 1/7] powerpc: Do not raise exception traps for fesetexcept/fesetexceptflag (BZ 30988)
Date: Fri, 24 Nov 2023 11:22:00 -0500 [thread overview]
Message-ID: <e3ea5058-c562-09d4-b175-a80aaeca353f@redhat.com> (raw)
In-Reply-To: <f78f4034-2a67-41b3-b5d5-1fb36f9b265f@linaro.org>
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.
> }
--
Cheers,
Carlos.
next prev parent reply other threads:[~2023-11-24 16:22 UTC|newest]
Thread overview: 33+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-11-06 13:27 [PATCH v2 0/7] Multiple floating-point environment fixes Adhemerval Zanella
2023-11-06 13:27 ` [PATCH v2 1/7] powerpc: Do not raise exception traps for fesetexcept/fesetexceptflag (BZ 30988) Adhemerval Zanella
2023-11-06 16:08 ` Carlos O'Donell
2023-11-06 16:50 ` Adhemerval Zanella Netto
2023-11-06 17:02 ` Carlos O'Donell
2023-11-06 17:11 ` Adhemerval Zanella Netto
2023-11-06 17:37 ` Adhemerval Zanella Netto
2023-11-06 17:38 ` Carlos O'Donell
2023-11-06 17:56 ` Adhemerval Zanella Netto
2023-11-06 20:46 ` Adhemerval Zanella Netto
2023-11-23 21:47 ` Carlos O'Donell
2023-11-24 12:28 ` Adhemerval Zanella Netto
2023-11-24 12:37 ` Adhemerval Zanella Netto
2023-11-24 16:22 ` Carlos O'Donell [this message]
2023-11-24 17:53 ` Adhemerval Zanella Netto
2023-11-24 18:15 ` Carlos O'Donell
2023-11-24 18:46 ` Adhemerval Zanella Netto
2023-11-27 13:46 ` Adhemerval Zanella Netto
2023-12-19 14:57 ` Carlos O'Donell
2023-11-06 13:27 ` [PATCH v2 2/7] i686: Do not raise exception traps on fesetexcept (BZ 30989) Adhemerval Zanella
2023-11-06 16:14 ` Carlos O'Donell
2023-11-06 13:27 ` [PATCH v2 3/7] x86: Do not raises floating-point exception traps on fesetexceptflag (BZ 30990) Adhemerval Zanella
2023-11-06 16:16 ` Carlos O'Donell
2023-11-06 13:27 ` [PATCH v2 4/7] manual: Clarify undefined behavior of feenableexcept (BZ 31019) Adhemerval Zanella
2023-11-06 16:17 ` Carlos O'Donell
2023-11-06 13:27 ` [PATCH v2 5/7] riscv: Fix feenvupdate with FE_DFL_ENV (BZ 31022) Adhemerval Zanella
2023-11-06 16:19 ` Carlos O'Donell
2023-11-06 13:27 ` [PATCH v2 6/7] alpha: Fix fesetexceptflag (BZ 30998) Adhemerval Zanella
2023-11-06 16:54 ` Carlos O'Donell
2023-11-06 17:36 ` Bruno Haible
2023-11-06 18:15 ` Carlos O'Donell
2023-11-06 13:27 ` [PATCH v2 7/7] hppa: Fix undefined behaviour in feclearexcept (BZ 30983) Adhemerval Zanella
2023-11-06 16:57 ` Carlos O'Donell
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=e3ea5058-c562-09d4-b175-a80aaeca353f@redhat.com \
--to=carlos@redhat.com \
--cc=adhemerval.zanella@linaro.org \
--cc=bruno@clisp.org \
--cc=libc-alpha@sourceware.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).