| Message ID | 1525138292-4526-1-git-send-email-omair.javaid@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | [PR,gdb/23127,AArch64] Fix tagged pointer support | expand |
On 01/05/18 02:31, Omair Javaid wrote: > This patch fixes tagged pointer support for AArch64 GDB. Linux kernel debugging > failure was reported after tagged pointer support was committed. > > After a discussion around best path forward to manage tagged pointers on GDB > side we are going to disable tagged pointers support for aarch64-none-elf-gdb > because for non-linux applications we cant be sure if tagged pointers will be > used by MMU or not. > > Also for aarch64-linux-gdb we are going to sign extend user-space address after > clearing tag bits. This will help us debug both kernel and user-space addresses > based on information from linux kernel documentation given below: > > According to AArch64 memory map: > 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." > > According to AArch64 tagged pointers document: > 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. > > Running gdb testsuite after applying this patch introduces no regressions and > tagged pointer test cases still pass. ... and I kicked the tyres a little bit using kgdb. print worked as expected, backtrace no longer provokes a gdb panic and breakpoints work (albeit for rather approximate definition of work... and the need for approximation is not gdb's fault). Daniel. > gdb/ChangeLog: > 2018-05-01 Omair Javaid <omair.javaid@linaro.org> > > * aarch64-linux-tdep.c (aarch64_linux_init_abi): Add call to > set_gdbarch_significant_addr_bit. > * aarch64-tdep.c (aarch64_gdbarch_init): Remove call to > set_gdbarch_significant_addr_bit. > * utils.c (address_significant): Update to sign extend addr. > --- > gdb/aarch64-linux-tdep.c | 5 +++++ > gdb/aarch64-tdep.c | 5 ----- > gdb/utils.c | 14 +++++++++----- > 3 files changed, 14 insertions(+), 10 deletions(-) > > diff --git a/gdb/aarch64-linux-tdep.c b/gdb/aarch64-linux-tdep.c > index 1f3e888..ba5757d 100644 > --- a/gdb/aarch64-linux-tdep.c > +++ b/gdb/aarch64-linux-tdep.c > @@ -1062,6 +1062,11 @@ aarch64_linux_init_abi (struct gdbarch_info info, struct gdbarch *gdbarch) > /* Syscall record. */ > tdep->aarch64_syscall_record = aarch64_linux_syscall_record; > > + /* The top byte of a user space address known as the "tag", > + is ignored by the kernel and can be regarded as additional > + data associated with the address. */ > + set_gdbarch_significant_addr_bit (gdbarch, 56); > + > /* Initialize the aarch64_linux_record_tdep. */ > /* These values are the size of the type that will be used in a system > call. They are obtained from Linux Kernel source. */ > diff --git a/gdb/aarch64-tdep.c b/gdb/aarch64-tdep.c > index 01566b4..3c1f389 100644 > --- a/gdb/aarch64-tdep.c > +++ b/gdb/aarch64-tdep.c > @@ -2972,11 +2972,6 @@ aarch64_gdbarch_init (struct gdbarch_info info, struct gdbarch_list *arches) > set_tdesc_pseudo_register_reggroup_p (gdbarch, > aarch64_pseudo_register_reggroup_p); > > - /* The top byte of an address is known as the "tag" and is > - 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); > - > /* ABI */ > set_gdbarch_short_bit (gdbarch, 16); > set_gdbarch_int_bit (gdbarch, 32); > diff --git a/gdb/utils.c b/gdb/utils.c > index b957b0d..1f9be8f 100644 > --- a/gdb/utils.c > +++ b/gdb/utils.c > @@ -2704,14 +2704,18 @@ When set, debugging messages will be marked with seconds and microseconds."), > 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. */ > + /* Clear insignificant bits of a target address and sign extend resulting > + 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; > + { > + CORE_ADDR sign = (CORE_ADDR) 1 << (addr_bit - 1); > + addr &= ((CORE_ADDR) 1 << addr_bit) - 1; > + addr = (addr ^ sign) - sign; > + } > > return addr; > } >
On 1 May 2018 at 20:02, Daniel Thompson <daniel.thompson@linaro.org> wrote: > On 01/05/18 02:31, Omair Javaid wrote: >> >> This patch fixes tagged pointer support for AArch64 GDB. Linux kernel >> debugging >> failure was reported after tagged pointer support was committed. >> >> After a discussion around best path forward to manage tagged pointers on >> GDB >> side we are going to disable tagged pointers support for >> aarch64-none-elf-gdb >> because for non-linux applications we cant be sure if tagged pointers will >> be >> used by MMU or not. >> >> Also for aarch64-linux-gdb we are going to sign extend user-space address >> after >> clearing tag bits. This will help us debug both kernel and user-space >> addresses >> based on information from linux kernel documentation given below: >> >> According to AArch64 memory map: >> 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." >> >> According to AArch64 tagged pointers document: >> 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. >> >> Running gdb testsuite after applying this patch introduces no regressions >> and >> tagged pointer test cases still pass. > > > ... and I kicked the tyres a little bit using kgdb. > > print worked as expected, backtrace no longer provokes a gdb panic and > breakpoints work (albeit for rather approximate definition of work... and > the need for approximation is not gdb's fault). > > > Daniel. > > > >> gdb/ChangeLog: >> 2018-05-01 Omair Javaid <omair.javaid@linaro.org> >> >> * aarch64-linux-tdep.c (aarch64_linux_init_abi): Add call to >> set_gdbarch_significant_addr_bit. >> * aarch64-tdep.c (aarch64_gdbarch_init): Remove call to >> set_gdbarch_significant_addr_bit. >> * utils.c (address_significant): Update to sign extend addr. >> --- >> gdb/aarch64-linux-tdep.c | 5 +++++ >> gdb/aarch64-tdep.c | 5 ----- >> gdb/utils.c | 14 +++++++++----- >> 3 files changed, 14 insertions(+), 10 deletions(-) >> >> diff --git a/gdb/aarch64-linux-tdep.c b/gdb/aarch64-linux-tdep.c >> index 1f3e888..ba5757d 100644 >> --- a/gdb/aarch64-linux-tdep.c >> +++ b/gdb/aarch64-linux-tdep.c >> @@ -1062,6 +1062,11 @@ aarch64_linux_init_abi (struct gdbarch_info info, >> struct gdbarch *gdbarch) >> /* Syscall record. */ >> tdep->aarch64_syscall_record = aarch64_linux_syscall_record; >> + /* The top byte of a user space address known as the "tag", >> + is ignored by the kernel and can be regarded as additional >> + data associated with the address. */ >> + set_gdbarch_significant_addr_bit (gdbarch, 56); >> + >> /* Initialize the aarch64_linux_record_tdep. */ >> /* These values are the size of the type that will be used in a system >> call. They are obtained from Linux Kernel source. */ >> diff --git a/gdb/aarch64-tdep.c b/gdb/aarch64-tdep.c >> index 01566b4..3c1f389 100644 >> --- a/gdb/aarch64-tdep.c >> +++ b/gdb/aarch64-tdep.c >> @@ -2972,11 +2972,6 @@ aarch64_gdbarch_init (struct gdbarch_info info, >> struct gdbarch_list *arches) >> set_tdesc_pseudo_register_reggroup_p (gdbarch, >> >> aarch64_pseudo_register_reggroup_p); >> - /* The top byte of an address is known as the "tag" and is >> - 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); >> - >> /* ABI */ >> set_gdbarch_short_bit (gdbarch, 16); >> set_gdbarch_int_bit (gdbarch, 32); >> diff --git a/gdb/utils.c b/gdb/utils.c >> index b957b0d..1f9be8f 100644 >> --- a/gdb/utils.c >> +++ b/gdb/utils.c >> @@ -2704,14 +2704,18 @@ When set, debugging messages will be marked with >> seconds and microseconds."), >> 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. */ >> + /* Clear insignificant bits of a target address and sign extend >> resulting >> + 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; >> + { >> + CORE_ADDR sign = (CORE_ADDR) 1 << (addr_bit - 1); >> + addr &= ((CORE_ADDR) 1 << addr_bit) - 1; >> + addr = (addr ^ sign) - sign; >> + } >> return addr; >> } >> > Ping! Hi Pedro, I was wondering if you can kindly help review this patch. This is a critical bug as it blocks kernel debugging on AArch64. Also can we push this to GDB 8.1.1 once it gets accepted? Thanks!
Hello, Thanks much for all the investigation & discussion, both of you. On 05/08/2018 10:31 AM, Omair Javaid wrote: > I was wondering if you can kindly help review this patch. > This is a critical bug as it blocks kernel debugging on AArch64. > Also can we push this to GDB 8.1.1 once it gets accepted? Yes, this is OK for both master and branch. Please add "PR gdb/23127" to the ChangeLog before pushing. Thanks, Pedro Alves
> > I was wondering if you can kindly help review this patch. > > This is a critical bug as it blocks kernel debugging on AArch64. > > Also can we push this to GDB 8.1.1 once it gets accepted? > > Yes, this is OK for both master and branch. > > Please add "PR gdb/23127" to the ChangeLog before pushing. And I've added 8.1.1 as the target milestone in the GDB PR as well. -- Joel
On Wed, May 9, 2018 at 7:19 AM, Pedro Alves <palves@redhat.com> wrote: > Hello, > > Thanks much for all the investigation & discussion, both of you. > > On 05/08/2018 10:31 AM, Omair Javaid wrote: > >> I was wondering if you can kindly help review this patch. >> This is a critical bug as it blocks kernel debugging on AArch64. >> Also can we push this to GDB 8.1.1 once it gets accepted? > > Yes, this is OK for both master and branch. > > Please add "PR gdb/23127" to the ChangeLog before pushing. > This breaks gdb on x86-64: https://sourceware.org/bugzilla/show_bug.cgi?id=23210 -- H.J.
On Mon, 21 May 2018, 9:06 PM H.J. Lu, <hjl.tools@gmail.com> wrote: > On Wed, May 9, 2018 at 7:19 AM, Pedro Alves <palves@redhat.com> wrote: > > Hello, > > > > Thanks much for all the investigation & discussion, both of you. > > > > On 05/08/2018 10:31 AM, Omair Javaid wrote: > > > >> I was wondering if you can kindly help review this patch. > >> This is a critical bug as it blocks kernel debugging on AArch64. > >> Also can we push this to GDB 8.1.1 once it gets accepted? > > > > Yes, this is OK for both master and branch. > > > > Please add "PR gdb/23127" to the ChangeLog before pushing. > > > > This breaks gdb on x86-64: > > https://sourceware.org/bugzilla/show_bug.cgi?id=23210 > > > Hmm. I ll check it out. Thanks for reporting. -- > H.J. >
On Mon, May 21, 2018 at 9:23 AM, Omair Javaid <omair.javaid@linaro.org> wrote: > > > On Mon, 21 May 2018, 9:06 PM H.J. Lu, <hjl.tools@gmail.com> wrote: >> >> On Wed, May 9, 2018 at 7:19 AM, Pedro Alves <palves@redhat.com> wrote: >> > Hello, >> > >> > Thanks much for all the investigation & discussion, both of you. >> > >> > On 05/08/2018 10:31 AM, Omair Javaid wrote: >> > >> >> I was wondering if you can kindly help review this patch. >> >> This is a critical bug as it blocks kernel debugging on AArch64. >> >> Also can we push this to GDB 8.1.1 once it gets accepted? >> > >> > Yes, this is OK for both master and branch. >> > >> > Please add "PR gdb/23127" to the ChangeLog before pushing. >> > >> >> This breaks gdb on x86-64: >> >> https://sourceware.org/bugzilla/show_bug.cgi?id=23210 >> >> > > Hmm. I ll check it out. Thanks for reporting. > Please fix both master and gdb-8.1-branch branches Thanks. -- H.J.
On Monday, May 21 2018, Omair Javaid wrote: > On Mon, 21 May 2018, 9:06 PM H.J. Lu, <hjl.tools@gmail.com> wrote: > >> On Wed, May 9, 2018 at 7:19 AM, Pedro Alves <palves@redhat.com> wrote: >> > Hello, >> > >> > Thanks much for all the investigation & discussion, both of you. >> > >> > On 05/08/2018 10:31 AM, Omair Javaid wrote: >> > >> >> I was wondering if you can kindly help review this patch. >> >> This is a critical bug as it blocks kernel debugging on AArch64. >> >> Also can we push this to GDB 8.1.1 once it gets accepted? >> > >> > Yes, this is OK for both master and branch. >> > >> > Please add "PR gdb/23127" to the ChangeLog before pushing. >> > >> >> This breaks gdb on x86-64: >> >> https://sourceware.org/bugzilla/show_bug.cgi?id=23210 >> >> >> > Hmm. I ll check it out. Thanks for reporting. Hi Omair, Any news on this failure? It is causing the testcase to take more than 14 hours to run when using --target_board=unix/-m32, and is impacting some of our builders on BuildBot. Thanks, -- Sergio GPG key ID: 237A 54B1 0287 28BF 00EF 31F4 D0EB 7628 65FC 5E36 Please send encrypted e-mail if possible http://sergiodj.net/
On Friday, May 25 2018, I wrote: > On Monday, May 21 2018, Omair Javaid wrote: > >> On Mon, 21 May 2018, 9:06 PM H.J. Lu, <hjl.tools@gmail.com> wrote: >> >>> On Wed, May 9, 2018 at 7:19 AM, Pedro Alves <palves@redhat.com> wrote: >>> > Hello, >>> > >>> > Thanks much for all the investigation & discussion, both of you. >>> > >>> > On 05/08/2018 10:31 AM, Omair Javaid wrote: >>> > >>> >> I was wondering if you can kindly help review this patch. >>> >> This is a critical bug as it blocks kernel debugging on AArch64. >>> >> Also can we push this to GDB 8.1.1 once it gets accepted? >>> > >>> > Yes, this is OK for both master and branch. >>> > >>> > Please add "PR gdb/23127" to the ChangeLog before pushing. >>> > >>> >>> This breaks gdb on x86-64: >>> >>> https://sourceware.org/bugzilla/show_bug.cgi?id=23210 >>> >>> >>> >> Hmm. I ll check it out. Thanks for reporting. > > Hi Omair, > > Any news on this failure? It is causing the testcase to take more than > 14 hours to run when using --target_board=unix/-m32, and is impacting > some of our builders on BuildBot. Apparently you sent a fix already. Sorry about this message. -- Sergio GPG key ID: 237A 54B1 0287 28BF 00EF 31F4 D0EB 7628 65FC 5E36 Please send encrypted e-mail if possible http://sergiodj.net/
On Fri, May 25, 2018 at 10:11 AM, Sergio Durigan Junior <sergiodj@redhat.com> wrote: > On Friday, May 25 2018, I wrote: > >> On Monday, May 21 2018, Omair Javaid wrote: >> >>> On Mon, 21 May 2018, 9:06 PM H.J. Lu, <hjl.tools@gmail.com> wrote: >>> >>>> On Wed, May 9, 2018 at 7:19 AM, Pedro Alves <palves@redhat.com> wrote: >>>> > Hello, >>>> > >>>> > Thanks much for all the investigation & discussion, both of you. >>>> > >>>> > On 05/08/2018 10:31 AM, Omair Javaid wrote: >>>> > >>>> >> I was wondering if you can kindly help review this patch. >>>> >> This is a critical bug as it blocks kernel debugging on AArch64. >>>> >> Also can we push this to GDB 8.1.1 once it gets accepted? >>>> > >>>> > Yes, this is OK for both master and branch. >>>> > >>>> > Please add "PR gdb/23127" to the ChangeLog before pushing. >>>> > >>>> >>>> This breaks gdb on x86-64: >>>> >>>> https://sourceware.org/bugzilla/show_bug.cgi?id=23210 >>>> >>>> >>>> >>> Hmm. I ll check it out. Thanks for reporting. >> >> Hi Omair, >> >> Any news on this failure? It is causing the testcase to take more than >> 14 hours to run when using --target_board=unix/-m32, and is impacting >> some of our builders on BuildBot. > > Apparently you sent a fix already. Sorry about this message. > But the fix is wrong. -- H.J.
On Fri, 25 May 2018, 10:12 PM H.J. Lu, <hjl.tools@gmail.com> wrote: > On Fri, May 25, 2018 at 10:11 AM, Sergio Durigan Junior > <sergiodj@redhat.com> wrote: > > On Friday, May 25 2018, I wrote: > > > >> On Monday, May 21 2018, Omair Javaid wrote: > >> > >>> On Mon, 21 May 2018, 9:06 PM H.J. Lu, <hjl.tools@gmail.com> wrote: > >>> > >>>> On Wed, May 9, 2018 at 7:19 AM, Pedro Alves <palves@redhat.com> > wrote: > >>>> > Hello, > >>>> > > >>>> > Thanks much for all the investigation & discussion, both of you. > >>>> > > >>>> > On 05/08/2018 10:31 AM, Omair Javaid wrote: > >>>> > > >>>> >> I was wondering if you can kindly help review this patch. > >>>> >> This is a critical bug as it blocks kernel debugging on AArch64. > >>>> >> Also can we push this to GDB 8.1.1 once it gets accepted? > >>>> > > >>>> > Yes, this is OK for both master and branch. > >>>> > > >>>> > Please add "PR gdb/23127" to the ChangeLog before pushing. > >>>> > > >>>> > >>>> This breaks gdb on x86-64: > >>>> > >>>> https://sourceware.org/bugzilla/show_bug.cgi?id=23210 > >>>> > >>>> > >>>> > >>> Hmm. I ll check it out. Thanks for reporting. > >> > >> Hi Omair, > >> > >> Any news on this failure? It is causing the testcase to take more than > >> 14 hours to run when using --target_board=unix/-m32, and is impacting > >> some of our builders on BuildBot. > > > > Apparently you sent a fix already. Sorry about this message. > > > > But the fix is wrong. > I will update gdbarch as suggested. Are there any other issues in the fix I should address.? > > -- > H.J. >
On Friday, May 25 2018, Omair Javaid wrote: >> But the fix is wrong. >> > > I will update gdbarch as suggested. Are there any other issues in the fix I > should address.? I think we should discuss that on the thread. -- Sergio GPG key ID: 237A 54B1 0287 28BF 00EF 31F4 D0EB 7628 65FC 5E36 Please send encrypted e-mail if possible http://sergiodj.net/
diff --git a/gdb/aarch64-linux-tdep.c b/gdb/aarch64-linux-tdep.c index 1f3e888..ba5757d 100644 --- a/gdb/aarch64-linux-tdep.c +++ b/gdb/aarch64-linux-tdep.c @@ -1062,6 +1062,11 @@ aarch64_linux_init_abi (struct gdbarch_info info, struct gdbarch *gdbarch) /* Syscall record. */ tdep->aarch64_syscall_record = aarch64_linux_syscall_record; + /* The top byte of a user space address known as the "tag", + is ignored by the kernel and can be regarded as additional + data associated with the address. */ + set_gdbarch_significant_addr_bit (gdbarch, 56); + /* Initialize the aarch64_linux_record_tdep. */ /* These values are the size of the type that will be used in a system call. They are obtained from Linux Kernel source. */ diff --git a/gdb/aarch64-tdep.c b/gdb/aarch64-tdep.c index 01566b4..3c1f389 100644 --- a/gdb/aarch64-tdep.c +++ b/gdb/aarch64-tdep.c @@ -2972,11 +2972,6 @@ aarch64_gdbarch_init (struct gdbarch_info info, struct gdbarch_list *arches) set_tdesc_pseudo_register_reggroup_p (gdbarch, aarch64_pseudo_register_reggroup_p); - /* The top byte of an address is known as the "tag" and is - 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); - /* ABI */ set_gdbarch_short_bit (gdbarch, 16); set_gdbarch_int_bit (gdbarch, 32); diff --git a/gdb/utils.c b/gdb/utils.c index b957b0d..1f9be8f 100644 --- a/gdb/utils.c +++ b/gdb/utils.c @@ -2704,14 +2704,18 @@ When set, debugging messages will be marked with seconds and microseconds."), 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. */ + /* Clear insignificant bits of a target address and sign extend resulting + 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; + { + CORE_ADDR sign = (CORE_ADDR) 1 << (addr_bit - 1); + addr &= ((CORE_ADDR) 1 << addr_bit) - 1; + addr = (addr ^ sign) - sign; + } return addr; }