Questions about hppa *context functions.

Questions about hppa *context functions.

[Carlos O'Donell]

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

Helge,

On December 15th, 2009 an additional *context test was added to glibc,
see stdlib/tst-makecontext3.c.

This test fails on hppa with:
~~~
makecontext: does not know how to handle more than 8 arguments
~~~

I have a couple of questions and comments about hppa's *context implementation:

1. Why limit the number of input arguments to 8?

The ABI should allow an unlimited number of arguments to be placed on
the stack. It is up to the caller to make sure that
ucp->uc_stack.ss_sp points to enough space to hold all of the
additional arguments.

2. It doesn't appear that the current implementation transfers
ucp->uc_stack.ss_sp to the stack pointer when makecontext() is called.

Take a look at:
http://www.opengroup.org/onlinepubs/009695399/functions/makecontext.html

The caller can set an alternate stack by setting ucp->uc_stack.ss_sp,
and makecontext should likely do:
ucp->uc_mcontext.sc_gr[30] = ucp->uc_stack.ss_sp + <space required by args>;

3. POSIX says that all the arguments must be of type int, but in a
comment you write "XXX: This implementation only handles integer
arguments."

Is there any reason this comment should stay there?

To give you an example, I'm attaching a modified makecontext.diff for
you to comment on.

Cheers,
Carlos.

[-- Attachment #2: makecontext.diff --]
[-- Type: application/octet-stream, Size: 1604 bytes --]

diff --git a/sysdeps/unix/sysv/linux/hppa/makecontext.c b/sysdeps/unix/sysv/linux/hppa/makecontext.c
index 69a1813..356a094 100644
--- a/sysdeps/unix/sysv/linux/hppa/makecontext.c
+++ b/sysdeps/unix/sysv/linux/hppa/makecontext.c
@@ -25,7 +25,7 @@
 #include <sysdep.h>
 #include <ucontext.h>
 
-/* XXX: This implementation only handles integer arguments.  */
+/* POSIX only supports integer arguments.  */
 
 void
 __makecontext (ucontext_t *ucp, void (*func) (void), int argc, ...)
@@ -50,29 +50,26 @@ makecontext: does not know how to handle more than 8 arguments\n"));
   va_start (ap, argc);
   /* Handle arguments.  */
   for (i = 0; i < argc; ++i)
-    switch (i)
-      {
-      case 0:
-      case 1:
-      case 2:
-      case 3:
-      	ucp->uc_mcontext.sc_gr[26-i] = va_arg (ap, int);
-	break;
-      case 4:
-      case 5:
-      case 6:
-      case 7:
-	if (sizeof(unsigned long) == 4) {
-		/* 32bit: put arg7-arg4 on stack.  */
-		sp[7-i] = va_arg (ap, int);
-	} else {
-		/* 64bit: r19-r22 are arg7-arg4.  */
-		ucp->uc_mcontext.sc_gr[22+4-i] = va_arg (ap, int);
+    {
+      if (i < 4)
+	{
+	  ucp->uc_mcontext.sc_gr[26-i] = va_arg (ap, int);
+	  continue;
 	}
-	break;
-      }
-  va_end (ap);
 
+      if ((i < 8) && (sizeof(unsigned int) == 8))
+	{
+	  /* 64bit: r19-r22 are arg7-arg4.  */
+	  ucp->uc_mcontext.sc_gr[22+4-i] = va_arg (ap, int);
+	  continue;
+	} 
+
+      /* All other arguments go on the stack.  */
+      sp[i] = va_arg (ap, int);
+    }
+  va_end (ap); 
+  /* Adjust the stack pointer to last used argument.  */
+  ucp->uc_mcontext.sc_gr[30] = sp[argc - 1];
 }
 
 

Re: Questions about hppa *context functions.

[Carlos O'Donell]

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

On Sun, Jan 31, 2010 at 11:25 AM, Carlos O'Donell
<carlos@systemhalted.org> wrote:
> Helge,
>
> On December 15th, 2009 an additional *context test was added to glibc,
> see stdlib/tst-makecontext3.c.
>
> This test fails on hppa with:
> ~~~
> makecontext: does not know how to handle more than 8 arguments
> ~~~

New patch attached with additional changes:
* Align the incoming stack to 64-bytes.
* Adjust for frame.
* Adjust saved stack pointer.

The following patch allows the new test to pass for the 32-bit runtime.

I can't guarantee this works for a 64-bit runtime.

Helge, can you comment on the attached patch?

Cheers,
Carlos.

[-- Attachment #2: makecontext.diff --]
[-- Type: application/octet-stream, Size: 2298 bytes --]

diff --git a/sysdeps/unix/sysv/linux/hppa/makecontext.c b/sysdeps/unix/sysv/linux/hppa/makecontext.c
index 69a1813..a84abe7 100644
--- a/sysdeps/unix/sysv/linux/hppa/makecontext.c
+++ b/sysdeps/unix/sysv/linux/hppa/makecontext.c
@@ -25,7 +25,9 @@
 #include <sysdep.h>
 #include <ucontext.h>
 
-/* XXX: This implementation only handles integer arguments.  */
+/* POSIX only supports integer arguments.  */
+#define STACK_ALIGN 64
+#define FRAME_SIZE 8
 
 void
 __makecontext (ucontext_t *ucp, void (*func) (void), int argc, ...)
@@ -34,15 +36,10 @@ __makecontext (ucontext_t *ucp, void (*func) (void), int argc, ...)
   va_list ap;
   int i;
 
-  if (argc > 8)
-    {
-      fprintf (stderr, _("\
-makecontext: does not know how to handle more than 8 arguments\n"));
-      exit (-1);
-    }
-
-  /* Get stack pointer.  */
-  sp = (unsigned int *) ucp->uc_stack.ss_sp;
+  /* Get stack pointer (64-byte aligned).  */
+  sp = (unsigned int *)((((unsigned int) ucp->uc_stack.ss_sp) 
+			 + FRAME_SIZE + argc + STACK_ALIGN) 
+		        & ~(STACK_ALIGN - 1));
 
   /* Store address to jump to.  */
   ucp->uc_mcontext.sc_gr[2] = (unsigned long) func;
@@ -50,29 +47,27 @@ makecontext: does not know how to handle more than 8 arguments\n"));
   va_start (ap, argc);
   /* Handle arguments.  */
   for (i = 0; i < argc; ++i)
-    switch (i)
-      {
-      case 0:
-      case 1:
-      case 2:
-      case 3:
-      	ucp->uc_mcontext.sc_gr[26-i] = va_arg (ap, int);
-	break;
-      case 4:
-      case 5:
-      case 6:
-      case 7:
-	if (sizeof(unsigned long) == 4) {
-		/* 32bit: put arg7-arg4 on stack.  */
-		sp[7-i] = va_arg (ap, int);
-	} else {
-		/* 64bit: r19-r22 are arg7-arg4.  */
-		ucp->uc_mcontext.sc_gr[22+4-i] = va_arg (ap, int);
+    {
+      if (i < 4)
+	{
+	  ucp->uc_mcontext.sc_gr[26-i] = va_arg (ap, int);
+	  continue;
 	}
-	break;
-      }
-  va_end (ap);
 
+      if ((i < 8) && (sizeof(unsigned int) == 8))
+	{
+	  /* 64bit: r19-r22 are arg7-arg4.  */
+	  ucp->uc_mcontext.sc_gr[22+4-i] = va_arg (ap, int);
+	  continue;
+	} 
+
+      /* All other arguments go on the stack.  */
+      sp[-1 * (FRAME_SIZE + 1 + i)] = va_arg (ap, int);
+    }
+  va_end (ap); 
+
+  /* Adjust the stack pointer to last used argument.  */
+  ucp->uc_mcontext.sc_gr[30] = (unsigned int) sp;
 }
 
 

Re: Questions about hppa *context functions.

[Helge Deller]

On 01/31/2010 09:15 PM, Carlos O'Donell wrote:
> On Sun, Jan 31, 2010 at 11:25 AM, Carlos O'Donell
> <carlos@systemhalted.org>  wrote:
>> Helge,
>>
>> On December 15th, 2009 an additional *context test was added to glibc,
>> see stdlib/tst-makecontext3.c.
>>
>> This test fails on hppa with:
>> ~~~
>> makecontext: does not know how to handle more than 8 arguments
>> ~~~
>
> New patch attached with additional changes:
> * Align the incoming stack to 64-bytes.
> * Adjust for frame.
> * Adjust saved stack pointer.
>
> The following patch allows the new test to pass for the 32-bit runtime.
>
> I can't guarantee this works for a 64-bit runtime.
>
> Helge, can you comment on the attached patch?

Carlos,

thanks a lot for your work!

First of all, if it works, I'm fine with it.

One minor question:
I'm used to test sizeof long instead of sizeof int to check for 32/64bit.
You changed it to "(sizeof(unsigned int) == 8)".
I always thought a long is usually the type of a register, while an int can be anything.

> 1. Why limit the number of input arguments to 8?

I didn't had real reasons.
At some point I read that software usually should not rely on the fact that the
*context funtions  can handle more than 4 arguments. To be on the safe side, software
should better hand over a pointer to a struct in one of the first 4 arguments.
But with your patch, which extends it for parisc to more than 8 arguments, it's even better :-)

> 2. It doesn't appear that the current implementation transfers
> ucp->uc_stack.ss_sp to the stack pointer when makecontext() is called.

You seem to have fixed this in your patch.

> 3. POSIX says that all the arguments must be of type int, but in a
> comment you write "XXX: This implementation only handles integer
> arguments."
> Is there any reason this comment should stay there?

Yep. Just drop it.


Again, I didn't tested you patch, but just by looking at it, it seems OK.

Thanks,
Helge

Re: Questions about hppa *context functions.

[Carlos O'Donell]

On Sun, Jan 31, 2010 at 3:47 PM, Helge Deller <deller@gmx.de> wrote:
> Carlos,
>
> thanks a lot for your work!
>
> First of all, if it works, I'm fine with it.
>
> One minor question:
> I'm used to test sizeof long instead of sizeof int to check for 32/64=
bit.
> You changed it to "(sizeof(unsigned int) =3D=3D 8)".
> I always thought a long is usually the type of a register, while an i=
nt can
> be anything.

You are correct, this code is wrong. Since hppa64 is LP64, and hppa32
is ILP32, the only way to tell the difference between the two is by
using "sizeof(unsigned long)" or "sizeof(void *)", the two types which
differ between the ABIs.

>> 1. Why limit the number of input arguments to 8?
>
> I didn't had real reasons.
> At some point I read that software usually should not rely on the fac=
t that
> the
> *context funtions =A0can handle more than 4 arguments. To be on the s=
afe side,
> software
> should better hand over a pointer to a struct in one of the first 4
> arguments.
> But with your patch, which extends it for parisc to more than 8 argum=
ents,
> it's even better :-)

OK, perfect, the new implementation should handle as many arguments as
you have stack space.

>> 2. It doesn't appear that the current implementation transfers
>> ucp->uc_stack.ss_sp to the stack pointer when makecontext() is calle=
d.
>
> You seem to have fixed this in your patch.

Yes, I did fix this, I was just curious if you considered this in the
initial implementation?

>> 3. POSIX says that all the arguments must be of type int, but in a
>> comment you write "XXX: This implementation only handles integer
>> arguments."
>> Is there any reason this comment should stay there?
>
> Yep. Just drop it.

Thanks.

> Again, I didn't tested you patch, but just by looking at it, it seems=
 OK.

Thanks.

Cheers,
Carlos.
--
To unsubscribe from this list: send the line "unsubscribe linux-parisc"=
 in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html