| Message ID | 1527023088-10837-1-git-send-email-omair.javaid@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | [PR,gdb/23210] Unset gdbarch significant_addr_bit by default | expand |
Hi Omair, > This patch fixes a bug introduced by fix to AArch64 pointer tagging. > > In our fix for tagged pointer support our agreed approach was to sign > extend user-space address after clearing tag bits. This is not same > for all architectures and this patch allows sign extension for > addresses on targets which specifically set significant_addr_bit. > > More information about patch that caused the issues and discussion > around tagged pointer support can be found in links below: > > https://sourceware.org/ml/gdb-patches/2018-05/msg00000.html > https://sourceware.org/ml/gdb-patches/2017-12/msg00159.html > > gdb/ChangeLog: > > 2018-05-23 Omair Javaid <omair.javaid@linaro.org> > > * gdbarch.c (verify_gdbarch): Update. > * utils.c (address_significant): Update. I haven't delved into the actual patch and whether the approach used is correct, but skimming it, I did notice a couple of things. The first one is that gdbarch.c is a generated file, so you should adjust gdbarch.sh instead so that executing gdbarch.sh gives you the gdbarch.c file with the behavior you want. In particular, I think you probably need to remove the default value for significant_addr_bit. > --- > gdb/gdbarch.c | 3 +-- > gdb/utils.c | 5 +++-- > 2 files changed, 4 insertions(+), 4 deletions(-) > > diff --git a/gdb/gdbarch.c b/gdb/gdbarch.c > index c430ebe..5593911 100644 > --- a/gdb/gdbarch.c > +++ b/gdb/gdbarch.c > @@ -615,8 +615,7 @@ verify_gdbarch (struct gdbarch *gdbarch) > /* Skip verify of stabs_argument_has_addr, invalid_p == 0 */ > /* Skip verify of convert_from_func_ptr_addr, invalid_p == 0 */ > /* Skip verify of addr_bits_remove, invalid_p == 0 */ > - if (gdbarch->significant_addr_bit == 0) > - gdbarch->significant_addr_bit = gdbarch_addr_bit (gdbarch); > + /* Skip verify of significant_addr_bit, invalid_p == 0 */ > /* Skip verify of software_single_step, has predicate. */ > /* Skip verify of single_step_through_delay, has predicate. */ > /* Skip verify of print_insn, invalid_p == 0 */ > diff --git a/gdb/utils.c b/gdb/utils.c > index 9c5bf68..91c0f2b 100644 > --- a/gdb/utils.c > +++ b/gdb/utils.c > @@ -2708,10 +2708,11 @@ address_significant (gdbarch *gdbarch, CORE_ADDR addr) > /* 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. */ > + when it won't occur. Skip updating of target address if current target has > + not set gdbarch significant_addr_bit. */ Small nit (GNU Coding Style): Two spaces after the period. > int addr_bit = gdbarch_significant_addr_bit (gdbarch); > > - if (addr_bit < (sizeof (CORE_ADDR) * HOST_CHAR_BIT)) > + if (addr_bit && (addr_bit < (sizeof (CORE_ADDR) * HOST_CHAR_BIT))) > { > CORE_ADDR sign = (CORE_ADDR) 1 << (addr_bit - 1); > addr &= ((CORE_ADDR) 1 << addr_bit) - 1; > -- > 2.7.4 -- Joel
On Wed, 23 May 2018, 3:32 PM Joel Brobecker, <brobecker@adacore.com> wrote: > Hi Omair, > > > > This patch fixes a bug introduced by fix to AArch64 pointer tagging. > > > > In our fix for tagged pointer support our agreed approach was to sign > > extend user-space address after clearing tag bits. This is not same > > for all architectures and this patch allows sign extension for > > addresses on targets which specifically set significant_addr_bit. > > > > More information about patch that caused the issues and discussion > > around tagged pointer support can be found in links below: > > > > https://sourceware.org/ml/gdb-patches/2018-05/msg00000.html > > https://sourceware.org/ml/gdb-patches/2017-12/msg00159.html > > > > gdb/ChangeLog: > > > > 2018-05-23 Omair Javaid <omair.javaid@linaro.org> > > > > * gdbarch.c (verify_gdbarch): Update. > > * utils.c (address_significant): Update. > > I haven't delved into the actual patch and whether the approach > used is correct, but skimming it, I did notice a couple of things. > > The first one is that gdbarch.c is a generated file, so you should > adjust gdbarch.sh instead so that executing gdbarch.sh gives you > the gdbarch.c file with the behavior you want. In particular, I think > you probably need to remove the default value for significant_addr_bit. > I will update gdbarch as suggested. Are there any other issues in the fix I should address.? > > > --- > > gdb/gdbarch.c | 3 +-- > > gdb/utils.c | 5 +++-- > > 2 files changed, 4 insertions(+), 4 deletions(-) > > > > diff --git a/gdb/gdbarch.c b/gdb/gdbarch.c > > index c430ebe..5593911 100644 > > --- a/gdb/gdbarch.c > > +++ b/gdb/gdbarch.c > > @@ -615,8 +615,7 @@ verify_gdbarch (struct gdbarch *gdbarch) > > /* Skip verify of stabs_argument_has_addr, invalid_p == 0 */ > > /* Skip verify of convert_from_func_ptr_addr, invalid_p == 0 */ > > /* Skip verify of addr_bits_remove, invalid_p == 0 */ > > - if (gdbarch->significant_addr_bit == 0) > > - gdbarch->significant_addr_bit = gdbarch_addr_bit (gdbarch); > > + /* Skip verify of significant_addr_bit, invalid_p == 0 */ > > /* Skip verify of software_single_step, has predicate. */ > > /* Skip verify of single_step_through_delay, has predicate. */ > > /* Skip verify of print_insn, invalid_p == 0 */ > > diff --git a/gdb/utils.c b/gdb/utils.c > > index 9c5bf68..91c0f2b 100644 > > --- a/gdb/utils.c > > +++ b/gdb/utils.c > > @@ -2708,10 +2708,11 @@ address_significant (gdbarch *gdbarch, CORE_ADDR > addr) > > /* 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. */ > > + when it won't occur. Skip updating of target address if current > target has > > + not set gdbarch significant_addr_bit. */ > > Small nit (GNU Coding Style): Two spaces after the period. > > > int addr_bit = gdbarch_significant_addr_bit (gdbarch); > > > > - if (addr_bit < (sizeof (CORE_ADDR) * HOST_CHAR_BIT)) > > + if (addr_bit && (addr_bit < (sizeof (CORE_ADDR) * HOST_CHAR_BIT))) > > { > > CORE_ADDR sign = (CORE_ADDR) 1 << (addr_bit - 1); > > addr &= ((CORE_ADDR) 1 << addr_bit) - 1; > > -- > > 2.7.4 > > -- > Joel >
On 05/25/2018 06:18 PM, Omair Javaid wrote: > I will update gdbarch as suggested. Are there any other issues in the fix I > should address.? I'm not sure if it will ever make sense for other ports to sign extend the address, but it doesn't hurt to go with this until we find some other need. Thanks, Pedro Alves
On Tuesday, May 22 2018, Omair Javaid wrote: > This patch fixes a bug introduced by fix to AArch64 pointer tagging. > > In our fix for tagged pointer support our agreed approach was to sign > extend user-space address after clearing tag bits. This is not same > for all architectures and this patch allows sign extension for > addresses on targets which specifically set significant_addr_bit. > > More information about patch that caused the issues and discussion > around tagged pointer support can be found in links below: > > https://sourceware.org/ml/gdb-patches/2018-05/msg00000.html > https://sourceware.org/ml/gdb-patches/2017-12/msg00159.html FWIW, I've tested your patch locally and it fixes the timeout issues I've been seeing when debugging 32-bit binaries with a 64-bit GDB. Thanks. > gdb/ChangeLog: > > 2018-05-23 Omair Javaid <omair.javaid@linaro.org> > > * gdbarch.c (verify_gdbarch): Update. > * utils.c (address_significant): Update. > --- > gdb/gdbarch.c | 3 +-- > gdb/utils.c | 5 +++-- > 2 files changed, 4 insertions(+), 4 deletions(-) > > diff --git a/gdb/gdbarch.c b/gdb/gdbarch.c > index c430ebe..5593911 100644 > --- a/gdb/gdbarch.c > +++ b/gdb/gdbarch.c > @@ -615,8 +615,7 @@ verify_gdbarch (struct gdbarch *gdbarch) > /* Skip verify of stabs_argument_has_addr, invalid_p == 0 */ > /* Skip verify of convert_from_func_ptr_addr, invalid_p == 0 */ > /* Skip verify of addr_bits_remove, invalid_p == 0 */ > - if (gdbarch->significant_addr_bit == 0) > - gdbarch->significant_addr_bit = gdbarch_addr_bit (gdbarch); > + /* Skip verify of significant_addr_bit, invalid_p == 0 */ > /* Skip verify of software_single_step, has predicate. */ > /* Skip verify of single_step_through_delay, has predicate. */ > /* Skip verify of print_insn, invalid_p == 0 */ > diff --git a/gdb/utils.c b/gdb/utils.c > index 9c5bf68..91c0f2b 100644 > --- a/gdb/utils.c > +++ b/gdb/utils.c > @@ -2708,10 +2708,11 @@ address_significant (gdbarch *gdbarch, CORE_ADDR addr) > /* 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. */ > + when it won't occur. Skip updating of target address if current target has > + not set gdbarch significant_addr_bit. */ > int addr_bit = gdbarch_significant_addr_bit (gdbarch); > > - if (addr_bit < (sizeof (CORE_ADDR) * HOST_CHAR_BIT)) > + if (addr_bit && (addr_bit < (sizeof (CORE_ADDR) * HOST_CHAR_BIT))) > { > CORE_ADDR sign = (CORE_ADDR) 1 << (addr_bit - 1); > addr &= ((CORE_ADDR) 1 << addr_bit) - 1; > -- > 2.7.4 -- 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/gdbarch.c b/gdb/gdbarch.c index c430ebe..5593911 100644 --- a/gdb/gdbarch.c +++ b/gdb/gdbarch.c @@ -615,8 +615,7 @@ verify_gdbarch (struct gdbarch *gdbarch) /* Skip verify of stabs_argument_has_addr, invalid_p == 0 */ /* Skip verify of convert_from_func_ptr_addr, invalid_p == 0 */ /* Skip verify of addr_bits_remove, invalid_p == 0 */ - if (gdbarch->significant_addr_bit == 0) - gdbarch->significant_addr_bit = gdbarch_addr_bit (gdbarch); + /* Skip verify of significant_addr_bit, invalid_p == 0 */ /* Skip verify of software_single_step, has predicate. */ /* Skip verify of single_step_through_delay, has predicate. */ /* Skip verify of print_insn, invalid_p == 0 */ diff --git a/gdb/utils.c b/gdb/utils.c index 9c5bf68..91c0f2b 100644 --- a/gdb/utils.c +++ b/gdb/utils.c @@ -2708,10 +2708,11 @@ address_significant (gdbarch *gdbarch, CORE_ADDR addr) /* 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. */ + when it won't occur. Skip updating of target address if current target has + not set gdbarch significant_addr_bit. */ int addr_bit = gdbarch_significant_addr_bit (gdbarch); - if (addr_bit < (sizeof (CORE_ADDR) * HOST_CHAR_BIT)) + if (addr_bit && (addr_bit < (sizeof (CORE_ADDR) * HOST_CHAR_BIT))) { CORE_ADDR sign = (CORE_ADDR) 1 << (addr_bit - 1); addr &= ((CORE_ADDR) 1 << addr_bit) - 1;