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

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

[Jan Kratochvil]

Hi,

original bugreport:
	https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=181390

(gdb) x/x $esp
0xffffce70:     0x00000001
(gdb) x/x $ebx
0xffffce70:     Cannot access memory at address 0xffffce70
(gdb) x/x 0xffffce70
0xffffce70:     0x00000001

One point is there should have been printed this error message instead:
0xffffffffffffce70:     Cannot access memory at address 0xffffffffffffce70
but this problem is just a consequence of paddress() truncating the printed
address width.  This printing issue is unrelated to the patch below.

The error happens because $ebx is considered signed while $esp unsigned, as
initialized by i386_register_type (or also amd64_register_type).  Therefore
the address width should be cut to the right size at the right point of
processing, I hope I caught (one of) such points.

Regression-tested on x86_64-unknown-linux-gnu (PASS), i386 build with
unix/-m32 (test skipped) and native build with unix/-m64 (new test FAILs as
the test's additionla_flags=-m32 gets overriden by target board's -m64).


Thanks,
Jan


2006-09-28  Jan Kratochvil  <jan.kratochvil@redhat.com>

	Fix signed 32bit inferior registers on 64bit GDB.
	* gdb/value.c (value_as_address): Make it static, rename it to ...
	(value_as_address1): ... this function.
	(value_as_address): New function.

2008-03-02  Jan Kratochvil  <jan.kratochvil@redhat.com>

	* gdb.arch/amd64-i386-address.exp, gdb.arch/amd64-i386-address.S: New.

--- gdb/value.c	21 Mar 2009 03:03:53 -0000	1.79
+++ gdb/value.c	29 Apr 2009 10:07:20 -0000
@@ -1258,11 +1258,11 @@ value_as_double (struct value *val)
   return foo;
 }
 
-/* Extract a value as a C pointer. Does not deallocate the value.  
-   Note that val's type may not actually be a pointer; value_as_long
-   handles all the cases.  */
-CORE_ADDR
-value_as_address (struct value *val)
+/* Extract a value as a C pointer.  Helper for value_as_address still does not
+   truncate the CORE_ADDR width.  */
+
+static CORE_ADDR
+value_as_address1 (struct value *val)
 {
   /* Assume a CORE_ADDR can fit in a LONGEST (for now).  Not sure
      whether we want this to be true eventually.  */
@@ -1362,6 +1362,27 @@ value_as_address (struct value *val)
   return unpack_long (value_type (val), value_contents (val));
 #endif
 }
+
+/* Extract a value as a C pointer.  Does not deallocate the value.  
+   Note that val's type may not actually be a pointer; value_as_long
+   handles all the cases.
+
+   This wrapper truncates the width to match target address width,  */
+
+CORE_ADDR
+value_as_address (struct value *val)
+{
+  CORE_ADDR addr;
+  int addr_bit = gdbarch_addr_bit (current_gdbarch);
+
+  addr = value_as_address1 (val);
+
+  /* Compare ADDR_BIT first to avoid a compiler warning on shift overflow.  */
+  if (addr_bit < (sizeof (CORE_ADDR) * HOST_CHAR_BIT))
+    addr &= ((CORE_ADDR) 1 << addr_bit) - 1;
+
+  return addr;
+}
 \f
 /* Unpack raw data (copied from debugee, target byte order) at VALADDR
    as a long, or as a double, assuming the raw data is described
--- /dev/null	1 Jan 1970 00:00:00 -0000
+++ gdb/testsuite/gdb.arch/amd64-i386-address.S	29 Apr 2009 10:07:20 -0000
@@ -0,0 +1,25 @@
+/* Copyright 2009 Free Software Foundation, Inc.
+
+   This program is free software; you can redistribute it and/or modify
+   it under the terms of the GNU General Public License as published by
+   the Free Software Foundation; either version 3 of the License, or
+   (at your option) any later version.
+
+   This program is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+   GNU General Public License for more details.
+
+   You should have received a copy of the GNU General Public License
+   along with this program.  If not, see <http://www.gnu.org/licenses/>.
+
+   This file is part of the gdb testsuite.  */
+
+_start:	.globl	_start
+	nop
+	int3
+	movl	%esp, %ebx
+	/* Examining memory from $ebx fails, from $esp it succeeds.  */
+	int3
+	nop
+	nop
--- /dev/null	1 Jan 1970 00:00:00 -0000
+++ gdb/testsuite/gdb.arch/amd64-i386-address.exp	29 Apr 2009 10:07:20 -0000
@@ -0,0 +1,44 @@
+# Copyright 2009 Free Software Foundation, Inc.
+
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 3 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.
+
+# This file is part of the gdb testsuite.
+
+# Test UNsigned extension of the 32-bit inferior address on a 64-bit host.
+
+if {![istarget "x86_64-*-*"]} then {
+    verbose "Skipping amd64->i386 adress test."
+    return
+}
+
+if [prepare_for_testing amd64-i386-address.exp amd64-i386-address amd64-i386-address.S [list debug "additional_flags=-m32 -nostdlib"]] {
+    return -1
+}
+
+gdb_run_cmd
+
+set test "trap stop"
+gdb_test_multiple "" $test {
+    -re "Program received signal SIGTRAP,.*_start .*$gdb_prompt $" {
+	pass $test
+    }
+}
+
+gdb_test "stepi" ".*_start .*int3.*"
+
+gdb_test "x/x \$esp" "0x\[0-9a-f\]*:\t0x0*1"
+
+# Failure case would be:
+# 	0xff8d7f00:     Cannot access memory at address 0xff8d7f00
+gdb_test "x/x \$ebx" "0x\[0-9a-f\]*:\t0x0*1"

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

