[1/3] Clear non-significant bits of address on memory access

Message ID 1512727471-30745-2-git-send-email-yao.qi@linaro.org
State New, archived
Headers

Commit Message

Yao Qi Dec. 8, 2017, 10:04 a.m. UTC
  ARMv8 supports tagged address, that is, the top one byte in address
is ignored.  It is always enabled on aarch64-linux.  See
https://www.kernel.org/doc/Documentation/arm64/tagged-pointers.txt

The tag in the tagged address is modeled as non-significant bits in
address, so this patch adds a new gdbarch method significant_addr_bit and
clear the non-significant bits (the top byte in ARMv8) of the virtual
address at the point before passing address to target cache layer.  IOW,
the address used in the target cache layer is already cleared.

Before this patch,
(gdb) x/x 0x0000000000411030
0x411030 <global>:	0x00000000
(gdb) x/x 0xf000000000411030
0xf000000000411030:	Cannot access memory at address 0xf000000000411030

After this patch,

(gdb) x/x 0x0000000000411030
0x411030 <global>:	0x00000000
(gdb) x/x 0xf000000000411030
0xf000000000411030:	0x00000000

Note that I used address_significant in paddress, but it causes a
regression gdb.base/long_long.exp, because gdb clears the non-significant
bits in address, but test still expects them.

p/a val.oct^M
$24 = 0x2ee53977053977^M
(gdb) FAIL: gdb.base/long_long.exp: p/a val.oct

so I defer the change there.

gdb:

2017-10-19  Yao Qi  <yao.qi@linaro.org>

	* aarch64-linux-tdep.c (aarch64_linux_init_abi): Install gdbarch
	significant_addr_bit.
	* gdbarch.sh (significant_addr_bit): New.
	* gdbarch.c, gdbarch.h: Re-generated.
	* target.c (memory_xfer_partial): Call address_significant.
	* utils.c (address_significant): New function.
	* utils.h (address_significant): Declare.

2017-10-19  Yao Qi  <yao.qi@linaro.org>

gdb/testsuite:

	* gdb.arch/aarch64-tagged-pointer.c: New file.
	* gdb.arch/aarch64-tagged-pointer.exp: New file.
---
 gdb/aarch64-linux-tdep.c                          |  2 +
 gdb/gdbarch.c                                     | 22 ++++++++
 gdb/gdbarch.h                                     |  8 +++
 gdb/gdbarch.sh                                    |  6 ++
 gdb/target.c                                      |  2 +
 gdb/testsuite/gdb.arch/aarch64-tagged-pointer.c   | 48 ++++++++++++++++
 gdb/testsuite/gdb.arch/aarch64-tagged-pointer.exp | 67 +++++++++++++++++++++++
 gdb/utils.c                                       | 17 ++++++
 gdb/utils.h                                       |  3 +
 9 files changed, 175 insertions(+)
 create mode 100644 gdb/testsuite/gdb.arch/aarch64-tagged-pointer.c
 create mode 100644 gdb/testsuite/gdb.arch/aarch64-tagged-pointer.exp
  

Comments

Pedro Alves Dec. 8, 2017, 12:22 p.m. UTC | #1
On 12/08/2017 10:04 AM, Yao Qi wrote:
> --- a/gdb/aarch64-linux-tdep.c
> +++ b/gdb/aarch64-linux-tdep.c
> @@ -1217,6 +1217,8 @@ aarch64_linux_init_abi (struct gdbarch_info info, struct gdbarch *gdbarch)
>    set_xml_syscall_file_name (gdbarch, "syscalls/aarch64-linux.xml");
>    set_gdbarch_get_syscall_number (gdbarch, aarch64_linux_get_syscall_number);
>  
> +  set_gdbarch_significant_addr_bit (gdbarch, 56);
> +

I think adding the comment about "tag" here would be nice.

   The top bits of an address are known as the "tag" and are
   ignored by the kernel, the hardware, etc. and can be regarded
   as additional data associated with the address. */
   set_gdbarch_significant_addr_bit (gdbarch, 56);

BTW, since this is ignored by the hardware, should it be done
in aarch64-tdep.c instead of just for Linux?

Looks good to me otherwise.

Thanks,
Pedro Alves
  
