From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 32658 invoked by alias); 9 Jan 2015 19:35:33 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org Received: (qmail 32642 invoked by uid 89); 9 Jan 2015 19:35:32 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-2.2 required=5.0 tests=AWL,BAYES_00,T_RP_MATCHES_RCVD autolearn=ham version=3.3.2 X-HELO: glazunov.sibelius.xs4all.nl Received: from sibelius.xs4all.nl (HELO glazunov.sibelius.xs4all.nl) (83.163.83.176) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with (AES256-GCM-SHA384 encrypted) ESMTPS; Fri, 09 Jan 2015 19:35:31 +0000 Received: from glazunov.sibelius.xs4all.nl (kettenis@localhost [127.0.0.1]) by glazunov.sibelius.xs4all.nl (8.14.5/8.14.3) with ESMTP id t09JZAC3013144; Fri, 9 Jan 2015 20:35:10 +0100 (CET) Received: (from kettenis@localhost) by glazunov.sibelius.xs4all.nl (8.14.5/8.14.3/Submit) id t09JZA6f017629; Fri, 9 Jan 2015 20:35:10 +0100 (CET) Date: Fri, 09 Jan 2015 19:35:00 -0000 Message-Id: <201501091935.t09JZA6f017629@glazunov.sibelius.xs4all.nl> From: Mark Kettenis To: palves@redhat.com CC: mark.kettenis@xs4all.nl, arnez@linux.vnet.ibm.com, jan.kratochvil@redhat.com, gdb-patches@sourceware.org In-reply-to: <54B00D92.4050409@redhat.com> (message from Pedro Alves on Fri, 09 Jan 2015 17:19:14 +0000) Subject: Re: [testsuite patch] for: [PATCH] [PR corefiles/17808] i386: Fix internal error when prstatus in core file is too big References: <874ms18cyz.fsf@br87z6lw.de.ibm.com> <20150108164327.GA29029@host2.jankratochvil.net> <87zj9s70bh.fsf@br87z6lw.de.ibm.com> <54B00160.5000309@redhat.com> <201501091659.t09GxO1q016197@glazunov.sibelius.xs4all.nl> <54B00D92.4050409@redhat.com> X-SW-Source: 2015-01/txt/msg00230.txt.bz2 > Date: Fri, 09 Jan 2015 17:19:14 +0000 > From: Pedro Alves > > On 01/09/2015 04:59 PM, Mark Kettenis wrote: > >> Date: Fri, 09 Jan 2015 16:27:12 +0000 > >> From: Pedro Alves > >> > >>> 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.