[testsuite] for: [PATCH] [PR corefiles/17808] i386: Fix internal error when prstatus in core file is too big
| Message ID | 20150108164327.GA29029@host2.jankratochvil.net |
|---|---|
| State | New, archived |
| Headers |
Received: (qmail 23701 invoked by alias); 8 Jan 2015 16:43: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 23689 invoked by uid 89); 8 Jan 2015 16:43:40 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-2.9 required=5.0 tests=AWL, BAYES_00, LOTS_OF_MONEY, SPF_HELO_PASS, SPF_PASS, T_RP_MATCHES_RCVD autolearn=no version=3.3.2 X-HELO: mx1.redhat.com Received: from mx1.redhat.com (HELO mx1.redhat.com) (209.132.183.28) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with (AES256-GCM-SHA384 encrypted) ESMTPS; Thu, 08 Jan 2015 16:43:34 +0000 Received: from int-mx13.intmail.prod.int.phx2.redhat.com (int-mx13.intmail.prod.int.phx2.redhat.com [10.5.11.26]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id t08GhVSO023113 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-GCM-SHA384 bits=256 verify=FAIL); Thu, 8 Jan 2015 11:43:31 -0500 Received: from host2.jankratochvil.net (ovpn-116-60.ams2.redhat.com [10.36.116.60]) by int-mx13.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id t08GhRLP008628 (version=TLSv1/SSLv3 cipher=AES128-SHA bits=128 verify=NO); Thu, 8 Jan 2015 11:43:30 -0500 Date: Thu, 8 Jan 2015 17:43:27 +0100 From: Jan Kratochvil <jan.kratochvil@redhat.com> To: Andreas Arnez <arnez@linux.vnet.ibm.com> Cc: gdb-patches@sourceware.org Subject: [testsuite patch] for: [PATCH] [PR corefiles/17808] i386: Fix internal error when prstatus in core file is too big Message-ID: <20150108164327.GA29029@host2.jankratochvil.net> References: <874ms18cyz.fsf@br87z6lw.de.ibm.com> MIME-Version: 1.0 Content-Type: multipart/mixed; boundary="NMuMz9nt05w80d4+" Content-Disposition: inline In-Reply-To: <874ms18cyz.fsf@br87z6lw.de.ibm.com> User-Agent: Mutt/1.5.23 (2014-03-12) X-IsSubscribed: yes |
Commit Message
Jan Kratochvil
Jan. 8, 2015, 4:43 p.m. UTC
On Thu, 08 Jan 2015 17:16:20 +0100, Andreas Arnez wrote: > Note that this behavior deviates from the default policy: In general, if > some future kernel adds new registers to a register set, then a GDB > unaware of this extension would read the known subset and just ignore > the unknown bytes. This patch is about 'assert' vs. 'if' so I find this paragraph outside of the topic of this thread/regression. Here is a testcase for the assertion crash regression. Thanks, Jan gdb/testsuite/ChangeLog 2015-01-08 Jan Kratochvil <jan.kratochvil@redhat.com> PR corefiles/17808 * gdb.arch/i386-biarch-core.core.bz2.uu: New file. * gdb.arch/i386-biarch-core.exp: New file.
Comments
On Thu, Jan 08 2015, Jan Kratochvil wrote: > On Thu, 08 Jan 2015 17:16:20 +0100, Andreas Arnez wrote: >> Note that this behavior deviates from the default policy: In general, if >> some future kernel adds new registers to a register set, then a GDB >> unaware of this extension would read the known subset and just ignore >> the unknown bytes. > > This patch is about 'assert' vs. 'if' so I find this paragraph outside of the > topic of this thread/regression. Right, it seems a bit off-topic. What I really mean is that "this behavior *still* deviates from the default policy", and I just wanted to make sure that we all agree to that. Anyway, I will remove this paragraph from the commit message. Any other comments? After adjusting the commit message, is the patch then OK to apply?
> Any other comments? Do we need to do the same in other places? This grep seems to suggest yes: $ grep assert * | grep sizeof | grep regset amd64obsd-tdep.c: gdb_assert (len >= tdep->sizeof_gregset + I387_SIZEOF_FXSAVE); amd64-tdep.c: gdb_assert (len == tdep->sizeof_fpregset); amd64-tdep.c: gdb_assert (len == tdep->sizeof_fpregset); i386obsd-tdep.c: gdb_assert (len >= tdep->sizeof_gregset + I387_SIZEOF_FSAVE); i386-tdep.c: gdb_assert (len == tdep->sizeof_gregset); i386-tdep.c: gdb_assert (len == tdep->sizeof_gregset); i386-tdep.c: gdb_assert (len == tdep->sizeof_fpregset); i386-tdep.c: gdb_assert (len == tdep->sizeof_fpregset); mips-linux-tdep.c: gdb_assert (len == sizeof (mips_elf_gregset_t)); mips-linux-tdep.c: gdb_assert (len == sizeof (mips_elf_gregset_t)); mips-linux-tdep.c: gdb_assert (len == sizeof (mips_elf_fpregset_t)); mips-linux-tdep.c: gdb_assert (len == sizeof (mips_elf_fpregset_t)); mips-linux-tdep.c: gdb_assert (len == sizeof (mips64_elf_gregset_t)); mips-linux-tdep.c: gdb_assert (len == sizeof (mips64_elf_gregset_t)); mips-linux-tdep.c: gdb_assert (len == sizeof (mips64_elf_fpregset_t)); mips-linux-tdep.c: gdb_assert (len == sizeof (mips64_elf_fpregset_t)); mn10300-linux-tdep.c: gdb_assert (len == sizeof (mn10300_elf_gregset_t)); mn10300-linux-tdep.c: gdb_assert (len == sizeof (mn10300_elf_fpregset_t)); mn10300-linux-tdep.c: gdb_assert (len == sizeof (mn10300_elf_gregset_t)); On 01/08/2015 04:16 PM, Andreas Arnez wrote: > Note that this behavior deviates from the default policy: In general, if > some future kernel adds new registers to a register set, then a GDB > unaware of this extension would read the known subset and just ignore > the unknown bytes. That's a good point. get_core_register_section checks the section size already: get_core_register_section (struct regcache *regcache, const struct regset *regset, const char *name, int min_size, int which, const char *human_name, int required) { ... size = bfd_section_size (core_bfd, section); if (size < min_size) { warning (_("Section `%s' in core file too small."), section_name); return; } ... Should we remove all those asserts, and make it the job of get_core_register_section to warn if the section size is bigger than expected? We may need to pass the "expected" section size to the callback, in addition to the "minimum" size though. Thanks, Pedro Alves
> Date: Fri, 09 Jan 2015 16:27:12 +0000 > From: Pedro Alves <palves@redhat.com> > > > Any other comments? > > Do we need to do the same in other places? This grep seems to suggest yes: > > $ grep assert * | grep sizeof | grep regset > amd64obsd-tdep.c: gdb_assert (len >= tdep->sizeof_gregset + I387_SIZEOF_FXSAVE); > amd64-tdep.c: gdb_assert (len == tdep->sizeof_fpregset); > amd64-tdep.c: gdb_assert (len == tdep->sizeof_fpregset); > i386obsd-tdep.c: gdb_assert (len >= tdep->sizeof_gregset + I387_SIZEOF_FSAVE); > i386-tdep.c: gdb_assert (len == tdep->sizeof_gregset); > i386-tdep.c: gdb_assert (len == tdep->sizeof_gregset); > i386-tdep.c: gdb_assert (len == tdep->sizeof_fpregset); > i386-tdep.c: gdb_assert (len == tdep->sizeof_fpregset); > mips-linux-tdep.c: gdb_assert (len == sizeof (mips_elf_gregset_t)); > mips-linux-tdep.c: gdb_assert (len == sizeof (mips_elf_gregset_t)); > mips-linux-tdep.c: gdb_assert (len == sizeof (mips_elf_fpregset_t)); > mips-linux-tdep.c: gdb_assert (len == sizeof (mips_elf_fpregset_t)); > mips-linux-tdep.c: gdb_assert (len == sizeof (mips64_elf_gregset_t)); > mips-linux-tdep.c: gdb_assert (len == sizeof (mips64_elf_gregset_t)); > mips-linux-tdep.c: gdb_assert (len == sizeof (mips64_elf_fpregset_t)); > mips-linux-tdep.c: gdb_assert (len == sizeof (mips64_elf_fpregset_t)); > mn10300-linux-tdep.c: gdb_assert (len == sizeof (mn10300_elf_gregset_t)); > mn10300-linux-tdep.c: gdb_assert (len == sizeof (mn10300_elf_fpregset_t)); > mn10300-linux-tdep.c: gdb_assert (len == sizeof (mn10300_elf_gregset_t)); > > On 01/08/2015 04:16 PM, Andreas Arnez wrote: > > Note that this behavior deviates from the default policy: In general, if > > some future kernel adds new registers to a register set, then a GDB > > unaware of this extension would read the known subset and just ignore > > the unknown bytes. > > That's a good point. > > get_core_register_section checks the section size already: > > get_core_register_section (struct regcache *regcache, > const struct regset *regset, > const char *name, > int min_size, > int which, > const char *human_name, > int required) > { > ... > size = bfd_section_size (core_bfd, section); > if (size < min_size) > { > warning (_("Section `%s' in core file too small."), section_name); > return; > } > ... > > Should we remove all those asserts, and make it the > job of get_core_register_section to warn if the section > size is bigger than expected? We may need to pass > the "expected" section size to the callback, in addition > to the "minimum" size though. The code is designed to allow these sections to grow such that the OS kernel can add more registers without breaking GDB.
On 01/09/2015 04:59 PM, Mark Kettenis wrote: >> Date: Fri, 09 Jan 2015 16:27:12 +0000 >> From: Pedro Alves <palves@redhat.com> >> >>> Any other comments? >> >> Do we need to do the same in other places? This grep seems to suggest yes: >> >> $ grep assert * | grep sizeof | grep regset >> amd64obsd-tdep.c: gdb_assert (len >= tdep->sizeof_gregset + I387_SIZEOF_FXSAVE); >> amd64-tdep.c: gdb_assert (len == tdep->sizeof_fpregset); >> amd64-tdep.c: gdb_assert (len == tdep->sizeof_fpregset); >> i386obsd-tdep.c: gdb_assert (len >= tdep->sizeof_gregset + I387_SIZEOF_FSAVE); >> i386-tdep.c: gdb_assert (len == tdep->sizeof_gregset); >> i386-tdep.c: gdb_assert (len == tdep->sizeof_gregset); >> i386-tdep.c: gdb_assert (len == tdep->sizeof_fpregset); >> i386-tdep.c: gdb_assert (len == tdep->sizeof_fpregset); >> mips-linux-tdep.c: gdb_assert (len == sizeof (mips_elf_gregset_t)); >> mips-linux-tdep.c: gdb_assert (len == sizeof (mips_elf_gregset_t)); >> mips-linux-tdep.c: gdb_assert (len == sizeof (mips_elf_fpregset_t)); >> mips-linux-tdep.c: gdb_assert (len == sizeof (mips_elf_fpregset_t)); >> mips-linux-tdep.c: gdb_assert (len == sizeof (mips64_elf_gregset_t)); >> mips-linux-tdep.c: gdb_assert (len == sizeof (mips64_elf_gregset_t)); >> mips-linux-tdep.c: gdb_assert (len == sizeof (mips64_elf_fpregset_t)); >> mips-linux-tdep.c: gdb_assert (len == sizeof (mips64_elf_fpregset_t)); >> mn10300-linux-tdep.c: gdb_assert (len == sizeof (mn10300_elf_gregset_t)); >> mn10300-linux-tdep.c: gdb_assert (len == sizeof (mn10300_elf_fpregset_t)); >> mn10300-linux-tdep.c: gdb_assert (len == sizeof (mn10300_elf_gregset_t)); >> >> On 01/08/2015 04:16 PM, Andreas Arnez wrote: >>> Note that this behavior deviates from the default policy: In general, if >>> some future kernel adds new registers to a register set, then a GDB >>> unaware of this extension would read the known subset and just ignore >>> the unknown bytes. >> >> That's a good point. >> >> get_core_register_section checks the section size already: >> >> get_core_register_section (struct regcache *regcache, >> const struct regset *regset, >> const char *name, >> int min_size, >> int which, >> const char *human_name, >> int required) >> { >> ... >> size = bfd_section_size (core_bfd, section); >> if (size < min_size) >> { >> warning (_("Section `%s' in core file too small."), section_name); >> return; >> } >> ... >> >> Should we remove all those asserts, and make it the >> job of get_core_register_section to warn if the section >> size is bigger than expected? We may need to pass >> the "expected" section size to the callback, in addition >> to the "minimum" size though. > > The code is designed to allow these sections to grow such that the OS > kernel can add more registers without breaking GDB. Not sure what you're disagreeing with. My comment is in that direction too (And Andreas' comment I'm quoting). That is, get_core_register_section would warn, but still continue processing the section. The current code clearly does not work that way, given the assertions. Thanks, Pedro Alves
> Date: Fri, 09 Jan 2015 17:19:14 +0000 > From: Pedro Alves <palves@redhat.com> > > On 01/09/2015 04:59 PM, Mark Kettenis wrote: > >> Date: Fri, 09 Jan 2015 16:27:12 +0000 > >> From: Pedro Alves <palves@redhat.com> > >> > >>> Any other comments? > >> > >> Do we need to do the same in other places? This grep seems to suggest yes: > >> > >> $ grep assert * | grep sizeof | grep regset > >> amd64obsd-tdep.c: gdb_assert (len >= tdep->sizeof_gregset + I387_SIZEOF_FXSAVE); > >> amd64-tdep.c: gdb_assert (len == tdep->sizeof_fpregset); > >> amd64-tdep.c: gdb_assert (len == tdep->sizeof_fpregset); > >> i386obsd-tdep.c: gdb_assert (len >= tdep->sizeof_gregset + I387_SIZEOF_FSAVE); > >> i386-tdep.c: gdb_assert (len == tdep->sizeof_gregset); > >> i386-tdep.c: gdb_assert (len == tdep->sizeof_gregset); > >> i386-tdep.c: gdb_assert (len == tdep->sizeof_fpregset); > >> i386-tdep.c: gdb_assert (len == tdep->sizeof_fpregset); > >> mips-linux-tdep.c: gdb_assert (len == sizeof (mips_elf_gregset_t)); > >> mips-linux-tdep.c: gdb_assert (len == sizeof (mips_elf_gregset_t)); > >> mips-linux-tdep.c: gdb_assert (len == sizeof (mips_elf_fpregset_t)); > >> mips-linux-tdep.c: gdb_assert (len == sizeof (mips_elf_fpregset_t)); > >> mips-linux-tdep.c: gdb_assert (len == sizeof (mips64_elf_gregset_t)); > >> mips-linux-tdep.c: gdb_assert (len == sizeof (mips64_elf_gregset_t)); > >> mips-linux-tdep.c: gdb_assert (len == sizeof (mips64_elf_fpregset_t)); > >> mips-linux-tdep.c: gdb_assert (len == sizeof (mips64_elf_fpregset_t)); > >> mn10300-linux-tdep.c: gdb_assert (len == sizeof (mn10300_elf_gregset_t)); > >> mn10300-linux-tdep.c: gdb_assert (len == sizeof (mn10300_elf_fpregset_t)); > >> mn10300-linux-tdep.c: gdb_assert (len == sizeof (mn10300_elf_gregset_t)); > >> > >> On 01/08/2015 04:16 PM, Andreas Arnez wrote: > >>> Note that this behavior deviates from the default policy: In general, if > >>> some future kernel adds new registers to a register set, then a GDB > >>> unaware of this extension would read the known subset and just ignore > >>> the unknown bytes. > >> > >> That's a good point. > >> > >> get_core_register_section checks the section size already: > >> > >> get_core_register_section (struct regcache *regcache, > >> const struct regset *regset, > >> const char *name, > >> int min_size, > >> int which, > >> const char *human_name, > >> int required) > >> { > >> ... > >> size = bfd_section_size (core_bfd, section); > >> if (size < min_size) > >> { > >> warning (_("Section `%s' in core file too small."), section_name); > >> return; > >> } > >> ... > >> > >> Should we remove all those asserts, and make it the > >> job of get_core_register_section to warn if the section > >> size is bigger than expected? We may need to pass > >> the "expected" section size to the callback, in addition > >> to the "minimum" size though. > > > > The code is designed to allow these sections to grow such that the OS > > kernel can add more registers without breaking GDB. > > Not sure what you're disagreeing with. My comment is in that direction > too (And Andreas' comment I'm quoting). That is, get_core_register_section > would warn, but still continue processing the section. > > The current code clearly does not work that way, given the assertions. It shouldn't warn if the sections is bigger that "expected", because in some cases the "expected" size is really the minimum supported size, where later versions of the OS added extra information. At least not unconditionally. I can imagine extending the interface to also specify a maximum size and interpreting a maximum size of 0 as "no maximum". Continiung after printing a warning if the section is larger than the maximum size probably makes sense. The asserts should probably be changed into >= whatever happens.
On 01/09/2015 07:35 PM, Mark Kettenis wrote: >> Date: Fri, 09 Jan 2015 17:19:14 +0000 >> From: Pedro Alves <palves@redhat.com> >> >> On 01/09/2015 04:59 PM, Mark Kettenis wrote: >>>> Date: Fri, 09 Jan 2015 16:27:12 +0000 >>>> From: Pedro Alves <palves@redhat.com> >>>> >>>>> Any other comments? >>>> >>>> Do we need to do the same in other places? This grep seems to suggest yes: >>>> >>>> $ grep assert * | grep sizeof | grep regset >>>> amd64obsd-tdep.c: gdb_assert (len >= tdep->sizeof_gregset + I387_SIZEOF_FXSAVE); >>>> amd64-tdep.c: gdb_assert (len == tdep->sizeof_fpregset); >>>> amd64-tdep.c: gdb_assert (len == tdep->sizeof_fpregset); >>>> i386obsd-tdep.c: gdb_assert (len >= tdep->sizeof_gregset + I387_SIZEOF_FSAVE); >>>> i386-tdep.c: gdb_assert (len == tdep->sizeof_gregset); >>>> i386-tdep.c: gdb_assert (len == tdep->sizeof_gregset); >>>> i386-tdep.c: gdb_assert (len == tdep->sizeof_fpregset); >>>> i386-tdep.c: gdb_assert (len == tdep->sizeof_fpregset); >>>> mips-linux-tdep.c: gdb_assert (len == sizeof (mips_elf_gregset_t)); >>>> mips-linux-tdep.c: gdb_assert (len == sizeof (mips_elf_gregset_t)); >>>> mips-linux-tdep.c: gdb_assert (len == sizeof (mips_elf_fpregset_t)); >>>> mips-linux-tdep.c: gdb_assert (len == sizeof (mips_elf_fpregset_t)); >>>> mips-linux-tdep.c: gdb_assert (len == sizeof (mips64_elf_gregset_t)); >>>> mips-linux-tdep.c: gdb_assert (len == sizeof (mips64_elf_gregset_t)); >>>> mips-linux-tdep.c: gdb_assert (len == sizeof (mips64_elf_fpregset_t)); >>>> mips-linux-tdep.c: gdb_assert (len == sizeof (mips64_elf_fpregset_t)); >>>> mn10300-linux-tdep.c: gdb_assert (len == sizeof (mn10300_elf_gregset_t)); >>>> mn10300-linux-tdep.c: gdb_assert (len == sizeof (mn10300_elf_fpregset_t)); >>>> mn10300-linux-tdep.c: gdb_assert (len == sizeof (mn10300_elf_gregset_t)); >>>> >>>> On 01/08/2015 04:16 PM, Andreas Arnez wrote: >>>>> Note that this behavior deviates from the default policy: In general, if >>>>> some future kernel adds new registers to a register set, then a GDB >>>>> unaware of this extension would read the known subset and just ignore >>>>> the unknown bytes. >>>> >>>> That's a good point. >>>> >>>> get_core_register_section checks the section size already: >>>> >>>> get_core_register_section (struct regcache *regcache, >>>> const struct regset *regset, >>>> const char *name, >>>> int min_size, >>>> int which, >>>> const char *human_name, >>>> int required) >>>> { >>>> ... >>>> size = bfd_section_size (core_bfd, section); >>>> if (size < min_size) >>>> { >>>> warning (_("Section `%s' in core file too small."), section_name); >>>> return; >>>> } >>>> ... >>>> >>>> Should we remove all those asserts, and make it the >>>> job of get_core_register_section to warn if the section >>>> size is bigger than expected? We may need to pass >>>> the "expected" section size to the callback, in addition >>>> to the "minimum" size though. >>> >>> The code is designed to allow these sections to grow such that the OS >>> kernel can add more registers without breaking GDB. >> >> Not sure what you're disagreeing with. My comment is in that direction >> too (And Andreas' comment I'm quoting). That is, get_core_register_section >> would warn, but still continue processing the section. >> >> The current code clearly does not work that way, given the assertions. > > It shouldn't warn if the sections is bigger that "expected", because > in some cases the "expected" size is really the minimum supported > size, where later versions of the OS added extra information. At > least not unconditionally. I think we're saying the same thing, but what I'm calling "expected", you're calling "maximum". As in, consider the case where GDB about a regset section that is supposed to have size A. GDB is taught about this, with "minimum" == A, and "expected/maximum" == A. Later at some point, a new variant of the machine appears with more registers, and the regset is extended, to size B. A GDB that only knows about A encounters a core dump with B, and thus issues a warning (which suggests that either more info is available that gdb doesn't grok, or the core is broken), but still presents the A registers to the user. Later, someone teaches GDB about B registers, and at that point, "minimum" stays A, but "expected/maximum" is set to B. At some point, if the regset is extended further to C, a GDB that knows about A and B warns when it sees C. And on and on. I think we've already seen something like that with the x86 xsave regset? > I can imagine extending the interface to also specify a maximum size > and interpreting a maximum size of 0 as "no maximum". Continiung > after printing a warning if the section is larger than the maximum > size probably makes sense. Thanks, Pedro Alves
> Date: Fri, 09 Jan 2015 20:11:04 +0000 > From: Pedro Alves <palves@redhat.com> > > On 01/09/2015 07:35 PM, Mark Kettenis wrote: > >> Date: Fri, 09 Jan 2015 17:19:14 +0000 > >> From: Pedro Alves <palves@redhat.com> > >> > >> On 01/09/2015 04:59 PM, Mark Kettenis wrote: > >>>> Date: Fri, 09 Jan 2015 16:27:12 +0000 > >>>> From: Pedro Alves <palves@redhat.com> > >>>> > >>>>> Any other comments? > >>>> > >>>> Do we need to do the same in other places? This grep seems to suggest yes: > >>>> > >>>> $ grep assert * | grep sizeof | grep regset > >>>> amd64obsd-tdep.c: gdb_assert (len >= tdep->sizeof_gregset + I387_SIZEOF_FXSAVE); > >>>> amd64-tdep.c: gdb_assert (len == tdep->sizeof_fpregset); > >>>> amd64-tdep.c: gdb_assert (len == tdep->sizeof_fpregset); > >>>> i386obsd-tdep.c: gdb_assert (len >= tdep->sizeof_gregset + I387_SIZEOF_FSAVE); > >>>> i386-tdep.c: gdb_assert (len == tdep->sizeof_gregset); > >>>> i386-tdep.c: gdb_assert (len == tdep->sizeof_gregset); > >>>> i386-tdep.c: gdb_assert (len == tdep->sizeof_fpregset); > >>>> i386-tdep.c: gdb_assert (len == tdep->sizeof_fpregset); > >>>> mips-linux-tdep.c: gdb_assert (len == sizeof (mips_elf_gregset_t)); > >>>> mips-linux-tdep.c: gdb_assert (len == sizeof (mips_elf_gregset_t)); > >>>> mips-linux-tdep.c: gdb_assert (len == sizeof (mips_elf_fpregset_t)); > >>>> mips-linux-tdep.c: gdb_assert (len == sizeof (mips_elf_fpregset_t)); > >>>> mips-linux-tdep.c: gdb_assert (len == sizeof (mips64_elf_gregset_t)); > >>>> mips-linux-tdep.c: gdb_assert (len == sizeof (mips64_elf_gregset_t)); > >>>> mips-linux-tdep.c: gdb_assert (len == sizeof (mips64_elf_fpregset_t)); > >>>> mips-linux-tdep.c: gdb_assert (len == sizeof (mips64_elf_fpregset_t)); > >>>> mn10300-linux-tdep.c: gdb_assert (len == sizeof (mn10300_elf_gregset_t)); > >>>> mn10300-linux-tdep.c: gdb_assert (len == sizeof (mn10300_elf_fpregset_t)); > >>>> mn10300-linux-tdep.c: gdb_assert (len == sizeof (mn10300_elf_gregset_t)); > >>>> > >>>> On 01/08/2015 04:16 PM, Andreas Arnez wrote: > >>>>> Note that this behavior deviates from the default policy: In general, if > >>>>> some future kernel adds new registers to a register set, then a GDB > >>>>> unaware of this extension would read the known subset and just ignore > >>>>> the unknown bytes. > >>>> > >>>> That's a good point. > >>>> > >>>> get_core_register_section checks the section size already: > >>>> > >>>> get_core_register_section (struct regcache *regcache, > >>>> const struct regset *regset, > >>>> const char *name, > >>>> int min_size, > >>>> int which, > >>>> const char *human_name, > >>>> int required) > >>>> { > >>>> ... > >>>> size = bfd_section_size (core_bfd, section); > >>>> if (size < min_size) > >>>> { > >>>> warning (_("Section `%s' in core file too small."), section_name); > >>>> return; > >>>> } > >>>> ... > >>>> > >>>> Should we remove all those asserts, and make it the > >>>> job of get_core_register_section to warn if the section > >>>> size is bigger than expected? We may need to pass > >>>> the "expected" section size to the callback, in addition > >>>> to the "minimum" size though. > >>> > >>> The code is designed to allow these sections to grow such that the OS > >>> kernel can add more registers without breaking GDB. > >> > >> Not sure what you're disagreeing with. My comment is in that direction > >> too (And Andreas' comment I'm quoting). That is, get_core_register_section > >> would warn, but still continue processing the section. > >> > >> The current code clearly does not work that way, given the assertions. > > > > It shouldn't warn if the sections is bigger that "expected", because > > in some cases the "expected" size is really the minimum supported > > size, where later versions of the OS added extra information. At > > least not unconditionally. > > I think we're saying the same thing, but what I'm calling "expected", > you're calling "maximum". As in, consider the case where GDB > about a regset section that is supposed to have size A. GDB is taught > about this, with "minimum" == A, and "expected/maximum" == A. Later at > some point, a new variant of the machine appears with more registers, and > the regset is extended, to size B. A GDB that only knows about A encounters > a core dump with B, and thus issues a warning (which suggests that either > more info is available that gdb doesn't grok, or the core is broken), but still > presents the A registers to the user. Later, someone teaches GDB about B > registers, and at that point, "minimum" stays A, but "expected/maximum" is > set to B. At some point, if the regset is extended further to C, a GDB > that knows about A and B warns when it sees C. And on and on. I think > we've already seen something like that with the x86 xsave regset? Yes, the x86 "FPU" register set certainly is an example I had in mind. It all started when SSE was introduced. There are also some BSD's where during the transition from a.out to ELF the floating-point registers were seperated out into their own section. In that case the section actually shrunk and the minmum size was adjusted.
On Fri, Jan 09 2015, Mark Kettenis wrote: >> Date: Fri, 09 Jan 2015 20:11:04 +0000 >> From: Pedro Alves <palves@redhat.com> >> >> ... >> I think we're saying the same thing, but what I'm calling "expected", >> you're calling "maximum". As in, consider the case where GDB >> about a regset section that is supposed to have size A. GDB is taught >> about this, with "minimum" == A, and "expected/maximum" == A. Later at >> some point, a new variant of the machine appears with more registers, and >> the regset is extended, to size B. A GDB that only knows about A encounters >> a core dump with B, and thus issues a warning (which suggests that either >> more info is available that gdb doesn't grok, or the core is broken), but still >> presents the A registers to the user. Later, someone teaches GDB about B >> registers, and at that point, "minimum" stays A, but "expected/maximum" is >> set to B. At some point, if the regset is extended further to C, a GDB >> that knows about A and B warns when it sees C. And on and on. I think >> we've already seen something like that with the x86 xsave regset? > > Yes, the x86 "FPU" register set certainly is an example I had in mind. > It all started when SSE was introduced. Right, the floating point register set has grown over time. The good thing is that x86 Linux platforms represent the different FPU regset versions with different target descriptions. (As determined from a core file with the core_read_description gdbarch method.) In this way the regset size passed to the iterator callback could match the "expected" size, not just the minimum. However, the size is currently not calculated at all for ".reg-xstate". I think this should be fixed, probably by using X86_XSTATE_SIZE.
diff --git a/gdb/testsuite/gdb.arch/i386-biarch-core.core.bz2.uu b/gdb/testsuite/gdb.arch/i386-biarch-core.core.bz2.uu new file mode 100644 index 0000000..4221e99 --- /dev/null +++ b/gdb/testsuite/gdb.arch/i386-biarch-core.core.bz2.uu @@ -0,0 +1,28 @@ +# Copyright 2015 Free Software Foundation, Inc. + +# This program is free software; you can redistribute it and/or modify +# it under the terms of the GNU General Public License as published by +# the Free Software Foundation; either version 3 of the License, or +# (at your option) any later version. +# +# This program is distributed in the hope that it will be useful, +# but WITHOUT ANY WARRANTY; without even the implied warranty of +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +# GNU General Public License for more details. +# +# You should have received a copy of the GNU General Public License +# along with this program. If not, see <http://www.gnu.org/licenses/>. + +begin 600 i386-biarch-core.core.bz2 +M0EIH.3%!629361`P1\P`!)?_____\9'@"8Q)6P380'9@'&#`0D@``"``%(`` +M@`#`"!<(L`%F"(:$GH13::F-)M&D&U,AD:`--#)M0&FT0XR9--,)D9`P(Q-& +M",(-&F``02)%38HT]0T`&AH```'H@``T^>9T*(,("&)SE`>`9@+GP=[,N)KB +M'I8BL(L]N5TCY\%V]/?DB.BN*UZ'U@]TN7-]UJ5\_%0QTT<*086#%MHT7XVJ +M9D"+C!"2*L:8D1XPD!`--M@*XT1H5RFYN&)(!0P0#:`I:;2;$5M&\*9"0@%: +MK@X[T()M)9N7`D$VA!^63)%,;@8LT`(7\K&[7G;U:"B6'!GG+46ALOZF.2F- +M!@>C*%86X$-]C2`KE;HG)UL(913VR2G]0BD:J=Z_`G@S,`W%.8RMS-#5P:J0 +MAJ2\8&X?@DE;UF68QHM<,D`('::J65/S:PAG*R-09["8DBI)'V]Y.[(/AM*L +M"X_O^V;%FY.S6Q]FM=D37>5F,%4-F1ZF#,CFJVU;H*^IT<(%<V`.32$`JU[" +/G`68?\7<D4X4)`0,$?,` +` +end diff --git a/gdb/testsuite/gdb.arch/i386-biarch-core.exp b/gdb/testsuite/gdb.arch/i386-biarch-core.exp new file mode 100644 index 0000000..0dcb256 --- /dev/null +++ b/gdb/testsuite/gdb.arch/i386-biarch-core.exp @@ -0,0 +1,59 @@ +# Copyright 2015 Free Software Foundation, Inc. + +# This program is free software; you can redistribute it and/or modify +# it under the terms of the GNU General Public License as published by +# the Free Software Foundation; either version 3 of the License, or +# (at your option) any later version. +# +# This program is distributed in the hope that it will be useful, +# but WITHOUT ANY WARRANTY; without even the implied warranty of +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +# GNU General Public License for more details. +# +# You should have received a copy of the GNU General Public License +# along with this program. If not, see <http://www.gnu.org/licenses/>. + +# Test ability to load an elf64-i386 core file. The provided core file was +# elf64-x8664 one but it got binary patched to i386: +# Elf32_Ehdr.e_machine @0x12..0x13 +# Elf64_Ehdr.e_machine @0x12..0x13 +# #define EM_386 3 /* Intel 80386 */ +# #define EM_X86_64 62 /* AMD x86-64 architecture */ +# patch @0x12: 0x3E -> 0x03 + +standard_testfile + +if { ![istarget "i?86-*-*"] && ![istarget "x86_64-*-*"] } then { + verbose "Skipping i386-biarch-core test." + return +} + +set corebz2uufile ${srcdir}/${subdir}/${testfile}.core.bz2.uu +set corefile ${objdir}/${subdir}/${testfile}.core +# Entry point of the original executable. +set address 0x400078 + +if {[catch "system \"uudecode -o - ${corebz2uufile} | bzip2 -dc >${corefile}\""] != 0} { + untested "failed uudecode or bzip2" + return -1 +} +file stat ${corefile} corestat +if {$corestat(size) != 102400} { + untested "uudecode or bzip2 produce invalid result" + return -1 +} + +gdb_exit +gdb_start +gdb_reinitialize_dir $srcdir/$subdir + +# Wrongly built GDB complains by: +# "..." is not a core dump: File format not recognized +# As the provided test core has 64bit PRSTATUS i386 built GDB cannot parse it. +# This is just a problem of the test care, real-world elf64-i386 file will have +# 32bit PRSTATUS. One cannot prepare elf64-i386 core file from elf32-i386 by +# objcopy as it corrupts the core file beyond all recognition. +# "\r\nCore was generated by `\[^\r\n\]*'\\.\r\nProgram terminated with signal 11, Segmentation fault\\.\r\n.*" +gdb_test "core-file ${corefile}" ".*" "core-file" + +gdb_test "x/i $address" "\r\n\[ \t\]*$address:\[ \t\]*hlt\[ \t\]*" ".text is readable"