[parisc-linux] hppa glibc clone() patch proposal (3/3)

[parisc-linux] hppa glibc clone() patch proposal (3/3)

[Joel Soete]

Hello Mike, pa*,

And this last step is my actual fix proposal:

--- glibc-2.3.6/sysdeps/unix/sysv/linux/hppa/clone.S.orig2      2006-03-2=
4
10:11:27.000000000 +0100
+++ glibc-2.3.6/sysdeps/unix/sysv/linux/hppa/clone.S    2006-03-24
10:13:12.000000000 +0100
@@ -45,6 +45,13 @@
 
         .text
 ENTRY(__clone)
+       /* this proto SAVE_RP: stw rp, -20(sp) */
+
+       /* Sanity check arguments.  */
+       comib,=3D         0, %arg0, .Larg_error   /* no NULL function poi=
nters */
+       ldi             -EINVAL, %ret0
+       comib,=3D         0, %arg1, .Larg_error   /* no NULL stack pointe=
rs */
+       nop
 
        /* Save the fn ptr and arg on the new stack.  */
        stwm            %arg0, 64(%arg1)
@@ -60,8 +67,8 @@
 
        /* Create frame for function */
        copy            %sp, %r21
+       stw             %r21, 60(%sp)
        stwm            %r3, 64(%sp)
-       stw             %r21, -4(%sp)
 
        /* Save the PIC register. */
 #ifdef PIC
@@ -94,13 +101,35 @@
 #endif
        /* Set errno */
        copy            %ret0, %r3
-       b               __syscall_error
+       bl              __syscall_error, %rp
        sub             %r0, %ret0, %arg0
        copy            %r3, %ret0
        /* Return after setting errno */
+       /* Restore rp from ENTRY() */
+       ldw             -84(%sp), %rp
        bv              %r0(%rp)
        ldwm            -64(%sp), %r3
 
+.Larg_error:
+
+       /* Save arg0: the fn ptr.  */
+       stw             %arg0, -36(%sp)
+       /* Save the PIC register? */
+       stwm            %r3, 64(%sp)
+
+       /* Set errno */
+       copy            %ret0, %r3
+       bl              __syscall_error, %rp
+       sub             %r0, %ret0, %arg0
+
+       /* Restore arg0: the fn ptr.  */
+       ldw             -100(%sp), %arg0
+       /* Return after setting errno */
+       /* Restore rp from ENTRY() */
+       ldw             -84(%sp), %rp
+       bv              %r0(%rp)
+       ldwm            -64(%sp), %r3
+
 thread_start:
 
        /* Load up the arguments.  */
=3D=3D=3D=3D<>=3D=3D=3D=3D

Description:
    1/ first hunk resurect the 'arg sanity check'
    2/ 2d is just of my taste ;-)
    3/ branching change was suggested to me by subsequent code added by C=
.
    4/ I did find yet a elegant way to merge the 2 .Lerror_ hunk

That said that bring me more questions then answer:
    1/ what's the role of this mistery r3? (comment in last hunk)
    2/ can I get rid of save/redtore of arg0?

Thanks for your attention,
    Joel

PS1: clone04 test on a chroot disk works fine:
# ./clone04
clone04     1  PASS  :  expected failure; Got EINVAL

PS2: already build 6 time kernel in a loop with 2.6.16-pa4 64bit on a b2k=


PS3: no regression versus debian stock pkg 2.3.6-4 :-)
 grep Err Glibc-2.6.3-4.builld.debian.org.txt
make[3]: ***
[/build/buildd/glibc-2.3.6/build-tree/hppa-libc/localedata/sort-test.out]=
 Error 1
make[2]: *** [localedata/tests] Error 2
make[3]: ***
[/build/buildd/glibc-2.3.6/build-tree/hppa-libc/math/test-float.out] Erro=
r 1
make[3]: ***
[/build/buildd/glibc-2.3.6/build-tree/hppa-libc/math/test-double.out] Err=
or 1
make[3]: ***
[/build/buildd/glibc-2.3.6/build-tree/hppa-libc/math/test-idouble.out] Er=
ror 1
make[2]: *** [math/tests] Error 2
make[3]: [/build/buildd/glibc-2.3.6/build-tree/hppa-libc/posix/annexc.out=
]
Error 1 (ignored)
make[3]: ***
[/build/buildd/glibc-2.3.6/build-tree/hppa-libc/posix/tst-rxspencer.out] =
Error 139
make[2]: *** [posix/tests] Error 2
make[3]: ***
[/build/buildd/glibc-2.3.6/build-tree/hppa-libc/linuxthreads/tst-attr1.ou=
t]
Error 1
make[2]: *** [linuxthreads/tests] Error 2
make[3]: ***
[/build/buildd/glibc-2.3.6/build-tree/hppa-libc/elf/tst-tls13.out] Error =
1
make[2]: *** [elf/tests] Error 2
make[1]: *** [check] Error 2

