[03/22] Return X86_XSTATE_SSE_MASK instead of 0 in i386fbsd_core_read_xcr0

  i386fbsd_core_read_xcr0 reads the value of xcr0 from the corefile.  If
it fails, returns 0.  This makes its caller {i386,amd64}_target_description
has to handle this special value.  IMO, i386fbsd_core_read_xcr0 should
return the default xcr0 in case of error.

gdb:

2017-08-21  Yao Qi  <yao.qi@linaro.org>

	* i386-fbsd-tdep.c (i386fbsd_core_read_xcr0): Return
	X86_XSTATE_SSE_MASK instead of 0.
---
 gdb/i386-fbsd-tdep.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)
  

On Monday, August 21, 2017 04:28:48 PM Yao Qi wrote:
> i386fbsd_core_read_xcr0 reads the value of xcr0 from the corefile.  If
> it fails, returns 0.  This makes its caller {i386,amd64}_target_description
> has to handle this special value.  IMO, i386fbsd_core_read_xcr0 should
> return the default xcr0 in case of error.
> 
> gdb:
> 
> 2017-08-21  Yao Qi  <yao.qi@linaro.org>
> 
> 	* i386-fbsd-tdep.c (i386fbsd_core_read_xcr0): Return
> 	X86_XSTATE_SSE_MASK instead of 0.
> ---
>  gdb/i386-fbsd-tdep.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/gdb/i386-fbsd-tdep.c b/gdb/i386-fbsd-tdep.c
> index 594b8f6..baca978 100644
> --- a/gdb/i386-fbsd-tdep.c
> +++ b/gdb/i386-fbsd-tdep.c
> @@ -248,14 +248,14 @@ i386fbsd_core_read_xcr0 (bfd *abfd)
>  	    {
>  	      warning (_("Couldn't read `xcr0' bytes from "
>  			 "`.reg-xstate' section in core file."));
> -	      return 0;
> +	      return X86_XSTATE_SSE_MASK;
>  	    }
>  
>  	  xcr0 = bfd_get_64 (abfd, contents);
>  	}
>      }
>    else
> -    xcr0 = 0;
> +    xcr0 = X86_XSTATE_SSE_MASK;
>  
>    return xcr0;
>  }

I think this should actually be X86_XSTATE_MMX_MASK.  Core dumps on FreeBSD/i386
only include the original 387 FPU state in .reg2, they do not write out SSE state
in a separate note as Linux does.

For i386 native FreeBSD (and probably other *BSD) targets the logic needs to
similarly be a bit more complicated, though I can help.  In particular, the
'static int have_ptrace_xmmregs' in i386-bsd-nat.c probably needs to be made
non-static with an extern in 'i386-bsd-nat.h', and i386fbsd_read_description
should try to use PT_GETXMMREGS once to probe it if it isn't set (it can just
fetch the gdb process' registers to test the flag) and then select
X86_XSTATE_SSE_MASK if there is no XSAVE support for PT_GETXMMREGS works,
else use X86_XSTATE_MMX_MASK.  Other BSD's don't have a target read description
target method, so only i386-fbsd-nat.c would need to have its method updated.
I could always work on this as a followup.
  

John Baldwin <jhb@freebsd.org> writes:

> I think this should actually be X86_XSTATE_MMX_MASK.  Core dumps on FreeBSD/i386

s/X86_XSTATE_MMX_MASK/X86_XSTATE_X87_MASK/  ?

> only include the original 387 FPU state in .reg2, they do not write
> out SSE state
> in a separate note as Linux does.
>

FAOD, the existing code (without my patches) get SSE target description
tdesc_i386 in default.  If we should use MMX target description
tdesc_i386_mmx in this case, we can change it after my patch #4, in
which i386_target_description returns tdesc_i386_mmx for X86_XSTATE_X87_MASK.

> For i386 native FreeBSD (and probably other *BSD) targets the logic needs to
> similarly be a bit more complicated, though I can help.  In particular, the
> 'static int have_ptrace_xmmregs' in i386-bsd-nat.c probably needs to be made
> non-static with an extern in 'i386-bsd-nat.h', and i386fbsd_read_description
> should try to use PT_GETXMMREGS once to probe it if it isn't set (it can just
> fetch the gdb process' registers to test the flag) and then select
> X86_XSTATE_SSE_MASK if there is no XSAVE support for PT_GETXMMREGS works,
> else use X86_XSTATE_MMX_MASK.  Other BSD's don't have a target read description
> target method, so only i386-fbsd-nat.c would need to have its method updated.
> I could always work on this as a followup.

To be clear, can I commit this patch as-is?
  

On Monday, August 21, 2017 05:45:50 PM Yao Qi wrote:
> John Baldwin <jhb@freebsd.org> writes:
> 
> > I think this should actually be X86_XSTATE_MMX_MASK.  Core dumps on FreeBSD/i386
> 
> s/X86_XSTATE_MMX_MASK/X86_XSTATE_X87_MASK/  ?

Yes.

> > only include the original 387 FPU state in .reg2, they do not write
> > out SSE state
> > in a separate note as Linux does.
> >
> 
> FAOD, the existing code (without my patches) get SSE target description
> tdesc_i386 in default.  If we should use MMX target description
> tdesc_i386_mmx in this case, we can change it after my patch #4, in
> which i386_target_description returns tdesc_i386_mmx for X86_XSTATE_X87_MASK.

Yes, this is an old bug.  I think it is fine to just reorder this after patch 4
so that returning X87_MASK will do the right thing.

> > For i386 native FreeBSD (and probably other *BSD) targets the logic needs to
> > similarly be a bit more complicated, though I can help.  In particular, the
> > 'static int have_ptrace_xmmregs' in i386-bsd-nat.c probably needs to be made
> > non-static with an extern in 'i386-bsd-nat.h', and i386fbsd_read_description
> > should try to use PT_GETXMMREGS once to probe it if it isn't set (it can just
> > fetch the gdb process' registers to test the flag) and then select
> > X86_XSTATE_SSE_MASK if there is no XSAVE support for PT_GETXMMREGS works,
> > else use X86_XSTATE_MMX_MASK.  Other BSD's don't have a target read description
> > target method, so only i386-fbsd-nat.c would need to have its method updated.
> > I could always work on this as a followup.
> 
> To be clear, can I commit this patch as-is?

Yes.