[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