diff mbox

[MIPS] Modify dl-machine.h for mips32r6/mips64r6

Message ID 1419379661.27606.88.camel@ubuntu-sellcey
State New
Headers show

Commit Message

Steve Ellcey Dec. 24, 2014, 12:07 a.m. UTC
On Fri, 2014-12-19 at 13:54 -0800, Matthew Fortune wrote:

> I never got to reviewing this internally.  We should switch this to using
> ADDIUPC.
> 
> The goal of this code I believe is to determine the relative offset from the
> static and dynamic locations of ld.so.
> 
> 	" STRINGXP (PTR_LA) " %0, 0f\n"
> 0: addiupc <reg>, 0
> " STRINGXP (PTR_SUBU) " %0, <reg>, %0\n"
> 
> We should also do something similar with the other case.
> 
> Thanks,
> Matthew

How does this look Matthew?  There were no regressions in testing.  I
have also fixed the preprocessor indentation issues that Joseph pointed
out and I added an include of <sysdep.h> so that __mips_isa_rev is
always defined.

Steve Ellcey
sellcey@imgtec.com



2014-12-23  Steve Ellcey  <sellcey@imgtec.com>

	* sysdeps/mips/dl-machine.h (elf_machine_load_address): Replace
	bltzal with addiupc.
	(RTLD_START): Ditto.

Comments

Matthew Fortune Dec. 24, 2014, 11:05 a.m. UTC | #1
Steve Ellcey <Steve.Ellcey@imgtec.com> writes:
> On Fri, 2014-12-19 at 13:54 -0800, Matthew Fortune wrote:
> 
> > I never got to reviewing this internally.  We should switch this to
> > using ADDIUPC.
> >
> > The goal of this code I believe is to determine the relative offset
> > from the static and dynamic locations of ld.so.
> >
> > 	" STRINGXP (PTR_LA) " %0, 0f\n"
> > 0: addiupc <reg>, 0
> > " STRINGXP (PTR_SUBU) " %0, <reg>, %0\n"
> >
> > We should also do something similar with the other case.
> >
> > Thanks,
> > Matthew
> 
> How does this look Matthew?  There were no regressions in testing.  I
> have also fixed the preprocessor indentation issues that Joseph pointed
> out and I added an include of <sysdep.h> so that __mips_isa_rev is
> always defined.

This looks correct from a read through. I think it would be good to have
another technical review of the code though as well if anyone else is
willing to comment. I think the first hunk is easy to see that it is
correct, the second hunk looks correct for R6 but I am unsure how the
pre-existing code actually works. I assume it is in a reorder block
otherwise $31 will not get the runtime address of .Lcoff (now .Lcof2) it
would be 4-bytes off if in a noreorder section. The bltzal using $8 is
also weird, I don't support it matters whether it is taken or not if
the code is in a reorder section. I don't have time to check what the
objdump of this code looks like for pre-r6 but if it either shows a NOP
after the bltzal or a reordering of the instruction before it into the
DS then it all makes sense.

On a related note...
There is a general plan to go through and update any pre-R6 code that
needs to obtain a pc-relative runtime address by using
bltzal $0, <whatever> which has been given the name 'nal' in the latest
binutils source. This is being promoted because it is statically
predictable as not-taken and avoids the problems with BAL where a
BAL .+8 may interfere with hardware return predictors.

Thanks,
Matthew

