Message ID | 20161202214613.GA54717@beast |
---|---|
State | New, archived |
Headers |
Received: (qmail 55161 invoked by alias); 2 Dec 2016 21:46:27 -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 55150 invoked by uid 89); 2 Dec 2016 21:46:26 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-1.9 required=5.0 tests=BAYES_00, RCVD_IN_DNSWL_NONE, SPF_PASS autolearn=ham version=3.3.2 spammy=Cook, H*Ad:D*canonical.com, Hx-languages-length:2330, risk X-HELO: mail-pg0-f51.google.com Received: from mail-pg0-f51.google.com (HELO mail-pg0-f51.google.com) (74.125.83.51) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Fri, 02 Dec 2016 21:46:16 +0000 Received: by mail-pg0-f51.google.com with SMTP id 3so111608319pgd.0 for <gdb-patches@sourceware.org>; Fri, 02 Dec 2016 13:46:16 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20130820; h=x-gm-message-state:date:from:to:cc:subject:message-id:mime-version :content-disposition; bh=TuobcsCVQ2VTDWrSIpwiy+vkV8LJtnzf+OJ3ylf1d1I=; b=FnA8wDGKXwlteF44TVvteanY2NIr3orrxWajC236xXjAld6TwWzGSIkUVoI7EsEFD9 lq6X/5SGn8pAmPkRaGkix6dISjODdFnotXmrOMatcjmUOVMVhqkltSej4FFHTC9o6/aB UtiS349WcW7kWgek72f9LnpY2DN1RmeQSBZWFYHzmxInOI4yZPWcZrn2QMyi5VcFei/7 tDqWBkTgaivGI3Y6piCOB+2dCBpFYg81yvb0VsPUzIu4Gx6lXbWMwsNnL1BU+IWiHPvG aYszj0EU67NDc8XR9Z0jgJLwyOf6sJxqr2cdWWYNxQFwnsAKOczXuM+NkgEk/2OsNFJn oDoQ== X-Gm-Message-State: AKaTC02V6wpfjrKrIRze/B26GHph/2wU6R3dhQc2mlWb7JPEgRQYTD1PhkiACBRK9o2GzK1k X-Received: by 10.98.26.88 with SMTP id a85mr46877466pfa.57.1480715174836; Fri, 02 Dec 2016 13:46:14 -0800 (PST) Received: from www.outflux.net (173-164-112-133-Oregon.hfc.comcastbusiness.net. [173.164.112.133]) by smtp.gmail.com with ESMTPSA id a22sm9976381pfg.7.2016.12.02.13.46.13 (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Fri, 02 Dec 2016 13:46:14 -0800 (PST) Date: Fri, 2 Dec 2016 13:46:13 -0800 From: Kees Cook <keescook@chromium.org> To: gdb-patches@sourceware.org Cc: brian.murray@canonical.com, matthias.klose@canonical.com Subject: [PATCH] Fix PTRACE_GETREGSET failure for compat inferiors on arm64 Message-ID: <20161202214613.GA54717@beast> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline |
Commit Message
Kees Cook
Dec. 2, 2016, 9:46 p.m. UTC
When running a 32-bit ARM inferior on a 64-bit ARM host, only the hardware floating-point registers (NT_ARM_VFP) are available. If the inferior uses hard-float, do not request soft-float registers (NT_PRFPREG) and run the risk of failing with EINVAL. This is most noticeably exposed when running "generate-core-file": (gdb) generate-core-file myprog.core Unable to fetch the floating point registers.: Invalid argument. ptrace(PTRACE_GETREGSET, 27642, NT_FPREGSET, 0xffcc67f0) = -1 EINVAL (Invalid argument) gdb/ChangeLog: 2016-12-02 Kees Cook <keescook@google.com> * gdb/arm-linux-nat.c: Skip soft-float registers when using hard-float. --- gdb/arm-linux-nat.c | 14 +++++++++----- 1 file changed, 9 insertions(+), 5 deletions(-)
Comments
On 16-12-02 13:46:13, Kees Cook wrote: > When running a 32-bit ARM inferior on a 64-bit ARM host, only the hardware > floating-point registers (NT_ARM_VFP) are available. If the inferior > uses hard-float, do not request soft-float registers (NT_PRFPREG) and > run the risk of failing with EINVAL. This is most noticeably exposed "soft-float" is not accurate. FPA is coprocessor. Both VFP and FPA is implemented in the combination of software and hardware. I'd like to rewrite the commit log like this, "When running a 32-bit ARM inferior with a 32-bit ARM GDB on 64-bit AArch64 host, only VFP registers (NT_ARM_VFP) are available. The FPA registers (NT_PRFPREG) is not available." > when running "generate-core-file": > > (gdb) generate-core-file myprog.core > Unable to fetch the floating point registers.: Invalid argument. > > ptrace(PTRACE_GETREGSET, 27642, NT_FPREGSET, 0xffcc67f0) = -1 EINVAL (Invalid argument) > > gdb/ChangeLog: > > 2016-12-02 Kees Cook <keescook@google.com> You don't have FSF copyright assignment. > > * gdb/arm-linux-nat.c: Skip soft-float registers when using hard-float. > > --- > gdb/arm-linux-nat.c | 14 +++++++++----- > 1 file changed, 9 insertions(+), 5 deletions(-) > > diff --git a/gdb/arm-linux-nat.c b/gdb/arm-linux-nat.c > index d11bdc6..2126cd7 100644 > --- a/gdb/arm-linux-nat.c > +++ b/gdb/arm-linux-nat.c > @@ -384,17 +384,19 @@ arm_linux_fetch_inferior_registers (struct target_ops *ops, > if (-1 == regno) > { > fetch_regs (regcache); > - fetch_fpregs (regcache); We should only call fetch_fpregs if tdep->have_fpa_registers is true. > if (tdep->have_wmmx_registers) > fetch_wmmx_regs (regcache); > if (tdep->vfp_register_count > 0) > fetch_vfp_regs (regcache); > + else > + fetch_fpregs (regcache); > } > - else > + else > { > if (regno < ARM_F0_REGNUM || regno == ARM_PS_REGNUM) > fetch_regs (regcache); > - else if (regno >= ARM_F0_REGNUM && regno <= ARM_FPS_REGNUM) > + else if (tdep->vfp_register_count == 0 > + && regno >= ARM_F0_REGNUM && regno <= ARM_FPS_REGNUM) > fetch_fpregs (regcache); Do we really need this change? If FPA registers are not available, REGNO can't fall in this range (ARM_F0_REGNUM, ARM_FPS_REGNUM). These two comments above also apply to store registers.
On Fri, Dec 2, 2016 at 2:49 PM, Yao Qi <qiyaoltc@gmail.com> wrote: > On 16-12-02 13:46:13, Kees Cook wrote: >> When running a 32-bit ARM inferior on a 64-bit ARM host, only the hardware >> floating-point registers (NT_ARM_VFP) are available. If the inferior >> uses hard-float, do not request soft-float registers (NT_PRFPREG) and >> run the risk of failing with EINVAL. This is most noticeably exposed > > "soft-float" is not accurate. FPA is coprocessor. Both VFP and FPA is > implemented in the combination of software and hardware. I'd like to > rewrite the commit log like this, > > "When running a 32-bit ARM inferior with a 32-bit ARM GDB on 64-bit > AArch64 host, only VFP registers (NT_ARM_VFP) are available. The FPA > registers (NT_PRFPREG) is not available." That would be fine by me. I was kind of scratching my head over the naming of the types of floating-point registers. :) Whatever the case, arm64 doesn't support FPA, so an inferior using FPA couldn't run there to start with. :) >> when running "generate-core-file": >> >> (gdb) generate-core-file myprog.core >> Unable to fetch the floating point registers.: Invalid argument. >> >> ptrace(PTRACE_GETREGSET, 27642, NT_FPREGSET, 0xffcc67f0) = -1 EINVAL (Invalid argument) >> >> gdb/ChangeLog: >> >> 2016-12-02 Kees Cook <keescook@google.com> > > You don't have FSF copyright assignment. Oh, hm, I thought there might be a Google-contributions-to-gdb one I was already covered under. What's the best approach for me to take to fix this? >> * gdb/arm-linux-nat.c: Skip soft-float registers when using hard-float. >> >> --- >> gdb/arm-linux-nat.c | 14 +++++++++----- >> 1 file changed, 9 insertions(+), 5 deletions(-) >> >> diff --git a/gdb/arm-linux-nat.c b/gdb/arm-linux-nat.c >> index d11bdc6..2126cd7 100644 >> --- a/gdb/arm-linux-nat.c >> +++ b/gdb/arm-linux-nat.c >> @@ -384,17 +384,19 @@ arm_linux_fetch_inferior_registers (struct target_ops *ops, >> if (-1 == regno) >> { >> fetch_regs (regcache); >> - fetch_fpregs (regcache); > > We should only call fetch_fpregs if tdep->have_fpa_registers is true. I couldn't determine how this was handled. What actually sets org.gnu.gdb.arm.fpa in tdesc? I found gdb/features/arm/arm-fpa.xml and seems to imply it's always included with arm? I wasn't able to follow, but it seemed like _having_ VFP was a indicator that FPA wasn't used. > >> if (tdep->have_wmmx_registers) >> fetch_wmmx_regs (regcache); >> if (tdep->vfp_register_count > 0) >> fetch_vfp_regs (regcache); >> + else >> + fetch_fpregs (regcache); >> } >> - else >> + else >> { >> if (regno < ARM_F0_REGNUM || regno == ARM_PS_REGNUM) >> fetch_regs (regcache); >> - else if (regno >= ARM_F0_REGNUM && regno <= ARM_FPS_REGNUM) >> + else if (tdep->vfp_register_count == 0 >> + && regno >= ARM_F0_REGNUM && regno <= ARM_FPS_REGNUM) >> fetch_fpregs (regcache); > > Do we really need this change? If FPA registers are not available, > REGNO can't fall in this range (ARM_F0_REGNUM, ARM_FPS_REGNUM). > > These two comments above also apply to store registers. It seemed like a reasonable change to make, but I see your point. -Kees
On Fri, Dec 2, 2016 at 2:49 PM, Yao Qi <qiyaoltc@gmail.com> wrote: > On 16-12-02 13:46:13, Kees Cook wrote: >> gdb/ChangeLog: >> >> 2016-12-02 Kees Cook <keescook@google.com> > > You don't have FSF copyright assignment. Hi. Google has a blanket copyright assignment, so this is ok in that regard.
On Fri, Dec 2, 2016 at 11:49 PM, Doug Evans <dje@google.com> wrote: > On Fri, Dec 2, 2016 at 2:49 PM, Yao Qi <qiyaoltc@gmail.com> wrote: >> On 16-12-02 13:46:13, Kees Cook wrote: >>> gdb/ChangeLog: >>> >>> 2016-12-02 Kees Cook <keescook@google.com> >> >> You don't have FSF copyright assignment. > > Hi. > Google has a blanket copyright assignment, so this is ok in that regard. Hi Doug, I am bad at parsing /gd/gnuorg/copyright.list :(, you are right. I find it now.
On Fri, Dec 2, 2016 at 11:08 PM, Kees Cook <keescook@chromium.org> wrote: >>> diff --git a/gdb/arm-linux-nat.c b/gdb/arm-linux-nat.c >>> index d11bdc6..2126cd7 100644 >>> --- a/gdb/arm-linux-nat.c >>> +++ b/gdb/arm-linux-nat.c >>> @@ -384,17 +384,19 @@ arm_linux_fetch_inferior_registers (struct target_ops *ops, >>> if (-1 == regno) >>> { >>> fetch_regs (regcache); >>> - fetch_fpregs (regcache); >> >> We should only call fetch_fpregs if tdep->have_fpa_registers is true. > > I couldn't determine how this was handled. What actually sets > org.gnu.gdb.arm.fpa in tdesc? I found gdb/features/arm/arm-fpa.xml and > seems to imply it's always included with arm? I wasn't able to follow, > but it seemed like _having_ VFP was a indicator that FPA wasn't used. > What is I meant is that instead of calling fetch_fpregs unconditionally, we call fetch_fpregs if tdep->have_fpa_registers is true, like this, if (tdep->have_fpa_registers) fetch_fpregs (regcache); IOW, we only fetch FPA registers if we know FPA registers are available, as described in target description.
On Sun, Dec 4, 2016 at 3:11 AM, Yao Qi <qiyaoltc@gmail.com> wrote: > On Fri, Dec 2, 2016 at 11:08 PM, Kees Cook <keescook@chromium.org> wrote: >>>> diff --git a/gdb/arm-linux-nat.c b/gdb/arm-linux-nat.c >>>> index d11bdc6..2126cd7 100644 >>>> --- a/gdb/arm-linux-nat.c >>>> +++ b/gdb/arm-linux-nat.c >>>> @@ -384,17 +384,19 @@ arm_linux_fetch_inferior_registers (struct target_ops *ops, >>>> if (-1 == regno) >>>> { >>>> fetch_regs (regcache); >>>> - fetch_fpregs (regcache); >>> >>> We should only call fetch_fpregs if tdep->have_fpa_registers is true. >> >> I couldn't determine how this was handled. What actually sets >> org.gnu.gdb.arm.fpa in tdesc? I found gdb/features/arm/arm-fpa.xml and >> seems to imply it's always included with arm? I wasn't able to follow, >> but it seemed like _having_ VFP was a indicator that FPA wasn't used. >> > > What is I meant is that instead of calling fetch_fpregs unconditionally, > we call fetch_fpregs if tdep->have_fpa_registers is true, like this, > > if (tdep->have_fpa_registers) > fetch_fpregs (regcache); > > IOW, we only fetch FPA registers if we know FPA registers are available, > as described in target description. Right, I was asking how the target description is built. I couldn't determine where FPA was detected. -Kees
On 16-12-04 11:30:36, Kees Cook wrote: > Right, I was asking how the target description is built. I couldn't > determine where FPA was detected. > It is detected in arm-tdep.c:arm_gdbarch_init, which looks for feature org.gnu.gdb.arm.fpa, and set have_fpa_registers to 1 if this feature is found.
On Mon, Dec 5, 2016 at 2:26 AM, Yao Qi <qiyaoltc@gmail.com> wrote: > On 16-12-04 11:30:36, Kees Cook wrote: >> Right, I was asking how the target description is built. I couldn't >> determine where FPA was detected. >> > > It is detected in arm-tdep.c:arm_gdbarch_init, which looks for feature > org.gnu.gdb.arm.fpa, and set have_fpa_registers to 1 if this feature is > found. Yup, that part I found. What sets org.gnu.gdb.arm.fpa? -Kees
On 16-12-05 08:06:06, Kees Cook wrote: > On Mon, Dec 5, 2016 at 2:26 AM, Yao Qi <qiyaoltc@gmail.com> wrote: > > On 16-12-04 11:30:36, Kees Cook wrote: > >> Right, I was asking how the target description is built. I couldn't > >> determine where FPA was detected. > >> > > > > It is detected in arm-tdep.c:arm_gdbarch_init, which looks for feature > > org.gnu.gdb.arm.fpa, and set have_fpa_registers to 1 if this feature is > > found. > > Yup, that part I found. What sets org.gnu.gdb.arm.fpa? > The remote stub may know the architecture support FPA, and reply the target description with feature org.gnu.gdb.arm.fpa to GDB.
diff --git a/gdb/arm-linux-nat.c b/gdb/arm-linux-nat.c index d11bdc6..2126cd7 100644 --- a/gdb/arm-linux-nat.c +++ b/gdb/arm-linux-nat.c @@ -384,17 +384,19 @@ arm_linux_fetch_inferior_registers (struct target_ops *ops, if (-1 == regno) { fetch_regs (regcache); - fetch_fpregs (regcache); if (tdep->have_wmmx_registers) fetch_wmmx_regs (regcache); if (tdep->vfp_register_count > 0) fetch_vfp_regs (regcache); + else + fetch_fpregs (regcache); } - else + else { if (regno < ARM_F0_REGNUM || regno == ARM_PS_REGNUM) fetch_regs (regcache); - else if (regno >= ARM_F0_REGNUM && regno <= ARM_FPS_REGNUM) + else if (tdep->vfp_register_count == 0 + && regno >= ARM_F0_REGNUM && regno <= ARM_FPS_REGNUM) fetch_fpregs (regcache); else if (tdep->have_wmmx_registers && regno >= ARM_WR0_REGNUM && regno <= ARM_WCGR7_REGNUM) @@ -420,17 +422,19 @@ arm_linux_store_inferior_registers (struct target_ops *ops, if (-1 == regno) { store_regs (regcache); - store_fpregs (regcache); if (tdep->have_wmmx_registers) store_wmmx_regs (regcache); if (tdep->vfp_register_count > 0) store_vfp_regs (regcache); + else + store_fpregs (regcache); } else { if (regno < ARM_F0_REGNUM || regno == ARM_PS_REGNUM) store_regs (regcache); - else if ((regno >= ARM_F0_REGNUM) && (regno <= ARM_FPS_REGNUM)) + else if (tdep->vfp_register_count == 0 + && (regno >= ARM_F0_REGNUM) && (regno <= ARM_FPS_REGNUM)) store_fpregs (regcache); else if (tdep->have_wmmx_registers && regno >= ARM_WR0_REGNUM && regno <= ARM_WCGR7_REGNUM)