Re: [PATCH v6 08/15] gdb: Update x86 Linux architectures to support XSAVE layouts.

[John Baldwin <jhb@FreeBSD.org>]

On 7/26/23 1:45 PM, Simon Marchi wrote:
>> diff --git a/gdb/i386-linux-tdep.c b/gdb/i386-linux-tdep.c
>> index 554c1455a5f..f27efbb1d59 100644
>> --- a/gdb/i386-linux-tdep.c
>> +++ b/gdb/i386-linux-tdep.c
>> @@ -638,45 +638,51 @@ static int i386_linux_sc_reg_offset[] =
>>     0 * 4				/* %gs */
>>   };
>>   
>> -/* Get XSAVE extended state xcr0 from core dump.  */
>> +/* See i386-linux-tdep.h.  */
>>   
>>   uint64_t
>> -i386_linux_core_read_xcr0 (bfd *abfd)
>> +i386_linux_core_read_xsave_info (bfd *abfd, x86_xsave_layout &layout)
>>   {
>>     asection *xstate = bfd_get_section_by_name (abfd, ".reg-xstate");
>> -  uint64_t xcr0;
>> +  if (xstate == nullptr)
>> +    return X86_XSTATE_SSE_MASK;
>>   
>> -  if (xstate)
>> +  /* Check extended state size.  */
>> +  size_t size = bfd_section_size (xstate);
>> +  if (size < X86_XSTATE_AVX_SIZE)
>> +    return X86_XSTATE_SSE_MASK;
>> +
>> +  char contents[8];
>> +  if (! bfd_get_section_contents (abfd, xstate, contents,
>> +				  I386_LINUX_XSAVE_XCR0_OFFSET, 8))
>>       {
>> -      size_t size = bfd_section_size (xstate);
>> -
>> -      /* Check extended state size.  */
>> -      if (size < X86_XSTATE_AVX_SIZE)
>> -	xcr0 = X86_XSTATE_SSE_MASK;
>> -      else
>> -	{
>> -	  char contents[8];
>> -
>> -	  if (! bfd_get_section_contents (abfd, xstate, contents,
>> -					  I386_LINUX_XSAVE_XCR0_OFFSET,
>> -					  8))
>> -	    {
>> -	      warning (_("Couldn't read `xcr0' bytes from "
>> -			 "`.reg-xstate' section in core file."));
>> -	      return 0;
>> -	    }
>> -
>> -	  xcr0 = bfd_get_64 (abfd, contents);
>> -	}
>> +      warning (_("Couldn't read `xcr0' bytes from "
>> +		 "`.reg-xstate' section in core file."));
>> +      return X86_XSTATE_SSE_MASK;
>>       }
>> -  else
>> -    xcr0 = 0;
>> +
>> +  uint64_t xcr0 = bfd_get_64 (abfd, contents);
>> +
>> +  if (!i387_set_xsave_layout (xcr0, size, layout))
>> +    return X86_XSTATE_SSE_MASK;
>>   
>>     return xcr0;
>>   }
> 
> I think I spotted a change of behavior in
> i386_linux_core_read_xsave_info, which has consequences down the line.
> 
> If there's no .reg-xstate section, we would previously return xcr0 == 0,
> i386_linux_read_description would return nullptr, and we would choose
> between X87 and SSE based on .reg-xfp.
> 
> After the patch, i386_linux_core_read_xsave_info returns SSE, meaning
> we'll never reach the point that checks for .reg-xfp.
> 
> Maybe the first return (if there's not .reg-xstate section) should
> return 0?

Ooo, that's a good catch.  This function is shared with amd64, so I think
it's best if it keeps returning an xcr0 value, but we could patch
i386_linux_core_read_description to instead do this:

static const struct target_desc *
i386_linux_core_read_description (struct gdbarch *gdbarch,
				  struct target_ops *target,
				  bfd *abfd)
{
   /* Linux/i386.  */
   x86_xsave_layout layout;
   uint64_t xcr0 = i386_linux_core_read_xsave_info (abfd, layout);
   if (xcr0 == X86_XSTATE_X87_MASK
       && bfd_get_section_by_name (abfd, ".reg-xfp") != NULL)
     xcr0 = X86_XSTATE_SSE_MASK;

   return i386_linux_read_description (xcr0);
}

-- 
John Baldwin