patch to RTLD_START to avoid store data below $sp on MIPS

patch to RTLD_START to avoid store data below $sp on MIPS

[Petar Jovanovic]

Hi everyone,
 
there is a store-data-below-stack-pointer case in RTLD_START in
sysdeps/mips/dl-machine.h. Is there any reason to be so?
 
If not, I'd like to propose a small fix for it. Valgrind has detected the
issue and reports an error in this region. With the change, it will not
report it.
 
Let me know what you think.
 
Thanks.
 
Regards,
Petar
 
diff --git a/sysdeps/mips/dl-machine.h b/sysdeps/mips/dl-machine.h
index bc03785..5aabc88 100644
--- a/sysdeps/mips/dl-machine.h
+++ b/sysdeps/mips/dl-machine.h
@@ -266,8 +266,9 @@ do {
\
        " STRINGXP(PTR_ADDU) " $7, $7, " STRINGXP (PTRSIZE) " \n\
        # Make sure the stack pointer is aligned for _dl_init_internal.\n\
        and $2, $29, -2 * " STRINGXP(SZREG) "\n\
-       " STRINGXP(PTR_S) " $29, -" STRINGXP(SZREG) "($2)\n\
+       move $8, $29\n\
        " STRINGXP(PTR_SUBIU) " $29, $2, 32\n\
+       " STRINGXP(PTR_S) " $8, 32-" STRINGXP(SZREG) "($29)\n\
        " STRINGXP(SAVE_GP(16)) "\n\
        # Call the function to run the initializers.\n\
        jal _dl_init_internal\n\

Re: patch to RTLD_START to avoid store data below $sp on MIPS

[Joseph S. Myers]

On Wed, 4 Jul 2012, Petar Jovanovic wrote:

> Hi everyone,
>  
> there is a store-data-below-stack-pointer case in RTLD_START in
> sysdeps/mips/dl-machine.h. Is there any reason to be so?

No, I think your change is probably correct, but how have you tested it?  
In particular, have you done some tests for all three ABIs (o32, n32, 
n64)?

-- 
Joseph S. Myers
joseph@codesourcery.com

Re: patch to RTLD_START to avoid store data below $sp on MIPS

[Maciej W. Rozycki]

On Wed, 4 Jul 2012, Petar Jovanovic wrote:

> diff --git a/sysdeps/mips/dl-machine.h b/sysdeps/mips/dl-machine.h
> index bc03785..5aabc88 100644
> --- a/sysdeps/mips/dl-machine.h
> +++ b/sysdeps/mips/dl-machine.h
> @@ -266,8 +266,9 @@ do {
> \
>         " STRINGXP(PTR_ADDU) " $7, $7, " STRINGXP (PTRSIZE) " \n\
>         # Make sure the stack pointer is aligned for _dl_init_internal.\n\
>         and $2, $29, -2 * " STRINGXP(SZREG) "\n\
> -       " STRINGXP(PTR_S) " $29, -" STRINGXP(SZREG) "($2)\n\
> +       move $8, $29\n\
>         " STRINGXP(PTR_SUBIU) " $29, $2, 32\n\
> +       " STRINGXP(PTR_S) " $8, 32-" STRINGXP(SZREG) "($29)\n\
                                    ^
 Spaces missing here, this is not unary "-" unlike in the old instruction, 
and the whole displacement expression should probably be bracketed to 
avoid reader's confusion.

>         " STRINGXP(SAVE_GP(16)) "\n\
>         # Call the function to run the initializers.\n\
>         jal _dl_init_internal\n\

  Maciej

RE: patch to RTLD_START to avoid store data below $sp on MIPS

[Petar Jovanovic]

@Joseph
I did try it out on o32 platform, this is how I know Valgrind will not
report error with that change. I have not run any test suite.

@Maciej
You are right, additional spaces and brackets will look nicer.
 
Regards,
Petar

-----Original Message-----
From: Maciej W. Rozycki [mailto:macro@codesourcery.com] 
Sent: Wednesday, July 04, 2012 6:30 PM
To: Petar Jovanovic
Cc: libc-ports@sourceware.org
Subject: Re: patch to RTLD_START to avoid store data below $sp on MIPS

On Wed, 4 Jul 2012, Petar Jovanovic wrote:

> diff --git a/sysdeps/mips/dl-machine.h b/sysdeps/mips/dl-machine.h
> index bc03785..5aabc88 100644
> --- a/sysdeps/mips/dl-machine.h
> +++ b/sysdeps/mips/dl-machine.h
> @@ -266,8 +266,9 @@ do {
> \
>         " STRINGXP(PTR_ADDU) " $7, $7, " STRINGXP (PTRSIZE) " \n\
>         # Make sure the stack pointer is aligned for _dl_init_internal.\n\
>         and $2, $29, -2 * " STRINGXP(SZREG) "\n\
> -       " STRINGXP(PTR_S) " $29, -" STRINGXP(SZREG) "($2)\n\
> +       move $8, $29\n\
>         " STRINGXP(PTR_SUBIU) " $29, $2, 32\n\
> +       " STRINGXP(PTR_S) " $8, 32-" STRINGXP(SZREG) "($29)\n\
                                    ^
 Spaces missing here, this is not unary "-" unlike in the old instruction, 
and the whole displacement expression should probably be bracketed to 
avoid reader's confusion.

>         " STRINGXP(SAVE_GP(16)) "\n\
>         # Call the function to run the initializers.\n\
>         jal _dl_init_internal\n\

  Maciej

RE: patch to RTLD_START to avoid store data below $sp on MIPS

[Joseph S. Myers]

On Thu, 5 Jul 2012, Petar Jovanovic wrote:

> @Joseph
> I did try it out on o32 platform, this is how I know Valgrind will not
> report error with that change. I have not run any test suite.
> 
> @Maciej
> You are right, additional spaces and brackets will look nicer.

Could you send a patch updated to take account of Maciej's comments?  I 
can then do some testing for all three ABIs before putting it in if it 
tests out OK.

-- 
Joseph S. Myers
joseph@codesourcery.com

RE: patch to RTLD_START to avoid store data below $sp on MIPS

[Petar Jovanovic]

[-- Attachment #1: Type: text/plain, Size: 774 bytes --]

Sure, it is attached.

Regards,
Petar

-----Original Message-----
From: Joseph Myers [mailto:joseph@codesourcery.com] 
Sent: Thursday, July 05, 2012 8:54 PM
To: Petar Jovanovic
Cc: 'Maciej W. Rozycki'; libc-ports@sourceware.org
Subject: RE: patch to RTLD_START to avoid store data below $sp on MIPS

On Thu, 5 Jul 2012, Petar Jovanovic wrote:

> @Joseph
> I did try it out on o32 platform, this is how I know Valgrind will not
> report error with that change. I have not run any test suite.
> 
> @Maciej
> You are right, additional spaces and brackets will look nicer.

Could you send a patch updated to take account of Maciej's comments?  I 
can then do some testing for all three ABIs before putting it in if it 
tests out OK.

-- 
Joseph S. Myers
joseph@codesourcery.com

[-- Attachment #2: dl-machine-patch.diff --]
[-- Type: application/octet-stream, Size: 1143 bytes --]

diff --git a/sysdeps/mips/dl-machine.h b/sysdeps/mips/dl-machine.h
index bc03785..cc7da76 100644
--- a/sysdeps/mips/dl-machine.h
+++ b/sysdeps/mips/dl-machine.h
@@ -266,13 +266,14 @@ do {                                                                      \
        " STRINGXP(PTR_ADDU) " $7, $7, " STRINGXP (PTRSIZE) " \n\
        # Make sure the stack pointer is aligned for _dl_init_internal.\n\
        and $2, $29, -2 * " STRINGXP(SZREG) "\n\
-       " STRINGXP(PTR_S) " $29, -" STRINGXP(SZREG) "($2)\n\
+       move $8, $29\n\
        " STRINGXP(PTR_SUBIU) " $29, $2, 32\n\
+       " STRINGXP(PTR_S) " $8, (32 - " STRINGXP(SZREG) ")($29)\n\
        " STRINGXP(SAVE_GP(16)) "\n\
        # Call the function to run the initializers.\n\
        jal _dl_init_internal\n\
        # Restore the stack pointer for _start.\n\
-       " STRINGXP(PTR_L)  " $29, 32-" STRINGXP(SZREG) "($29)\n\
+       " STRINGXP(PTR_L)  " $29, (32 - " STRINGXP(SZREG) ")($29)\n\
        # Pass our finalizer function to the user in $2 as per ELF ABI.\n\
        " STRINGXP(PTR_LA) " $2, _dl_fini\n\
        # Jump to the user entry point.\n\

RE: patch to RTLD_START to avoid store data below $sp on MIPS

[Joseph S. Myers]

On Fri, 6 Jul 2012, Petar Jovanovic wrote:

> Sure, it is attached.

Thanks, I've committed it with the ChangeLog entry:

2012-07-06  Petar Jovanovic  <petar.jovanovic@rt-rk.com>

        * sysdeps/mips/dl-machine.h (RTLD_START): Do not store data below
        the stack pointer.

-- 
Joseph S. Myers
joseph@codesourcery.com