[Mark Kettenis]

> Date: Wed, 29 Apr 2009 12:27:19 +0200
> From: Jan Kratochvil <jan.kratochvil@redhat.com>
> 
> Hi,
> 
> original bugreport:
> 	https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=181390
> 
> (gdb) x/x $esp
> 0xffffce70:     0x00000001
> (gdb) x/x $ebx
> 0xffffce70:     Cannot access memory at address 0xffffce70
> (gdb) x/x 0xffffce70
> 0xffffce70:     0x00000001
> 
> One point is there should have been printed this error message instead:
> 0xffffffffffffce70:     Cannot access memory at address 0xffffffffffffce70
> but this problem is just a consequence of paddress() truncating the printed
> address width.  This printing issue is unrelated to the patch below.
> 
> The error happens because $ebx is considered signed while $esp unsigned, as
> initialized by i386_register_type (or also amd64_register_type).  Therefore
> the address width should be cut to the right size at the right point of
> processing, I hope I caught (one of) such points.

I'm not sure this is the right solution.  On 64-bit machines where
addresses are signed I think we actually want the sign extension to
happen.

> 2006-09-28  Jan Kratochvil  <jan.kratochvil@redhat.com>
> 
> 	Fix signed 32bit inferior registers on 64bit GDB.
> 	* gdb/value.c (value_as_address): Make it static, rename it to ...
> 	(value_as_address1): ... this function.
> 	(value_as_address): New function.

What is your motiviation for using a wrapper function?

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

[Jan Kratochvil]

On Wed, 29 Apr 2009 21:04:33 +0200, Mark Kettenis wrote:
> I'm not sure this is the right solution.  On 64-bit machines where
> addresses are signed I think we actually want the sign extension to
> happen.

While not trying to judge what is right or wrong:

I believe gdb.x86_64 debugging gdb.i386 inferior should behave exactly as
gdb.i386 debugging gdb.i386 inferior.

As gdb.i386 already has sizeof (CORE_ADDR) == 4 I find right that gdb.x86_64
with i386 inferior should cut CORE_ADDR whenever possible.

Otherwise we should fix gdb.i386 to also error on this (current behavior) case:
(gdb) x/x 0xfffffffff7ffcfc4
0xf7ffcfc4:	0x00020efc


> > 2006-09-28  Jan Kratochvil  <jan.kratochvil@redhat.com>
> > 
> > 	Fix signed 32bit inferior registers on 64bit GDB.
> > 	* gdb/value.c (value_as_address): Make it static, rename it to ...
> > 	(value_as_address1): ... this function.
> > 	(value_as_address): New function.
> 
> What is your motiviation for using a wrapper function?

As value_as_address is a long function with many `return' commands.  But I do
not have any strong opinion on it - would you like to fill a variable and
using a single exit path which would cut the result width?


Thanks,
Jan

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

[Jan Kratochvil]

On Wed, 29 Apr 2009 22:29:16 +0200, Jan Kratochvil wrote:
> On Wed, 29 Apr 2009 21:04:33 +0200, Mark Kettenis wrote:
> > I'm not sure this is the right solution.  On 64-bit machines where
> > addresses are signed I think we actually want the sign extension to
> > happen.
> 
> While not trying to judge what is right or wrong:
> 
> I believe gdb.x86_64 debugging gdb.i386 inferior should behave exactly as
> gdb.i386 debugging gdb.i386 inferior.
> 
> As gdb.i386 already has sizeof (CORE_ADDR) == 4 I find right that gdb.x86_64
> with i386 inferior should cut CORE_ADDR whenever possible.

That's not true, even gdb.i386 has sizeof (CORE_ADDR) == 8.  But the behavior
is as described.


> Otherwise we should fix gdb.i386 to also error on this (current behavior) case:
> (gdb) x/x 0xfffffffff7ffcfc4
> 0xf7ffcfc4:	0x00020efc


Sorry,
Jan

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

[Tom Tromey]

>>>>> "Jan" == Jan Kratochvil <jan.kratochvil@redhat.com> writes:

Resurrecting an oldish thread.

Jan> While not trying to judge what is right or wrong:

Jan> I believe gdb.x86_64 debugging gdb.i386 inferior should behave exactly as
Jan> gdb.i386 debugging gdb.i386 inferior.

This sounds reasonable to me.  If I'm debugging a 32-bit inferior, it
seems weird to see a 64-bit address.

Jan> As value_as_address is a long function with many `return'
Jan> commands.  But I do not have any strong opinion on it - would you
Jan> like to fill a variable and using a single exit path which would
Jan> cut the result width?

