diff mbox series

[11/15] math: Fix acos template for arguments greater than 1

Message ID 20240327164527.3717523-12-adhemerval.zanella@linaro.org
State New
Headers show
Series Fix some libm static issues | expand

Commit Message

Adhemerval Zanella March 27, 2024, 4:45 p.m. UTC
The template is used by some ABsI for static build, and it fails set
the expected floating exceptions if the argument is outside of the
range (on x86_64 this triggers an overflow calculation in
__ieee754_acos).

Checked on x86_64-linux-gnu.
---
 math/Makefile          | 1 +
 math/w_acos_template.c | 9 +++++++--
 2 files changed, 8 insertions(+), 2 deletions(-)

Comments

Joseph Myers March 27, 2024, 5:14 p.m. UTC | #1
On Wed, 27 Mar 2024, Adhemerval Zanella wrote:

> The template is used by some ABsI for static build, and it fails set
> the expected floating exceptions if the argument is outside of the
> range (on x86_64 this triggers an overflow calculation in
> __ieee754_acos).

Patches 11 through 15 all seem incorrect; it's the responsibility of the 
__ieee754_* functions to raise the correct exceptions, not of the 
wrappers.  Please make sure you don't have a compiler with a bug like 
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=95115 miscompiling those 
__ieee754_* functions.
Adhemerval Zanella March 27, 2024, 5:58 p.m. UTC | #2
On 27/03/24 14:14, Joseph Myers wrote:
> On Wed, 27 Mar 2024, Adhemerval Zanella wrote:
> 
>> The template is used by some ABsI for static build, and it fails set
>> the expected floating exceptions if the argument is outside of the
>> range (on x86_64 this triggers an overflow calculation in
>> __ieee754_acos).
> 
> Patches 11 through 15 all seem incorrect; it's the responsibility of the 
> __ieee754_* functions to raise the correct exceptions, not of the 
> wrappers.  Please make sure you don't have a compiler with a bug like 
> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=95115 miscompiling those 
> __ieee754_* functions.
> 

The failures are both:

math/test-float64x-acos-static
math/test-ldouble-acos-static

$ cat math/test-ldouble-acos-static.out
testing long double (without inline functions)
Failure: acos (max_value): Exception "Overflow" set
Failure: acos (-max_value): Exception "Overflow" set
Failure: acos_downward (max_value): Exception "Overflow" set
Failure: acos_downward (-max_value): Exception "Overflow" set
Failure: acos_towardzero (max_value): Exception "Overflow" set
Failure: acos_towardzero (-max_value): Exception "Overflow" set
Failure: acos_upward (max_value): Exception "Overflow" set
Failure: acos_upward (-max_value): Exception "Overflow" set

Test suite completed:
  452 test cases plus 448 tests for exception flags and
    448 tests for errno executed.
  8 errors occurred.

And I think it is unrelated to gcc PR95115 because x86_64/i686 will use
and specific sysdeps/i386/fpu/e_acosl.c that explicit does not handle this
input case for overflow exceptions.  For shared build this case is
handle by w_acosl_compat.c:

  if (__builtin_expect (isgreater (fabsl (x), 1.0L), 0)
      && _LIB_VERSION != _IEEE_)
    {
      /* acos(|x|>1) */
      feraiseexcept (FE_INVALID);
      return __kernel_standard_l (x, x, 201);
    }

And that's why I though following the same logic on template would be
better.  But I think maybe we should fix on x86_64 implementation instead.
Joseph Myers March 27, 2024, 7:04 p.m. UTC | #3
On Wed, 27 Mar 2024, Adhemerval Zanella Netto wrote:

