[v5,09/19] gdb: Update x86 FreeBSD architectures to support XSAVE layouts.
Commit Message
Refactor i386fbsd_core_read_xcr0 to fetch and return a corresponding
x86_xsave_layout as well as xcr0 using the size of an existing
NT_X86_XSTATE core dump to determine the offsets via
i387_set_xsave_layout. Use this to add an implementation of
gdbarch_core_xfer_x86_xsave_layout.
Use tdep->xsave_layout.sizeof_xsave as the size of the XSTATE register
set and only fetch/store the register set if this size is non-zero.
---
gdb/amd64-fbsd-tdep.c | 12 ++++++--
gdb/i386-fbsd-tdep.c | 72 ++++++++++++++++++++++++-------------------
gdb/i386-fbsd-tdep.h | 10 ++++--
3 files changed, 58 insertions(+), 36 deletions(-)
Comments
> @@ -285,7 +291,9 @@ i386fbsd_core_read_description (struct gdbarch *gdbarch,
> struct target_ops *target,
> bfd *abfd)
> {
> - return i386_target_description (i386fbsd_core_read_xcr0 (abfd), true);
> + x86_xsave_layout layout;
> + return i386_target_description (i386_fbsd_core_read_xsave_info (abfd, layout),
> + true);
Reading this gives me some questions. Just thinking out loud, nothing
necessarily actionable at the moment
I found it strange that i386_fbsd_core_read_xsave_info fills an
x86_xsave_layout object that we don't use. We get xcr0 to generate an
appropriate target description here, and later we'll call
target_fetch_x86_xsave_layout (when initializing the gdbarch) to get the
x86_xsave_layout and save it in the i386_gdbarch_tdep object. I'm
wondering if, design-wise, this means that the target_desc object should
carry the xsave layout information. It would be saved in the tdesc
here, and i386_gdbarch_init would just get it from the tdesc.
It's probably not as simple as that, since target descriptions are
transferred as XML from remote targets, and you still have to consider
older target descriptions that wouldn't include that information. But
I'm just trying to think what the ideal design would be.
> diff --git a/gdb/i386-fbsd-tdep.h b/gdb/i386-fbsd-tdep.h
> index cb991af9e49..f96c00d45eb 100644
> --- a/gdb/i386-fbsd-tdep.h
> +++ b/gdb/i386-fbsd-tdep.h
> @@ -20,10 +20,16 @@
> #ifndef I386_FBSD_TDEP_H
> #define I386_FBSD_TDEP_H
>
> +#include "gdbsupport/x86-xstate.h"
> #include "regset.h"
>
> -/* Get XSAVE extended state xcr0 from core dump. */
> -extern uint64_t i386fbsd_core_read_xcr0 (bfd *abfd);
> +/* Validate and fetch XSAVE extended state xcr0 and extended area
> + layout from core dump. */
> +uint64_t i386_fbsd_core_read_xsave_info (bfd *abfd, x86_xsave_layout &layout);
I was a bit confused when I read the comment above for the first time.
Can you rephrase it to make it clear that the function returns the XSAVE
extended state, and fills LAYOUT?
Also, what does "validate" mean, what happens if the thing we validate
is not valid?
Otherwise:
Approved-By: Simon Marchi <simon.marchi@efficios.com>
Simon
On 5/3/23 13:14, Simon Marchi via Gdb-patches wrote:
>> @@ -285,7 +291,9 @@ i386fbsd_core_read_description (struct gdbarch *gdbarch,
>> struct target_ops *target,
>> bfd *abfd)
>> {
>> - return i386_target_description (i386fbsd_core_read_xcr0 (abfd), true);
>> + x86_xsave_layout layout;
>> + return i386_target_description (i386_fbsd_core_read_xsave_info (abfd, layout),
>> + true);
>
> Reading this gives me some questions. Just thinking out loud, nothing
> necessarily actionable at the moment
>
> I found it strange that i386_fbsd_core_read_xsave_info fills an
> x86_xsave_layout object that we don't use. We get xcr0 to generate an
> appropriate target description here, and later we'll call
> target_fetch_x86_xsave_layout (when initializing the gdbarch) to get the
> x86_xsave_layout and save it in the i386_gdbarch_tdep object. I'm
> wondering if, design-wise, this means that the target_desc object should
> carry the xsave layout information. It would be saved in the tdesc
> here, and i386_gdbarch_init would just get it from the tdesc.
>
> It's probably not as simple as that, since target descriptions are
> transferred as XML from remote targets, and you still have to consider
> older target descriptions that wouldn't include that information. But
> I'm just trying to think what the ideal design would be.
>
>> diff --git a/gdb/i386-fbsd-tdep.h b/gdb/i386-fbsd-tdep.h
>> index cb991af9e49..f96c00d45eb 100644
>> --- a/gdb/i386-fbsd-tdep.h
>> +++ b/gdb/i386-fbsd-tdep.h
>> @@ -20,10 +20,16 @@
>> #ifndef I386_FBSD_TDEP_H
>> #define I386_FBSD_TDEP_H
>>
>> +#include "gdbsupport/x86-xstate.h"
>> #include "regset.h"
>>
>> -/* Get XSAVE extended state xcr0 from core dump. */
>> -extern uint64_t i386fbsd_core_read_xcr0 (bfd *abfd);
>> +/* Validate and fetch XSAVE extended state xcr0 and extended area
>> + layout from core dump. */
>> +uint64_t i386_fbsd_core_read_xsave_info (bfd *abfd, x86_xsave_layout &layout);
>
> I was a bit confused when I read the comment above for the first time.
> Can you rephrase it to make it clear that the function returns the XSAVE
> extended state, and fills LAYOUT?
Sorry, I meant "returns xcr0" here.
Simon
On 5/3/23 10:14 AM, Simon Marchi wrote:
>> @@ -285,7 +291,9 @@ i386fbsd_core_read_description (struct gdbarch *gdbarch,
>> struct target_ops *target,
>> bfd *abfd)
>> {
>> - return i386_target_description (i386fbsd_core_read_xcr0 (abfd), true);
>> + x86_xsave_layout layout;
>> + return i386_target_description (i386_fbsd_core_read_xsave_info (abfd, layout),
>> + true);
>
> Reading this gives me some questions. Just thinking out loud, nothing
> necessarily actionable at the moment
>
> I found it strange that i386_fbsd_core_read_xsave_info fills an
> x86_xsave_layout object that we don't use. We get xcr0 to generate an
> appropriate target description here, and later we'll call
> target_fetch_x86_xsave_layout (when initializing the gdbarch) to get the
> x86_xsave_layout and save it in the i386_gdbarch_tdep object. I'm
> wondering if, design-wise, this means that the target_desc object should
> carry the xsave layout information. It would be saved in the tdesc
> here, and i386_gdbarch_init would just get it from the tdesc.
>
> It's probably not as simple as that, since target descriptions are
> transferred as XML from remote targets, and you still have to consider
> older target descriptions that wouldn't include that information. But
> I'm just trying to think what the ideal design would be.
It's kind of odd as the layout doesn't affect the set of registers that
are available (and today tdesc's are AFAICT just about which registers
the target provides).
The reason I extended the existing method that reads xcr0 is that I
needed the section size along with the value of xcr0 to compute the
layout via i386_set_xsave_layout and I was trying to avoid duplicating
the code to fetch the section. I could perhaps change the function
to return the value of xcr0 and the section size instead if that is
less confusing?
>> diff --git a/gdb/i386-fbsd-tdep.h b/gdb/i386-fbsd-tdep.h
>> index cb991af9e49..f96c00d45eb 100644
>> --- a/gdb/i386-fbsd-tdep.h
>> +++ b/gdb/i386-fbsd-tdep.h
>> @@ -20,10 +20,16 @@
>> #ifndef I386_FBSD_TDEP_H
>> #define I386_FBSD_TDEP_H
>>
>> +#include "gdbsupport/x86-xstate.h"
>> #include "regset.h"
>>
>> -/* Get XSAVE extended state xcr0 from core dump. */
>> -extern uint64_t i386fbsd_core_read_xcr0 (bfd *abfd);
>> +/* Validate and fetch XSAVE extended state xcr0 and extended area
>> + layout from core dump. */
>> +uint64_t i386_fbsd_core_read_xsave_info (bfd *abfd, x86_xsave_layout &layout);
>
> I was a bit confused when I read the comment above for the first time.
> Can you rephrase it to make it clear that the function returns the XSAVE
> extended state, and fills LAYOUT?
>
> Also, what does "validate" mean, what happens if the thing we validate
> is not valid?
Hmm, the intent is "Validate and fetch xcr0 and the XSAVE layout". In
particular, this does not return the extended state (which would be the
value of all the AVX registers, etc.) but instead returns only the mask of
enabled state (xcr0) and the layout of the extended area.
If xcr0 does not appear valid or the section doesn't seem to have a valid
size (i386_set_xsave_layout returns false) then the function claims that
only SSE is reported by returning X86_XSTATE_SSE_MASK. That is the
validation that "validate" is referring to.
On 5/3/23 19:45, John Baldwin wrote:
> On 5/3/23 10:14 AM, Simon Marchi wrote:
>>> @@ -285,7 +291,9 @@ i386fbsd_core_read_description (struct gdbarch *gdbarch,
>>> struct target_ops *target,
>>> bfd *abfd)
>>> {
>>> - return i386_target_description (i386fbsd_core_read_xcr0 (abfd), true);
>>> + x86_xsave_layout layout;
>>> + return i386_target_description (i386_fbsd_core_read_xsave_info (abfd, layout),
>>> + true);
>>
>> Reading this gives me some questions. Just thinking out loud, nothing
>> necessarily actionable at the moment
>>
>> I found it strange that i386_fbsd_core_read_xsave_info fills an
>> x86_xsave_layout object that we don't use. We get xcr0 to generate an
>> appropriate target description here, and later we'll call
>> target_fetch_x86_xsave_layout (when initializing the gdbarch) to get the
>> x86_xsave_layout and save it in the i386_gdbarch_tdep object. I'm
>> wondering if, design-wise, this means that the target_desc object should
>> carry the xsave layout information. It would be saved in the tdesc
>> here, and i386_gdbarch_init would just get it from the tdesc.
>>
>> It's probably not as simple as that, since target descriptions are
>> transferred as XML from remote targets, and you still have to consider
>> older target descriptions that wouldn't include that information. But
>> I'm just trying to think what the ideal design would be.
>
> It's kind of odd as the layout doesn't affect the set of registers that
> are available (and today tdesc's are AFAICT just about which registers
> the target provides).
Yeah, so I'm thinking that this could be some arch-specific information
in addition to the register list.
> The reason I extended the existing method that reads xcr0 is that I
> needed the section size along with the value of xcr0 to compute the
> layout via i386_set_xsave_layout and I was trying to avoid duplicating
> the code to fetch the section. I could perhaps change the function
> to return the value of xcr0 and the section size instead if that is
> less confusing?
I don't think any change is necessary to your code, it's ok like that.
>>> diff --git a/gdb/i386-fbsd-tdep.h b/gdb/i386-fbsd-tdep.h
>>> index cb991af9e49..f96c00d45eb 100644
>>> --- a/gdb/i386-fbsd-tdep.h
>>> +++ b/gdb/i386-fbsd-tdep.h
>>> @@ -20,10 +20,16 @@
>>> #ifndef I386_FBSD_TDEP_H
>>> #define I386_FBSD_TDEP_H
>>> +#include "gdbsupport/x86-xstate.h"
>>> #include "regset.h"
>>> -/* Get XSAVE extended state xcr0 from core dump. */
>>> -extern uint64_t i386fbsd_core_read_xcr0 (bfd *abfd);
>>> +/* Validate and fetch XSAVE extended state xcr0 and extended area
>>> + layout from core dump. */
>>> +uint64_t i386_fbsd_core_read_xsave_info (bfd *abfd, x86_xsave_layout &layout);
>>
>> I was a bit confused when I read the comment above for the first time.
>> Can you rephrase it to make it clear that the function returns the XSAVE
>> extended state, and fills LAYOUT?
>>
>> Also, what does "validate" mean, what happens if the thing we validate
>> is not valid?
>
> Hmm, the intent is "Validate and fetch xcr0 and the XSAVE layout". In
> particular, this does not return the extended state (which would be the
> value of all the AVX registers, etc.) but instead returns only the mask of
> enabled state (xcr0) and the layout of the extended area.
>
> If xcr0 does not appear valid or the section doesn't seem to have a valid
> size (i386_set_xsave_layout returns false) then the function claims that
> only SSE is reported by returning X86_XSTATE_SSE_MASK. That is the
> validation that "validate" is referring to.
Maybe it's the two "and" in the sentence that make it ambiguous. So,
perhaps:
Read the xcr0 value from the ABFD core file. If it appears to be valid,
return it and fill LAYOUT with values inferred from that value.
Otherwise, return X86_XSTATE_SSE_MASK as a fallback and leave LAYOUT
untouched.
Simon
On 5/4/23 10:20 AM, Simon Marchi wrote:
> On 5/3/23 19:45, John Baldwin wrote:
>> On 5/3/23 10:14 AM, Simon Marchi wrote:
>>>> diff --git a/gdb/i386-fbsd-tdep.h b/gdb/i386-fbsd-tdep.h
>>>> index cb991af9e49..f96c00d45eb 100644
>>>> --- a/gdb/i386-fbsd-tdep.h
>>>> +++ b/gdb/i386-fbsd-tdep.h
>>>> @@ -20,10 +20,16 @@
>>>> #ifndef I386_FBSD_TDEP_H
>>>> #define I386_FBSD_TDEP_H
>>>> +#include "gdbsupport/x86-xstate.h"
>>>> #include "regset.h"
>>>> -/* Get XSAVE extended state xcr0 from core dump. */
>>>> -extern uint64_t i386fbsd_core_read_xcr0 (bfd *abfd);
>>>> +/* Validate and fetch XSAVE extended state xcr0 and extended area
>>>> + layout from core dump. */
>>>> +uint64_t i386_fbsd_core_read_xsave_info (bfd *abfd, x86_xsave_layout &layout);
>>>
>>> I was a bit confused when I read the comment above for the first time.
>>> Can you rephrase it to make it clear that the function returns the XSAVE
>>> extended state, and fills LAYOUT?
>>>
>>> Also, what does "validate" mean, what happens if the thing we validate
>>> is not valid?
>>
>> Hmm, the intent is "Validate and fetch xcr0 and the XSAVE layout". In
>> particular, this does not return the extended state (which would be the
>> value of all the AVX registers, etc.) but instead returns only the mask of
>> enabled state (xcr0) and the layout of the extended area.
>>
>> If xcr0 does not appear valid or the section doesn't seem to have a valid
>> size (i386_set_xsave_layout returns false) then the function claims that
>> only SSE is reported by returning X86_XSTATE_SSE_MASK. That is the
>> validation that "validate" is referring to.
>
> Maybe it's the two "and" in the sentence that make it ambiguous. So,
> perhaps:
>
> Read the xcr0 value from the ABFD core file. If it appears to be valid,
> return it and fill LAYOUT with values inferred from that value.
>
> Otherwise, return X86_XSTATE_SSE_MASK as a fallback and leave LAYOUT
> untouched.
Yes, that is clearer. I've fixed it here and in the similar commit to
i386-linux-tdep.h.
@@ -224,7 +224,9 @@ amd64fbsd_core_read_description (struct gdbarch *gdbarch,
struct target_ops *target,
bfd *abfd)
{
- return amd64_target_description (i386fbsd_core_read_xcr0 (abfd), true);
+ x86_xsave_layout layout;
+ return amd64_target_description (i386_fbsd_core_read_xsave_info (abfd, layout),
+ true);
}
/* Similar to amd64_supply_fpregset, but use XSAVE extended state. */
@@ -271,8 +273,10 @@ amd64fbsd_iterate_over_regset_sections (struct gdbarch *gdbarch,
cb (".reg-x86-segbases", AMD64_FBSD_SIZEOF_SEGBASES_REGSET,
AMD64_FBSD_SIZEOF_SEGBASES_REGSET, &amd64_fbsd_segbases_regset,
"segment bases", cb_data);
- cb (".reg-xstate", X86_XSTATE_SIZE (tdep->xcr0), X86_XSTATE_SIZE (tdep->xcr0),
- &amd64fbsd_xstateregset, "XSAVE extended state", cb_data);
+ if (tdep->xsave_layout.sizeof_xsave != 0)
+ cb (".reg-xstate", tdep->xsave_layout.sizeof_xsave,
+ tdep->xsave_layout.sizeof_xsave, &amd64fbsd_xstateregset,
+ "XSAVE extended state", cb_data);
}
/* Implement the get_thread_local_address gdbarch method. */
@@ -313,6 +317,8 @@ amd64fbsd_init_abi (struct gdbarch_info info, struct gdbarch *gdbarch)
tramp_frame_prepend_unwinder (gdbarch, &amd64_fbsd_sigframe);
tdep->xsave_xcr0_offset = I386_FBSD_XSAVE_XCR0_OFFSET;
+ set_gdbarch_core_read_x86_xsave_layout
+ (gdbarch, i386_fbsd_core_read_x86_xsave_layout);
/* Iterate over core file register note sections. */
set_gdbarch_iterate_over_regset_sections
@@ -18,13 +18,13 @@
along with this program. If not, see <http://www.gnu.org/licenses/>. */
#include "defs.h"
+#include "gdbcore.h"
#include "osabi.h"
#include "regcache.h"
#include "regset.h"
#include "trad-frame.h"
#include "tramp-frame.h"
#include "i386-fbsd-tdep.h"
-#include "gdbsupport/x86-xstate.h"
#include "i386-tdep.h"
#include "i387-tdep.h"
@@ -241,43 +241,49 @@ static const struct tramp_frame i386_fbsd64_sigframe =
i386_fbsd_sigframe_init
};
-/* Get XSAVE extended state xcr0 from core dump. */
+/* See i386-fbsd-tdep.h. */
uint64_t
-i386fbsd_core_read_xcr0 (bfd *abfd)
+i386_fbsd_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_FBSD_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_FBSD_XSAVE_XCR0_OFFSET,
- 8))
- {
- warning (_("Couldn't read `xcr0' bytes from "
- "`.reg-xstate' section in core file."));
- return X86_XSTATE_SSE_MASK;
- }
-
- 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 = X86_XSTATE_SSE_MASK;
+
+ uint64_t xcr0 = bfd_get_64 (abfd, contents);
+
+ if (!i387_set_xsave_layout (xcr0, size, layout))
+ return X86_XSTATE_SSE_MASK;
return xcr0;
}
+/* Implement the core_read_x86_xsave_layout gdbarch method. */
+
+bool
+i386_fbsd_core_read_x86_xsave_layout (struct gdbarch *gdbarch,
+ x86_xsave_layout &layout)
+{
+ if (i386_fbsd_core_read_xsave_info (core_bfd, layout) == X86_XSTATE_SSE_MASK)
+ return false;
+
+ return true;
+}
+
/* Implement the core_read_description gdbarch method. */
static const struct target_desc *
@@ -285,7 +291,9 @@ i386fbsd_core_read_description (struct gdbarch *gdbarch,
struct target_ops *target,
bfd *abfd)
{
- return i386_target_description (i386fbsd_core_read_xcr0 (abfd), true);
+ x86_xsave_layout layout;
+ return i386_target_description (i386_fbsd_core_read_xsave_info (abfd, layout),
+ true);
}
/* Similar to i386_supply_fpregset, but use XSAVE extended state. */
@@ -335,9 +343,9 @@ i386fbsd_iterate_over_regset_sections (struct gdbarch *gdbarch,
I386_FBSD_SIZEOF_SEGBASES_REGSET, &i386_fbsd_segbases_regset,
"segment bases", cb_data);
- if (tdep->xcr0 & X86_XSTATE_AVX)
- cb (".reg-xstate", X86_XSTATE_SIZE (tdep->xcr0),
- X86_XSTATE_SIZE (tdep->xcr0), &i386fbsd_xstateregset,
+ if (tdep->xsave_layout.sizeof_xsave != 0)
+ cb (".reg-xstate", tdep->xsave_layout.sizeof_xsave,
+ tdep->xsave_layout.sizeof_xsave, &i386fbsd_xstateregset,
"XSAVE extended state", cb_data);
}
@@ -386,6 +394,8 @@ i386fbsd_init_abi (struct gdbarch_info info, struct gdbarch *gdbarch)
i386_elf_init_abi (info, gdbarch);
tdep->xsave_xcr0_offset = I386_FBSD_XSAVE_XCR0_OFFSET;
+ set_gdbarch_core_read_x86_xsave_layout
+ (gdbarch, i386_fbsd_core_read_x86_xsave_layout);
/* Iterate over core file register note sections. */
set_gdbarch_iterate_over_regset_sections
@@ -20,10 +20,16 @@
#ifndef I386_FBSD_TDEP_H
#define I386_FBSD_TDEP_H
+#include "gdbsupport/x86-xstate.h"
#include "regset.h"
-/* Get XSAVE extended state xcr0 from core dump. */
-extern uint64_t i386fbsd_core_read_xcr0 (bfd *abfd);
+/* Validate and fetch XSAVE extended state xcr0 and extended area
+ layout from core dump. */
+uint64_t i386_fbsd_core_read_xsave_info (bfd *abfd, x86_xsave_layout &layout);
+
+/* Implement the core_read_x86_xsave_layout gdbarch method. */
+bool i386_fbsd_core_read_x86_xsave_layout (struct gdbarch *gdbarch,
+ x86_xsave_layout &layout);
/* The format of the XSAVE extended area is determined by hardware.
Cores store the XSAVE extended area in a NT_X86_XSTATE note that