Message ID | 1527023088-10837-1-git-send-email-omair.javaid@linaro.org |
---|---|
State | New, archived |
Headers |
Received: (qmail 46397 invoked by alias); 22 May 2018 21:06:41 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: <gdb-patches.sourceware.org> List-Unsubscribe: <mailto:gdb-patches-unsubscribe-##L=##H@sourceware.org> List-Subscribe: <mailto:gdb-patches-subscribe@sourceware.org> List-Archive: <http://sourceware.org/ml/gdb-patches/> List-Post: <mailto:gdb-patches@sourceware.org> List-Help: <mailto:gdb-patches-help@sourceware.org>, <http://sourceware.org/ml/#faqs> Sender: gdb-patches-owner@sourceware.org Delivered-To: mailing list gdb-patches@sourceware.org Received: (qmail 46366 invoked by uid 89); 22 May 2018 21:06:40 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-26.9 required=5.0 tests=BAYES_00, GIT_PATCH_0, GIT_PATCH_1, GIT_PATCH_2, GIT_PATCH_3, RCVD_IN_DNSWL_NONE, SPF_PASS autolearn=ham version=3.3.2 spammy=23210, gdbarch.c, Hx-languages-length:2419, gdbarchc X-HELO: mail-wr0-f179.google.com Received: from mail-wr0-f179.google.com (HELO mail-wr0-f179.google.com) (209.85.128.179) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Tue, 22 May 2018 21:06:38 +0000 Received: by mail-wr0-f179.google.com with SMTP id 94-v6so22598909wrf.5 for <gdb-patches@sourceware.org>; Tue, 22 May 2018 14:06:38 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:from:to:cc:subject:date:message-id; bh=DWfmwY46cXmWdhot3P6nkATC1YDouojfsUt+ZXPBuOw=; b=IuHT8ae3/zOB5JQPhPAa2ta6PaFMG/nLq477aI99+Ly05fA0OLB67PDI+Z093TAIZP iZPMyEoF1F/4Dv996zKpNxSR53XxAkTSraNjUv2XLOv90hGFSWAos+KixxdFn3iaRiXz o9Ag083HwM+ywm8XbOWNQz1v95D7a+TgCXbk0sS3H0ceo35DjF+hLNp67IRr4hfrPtkg 1YuCA0UBF9qCVQJLbsDi6XtzvjwRgjOCtjsQ4U1DnrYUr5twWuM7wP10xSSPRDabbbnn D85l5jKAp9HzVHuQK1Ioefv8OTirkli806ZKhw5NAQ2IEfr91GSaykNb6CLLA5WUJpmR wxiA== X-Gm-Message-State: ALKqPwdOrbgdKDRWt1JRSBF0OQbwvhvrGxz+MlEDeo9Dwzaa5UupS7hp L2GH5NTFr8Ie2o+BKeqYkJXyH9z18Hc= X-Google-Smtp-Source: AB8JxZovv2M9cgaAKpyheRqhWSkYV9IB+73RGBnUByI6SuxoONms8OKYHbtojiBWwnB3aZju6ayi9g== X-Received: by 2002:adf:8406:: with SMTP id 6-v6mr52794wrf.105.1527023196286; Tue, 22 May 2018 14:06:36 -0700 (PDT) Received: from localhost.localdomain ([119.152.132.180]) by smtp.gmail.com with ESMTPSA id 187-v6sm967245wmu.41.2018.05.22.14.06.34 (version=TLS1_2 cipher=ECDHE-RSA-AES128-SHA bits=128/128); Tue, 22 May 2018 14:06:35 -0700 (PDT) From: Omair Javaid <omair.javaid@linaro.org> To: gdb-patches@sourceware.org Cc: Omair Javaid <omair.javaid@linaro.org> Subject: [PATCH] [PR gdb/23210] Unset gdbarch significant_addr_bit by default Date: Wed, 23 May 2018 02:04:48 +0500 Message-Id: <1527023088-10837-1-git-send-email-omair.javaid@linaro.org> X-IsSubscribed: yes |
Commit Message
Omair Javaid
May 22, 2018, 9:04 p.m. UTC
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(-)
Comments
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
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;