My build:
# tail -f glibc-2.3.6-4.1 | grep Err

make[3]: ***
[/CAD/parisc-linux/Dpkg/dpkg-work/glibc-2.3.6/build-tree/hppa-libc/locale=
data/sort-test.out]
Error 1
make[2]: *** [localedata/tests] Error 2
make[3]: ***
[/CAD/parisc-linux/Dpkg/dpkg-work/glibc-2.3.6/build-tree/hppa-libc/math/t=
est-float.out]
Error 1
make[3]: ***
[/CAD/parisc-linux/Dpkg/dpkg-work/glibc-2.3.6/build-tree/hppa-libc/math/t=
est-double.out]
Error 1
make[3]: ***
[/CAD/parisc-linux/Dpkg/dpkg-work/glibc-2.3.6/build-tree/hppa-libc/math/t=
est-idouble.out]
Error 1
make[2]: *** [math/tests] Error 2
make[3]: ***
[/CAD/parisc-linux/Dpkg/dpkg-work/glibc-2.3.6/build-tree/hppa-libc/posix/=
tst-rxspencer.out]
Error 139
make[3]:
[/CAD/parisc-linux/Dpkg/dpkg-work/glibc-2.3.6/build-tree/hppa-libc/posix/=
annexc.out]
Error 1 (ignored)
make[2]: *** [posix/tests] Error 2
make[3]: ***
[/CAD/parisc-linux/Dpkg/dpkg-work/glibc-2.3.6/build-tree/hppa-libc/linuxt=
hreads/tst-attr1.out]
Error 1
make[2]: *** [linuxthreads/tests] Error 2
make[3]: ***
[/CAD/parisc-linux/Dpkg/dpkg-work/glibc-2.3.6/build-tree/hppa-libc/elf/ts=
t-tls13.out]
Error 1
make[2]: *** [elf/tests] Error 2
make[1]: *** [check] Error 2
=0A=0A-------------------------------------------------------=0ANOTE! My =
email address is changing to ... @scarlet.be=0APlease make the necessary =
changes in your address book. =0A=0A

_______________________________________________
parisc-linux mailing list
parisc-linux@lists.parisc-linux.org
http://lists.parisc-linux.org/mailman/listinfo/parisc-linux

Re: [parisc-linux] hppa glibc clone() patch proposal (3/3)

[Grant Grundler]

On Fri, Mar 24, 2006 at 11:18:25AM +0100, Joel Soete wrote:
> Hello Mike, pa*,
> 
> And this last step is my actual fix proposal:
...
>  ENTRY(__clone)
> +       /* Sanity check arguments.  */
> +       comib,=         0, %arg0, .Larg_error   /* no NULL function pointers */
> +       ldi             -EINVAL, %ret0
> +       comib,=         0, %arg1, .Larg_error   /* no NULL stack pointers */
> +       nop

If this is correct, then I would prefer:

	/* catch NULL stack or function ptrs */
	xor		%arg1, %arg0, %ret0
	comib,=,n	0, %ret0, .Larg_error
	ldi             -EINVAL, %ret0


> 
>         /* Save the fn ptr and arg on the new stack.  */
>         stwm            %arg0, 64(%arg1)
> @@ -60,8 +67,8 @@
> 
>         /* Create frame for function */
>         copy            %sp, %r21
> +       stw             %r21, 60(%sp)

Is %r21 used later again?
If not, then these two ops can become:
	stw            %sp, 60(%sp)

If yes, then add the copy() *after* the stw.
PA-RISC can only do one store at a time.
PA20 can do the copy in parallel with the store.

hth,
grant
_______________________________________________
parisc-linux mailing list
parisc-linux@lists.parisc-linux.org
http://lists.parisc-linux.org/mailman/listinfo/parisc-linux

[parisc-linux] Fwd: hppa glibc clone() patch proposal (3/3)

