[ARM] ] Add support for fenv_private on ARM

Message ID 000c01cf3972$bd2b72f0$378258d0$@com
State Superseded
Headers show

Commit Message

Wilco Dijkstra March 6, 2014, 7:31 p.m. UTC
On Thu, 6 Mar 2014, Joseph Myers wrote:
> ARM_HAVE_VFP isn't suitable for use in #if; see the 
> sysdeps/unix/sysv/linux/arm/arm-features.h definition.  You need either a 
> new macro meaning "VFP is known at compile time to be available", or to 
> move to checks with "if" inside the functions.

Good point - it appears __SOFTFP__ is what I wanted:



> (The case of a soft-float build, VFP hardware available at runtime - the 
> one addressed by "if" conditionals - has other problems with exceptions 
> and rounding modes, that I think would best be addressed by use of IFUNCs 
> in libgcc for the relevant RTABI functions; see bug 10064.  Anyway, the 
> present patch is purely an optimization, so there's certainly no need for 
> it to cover all cases as long as it doesn't break them.)

I think the best option is to not change the behaviour of fenv when you 
use softfp but run on a CPU that supports VFP - especially since softfp 
doesn't support rounding modes or exceptions. 

I'm working on a patch to improve fenv with the same optimizations and
clean up the code, so can fix all the if (ARM_HAVE_VFP) to use __SOFTFP__.

On a related issue, the fenv testing code doesn't check the return code
of the fenv functions, which causes failures. It also assumes that fenv 
functions set errno when this is not part of the C99 spec. Is that on
purpose or a bug?

Wilco

Comments

Joseph Myers March 6, 2014, 10:32 p.m. UTC | #1
On Thu, 6 Mar 2014, Wilco wrote:

> > (The case of a soft-float build, VFP hardware available at runtime - the 
> > one addressed by "if" conditionals - has other problems with exceptions 
> > and rounding modes, that I think would best be addressed by use of IFUNCs 
> > in libgcc for the relevant RTABI functions; see bug 10064.  Anyway, the 
> > present patch is purely an optimization, so there's certainly no need for 
> > it to cover all cases as long as it doesn't break them.)
> 
> I think the best option is to not change the behaviour of fenv when you 
> use softfp but run on a CPU that supports VFP - especially since softfp 
> doesn't support rounding modes or exceptions. 

The best option for the present patch.  I'd like such a configuration to 
work properly (i.e. when the fenv functions succeed they also do affect 
arithmetic operations), but that needs the IFUNCs.

(Having VFP implementations of the RTABI functions in libgcc conditioned 
at compile time on __SOFTFP__ is a good idea simply for efficiency, if a 
soft-float object file is linked with libraries built for VFP - as well as 
making libgcc smaller when built for VFP, as the VFP implementations 
should be smaller than the soft-float ones (cf. the recent change in 
libgcc on MIPS to use minimal implementations of various libgcc functions 
for hard float, rather than the previous soft-float versions also used in 
hard-float libraries).  IFUNCs for libgcc built for soft float is a step 
beyond that, which would make libgcc consistent with glibc.)

> I'm working on a patch to improve fenv with the same optimizations and
> clean up the code, so can fix all the if (ARM_HAVE_VFP) to use __SOFTFP__.

But that's not correct in the cases where a runtime check is used (and in 
general, if () conditions are better than #if because they mean both cases 
still get checked for syntax on every compilation).

I suppose logically there are two meaningful soft-float configurations, 
one with runtime checks for VFP in both libgcc and glibc, and one with no 
such checks in either place and the present VFP support in glibc disabled.  
The former makes sense for general-purpose distributions supporting a wide 
range of hardware (e.g. Debian armel), by producing binaries that adapt 
automatically to the functionality of the hardware they run on; the latter 
if the binaries are only intended for specific soft-float hardware.

> On a related issue, the fenv testing code doesn't check the return code
> of the fenv functions, which causes failures. It also assumes that fenv 
> functions set errno when this is not part of the C99 spec. Is that on
> purpose or a bug?

That should just be math/test-fenv.  There's a trade-off between:

* detecting a failure as meaning the feature is unsupported and so making 
the test quietly PASS, when actually the failure is not expected on the 
given platform and so it should have FAILed, and

* treating any failure as meaning the test fails, when actually it might 
be expected on a given platform.

I suppose I'd be inclined to say that test-fenv should use macros from 
math-tests.h to indicate which functions are expected to work on a given 
platform and which are expected to fail (on ARM you'd need some 
initialization step to determine whether traps for exceptions are 
supported).  So unless a platform says certain failures are OK, they would 
make the test as a whole fail.

As for errno setting, generally ISO C says little about errno but the norm 
for POSIX and glibc functions is to set errno on error unless there is 
some other defined way to indicate what error occurred (e.g. functions 
defined to return an error number).
Marcus Shawcroft March 7, 2014, 8:34 a.m. UTC | #2
On 6 March 2014 22:32, Joseph S. Myers <joseph@codesourcery.com> wrote:

> That should just be math/test-fenv.  There's a trade-off between:
>
> * detecting a failure as meaning the feature is unsupported and so making
> the test quietly PASS, when actually the failure is not expected on the
> given platform and so it should have FAILed, and
>
> * treating any failure as meaning the test fails, when actually it might
> be expected on a given platform.
>
> I suppose I'd be inclined to say that test-fenv should use macros from
> math-tests.h to indicate which functions are expected to work on a given
> platform and which are expected to fail (on ARM you'd need some
> initialization step to determine whether traps for exceptions are
> supported).  So unless a platform says certain failures are OK, they would
> make the test as a whole fail.
>
> As for errno setting, generally ISO C says little about errno but the norm
> for POSIX and glibc functions is to set errno on error unless there is
> some other defined way to indicate what error occurred (e.g. functions
> defined to return an error number).

IIRC the relevant part of test-fenv.c in the context of this thread is:

  errno = 0;
  fesetenv (FE_NOMASK_ENV);
  status = errno;
  fesetenv (&saved);
  if (status == ENOSYS)

There is one target in the glibc tree that appears to make use of
errno in this manner, sysdeps/powerpc/fpu/fesetenv.c has a path that
calls __fe_nomask_env_priv() setting ENOSYS, curiously this fesetenv()
implementation always returns 0 even when setting errno, which seems
rather odd to me, I wonder if  this is by design or simply a hang over
from the days before fesetenv() gained a return value?

Given Joseph's comments above and the existing use of errno in this
context it doesn;t seem unreasonable to me that both arm and aarch64
might also set ENOSYS on platforms that do not support trapping
exceptions.

/Marcus
Joseph Myers March 7, 2014, 5:08 p.m. UTC | #3
On Fri, 7 Mar 2014, Marcus Shawcroft wrote:

> IIRC the relevant part of test-fenv.c in the context of this thread is:
> 
>   errno = 0;
>   fesetenv (FE_NOMASK_ENV);
>   status = errno;
>   fesetenv (&saved);
>   if (status == ENOSYS)
> 
> There is one target in the glibc tree that appears to make use of
> errno in this manner, sysdeps/powerpc/fpu/fesetenv.c has a path that
> calls __fe_nomask_env_priv() setting ENOSYS, curiously this fesetenv()
> implementation always returns 0 even when setting errno, which seems
> rather odd to me, I wonder if  this is by design or simply a hang over
> from the days before fesetenv() gained a return value?

Given the lack of other cases setting errno in <fenv.h> functions, it 
might be better here simply to check the return value - and not to check 
errno at all.  But I think allowing failure should be conditional on 
something from math-tests.h saying the architecture intends that this 
feature might not be supported.

Patch

diff --git a/sysdeps/arm/fenv_private.h b/sysdeps/arm/fenv_private.h
index 6c65cfa..0b73816 100644
--- a/sysdeps/arm/fenv_private.h
+++ b/sysdeps/arm/fenv_private.h
@@ -23,7 +23,7 @@ 
 #include <fpu_control.h>
 #include <arm-features.h>
 
-#if ARM_HAVE_VFP
+#ifndef __SOFTFP__
 
 static __always_inline void
 libc_feholdexcept_vfp (fenv_t *envp)