This sounds ok to me.  value_as_address is really long, true, but it
is mostly comments.  There are really only 3 returns.

Tom

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

[Jan Kratochvil]

On Thu, 25 Jun 2009 18:33:43 +0200, Tom Tromey wrote:
> 
> Jan> I believe gdb.x86_64 debugging gdb.i386 inferior should behave exactly as
> Jan> gdb.i386 debugging gdb.i386 inferior.
> 
> This sounds reasonable to me.  If I'm debugging a 32-bit inferior, it
> seems weird to see a 64-bit address.

Found out now the original patch
	http://sourceware.org/ml/gdb-patches/2009-04/msg00786.html

had a regression on x86_64-fedora-linux-gnu for `--target_board unix/-m32':
	+FAIL: gdb.base/dump.exp: array copy, srec; value restored ok
	+FAIL: gdb.base/dump.exp: array copy, ihex; value restored ok
	+FAIL: gdb.base/dump.exp: array copy, tekhex; value restored ok
	+FAIL: gdb.base/dump.exp: array partial with expressions; value restored ok

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.

Created a new (PASSing) test `highmem-debugger' to ensure ptrace will still
behave as expected (returning EIO and not just stripping the address width):
	http://sourceware.org/systemtap/wiki/utrace/tests

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

As I changed the patch requesting new approval.  Sorry for the wrong patch.

