diff mbox

MIPS/libgcc: Avoid the PLT in MIPS16 stub calls

Message ID alpine.DEB.1.10.1307302351120.32382@tp.orcam.me.uk
State Superseded
Headers show

Commit Message

Maciej W. Rozycki Aug. 1, 2013, 5:11 p.m. UTC
Hi,

 As originally signalled here:

http://gcc.gnu.org/ml/gcc-patches/2012-02/msg00140.html

here is a change to prevent MIPS16 stub calls from being made through the 
PLT by making them hidden.  This is needed to avoid $2 and possibly $3 
from being clobbered by MIPS16 code in the PLT.  Originally I thought only 
return stubs were affected, but I have since realised all the libgcc stubs 
rely on $2, so none can be called via the PLT.  This breakage can be 
triggered by linking with the `-shared-libgcc' option; otherwise libgcc.a 
code is used avoiding the problem.

 Since these stubs are a preexisting interface I have decided there is no 
need to go through the pain of link-once functions as you did for 
__mips16_rdhwr.  I think we'd only need them if we changed the API, at 
which point we'd have to keep current libgcc bits for preexisting object 
code anyway.  For the same reason this change retains libgcc_s.so bits for 
preexisting executables and shared libraries, but stops them being 
exported for new static links so that libgcc.a bits are always used 
instead.

 I have tested this change for the mips-linux-gnu target and o32/MIPS32r2 
multilib with `-mips16' and `-shared-libgcc' options.  There have been no 
regressions to a plain `-mips16' run whereas as it stands we have 
numerous.  OK to apply?

 BTW, what's the "Check for MicroMIPS support." note seen in the 
config.host piece of the patch referring to?

2013-08-01  Maciej W. Rozycki  <macro@codesourcery.com>
            Catherine Moore  <clm@codesourcery.com>

	libgcc/
	* config/mips/mips16.S (RET_FUNCTION): Rename to...
	(_RET_FUNCTION): ... this.
	(RET_FUNCTION): New macro.
	(CALL_STUB_NO_RET): Rename to...
	(_CALL_STUB_NO_RET): ... this.
	(CALL_STUB_NO_RET): New macro.
	(CALL_STUB_RET): Rename to...
	(_CALL_STUB_RET): ... this.
	(CALL_STUB_RET): New macro.
	* config.host <mips*-*-linux>: For non-R5900 add t-slibgcc-libgcc
	to tmake_file.

  Maciej

gcc-mips16-hidden-stub.diff

Comments

Richard Sandiford Aug. 7, 2013, 8:19 p.m. UTC | #1
"Maciej W. Rozycki" <macro@codesourcery.com> writes:
>  BTW, what's the "Check for MicroMIPS support." note seen in the 
> config.host piece of the patch referring to?

No idea, please remove it.

>  /* Define a function NAME that moves a return value of mode MODE from
>     FPRs to GPRs.  */
>  
> -#define RET_FUNCTION(NAME, MODE)	\
> +#define _RET_FUNCTION(NAME, MODE)	\
>  STARTFN (NAME);				\
>  	MOVE_##MODE##_RET (t, $31);	\
>  	ENDFN (NAME)
>  
> +#ifdef SHARED
> +#define RET_FUNCTION(NAME, MODE)		\
> +	_RET_FUNCTION (NAME##_compat, MODE);	\
> +	.symver	NAME##_compat, NAME##@GCC_4.4.0
> +#else
> +#define RET_FUNCTION(NAME, MODE)		\
> +	_RET_FUNCTION (NAME, MODE);		\
> +	.hidden	NAME
> +#endif

Rather than repeat this, I think we should have:

#ifdef SHARED
#define CE_STARTFN(NAME) \
  STARTFN (NAME##_compat); \
  .symver NAME##_compat, NAME@GCC_4.4.0
#define CE_ENDFN(NAME) ENDFN (NAME##_compat)
#else
#define CE_STARTFN(NAME) STARTFN (NAME); .hidden NAME
#define CE_ENDFN(NAME) ENDFN (NAME)
#endif

below your new comment, with CE arbitrarily standing for "compat export".
Feel free to use different names if you can think of something better.
Note no "##" before "@".

Please also delete the corresponding entries in libgcc.ver, which should
no longer be needed.

OK with those changes, thanks.  No need for a full retest; checking
that libgcc_s.so.1 and libgcc.a are unchanged from your posted version
should be fine.

Richard
diff mbox

Patch

Index: gcc-fsf-trunk-quilt/libgcc/config.host
===================================================================
--- gcc-fsf-trunk-quilt.orig/libgcc/config.host	2013-07-31 17:48:35.000000000 +0100
+++ gcc-fsf-trunk-quilt/libgcc/config.host	2013-07-31 17:50:02.438694583 +0100
@@ -742,13 +742,13 @@  mips*-*-linux*)				# Linux MIPS, either 
 	tmake_file="${tmake_file} t-crtfm"
 	# Check for MicroMIPS support.
 	case ${host} in
-		mips64r5900* | mipsr5900*)
-			# The MIPS16 support code uses floating point
-			# instructions that are not supported on r5900.
-			;;
-		*)
-			tmake_file="${tmake_file} mips/t-mips16"
-			;;
+	  mips64r5900* | mipsr5900*)
+	    # The MIPS16 support code uses floating point
+	    # instructions that are not supported on r5900.
+	    ;;
+	  *)
+	    tmake_file="${tmake_file} mips/t-mips16 t-slibgcc-libgcc"
+	    ;;
 	esac
 	md_unwind_header=mips/linux-unwind.h
 	if test "${ac_cv_sizeof_long_double}" = 16; then
Index: gcc-fsf-trunk-quilt/libgcc/config/mips/mips16.S
===================================================================
--- gcc-fsf-trunk-quilt.orig/libgcc/config/mips/mips16.S	2013-07-31 17:48:35.000000000 +0100
+++ gcc-fsf-trunk-quilt/libgcc/config/mips/mips16.S	2013-07-31 17:58:30.058731920 +0100
@@ -479,14 +479,34 @@  STARTFN (__mips16_fix_truncdfsi)
 #endif
 #endif /* !__mips_single_float */
 
+/* We don't export stubs from libgcc_s.so and always require static
+   versions to be pulled from libgcc.a as needed because they use $2
+   and possibly $3 as arguments, diverging from the standard SysV ABI,
+   and as such would require severe pessimisation of MIPS16 PLT entries
+   just for this single special case.
+
+   For compatibility with old binaries that used safe standard MIPS PLT
+   entries and referred to these functions we still export them at
+   version GCC_4.4.0 for run-time loading only.  /*
+
 /* Define a function NAME that moves a return value of mode MODE from
    FPRs to GPRs.  */
 
-#define RET_FUNCTION(NAME, MODE)	\
+#define _RET_FUNCTION(NAME, MODE)	\
 STARTFN (NAME);				\
 	MOVE_##MODE##_RET (t, $31);	\
 	ENDFN (NAME)
 
+#ifdef SHARED
+#define RET_FUNCTION(NAME, MODE)		\
+	_RET_FUNCTION (NAME##_compat, MODE);	\
+	.symver	NAME##_compat, NAME##@GCC_4.4.0
+#else
+#define RET_FUNCTION(NAME, MODE)		\
+	_RET_FUNCTION (NAME, MODE);		\
+	.hidden	NAME
+#endif
+
 #ifdef L_m16retsf
 RET_FUNCTION (__mips16_ret_sf, SF)
 #endif
@@ -525,7 +545,7 @@  RET_FUNCTION (__mips16_ret_dc, DC)
    pointer.  They must copy the floating point arguments from the GPRs
    to FPRs and then call function $2.  */
 
-#define CALL_STUB_NO_RET(NAME, CODE)	\
+#define _CALL_STUB_NO_RET(NAME, CODE)	\
 STARTFN (NAME);				\
 	STUB_ARGS_##CODE;		\
 	.set	noreorder;		\
@@ -534,6 +554,16 @@  STARTFN (NAME);				\
 	.set	reorder;		\
 	ENDFN (NAME)
 
+#ifdef SHARED
+#define CALL_STUB_NO_RET(NAME, CODE)			\
+	_CALL_STUB_NO_RET (NAME##_compat, CODE);	\
+	.symver	NAME##_compat, NAME##@GCC_4.4.0
+#else
+#define CALL_STUB_NO_RET(NAME, CODE)			\
+	_CALL_STUB_NO_RET (NAME, CODE);			\
+	.hidden	NAME
+#endif
+
 #ifdef L_m16stub1
 CALL_STUB_NO_RET (__mips16_call_stub_1, 1)
 #endif
@@ -574,7 +604,7 @@  CALL_STUB_NO_RET (__mips16_call_stub_10,
    being called is 16 bits, in which case the copy is unnecessary;
    however, it's faster to always do the copy.  */
 
-#define CALL_STUB_RET(NAME, CODE, MODE)					\
+#define _CALL_STUB_RET(NAME, CODE, MODE)				\
 STARTFN (NAME);								\
 	.cfi_startproc;							\
 	/* Create a fake CFA 4 bytes below the stack pointer.  */	\
@@ -593,6 +623,16 @@  STARTFN (NAME);								\
 	.cfi_endproc;							\
 	ENDFN (NAME)
 
+#ifdef SHARED
+#define CALL_STUB_RET(NAME, CODE, MODE)			\
+	_CALL_STUB_RET (NAME##_compat, CODE, MODE);	\
+	.symver	NAME##_compat, NAME##@GCC_4.4.0
+#else
+#define CALL_STUB_RET(NAME, CODE, MODE)			\
+	_CALL_STUB_RET (NAME, CODE, MODE);		\
+	.hidden	NAME
+#endif
+
 /* First, instantiate the single-float set.  */
 
 #ifdef L_m16stubsf0