[PR,gdb/23210] Unset gdbarch significant_addr_bit by default

  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.
---
 gdb/gdbarch.c | 3 +--
 gdb/utils.c   | 5 +++--
 2 files changed, 4 insertions(+), 4 deletions(-)
  

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
  

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