Regression tested on:
x86_64-fedora-linux-gnu with `--target_board unix/-m64'.
x86_64-fedora-linux-gnu with `--target_board unix/-m32'.
  i386-fedora-linux-gnu with `--target_board unix/-m32'.


Thanks,
Jan


2009-07-06  Jan Kratochvil  <jan.kratochvil@redhat.com>

	Fix signed 32bit inferior registers on 64bit GDB.
	* inf-ptrace.c (inf_ptrace_xfer_partial): New variables gdbarch
	and addr_bit.  Mask OFFSET by the ADDR_BIT width.

2009-07-06  Jan Kratochvil  <jan.kratochvil@redhat.com>

	* gdb.arch/amd64-i386-address.exp, gdb.arch/amd64-i386-address.S: New.

--- a/gdb/inf-ptrace.c
+++ b/gdb/inf-ptrace.c
@@ -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);
   pid_t pid = ptid_get_pid (inferior_ptid);
 
+  /* 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;
+
   switch (object)
     {
     case TARGET_OBJECT_MEMORY:
new file mode 100644
--- /dev/null
+++ b/gdb/testsuite/gdb.arch/amd64-i386-address.S
@@ -0,0 +1,25 @@
+/* Copyright 2009 Free Software Foundation, Inc.
+
+   This program is free software; you can redistribute it and/or modify
+   it under the terms of the GNU General Public License as published by
+   the Free Software Foundation; either version 3 of the License, or
+   (at your option) any later version.
+
+   This program is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+   GNU General Public License for more details.
+
+   You should have received a copy of the GNU General Public License
+   along with this program.  If not, see <http://www.gnu.org/licenses/>.
+
+   This file is part of the gdb testsuite.  */
+
+_start:	.globl	_start
+	nop
+	int3
+	movl	%esp, %ebx
+	/* Examining memory from $ebx fails, from $esp it succeeds.  */
+	int3
+	nop
+	nop
new file mode 100644
--- /dev/null
+++ b/gdb/testsuite/gdb.arch/amd64-i386-address.exp
@@ -0,0 +1,45 @@
+# Copyright 2009 Free Software Foundation, Inc.
+
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 3 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.
+
+# This file is part of the gdb testsuite.
+
+# Test UNsigned extension of the 32-bit inferior address on a 64-bit host.
+# On native 32-bit host the test always PASSed.
+
+if {![istarget "x86_64-*-*"] && ![istarget "i?86-*-*"]} then {
+    verbose "Skipping amd64->i386 adress test."
+    return
+}
+
+if [prepare_for_testing amd64-i386-address.exp amd64-i386-address amd64-i386-address.S [list debug "additional_flags=-m32 -nostdlib"]] {
+    return -1
+}
+
+gdb_run_cmd
+
+set test "trap stop"
+gdb_test_multiple "" $test {
+    -re "Program received signal SIGTRAP,.*_start .*$gdb_prompt $" {
+	pass $test
+    }
+}
+
+gdb_test "stepi" ".*_start .*int3.*"
+
+gdb_test "x/x \$esp" "0x\[0-9a-f\]*:\t0x0*1"
+
+# Failure case would be:
+# 	0xff8d7f00:     Cannot access memory at address 0xff8d7f00
+gdb_test "x/x \$ebx" "0x\[0-9a-f\]*:\t0x0*1"

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

[Ulrich Weigand]

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

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

[Daniel Jacobowitz]

On Tue, Jul 07, 2009 at 06:24:06PM +0200, Ulrich Weigand wrote:
> 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?

Ptrace takes a long.  If GDB is o32/n32, we're only passing it 32
bits.  If we are an n64 application, debugging an o32 application, I'm
not sure what happens with sign extension... but it doesn't much
matter; 32-bit userspace applications only get to use the low half of
the address space anyway.

-- 
Daniel Jacobowitz
CodeSourcery

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

[Mark Kettenis]

> Date: Tue, 7 Jul 2009 18:24:06 +0200 (CEST)
> From: "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?

Yes, I expect that to be the case for targets that do an ILP32 ABI on
a 64-bit CPU where there's a "hole" in the moddle of the 64-bit
virtual adddress space.

> 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.

But then I don't understand Jan's diff at all.  Linux has its own
implementation for TARGET_OBJECT_MEMORY in linux-nat.c.  Why isn't
that one used?

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

[Jan Kratochvil]

On Tue, 07 Jul 2009 19:59:43 +0200, Mark Kettenis wrote:
> But then I don't understand Jan's diff at all.  Linux has its own
> implementation for TARGET_OBJECT_MEMORY in linux-nat.c.  Why isn't
> that one used?

Expecting the same problem must affect even non-Linux ptrace usage.  I will
move it to linux-nat.c if you think so.


Thanks,
Jan

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

[Mark Kettenis]

> Date: Tue, 7 Jul 2009 20:22:04 +0200
> From: Jan Kratochvil <jan.kratochvil@redhat.com>
> 
> On Tue, 07 Jul 2009 19:59:43 +0200, Mark Kettenis wrote:
> > But then I don't understand Jan's diff at all.  Linux has its own
> > implementation for TARGET_OBJECT_MEMORY in linux-nat.c.  Why isn't
> > that one used?
> 
> Expecting the same problem must affect even non-Linux ptrace usage.  I will
> move it to linux-nat.c if you think so.

I suppose I should have been more specific.  You are trying to fix a
bug that you see on Linux isn't it?

And this fix should only be relevant for TARGET_OBJECT_MEMORY isn't
it?

But Linux has its own implementation for doing TARGET_OBJECT_MEMORY
xfers in linux_proc_xfer_partial.

So I don't understand how this change fixes anything on Linux.

Can you enlighten me here?

[patch] /* */ for target_thread_architecture [Re: [patch] Fix i386 memory-by-register access on amd64]

[Jan Kratochvil]

On Tue, 07 Jul 2009 18:24:06 +0200, Ulrich Weigand wrote:
> Jan Kratochvil wrote:
> > @@ -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.

While thanks for the explanation I believe it should be in the sources.  It is
mostly your text from:
	http://sourceware.org/ml/gdb-patches/2008-09/msg00133.html

OK to check-in?


Thanks,
Jan


2009-07-08  Ulrich Weigand  <uweigand@de.ibm.com>
	    Jan Kratochvil  <jan.kratochvil@redhat.com>

	* target.h (struct target_ops <to_thread_architecture>): Describe it.
	(target_thread_architecture): Refer to to_thread_architecture.

--- a/gdb/target.h
+++ b/gdb/target.h
@@ -543,7 +543,16 @@ struct target_ops
        simultaneously?  */
     int (*to_supports_multi_process) (void);
 
-    /* Determine current architecture of thread PTID.  */
+    /* Determine current architecture of thread PTID.
+
+       The target is supposed to determine the architecture of the code where
+       the target is currently stopped at (on Cell, if a target is in spu_run,
+       to_thread_architecture would return SPU, otherwise PPC32 or PPC64).
+       This is architecture used to perform decr_pc_after_break adjustment,
+       and also determines the frame architecture of the innermost frame.
+       ptrace operations need to operate according to target_gdbarch.
+
+       The default implementation always returns target_gdbarch.  */
     struct gdbarch *(*to_thread_architecture) (struct target_ops *, ptid_t);
 
     int to_magic;
@@ -1043,7 +1052,7 @@ extern char *normal_pid_to_str (ptid_t ptid);
 #define target_pid_to_exec_file(pid) \
      (current_target.to_pid_to_exec_file) (pid)
 
-/* Determine current architecture of thread PTID.  */
+/* See the to_thread_architecture description in struct target_ops.  */
 
 #define target_thread_architecture(ptid) \
      (current_target.to_thread_architecture (&current_target, ptid))

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

[Jan Kratochvil]

On Tue, 07 Jul 2009 18:24:06 +0200, Ulrich Weigand wrote:
> target_thread_architecture is wrong for this purpose;
+
> This should be done inside the TARGET_OBJECT_MEMORY case;

Fixed in the patch.


> (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.)

Originally I thought OFFSET should be later changed to CORE_ADDR.  Now I see
for other `enum target_object' larger OFFSET may make sense.


