From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 18289 invoked by alias); 21 Apr 2010 19:38:08 -0000 Received: (qmail 18274 invoked by uid 22791); 21 Apr 2010 19:38:07 -0000 X-SWARE-Spam-Status: No, hits=-1.8 required=5.0 tests=BAYES_00,TW_XF,T_RP_MATCHES_RCVD X-Spam-Check-By: sourceware.org Received: from sibelius.xs4all.nl (HELO glazunov.sibelius.xs4all.nl) (83.163.83.176) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Wed, 21 Apr 2010 19:38:01 +0000 Received: from glazunov.sibelius.xs4all.nl (kettenis@localhost [127.0.0.1]) by glazunov.sibelius.xs4all.nl (8.14.3/8.14.3) with ESMTP id o3LJbuEp005227; Wed, 21 Apr 2010 21:37:56 +0200 (CEST) Received: (from kettenis@localhost) by glazunov.sibelius.xs4all.nl (8.14.3/8.14.3/Submit) id o3LJbs6Q019095; Wed, 21 Apr 2010 21:37:55 +0200 (CEST) Date: Wed, 21 Apr 2010 19:38:00 -0000 Message-Id: <201004211937.o3LJbs6Q019095@glazunov.sibelius.xs4all.nl> From: Mark Kettenis To: hjl.tools@gmail.com CC: gdb-patches@sourceware.org In-reply-to: <20100420184339.GA18641@intel.com> (hongjiu.lu@intel.com) Subject: Re: PATCH: Fix 32bit coredump read on Linux/AVX References: <20100420032234.GA10544@intel.com> <20100420184339.GA18641@intel.com> 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 X-SW-Source: 2010-04/txt/msg00695.txt.bz2 > Date: Tue, 20 Apr 2010 11:43:39 -0700 > From: "H.J. Lu" > > On Mon, Apr 19, 2010 at 08:22:34PM -0700, H.J. Lu wrote: > > Hi, > > > > This patch: > > > > http://sourceware.org/ml/gdb-patches/2010-04/msg00276.html Sorry about that. I thought that you had tested that diff. > > breaks 32bit coredump read on Linux/AVX since we only dump .reg and > > .reg-xstate sections on AVX. But i386_linux_core_read_description > > checks .reg2 and .reg-xfp sections first. Since there are no > > .reg2 and .reg-xfp sections, NULL is returned and SSE target is used. > > This patch changes the section check order to .reg-xstate, .reg-xfp, > > .reg2. OK to install? > > > > Thanks. > > > > .reg2 section has x87 regiters on i386 and SSE registers on amd64. > Here is the updated patch to properly handle it. OK to install? Not quite. > 2010-04-20 H.J. Lu > > PR corefiles/11523 > * amd64-linux-tdep.c (amd64_linux_core_read_description): Check > XCR0 first. > > * i386-linux-tdep.c (i386_linux_core_read_xcr0): Return 0 if > there is no .reg-xstate section. > (i386_linux_core_read_description): Check XCR0 first. > > diff --git a/gdb/amd64-linux-tdep.c b/gdb/amd64-linux-tdep.c > index f249d5d..1f7a052 100644 > --- a/gdb/amd64-linux-tdep.c > +++ b/gdb/amd64-linux-tdep.c > @@ -1269,18 +1269,23 @@ amd64_linux_core_read_description (struct gdbarch *gdbarch, > struct target_ops *target, > bfd *abfd) > { > - asection *section = bfd_get_section_by_name (abfd, ".reg2"); > - uint64_t xcr0; > - > - if (section == NULL) > - return NULL; > - > /* Linux/x86-64. */ > - xcr0 = i386_linux_core_read_xcr0 (gdbarch, target, abfd); > - if ((xcr0 & I386_XSTATE_AVX_MASK) == I386_XSTATE_AVX_MASK) > - return tdesc_amd64_avx_linux; > - else > + uint64_t xcr0 = i386_linux_core_read_xcr0 (gdbarch, target, abfd); > + switch ((xcr0 & I386_XSTATE_AVX_MASK)) > + { > + case I386_XSTATE_AVX_MASK: > + return tdesc_amd64_avx_linux; > + return tdesc_i386_avx_linux; There's a duplicate return statement there. > + case I386_XSTATE_SSE_MASK: > + return tdesc_amd64_linux; > + default: > + break; > + } > + > + if (bfd_get_section_by_name (abfd, ".reg2") != NULL) > return tdesc_amd64_linux; > + else > + return NULL; I think we should always return tdesc_amd64_linux here. That is the minimal target description for amd64. > diff --git a/gdb/i386-linux-tdep.c b/gdb/i386-linux-tdep.c > index 5952153..ee7c23f 100644 > --- a/gdb/i386-linux-tdep.c > +++ b/gdb/i386-linux-tdep.c > @@ -611,7 +611,7 @@ i386_linux_core_read_xcr0 (struct gdbarch *gdbarch, > } > } > else > - xcr0 = I386_XSTATE_SSE_MASK; > + xcr0 = 0; > > return xcr0; > } > @@ -623,22 +623,26 @@ i386_linux_core_read_description (struct gdbarch *gdbarch, > struct target_ops *target, > bfd *abfd) > { > - asection *section = bfd_get_section_by_name (abfd, ".reg2"); > - uint64_t xcr0; > - > - if (section == NULL) > - return NULL; > + /* Linux/i386. */ > + uint64_t xcr0 = i386_linux_core_read_xcr0 (gdbarch, target, abfd); > + switch ((xcr0 & I386_XSTATE_AVX_MASK)) > + { > + case I386_XSTATE_AVX_MASK: > + return tdesc_i386_avx_linux; > + case I386_XSTATE_SSE_MASK: > + return tdesc_i386_linux; > + case I386_XSTATE_X87_MASK: > + return tdesc_i386_mmx_linux; > + default: > + break; > + } This is fine, however, > - section = bfd_get_section_by_name (abfd, ".reg-xfp"); > - if (section == NULL) > + if (bfd_get_section_by_name (abfd, ".reg-xfp") != NULL) > + return tdesc_i386_linux; > + else if (bfd_get_section_by_name (abfd, ".reg2") != NULL) > return tdesc_i386_mmx_linux; > - > - /* Linux/i386. */ > - xcr0 = i386_linux_core_read_xcr0 (gdbarch, target, abfd); > - if ((xcr0 & I386_XSTATE_AVX_MASK) == I386_XSTATE_AVX_MASK) > - return tdesc_i386_avx_linux; > else > - return tdesc_i386_linux; > + return NULL; I think we should return tdesc_i386_linux if ".reg-xfp" is found, otherwise return tdesc_i386_mmx_linux.