Re: PATCH: Add x32 arch_prctl support

[Roland McGrath <roland at hack dot frob dot com>]

> I submitted a patch to remove x32 PTRACE_ARCH_PRCTL from
> kernel.  Here is a patch with only arch_prctl.c. OK to install?

I'm certainly happy with the ptrace cleanup.

> +/* Since x32 arch_prctl stores 32-bit base address of segment register %fs

"the 32-bit ..."
"segment registers"

> +   and %gs as unsigned 64-bit value via ARCH_GET_FS and ARCH_GET_GS, we

"as an unsigned 64-bit value"

> +   use a local unsigned 64-bit variable to hold the base address and copy
> +   it to ADDR after arch_prctl return.  */

"after the system call returns"

> +int
> +__arch_prctl (int code, uintptr_t *addr)
> +{
> +  int res;
> +  uint64_t addr64;
> +  uintptr_t *addr_saved;
> +
> +  switch (code)
> +    {
> +    case ARCH_GET_FS:
> +    case ARCH_GET_GS:
> +      addr_saved = addr;
> +      addr = (uintptr_t *) &addr64;
> +      break;
> +    }

I think this will still get a warning about addr_saved.
I think you can avoid it without affecting the generated code:

	{
	  int res;
	  void *prctl_arg = addr;
	  uint64_t addr64;

	  switch (code)
	    {
	    case ARCH_GET_FS:
	    case ARCH_GET_GS:
	      prctl_arg = &addr64;
	      break;
	    }

	  res = INLINE_SYSCALL (arch_prctl, 2, code, prctl_arg);
	  if (res == 0)
	    switch (code)
	      {
	      case ARCH_GET_FS:
	      case ARCH_GET_GS:
		 /* Check for a large value that overflows.  */
		if ((uintptr_t) addr64 != addr64)
		  {
		    __set_errno (EOVERFLOW);
		    return -1;
		  }
		*addr = (uintptr_t) addr64;
		break;
	      }

	  return res;
	}

Even if that gets a warning about addr64, I think it's cleaner with fewer
casts.

> +}
> +
> +weak_alias (__arch_prctl, arch_prctl)

We usually don't have a blank line before a weak_alias like this.


Thanks,
Roland