> 2014-12-23  Steve Ellcey  <sellcey@imgtec.com>
> 
> 	* sysdeps/mips/dl-machine.h (elf_machine_load_address): Replace
> 	bltzal with addiupc.
> 	(RTLD_START): Ditto.
> 
> 
> diff --git a/sysdeps/mips/dl-machine.h b/sysdeps/mips/dl-machine.h index
> 5000d2a..c69fdd8 100644
> --- a/sysdeps/mips/dl-machine.h
> +++ b/sysdeps/mips/dl-machine.h
> @@ -30,6 +30,7 @@
>  #endif
> 
>  #include <sgidefs.h>
> +#include <sysdep.h>
>  #include <sys/asm.h>
>  #include <dl-tls.h>
> 
> @@ -138,9 +139,14 @@ elf_machine_load_address (void)  #ifndef __mips16
>    asm ("	.set noreorder\n"
>         "	" STRINGXP (PTR_LA) " %0, 0f\n"
> +# if __mips_isa_rev < 6
>         "	bltzal $0, 0f\n"
>         "	nop\n"
>         "0:	" STRINGXP (PTR_SUBU) " %0, $31, %0\n"
> +# else
> +       "0:	addiupc $31, 0\n"
> +       "	" STRINGXP (PTR_SUBU) " %0, $31, %0\n"
> +# endif
>         "	.set reorder\n"
>         :	"=r" (addr)
>         :	/* No inputs */
> @@ -241,6 +247,13 @@ do {									\
>        and not just plain _start.  */
> 
>  #ifndef __mips16
> +# if __mips_isa_rev < 6
> +#  define LCOFF STRINGXP(.Lcof2)
> +#  define LOAD_31 STRINGXP(bltzal $8) "," STRINGXP(.Lcof2) # else #
> +define LCOFF STRINGXP(.Lcof1) #  define LOAD_31 "addiupc $31, 0"
> +# endif
>  # define RTLD_START asm (\
>  	".text\n\
>  	" _RTLD_PROLOGUE(ENTRY_POINT) "\
> @@ -255,9 +268,9 @@ do {									\
>  	move $4, $29\n\
>  	" STRINGXP(PTR_SUBIU) " $29, 16\n\
>  	\n\
> -	" STRINGXP(PTR_LA) " $8, .Lcoff\n\
> -	bltzal $8, .Lcoff\n\
> -.Lcoff:	" STRINGXP(PTR_SUBU) " $8, $31, $8\n\
> +	" STRINGXP(PTR_LA) " $8, " LCOFF "\n\
> +.Lcof1:	" LOAD_31 "\n\
> +.Lcof2:	" STRINGXP(PTR_SUBU) " $8, $31, $8\n\
>  	\n\
>  	" STRINGXP(PTR_LA) " $25, _dl_start\n\
>  	" STRINGXP(PTR_ADDU) " $25, $8\n\
>
Joseph Myers Dec. 30, 2014, 8:56 p.m. UTC | #2
On Tue, 23 Dec 2014, Steve Ellcey wrote:

> 2014-12-23  Steve Ellcey  <sellcey@imgtec.com>
> 
> 	* sysdeps/mips/dl-machine.h (elf_machine_load_address): Replace
> 	bltzal with addiupc.
> 	(RTLD_START): Ditto.

OK.
diff mbox

Patch

diff --git a/sysdeps/mips/dl-machine.h b/sysdeps/mips/dl-machine.h
index 5000d2a..c69fdd8 100644
--- a/sysdeps/mips/dl-machine.h
+++ b/sysdeps/mips/dl-machine.h
@@ -30,6 +30,7 @@ 
 #endif
 
 #include <sgidefs.h>
+#include <sysdep.h>
 #include <sys/asm.h>
 #include <dl-tls.h>
 
@@ -138,9 +139,14 @@  elf_machine_load_address (void)
 #ifndef __mips16
   asm ("	.set noreorder\n"
        "	" STRINGXP (PTR_LA) " %0, 0f\n"
+# if __mips_isa_rev < 6
        "	bltzal $0, 0f\n"
        "	nop\n"
        "0:	" STRINGXP (PTR_SUBU) " %0, $31, %0\n"
+# else
+       "0:	addiupc $31, 0\n"
+       "	" STRINGXP (PTR_SUBU) " %0, $31, %0\n"
+# endif
        "	.set reorder\n"
        :	"=r" (addr)
        :	/* No inputs */
@@ -241,6 +247,13 @@  do {									\
       and not just plain _start.  */
 
 #ifndef __mips16
+# if __mips_isa_rev < 6
+#  define LCOFF STRINGXP(.Lcof2)
+#  define LOAD_31 STRINGXP(bltzal $8) "," STRINGXP(.Lcof2)
+# else
+#  define LCOFF STRINGXP(.Lcof1)
+#  define LOAD_31 "addiupc $31, 0"
+# endif
 # define RTLD_START asm (\
 	".text\n\
 	" _RTLD_PROLOGUE(ENTRY_POINT) "\
@@ -255,9 +268,9 @@  do {									\
 	move $4, $29\n\
 	" STRINGXP(PTR_SUBIU) " $29, 16\n\
 	\n\
-	" STRINGXP(PTR_LA) " $8, .Lcoff\n\
-	bltzal $8, .Lcoff\n\
-.Lcoff:	" STRINGXP(PTR_SUBU) " $8, $31, $8\n\
+	" STRINGXP(PTR_LA) " $8, " LCOFF "\n\
+.Lcof1:	" LOAD_31 "\n\
+.Lcof2:	" STRINGXP(PTR_SUBU) " $8, $31, $8\n\
 	\n\
 	" STRINGXP(PTR_LA) " $25, _dl_start\n\
 	" STRINGXP(PTR_ADDU) " $25, $8\n\