On Tue, 07 Jul 2009 20:43:18 +0200, Mark Kettenis wrote:
> > Date: Tue, 7 Jul 2009 20:22:04 +0200
> > From: Jan Kratochvil <jan.kratochvil@redhat.com>
> > 
> > On Tue, 07 Jul 2009 19:59:43 +0200, Mark Kettenis wrote:
> > > But then I don't understand Jan's diff at all.  Linux has its own
> > > implementation for TARGET_OBJECT_MEMORY in linux-nat.c.  Why isn't
> > > that one used?
> > 
> > Expecting the same problem must affect even non-Linux ptrace usage.  I will
> > move it to linux-nat.c if you think so.
> 
> I suppose I should have been more specific.  You are trying to fix a
> bug that you see on Linux isn't it?

Yes.

> And this fix should only be relevant for TARGET_OBJECT_MEMORY isn't
> it?

Yes.  I was not sure before and I though it would not hurt other
`enum target_object's but - you are right, yes.


> But Linux has its own implementation for doing TARGET_OBJECT_MEMORY
> xfers in linux_proc_xfer_partial.

I did not much notice linux_proc_xfer_partial role in this bug, thanks.

* I was not able to reproduce the problem for linux_proc_xfer_partial without
  removing there:
    /* Don't bother for one word.  */
    if (len < 3 * sizeof (long))
      return 0;
  because:
   * x/gx is the largest size - 8 bytes - which is still smaller.
   * x/NUMx generates NUM small transfers (not one NUM*8 transfer).
   * These already truncate the address to gdbarch_addr_bit in the caller:
     * dump memory
     * print *(struct large *) $ebx

* inf_ptrace_xfer_partial gets silently used as a fallback when
  linux_proc_xfer_partial fails.

  * linux_proc_xfer_partial has been broken so far even on native 32bit GDB
    debugging 32bit inferior with GDB built using --enable-64-bit-bfd (and
    thus having 64-bit CORE_ADDR).  Due to the silent fallback just nobody has
    noticed it.

Therefore fixed linux_xfer_partial.  Therefore it no longer needs the fix to
be present also in inf_ptrace_xfer_partial.  I do not know how non-Linux OSes
handle the debugging of 32bit inferior on 64bit GDB so I have no opinion
whether the inf_ptrace_xfer_partial patch makes sense, i can drop it.


> So I don't understand how this change fixes anything on Linux.

With my former patch linux_proc_xfer_partial failed and the fix in the ptrace
backend did handle the transfer.


Thanks,
Jan


2009-07-08  Jan Kratochvil  <jan.kratochvil@redhat.com>

	Fix memory access from signed 32bit inferior registers on 64bit GDB.
	* inf-ptrace.c (inf_ptrace_xfer_partial <TARGET_OBJECT_MEMORY>): New
	variable addr_bit.  Mask OFFSET by the ADDR_BIT width.
	* linux-nat.c (linux_xfer_partial <TARGET_OBJECT_MEMORY>): Likewise.

2009-07-08  Jan Kratochvil  <jan.kratochvil@redhat.com>

	* gdb.arch/amd64-i386-address.exp, gdb.arch/amd64-i386-address.S: New.

--- a/gdb/inf-ptrace.c
+++ b/gdb/inf-ptrace.c
@@ -457,6 +457,19 @@ inf_ptrace_xfer_partial (struct target_ops *ops, enum target_object object,
   switch (object)
     {
     case TARGET_OBJECT_MEMORY:
+      {
+	int addr_bit = gdbarch_addr_bit (target_gdbarch);
+
+	/* GDB calculates all the addresses in possibly larget width of the
+	   address.  Address width needs to be masked before its final use
+	   - either by linux_proc_xfer_partial or inf_ptrace_xfer_partial.
+
+	   Compare ADDR_BIT first to avoid a compiler warning on shift
+	   overflow.  */
+
+	if (addr_bit < (sizeof (ULONGEST) * HOST_CHAR_BIT))
+	  offset &= ((ULONGEST) 1 << addr_bit) - 1;
+      }
 #ifdef PT_IO
       /* OpenBSD 3.1, NetBSD 1.6 and FreeBSD 5.0 have a new PT_IO
 	 request that promises to be much more efficient in reading
--- a/gdb/linux-nat.c
+++ b/gdb/linux-nat.c
@@ -4260,6 +4260,20 @@ linux_xfer_partial (struct target_ops *ops, enum target_object object,
     return linux_nat_xfer_osdata (ops, object, annex, readbuf, writebuf,
                                offset, len);
 
+  /* GDB calculates all the addresses in possibly larget width of the address.
+     Address width needs to be masked before its final use - either by
+     linux_proc_xfer_partial or inf_ptrace_xfer_partial.
+
+     Compare ADDR_BIT first to avoid a compiler warning on shift overflow.  */
+
+  if (object == TARGET_OBJECT_MEMORY)
+    {
+      int addr_bit = gdbarch_addr_bit (target_gdbarch);
+
+      if (addr_bit < (sizeof (ULONGEST) * HOST_CHAR_BIT))
+	offset &= ((ULONGEST) 1 << addr_bit) - 1;
+    }
+
   xfer = linux_proc_xfer_partial (ops, object, annex, readbuf, writebuf,
 				  offset, len);
   if (xfer != 0)
--- /dev/null
+++ b/gdb/testsuite/gdb.arch/amd64-i386-address.S
@@ -0,0 +1,24 @@
+/* Copyright 2009 Free Software Foundation, Inc.
+
+   This program is free software; you can redistribute it and/or modify
+   it under the terms of the GNU General Public License as published by
+   the Free Software Foundation; either version 3 of the License, or
+   (at your option) any later version.
+
+   This program is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+   GNU General Public License for more details.
+
+   You should have received a copy of the GNU General Public License
+   along with this program.  If not, see <http://www.gnu.org/licenses/>.
+
+   This file is part of the gdb testsuite.  */
+
+_start:	.globl	_start
+	movl	$0xdeadf00d, %eax
+	pushl	%eax
+	movl	%esp, %ebx
+	int3
+	nop
+	nop
--- /dev/null
+++ b/gdb/testsuite/gdb.arch/amd64-i386-address.exp
@@ -0,0 +1,43 @@
+# Copyright 2009 Free Software Foundation, Inc.
+
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 3 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.
+
+# This file is part of the gdb testsuite.
+
+# Test UNsigned extension of the 32-bit inferior address on a 64-bit host.
+# On native 32-bit host the test always PASSed.
+
+if {![istarget "x86_64-*-*"] && ![istarget "i?86-*-*"]} then {
+    verbose "Skipping amd64->i386 adress test."
+    return
+}
+
+if [prepare_for_testing amd64-i386-address.exp amd64-i386-address amd64-i386-address.S [list debug "additional_flags=-m32 -nostdlib"]] {
+    return -1
+}
+
+gdb_run_cmd
+
+set test "trap stop"
+gdb_test_multiple "" $test {
+    -re "Program received signal SIGTRAP,.*_start .*$gdb_prompt $" {
+	pass $test
+    }
+}
+
+gdb_test "x/wx \$esp" "0x\[0-9a-f\]*:\t0xdeadf00d"
+
+# Failure case would be:
+# 	0xff8d7f00:     Cannot access memory at address 0xff8d7f00
+gdb_test "x/wx \$ebx" "0x\[0-9a-f\]*:\t0xdeadf00d"

Re: [patch] /* */ for target_thread_architecture [Re: [patch] Fix i386 memory-by-register access on amd64]

[Ulrich Weigand]

Jan Kratochvil wrote:

> While thanks for the explanation I believe it should be in the sources.

Good point.

> 2009-07-08  Ulrich Weigand  <uweigand@de.ibm.com>
> 	    Jan Kratochvil  <jan.kratochvil@redhat.com>
> 
> 	* target.h (struct target_ops <to_thread_architecture>): Describe it.
> 	(target_thread_architecture): Refer to to_thread_architecture.

This is OK.

Thanks,
Ulrich

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

Re: [patch] /* */ for target_thread_architecture [Re: [patch] Fix i386 memory-by-register access on amd64]

[Jan Kratochvil]

On Thu, 09 Jul 2009 14:20:04 +0200, Ulrich Weigand wrote:
> Jan Kratochvil wrote:
> > 2009-07-08  Ulrich Weigand  <uweigand@de.ibm.com>
> > 	    Jan Kratochvil  <jan.kratochvil@redhat.com>
> > 
> > 	* target.h (struct target_ops <to_thread_architecture>): Describe it.
> > 	(target_thread_architecture): Refer to to_thread_architecture.
> 
> This is OK.

Checked-in:
	http://sourceware.org/ml/gdb-cvs/2009-07/msg00076.html


Thanks,
Jan

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

[Ulrich Weigand]

Jan Kratochvil wrote:

> Therefore fixed linux_xfer_partial.  Therefore it no longer needs the fix to
> be present also in inf_ptrace_xfer_partial.  I do not know how non-Linux OSes
> handle the debugging of 32bit inferior on 64bit GDB so I have no opinion
> whether the inf_ptrace_xfer_partial patch makes sense, i can drop it.

I think at this point it would probably be better to just to the Linux fix.
I've looked at a couple of ptrace implementations in the Linux kernel, and
it seems this fix should be correct on all bi-arch Linux platforms.

I simply do not know enough how ptrace behaves on other OSes -- until we
get a specific bug report on some other OS, I think we should leave the
behaviour unchanged for now.

> 2009-07-08  Jan Kratochvil  <jan.kratochvil@redhat.com>
> 
> 	Fix memory access from signed 32bit inferior registers on 64bit GDB.
> 	* inf-ptrace.c (inf_ptrace_xfer_partial <TARGET_OBJECT_MEMORY>): New
> 	variable addr_bit.  Mask OFFSET by the ADDR_BIT width.
> 	* linux-nat.c (linux_xfer_partial <TARGET_OBJECT_MEMORY>): Likewise.
> 
> 2009-07-08  Jan Kratochvil  <jan.kratochvil@redhat.com>
> 
> 	* gdb.arch/amd64-i386-address.exp, gdb.arch/amd64-i386-address.S: New.

The linux-nat.c change and the test case are OK.

Thanks,
Ulrich

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

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

[Mark Kettenis]

> Date: Mon, 13 Jul 2009 19:08:09 +0200 (CEST)
> From: "Ulrich Weigand" <uweigand@de.ibm.com>
> 
> Jan Kratochvil wrote:
> 
> > Therefore fixed linux_xfer_partial.  Therefore it no longer needs
> > the fix to be present also in inf_ptrace_xfer_partial.  I do not
> > know how non-Linux OSes handle the debugging of 32bit inferior on
> > 64bit GDB so I have no opinion whether the inf_ptrace_xfer_partial
> > patch makes sense, i can drop it.
> 
> I think at this point it would probably be better to just to the Linux fix.
> I've looked at a couple of ptrace implementations in the Linux kernel, and
> it seems this fix should be correct on all bi-arch Linux platforms.
> 
> I simply do not know enough how ptrace behaves on other OSes -- until we
> get a specific bug report on some other OS, I think we should leave the
> behaviour unchanged for now.

I agree.

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

[Jan Kratochvil]

On Mon, 13 Jul 2009 19:08:09 +0200, Ulrich Weigand wrote:
> I've looked at a couple of ptrace implementations in the Linux kernel, and
> it seems this fix should be correct on all bi-arch Linux platforms.

Extended now the kernel ptrace testcase / tested besides x86_64 now also s390x
and ppc64:
	http://sources.redhat.com/cgi-bin/cvsweb.cgi/~checkout~/tests/ptrace-tests/tests/highmem-debugger.c?cvsroot=systemtap
	http://sources.redhat.com/cgi-bin/cvsweb.cgi/~checkout~/tests/ptrace-tests/tests/highmem-debuggee.c?cvsroot=systemtap


> > 2009-07-08  Jan Kratochvil  <jan.kratochvil@redhat.com>
> > 
> > 	Fix memory access from signed 32bit inferior registers on 64bit GDB.
> > 	* inf-ptrace.c (inf_ptrace_xfer_partial <TARGET_OBJECT_MEMORY>): New
> > 	variable addr_bit.  Mask OFFSET by the ADDR_BIT width.
> > 	* linux-nat.c (linux_xfer_partial <TARGET_OBJECT_MEMORY>): Likewise.
> > 
> > 2009-07-08  Jan Kratochvil  <jan.kratochvil@redhat.com>
> > 
> > 	* gdb.arch/amd64-i386-address.exp, gdb.arch/amd64-i386-address.S: New.
> 
> The linux-nat.c change and the test case are OK.

Checked-in:
	http://sourceware.org/ml/gdb-cvs/2009-07/msg00102.html


Thanks,
Jan


--- src/gdb/ChangeLog	2009/07/13 04:56:12	1.10713
+++ src/gdb/ChangeLog	2009/07/13 20:16:46	1.10714
@@ -1,3 +1,9 @@
+2009-07-13  Jan Kratochvil  <jan.kratochvil@redhat.com>
+
+	Fix memory access from signed 32bit inferior registers on 64bit GDB.
+	* linux-nat.c (linux_xfer_partial <TARGET_OBJECT_MEMORY>): New variable
+	addr_bit.  Mask OFFSET by the ADDR_BIT width.
+
 2009-07-13  Anthony Green  <green@moxielogic.com>
 
 	* moxie-tdep.c (moxie_gdbarch_init): Call
--- src/gdb/linux-nat.c	2009/07/02 21:57:27	1.144
+++ src/gdb/linux-nat.c	2009/07/13 20:16:47	1.145
@@ -4260,6 +4260,20 @@
     return linux_nat_xfer_osdata (ops, object, annex, readbuf, writebuf,
                                offset, len);
 
+  /* GDB calculates all the addresses in possibly larget width of the address.
+     Address width needs to be masked before its final use - either by
+     linux_proc_xfer_partial or inf_ptrace_xfer_partial.
+
+     Compare ADDR_BIT first to avoid a compiler warning on shift overflow.  */
+
+  if (object == TARGET_OBJECT_MEMORY)
+    {
+      int addr_bit = gdbarch_addr_bit (target_gdbarch);
+
+      if (addr_bit < (sizeof (ULONGEST) * HOST_CHAR_BIT))
+	offset &= ((ULONGEST) 1 << addr_bit) - 1;
+    }
+
   xfer = linux_proc_xfer_partial (ops, object, annex, readbuf, writebuf,
 				  offset, len);
   if (xfer != 0)
--- src/gdb/testsuite/ChangeLog	2009/07/13 19:24:17	1.1925
+++ src/gdb/testsuite/ChangeLog	2009/07/13 20:16:47	1.1926
@@ -1,5 +1,9 @@
 2009-07-13  Jan Kratochvil  <jan.kratochvil@redhat.com>
 
+	* gdb.arch/amd64-i386-address.exp, gdb.arch/amd64-i386-address.S: New.
+
+2009-07-13  Jan Kratochvil  <jan.kratochvil@redhat.com>
+
 	Fix gdb.base/macscp.exp when using custom inputrc.
 	* gdb.base/completion.exp: Remove env(INPUTRC) set and restore.
 	* gdb.base/readline.exp: Remove env(INPUTRC) set and restore.  Remove
--- src/gdb/testsuite/gdb.arch/amd64-i386-address.S
+++ src/gdb/testsuite/gdb.arch/amd64-i386-address.S	2009-07-13 20:17:35.179465000 +0000
@@ -0,0 +1,24 @@
+/* Copyright 2009 Free Software Foundation, Inc.
+
+   This program is free software; you can redistribute it and/or modify
+   it under the terms of the GNU General Public License as published by
+   the Free Software Foundation; either version 3 of the License, or
+   (at your option) any later version.
+
+   This program is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+   GNU General Public License for more details.
+
+   You should have received a copy of the GNU General Public License
+   along with this program.  If not, see <http://www.gnu.org/licenses/>.
+
+   This file is part of the gdb testsuite.  */
+
+_start:	.globl	_start
+	movl	$0xdeadf00d, %eax
+	pushl	%eax
+	movl	%esp, %ebx
+	int3
+	nop
+	nop
--- src/gdb/testsuite/gdb.arch/amd64-i386-address.exp
+++ src/gdb/testsuite/gdb.arch/amd64-i386-address.exp	2009-07-13 20:17:36.993238000 +0000
@@ -0,0 +1,43 @@
+# Copyright 2009 Free Software Foundation, Inc.
+
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 3 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.
+
+# This file is part of the gdb testsuite.
+
+# Test UNsigned extension of the 32-bit inferior address on a 64-bit host.
+# On native 32-bit host the test always PASSed.
+
+if {![istarget "x86_64-*-*"] && ![istarget "i?86-*-*"]} then {
+    verbose "Skipping amd64->i386 adress test."
+    return
+}
+
+if [prepare_for_testing amd64-i386-address.exp amd64-i386-address amd64-i386-address.S [list debug "additional_flags=-m32 -nostdlib"]] {
+    return -1
+}
+
+gdb_run_cmd
+
+set test "trap stop"
+gdb_test_multiple "" $test {
+    -re "Program received signal SIGTRAP,.*_start .*$gdb_prompt $" {
+	pass $test
+    }
+}
+
+gdb_test "x/wx \$esp" "0x\[0-9a-f\]*:\t0xdeadf00d"
+
+# Failure case would be:
+# 	0xff8d7f00:     Cannot access memory at address 0xff8d7f00
+gdb_test "x/wx \$ebx" "0x\[0-9a-f\]*:\t0xdeadf00d"