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

John Baldwin
2023-07-26T21:17:19Z

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