Re: PATCH: Use "unsigned long long int" in x86-64 __makecontext

["H.J. Lu" <hjl dot tools at gmail dot com>]

On Fri, Mar 16, 2012 at 2:20 PM, Roland McGrath <roland@hack.frob.com> wrote:
>> On Fri, Mar 16, 2012 at 1:13 PM, Roland McGrath <roland@hack.frob.com> wrote:
>> > Why not use greg_t in va_arg too?
>>
>> I thought about it. ?I am concerned
>>
>> ? ? ? default:
>> ? ? ? ? /* Put value on stack. ?*/
>> ? ? ? ? sp[i - 5] = va_arg (ap, unsigned long int);
>> ? ? ? ? break;
>>
>> Why is unsigned long int used here when all other places use long int?
>> I think they should be the same. ?But I may have missed something.
>
> It certainly doesn't matter to the behavior of va_arg.
> I can't see how it would matter to the assignment either,
> since there is no widening taking place.
>
> I guess to be paranoid you could compare the generated code before and
> after. ?The only change that should potentially make a difference is the
> type of IDX_UC_LINK. ?But off hand it seems to me that should be size_t
> anyway, and then the generated code should be completely identical unless
> I've overlooked something.
>
> I also notice the XXX comment in that file, which doesn't apply to the code
> there now (but does still apply to the test case, which probably ought to
> test int, long int, and pointer arguments explicitly just to be thorough).
>
> Hmm. ?The specification for makecontext actually says all the arguments are
> just int, though our x86_64 code supports long int and pointer as well. ?If
> the specification were modernized it might say something like "integer
> types no larger than intptr_t" or something like that. ?But still it seems
> unlikely it would specify that you have to handle anything larger than a
> pointer. ?You're making x32 handle int64_t even though that's larger than a
> pointer. ?That makes some intrinsic sense for x32 because of its calling
> convention, but the generic specification would never require that since
> it's not going handle e.g. int64_t on i386. ?Since the x86_64 and x32 ABIs
> specify 64-bit locations for all smaller integer arguments, it may make no
> difference. ?But it's not entirely clear to me from the ABI document what
> the rules are about sign-extension. ?If the callee is always required to
> ignore the high bits of an argument of type int (rather than expecting the
> caller to have sign-extended it), then it doesn't matter if the argument is
> treat as int or int64_t for va_arg in makecontext.
>

I compared the assembly outputs for both x32 and x86-64.  There
are no differences between "long int" and "unsigned long int" vs.
"greg_t" on va_arg.  Here is the patch to use  greg_t exclusively.
OK to install?

Thanks.


-- 
H.J.

2012-03-16  H.J. Lu  <hongjiu.lu@intel.com>

	* sysdeps/unix/sysv/linux/x86_64/makecontext.c (__makecontext): Use
	greg_t on sp.  Use unsigned int on idx_uc_link.  Cast adresses to
	uintptr_t first.  Replace "long int" and "unsigned long int" with
	"greg_t" on va_arg.

diff --git a/sysdeps/unix/sysv/linux/x86_64/makecontext.c b/sysdeps/unix/sysv/linux/x86_64/makecontext.c
index 860925f..c2eda26 100644
--- a/sysdeps/unix/sysv/linux/x86_64/makecontext.c
+++ b/sysdeps/unix/sysv/linux/x86_64/makecontext.c
@@ -52,29 +52,30 @@ void
 __makecontext (ucontext_t *ucp, void (*func) (void), int argc, ...)
 {
   extern void __start_context (void);
-  unsigned long int *sp, idx_uc_link;
+  greg_t *sp;
+  unsigned int idx_uc_link;
   va_list ap;
   int i;
 
   /* Generate room on stack for parameter if needed and uc_link.  */
-  sp = (unsigned long int *) ((uintptr_t) ucp->uc_stack.ss_sp
-			      + ucp->uc_stack.ss_size);
+  sp = (greg_t *) ((uintptr_t) ucp->uc_stack.ss_sp
+		   + ucp->uc_stack.ss_size);
   sp -= (argc > 6 ? argc - 6 : 0) + 1;
   /* Align stack and make space for trampoline address.  */
-  sp = (unsigned long int *) ((((uintptr_t) sp) & -16L) - 8);
+  sp = (greg_t *) ((((uintptr_t) sp) & -16L) - 8);
 
   idx_uc_link = (argc > 6 ? argc - 6 : 0) + 1;
 
   /* Setup context ucp.  */
   /* Address to jump to.  */
-  ucp->uc_mcontext.gregs[REG_RIP] = (long int) func;
+  ucp->uc_mcontext.gregs[REG_RIP] = (greg_t) (uintptr_t) func;
   /* Setup rbx.*/
-  ucp->uc_mcontext.gregs[REG_RBX] = (long int) &sp[idx_uc_link];
-  ucp->uc_mcontext.gregs[REG_RSP] = (long int) sp;
+  ucp->uc_mcontext.gregs[REG_RBX] = (greg_t) (uintptr_t) &sp[idx_uc_link];
+  ucp->uc_mcontext.gregs[REG_RSP] = (greg_t) (uintptr_t) sp;
 
   /* Setup stack.  */
-  sp[0] = (unsigned long int) &__start_context;
-  sp[idx_uc_link] = (unsigned long int) ucp->uc_link;
+  sp[0] = (greg_t) (uintptr_t) &__start_context;
+  sp[idx_uc_link] = (greg_t) (uintptr_t) ucp->uc_link;
 
   va_start (ap, argc);
   /* Handle arguments.
@@ -90,26 +91,26 @@ __makecontext (ucontext_t *ucp, void (*func) (void), int argc, ...)
     switch (i)
       {
       case 0:
-	ucp->uc_mcontext.gregs[REG_RDI] = va_arg (ap, long int);
+	ucp->uc_mcontext.gregs[REG_RDI] = va_arg (ap, greg_t);
 	break;
       case 1:
-	ucp->uc_mcontext.gregs[REG_RSI] = va_arg (ap, long int);
+	ucp->uc_mcontext.gregs[REG_RSI] = va_arg (ap, greg_t);
 	break;
       case 2:
-	ucp->uc_mcontext.gregs[REG_RDX] = va_arg (ap, long int);
+	ucp->uc_mcontext.gregs[REG_RDX] = va_arg (ap, greg_t);
 	break;
       case 3:
-	ucp->uc_mcontext.gregs[REG_RCX] = va_arg (ap, long int);
+	ucp->uc_mcontext.gregs[REG_RCX] = va_arg (ap, greg_t);
 	break;
       case 4:
-	ucp->uc_mcontext.gregs[REG_R8] = va_arg (ap, long int);
+	ucp->uc_mcontext.gregs[REG_R8] = va_arg (ap, greg_t);
 	break;
       case 5:
-	ucp->uc_mcontext.gregs[REG_R9] = va_arg (ap, long int);
+	ucp->uc_mcontext.gregs[REG_R9] = va_arg (ap, greg_t);
 	break;
       default:
 	/* Put value on stack.  */
-	sp[i - 5] = va_arg (ap, unsigned long int);
+	sp[i - 5] = va_arg (ap, greg_t);
 	break;
       }
   va_end (ap);