* [PATCH 0/3 v3] [AArch64] Support tagged pointer @ 2017-12-08 10:04 Yao Qi 2017-12-08 10:04 ` [PATCH 2/3] Adjust breakpoint address by clearing non-significant bits Yao Qi ` (4 more replies) 0 siblings, 5 replies; 40+ messages in thread From: Yao Qi @ 2017-12-08 10:04 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 Some parts of GDB related to address are updated in this patch series, - Memory access, like command 'x', - Setting hw breakpoint on some address, - Setting watchpoint on some address, Address tag is treated as non-significant bits of address, so this patch series add a new gdbarch significant_addr_bit, and use it in gdbarch to get rid of non-significant bits. This was the suggestion in the v2 review. (https://sourceware.org/ml/gdb-patches/2017-10/msg00792.html) When I test this patch series, and I find a regression in linespec. I posted the fix https://sourceware.org/ml/gdb-patches/2017-12/msg00158.html Without this fix, this series causes a regression. *** BLURB HERE *** Yao Qi (3): Clear non-significant bits of address on memory access Adjust breakpoint address by clearing non-significant bits Clear non-significant bits of address in watchpoint gdb/aarch64-linux-tdep.c | 2 + gdb/breakpoint.c | 22 ++--- gdb/gdbarch.c | 22 +++++ gdb/gdbarch.h | 8 ++ gdb/gdbarch.sh | 6 ++ gdb/target.c | 2 + gdb/testsuite/gdb.arch/aarch64-tagged-pointer.c | 57 ++++++++++++ gdb/testsuite/gdb.arch/aarch64-tagged-pointer.exp | 105 ++++++++++++++++++++++ gdb/utils.c | 17 ++++ gdb/utils.h | 3 + 10 files changed, 233 insertions(+), 11 deletions(-) create mode 100644 gdb/testsuite/gdb.arch/aarch64-tagged-pointer.c create mode 100644 gdb/testsuite/gdb.arch/aarch64-tagged-pointer.exp -- 1.9.1 ^ permalink raw reply [flat|nested] 40+ messages in thread
* [PATCH 2/3] Adjust breakpoint address by clearing non-significant bits 2017-12-08 10:04 [PATCH 0/3 v3] [AArch64] Support tagged pointer Yao Qi @ 2017-12-08 10:04 ` Yao Qi 2017-12-08 12:22 ` Pedro Alves 2017-12-08 10:04 ` [PATCH 1/3] Clear non-significant bits of address on memory access Yao Qi ` (3 subsequent siblings) 4 siblings, 1 reply; 40+ messages in thread From: Yao Qi @ 2017-12-08 10:04 UTC (permalink / raw) To: gdb-patches Tag in tagged address on AArch64 is treated as a non-significant bits of address, which can be got by gdbarch method significant_addr_bit, and gdb can clear these bits. With this patch, when user sets a breakpoint on tagged address on AArch64, 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 kernel 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. gdb: 2017-10-24 Yao Qi <yao.qi@linaro.org> * breakpoint.c (adjust_breakpoint_address): Call address_significant. 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/breakpoint.c | 20 +++++++++---------- gdb/testsuite/gdb.arch/aarch64-tagged-pointer.c | 8 ++++++++ gdb/testsuite/gdb.arch/aarch64-tagged-pointer.exp | 24 +++++++++++++++++++++++ 3 files changed, 42 insertions(+), 10 deletions(-) diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c index 76bfd53..22b3069 100644 --- a/gdb/breakpoint.c +++ b/gdb/breakpoint.c @@ -6987,12 +6987,7 @@ static CORE_ADDR adjust_breakpoint_address (struct gdbarch *gdbarch, CORE_ADDR bpaddr, enum bptype bptype) { - if (!gdbarch_adjust_breakpoint_address_p (gdbarch)) - { - /* Very few targets need any kind of breakpoint adjustment. */ - return bpaddr; - } - else if (bptype == bp_watchpoint + if (bptype == bp_watchpoint || bptype == bp_hardware_watchpoint || bptype == bp_read_watchpoint || bptype == bp_access_watchpoint @@ -7014,11 +7009,16 @@ adjust_breakpoint_address (struct gdbarch *gdbarch, } else { - CORE_ADDR adjusted_bpaddr; + CORE_ADDR adjusted_bpaddr = bpaddr; + + if (gdbarch_adjust_breakpoint_address_p (gdbarch)) + { + /* Some targets have architectural constraints on the placement + of breakpoint instructions. Obtain the adjusted address. */ + adjusted_bpaddr = gdbarch_adjust_breakpoint_address (gdbarch, bpaddr); + } - /* Some targets have architectural constraints on the placement - of breakpoint instructions. Obtain the adjusted address. */ - adjusted_bpaddr = gdbarch_adjust_breakpoint_address (gdbarch, bpaddr); + adjusted_bpaddr = address_significant (gdbarch, adjusted_bpaddr); /* An adjusted breakpoint address can significantly alter a user's expectations. Print a warning if an adjustment diff --git a/gdb/testsuite/gdb.arch/aarch64-tagged-pointer.c b/gdb/testsuite/gdb.arch/aarch64-tagged-pointer.c index 7c90132..9bfe41e 100644 --- a/gdb/testsuite/gdb.arch/aarch64-tagged-pointer.c +++ b/gdb/testsuite/gdb.arch/aarch64-tagged-pointer.c @@ -45,4 +45,12 @@ main (void) void (*func_ptr) (void) = foo; func_ptr = (void (*) (void)) ((uintptr_t) func_ptr | 0xf000000000000000ULL); sp2->i = 4321; /* breakpoint here. */ + + for (int i = 0; i < 2; i++) + { + 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 4f2b44c..fcab1b7 100644 --- a/gdb/testsuite/gdb.arch/aarch64-tagged-pointer.exp +++ b/gdb/testsuite/gdb.arch/aarch64-tagged-pointer.exp @@ -65,3 +65,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] 40+ messages in thread
* Re: [PATCH 2/3] Adjust breakpoint address by clearing non-significant bits 2017-12-08 10:04 ` [PATCH 2/3] Adjust breakpoint address by clearing non-significant bits Yao Qi @ 2017-12-08 12:22 ` Pedro Alves 0 siblings, 0 replies; 40+ messages in thread From: Pedro Alves @ 2017-12-08 12:22 UTC (permalink / raw) To: Yao Qi, gdb-patches On 12/08/2017 10:04 AM, Yao Qi wrote: > When program hits a breakpoint, the stopped pc reported by Linux kernel 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. > Agreed. LGTM with formatting nit below. > 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/breakpoint.c | 20 +++++++++---------- > gdb/testsuite/gdb.arch/aarch64-tagged-pointer.c | 8 ++++++++ > gdb/testsuite/gdb.arch/aarch64-tagged-pointer.exp | 24 +++++++++++++++++++++++ > 3 files changed, 42 insertions(+), 10 deletions(-) > > diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c > index 76bfd53..22b3069 100644 > --- a/gdb/breakpoint.c > +++ b/gdb/breakpoint.c > @@ -6987,12 +6987,7 @@ static CORE_ADDR > adjust_breakpoint_address (struct gdbarch *gdbarch, > CORE_ADDR bpaddr, enum bptype bptype) > { > - if (!gdbarch_adjust_breakpoint_address_p (gdbarch)) > - { > - /* Very few targets need any kind of breakpoint adjustment. */ > - return bpaddr; > - } > - else if (bptype == bp_watchpoint > + if (bptype == bp_watchpoint > || bptype == bp_hardware_watchpoint > || bptype == bp_read_watchpoint > || bptype == bp_access_watchpoint Please indent the rest of the condition before pushing. OK with that fixed. > @@ -7014,11 +7009,16 @@ adjust_breakpoint_address (struct gdbarch *gdbarch, > } > else Thanks, Pedro Alves ^ permalink raw reply [flat|nested] 40+ messages in thread
* [PATCH 1/3] Clear non-significant bits of address on memory access 2017-12-08 10:04 [PATCH 0/3 v3] [AArch64] Support tagged pointer Yao Qi 2017-12-08 10:04 ` [PATCH 2/3] Adjust breakpoint address by clearing non-significant bits Yao Qi @ 2017-12-08 10:04 ` Yao Qi 2017-12-08 12:22 ` Pedro Alves 2017-12-19 13:50 ` Ulrich Weigand 2017-12-08 10:04 ` [PATCH 3/3] Clear non-significant bits of address in watchpoint Yao Qi ` (2 subsequent siblings) 4 siblings, 2 replies; 40+ messages in thread From: Yao Qi @ 2017-12-08 10:04 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 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 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); -- 1.9.1 ^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [PATCH 1/3] Clear non-significant bits of address on memory access 2017-12-08 10:04 ` [PATCH 1/3] Clear non-significant bits of address on memory access Yao Qi @ 2017-12-08 12:22 ` Pedro Alves 2017-12-08 15:13 ` Ulrich Weigand 2017-12-19 13:50 ` Ulrich Weigand 1 sibling, 1 reply; 40+ messages in thread From: Pedro Alves @ 2017-12-08 12:22 UTC (permalink / raw) To: Yao Qi, gdb-patches 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 ^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [PATCH 1/3] Clear non-significant bits of address on memory access 2017-12-08 12:22 ` Pedro Alves @ 2017-12-08 15:13 ` Ulrich Weigand 2017-12-08 15:36 ` Yao Qi 0 siblings, 1 reply; 40+ messages in thread From: Ulrich Weigand @ 2017-12-08 15:13 UTC (permalink / raw) To: Pedro Alves; +Cc: Yao Qi, gdb-patches Pedro Alves wrote: > 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. This seems to duplicate the functionality of gdbarch_addr_bits_remove to some extent ... Could those be merged back again? Bye, Ulrich -- Dr. Ulrich Weigand GNU/Linux compilers and toolchain Ulrich.Weigand@de.ibm.com ^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [PATCH 1/3] Clear non-significant bits of address on memory access 2017-12-08 15:13 ` Ulrich Weigand @ 2017-12-08 15:36 ` Yao Qi 0 siblings, 0 replies; 40+ messages in thread From: Yao Qi @ 2017-12-08 15:36 UTC (permalink / raw) To: Ulrich Weigand; +Cc: Pedro Alves, gdb-patches "Ulrich Weigand" <uweigand@de.ibm.com> writes: > This seems to duplicate the functionality of gdbarch_addr_bits_remove > to some extent ... Could those be merged back again? They are a little bit different, addr_bits_remove only applies to PC (for arm and mips), but significant_addr_bit applies to every address (both data and instruction). On the other hand, the LSB of address in arm and mips is still significant, but it has some different meaning other than address when it is the target address of branch. We only clear the LSB when it is the target address. In normal memory access, watchpoing setting, we don't clear it. Simon asked the same question, see https://sourceware.org/ml/gdb-patches/2017-11/msg00213.html These two gdbarch methods are quite similar, but difficult to merge these two, unless we bring more context in the gdbarch hook method. -- Yao (齐尧) ^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [PATCH 1/3] Clear non-significant bits of address on memory access 2017-12-08 10:04 ` [PATCH 1/3] Clear non-significant bits of address on memory access Yao Qi 2017-12-08 12:22 ` Pedro Alves @ 2017-12-19 13:50 ` Ulrich Weigand 2017-12-19 15:41 ` Yao Qi 1 sibling, 1 reply; 40+ messages in thread From: Ulrich Weigand @ 2017-12-19 13:50 UTC (permalink / raw) To: Yao Qi; +Cc: gdb-patches 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. 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 this call not be pushed down further in the target stack, to below the xfer_partial implementation in the spu-multiarch target? Bye, Ulrich -- Dr. Ulrich Weigand GNU/Linux compilers and toolchain Ulrich.Weigand@de.ibm.com ^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [PATCH 1/3] Clear non-significant bits of address on memory access 2017-12-19 13:50 ` Ulrich Weigand @ 2017-12-19 15:41 ` Yao Qi 2017-12-19 16:15 ` Ulrich Weigand 0 siblings, 1 reply; 40+ messages in thread From: Yao Qi @ 2017-12-19 15:41 UTC (permalink / raw) To: Ulrich Weigand; +Cc: GDB Patches 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). -- Yao (齐尧) ^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [PATCH 1/3] Clear non-significant bits of address on memory access 2017-12-19 15:41 ` Yao Qi @ 2017-12-19 16:15 ` Ulrich Weigand 2017-12-20 9:57 ` Yao Qi 0 siblings, 1 reply; 40+ messages in thread From: Ulrich Weigand @ 2017-12-19 16:15 UTC (permalink / raw) To: Yao Qi; +Cc: GDB Patches Yao Qi wrote: > On Tue, Dec 19, 2017 at 1:50 PM, Ulrich Weigand <uweigand@de.ibm.com> wrote: > > 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. It's a "merged" address space containing the PowerPC address space (with top bit 0) as well as all the SPU address spaces (top bit 1, next 31 bits the SPU ID, last 32 bit the address on that SPU). I wouldn't need any of that if we actually supported "real" address spaces, i.e. instead of always passing just a CORE_ADDR, use a pair of address-space identifier and CORE_ADDR. But that would be a huge change, and so I worked around that by using the "merged" encoding. > > 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. Yes, that should still work (e.g. when the program stops anywhere, GDB will check software watchpoints that may be simultaneously set in SPU and PPU memory). Therefore target_thread_architecture doesn't really solve the problem either. > > 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 appr= > oach > 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). Well, a simple workaround is to just always set significant_addr_bits to 64 in ppc as well. (This just doesn't matter for actual ppc addresses.) But that doesn't really look like a clean solution for the generic multi-architecture case ... Bye, Ulrich -- Dr. Ulrich Weigand GNU/Linux compilers and toolchain Ulrich.Weigand@de.ibm.com ^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [PATCH 1/3] Clear non-significant bits of address on memory access 2017-12-19 16:15 ` Ulrich Weigand @ 2017-12-20 9:57 ` Yao Qi 2017-12-20 13:03 ` [pushed] Fix Cell/B.E. regression (Re: [PATCH 1/3] Clear non-significant bits of address on memory access) Ulrich Weigand 0 siblings, 1 reply; 40+ messages in thread From: Yao Qi @ 2017-12-20 9:57 UTC (permalink / raw) To: Ulrich Weigand; +Cc: GDB Patches "Ulrich Weigand" <uweigand@de.ibm.com> writes: >> I am stilling thinking how to do it... alternatively, do you like the appr= >> oach >> 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). > > Well, a simple workaround is to just always set significant_addr_bits > to 64 in ppc as well. (This just doesn't matter for actual ppc addresses.) > But that doesn't really look like a clean solution for the generic > multi-architecture case ... Nowadays, we strip non-significant bits in address, pass the stripped address to target cache and to_xfer_partial. It works for aarch64, but breaks ppc32/spu. However, in ppc32/spu case, ppu address and spu address is mixed together, differentiated by the top bit, so the number of significant bits of address is 64, because if we can't remove any of them. IMO, it is reasonable to set significant_addr_bits to 64 in ppc. I considered your suggestion that pushing address_significant call down, below spu-multiarch target, that means, many target's to_xfer_partial need to call address_significant, so I don't do that. Secondly, in the way you suggested, we still pass the original address to target cache, which works for ppu/spu, but it doesn't work for aarch64. -- Yao (齐尧) ^ permalink raw reply [flat|nested] 40+ messages in thread
* [pushed] Fix Cell/B.E. regression (Re: [PATCH 1/3] Clear non-significant bits of address on memory access) 2017-12-20 9:57 ` Yao Qi @ 2017-12-20 13:03 ` Ulrich Weigand 2017-12-20 13:59 ` Yao Qi 0 siblings, 1 reply; 40+ messages in thread From: Ulrich Weigand @ 2017-12-20 13:03 UTC (permalink / raw) To: Yao Qi; +Cc: GDB Patches Yao Qi wrote: > Nowadays, we strip non-significant bits in address, pass the stripped > address to target cache and to_xfer_partial. It works for aarch64, but > breaks ppc32/spu. However, in ppc32/spu case, ppu address and spu > address is mixed together, differentiated by the top bit, so the number > of significant bits of address is 64, because if we can't remove any of > them. IMO, it is reasonable to set significant_addr_bits to 64 in ppc. > > I considered your suggestion that pushing address_significant call down, > below spu-multiarch target, that means, many target's to_xfer_partial > need to call address_significant, so I don't do that. Secondly, in the > way you suggested, we still pass the original address to target cache, > which works for ppu/spu, but it doesn't work for aarch64. I've now pushed the patch below, which fixes the regression for now. Longer term, I think the correct fix would probably be to make address spaces explit, e.g. by passing an address space identifer to xfer_partial. The gdbarch associated with that address space should then determine whether truncation is required ... Bye, Ulrich gdb/ChangeLog: * spu-tdep.c (spu_gdbarch_init): Set set_gdbarch_significant_addr_bit to 64 bits. (ppc_linux_init_abi): Likewise, if Cell/B.E. is supported. diff --git a/gdb/ppc-linux-tdep.c b/gdb/ppc-linux-tdep.c index 0e43a64..5120490 100644 --- a/gdb/ppc-linux-tdep.c +++ b/gdb/ppc-linux-tdep.c @@ -1809,6 +1809,10 @@ ppc_linux_init_abi (struct gdbarch_info info, /* Cell/B.E. cross-architecture unwinder support. */ frame_unwind_prepend_unwinder (gdbarch, &ppu2spu_unwind); + + /* We need to support more than "addr_bit" significant address bits + in order to support SPUADDR_ADDR encoded values. */ + set_gdbarch_significant_addr_bit (gdbarch, 64); } set_gdbarch_displaced_step_location (gdbarch, diff --git a/gdb/spu-tdep.c b/gdb/spu-tdep.c index fb9a5d8..dda3011 100644 --- a/gdb/spu-tdep.c +++ b/gdb/spu-tdep.c @@ -2720,6 +2720,9 @@ spu_gdbarch_init (struct gdbarch_info info, struct gdbarch_list *arches) set_gdbarch_address_class_name_to_type_flags (gdbarch, spu_address_class_name_to_type_flags); + /* We need to support more than "addr_bit" significant address bits + in order to support SPUADDR_ADDR encoded values. */ + set_gdbarch_significant_addr_bit (gdbarch, 64); /* Inferior function calls. */ set_gdbarch_call_dummy_location (gdbarch, ON_STACK); -- Dr. Ulrich Weigand GNU/Linux compilers and toolchain Ulrich.Weigand@de.ibm.com ^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [pushed] Fix Cell/B.E. regression (Re: [PATCH 1/3] Clear non-significant bits of address on memory access) 2017-12-20 13:03 ` [pushed] Fix Cell/B.E. regression (Re: [PATCH 1/3] Clear non-significant bits of address on memory access) Ulrich Weigand @ 2017-12-20 13:59 ` Yao Qi 0 siblings, 0 replies; 40+ messages in thread From: Yao Qi @ 2017-12-20 13:59 UTC (permalink / raw) To: Ulrich Weigand; +Cc: GDB Patches On Wed, Dec 20, 2017 at 1:03 PM, Ulrich Weigand <uweigand@de.ibm.com> wrote: > I've now pushed the patch below, which fixes the regression for now. > Thank you. > Longer term, I think the correct fix would probably be to make address > spaces explit, e.g. by passing an address space identifer to xfer_partial. > The gdbarch associated with that address space should then determine > whether truncation is required ... > Agreed. -- Yao (齐尧) ^ permalink raw reply [flat|nested] 40+ messages in thread
* [PATCH 3/3] Clear non-significant bits of address in watchpoint 2017-12-08 10:04 [PATCH 0/3 v3] [AArch64] Support tagged pointer Yao Qi 2017-12-08 10:04 ` [PATCH 2/3] Adjust breakpoint address by clearing non-significant bits Yao Qi 2017-12-08 10:04 ` [PATCH 1/3] Clear non-significant bits of address on memory access Yao Qi @ 2017-12-08 10:04 ` Yao Qi 2017-12-08 12:23 ` Pedro Alves 2017-12-08 12:24 ` [PATCH 0/3 v3] [AArch64] Support tagged pointer Pedro Alves 2017-12-08 17:31 ` Yao Qi 4 siblings, 1 reply; 40+ messages in thread From: Yao Qi @ 2017-12-08 10:04 UTC (permalink / raw) To: gdb-patches Nowadays, GDB can't set watchpoint on tagged address on AArch64, (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 by getting rid of non-significant bits of address. gdb: 2017-12-01 Yao Qi <yao.qi@linaro.org> * breakpoint.c (update_watchpoint): Call significant_addr. gdb/testsuite: 2017-12-01 Yao Qi <yao.qi@linaro.org> * gdb.arch/aarch64-tagged-pointer.c (main): Update. * gdb.arch/aarch64-tagged-pointer.exp: Add tests for watchpoint. --- gdb/breakpoint.c | 2 +- gdb/testsuite/gdb.arch/aarch64-tagged-pointer.c | 1 + gdb/testsuite/gdb.arch/aarch64-tagged-pointer.exp | 14 ++++++++++++++ 3 files changed, 16 insertions(+), 1 deletion(-) diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c index 22b3069..6570513 100644 --- a/gdb/breakpoint.c +++ b/gdb/breakpoint.c @@ -1867,7 +1867,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 = address_significant (loc->gdbarch, addr); if (bitsize != 0) { diff --git a/gdb/testsuite/gdb.arch/aarch64-tagged-pointer.c b/gdb/testsuite/gdb.arch/aarch64-tagged-pointer.c index 9bfe41e..5754785 100644 --- a/gdb/testsuite/gdb.arch/aarch64-tagged-pointer.c +++ b/gdb/testsuite/gdb.arch/aarch64-tagged-pointer.c @@ -53,4 +53,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 fcab1b7..c08993e 100644 --- a/gdb/testsuite/gdb.arch/aarch64-tagged-pointer.exp +++ b/gdb/testsuite/gdb.arch/aarch64-tagged-pointer.exp @@ -89,3 +89,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] 40+ messages in thread
* Re: [PATCH 3/3] Clear non-significant bits of address in watchpoint 2017-12-08 10:04 ` [PATCH 3/3] Clear non-significant bits of address in watchpoint Yao Qi @ 2017-12-08 12:23 ` Pedro Alves 0 siblings, 0 replies; 40+ messages in thread From: Pedro Alves @ 2017-12-08 12:23 UTC (permalink / raw) To: Yao Qi, gdb-patches Looks good to me with formatting nits pointed out below fixed. On 12/08/2017 10:04 AM, Yao Qi wrote: > 2017-12-01 Yao Qi <yao.qi@linaro.org> > > * breakpoint.c (update_watchpoint): Call > significant_addr. Wrong name: significant_addr -> address_significant. Guess you went back and forth deciding the function name. :-) > @@ -1867,7 +1867,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 = address_significant (loc->gdbarch, addr); Spurious space before '='. Thanks, Pedro Alves ^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [PATCH 0/3 v3] [AArch64] Support tagged pointer 2017-12-08 10:04 [PATCH 0/3 v3] [AArch64] Support tagged pointer Yao Qi ` (2 preceding siblings ...) 2017-12-08 10:04 ` [PATCH 3/3] Clear non-significant bits of address in watchpoint Yao Qi @ 2017-12-08 12:24 ` Pedro Alves 2017-12-08 17:31 ` Yao Qi 4 siblings, 0 replies; 40+ messages in thread From: Pedro Alves @ 2017-12-08 12:24 UTC (permalink / raw) To: Yao Qi, gdb-patches On 12/08/2017 10:04 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 > > Some parts of GDB related to address are updated in this patch series, > > - Memory access, like command 'x', > - Setting hw breakpoint on some address, > - Setting watchpoint on some address, > > Address tag is treated as non-significant bits of address, so this patch > series add a new gdbarch significant_addr_bit, and use it in gdbarch to > get rid of non-significant bits. This was the suggestion in the v2 review. > (https://sourceware.org/ml/gdb-patches/2017-10/msg00792.html) Thanks much for the update. I like this version a lot better. > > When I test this patch series, and I find a regression in linespec. I > posted the fix https://sourceware.org/ml/gdb-patches/2017-12/msg00158.html > Without this fix, this series causes a regression. Thanks again for that fix. Pedro Alves ^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [PATCH 0/3 v3] [AArch64] Support tagged pointer 2017-12-08 10:04 [PATCH 0/3 v3] [AArch64] Support tagged pointer Yao Qi ` (3 preceding siblings ...) 2017-12-08 12:24 ` [PATCH 0/3 v3] [AArch64] Support tagged pointer Pedro Alves @ 2017-12-08 17:31 ` Yao Qi 2018-04-11 0:16 ` Omair Javaid 4 siblings, 1 reply; 40+ messages in thread From: Yao Qi @ 2017-12-08 17:31 UTC (permalink / raw) To: GDB Patches On Fri, Dec 8, 2017 at 10:04 AM, Yao Qi <qiyaoltc@gmail.com> 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 > > Some parts of GDB related to address are updated in this patch series, > > - Memory access, like command 'x', > - Setting hw breakpoint on some address, > - Setting watchpoint on some address, > I pushed them in as we are close to branching. We can still keep the addr_bits_remove vs significant_addr_bit conversation ongoing. -- Yao (齐尧) ^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [PATCH 0/3 v3] [AArch64] Support tagged pointer 2017-12-08 17:31 ` Yao Qi @ 2018-04-11 0:16 ` Omair Javaid 2018-04-11 0:37 ` Omair Javaid 2018-04-11 10:14 ` Pedro Alves 0 siblings, 2 replies; 40+ messages in thread From: Omair Javaid @ 2018-04-11 0:16 UTC (permalink / raw) To: Yao Qi; +Cc: GDB Patches On 8 December 2017 at 22:31, Yao Qi <qiyaoltc@gmail.com> wrote: > On Fri, Dec 8, 2017 at 10:04 AM, Yao Qi <qiyaoltc@gmail.com> 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 > > > > Some parts of GDB related to address are updated in this patch series, > > > > - Memory access, like command 'x', > > - Setting hw breakpoint on some address, > > - Setting watchpoint on some address, > > > > I pushed them in as we are close to branching. We can still keep the > addr_bits_remove vs significant_addr_bit conversation ongoing. > This patch has broken kernel debugging using kgdb and openOCD. Tagged address should only be considered while debugging only user-space programs on linux. I still need to understand the full background behind this patch but can we pull it out of 8.1 release to make sure kernel debugging works? -- > Yao (齐尧) > ^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [PATCH 0/3 v3] [AArch64] Support tagged pointer 2018-04-11 0:16 ` Omair Javaid @ 2018-04-11 0:37 ` Omair Javaid 2018-04-11 2:46 ` Simon Marchi 2018-04-11 10:14 ` Pedro Alves 1 sibling, 1 reply; 40+ messages in thread From: Omair Javaid @ 2018-04-11 0:37 UTC (permalink / raw) To: Yao Qi; +Cc: GDB Patches On 11 April 2018 at 05:15, Omair Javaid <omair.javaid@linaro.org> wrote: > On 8 December 2017 at 22:31, Yao Qi <qiyaoltc@gmail.com> wrote: > >> On Fri, Dec 8, 2017 at 10:04 AM, Yao Qi <qiyaoltc@gmail.com> 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 >> > >> > Some parts of GDB related to address are updated in this patch series, >> > >> > - Memory access, like command 'x', >> > - Setting hw breakpoint on some address, >> > - Setting watchpoint on some address, >> > >> >> I pushed them in as we are close to branching. We can still keep the >> addr_bits_remove vs significant_addr_bit conversation ongoing. >> > > This patch has broken kernel debugging using kgdb and openOCD. > > Tagged address should only be considered while debugging only user-space > programs on linux. > > I still need to understand the full background behind this patch but can > we pull it out of 8.1 release to make sure kernel debugging works? > > Just found out that patch series posted here https://sourceware.org/ml/gdb-patches/2017-12/msg00160.html does change aarch64-linux-tdep only. But the committed version here https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;a=commit;h=a738ea1d41daeec0cccb4ab6671f4f6d53bd9e18 is applying it to aarch64-tdep > -- >> Yao (齐尧) >> > > ^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [PATCH 0/3 v3] [AArch64] Support tagged pointer 2018-04-11 0:37 ` Omair Javaid @ 2018-04-11 2:46 ` Simon Marchi 0 siblings, 0 replies; 40+ messages in thread From: Simon Marchi @ 2018-04-11 2:46 UTC (permalink / raw) To: Omair Javaid; +Cc: Yao Qi, GDB Patches, Pedro Alves On 2018-04-10 20:36, Omair Javaid wrote: >> This patch has broken kernel debugging using kgdb and openOCD. >> >> Tagged address should only be considered while debugging only >> user-space >> programs on linux. >> >> I still need to understand the full background behind this patch but >> can >> we pull it out of 8.1 release to make sure kernel debugging works? >> >> > Just found out that patch series posted here > https://sourceware.org/ml/gdb-patches/2017-12/msg00160.html does change > aarch64-linux-tdep only. > But the committed version here > https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;a=commit;h=a738ea1d41daeec0cccb4ab6671f4f6d53bd9e18 > is > applying it to aarch64-tdep I think it was because of Pedro's suggestion in this reply: https://sourceware.org/ml/gdb-patches/2017-12/msg00179.html although there was now acknowledgement from Yao, so I can only guess. As Pedro said, it's the hardware that ignores these top bits. But it first has to be configured to do so, so we can't assume that all aarch64 code behaves like this. So far we only know that the Linux userspace threads work like that, so I think it would make sense to move it to aarch64-linux-tdep.c. Did you confirm that it indeed fixes your problem? I could see a fix for this getting into the 8.1 branch (not reverting the patch though). Simon ^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [PATCH 0/3 v3] [AArch64] Support tagged pointer 2018-04-11 0:16 ` Omair Javaid 2018-04-11 0:37 ` Omair Javaid @ 2018-04-11 10:14 ` Pedro Alves 2018-04-11 11:13 ` Omair Javaid 1 sibling, 1 reply; 40+ messages in thread From: Pedro Alves @ 2018-04-11 10:14 UTC (permalink / raw) To: Omair Javaid, Yao Qi; +Cc: GDB Patches On 04/11/2018 01:15 AM, Omair Javaid wrote: > This patch has broken kernel debugging using kgdb and openOCD. OOC, can you qualify this a bit more, please? Does the kernel use the high bits for something? Thanks, Pedro Alves ^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [PATCH 0/3 v3] [AArch64] Support tagged pointer 2018-04-11 10:14 ` Pedro Alves @ 2018-04-11 11:13 ` Omair Javaid 2018-04-11 11:19 ` Pedro Alves 0 siblings, 1 reply; 40+ messages in thread From: Omair Javaid @ 2018-04-11 11:13 UTC (permalink / raw) To: Pedro Alves; +Cc: Yao Qi, GDB Patches On 11 April 2018 at 15:13, Pedro Alves <palves@redhat.com> wrote: > On 04/11/2018 01:15 AM, Omair Javaid wrote: > > > This patch has broken kernel debugging using kgdb and openOCD. > > OOC, can you qualify this a bit more, please? > > Does the kernel use the high bits for something? > We can safely assume that top byte is 0 in case of user address space on linux because it enables tagging support but not for kernel address space. According to linux memory layout of AArch64 given here: https://www.kernel.org/doc/Documentation/arm64/memory.txt "User addresses have bits 63:48 set to 0 while the kernel addresses have the same bits set to 1. TTBRx selection is given by bit 63 of the virtual address." According to kernel document on tagged pointer support in AArch64 given here: https://www.kernel.org/doc/Documentation/arm64/tagged-pointers.txt The kernel configures the translation tables so that translations made via TTBR0 (i.e. userspace mappings) have the top byte (bits 63:56) of the virtual address ignored by the translation hardware. This frees up this byte for application use. With set_gdbarch_significant_addr_bit applied to aarch64-tdep following happens when gdb tries reading kernel address space memory: query the 0xffffffc000092698 memory data, GDB sent "m00ffffc000092698,4" instead of "mffffffc000092698,4" > Thanks, > Pedro Alves > ^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [PATCH 0/3 v3] [AArch64] Support tagged pointer 2018-04-11 11:13 ` Omair Javaid @ 2018-04-11 11:19 ` Pedro Alves 2018-04-11 12:01 ` Omair Javaid 0 siblings, 1 reply; 40+ messages in thread From: Pedro Alves @ 2018-04-11 11:19 UTC (permalink / raw) To: Omair Javaid; +Cc: Yao Qi, GDB Patches On 04/11/2018 12:12 PM, Omair Javaid wrote: > On 11 April 2018 at 15:13, Pedro Alves <palves@redhat.com <mailto:palves@redhat.com>> wrote: > > On 04/11/2018 01:15 AM, Omair Javaid wrote: > > > This patch has broken kernel debugging using kgdb and openOCD. > > OOC, can you qualify this a bit more, please? > > Does the kernel use the high bits for something? > > > We can safely assume that top byte is 0 in case of user address space on linux because it enables tagging support but not for kernel address space. > > According to linux memory layout of AArch64 given here: https://www.kernel.org/doc/Documentation/arm64/memory.txt > > "User addresses have bits 63:48 set to 0 while the kernel addresses have ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ > the same bits set to 1. TTBRx selection is given by bit 63 of the ^^^^^^^^^^^^^^^^^^^^^^^^^ > virtual address." Ah, that's clear as day now. > > According to kernel document on tagged pointer support in AArch64 given here: https://www.kernel.org/doc/Documentation/arm64/tagged-pointers.txt > > The kernel configures the translation tables so that translations made > via TTBR0 (i.e. userspace mappings) have the top byte (bits 63:56) of > the virtual address ignored by the translation hardware. This frees up > this byte for application use. > > With set_gdbarch_significant_addr_bit applied to aarch64-tdep following happens when gdb tries reading kernel address space memory: > > query the 0xffffffc000092698 memory data, GDB sent "m00ffffc000092698,4" instead of "mffffffc000092698,4" > OK, that makes a lot more sense now. The above is the perfect info to be included in a git commit log. Want to submit a patch? Thanks, Pedro Alves ^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [PATCH 0/3 v3] [AArch64] Support tagged pointer 2018-04-11 11:19 ` Pedro Alves @ 2018-04-11 12:01 ` Omair Javaid 2018-04-11 18:27 ` Pedro Alves 0 siblings, 1 reply; 40+ messages in thread From: Omair Javaid @ 2018-04-11 12:01 UTC (permalink / raw) To: Pedro Alves; +Cc: Yao Qi, GDB Patches On 11 April 2018 at 16:19, Pedro Alves <palves@redhat.com> wrote: > On 04/11/2018 12:12 PM, Omair Javaid wrote: > > On 11 April 2018 at 15:13, Pedro Alves <palves@redhat.com <mailto: > palves@redhat.com>> wrote: > > > > On 04/11/2018 01:15 AM, Omair Javaid wrote: > > > > > This patch has broken kernel debugging using kgdb and openOCD. > > > > OOC, can you qualify this a bit more, please? > > > > Does the kernel use the high bits for something? > > > > > > We can safely assume that top byte is 0 in case of user address space on > linux because it enables tagging support but not for kernel address space. > > > > According to linux memory layout of AArch64 given here: > https://www.kernel.org/doc/Documentation/arm64/memory.txt > > > > "User addresses have bits 63:48 set to 0 while the kernel addresses have > ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ > > the same bits set to 1. TTBRx selection is given by bit 63 of the > ^^^^^^^^^^^^^^^^^^^^^^^^^ > > virtual address." > > Ah, that's clear as day now. > > > > > According to kernel document on tagged pointer support in AArch64 given > here: https://www.kernel.org/doc/Documentation/arm64/tagged-pointers.txt > > > > The kernel configures the translation tables so that translations made > > via TTBR0 (i.e. userspace mappings) have the top byte (bits 63:56) of > > the virtual address ignored by the translation hardware. This frees up > > this byte for application use. > > > > With set_gdbarch_significant_addr_bit applied to aarch64-tdep following > happens when gdb tries reading kernel address space memory: > > > > query the 0xffffffc000092698 memory data, GDB sent "m00ffffc000092698,4" > instead of "mffffffc000092698,4" > > > OK, that makes a lot more sense now. The above is the perfect > info to be included in a git commit log. Want to submit a patch? > Yes I can submit a patch that enables set_gdbarch_significant_addr_bit for aarch64-linux-tdep only. But a point to discuss here is the use-case where some people use *-linux-gdb for debugging seamlessly between kernel and user-space. There can be ways we can distinguish between user/kernel address space and clear or set top byte of the address even in case of linux targets. Does this sound something we should do? > > Thanks, > Pedro Alves > ^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [PATCH 0/3 v3] [AArch64] Support tagged pointer 2018-04-11 12:01 ` Omair Javaid @ 2018-04-11 18:27 ` Pedro Alves 2018-04-16 1:36 ` Omair Javaid 0 siblings, 1 reply; 40+ messages in thread From: Pedro Alves @ 2018-04-11 18:27 UTC (permalink / raw) To: Omair Javaid; +Cc: Yao Qi, GDB Patches On 04/11/2018 12:59 PM, Omair Javaid wrote: > Yes I can submit a patch that enables set_gdbarch_significant_addr_bit for aarch64-linux-tdep only. > > But a point to discuss here is the use-case where some people use *-linux-gdb for debugging seamlessly between kernel and user-space. > > There can be ways we can distinguish between user/kernel address space and clear or set top byte of the address even in case of linux targets. > > Does this sound something we should do? Yeah, why not. What are the pending kernel debugging patches using to distinguish userspace and kernel debugging modes? Off hand, I'd think we'd want to make those separate ABIs / osabis / gdbarchs. Thanks, Pedro Alves ^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [PATCH 0/3 v3] [AArch64] Support tagged pointer 2018-04-11 18:27 ` Pedro Alves @ 2018-04-16 1:36 ` Omair Javaid 2018-04-16 22:57 ` Pedro Alves 0 siblings, 1 reply; 40+ messages in thread From: Omair Javaid @ 2018-04-16 1:36 UTC (permalink / raw) To: Pedro Alves; +Cc: Yao Qi, GDB Patches On 11 April 2018 at 23:27, Pedro Alves <palves@redhat.com> wrote: > On 04/11/2018 12:59 PM, Omair Javaid wrote: > > > Yes I can submit a patch that enables set_gdbarch_significant_addr_bit > for aarch64-linux-tdep only. > > > > But a point to discuss here is the use-case where some people use > *-linux-gdb for debugging seamlessly between kernel and user-space. > > > > There can be ways we can distinguish between user/kernel address space > and clear or set top byte of the address even in case of linux targets. > > > > Does this sound something we should do? > > Yeah, why not. > > What are the pending kernel debugging patches using to distinguish > userspace and kernel debugging modes? Off hand, I'd think we'd want to > make those separate ABIs / osabis / gdbarchs. > Sorry for late reply on this I am out of office this week. I have given this a thought and I propose to do the following: Turn on pointer tagging on OSABI (LINUX) by default. Add commands set aarch64 pointer-tagging show/enable/disable. Once LKD patches for aarch64/arm land in our need for this will automatically be solved. ^ Does this make sense? For Linux Kernel Debugging we have separate OSABI (LINUX_KERNEL). Patch snippet from LKD patches by Phillip given below: +static enum gdb_osabi +s390_lk_osabi_sniffer (bfd *abfd) +{ + if (bfd_get_section_by_name (abfd, ".reg-s390-prefix") + || bfd_get_section_by_name (abfd, "__ksymtab")) + return GDB_OSABI_LINUX_KERNEL; + + return GDB_OSABI_UNKNOWN; +} + +extern initialize_file_ftype _initialize_s390_lk_tdep; /* -Wmissing-prototypes */ + +void +_initialize_s390_lk_tdep (void) +{ + /* Hook us into the OSABI mechanism. */ + gdbarch_register_osabi (bfd_arch_s390, bfd_mach_s390_64, GDB_OSABI_LINUX_KERNEL, + s390_lk_init_abi_64); + /* Add OSABI sniffer. */ + gdbarch_register_osabi_sniffer (bfd_arch_s390, bfd_target_elf_flavour, + s390_lk_osabi_sniffer); + + /* Initialize the Linux kernel target descriptions. */ + initialize_tdesc_s390x_cr_linux64 (); + initialize_tdesc_s390x_vxcr_linux64 (); +} Thanks, > Pedro Alves > ^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [PATCH 0/3 v3] [AArch64] Support tagged pointer 2018-04-16 1:36 ` Omair Javaid @ 2018-04-16 22:57 ` Pedro Alves 2018-04-20 14:34 ` Omair Javaid 0 siblings, 1 reply; 40+ messages in thread From: Pedro Alves @ 2018-04-16 22:57 UTC (permalink / raw) To: Omair Javaid; +Cc: Yao Qi, GDB Patches On 04/16/2018 02:36 AM, Omair Javaid wrote: > On 11 April 2018 at 23:27, Pedro Alves <palves@redhat.com> wrote: > >> On 04/11/2018 12:59 PM, Omair Javaid wrote: >> >>> Yes I can submit a patch that enables set_gdbarch_significant_addr_bit >> for aarch64-linux-tdep only. >>> >>> But a point to discuss here is the use-case where some people use >> *-linux-gdb for debugging seamlessly between kernel and user-space. >>> >>> There can be ways we can distinguish between user/kernel address space >> and clear or set top byte of the address even in case of linux targets. >>> >>> Does this sound something we should do? >> >> Yeah, why not. >> >> What are the pending kernel debugging patches using to distinguish >> userspace and kernel debugging modes? Off hand, I'd think we'd want to >> make those separate ABIs / osabis / gdbarchs. >> > > Sorry for late reply on this I am out of office this week. > > I have given this a thought and I propose to do the following: > > Turn on pointer tagging on OSABI (LINUX) by default. > > Add commands set aarch64 pointer-tagging show/enable/disable. > > Once LKD patches for aarch64/arm land in our need for this will > automatically be solved. Makes sense, but I'd like to clarify usefulness of the separate "set aarch64 pointer-tagging" command. If indeed we're doing to end up with a separate osabi for the Linux kernel, then "set osabi linux-kernel" will result in disabling pointer-tagging too. So, will it still be useful to have the specific "set aarch64 pointer-tagging" commands? Do you see use cases for "set aarch64 pointer-tagging" beyond disabling it for Linux kernel debugging? I'm thinking that it may be useful for bare metal debugging. But, ideally, GDB would figure it out on its own without user intervention. Is there's some bit in some register gdb could read that indicates whether tagging is enabled? Thanks, Pedro Alves ^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [PATCH 0/3 v3] [AArch64] Support tagged pointer 2018-04-16 22:57 ` Pedro Alves @ 2018-04-20 14:34 ` Omair Javaid 2018-04-20 16:13 ` Daniel Thompson 2018-04-24 11:48 ` Pedro Alves 0 siblings, 2 replies; 40+ messages in thread From: Omair Javaid @ 2018-04-20 14:34 UTC (permalink / raw) To: Pedro Alves; +Cc: Yao Qi, GDB Patches On 17 April 2018 at 03:57, Pedro Alves <palves@redhat.com> wrote: > On 04/16/2018 02:36 AM, Omair Javaid wrote: > > On 11 April 2018 at 23:27, Pedro Alves <palves@redhat.com> wrote: > > > >> On 04/11/2018 12:59 PM, Omair Javaid wrote: > >> > >>> Yes I can submit a patch that enables set_gdbarch_significant_addr_bit > >> for aarch64-linux-tdep only. > >>> > >>> But a point to discuss here is the use-case where some people use > >> *-linux-gdb for debugging seamlessly between kernel and user-space. > >>> > >>> There can be ways we can distinguish between user/kernel address space > >> and clear or set top byte of the address even in case of linux targets. > >>> > >>> Does this sound something we should do? > >> > >> Yeah, why not. > >> > >> What are the pending kernel debugging patches using to distinguish > >> userspace and kernel debugging modes? Off hand, I'd think we'd want to > >> make those separate ABIs / osabis / gdbarchs. > >> > > > > Sorry for late reply on this I am out of office this week. > > > > I have given this a thought and I propose to do the following: > > > > Turn on pointer tagging on OSABI (LINUX) by default. > > > > Add commands set aarch64 pointer-tagging show/enable/disable. > > > > Once LKD patches for aarch64/arm land in our need for this will > > automatically be solved. > > Makes sense, but I'd like to clarify usefulness of the separate > "set aarch64 pointer-tagging" command. > If indeed we're doing to end up with a separate osabi for the Linux > kernel, then "set osabi linux-kernel" will result > in disabling pointer-tagging too. So, will it still be useful to have > the specific "set aarch64 pointer-tagging" commands? Do you see > use cases for "set aarch64 pointer-tagging" beyond disabling it > for Linux kernel debugging? I'm thinking that it may be useful > for bare metal debugging. But, ideally, GDB would figure it out > on its own without user intervention. Is there's some bit in some > register gdb could read that indicates whether tagging is enabled? > > Pointer tagging information is stored in MMU registers so in linux user-space we cannot actually read if pointer tagging is enabled or not based on register bits. JTAG debuggers should be able to read MMU registers and know whether pointer tagging is enabled or not. Rationale behind adding a separate command is to allow other application to control pointer tagging for example bare-metal (non-linux OSes) which want to use pointer tagging can enable it. I must admit I dont know of any such use-case as of now. Also I am not sure about the timeline of Linux Kernel patches going into gdb and for now I thought of this command as the most suitable option. Moreover some users might also be interested in combination where pointer tagging is enabled but Linux Kernel threads support is disabled so I thought we should give the control to the user in cases where we cannot predict use-cases. > Thanks, > Pedro Alves > ^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: Re: [PATCH 0/3 v3] [AArch64] Support tagged pointer 2018-04-20 14:34 ` Omair Javaid @ 2018-04-20 16:13 ` Daniel Thompson 2018-04-23 7:50 ` Omair Javaid 2018-04-24 11:48 ` Pedro Alves 1 sibling, 1 reply; 40+ messages in thread From: Daniel Thompson @ 2018-04-20 16:13 UTC (permalink / raw) To: Omair Javaid, Pedro Alves; +Cc: Yao Qi, GDB Patches On 20/04/18 15:33, Omair Javaid wrote: > On 17 April 2018 at 03:57, Pedro Alves <palves@redhat.com> wrote: > >> On 04/16/2018 02:36 AM, Omair Javaid wrote: >>> On 11 April 2018 at 23:27, Pedro Alves <palves@redhat.com> wrote: >>> >>>> On 04/11/2018 12:59 PM, Omair Javaid wrote: >>>> >>>>> Yes I can submit a patch that enables set_gdbarch_significant_addr_bit >>>> for aarch64-linux-tdep only. >>>>> >>>>> But a point to discuss here is the use-case where some people use >>>> *-linux-gdb for debugging seamlessly between kernel and user-space. >>>>> >>>>> There can be ways we can distinguish between user/kernel address space >>>> and clear or set top byte of the address even in case of linux targets. >>>>> >>>>> Does this sound something we should do? >>>> >>>> Yeah, why not. >>>> >>>> What are the pending kernel debugging patches using to distinguish >>>> userspace and kernel debugging modes? Off hand, I'd think we'd want to >>>> make those separate ABIs / osabis / gdbarchs. >>>> >>> >>> Sorry for late reply on this I am out of office this week. >>> >>> I have given this a thought and I propose to do the following: >>> >>> Turn on pointer tagging on OSABI (LINUX) by default. >>> >>> Add commands set aarch64 pointer-tagging show/enable/disable. >>> >>> Once LKD patches for aarch64/arm land in our need for this will >>> automatically be solved. >> >> Makes sense, but I'd like to clarify usefulness of the separate >> "set aarch64 pointer-tagging" command. >> If indeed we're doing to end up with a separate osabi for the Linux >> kernel, then "set osabi linux-kernel" will result >> in disabling pointer-tagging too. So, will it still be useful to have >> the specific "set aarch64 pointer-tagging" commands? Do you see >> use cases for "set aarch64 pointer-tagging" beyond disabling it >> for Linux kernel debugging? I'm thinking that it may be useful >> for bare metal debugging. But, ideally, GDB would figure it out >> on its own without user intervention. Is there's some bit in some >> register gdb could read that indicates whether tagging is enabled? >> >> > Pointer tagging information is stored in MMU registers so in linux > user-space we cannot actually read if pointer tagging is enabled or not > based on register bits. > JTAG debuggers should be able to read MMU registers and know whether > pointer tagging is enabled or not. Perhaps a dumb question but could gdb be persuaded to mask the pointers at a lower level. The current patches end up masking the pointer tags relatively early, which results in masked pointers being sent via the gdb remote protocol (which is what causes some of the problems at present: kgdb and OpenOCD get asked for the wrong pointer). If the pointers were masked as the arguments to ptrace() were marshaled this would behave much more like the real hardware and would make debugging Linux kernel mode entirely transparent (since you cannot ptrace() kernel memory we would never try masking out the tag). More generally masking would become the problem of the register read code for the target. I suspect many JTAG debuggers would (and certainly should) already work correctly as their register reads can honour the TTBR0 status. Daniel. ^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: Re: [PATCH 0/3 v3] [AArch64] Support tagged pointer 2018-04-20 16:13 ` Daniel Thompson @ 2018-04-23 7:50 ` Omair Javaid 2018-04-24 11:39 ` Pedro Alves 0 siblings, 1 reply; 40+ messages in thread From: Omair Javaid @ 2018-04-23 7:50 UTC (permalink / raw) To: Daniel Thompson; +Cc: Pedro Alves, Yao Qi, GDB Patches On 20 April 2018 at 21:13, Daniel Thompson <daniel.thompson@linaro.org> wrote: > > On 20/04/18 15:33, Omair Javaid wrote: >> >> On 17 April 2018 at 03:57, Pedro Alves <palves@redhat.com> wrote: >> >>> On 04/16/2018 02:36 AM, Omair Javaid wrote: >>>> >>>> On 11 April 2018 at 23:27, Pedro Alves <palves@redhat.com> wrote: >>>> >>>>> On 04/11/2018 12:59 PM, Omair Javaid wrote: >>>>> >>>>>> Yes I can submit a patch that enables set_gdbarch_significant_addr_bit >>>>> >>>>> for aarch64-linux-tdep only. >>>>>> >>>>>> >>>>>> But a point to discuss here is the use-case where some people use >>>>> >>>>> *-linux-gdb for debugging seamlessly between kernel and user-space. >>>>>> >>>>>> >>>>>> There can be ways we can distinguish between user/kernel address space >>>>> >>>>> and clear or set top byte of the address even in case of linux targets. >>>>>> >>>>>> >>>>>> Does this sound something we should do? >>>>> >>>>> >>>>> Yeah, why not. >>>>> >>>>> What are the pending kernel debugging patches using to distinguish >>>>> userspace and kernel debugging modes? Off hand, I'd think we'd want to >>>>> make those separate ABIs / osabis / gdbarchs. >>>>> >>>> >>>> Sorry for late reply on this I am out of office this week. >>>> >>>> I have given this a thought and I propose to do the following: >>>> >>>> Turn on pointer tagging on OSABI (LINUX) by default. >>>> >>>> Add commands set aarch64 pointer-tagging show/enable/disable. >>>> >>>> Once LKD patches for aarch64/arm land in our need for this will >>>> automatically be solved. >>> >>> >>> Makes sense, but I'd like to clarify usefulness of the separate >>> "set aarch64 pointer-tagging" command. >>> If indeed we're doing to end up with a separate osabi for the Linux >>> kernel, then "set osabi linux-kernel" will result >>> in disabling pointer-tagging too. So, will it still be useful to have >>> the specific "set aarch64 pointer-tagging" commands? Do you see >>> use cases for "set aarch64 pointer-tagging" beyond disabling it >>> for Linux kernel debugging? I'm thinking that it may be useful >>> for bare metal debugging. But, ideally, GDB would figure it out >>> on its own without user intervention. Is there's some bit in some >>> register gdb could read that indicates whether tagging is enabled? >>> >>> >> Pointer tagging information is stored in MMU registers so in linux >> user-space we cannot actually read if pointer tagging is enabled or not >> based on register bits. >> JTAG debuggers should be able to read MMU registers and know whether >> pointer tagging is enabled or not. > > > Perhaps a dumb question but could gdb be persuaded to mask the pointers at a lower level. > > The current patches end up masking the pointer tags relatively early, which results in masked pointers being sent via the gdb remote protocol (which is what causes some of the problems at present: kgdb and OpenOCD get asked for the wrong pointer). > > If the pointers were masked as the arguments to ptrace() were marshaled this would behave much more like the real hardware and would make debugging Linux kernel mode entirely transparent (since you cannot ptrace() kernel memory we would never try masking out the tag). Although this can be done with a hook but will require some fundamental changes to the way ptrace inf_ptrace_xfer_partial memory accesses routines are written. Currently we use a generic implementation inf_ptrace_xfer_partial for all target architectures. Same is the case with GDBServer it just handles the ptrace calls except in a few cases where we need extra architecture specific code before ptrace call like setting hardware breakpoints watchpoints etc. As top byte in tagged address is essentially data, pushing masking down to gdbserver will mean that we ll be sending out data mangled as part of the address. Passing mangled address over RSP expecting other side will correct it doesnt sound right. Lets see what Pedro has to see on this. > > More generally masking would become the problem of the register read code for the target. I suspect many JTAG debuggers would (and certainly should) already work correctly as their register reads can honour the TTBR0 status. > > > Daniel. ^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [PATCH 0/3 v3] [AArch64] Support tagged pointer 2018-04-23 7:50 ` Omair Javaid @ 2018-04-24 11:39 ` Pedro Alves 2018-04-24 15:44 ` Daniel Thompson 0 siblings, 1 reply; 40+ messages in thread From: Pedro Alves @ 2018-04-24 11:39 UTC (permalink / raw) To: Omair Javaid, Daniel Thompson; +Cc: Yao Qi, GDB Patches On 04/23/2018 08:50 AM, Omair Javaid wrote: > On 20 April 2018 at 21:13, Daniel Thompson <daniel.thompson@linaro.org> wrote: >> Perhaps a dumb question but could gdb be persuaded to mask the pointers at a lower level. >> >> The current patches end up masking the pointer tags relatively early, which results in masked pointers being sent via the gdb remote protocol (which is what causes some of the problems at present: kgdb and OpenOCD get asked for the wrong pointer). >> >> If the pointers were masked as the arguments to ptrace() were marshaled this would behave much more like the real hardware and would make debugging Linux kernel mode entirely transparent (since you cannot ptrace() kernel memory we would never try masking out the tag). > > Although this can be done with a hook but will require some > fundamental changes to the way ptrace inf_ptrace_xfer_partial memory > accesses routines are written. Currently we use a generic > implementation inf_ptrace_xfer_partial for all target architectures. > Same is the case with GDBServer it just handles the ptrace calls > except in a few cases where we need extra architecture specific code > before ptrace call like setting hardware breakpoints watchpoints etc. > > As top byte in tagged address is essentially data, pushing masking > down to gdbserver will mean that we ll be sending out data mangled as > part of the address. Passing mangled address over RSP expecting other > side will correct it doesnt sound right. > > Lets see what Pedro has to see on this. It seems like you are proposing going back to what was originally proposed in v1. I think these urls link through all of the history: v1: https://sourceware.org/ml/gdb-patches/2017-10/msg00593.html v2: https://sourceware.org/ml/gdb-patches/2017-10/msg00792.html v3: https://sourceware.org/ml/gdb-patches/2017-12/msg00159.html As can be seen in v2, as soon we started considering watchpoints, here: https://sourceware.org/ml/gdb-patches/2017-10/msg00793.html the gdb core needed to be made aware of tagged pointers, the bit masking could no longer be deferred to the lower level ptrace calls alone. And then given that the core of gdb needs to know to mask out tagged address, by v3, we had no masking at the ptrace level anymore. So I'm not sure how that would work; we'd need a more detailed proposal. Thanks, Pedro Alves ^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [PATCH 0/3 v3] [AArch64] Support tagged pointer 2018-04-24 11:39 ` Pedro Alves @ 2018-04-24 15:44 ` Daniel Thompson 0 siblings, 0 replies; 40+ messages in thread From: Daniel Thompson @ 2018-04-24 15:44 UTC (permalink / raw) To: Pedro Alves; +Cc: Omair Javaid, Yao Qi, GDB Patches On Tue, Apr 24, 2018 at 12:39:15PM +0100, Pedro Alves wrote: > On 04/23/2018 08:50 AM, Omair Javaid wrote: > > On 20 April 2018 at 21:13, Daniel Thompson <daniel.thompson@linaro.org> wrote: > > >> Perhaps a dumb question but could gdb be persuaded to mask the pointers at a lower level. > >> > >> The current patches end up masking the pointer tags relatively early, which results in masked pointers being sent via the gdb remote protocol (which is what causes some of the problems at present: kgdb and OpenOCD get asked for the wrong pointer). > >> > >> If the pointers were masked as the arguments to ptrace() were marshaled this would behave much more like the real hardware and would make debugging Linux kernel mode entirely transparent (since you cannot ptrace() kernel memory we would never try masking out the tag). > > > > Although this can be done with a hook but will require some > > fundamental changes to the way ptrace inf_ptrace_xfer_partial memory > > accesses routines are written. Currently we use a generic > > implementation inf_ptrace_xfer_partial for all target architectures. > > Same is the case with GDBServer it just handles the ptrace calls > > except in a few cases where we need extra architecture specific code > > before ptrace call like setting hardware breakpoints watchpoints etc. > > > > As top byte in tagged address is essentially data, pushing masking > > down to gdbserver will mean that we ll be sending out data mangled as > > part of the address. Passing mangled address over RSP expecting other > > side will correct it doesnt sound right. > > > > Lets see what Pedro has to see on this. > > It seems like you are proposing going back to what was originally > proposed in v1. I think these urls link through all of the > history: > > v1: > https://sourceware.org/ml/gdb-patches/2017-10/msg00593.html > v2: > https://sourceware.org/ml/gdb-patches/2017-10/msg00792.html > v3: > https://sourceware.org/ml/gdb-patches/2017-12/msg00159.html > > As can be seen in v2, as soon we started considering watchpoints, > here: > https://sourceware.org/ml/gdb-patches/2017-10/msg00793.html > the gdb core needed to be made aware of tagged pointers, the bit > masking could no longer be deferred to the lower level ptrace calls > alone. And then given that the core of gdb needs to know to > mask out tagged address, by v3, we had no masking at the ptrace > level anymore. So I'm not sure how that would work; we'd need > a more detailed proposal. Thanks for the links. If you've been round this loop once before I don't think I want to encourage anyone to go round it again. Daniel. ^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [PATCH 0/3 v3] [AArch64] Support tagged pointer 2018-04-20 14:34 ` Omair Javaid 2018-04-20 16:13 ` Daniel Thompson @ 2018-04-24 11:48 ` Pedro Alves 2018-04-24 16:05 ` Daniel Thompson 1 sibling, 1 reply; 40+ messages in thread From: Pedro Alves @ 2018-04-24 11:48 UTC (permalink / raw) To: Omair Javaid; +Cc: Yao Qi, GDB Patches Hi, On 04/20/2018 03:33 PM, Omair Javaid wrote: > Pointer tagging information is stored in MMU registers so in linux > user-space we cannot actually read if pointer tagging is enabled or not > based on register bits. > JTAG debuggers should be able to read MMU registers and know whether > pointer tagging is enabled or not. > > Rationale behind adding a separate command is to allow other application to > control pointer tagging for example bare-metal (non-linux OSes) which want > to use pointer tagging can enable it. I must admit I dont know of any such > use-case as of now. Alright, that's in line with what I was thinking. Though, bare metal should have access to MMU registers too. Ideally, things would Just Work without user intervention. But I don't mind starting by adding a user-controllable knob, it might be a convenient escape hatch. We can always extend it from "on/off" -> "on/off/auto" setting, with auto the default in future. > Also I am not sure about the timeline of Linux Kernel patches going into > gdb and for now I thought of this command as the most suitable option. > Moreover some users might also be interested in combination where pointer > tagging is enabled but Linux Kernel threads support is disabled so I > thought we should give the control to the user in cases where we cannot > predict use-cases. If everyone agrees that proper Linux kernel support benefits from its own osabi setting/name, then I don't see why we couldn't start by adding the osabi setting as soon as we have a use for it, even if the larger Linux Kernel patches aren't ready yet. Thanks, Pedro Alves ^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [PATCH 0/3 v3] [AArch64] Support tagged pointer 2018-04-24 11:48 ` Pedro Alves @ 2018-04-24 16:05 ` Daniel Thompson 2018-04-24 23:42 ` Omair Javaid 0 siblings, 1 reply; 40+ messages in thread From: Daniel Thompson @ 2018-04-24 16:05 UTC (permalink / raw) To: Pedro Alves; +Cc: Omair Javaid, Yao Qi, GDB Patches On Tue, Apr 24, 2018 at 12:48:19PM +0100, Pedro Alves wrote: > Hi, > > On 04/20/2018 03:33 PM, Omair Javaid wrote: > > > Pointer tagging information is stored in MMU registers so in linux > > user-space we cannot actually read if pointer tagging is enabled or not > > based on register bits. > > JTAG debuggers should be able to read MMU registers and know whether > > pointer tagging is enabled or not. > > > > Rationale behind adding a separate command is to allow other application to > > control pointer tagging for example bare-metal (non-linux OSes) which want > > to use pointer tagging can enable it. I must admit I dont know of any such > > use-case as of now. > > Alright, that's in line with what I was thinking. Though, bare metal > should have access to MMU registers too. Ideally, things would Just Work > without user intervention. But I don't mind starting by adding a > user-controllable knob, it might be a convenient escape hatch. We can always > extend it from "on/off" -> "on/off/auto" setting, with auto the default > in future. For bare metal cases this is not a simple on/off control! Top byte ignore (a.k.a. pointer tagging) has separate on/off switches for TTBR0 (0x0 upwards) and TTBR1 (0xffffffffffff downwards) *and* we have to know the respective sizes of TTBR0 and TTBR1 to be sure which table we are using. > > Also I am not sure about the timeline of Linux Kernel patches going into > > gdb and for now I thought of this command as the most suitable option. > > Moreover some users might also be interested in combination where pointer > > tagging is enabled but Linux Kernel threads support is disabled so I > > thought we should give the control to the user in cases where we cannot > > predict use-cases. > > If everyone agrees that proper Linux kernel support benefits from > its own osabi setting/name, then I don't see why we couldn't start by > adding the osabi setting as soon as we have a use for it, even if > the larger Linux Kernel patches aren't ready yet. Following on from the above, for aarch64-linux-tdep we can apply domain knowledge regarding how things are configured. Here we know that TTBR0 is guaranteed to have top byte ignore set, TTBR1 does not *and* we also know (from memory-layout.txt) that TTBR0 is sufficiently small that bit 55 can be used to discriminate between the two cases. In others words regardless of whether we are running at EL0 or EL1 then I think we should mask the top byte from pointers if and only if bit 55 is unset, otherwise leave them as they are. Daniel. ^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [PATCH 0/3 v3] [AArch64] Support tagged pointer 2018-04-24 16:05 ` Daniel Thompson @ 2018-04-24 23:42 ` Omair Javaid 2018-04-25 0:09 ` Andrew Pinski 2018-04-25 8:04 ` Daniel Thompson 0 siblings, 2 replies; 40+ messages in thread From: Omair Javaid @ 2018-04-24 23:42 UTC (permalink / raw) To: Daniel Thompson; +Cc: Pedro Alves, Yao Qi, GDB Patches On 24 April 2018 at 21:05, Daniel Thompson <daniel.thompson@linaro.org> wrote: > On Tue, Apr 24, 2018 at 12:48:19PM +0100, Pedro Alves wrote: >> Hi, >> >> On 04/20/2018 03:33 PM, Omair Javaid wrote: >> >> > Pointer tagging information is stored in MMU registers so in linux >> > user-space we cannot actually read if pointer tagging is enabled or not >> > based on register bits. >> > JTAG debuggers should be able to read MMU registers and know whether >> > pointer tagging is enabled or not. >> > >> > Rationale behind adding a separate command is to allow other application to >> > control pointer tagging for example bare-metal (non-linux OSes) which want >> > to use pointer tagging can enable it. I must admit I dont know of any such >> > use-case as of now. >> >> Alright, that's in line with what I was thinking. Though, bare metal >> should have access to MMU registers too. Ideally, things would Just Work >> without user intervention. But I don't mind starting by adding a >> user-controllable knob, it might be a convenient escape hatch. We can always >> extend it from "on/off" -> "on/off/auto" setting, with auto the default >> in future. > > For bare metal cases this is not a simple on/off control! > > Top byte ignore (a.k.a. pointer tagging) has separate on/off switches > for TTBR0 (0x0 upwards) and TTBR1 (0xffffffffffff downwards) *and* we > have to know the respective sizes of TTBR0 and TTBR1 to be sure which > table we are using. > > >> > Also I am not sure about the timeline of Linux Kernel patches going into >> > gdb and for now I thought of this command as the most suitable option. >> > Moreover some users might also be interested in combination where pointer >> > tagging is enabled but Linux Kernel threads support is disabled so I >> > thought we should give the control to the user in cases where we cannot >> > predict use-cases. >> >> If everyone agrees that proper Linux kernel support benefits from >> its own osabi setting/name, then I don't see why we couldn't start by >> adding the osabi setting as soon as we have a use for it, even if >> the larger Linux Kernel patches aren't ready yet. > > Following on from the above, for aarch64-linux-tdep we can apply domain > knowledge regarding how things are configured. Here we know that TTBR0 > is guaranteed to have top byte ignore set, TTBR1 does not *and* we > also know (from memory-layout.txt) that TTBR0 is sufficiently small > that bit 55 can be used to discriminate between the two cases. > > In others words regardless of whether we are running at EL0 or EL1 then > I think we should mask the top byte from pointers if and only if bit 55 > is unset, otherwise leave them as they are. What I am understanding here is that you are basing your decision on the fact that: "User addresses have bits 63:48 set to 0 while the kernel addresses have the same bits set to 1. TTBRx selection is given by bit 63 of the virtual address." Sounds legitimate for now but are we ever going to use more than 48-bit virtual addresses in arm64 linux? This actually means we wont need any set pointer-tagging command and can modify existing implementation. Sounds good? > > > Daniel. ^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [PATCH 0/3 v3] [AArch64] Support tagged pointer 2018-04-24 23:42 ` Omair Javaid @ 2018-04-25 0:09 ` Andrew Pinski 2018-04-25 8:04 ` Daniel Thompson 1 sibling, 0 replies; 40+ messages in thread From: Andrew Pinski @ 2018-04-25 0:09 UTC (permalink / raw) To: Omair Javaid; +Cc: Daniel Thompson, Pedro Alves, Yao Qi, GDB Patches On Tue, Apr 24, 2018 at 4:41 PM, Omair Javaid <omair.javaid@linaro.org> wrote: > On 24 April 2018 at 21:05, Daniel Thompson <daniel.thompson@linaro.org> wrote: >> On Tue, Apr 24, 2018 at 12:48:19PM +0100, Pedro Alves wrote: >>> Hi, >>> >>> On 04/20/2018 03:33 PM, Omair Javaid wrote: >>> >>> > Pointer tagging information is stored in MMU registers so in linux >>> > user-space we cannot actually read if pointer tagging is enabled or not >>> > based on register bits. >>> > JTAG debuggers should be able to read MMU registers and know whether >>> > pointer tagging is enabled or not. >>> > >>> > Rationale behind adding a separate command is to allow other application to >>> > control pointer tagging for example bare-metal (non-linux OSes) which want >>> > to use pointer tagging can enable it. I must admit I dont know of any such >>> > use-case as of now. >>> >>> Alright, that's in line with what I was thinking. Though, bare metal >>> should have access to MMU registers too. Ideally, things would Just Work >>> without user intervention. But I don't mind starting by adding a >>> user-controllable knob, it might be a convenient escape hatch. We can always >>> extend it from "on/off" -> "on/off/auto" setting, with auto the default >>> in future. >> >> For bare metal cases this is not a simple on/off control! >> >> Top byte ignore (a.k.a. pointer tagging) has separate on/off switches >> for TTBR0 (0x0 upwards) and TTBR1 (0xffffffffffff downwards) *and* we >> have to know the respective sizes of TTBR0 and TTBR1 to be sure which >> table we are using. >> >> >>> > Also I am not sure about the timeline of Linux Kernel patches going into >>> > gdb and for now I thought of this command as the most suitable option. >>> > Moreover some users might also be interested in combination where pointer >>> > tagging is enabled but Linux Kernel threads support is disabled so I >>> > thought we should give the control to the user in cases where we cannot >>> > predict use-cases. >>> >>> If everyone agrees that proper Linux kernel support benefits from >>> its own osabi setting/name, then I don't see why we couldn't start by >>> adding the osabi setting as soon as we have a use for it, even if >>> the larger Linux Kernel patches aren't ready yet. >> >> Following on from the above, for aarch64-linux-tdep we can apply domain >> knowledge regarding how things are configured. Here we know that TTBR0 >> is guaranteed to have top byte ignore set, TTBR1 does not *and* we >> also know (from memory-layout.txt) that TTBR0 is sufficiently small >> that bit 55 can be used to discriminate between the two cases. >> >> In others words regardless of whether we are running at EL0 or EL1 then >> I think we should mask the top byte from pointers if and only if bit 55 >> is unset, otherwise leave them as they are. > > What I am understanding here is that you are basing your decision on > the fact that: > > "User addresses have bits 63:48 set to 0 while the kernel addresses have > the same bits set to 1. TTBRx selection is given by bit 63 of the > virtual address." > > Sounds legitimate for now but are we ever going to use more than > 48-bit virtual addresses in arm64 linux? YES. 52bit VA userspace addresses are coming soon. Thanks, Andrew > > This actually means we wont need any set pointer-tagging command and > can modify existing implementation. Sounds good? > >> >> >> Daniel. ^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [PATCH 0/3 v3] [AArch64] Support tagged pointer 2018-04-24 23:42 ` Omair Javaid 2018-04-25 0:09 ` Andrew Pinski @ 2018-04-25 8:04 ` Daniel Thompson 2018-04-26 8:11 ` Omair Javaid 1 sibling, 1 reply; 40+ messages in thread From: Daniel Thompson @ 2018-04-25 8:04 UTC (permalink / raw) To: Omair Javaid; +Cc: Pedro Alves, Yao Qi, GDB Patches On Wed, Apr 25, 2018 at 04:41:40AM +0500, Omair Javaid wrote: > >> If everyone agrees that proper Linux kernel support benefits from > >> its own osabi setting/name, then I don't see why we couldn't start by > >> adding the osabi setting as soon as we have a use for it, even if > >> the larger Linux Kernel patches aren't ready yet. > > > > Following on from the above, for aarch64-linux-tdep we can apply domain > > knowledge regarding how things are configured. Here we know that TTBR0 > > is guaranteed to have top byte ignore set, TTBR1 does not *and* we > > also know (from memory-layout.txt) that TTBR0 is sufficiently small > > that bit 55 can be used to discriminate between the two cases. > > > > In others words regardless of whether we are running at EL0 or EL1 then > > I think we should mask the top byte from pointers if and only if bit 55 > > is unset, otherwise leave them as they are. > > What I am understanding here is that you are basing your decision on > the fact that: > > "User addresses have bits 63:48 set to 0 while the kernel addresses have > the same bits set to 1. TTBRx selection is given by bit 63 of the > virtual address." > > Sounds legitimate for now but are we ever going to use more than > 48-bit virtual addresses in arm64 linux? Almost guaranteed I would have thought! However since the suggestion is *not* based on the assumption that bits 63:48 are zero then I don't think this matters. It is based on the assumption that bits 63:56 are unknown and cannot be used for decision making (because tag 0xff is not reserved) and also that bit 55 is not part of the VA. Bits 54:48 are not involved at all. For 52-bit VAs (and any other number of bits <56) the hueristic remains correct. For 56-bit VAs the pointer tagging feature cannot survive without being changed because with bit 55 allocated there would be no way for the hardware to discriminate between TTBR0 and TTBR1 pointers either. Thus whilst I don't deny the possibility that 56-bit addresses may eventually happen, *any* implementation of pointer tagging support in gdb would need to be updated at that point anyway. Daniel. ^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [PATCH 0/3 v3] [AArch64] Support tagged pointer 2018-04-25 8:04 ` Daniel Thompson @ 2018-04-26 8:11 ` Omair Javaid 2018-04-27 16:29 ` Daniel Thompson 0 siblings, 1 reply; 40+ messages in thread From: Omair Javaid @ 2018-04-26 8:11 UTC (permalink / raw) To: Daniel Thompson; +Cc: Pedro Alves, Yao Qi, GDB Patches On 25 April 2018 at 13:04, Daniel Thompson <daniel.thompson@linaro.org> wrote: > On Wed, Apr 25, 2018 at 04:41:40AM +0500, Omair Javaid wrote: >> >> If everyone agrees that proper Linux kernel support benefits from >> >> its own osabi setting/name, then I don't see why we couldn't start by >> >> adding the osabi setting as soon as we have a use for it, even if >> >> the larger Linux Kernel patches aren't ready yet. >> > >> > Following on from the above, for aarch64-linux-tdep we can apply domain >> > knowledge regarding how things are configured. Here we know that TTBR0 >> > is guaranteed to have top byte ignore set, TTBR1 does not *and* we >> > also know (from memory-layout.txt) that TTBR0 is sufficiently small >> > that bit 55 can be used to discriminate between the two cases. >> > >> > In others words regardless of whether we are running at EL0 or EL1 then >> > I think we should mask the top byte from pointers if and only if bit 55 >> > is unset, otherwise leave them as they are. >> >> What I am understanding here is that you are basing your decision on >> the fact that: >> >> "User addresses have bits 63:48 set to 0 while the kernel addresses have >> the same bits set to 1. TTBRx selection is given by bit 63 of the >> virtual address." >> >> Sounds legitimate for now but are we ever going to use more than >> 48-bit virtual addresses in arm64 linux? > > Almost guaranteed I would have thought! > > However since the suggestion is *not* based on the assumption that bits > 63:48 are zero then I don't think this matters. > > It is based on the assumption that bits 63:56 are unknown and cannot be > used for decision making (because tag 0xff is not reserved) and also > that bit 55 is not part of the VA. Bits 54:48 are not involved at all. > > For 52-bit VAs (and any other number of bits <56) the hueristic remains > correct. > > For 56-bit VAs the pointer tagging feature cannot survive without being > changed because with bit 55 allocated there would be no way for the > hardware to discriminate between TTBR0 and TTBR1 pointers either. Thus > whilst I don't deny the possibility that 56-bit addresses may eventually > happen, *any* implementation of pointer tagging support in gdb would > need to be updated at that point anyway. Above discussion seems to have sufficient points in favor of using bit 55 as the indicator bit for pointer tagging on osabi Linux. While we should also remove tagging support from non linux osabi. Lets wait for a final agreement from Pedro on this. > > > Daniel. ^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [PATCH 0/3 v3] [AArch64] Support tagged pointer 2018-04-26 8:11 ` Omair Javaid @ 2018-04-27 16:29 ` Daniel Thompson 2018-04-30 13:42 ` Omair Javaid 0 siblings, 1 reply; 40+ messages in thread From: Daniel Thompson @ 2018-04-27 16:29 UTC (permalink / raw) To: Omair Javaid; +Cc: Pedro Alves, Yao Qi, GDB Patches On Thu, Apr 26, 2018 at 01:11:04PM +0500, Omair Javaid wrote: > On 25 April 2018 at 13:04, Daniel Thompson <daniel.thompson@linaro.org> wrote: > > On Wed, Apr 25, 2018 at 04:41:40AM +0500, Omair Javaid wrote: > >> >> If everyone agrees that proper Linux kernel support benefits from > >> >> its own osabi setting/name, then I don't see why we couldn't start by > >> >> adding the osabi setting as soon as we have a use for it, even if > >> >> the larger Linux Kernel patches aren't ready yet. > >> > > >> > Following on from the above, for aarch64-linux-tdep we can apply domain > >> > knowledge regarding how things are configured. Here we know that TTBR0 > >> > is guaranteed to have top byte ignore set, TTBR1 does not *and* we > >> > also know (from memory-layout.txt) that TTBR0 is sufficiently small > >> > that bit 55 can be used to discriminate between the two cases. > >> > > >> > In others words regardless of whether we are running at EL0 or EL1 then > >> > I think we should mask the top byte from pointers if and only if bit 55 > >> > is unset, otherwise leave them as they are. > >> > >> What I am understanding here is that you are basing your decision on > >> the fact that: > >> > >> "User addresses have bits 63:48 set to 0 while the kernel addresses have > >> the same bits set to 1. TTBRx selection is given by bit 63 of the > >> virtual address." > >> > >> Sounds legitimate for now but are we ever going to use more than > >> 48-bit virtual addresses in arm64 linux? > > > > Almost guaranteed I would have thought! > > > > However since the suggestion is *not* based on the assumption that bits > > 63:48 are zero then I don't think this matters. > > > > It is based on the assumption that bits 63:56 are unknown and cannot be > > used for decision making (because tag 0xff is not reserved) and also > > that bit 55 is not part of the VA. Bits 54:48 are not involved at all. > > > > For 52-bit VAs (and any other number of bits <56) the hueristic remains > > correct. > > > > For 56-bit VAs the pointer tagging feature cannot survive without being > > changed because with bit 55 allocated there would be no way for the > > hardware to discriminate between TTBR0 and TTBR1 pointers either. Thus > > whilst I don't deny the possibility that 56-bit addresses may eventually > > happen, *any* implementation of pointer tagging support in gdb would > > need to be updated at that point anyway. > > Above discussion seems to have sufficient points in favor of using bit > 55 as the indicator bit for pointer tagging on osabi Linux. When you say "use as indicator bit" is it not clear if you are still considering modal behaviour (pointer tagging is "on" or "off" based on bit 55 of one of the pointer registers) or whether you hope to move the masking function into the tdep code (so instead of saying all pointers at 56-bit, you can say all pointers need filtering in this manner). Conditional masking based on bit 55 is idempotent so there's no risk even if the filtering is applied multiple times at different places in gdb! > While we should also remove tagging support from non linux osabi. Agree. This is perhaps more urgent since then at least a non-Linux gdb can be used to debug the kernel. As things stand today no gdb-8.1 version can be used for AArch64 kernel debugging. Daniel. ^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [PATCH 0/3 v3] [AArch64] Support tagged pointer 2018-04-27 16:29 ` Daniel Thompson @ 2018-04-30 13:42 ` Omair Javaid 0 siblings, 0 replies; 40+ messages in thread From: Omair Javaid @ 2018-04-30 13:42 UTC (permalink / raw) To: Daniel Thompson; +Cc: Pedro Alves, Yao Qi, GDB Patches On 27 April 2018 at 21:28, Daniel Thompson <daniel.thompson@linaro.org> wrote: > On Thu, Apr 26, 2018 at 01:11:04PM +0500, Omair Javaid wrote: >> On 25 April 2018 at 13:04, Daniel Thompson <daniel.thompson@linaro.org> wrote: >> > On Wed, Apr 25, 2018 at 04:41:40AM +0500, Omair Javaid wrote: >> >> >> If everyone agrees that proper Linux kernel support benefits from >> >> >> its own osabi setting/name, then I don't see why we couldn't start by >> >> >> adding the osabi setting as soon as we have a use for it, even if >> >> >> the larger Linux Kernel patches aren't ready yet. >> >> > >> >> > Following on from the above, for aarch64-linux-tdep we can apply domain >> >> > knowledge regarding how things are configured. Here we know that TTBR0 >> >> > is guaranteed to have top byte ignore set, TTBR1 does not *and* we >> >> > also know (from memory-layout.txt) that TTBR0 is sufficiently small >> >> > that bit 55 can be used to discriminate between the two cases. >> >> > >> >> > In others words regardless of whether we are running at EL0 or EL1 then >> >> > I think we should mask the top byte from pointers if and only if bit 55 >> >> > is unset, otherwise leave them as they are. >> >> >> >> What I am understanding here is that you are basing your decision on >> >> the fact that: >> >> >> >> "User addresses have bits 63:48 set to 0 while the kernel addresses have >> >> the same bits set to 1. TTBRx selection is given by bit 63 of the >> >> virtual address." >> >> >> >> Sounds legitimate for now but are we ever going to use more than >> >> 48-bit virtual addresses in arm64 linux? >> > >> > Almost guaranteed I would have thought! >> > >> > However since the suggestion is *not* based on the assumption that bits >> > 63:48 are zero then I don't think this matters. >> > >> > It is based on the assumption that bits 63:56 are unknown and cannot be >> > used for decision making (because tag 0xff is not reserved) and also >> > that bit 55 is not part of the VA. Bits 54:48 are not involved at all. >> > >> > For 52-bit VAs (and any other number of bits <56) the hueristic remains >> > correct. >> > >> > For 56-bit VAs the pointer tagging feature cannot survive without being >> > changed because with bit 55 allocated there would be no way for the >> > hardware to discriminate between TTBR0 and TTBR1 pointers either. Thus >> > whilst I don't deny the possibility that 56-bit addresses may eventually >> > happen, *any* implementation of pointer tagging support in gdb would >> > need to be updated at that point anyway. >> >> Above discussion seems to have sufficient points in favor of using bit >> 55 as the indicator bit for pointer tagging on osabi Linux. > > When you say "use as indicator bit" is it not clear if you are still > considering modal behaviour (pointer tagging is "on" or "off" based on > bit 55 of one of the pointer registers) or whether you hope to move the > masking function into the tdep code (so instead of saying all pointers > at 56-bit, you can say all pointers need filtering in this manner). > > Conditional masking based on bit 55 is idempotent so there's no risk > even if the filtering is applied multiple times at different places in > gdb! > > >> While we should also remove tagging support from non linux osabi. > > Agree. > > This is perhaps more urgent since then at least a non-Linux gdb > can be used to debug the kernel. As things stand today no gdb-8.1 > version can be used for AArch64 kernel debugging. I have written a patch for this issue will be posting it on for review after testing. Approach i am using is to sign extend address based on 55th bit after clearing top byte. Clearing pointer tagging from aarch64-elf and only enabling it for aarch64-linux. > > > Daniel. ^ permalink raw reply [flat|nested] 40+ messages in thread
end of thread, other threads:[~2018-04-30 13:42 UTC | newest] Thread overview: 40+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2017-12-08 10:04 [PATCH 0/3 v3] [AArch64] Support tagged pointer Yao Qi 2017-12-08 10:04 ` [PATCH 2/3] Adjust breakpoint address by clearing non-significant bits Yao Qi 2017-12-08 12:22 ` Pedro Alves 2017-12-08 10:04 ` [PATCH 1/3] Clear non-significant bits of address on memory access Yao Qi 2017-12-08 12:22 ` Pedro Alves 2017-12-08 15:13 ` Ulrich Weigand 2017-12-08 15:36 ` Yao Qi 2017-12-19 13:50 ` Ulrich Weigand 2017-12-19 15:41 ` Yao Qi 2017-12-19 16:15 ` Ulrich Weigand 2017-12-20 9:57 ` Yao Qi 2017-12-20 13:03 ` [pushed] Fix Cell/B.E. regression (Re: [PATCH 1/3] Clear non-significant bits of address on memory access) Ulrich Weigand 2017-12-20 13:59 ` Yao Qi 2017-12-08 10:04 ` [PATCH 3/3] Clear non-significant bits of address in watchpoint Yao Qi 2017-12-08 12:23 ` Pedro Alves 2017-12-08 12:24 ` [PATCH 0/3 v3] [AArch64] Support tagged pointer Pedro Alves 2017-12-08 17:31 ` Yao Qi 2018-04-11 0:16 ` Omair Javaid 2018-04-11 0:37 ` Omair Javaid 2018-04-11 2:46 ` Simon Marchi 2018-04-11 10:14 ` Pedro Alves 2018-04-11 11:13 ` Omair Javaid 2018-04-11 11:19 ` Pedro Alves 2018-04-11 12:01 ` Omair Javaid 2018-04-11 18:27 ` Pedro Alves 2018-04-16 1:36 ` Omair Javaid 2018-04-16 22:57 ` Pedro Alves 2018-04-20 14:34 ` Omair Javaid 2018-04-20 16:13 ` Daniel Thompson 2018-04-23 7:50 ` Omair Javaid 2018-04-24 11:39 ` Pedro Alves 2018-04-24 15:44 ` Daniel Thompson 2018-04-24 11:48 ` Pedro Alves 2018-04-24 16:05 ` Daniel Thompson 2018-04-24 23:42 ` Omair Javaid 2018-04-25 0:09 ` Andrew Pinski 2018-04-25 8:04 ` Daniel Thompson 2018-04-26 8:11 ` Omair Javaid 2018-04-27 16:29 ` Daniel Thompson 2018-04-30 13:42 ` Omair Javaid
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).