Re: [PATCH]: Fix sparc localplt and C++ type testsuite failures.

[Jakub Jelinek <jakub at redhat dot com>]

On Thu, May 08, 2008 at 04:02:19PM -0700, David Miller wrote:
> The first part of the change reworks how system calls are emitted on
> sparc.  Previously we emitted stubs on the error path, out of line,
> that invoked a call to __errno_location from the inline asm.  This
> doesn't get wrapped and renamed properly, so we'd get bogus non-local
> PLT entries for __errno_location in the shared libraries.
> 
> I tried to wrap them using CPP string pasting and other tricks to get
> the symbol name right in these inline assembler strings, but the
> amount of ifdefs became messy, complicated, and after trying for two
> straight days I could not get all cases to work correctly all at once.

I've tested just sparc64 so far, but it isn't that hard, the following
patch was all that was needed to get rid of __errno_location PLT slot in
libc.so.

--- sysdeps/unix/sysv/linux/sparc/sparc64/sysdep.h.jj   2006-03-05 20:33:03.000000000 -0500
+++ sysdeps/unix/sysv/linux/sparc/sparc64/sysdep.h      2008-05-20 04:18:55.000000000 -0400
@@ -106,12 +106,19 @@ ENTRY(name);                                      \
 
 #else  /* __ASSEMBLER__ */
 
+#if defined SHARED && defined DO_VERSIONING && defined PIC \
+    && !defined NO_HIDDEN && !defined NOT_IN_libc
+# define CALL_ERRNO_LOCATION "call   __GI___errno_location;"
+#else
+# define CALL_ERRNO_LOCATION "call   __errno_location;"
+#endif
+
 #define __SYSCALL_STRING                                               \
        "ta     0x6d;"                                                  \
        "bcc,pt %%xcc, 1f;"                                             \
        " nop;"                                                         \
        "save   %%sp, -192, %%sp;"                                      \
-       "call   __errno_location;"                                      \
+       CALL_ERRNO_LOCATION                                             \
        " nop;"                                                         \
        "st     %%i0,[%%o0];"                                           \
        "restore %%g0, -1, %%o0;"                                       \
@@ -122,7 +129,7 @@ ENTRY(name);                                        \
        "bcc,pt %%xcc, 1f;"                                             \
        " sub   %%o1, 1, %%o1;"                                         \
        "save   %%sp, -192, %%sp;"                                      \
-       "call   __errno_location;"                                      \
+       CALL_ERRNO_LOCATION                                             \
        " mov   -1, %%i1;"                                              \
        "st     %%i0,[%%o0];"                                           \
        "restore %%g0, -1, %%o0;"                                       \
--- sysdeps/unix/sysv/linux/sparc/sparc64/brk.S.jj      2006-03-05 20:33:03.000000000 -0500
+++ sysdeps/unix/sysv/linux/sparc/sparc64/brk.S 2008-05-20 04:20:05.000000000 -0400
@@ -86,7 +86,11 @@ ENTRY (__brk)
 #endif
        st      %o0, [%g1]
 #else
+#ifndef NOT_IN_libc
+       call    HIDDEN_JUMPTARGET(__errno_location)
+#else
        call    __errno_location
+#endif
         mov    %o0,%l1
        st      %l1, [%o0]
 #endif

While it is true that both approaches result in the same size of
libc.so's .text, the thing I don't like on your patch is that the fast path
(most syscalls succeed) is now longer, say pselect now has:

  80:   91 d0 20 6d     ta  0x6d
  84:   94 10 00 00     mov  %g0, %o2
  88:   2a 60 00 02     bcs,a,pn   %xcc, 90 <__libc_pselect+0x70>
  8c:   94 10 20 01     mov  1, %o2
  90:   0a c2 80 07     brnz,pn   %o2, ac <__libc_pselect+0x8c>
  94:   b0 10 00 08     mov  %o0, %i0
  98:   80 a6 3f ff     cmp  %i0, -1
  9c:   02 40 00 0a     be,pn   %icc, c4 <__libc_pselect+0xa4>
  a0:   03 00 00 00     sethi  %hi(0), %g1
                        a0: R_SPARC_TLS_IE_HI22 __libc_errno
  a4:   81 cf e0 08     rett  %i7 + 8
  a8:   91 3a 20 00     sra  %o0, 0, %o0

There is an extra conditional branch when the syscall succeeded, one that
depends on a conditionally set register immediately before that.

If you don't like the extra register window in the syscall failure case,
perhaps we could have a helper routine for this instead and replace
save %sp, -192, %sp;
CALL_ERRNO_LOCATION;
nop;
st %i0, [%o0];
restore %g0, -1, %o0;
with say
mov %o7, %o2;
sethi %hi(_GLOBAL_OFFSET_TABLE_-4), %o3;
call __inline_syscall_set_errno;
add %o3, %lo(_GLOBAL_OFFSET_TABLE_+4), %o3

and have:

.globl __inline_syscall_set_errno;
.hidden __inline_syscall_set_errno;
__inline_syscall_set_errno:
	sethi	%tie_hi22(__libc_errno), %o4
	add	%o7, %o3, %o3
	add	%o4, %tie_lo10(__libc_errno), %o4
	ldx	[%o3 + %o4], %o4, %tie_ldx(__libc_errno)
	st	%o0, [%g7 + %o4]
	mov	-1, %o0
	jmpl	%o7 + 8, %g0
	 mov	%o2, %o7

(that would actually save 2 register windows, one save in INLINE_SYSCALL
and one in __errno_location).  While the patch above is tested,
the above snippets are completely untested.

The other parts of the patch (scripts/data, or removal of %g* registers
from __SYSCALL_CLOBBERS) look good.

	Jakub