Yao Qi Dec. 19, 2017, 3:41 p.m. UTC | #2
On Tue, Dec 19, 2017 at 1:50 PM, Ulrich Weigand <uweigand@de.ibm.com> wrote:
> Yao Qi wrote:
>
>> diff --git a/gdb/target.c b/gdb/target.c
>> index 3b8b8ea..767a2ad 100644
>> --- a/gdb/target.c
>> +++ b/gdb/target.c
>> @@ -1214,6 +1214,8 @@ memory_xfer_partial (struct target_ops *ops, enum target_object object,
>>    if (len == 0)
>>      return TARGET_XFER_EOF;
>>
>> +  memaddr = address_significant (target_gdbarch (), memaddr);
>> +
>>    /* Fill in READBUF with breakpoint shadows, or WRITEBUF with
>>       breakpoint insns, thus hiding out from higher layers whether
>>       there are software breakpoints inserted in the code stream.  */
>
> It turns out this breaks SPU multi-architecture debugging.  The problem is
> that SPU memory addresses have 64 significant bits since we encode the
> SPU ID in the upper 32 bits.  This means that spu-tdep.c needs to call
> set_gdbarch_significant_addr_bits -- which is fine.
>

Right, or SPU memory address has 63 significant bits?  The top bit is 1.

> However, this doesn't fix the problem, since "target_gdbarch ()" at this
> point may well return a (32-bit) ppc architecture, and then the address
> is still truncated incorrectly.  In general, using target_gdbarch is
> nearly never the right thing to do in generic code as long as we want
> to support multi-architecture debugging.
>

Can we use target_thread_architecture (inferior_ptid) instead?  If GDB
still access SPU memory even program stops in PPU, this doesn't work.

> Can this call not be pushed down further in the target stack, to below

I think you meant "Can this call be", without "not".

> the xfer_partial implementation in the spu-multiarch target?
>

I am stilling thinking how to do it...  alternatively, do you like the approach
that we pass 'address' to gdbarch significant_addr_bits, and teach
ppc32 gdbarch significant_addr_bits be aware of SPU address, that is,
return 64 if the top bit is one (this address is the SPU address),
otherwise, return 32 (this address is normal PPU address).
  

Patch

diff --git a/gdb/aarch64-linux-tdep.c b/gdb/aarch64-linux-tdep.c
index c2c2c30..b6c2a81 100644
--- a/gdb/aarch64-linux-tdep.c
+++ b/gdb/aarch64-linux-tdep.c
@@ -1217,6 +1217,8 @@  aarch64_linux_init_abi (struct gdbarch_info info, struct gdbarch *gdbarch)
   set_xml_syscall_file_name (gdbarch, "syscalls/aarch64-linux.xml");
   set_gdbarch_get_syscall_number (gdbarch, aarch64_linux_get_syscall_number);
 
+  set_gdbarch_significant_addr_bit (gdbarch, 56);
+
   /* Displaced stepping.  */
   set_gdbarch_max_insn_length (gdbarch, 4 * DISPLACED_MODIFIED_INSNS);
   set_gdbarch_displaced_step_copy_insn (gdbarch,
diff --git a/gdb/gdbarch.c b/gdb/gdbarch.c
index 007392c..8177f05 100644
--- a/gdb/gdbarch.c
+++ b/gdb/gdbarch.c
@@ -259,6 +259,7 @@  struct gdbarch
   int frame_red_zone_size;
   gdbarch_convert_from_func_ptr_addr_ftype *convert_from_func_ptr_addr;
   gdbarch_addr_bits_remove_ftype *addr_bits_remove;
+  int significant_addr_bit;
   gdbarch_software_single_step_ftype *software_single_step;
   gdbarch_single_step_through_delay_ftype *single_step_through_delay;
   gdbarch_print_insn_ftype *print_insn;
@@ -618,6 +619,8 @@  verify_gdbarch (struct gdbarch *gdbarch)
   /* Skip verify of stabs_argument_has_addr, invalid_p == 0 */
   /* Skip verify of convert_from_func_ptr_addr, invalid_p == 0 */
   /* Skip verify of addr_bits_remove, invalid_p == 0 */
+  if (gdbarch->significant_addr_bit == 0)
+    gdbarch->significant_addr_bit = gdbarch_addr_bit (gdbarch);
   /* Skip verify of software_single_step, has predicate.  */
   /* Skip verify of single_step_through_delay, has predicate.  */
   /* Skip verify of print_insn, invalid_p == 0 */
@@ -1325,6 +1328,9 @@  gdbarch_dump (struct gdbarch *gdbarch, struct ui_file *file)
                       "gdbarch_dump: short_bit = %s\n",
                       plongest (gdbarch->short_bit));
   fprintf_unfiltered (file,
+                      "gdbarch_dump: significant_addr_bit = %s\n",
+                      plongest (gdbarch->significant_addr_bit));
+  fprintf_unfiltered (file,
                       "gdbarch_dump: gdbarch_single_step_through_delay_p() = %d\n",
                       gdbarch_single_step_through_delay_p (gdbarch));
   fprintf_unfiltered (file,
@@ -3216,6 +3222,22 @@  set_gdbarch_addr_bits_remove (struct gdbarch *gdbarch,
 }
 
 int
+gdbarch_significant_addr_bit (struct gdbarch *gdbarch)
+{
+  gdb_assert (gdbarch != NULL);
+  if (gdbarch_debug >= 2)
+    fprintf_unfiltered (gdb_stdlog, "gdbarch_significant_addr_bit called\n");
+  return gdbarch->significant_addr_bit;
+}
+
+void
+set_gdbarch_significant_addr_bit (struct gdbarch *gdbarch,
+                                  int significant_addr_bit)
+{
+  gdbarch->significant_addr_bit = significant_addr_bit;
+}
+
+int
 gdbarch_software_single_step_p (struct gdbarch *gdbarch)
 {
   gdb_assert (gdbarch != NULL);
diff --git a/gdb/gdbarch.h b/gdb/gdbarch.h
index d2e6b6f..1a654b6 100644
--- a/gdb/gdbarch.h
+++ b/gdb/gdbarch.h
@@ -677,6 +677,14 @@  typedef CORE_ADDR (gdbarch_addr_bits_remove_ftype) (struct gdbarch *gdbarch, COR
 extern CORE_ADDR gdbarch_addr_bits_remove (struct gdbarch *gdbarch, CORE_ADDR addr);
 extern void set_gdbarch_addr_bits_remove (struct gdbarch *gdbarch, gdbarch_addr_bits_remove_ftype *addr_bits_remove);
 
+/* On some machines, not all bits of an address word are significant.
+   For example, on AArch64, the top bits of an address known as the "tag"
+   are ignored by the kernel, the hardware, etc. and can be regarded as
+   additional data associated with the address. */
+
+extern int gdbarch_significant_addr_bit (struct gdbarch *gdbarch);
+extern void set_gdbarch_significant_addr_bit (struct gdbarch *gdbarch, int significant_addr_bit);
+
 /* FIXME/cagney/2001-01-18: This should be split in two.  A target method that
    indicates if the target needs software single step.  An ISA method to
    implement it.
diff --git a/gdb/gdbarch.sh b/gdb/gdbarch.sh
index 6459b12..1f165cf 100755
--- a/gdb/gdbarch.sh
+++ b/gdb/gdbarch.sh
@@ -621,6 +621,12 @@  m;CORE_ADDR;convert_from_func_ptr_addr;CORE_ADDR addr, struct target_ops *targ;a
 # possible it should be in TARGET_READ_PC instead).
 m;CORE_ADDR;addr_bits_remove;CORE_ADDR addr;addr;;core_addr_identity;;0
 
+# On some machines, not all bits of an address word are significant.
+# For example, on AArch64, the top bits of an address known as the "tag"
+# are ignored by the kernel, the hardware, etc. and can be regarded as
+# additional data associated with the address.
+v;int;significant_addr_bit;;;;;gdbarch_addr_bit (gdbarch);
+
 # FIXME/cagney/2001-01-18: This should be split in two.  A target method that
 # indicates if the target needs software single step.  An ISA method to
 # implement it.
diff --git a/gdb/target.c b/gdb/target.c
index 3b8b8ea..767a2ad 100644
--- a/gdb/target.c
+++ b/gdb/target.c
@@ -1214,6 +1214,8 @@  memory_xfer_partial (struct target_ops *ops, enum target_object object,
   if (len == 0)
     return TARGET_XFER_EOF;
 
+  memaddr = address_significant (target_gdbarch (), memaddr);
+
   /* Fill in READBUF with breakpoint shadows, or WRITEBUF with
      breakpoint insns, thus hiding out from higher layers whether
      there are software breakpoints inserted in the code stream.  */
diff --git a/gdb/testsuite/gdb.arch/aarch64-tagged-pointer.c b/gdb/testsuite/gdb.arch/aarch64-tagged-pointer.c
new file mode 100644
index 0000000..7c90132
--- /dev/null
+++ b/gdb/testsuite/gdb.arch/aarch64-tagged-pointer.c
@@ -0,0 +1,48 @@ 
+/* This file is part of GDB, the GNU debugger.
+
+   Copyright 2017 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/>.  */
+
+#include <stdint.h>
+
+struct s
+{
+  int i;
+};
+
+static void
+foo (void)
+{
+}
+
+int
+main (void)
+{
+  struct s s1;
+  struct s *sp1, *sp2;
+  int i = 1234;
+  int *p1, *p2;
+
+  s1.i = 1234;
+  sp1 = &s1;
+  p1 = &i;
+  /* SP1 and SP2 have different tags, but point to the same address.  */
+  sp2 = (struct s *) ((uintptr_t) sp1 | 0xf000000000000000ULL);
+  p2 = (int *) ((uintptr_t) p1 | 0xf000000000000000ULL);
+
+  void (*func_ptr) (void) = foo;
+  func_ptr = (void (*) (void)) ((uintptr_t) func_ptr | 0xf000000000000000ULL);
+  sp2->i = 4321; /* breakpoint here.  */
+}
diff --git a/gdb/testsuite/gdb.arch/aarch64-tagged-pointer.exp b/gdb/testsuite/gdb.arch/aarch64-tagged-pointer.exp
new file mode 100644
index 0000000..4f2b44c
--- /dev/null
+++ b/gdb/testsuite/gdb.arch/aarch64-tagged-pointer.exp
@@ -0,0 +1,67 @@ 
+# Copyright 2017 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.
+
+if {![is_aarch64_target]} {
+    verbose "Skipping ${gdb_test_file_name}."
+    return
+}
+
+standard_testfile
+if { [prepare_for_testing "failed to prepare" ${testfile} ${srcfile}] } {
+    return -1
+}
+
+if ![runto_main] {
+    untested "could not run to main"
+    return -1
+}
+
+gdb_breakpoint [gdb_get_line_number "breakpoint here"]
+gdb_continue_to_breakpoint "breakpoint here"
+
+# Test that GDB manages caches correctly for tagged address.
+# Read from P2,
+gdb_test "x p2" "$hex:\[\t \]+0x000004d2"
+gdb_test_no_output "set variable i = 5678"
+# Test that *P2 is updated.
+gdb_test "x p2" "$hex:\[\t \]+0x0000162e"
+
+# Read from SP1->i,
+gdb_test "print sp1->i" " = 1234"
+# Write to SP2->i,
+gdb_test_no_output "set variable sp2->i = 5678"
+# Test that SP1->i is updated.
+gdb_test "print sp1->i" " = 5678"
+
+gdb_test "x/d &sp2->i" "$hex:\[\t \]+5678"
+gdb_test "x/d &sp1->i" "$hex:\[\t \]+5678"
+
+# Test that the same disassembly is got when disassembling function vs
+# tagged function pointer.
+set insn1 ""
+set insn2 ""
+set test "disassemble foo,+8"
+gdb_test_multiple $test $test {
+    -re ":\[\t \]+(\[a-z\]*)\[ \r\n\]+.*:\[\t \]+(\[a-z\]*).*$gdb_prompt $" {
+	set insn1 $expect_out(1,string)
+	set insn2 $expect_out(2,string)
+	pass $test
+    }
+}
+
+gdb_test "disassemble func_ptr,+8" \
+    ":\[\t \]+$insn1\[ \r\n\]+.*:\[\t \]+$insn2.*"
diff --git a/gdb/utils.c b/gdb/utils.c
index b95dcfd..2f8f06f 100644
--- a/gdb/utils.c
+++ b/gdb/utils.c
@@ -2724,6 +2724,23 @@  When set, debugging messages will be marked with seconds and microseconds."),
 			   &setdebuglist, &showdebuglist);
 }
 
+/* See utils.h.  */
+
+CORE_ADDR
+address_significant (gdbarch *gdbarch, CORE_ADDR addr)
+{
+  /* Truncate address to the significant bits of a target address,
+     avoiding shifts larger or equal than the width of a CORE_ADDR.
+     The local variable ADDR_BIT stops the compiler reporting a shift
+     overflow when it won't occur.  */
+  int addr_bit = gdbarch_significant_addr_bit (gdbarch);
+
+  if (addr_bit < (sizeof (CORE_ADDR) * HOST_CHAR_BIT))
+    addr &= ((CORE_ADDR) 1 << addr_bit) - 1;
+
+  return addr;
+}
+
 const char *
 paddress (struct gdbarch *gdbarch, CORE_ADDR addr)
 {
diff --git a/gdb/utils.h b/gdb/utils.h
index 349ab93..b2e3d57 100644
--- a/gdb/utils.h
+++ b/gdb/utils.h
@@ -438,6 +438,9 @@  extern void gdb_print_host_address_1 (const void *addr, struct ui_file *stream);
 #define gdb_print_host_address(ADDR, STREAM) \
   gdb_print_host_address_1 ((const void *) ADDR, STREAM)
 
+/* Return the address only having significant bits.  */
+extern CORE_ADDR address_significant (gdbarch *gdbarch, CORE_ADDR addr);
+
 /* Convert CORE_ADDR to string in platform-specific manner.
    This is usually formatted similar to 0x%lx.  */
 extern const char *paddress (struct gdbarch *gdbarch, CORE_ADDR addr);