Re: [patch] Fix i386 memory-by-register access on amd64

["Ulrich Weigand" <uweigand@de.ibm.com>]

Jan Kratochvil wrote:

> Updated the patch to do on 64bit hosts exactly the same what 32bit hosts do.
> 32bit hosts do all the CORE_ADDR calculations 64bit, just the final ptrace
> call strips the width to 32bits.

Hmm, I'm wondering how this would affect platforms where addresses are
generally treated as signed integers (MIPS ?).  Dan, do you know if the
kernel expects the ptrace address argument to be sign-extended on MIPS?

> There is some suspection a similiar patch would be appropriate for theses
> functions but I have no such test OSes/machines available:
> 	config/pa/hpux.mh	inf-ttrace.c	inf_ttrace_xfer_memory
> 	config/powerpc/aix.mh	rs6000-nat.c	rs6000_xfer_partial

It seems the AIX version already performs truncations via casts:

                  buffer.word = rs6000_ptrace32 (PT_READ_I, pid,
                                                 (int *)(uintptr_t)rounded_offset,
                                                 0, NULL);

> @@ -452,8 +452,20 @@ inf_ptrace_xfer_partial (struct target_ops *ops, enum target_object object,
>  			 const gdb_byte *writebuf,
>  			 ULONGEST offset, LONGEST len)
>  {
> +  struct gdbarch *gdbarch = target_thread_architecture (inferior_ptid);
> +  int addr_bit = gdbarch_addr_bit (gdbarch);

target_thread_architecture is wrong for this purpose; it is the user-visible
architecture to be used for this thread.  The architecture to be used for
target (e.g. ptrace) operations is target_gdbarch.

For example, on an SPU thread on the Cell/B.E. target_thread_architecture might 
be SPU, while target_gdbarch is PPC32 or PPC64.  ptrace operations need to operate
according to the latter.

> +  /* 32-bit host will pass only the lower 32-bits of OFFSET to the ptrace
> +     syscall.  64-bit host debugging 32-bit inferior would get EIO for non-zero
> +     higher 32-bits in the same case.  Match the behavior of 32-bit host GDB
> +     for 64-bit host GDB debugging 32-bit inferior.
> +
> +     Compare ADDR_BIT first to avoid a compiler warning on shift overflow.  */
> +  gdb_assert (sizeof (offset) == sizeof (ULONGEST));
> +  if (addr_bit < (sizeof (ULONGEST) * HOST_CHAR_BIT))
> +    offset &= ((ULONGEST) 1 << addr_bit) - 1;

This should be done inside the TARGET_OBJECT_MEMORY case; there is no reason
why the same truncation should be performed for other object types.

(The assert seems superfluous to me; "offset" is a local variable to this
function, so we should know its type already.  Other code in this function
would already fail if offset were of any other type.)


Otherwise, the patch looks reasonable to me -- if the MIPS question above
can be resolved.

Bye,
Ulrich

-- 
  Dr. Ulrich Weigand
  GNU Toolchain for Linux on System z and Cell BE
  Ulrich.Weigand@de.ibm.com