diff mbox

[RFC] Using protected for math symbols

Message ID DB6PR0801MB2053F4FB2315A96BA5D7E32F837E0@DB6PR0801MB2053.eurprd08.prod.outlook.com
State New, archived
Headers show

Commit Message

Wilco Dijkstra Sept. 29, 2017, 4:46 p.m. UTC
GLIBC uses non-standard names internally for math calls. As a result
even basic functions like sqrt and fabs are not inlined by default.
I recently posted patches to inline sqrt and fabs. In order for this to work
in cases GCC doesn't inline, asm redirects and conditional defines
are needed.

An alternative approach to this is to completely remove the __sqrt
variants and use protected symbol visibility. This means math functions
are defined using their real name, internal calls use the real name and
don't generate PLT calls, and no more weak/redirect magic is needed.

It seems much cleaner to do it this way, so my question is, are there
any issues with this approach? 

The patch below shows how it might look like for sqrt as the difference
between the current approach and the proposed one. Only generic code
has been changed - target specific assembly code may need similar
edits:

--

Comments

Joseph Myers Sept. 29, 2017, 4:58 p.m. UTC | #1
On Fri, 29 Sep 2017, Wilco Dijkstra wrote:

> An alternative approach to this is to completely remove the __sqrt
> variants and use protected symbol visibility. This means math functions
> are defined using their real name, internal calls use the real name and
> don't generate PLT calls, and no more weak/redirect magic is needed.

Protected visibility is best avoided in general.

https://www.airs.com/blog/archives/307

> -__sqrt (double x)
> +sqrt (double x)
>  {
>    if (__builtin_expect (isless (x, 0.0), 0) && _LIB_VERSION != _IEEE_)
>      return __kernel_standard (x, x, 26); /* sqrt(negative) */
>  
>    return __ieee754_sqrt (x);
>  }
> -libm_alias_double (__sqrt, sqrt)

Having names such as __sqrt is convenient even if they aren't called 
anywhere, as it allows libm_alias_double to be used as a uniform way of 
getting sqrt, sqrtl (in long double = double case), sqrtl compat symbol 
(in case where long double used to = double), sqrtf64 (in future) and 
sqrtf32x (in future) all defined at once.
Alexander Monakov Sept. 29, 2017, 5:08 p.m. UTC | #2
On Fri, 29 Sep 2017, Wilco Dijkstra wrote:
> GLIBC uses non-standard names internally for math calls. As a result
> even basic functions like sqrt and fabs are not inlined by default.
> I recently posted patches to inline sqrt and fabs. In order for this to work
> in cases GCC doesn't inline, asm redirects and conditional defines
> are needed.
> 
> An alternative approach to this is to completely remove the __sqrt
> variants and use protected symbol visibility. This means math functions
> are defined using their real name, internal calls use the real name and
> don't generate PLT calls, and no more weak/redirect magic is needed.
> 
> It seems much cleaner to do it this way, so my question is, are there
> any issues with this approach? 

Yes, you would need to post-process libc.so to change protected
visibility to default, otherwise LLD will fail to link non-PIE
executables that need PLT slots for such protected functions.  Their
position is that otherwise those functions may have two different addresses:
from the point of view of libc.so (the address of the implementation)
and any other loaded module (the address of the PLT slot in the executable),
and the linker should not allow that by default.

https://bugs.llvm.org/show_bug.cgi?id=32425

Alexander
Florian Weimer Sept. 29, 2017, 7:34 p.m. UTC | #3
On 09/29/2017 06:46 PM, Wilco Dijkstra wrote:
> An alternative approach to this is to completely remove the __sqrt
> variants and use protected symbol visibility. This means math functions
> are defined using their real name, internal calls use the real name and
> don't generate PLT calls, and no more weak/redirect magic is needed.

I don't think this allows us to address namespace issues with static 
linking.

Thanks,
Florian
diff mbox

Patch

