Re: PATCH: Fix 32bit coredump read on Linux/AVX

[Mark Kettenis <mark.kettenis@xs4all.nl>]

> Date: Tue, 20 Apr 2010 11:43:39 -0700
> From: "H.J. Lu" <hongjiu.lu@intel.com>
> 
> 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  <hongjiu.lu@intel.com>
> 
> 	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.