[Carlos O'Donell]

>         /* Create frame for function */
>         copy            %sp, %r21
> +       stw             %r21, 60(%sp)
>         stwm            %r3, 64(%sp)
> -       stw             %r21, -4(%sp)

Bug.

c.
_______________________________________________
parisc-linux mailing list
parisc-linux@lists.parisc-linux.org
http://lists.parisc-linux.org/mailman/listinfo/parisc-linux

Re: [parisc-linux] Fwd: hppa glibc clone() patch proposal (3/3)

[Joel Soete]

Carlos O'Donell wrote:
>>PS: when I will revert this hunk, do you think reasonable that I send the patch to Aurelien to insert it in debian pkg?
> 
> 
> No, post again for review. The whole patch please.
> 
No pb here it is:
--- glibc-2.3.6/sysdeps/unix/sysv/linux/hppa/clone.S.orig2      2006-03-24 09:11:27.000000000 +0000
+++ glibc-2.3.6/sysdeps/unix/sysv/linux/hppa/clone.S    2006-03-26 10:20:13.000000000 +0000
@@ -45,6 +45,14 @@

          .text
  ENTRY(__clone)
+    /* this proto do: stw rp, -20(sp) */
+       /* FIXME: I have no idea how profiling works on hppa. */
+
+       /* Sanity check arguments.  */
+       comib,=         0, %arg0, .Larg_error   /* no NULL function pointers */
+       ldi             -EINVAL, %ret0
+       comib,=         0, %arg1, .Larg_error   /* no NULL stack pointers */
+       nop

         /* Save the fn ptr and arg on the new stack.  */
         stwm            %arg0, 64(%arg1)
@@ -94,13 +102,35 @@
  #endif
         /* Set errno */
         copy            %ret0, %r3
-       b               __syscall_error
+       bl              __syscall_error, %rp
         sub             %r0, %ret0, %arg0
         copy            %r3, %ret0
         /* Return after setting errno */
+       /* Restore rp from ENTRY() */
+       ldw             -84(%sp), %rp
         bv              %r0(%rp)
         ldwm            -64(%sp), %r3

+.Larg_error:
+
+       /* Save arg0: the fn ptr.  */
+       stw             %arg0, -36(%sp)
+       /* Save the PIC register? */
+       stwm            %r3, 64(%sp)
+
+       /* Set errno */
+       copy            %ret0, %r3
+       bl              __syscall_error, %rp
+       sub             %r0, %ret0, %arg0
+
+       /* Restore arg0: the fn ptr.  */
+       ldw             -100(%sp), %arg0
+       /* Return after setting errno */
+       /* Restore rp from ENTRY() */
+       ldw             -84(%sp), %rp
+       bv              %r0(%rp)
+       ldwm            -64(%sp), %r3
+
  thread_start:

         /* Load up the arguments.  */
===<>====

clone04 still works fine ;-)

mmm still have many questions (for so few insn):
     1/ what's the role of this mistery r3? (see comment)

     2/ is it the right palce to save 'stw %arg0, -36(%sp)' (I grab it from some other of your src)?

     3/ though, may be finaly better 'stw %arg0, -100(%sp)' after 'stwm %r3, 64(%sp)'?

     3/ can I get rid of save/restore of arg0?


Thanks again for your attention,
     Joel


PS: I still got the same 'Error' as buildd:
   # tail -f glibc-2.3.6-4.2 | grep Err
make[3]: *** [/CAD/parisc-linux/Dpkg/dpkg-work/glibc-2.3.6/build-tree/hppa-libc/localedata/sort-test.out] Error 1
make[2]: *** [localedata/tests] Error 2
make[3]: *** [/CAD/parisc-linux/Dpkg/dpkg-work/glibc-2.3.6/build-tree/hppa-libc/math/test-float.out] Error 1
make[3]: *** [/CAD/parisc-linux/Dpkg/dpkg-work/glibc-2.3.6/build-tree/hppa-libc/math/test-double.out] Error 1
make[3]: *** [/CAD/parisc-linux/Dpkg/dpkg-work/glibc-2.3.6/build-tree/hppa-libc/math/test-idouble.out] Error 1
make[2]: *** [math/tests] Error 2
make[3]: *** [/CAD/parisc-linux/Dpkg/dpkg-work/glibc-2.3.6/build-tree/hppa-libc/posix/tst-rxspencer.out] Error 139
make[3]: [/CAD/parisc-linux/Dpkg/dpkg-work/glibc-2.3.6/build-tree/hppa-libc/posix/annexc.out] Error 1 (ignored)
make[2]: *** [posix/tests] Error 2
make[3]: *** [/CAD/parisc-linux/Dpkg/dpkg-work/glibc-2.3.6/build-tree/hppa-libc/linuxthreads/tst-attr1.out] Error 1
make[2]: *** [linuxthreads/tests] Error 2
make[3]: *** [/CAD/parisc-linux/Dpkg/dpkg-work/glibc-2.3.6/build-tree/hppa-libc/rt/tst-timer.out] Error 137
make[3]: *** [/CAD/parisc-linux/Dpkg/dpkg-work/glibc-2.3.6/build-tree/hppa-libc/rt/tst-aio4.out] Error 1
make[3]: *** [/CAD/parisc-linux/Dpkg/dpkg-work/glibc-2.3.6/build-tree/hppa-libc/rt/tst-timer4.out] Error 1
make[2]: *** [rt/tests] Error 2
make[3]: *** [/CAD/parisc-linux/Dpkg/dpkg-work/glibc-2.3.6/build-tree/hppa-libc/elf/tst-tls13.out] Error 1
make[2]: *** [elf/tests] Error 2
make[1]: *** [check] Error 2