> And I think it is unrelated to gcc PR95115 because x86_64/i686 will use
> and specific sysdeps/i386/fpu/e_acosl.c that explicit does not handle this
> input case for overflow exceptions.  For shared build this case is
> handle by w_acosl_compat.c:
> 
>   if (__builtin_expect (isgreater (fabsl (x), 1.0L), 0)
>       && _LIB_VERSION != _IEEE_)
>     {
>       /* acos(|x|>1) */
>       feraiseexcept (FE_INVALID);
>       return __kernel_standard_l (x, x, 201);
>     }

The compat code is dealing with the possibility of SVID exceptions, which 
isn't relevant here.

> And that's why I though following the same logic on template would be
> better.  But I think maybe we should fix on x86_64 implementation instead.

Yes, we should fix the x86_64 implementation.

Such issues in dbl-64, flt-32 or ldbl-128 sources would largely have been 
fixed (modulo compiler bugs) when we started adding new architectures 
after new architectures stopped using the compat wrappers - but any issues 
for ldbl-128ibm, ldbl-96 or architecture-specific sources wouldn't have 
been detected then.
Adhemerval Zanella March 27, 2024, 7:31 p.m. UTC | #4
On 27/03/24 16:04, Joseph Myers wrote:
> On Wed, 27 Mar 2024, Adhemerval Zanella Netto wrote:
> 
>> And I think it is unrelated to gcc PR95115 because x86_64/i686 will use
>> and specific sysdeps/i386/fpu/e_acosl.c that explicit does not handle this
>> input case for overflow exceptions.  For shared build this case is
>> handle by w_acosl_compat.c:
>>
>>   if (__builtin_expect (isgreater (fabsl (x), 1.0L), 0)
>>       && _LIB_VERSION != _IEEE_)
>>     {
>>       /* acos(|x|>1) */
>>       feraiseexcept (FE_INVALID);
>>       return __kernel_standard_l (x, x, 201);
>>     }
> 
> The compat code is dealing with the possibility of SVID exceptions, which 
> isn't relevant here.

I meant that __ieee754_acosl in being called in both code paths, but 
the compat code returns early without calling __ieee754_acosl for the
possible problematic code path.

> 
>> And that's why I though following the same logic on template would be
>> better.  But I think maybe we should fix on x86_64 implementation instead.
> 
> Yes, we should fix the x86_64 implementation.
> 
> Such issues in dbl-64, flt-32 or ldbl-128 sources would largely have been 
> fixed (modulo compiler bugs) when we started adding new architectures 
> after new architectures stopped using the compat wrappers - but any issues 
> for ldbl-128ibm, ldbl-96 or architecture-specific sources wouldn't have 
> been detected then.
> 

I will drop this change and rather focus on the original issues on missing
symbols. I will open some bug against the x86 implementation so we keep
track of these issues with static linkage.
diff mbox series

Patch

diff --git a/math/Makefile b/math/Makefile
index af1909d0c7..2b2683a9fa 100644
--- a/math/Makefile
+++ b/math/Makefile
@@ -368,6 +368,7 @@  $(libm-test-c-narrow-obj): $(objpfx)libm-test%.c: libm-test%.inc \
 
 
 libm-test-funcs-auto-static = \
+  acos \
   exp10 \
   # libm-test-funcs-auto-static
 libm-test-funcs-noauto-static = \
diff --git a/math/w_acos_template.c b/math/w_acos_template.c
index fe6d6c01db..f4b0e91ae1 100644
--- a/math/w_acos_template.c
+++ b/math/w_acos_template.c
@@ -30,8 +30,13 @@  FLOAT
 M_DECL_FUNC (__acos) (FLOAT x)
 {
   if (__glibc_unlikely (isgreater (M_FABS (x), M_LIT (1.0))))
-    /* Domain error: acos(|x|>1).  */
-    __set_errno (EDOM);
+    {
+      /* Domain error: acos(|x|>1).  */
+      __feraiseexcept (FE_INVALID);
+      __set_errno (EDOM);
+      return __builtin_nanl ("");
+    }
+
   return M_SUF (__ieee754_acos) (x);
 }
 declare_mgen_alias (__acos, acos)