* [PATCH 2/3] [AArch64] Adjust breakpoint on tagged address
2017-10-26 8:29 [PATCH 0/3 v2] [AArch64] Support tagged pointer Yao Qi
@ 2017-10-26 8:30 ` Yao Qi
2017-10-26 8:30 ` [PATCH 3/3] [AArch64] Remove tag from address for watchpoint Yao Qi
2017-10-26 8:30 ` [PATCH 1/3] [AArch64 Linux] Get rid of top byte from tagged address on memory access Yao Qi
2 siblings, 0 replies; 10+ messages in thread
From: Yao Qi @ 2017-10-26 8:30 UTC (permalink / raw)
To: gdb-patches
Nowadays, GDB can't set breakpoint on a tagged address, because kernel
requires tag bits should be removed, and reject the request,
(gdb) hbreak *func_ptr
gdb.execute_unwinders function is missing:
Hardware assisted breakpoint 2 at 0xf0000000004006f0
(gdb) c
Continuing.
main () at binutils-gdb/gdb/testsuite/gdb.arch/aarch64-tagged-pointer.c:47
47 sp2->i = 4321; /* breakpoint here. */
Unexpected error setting hardware debug registers
With this patch, when user sets a breakpoint on tagged address, GDB will
drop the top byte of address, and put breakpoint at the new place, as
shown below,
(gdb) hbreak *func_ptr
warning: Breakpoint address adjusted from 0xf000000000400690 to 0x00400690.
Hardware assisted breakpoint 2 at 0x400690
(gdb) break *func_ptr
warning: Breakpoint address adjusted from 0xf000000000400690 to 0x00400690.
Breakpoint 3 at 0x400690
When program hits a breakpoint, the stopped pc reported by Linux kernle is
the address *without* tag, so it is better the address recorded in
breakpoint location is the one without tag too, so we can still match
breakpoint location address and stopped pc reported by Linux kernel, by
simple compare. I did try the different approach, that is keep the tag in
the address saved in the breakpoint location, but I need to change many
places of "loc->address == pc", so I give up on this way.
gdb:
2017-10-24 Yao Qi <yao.qi@linaro.org>
* aarch64-tdep.c (aarch64_adjust_breakpoint_address): New
function.
(aarch64_gdbarch_init): Install adjust_breakpoint_address.
gdb/testsuite:
2017-10-24 Yao Qi <yao.qi@linaro.org>
* gdb.arch/aarch64-tagged-pointer.c (main): Update.
* gdb.arch/aarch64-tagged-pointer.exp: Add test for breakpoint.
---
gdb/aarch64-tdep.c | 10 ++++++++++
gdb/testsuite/gdb.arch/aarch64-tagged-pointer.c | 9 +++++++++
gdb/testsuite/gdb.arch/aarch64-tagged-pointer.exp | 24 +++++++++++++++++++++++
3 files changed, 43 insertions(+)
diff --git a/gdb/aarch64-tdep.c b/gdb/aarch64-tdep.c
index 85cabfd..42c14c2 100644
--- a/gdb/aarch64-tdep.c
+++ b/gdb/aarch64-tdep.c
@@ -2828,6 +2828,14 @@ aarch64_displaced_step_hw_singlestep (struct gdbarch *gdbarch,
return 1;
}
+/* Implement the "adjust_breakpoint_address" gdbarch method. */
+
+static CORE_ADDR
+aarch64_adjust_breakpoint_address (struct gdbarch *gdbarch, CORE_ADDR bpaddr)
+{
+ return bpaddr & 0x0fffffffffffffffULL;
+}
+
/* Initialize the current architecture based on INFO. If possible,
re-use an architecture from ARCHES, which is a list of
architectures already created during this debugging session.
@@ -2945,6 +2953,8 @@ aarch64_gdbarch_init (struct gdbarch_info info, struct gdbarch_list *arches)
aarch64_breakpoint::bp_from_kind);
set_gdbarch_have_nonsteppable_watchpoint (gdbarch, 1);
set_gdbarch_software_single_step (gdbarch, aarch64_software_single_step);
+ set_gdbarch_adjust_breakpoint_address (gdbarch,
+ aarch64_adjust_breakpoint_address);
/* Information about registers, etc. */
set_gdbarch_sp_regnum (gdbarch, AARCH64_SP_REGNUM);
diff --git a/gdb/testsuite/gdb.arch/aarch64-tagged-pointer.c b/gdb/testsuite/gdb.arch/aarch64-tagged-pointer.c
index 7c90132..ec3574d 100644
--- a/gdb/testsuite/gdb.arch/aarch64-tagged-pointer.c
+++ b/gdb/testsuite/gdb.arch/aarch64-tagged-pointer.c
@@ -45,4 +45,13 @@ main (void)
void (*func_ptr) (void) = foo;
func_ptr = (void (*) (void)) ((uintptr_t) func_ptr | 0xf000000000000000ULL);
sp2->i = 4321; /* breakpoint here. */
+
+ int j;
+ for (j = 0; j < 2; j++)
+ {
+ foo ();
+ (*func_ptr) ();
+ }
+
+ sp1->i = 8765;
}
diff --git a/gdb/testsuite/gdb.arch/aarch64-tagged-pointer.exp b/gdb/testsuite/gdb.arch/aarch64-tagged-pointer.exp
index 4d36d09..2a5b1eb 100644
--- a/gdb/testsuite/gdb.arch/aarch64-tagged-pointer.exp
+++ b/gdb/testsuite/gdb.arch/aarch64-tagged-pointer.exp
@@ -66,3 +66,27 @@ gdb_test_multiple $test $test {
gdb_test "disassemble func_ptr,+8" \
":\[\t \]+$insn1\[ \r\n\]+.*:\[\t \]+$insn2.*"
+
+foreach_with_prefix bptype {"hbreak" "break"} {
+
+ # Set a breakpoint on a tagged address, func_ptr,
+ gdb_test "$bptype *func_ptr" \
+ "warning: Breakpoint address adjusted from .*reakpoint $decimal at .*" \
+ "breakpoint at *func_ptr"
+ # Resume the program and expect it hits foo,
+ gdb_test "continue" \
+ "Continuing\\..*Breakpoint \[0-9\]+, foo \\(\\) at .*" \
+ "run until breakpoint set *func_ptr"
+ gdb_test "up" "foo \\(\\).*" "caller is foo"
+ delete_breakpoints
+
+ # Set a breakpoint on normal function, call it through tagged
+ # function pointer.
+ gdb_test "$bptype foo" "reakpoint $decimal at .*" \
+ "hardware breakpoint at foo"
+ gdb_test "continue" \
+ "Continuing\\..*Breakpoint \[0-9\]+, foo \\(\\) at .*" \
+ "run until breakpoint set foo"
+ gdb_test "up" "\\(\*func_ptr\\) \\(\\).*" "caller is *func_ptr"
+ delete_breakpoints
+}
--
1.9.1
^ permalink raw reply [flat|nested] 10+ messages in thread* [PATCH 3/3] [AArch64] Remove tag from address for watchpoint
2017-10-26 8:29 [PATCH 0/3 v2] [AArch64] Support tagged pointer Yao Qi
2017-10-26 8:30 ` [PATCH 2/3] [AArch64] Adjust breakpoint on tagged address Yao Qi
@ 2017-10-26 8:30 ` Yao Qi
2017-11-09 20:30 ` Pedro Alves
2017-10-26 8:30 ` [PATCH 1/3] [AArch64 Linux] Get rid of top byte from tagged address on memory access Yao Qi
2 siblings, 1 reply; 10+ messages in thread
From: Yao Qi @ 2017-10-26 8:30 UTC (permalink / raw)
To: gdb-patches
Nowadays, GDB can't set watchpoint on tagged address,
(gdb) p p2
$1 = (int *) 0xf000fffffffff474
(gdb) watch *((int *) 0xf000fffffffff474)
Hardware watchpoint 2: *((int *) 0xf000fffffffff474)
(gdb) c
Continuing.
main () at
binutils-gdb/gdb/testsuite/gdb.arch/aarch64-tagged-pointer.c:45
45 void (*func_ptr) (void) = foo;
Unexpected error setting hardware debug registers
This patch is about setting watchpoint on a tagged address. Unlike
breakpoint, watchpoint record the expression rather than the address, and
when a watchpoint is fired, GDB checks the expression value changed
instead of matching address, so we can mask the watchpoint address for
aarch64, by adding a gdbarch method addr_tag_remove.
gdb:
2017-10-25 Yao Qi <yao.qi@linaro.org>
* aarch64-linux-tdep.c (aarch64_linux_addr_tag_remove): New method.
(aarch64_linux_init_abi): Install gdbarch method.
* gdbarch.sh (addr_tag_remove): New gdbarch method.
* gdbarch.c: Re-generated.
* gdbarch.h: Re-generated.
* breakpoint.c (update_watchpoint):
gdb/testsuite:
2017-10-25 Yao Qi <yao.qi@linaro.org>
* gdb.arch/aarch64-tagged-pointer.exp: Add tests for watchpoint.
---
gdb/aarch64-linux-tdep.c | 11 +++++++++++
gdb/breakpoint.c | 3 +--
gdb/gdbarch.c | 23 +++++++++++++++++++++++
gdb/gdbarch.h | 8 ++++++++
gdb/gdbarch.sh | 5 +++++
gdb/testsuite/gdb.arch/aarch64-tagged-pointer.c | 1 +
gdb/testsuite/gdb.arch/aarch64-tagged-pointer.exp | 14 ++++++++++++++
7 files changed, 63 insertions(+), 2 deletions(-)
diff --git a/gdb/aarch64-linux-tdep.c b/gdb/aarch64-linux-tdep.c
index b6052ba..139f336 100644
--- a/gdb/aarch64-linux-tdep.c
+++ b/gdb/aarch64-linux-tdep.c
@@ -1001,6 +1001,15 @@ aarch64_linux_syscall_record (struct regcache *regcache,
return 0;
}
+/* Implement the "addr_tag_remove" gdbarch method. */
+
+static CORE_ADDR
+aarch64_linux_addr_tag_remove (struct gdbarch *gdbarch, CORE_ADDR val)
+{
+ /* Ignore the top byte of virtual address. */
+ return val & 0x0fffffffffffffffULL;
+}
+
static void
aarch64_linux_init_abi (struct gdbarch_info info, struct gdbarch *gdbarch)
{
@@ -1217,6 +1226,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_addr_tag_remove (gdbarch, aarch64_linux_addr_tag_remove);
+
/* Displaced stepping. */
set_gdbarch_max_insn_length (gdbarch, 4 * DISPLACED_MODIFIED_INSNS);
set_gdbarch_displaced_step_copy_insn (gdbarch,
diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
index e16cfb6..16f03ff 100644
--- a/gdb/breakpoint.c
+++ b/gdb/breakpoint.c
@@ -1894,8 +1894,7 @@ update_watchpoint (struct watchpoint *b, int reparse)
loc->gdbarch = get_type_arch (value_type (v));
loc->pspace = frame_pspace;
- loc->address = addr;
-
+ loc->address = gdbarch_addr_tag_remove (loc->gdbarch, addr);
if (bitsize != 0)
{
/* Just cover the bytes that make up the bitfield. */
diff --git a/gdb/gdbarch.c b/gdb/gdbarch.c
index 007392c..76574d4 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;
+ gdbarch_addr_tag_remove_ftype *addr_tag_remove;
gdbarch_software_single_step_ftype *software_single_step;
gdbarch_single_step_through_delay_ftype *single_step_through_delay;
gdbarch_print_insn_ftype *print_insn;
@@ -428,6 +429,7 @@ gdbarch_alloc (const struct gdbarch_info *info,
gdbarch->stabs_argument_has_addr = default_stabs_argument_has_addr;
gdbarch->convert_from_func_ptr_addr = convert_from_func_ptr_addr_identity;
gdbarch->addr_bits_remove = core_addr_identity;
+ gdbarch->addr_tag_remove = core_addr_identity;
gdbarch->print_insn = default_print_insn;
gdbarch->skip_trampoline_code = generic_skip_trampoline_code;
gdbarch->skip_solib_resolver = generic_skip_solib_resolver;
@@ -618,6 +620,7 @@ 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 */
+ /* Skip verify of addr_tag_remove, invalid_p == 0 */
/* 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 */
@@ -737,6 +740,9 @@ gdbarch_dump (struct gdbarch *gdbarch, struct ui_file *file)
"gdbarch_dump: addr_bits_remove = <%s>\n",
host_address_to_string (gdbarch->addr_bits_remove));
fprintf_unfiltered (file,
+ "gdbarch_dump: addr_tag_remove = <%s>\n",
+ host_address_to_string (gdbarch->addr_tag_remove));
+ fprintf_unfiltered (file,
"gdbarch_dump: gdbarch_address_class_name_to_type_flags_p() = %d\n",
gdbarch_address_class_name_to_type_flags_p (gdbarch));
fprintf_unfiltered (file,
@@ -3215,6 +3221,23 @@ set_gdbarch_addr_bits_remove (struct gdbarch *gdbarch,
gdbarch->addr_bits_remove = addr_bits_remove;
}
+CORE_ADDR
+gdbarch_addr_tag_remove (struct gdbarch *gdbarch, CORE_ADDR addr)
+{
+ gdb_assert (gdbarch != NULL);
+ gdb_assert (gdbarch->addr_tag_remove != NULL);
+ if (gdbarch_debug >= 2)
+ fprintf_unfiltered (gdb_stdlog, "gdbarch_addr_tag_remove called\n");
+ return gdbarch->addr_tag_remove (gdbarch, addr);
+}
+
+void
+set_gdbarch_addr_tag_remove (struct gdbarch *gdbarch,
+ gdbarch_addr_tag_remove_ftype addr_tag_remove)
+{
+ gdbarch->addr_tag_remove = addr_tag_remove;
+}
+
int
gdbarch_software_single_step_p (struct gdbarch *gdbarch)
{
diff --git a/gdb/gdbarch.h b/gdb/gdbarch.h
index d2e6b6f..769c8fb 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, there are bits in address which are ignored by the
+ kernel, the hardeware, etc. They are called "tag", which can be
+ regarded as additional data associated with the address. */
+
+typedef CORE_ADDR (gdbarch_addr_tag_remove_ftype) (struct gdbarch *gdbarch, CORE_ADDR addr);
+extern CORE_ADDR gdbarch_addr_tag_remove (struct gdbarch *gdbarch, CORE_ADDR addr);
+extern void set_gdbarch_addr_tag_remove (struct gdbarch *gdbarch, gdbarch_addr_tag_remove_ftype *addr_tag_remove);
+
/* 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..1f673e7 100755
--- a/gdb/gdbarch.sh
+++ b/gdb/gdbarch.sh
@@ -621,6 +621,11 @@ 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, there are bits in address which are ignored by the
+# kernel, the hardeware, etc. They are called "tag", which can be
+# regarded as additional data associated with the address.
+m;CORE_ADDR;addr_tag_remove;CORE_ADDR addr;addr;;core_addr_identity;;0
+
# 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/testsuite/gdb.arch/aarch64-tagged-pointer.c b/gdb/testsuite/gdb.arch/aarch64-tagged-pointer.c
index ec3574d..19395f5 100644
--- a/gdb/testsuite/gdb.arch/aarch64-tagged-pointer.c
+++ b/gdb/testsuite/gdb.arch/aarch64-tagged-pointer.c
@@ -54,4 +54,5 @@ main (void)
}
sp1->i = 8765;
+ i = 1;
}
diff --git a/gdb/testsuite/gdb.arch/aarch64-tagged-pointer.exp b/gdb/testsuite/gdb.arch/aarch64-tagged-pointer.exp
index 2a5b1eb..f38b876 100644
--- a/gdb/testsuite/gdb.arch/aarch64-tagged-pointer.exp
+++ b/gdb/testsuite/gdb.arch/aarch64-tagged-pointer.exp
@@ -90,3 +90,17 @@ foreach_with_prefix bptype {"hbreak" "break"} {
gdb_test "up" "\\(\*func_ptr\\) \\(\\).*" "caller is *func_ptr"
delete_breakpoints
}
+
+gdb_test "down"
+gdb_test "finish"
+# Watch on tagged pointer.
+gdb_test "watch *sp2"
+gdb_test "continue" \
+ "Continuing\\..*Hardware watchpoint \[0-9\]+.*" \
+ "run until watchpoint on s1"
+delete_breakpoints
+
+gdb_test "watch *p2"
+gdb_test "continue" \
+ "Continuing\\..*Hardware watchpoint \[0-9\]+.*" \
+ "run until watchpoint on i"
--
1.9.1
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [PATCH 3/3] [AArch64] Remove tag from address for watchpoint
2017-10-26 8:30 ` [PATCH 3/3] [AArch64] Remove tag from address for watchpoint Yao Qi
@ 2017-11-09 20:30 ` Pedro Alves
2017-11-09 21:08 ` Simon Marchi
2017-11-09 21:09 ` Yao Qi
0 siblings, 2 replies; 10+ messages in thread
From: Pedro Alves @ 2017-11-09 20:30 UTC (permalink / raw)
To: Yao Qi, gdb-patches
On 10/26/2017 09:29 AM, Yao Qi wrote:
> +typedef CORE_ADDR (gdbarch_addr_tag_remove_ftype) (struct gdbarch *gdbarch, CORE_ADDR addr);
> +extern CORE_ADDR gdbarch_addr_tag_remove (struct gdbarch *gdbarch, CORE_ADDR addr);
> +extern void set_gdbarch_addr_tag_remove (struct gdbarch *gdbarch, gdbarch_addr_tag_remove_ftype *addr_tag_remove);
> +
> /* 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..1f673e7 100755
> --- a/gdb/gdbarch.sh
> +++ b/gdb/gdbarch.sh
> @@ -621,6 +621,11 @@ 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, there are bits in address which are ignored by the
> +# kernel, the hardeware, etc. They are called "tag", which can be
> +# regarded as additional data associated with the address.
> +m;CORE_ADDR;addr_tag_remove;CORE_ADDR addr;addr;;core_addr_identity;;0
typo: "hardeware".
Hmmm. We have gdbarch_addr_bit / addr_bit to represent the size
of a target address. I'm thinking that instead of addr_tag_remove,
this would a bit more in line with the current scheme if this were
a new "significant_addr_bit" gdbarch property? I.e.:
/* 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. */
int gdbarch_significant_addr_bit (struct gdbarch *gdbarch);
significant_addr_bit would default to addr_bit.
And then at places where we need to save or compare memory addresses,
like in the watchpoint location addresses case we strip out / ignore
non-significant bits.
And the next question is: if you're adding a gdbarch hook such as
this one (either significant_addr_bit or addr_tag_remove)
why not use it for all the cases handled by the different patches in
this series, instead of using different solutions for each case?
I.e., for memory access, saving breakpoint and watchpoint
location addresses, the dcache, and any other future case we run
into, like e.g., maybe agent expressions.
Thanks,
Pedro Alves
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [PATCH 3/3] [AArch64] Remove tag from address for watchpoint
2017-11-09 20:30 ` Pedro Alves
@ 2017-11-09 21:08 ` Simon Marchi
2017-11-09 21:20 ` Yao Qi
2017-11-09 21:09 ` Yao Qi
1 sibling, 1 reply; 10+ messages in thread
From: Simon Marchi @ 2017-11-09 21:08 UTC (permalink / raw)
To: Pedro Alves; +Cc: Yao Qi, gdb-patches
On 2017-11-09 15:30, Pedro Alves wrote:
> On 10/26/2017 09:29 AM, Yao Qi wrote:
>
>> +typedef CORE_ADDR (gdbarch_addr_tag_remove_ftype) (struct gdbarch
>> *gdbarch, CORE_ADDR addr);
>> +extern CORE_ADDR gdbarch_addr_tag_remove (struct gdbarch *gdbarch,
>> CORE_ADDR addr);
>> +extern void set_gdbarch_addr_tag_remove (struct gdbarch *gdbarch,
>> gdbarch_addr_tag_remove_ftype *addr_tag_remove);
>> +
>> /* 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..1f673e7 100755
>> --- a/gdb/gdbarch.sh
>> +++ b/gdb/gdbarch.sh
>> @@ -621,6 +621,11 @@ 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, there are bits in address which are ignored by
>> the
>> +# kernel, the hardeware, etc. They are called "tag", which can be
>> +# regarded as additional data associated with the address.
>> +m;CORE_ADDR;addr_tag_remove;CORE_ADDR
>> addr;addr;;core_addr_identity;;0
>
> typo: "hardeware".
>
> Hmmm. We have gdbarch_addr_bit / addr_bit to represent the size
> of a target address. I'm thinking that instead of addr_tag_remove,
> this would a bit more in line with the current scheme if this were
> a new "significant_addr_bit" gdbarch property? I.e.:
>
> /* 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. */
> int gdbarch_significant_addr_bit (struct gdbarch *gdbarch);
>
> significant_addr_bit would default to addr_bit.
>
> And then at places where we need to save or compare memory addresses,
> like in the watchpoint location addresses case we strip out / ignore
> non-significant bits.
>
> And the next question is: if you're adding a gdbarch hook such as
> this one (either significant_addr_bit or addr_tag_remove)
> why not use it for all the cases handled by the different patches in
> this series, instead of using different solutions for each case?
> I.e., for memory access, saving breakpoint and watchpoint
> location addresses, the dcache, and any other future case we run
> into, like e.g., maybe agent expressions.
>
> Thanks,
> Pedro Alves
There's gdbarch_addr_bits_remove already as well, I don't know if that
has the same goal.
Simon
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 3/3] [AArch64] Remove tag from address for watchpoint
2017-11-09 21:08 ` Simon Marchi
@ 2017-11-09 21:20 ` Yao Qi
0 siblings, 0 replies; 10+ messages in thread
From: Yao Qi @ 2017-11-09 21:20 UTC (permalink / raw)
To: Simon Marchi; +Cc: Pedro Alves, gdb-patches
On 17-11-09 16:08:40, Simon Marchi wrote:
>
> There's gdbarch_addr_bits_remove already as well, I don't know if that has
> the same goal.
>
Nowadays, it only used for code,
# On some machines there are bits in addresses which are not really
# part of the address, but are used by the kernel, the hardware, etc.
# for special purposes. gdbarch_addr_bits_remove takes out any such bits so
# we get a "real" address such as one would find in a symbol table.
# This is used only for addresses of instructions, and even then I'm
# not sure it's used in all contexts. It exists to deal with there
# being a few stray bits in the PC which would mislead us, not as some
# sort of generic thing to handle alignment or segmentation (it's
# possible it should be in TARGET_READ_PC instead).
m:CORE_ADDR:addr_bits_remove:CORE_ADDR addr:addr::core_addr_identity::0
arm and mips reuses the LSB of PC to indicate the execution state change,
arm <-> thumb. gdbarch_addr_bits_remove for arm and mips clears the LSB
of PC. However, this gdbarch method can't be used for data address.
armv8 tagged pointer/address can be used for both code and data address.
--
Yao (é½å°§)
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 3/3] [AArch64] Remove tag from address for watchpoint
2017-11-09 20:30 ` Pedro Alves
2017-11-09 21:08 ` Simon Marchi
@ 2017-11-09 21:09 ` Yao Qi
2017-11-09 22:25 ` Pedro Alves
1 sibling, 1 reply; 10+ messages in thread
From: Yao Qi @ 2017-11-09 21:09 UTC (permalink / raw)
To: Pedro Alves; +Cc: gdb-patches
On 17-11-09 20:30:42, Pedro Alves wrote:
> And then at places where we need to save or compare memory addresses,
> like in the watchpoint location addresses case we strip out / ignore
> non-significant bits.
>
> And the next question is: if you're adding a gdbarch hook such as
> this one (either significant_addr_bit or addr_tag_remove)
> why not use it for all the cases handled by the different patches in
> this series, instead of using different solutions for each case?
> I.e., for memory access, saving breakpoint and watchpoint
> location addresses, the dcache, and any other future case we run
> into, like e.g., maybe agent expressions.
I thought about this, using this new added gdbarch method everywhere.
The reason I didn't do that is due to breakpoint location address
comparison (I think I can use the new gdbarch method for the rest,
watchpoint location and dcache).
As I wrote in patch #2,
> When program hits a breakpoint, the stopped pc reported by Linux kernle is
> the address *without* tag, so it is better the address recorded in
> breakpoint location is the one without tag too, so we can still match
> breakpoint location address and stopped pc reported by Linux kernel, by
> simple compare. I did try the different approach, that is keep the tag in
> the address saved in the breakpoint location, but I need to change many
> places of "loc->address == pc", so I give up on this way.
I remember code pattern "loc->address == pc" exists many places in
breakpoint.c. I can give a try again.
--
Yao (é½å°§)
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 3/3] [AArch64] Remove tag from address for watchpoint
2017-11-09 21:09 ` Yao Qi
@ 2017-11-09 22:25 ` Pedro Alves
0 siblings, 0 replies; 10+ messages in thread
From: Pedro Alves @ 2017-11-09 22:25 UTC (permalink / raw)
To: Yao Qi; +Cc: gdb-patches
On 11/09/2017 09:09 PM, Yao Qi wrote:
> On 17-11-09 20:30:42, Pedro Alves wrote:
>> And then at places where we need to save or compare memory addresses,
>> like in the watchpoint location addresses case we strip out / ignore
>> non-significant bits.
>>
>> And the next question is: if you're adding a gdbarch hook such as
>> this one (either significant_addr_bit or addr_tag_remove)
>> why not use it for all the cases handled by the different patches in
>> this series, instead of using different solutions for each case?
>> I.e., for memory access, saving breakpoint and watchpoint
>> location addresses, the dcache, and any other future case we run
>> into, like e.g., maybe agent expressions.
>
> I thought about this, using this new added gdbarch method everywhere.
> The reason I didn't do that is due to breakpoint location address
> comparison (I think I can use the new gdbarch method for the rest,
> watchpoint location and dcache).
>
> As I wrote in patch #2,
>
>> When program hits a breakpoint, the stopped pc reported by Linux kernle is
>> the address *without* tag, so it is better the address recorded in
>> breakpoint location is the one without tag too, so we can still match
>> breakpoint location address and stopped pc reported by Linux kernel, by
>> simple compare. I did try the different approach, that is keep the tag in
>> the address saved in the breakpoint location, but I need to change many
>> places of "loc->address == pc", so I give up on this way.
>
> I remember code pattern "loc->address == pc" exists many places in
> breakpoint.c. I can give a try again.
Since target-reported addresses always have the non-significant bits
zeroed, you'd only need to use the new gdbarch hook in
breakpoint.c:adjust_breakpoint_address ? Seems like that's already
used for watchpoints too, even.
Thanks,
Pedro Alves
^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH 1/3] [AArch64 Linux] Get rid of top byte from tagged address on memory access
2017-10-26 8:29 [PATCH 0/3 v2] [AArch64] Support tagged pointer Yao Qi
2017-10-26 8:30 ` [PATCH 2/3] [AArch64] Adjust breakpoint on tagged address Yao Qi
2017-10-26 8:30 ` [PATCH 3/3] [AArch64] Remove tag from address for watchpoint Yao Qi
@ 2017-10-26 8:30 ` Yao Qi
2017-11-09 20:24 ` Pedro Alves
2 siblings, 1 reply; 10+ messages in thread
From: Yao Qi @ 2017-10-26 8:30 UTC (permalink / raw)
To: gdb-patches
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 patch clear the top byte of the virtual address, at the point before
GDB/GDBserver pass the address to /proc or ptrace syscall on memory access.
The top byte of address is still retained in the rest of GDB, because
these bits can be used by different applications in different ways.
That is reason I didn't implement gdbarch method addr_bits_remove to get
rid of them.
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
With the tagged address, the variables/memory can be access via different
addresses (or tags), but GDB uses cache for stack variable access and code
access to speed up remote debugging. Fortunately, tagged address and
GDB stack/code cache can coexist, because,
- 'x' command doesn't go through cache, so we don't have to worry,
- gdb only uses stack cache when it believes the variable is on stack,
int i;
int *p = &i;
when print 'i' or 'p', gdb uses stack caches, but when print '*p', gdb
only uses stack caches to get 'p', and get '*p' without cache, because
gdb doesn't know the address p points to is on stack or not.
- gdb uses code caches to access code, do disassembly for example, when
gdb does disassembly for a function (without tag) and a tagged function
pointer, gdb creates thinks they are different addresses, and creates
two different cache lines, but we only have cache when inferior stops,
and code caches are regarded read-only.
gdb:
2017-10-19 Yao Qi <yao.qi@linaro.org>
* aarch64-linux-nat.c (super_xfer_partial): New function pointer.
(aarch64_linux_xfer_partial): New function.
(_initialize_aarch64_linux_nat): Initialize super_xfer_partial,
and update to_xfer_partial.
gdb/gdbserver:
2017-10-19 Yao Qi <yao.qi@linaro.org>
* linux-aarch64-low.c (super_read_memory): New function pointer.
(super_write_memory): Likewise.
(aarch64_linux_read_memory): New function.
(aarch64_linux_write_memory): New function.
(initialize_low_arch): Initialize super_read_memory and
super_write_memory. Update read_memory and write_memory.
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-nat.c | 25 +++++++++
gdb/gdbserver/linux-aarch64-low.c | 33 +++++++++++
gdb/testsuite/gdb.arch/aarch64-tagged-pointer.c | 48 ++++++++++++++++
gdb/testsuite/gdb.arch/aarch64-tagged-pointer.exp | 68 +++++++++++++++++++++++
4 files changed, 174 insertions(+)
create mode 100644 gdb/testsuite/gdb.arch/aarch64-tagged-pointer.c
create mode 100644 gdb/testsuite/gdb.arch/aarch64-tagged-pointer.exp
diff --git a/gdb/aarch64-linux-nat.c b/gdb/aarch64-linux-nat.c
index de18edd..78c038b 100644
--- a/gdb/aarch64-linux-nat.c
+++ b/gdb/aarch64-linux-nat.c
@@ -780,6 +780,27 @@ aarch64_linux_can_do_single_step (struct target_ops *target)
return 1;
}
+static target_xfer_partial_ftype *super_xfer_partial;
+
+/* Implement the "to_xfer_partial" target_ops method. */
+
+static enum target_xfer_status
+aarch64_linux_xfer_partial (struct target_ops *ops, enum target_object object,
+ const char *annex, gdb_byte *readbuf,
+ const gdb_byte *writebuf, ULONGEST offset,
+ ULONGEST len, ULONGEST *xfered_len)
+{
+ if (object == TARGET_OBJECT_MEMORY)
+ {
+ /* ARMv8 supports tagged address, that is, the top one byte in
+ virtual address is ignored. */
+ offset &= 0x0fffffffffffffffULL;
+ }
+
+ return super_xfer_partial (ops, object, annex, readbuf, writebuf,
+ offset, len, xfered_len);
+}
+
/* Define AArch64 maintenance commands. */
static void
@@ -834,6 +855,10 @@ _initialize_aarch64_linux_nat (void)
super_post_startup_inferior = t->to_post_startup_inferior;
t->to_post_startup_inferior = aarch64_linux_child_post_startup_inferior;
+ /* Override the default to_xfer_partial. */
+ super_xfer_partial = t->to_xfer_partial;
+ t->to_xfer_partial = aarch64_linux_xfer_partial;
+
/* Register the target. */
linux_nat_add_target (t);
linux_nat_set_new_thread (t, aarch64_linux_new_thread);
diff --git a/gdb/gdbserver/linux-aarch64-low.c b/gdb/gdbserver/linux-aarch64-low.c
index b00d5c5..bc44092 100644
--- a/gdb/gdbserver/linux-aarch64-low.c
+++ b/gdb/gdbserver/linux-aarch64-low.c
@@ -3004,6 +3004,31 @@ struct linux_target_ops the_low_target =
aarch64_get_syscall_trapinfo,
};
+static int (*super_read_memory) (CORE_ADDR memaddr, unsigned char *myaddr,
+ int len);
+
+static int (*super_write_memory) (CORE_ADDR memaddr,
+ const unsigned char *myaddr, int len);
+
+static int
+aarch64_linux_read_memory (CORE_ADDR memaddr, unsigned char *myaddr, int len)
+{
+ /* ARMv8 supports tagged address, that is, the top one byte in
+ virtual address is ignored. */
+ memaddr &= 0x0fffffffffffffffULL;
+ return super_read_memory (memaddr, myaddr, len);
+}
+
+static int
+aarch64_linux_write_memory (CORE_ADDR memaddr, const unsigned char *myaddr,
+ int len)
+{
+ /* ARMv8 supports tagged address, that is, the top one byte in
+ virtual address is ignored. */
+ memaddr &= 0x0fffffffffffffffULL;
+ return super_write_memory (memaddr, myaddr, len);
+}
+
void
initialize_low_arch (void)
{
@@ -3012,4 +3037,12 @@ initialize_low_arch (void)
initialize_low_arch_aarch32 ();
initialize_regsets_info (&aarch64_regsets_info);
+
+ /* Override the default read_memory. */
+ super_read_memory = the_target->read_memory;
+ the_target->read_memory = aarch64_linux_read_memory;
+
+ /* Override the default write_memory. */
+ super_write_memory = the_target->write_memory;
+ the_target->write_memory = aarch64_linux_write_memory;
}
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..4d36d09
--- /dev/null
+++ b/gdb/testsuite/gdb.arch/aarch64-tagged-pointer.exp
@@ -0,0 +1,68 @@
+# 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, write to the Free Software
+# Foundation, Inc., 59 Temple Place - Suite 330, Boston, MA 02111-1307, USA.
+#
+# 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.*"
--
1.9.1
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [PATCH 1/3] [AArch64 Linux] Get rid of top byte from tagged address on memory access
2017-10-26 8:30 ` [PATCH 1/3] [AArch64 Linux] Get rid of top byte from tagged address on memory access Yao Qi
@ 2017-11-09 20:24 ` Pedro Alves
0 siblings, 0 replies; 10+ messages in thread
From: Pedro Alves @ 2017-11-09 20:24 UTC (permalink / raw)
To: Yao Qi, gdb-patches
Hi Yao,
On 10/26/2017 09:29 AM, Yao Qi wrote:
> 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 patch clear the top byte of the virtual address, at the point before
> GDB/GDBserver pass the address to /proc or ptrace syscall on memory access.
> The top byte of address is still retained in the rest of GDB, because
> these bits can be used by different applications in different ways.
> That is reason I didn't implement gdbarch method addr_bits_remove to get
> rid of them.
>
> 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
>
> With the tagged address, the variables/memory can be access via different
> addresses (or tags), but GDB uses cache for stack variable access and code
> access to speed up remote debugging. Fortunately, tagged address and
> GDB stack/code cache can coexist, because,
>
> - 'x' command doesn't go through cache, so we don't have to worry,
> - gdb only uses stack cache when it believes the variable is on stack,
>
> int i;
> int *p = &i;
>
> when print 'i' or 'p', gdb uses stack caches, but when print '*p', gdb
> only uses stack caches to get 'p', and get '*p' without cache, because
> gdb doesn't know the address p points to is on stack or not.
That sounds a bit fragile to me...
> - gdb uses code caches to access code, do disassembly for example, when
> gdb does disassembly for a function (without tag) and a tagged function
> pointer, gdb creates thinks they are different addresses, and creates
> two different cache lines, but we only have cache when inferior stops,
> and code caches are regarded read-only.
I don't understand the second point, the one about the code cache.
The tail end of raw_memory_xfer_partial writes through the code cache:
if (writebuf != NULL
&& !ptid_equal (inferior_ptid, null_ptid)
&& target_dcache_init_p ()
&& (stack_cache_enabled_p () || code_cache_enabled_p ()))
{
DCACHE *dcache = target_dcache_get ();
/* Note that writing to an area of memory which wasn't present
in the cache doesn't cause it to be loaded in. */
dcache_update (dcache, res, memaddr, writebuf, *xfered_len);
}
> +++ b/gdb/testsuite/gdb.arch/aarch64-tagged-pointer.exp
> @@ -0,0 +1,68 @@
> +# 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, write to the Free Software
> +# Foundation, Inc., 59 Temple Place - Suite 330, Boston, MA 02111-1307, USA.
We shouldn't be using this old snail mail address header anymore.
If we still have any, they should be converted to the newer one
with the URL instead.
Thanks,
Pedro Alves
^ permalink raw reply [flat|nested] 10+ messages in thread