So it doesn't seems to introduce regressions?

_______________________________________________
parisc-linux mailing list
parisc-linux@lists.parisc-linux.org
http://lists.parisc-linux.org/mailman/listinfo/parisc-linux

Re: [parisc-linux] Fwd: hppa glibc clone() patch proposal (3/3)

[Randolph Chung]

Joel,

Some comments:

1) I think you are right in fixing the first __syscall_error call. I
don't know why we are doing a b instead of bl now. It seems to be that
the current code will result in an orphaned stack frame if we ever
errored out. However, I see that in the other syscalls, the call to
__syscall_error is inlined. Maybe that would be better.....

2)
>          .text
>  ENTRY(__clone)
> +    /* this proto do: stw rp, -20(sp) */

Yes, you should put "stw %rp, -20(%sp)" here.

> +       /* FIXME: I have no idea how profiling works on hppa. */

Let's leave profiling out of this for now....

> +       /* Sanity check arguments.  */
> +       comib,=         0, %arg0, .Larg_error   /* no NULL function
> pointers */
> +       ldi             -EINVAL, %ret0
> +       comib,=         0, %arg1, .Larg_error   /* no NULL stack
> pointers */
> +       nop

There seems to be some confusion here....

You want clone() to return -1, and set errno = -EINVAL. The logic to do
this is in .Larg_error. Probably better here to just detect the two
error conditions and branch to .Larg_error and handle all the logic there.

>         /* Save the fn ptr and arg on the new stack.  */
>         stwm            %arg0, 64(%arg1)
> @@ -94,13 +102,35 @@
>  #endif
>         /* Set errno */
>         copy            %ret0, %r3
> -       b               __syscall_error
> +       bl              __syscall_error, %rp
>         sub             %r0, %ret0, %arg0
>         copy            %r3, %ret0
>         /* Return after setting errno */
> +       /* Restore rp from ENTRY() */
> +       ldw             -84(%sp), %rp

Right now since you don't store rp, this will give you junk.

> +.Larg_error:
> +
> +       /* Save arg0: the fn ptr.  */
> +       stw             %arg0, -36(%sp)

In the error case, you don't invoke the function, so no need to save and
restore arg0.

> +       /* Save the PIC register? */
> +       stwm            %r3, 64(%sp)

r3 is just a caller-save register; it has nothing to do with PIC. You
don't need to use r3 in this path; just create a stack frame with:
	ldo 64(%sp), %sp

> +       /* Set errno */
> +       copy            %ret0, %r3
> +       bl              __syscall_error, %rp
> +       sub             %r0, %ret0, %arg0

Here, I think you want to do (or inline the __syscall_error logic):
	bl __syscall_error, %rp
	ldi EINVAL, %arg0
	ldi -1, %ret0

> +       /* Restore arg0: the fn ptr.  */
> +       ldw             -100(%sp), %arg0
> +       /* Return after setting errno */
> +       /* Restore rp from ENTRY() */
> +       ldw             -84(%sp), %rp
> +       bv              %r0(%rp)
> +       ldwm            -64(%sp), %r3

and here you can just do:
	ldw -84(%sp), %rp
	bv %r0(%rp)
	ldo -64(%sp), %sp

randolph

_______________________________________________
parisc-linux mailing list
parisc-linux@lists.parisc-linux.org
http://lists.parisc-linux.org/mailman/listinfo/parisc-linux