Re: [PATCH] In _IEEE_LIBM mode, use weak symbols instead of wrapper funcs [v2]

[Craig Howland <howland at LGSInnovations dot com>]

On 09/20/2018 05:51 PM, Keith Packard wrote:
> When the math library is compiled to just use bare IEEE_LIBM mode, many
> public functions are just wrappers around the __ieee754 version.
> Eliminate the extra function by creating a weak alias symbol for the
> public name directly from the ieee754 name.
>
> v2:
> 	Use __weak_reference macro instead of using
> 	'__attribute__((weak, alias' to make this code portable
> 	to non-ELF systems.
>
> Signed-off-by: Keith Packard <keithp@keithp.com>
> ---
>   newlib/libm/math/e_acos.c       |  4 ++++
>   ...
>   87 files changed, 303 insertions(+), 197 deletions(-)
Unless I'm missing something, there seems to be a fundamental problem with the 
approach in that the regular function definitions are unconditionally eliminated 
when the weak aliases are defined. So while this will work fine for targets 
which can do the aliases, the needed function wrappers are missing for when 
aliases do not work.  One example shown.
> diff --git a/newlib/libm/math/e_acos.c b/newlib/libm/math/e_acos.c
> index 319b1d56f..e2104a675 100644
> --- a/newlib/libm/math/e_acos.c
> +++ b/newlib/libm/math/e_acos.c
> @@ -59,6 +59,10 @@ qS2 =  2.02094576023350569471e+00, /* 0x40002AE5, 0x9C598AC8 */
>   qS3 = -6.88283971605453293030e-01, /* 0xBFE6066C, 0x1B8D0159 */
>   qS4 =  7.70381505559019352791e-02; /* 0x3FB3B8C5, 0xB12E9282 */
>   
> +#ifdef _IEEE_LIBM
> +__weak_reference(__ieee754_acos, acos);
> +#endif
> +
>   #ifdef __STDC__
>   	double __ieee754_acos(double x)
>   #else
> ...
OK, so a weak alias of __ieee754_acos() for acos() is added when 
defined(_IEEE_LIBM).
> diff --git a/newlib/libm/math/w_acos.c b/newlib/libm/math/w_acos.c
> index eb3e20111..2cd5247e2 100644
> --- a/newlib/libm/math/w_acos.c
> +++ b/newlib/libm/math/w_acos.c
> @@ -70,6 +70,7 @@ MATHREF
>   #include "fdlibm.h"
>   #include <errno.h>
>   
> +#ifndef _IEEE_LIBM
>   #ifndef _DOUBLE_IS_32BITS
>   
>   #ifdef __STDC__
> @@ -79,9 +80,6 @@ MATHREF
>   	double x;
>   #endif
>   {
> -#ifdef _IEEE_LIBM
> -	return __ieee754_acos(x);
> -#else
>   	double z;
>          	struct exception exc;
>          	z = __ieee754_acos(x);
> @@ -103,7 +101,7 @@ MATHREF
>   	    return exc.retval;
>   	} else
>   	    return z;
> -#endif
>   }
>   
>   #endif /* defined(_DOUBLE_IS_32BITS) */
> +#endif /* defined(_IEEE_LIBM) */
But then the entire acos() function is deleted when defined(_IEEE_LIBM).  This 
is fine when the alias works, but not when the alias does not work.  That is, 
this gate also needs to know if the alias will work, rather than being based 
only on _IEEE_LIBM.

In addition, this method can potentially introduce a change in link-time 
behavior, because the library now has the primary function names as weak aliases 
instead of definite functions.  This won't necessarily make a difference while 
linking, but it could.  (While you could guess that the vast majority of uses 
would have no trouble at all, the question is if there are any which would.)  
This may or may not be acceptable, but the possibility needs to be considered 
when evaluating this approach.  (I'm pretty sure it would not affect any of my 
targets.)  Even ignoring that possibility, just the principle of having primary 
C library functions in the library as weak aliases seems like a bad idea.

Why not just abandon the alias approach and instead just re-name the ieee 
functions?  It solves both of the problems mentioned.  These edits are half-way 
there, as they get rid of the wrapper functions. But instead of adding the weak 
references, map the ieee names with the preprocessor.  It would have to be done 
in something like fdlibm.h because there are some internal calls from ieee to 
ieee, but the basic name-mapping collection could readily be retargeted.

Taking a step back, there's a higher-level question.  The Newlib math libary is 
already quite goofy with the variations in error handing, making multiple 
flavors, etc.  Is it perhaps time to give this strangeness up and just make them 
so that they are C/POSIX compliant?  This too would achieve the desired goal of 
getting rid of the wrappers.

Craig