diff --git a/include/math.h b/include/math.h
index e176b090caa9f07c226aff0f9c6c5bf34740f2ec..e6be1ac349c75b2ddd16f17384bb9267e15984dd 100644
--- a/include/math.h
+++ b/include/math.h
@@ -55,14 +55,10 @@  libm_hidden_proto (__expf128)
 libm_hidden_proto (__expm1f128)
 # endif
 
-# ifndef NO_SQRT_REDIRECT
-/* Declare sqrt for use within GLIBC.  Sqrt will typically inline into a
-   single instruction.  Use an asm to avoid use of PLTs if it doesn't.  */
-float sqrtf (float) asm ("__ieee754_sqrtf");
-double sqrt (double) asm ("__ieee754_sqrt");
-#  ifndef __NO_LONG_DOUBLE_MATH
-long double sqrtl (long double) asm ("__ieee754_sqrtl");
-#  endif
+float sqrtf (float)  __attribute__ ((visibility ("protected")));
+double sqrt (double)  __attribute__ ((visibility ("protected")));
+# ifndef __NO_LONG_DOUBLE_MATH
+long double sqrtl (long double)  __attribute__ ((visibility ("protected")));
 # endif
 
 #endif
diff --git a/math/w_sqrt_compat.c b/math/w_sqrt_compat.c
index fe068af9597ffb0f15b32cd454b4c34ba8bc060e..560cab2eb813df3ee56a2f88a835068139be0ac9 100644
--- a/math/w_sqrt_compat.c
+++ b/math/w_sqrt_compat.c
@@ -16,7 +16,6 @@ 
    License along with the GNU C Library; if not, see
    <http://www.gnu.org/licenses/>.  */
 
-#define NO_SQRT_REDIRECT
 #include <math.h>
 #include <math_private.h>
 #include <math-svid-compat.h>
@@ -26,12 +25,11 @@ 
 #if LIBM_SVID_COMPAT
 /* wrapper sqrt */
 double
-__sqrt (double x)
+sqrt (double x)
 {
   if (__builtin_expect (isless (x, 0.0), 0) && _LIB_VERSION != _IEEE_)
     return __kernel_standard (x, x, 26); /* sqrt(negative) */
 
   return __ieee754_sqrt (x);
 }
-libm_alias_double (__sqrt, sqrt)
 #endif
diff --git a/math/w_sqrt_template.c b/math/w_sqrt_template.c
index 235c263e60c85422e41bca7c817148b66030438f..58b95d0f3751e05760d9841a37cef7217c0caccb 100644
--- a/math/w_sqrt_template.c
+++ b/math/w_sqrt_template.c
@@ -21,20 +21,18 @@ 
    for each floating-point type.  */
 #if __USE_WRAPPER_TEMPLATE
 
-#define NO_SQRT_REDIRECT
 # include <errno.h>
 # include <fenv.h>
 # include <math.h>
 # include <math_private.h>
 
 FLOAT
-M_DECL_FUNC (__sqrt) (FLOAT x)
+M_DECL_FUNC (sqrt) (FLOAT x)
 {
   if (__glibc_unlikely (isless (x, M_LIT (0.0))))
     /* Domain error: sqrt(x<-0).  */
     __set_errno (EDOM);
   return M_SUF (__ieee754_sqrt) (x);
 }
-declare_mgen_alias (__sqrt, sqrt)
 
 #endif /* __USE_WRAPPER_TEMPLATE.  */
diff --git a/math/w_sqrtf_compat.c b/math/w_sqrtf_compat.c
index 880be0936d9a20f9aa75cf2d66000f0c621f090f..1ed54f1e522d0cf179aa2eed3e0870defc87d9e3 100644
--- a/math/w_sqrtf_compat.c
+++ b/math/w_sqrtf_compat.c
@@ -16,7 +16,6 @@ 
    License along with the GNU C Library; if not, see
    <http://www.gnu.org/licenses/>.  */
 
-#define NO_SQRT_REDIRECT
 #include <math.h>
 #include <math_private.h>
 #include <math-svid-compat.h>
@@ -26,12 +25,11 @@ 
 #if LIBM_SVID_COMPAT
 /* wrapper sqrtf */
 float