On 4/27/23 17:00, John Baldwin wrote:
> This structure is fetched from the current target in i386_gdbarch_init
> via a new "fetch_x86_xsave_layout" target method.
I got:
Applying: gdb: Store an x86_xsave_layout in i386_gdbarch_tdep.
.git/rebase-apply/patch:147: new blank line at EOF.
+
warning: 1 line adds whitespace errors.
> ---
> gdb/i386-tdep.c | 19 ++++++++++++++++++-
> gdb/i386-tdep.h | 4 ++++
> gdb/target-debug.h | 20 ++++++++++++++++++++
> gdb/target-delegates.c | 27 +++++++++++++++++++++++++++
> gdb/target.c | 6 ++++++
> gdb/target.h | 7 +++++++
> 6 files changed, 82 insertions(+), 1 deletion(-)
>
> diff --git a/gdb/i386-tdep.c b/gdb/i386-tdep.c
> index 1ab9fc0e87d..43f2ae6c14e 100644
> --- a/gdb/i386-tdep.c
> +++ b/gdb/i386-tdep.c
> @@ -8504,8 +8504,24 @@ i386_gdbarch_init (struct gdbarch_info info, struct gdbarch_list *arches)
> int bnd0_regnum;
> int num_bnd_cooked;
>
> + x86_xsave_layout xsave_layout = target_fetch_x86_xsave_layout ();
> +
> /* If there is already a candidate, use it. */
> - arches = gdbarch_list_lookup_by_info (arches, &info);
> + for (arches = gdbarch_list_lookup_by_info (arches, &info);
> + arches != NULL;
> + arches = gdbarch_list_lookup_by_info (arches->next, &info))
> + {
> + /* Check that the XSAVE layout of ARCHES matches the layout for
> + the current target. */
> + i386_gdbarch_tdep *other_tdep
> + = gdbarch_tdep<i386_gdbarch_tdep> (arches->gdbarch);
> +
> + if (other_tdep->xsave_layout != xsave_layout)
> + continue;
> +
> + break;
> + }
> +
> if (arches != NULL)
> return arches->gdbarch;
I think that returningd from the loop would be a tiny bit simpler
(replacing the break with `return arches->gdbarch`. You could drop this
if.
Other than that:
Approved-By: Simon Marchi <simon.marchi@efficios.com>
Simon
On 5/3/23 9:22 AM, Simon Marchi wrote:
> On 4/27/23 17:00, John Baldwin wrote:
>> This structure is fetched from the current target in i386_gdbarch_init
>> via a new "fetch_x86_xsave_layout" target method.
>
> I got:
>
> Applying: gdb: Store an x86_xsave_layout in i386_gdbarch_tdep.
> .git/rebase-apply/patch:147: new blank line at EOF.
> +
> warning: 1 line adds whitespace errors.
Humm, this is is target-delegates.c which is generated by
make-target-delegates.py. The current version of this file also
has an extra blank line of EOF.
>> ---
>> gdb/i386-tdep.c | 19 ++++++++++++++++++-
>> gdb/i386-tdep.h | 4 ++++
>> gdb/target-debug.h | 20 ++++++++++++++++++++
>> gdb/target-delegates.c | 27 +++++++++++++++++++++++++++
>> gdb/target.c | 6 ++++++
>> gdb/target.h | 7 +++++++
>> 6 files changed, 82 insertions(+), 1 deletion(-)
>>
>> diff --git a/gdb/i386-tdep.c b/gdb/i386-tdep.c
>> index 1ab9fc0e87d..43f2ae6c14e 100644
>> --- a/gdb/i386-tdep.c
>> +++ b/gdb/i386-tdep.c
>> @@ -8504,8 +8504,24 @@ i386_gdbarch_init (struct gdbarch_info info, struct gdbarch_list *arches)
>> int bnd0_regnum;
>> int num_bnd_cooked;
>>
>> + x86_xsave_layout xsave_layout = target_fetch_x86_xsave_layout ();
>> +
>> /* If there is already a candidate, use it. */
>> - arches = gdbarch_list_lookup_by_info (arches, &info);
>> + for (arches = gdbarch_list_lookup_by_info (arches, &info);
>> + arches != NULL;
>> + arches = gdbarch_list_lookup_by_info (arches->next, &info))
>> + {
>> + /* Check that the XSAVE layout of ARCHES matches the layout for
>> + the current target. */
>> + i386_gdbarch_tdep *other_tdep
>> + = gdbarch_tdep<i386_gdbarch_tdep> (arches->gdbarch);
>> +
>> + if (other_tdep->xsave_layout != xsave_layout)
>> + continue;
>> +
>> + break;
>> + }
>> +
>> if (arches != NULL)
>> return arches->gdbarch;
>
> I think that returningd from the loop would be a tiny bit simpler
> (replacing the break with `return arches->gdbarch`. You could drop this
> if.
Ok. I had thought this pattern was used more consistently in various *gdbarch_init
routines, but looking at them now, aarch64 does an early return as you mention,
and MIPS does a weird hybrid. riscv does something akin to the above. I'll change
it to do a == and quick return.
I did have to update patch 1 to add an operator== for this to work FYI.
> Other than that:
>
> Approved-By: Simon Marchi <simon.marchi@efficios.com>
>
> Simon
@@ -8504,8 +8504,24 @@ i386_gdbarch_init (struct gdbarch_info info, struct gdbarch_list *arches)
int bnd0_regnum;
int num_bnd_cooked;
+ x86_xsave_layout xsave_layout = target_fetch_x86_xsave_layout ();
+
/* If there is already a candidate, use it. */
- arches = gdbarch_list_lookup_by_info (arches, &info);
+ for (arches = gdbarch_list_lookup_by_info (arches, &info);
+ arches != NULL;
+ arches = gdbarch_list_lookup_by_info (arches->next, &info))
+ {
+ /* Check that the XSAVE layout of ARCHES matches the layout for
+ the current target. */
+ i386_gdbarch_tdep *other_tdep
+ = gdbarch_tdep<i386_gdbarch_tdep> (arches->gdbarch);
+
+ if (other_tdep->xsave_layout != xsave_layout)
+ continue;
+
+ break;
+ }
+
if (arches != NULL)
return arches->gdbarch;
@@ -8762,6 +8778,7 @@ i386_gdbarch_init (struct gdbarch_info info, struct gdbarch_list *arches)
gdbarch_free (gdbarch);
return NULL;
}
+ tdep->xsave_layout = xsave_layout;
num_bnd_cooked = (tdep->bnd0r_regnum > 0 ? I387_NUM_BND_REGS : 0);
@@ -23,6 +23,7 @@
#include "gdbarch.h"
#include "infrun.h"
#include "expression.h"
+#include "gdbsupport/x86-xstate.h"
class frame_info_ptr;
struct gdbarch;
@@ -145,6 +146,9 @@ struct i386_gdbarch_tdep : gdbarch_tdep_base
/* Offset of XCR0 in XSAVE extended state. */
int xsave_xcr0_offset = 0;
+ /* Layout of the XSAVE area extended region. */
+ x86_xsave_layout xsave_layout;
+
/* Register names. */
const char * const *register_names = nullptr;
@@ -236,4 +236,24 @@ target_debug_print_gdb_byte_vector_r (gdb::byte_vector &vector)
{
target_debug_print_const_gdb_byte_vector_r (vector);
}
+
+static void
+target_debug_print_x86_xsave_layout (const x86_xsave_layout &layout)
+{
+ gdb_puts ("{", gdb_stdlog);
+ gdb_printf (gdb_stdlog, " sizeof_xsave=%d", layout.sizeof_xsave);
+#define POFFS(region) \
+ if (layout.region##_offset != 0) \
+ gdb_printf (gdb_stdlog, ", %s_offset=%d", #region, \
+ layout.region##_offset)
+ POFFS(avx);
+ POFFS(bndregs);
+ POFFS(bndcfg);
+ POFFS(k);
+ POFFS(zmm_h);
+ POFFS(zmm);
+ POFFS(pkru);
+#undef POFFS
+ gdb_puts (" }", gdb_stdlog);
+}
#endif /* TARGET_DEBUG_H */
@@ -196,6 +196,7 @@ struct dummy_target : public target_ops
bool supports_memory_tagging () override;
bool fetch_memtags (CORE_ADDR arg0, size_t arg1, gdb::byte_vector &arg2, int arg3) override;
bool store_memtags (CORE_ADDR arg0, size_t arg1, const gdb::byte_vector &arg2, int arg3) override;
+ x86_xsave_layout fetch_x86_xsave_layout () override;
};
struct debug_target : public target_ops
@@ -370,6 +371,7 @@ struct debug_target : public target_ops
bool supports_memory_tagging () override;
bool fetch_memtags (CORE_ADDR arg0, size_t arg1, gdb::byte_vector &arg2, int arg3) override;
bool store_memtags (CORE_ADDR arg0, size_t arg1, const gdb::byte_vector &arg2, int arg3) override;
+ x86_xsave_layout fetch_x86_xsave_layout () override;
};
void
@@ -4536,3 +4538,28 @@ debug_target::store_memtags (CORE_ADDR arg0, size_t arg1, const gdb::byte_vector
return result;
}
+x86_xsave_layout
+target_ops::fetch_x86_xsave_layout ()
+{
+ return this->beneath ()->fetch_x86_xsave_layout ();
+}
+
+x86_xsave_layout
+dummy_target::fetch_x86_xsave_layout ()
+{
+ return x86_xsave_layout ();
+}
+
+x86_xsave_layout
+debug_target::fetch_x86_xsave_layout ()
+{
+ x86_xsave_layout result;
+ gdb_printf (gdb_stdlog, "-> %s->fetch_x86_xsave_layout (...)\n", this->beneath ()->shortname ());
+ result = this->beneath ()->fetch_x86_xsave_layout ();
+ gdb_printf (gdb_stdlog, "<- %s->fetch_x86_xsave_layout (", this->beneath ()->shortname ());
+ gdb_puts (") = ", gdb_stdlog);
+ target_debug_print_x86_xsave_layout (result);
+ gdb_puts ("\n", gdb_stdlog);
+ return result;
+}
+
@@ -832,6 +832,12 @@ target_store_memtags (CORE_ADDR address, size_t len,
return current_inferior ()->top_target ()->store_memtags (address, len, tags, type);
}
+x86_xsave_layout
+target_fetch_x86_xsave_layout ()
+{
+ return current_inferior ()->top_target ()->fetch_x86_xsave_layout ();
+}
+
void
target_log_command (const char *p)
{
@@ -82,6 +82,7 @@ struct inferior;
#include "disasm-flags.h"
#include "tracepoint.h"
#include "gdbsupport/fileio.h"
+#include "gdbsupport/x86-xstate.h"
#include "gdbsupport/break-common.h" /* For enum target_hw_bp_type. */
@@ -1321,6 +1322,10 @@ struct target_ops
virtual bool store_memtags (CORE_ADDR address, size_t len,
const gdb::byte_vector &tags, int type)
TARGET_DEFAULT_NORETURN (tcomplain ());
+
+ /* Return the x86 XSAVE extended state area layout. */
+ virtual x86_xsave_layout fetch_x86_xsave_layout ()
+ TARGET_DEFAULT_RETURN (x86_xsave_layout ());
};
/* Deleter for std::unique_ptr. See comments in
@@ -2310,6 +2315,8 @@ extern bool target_fetch_memtags (CORE_ADDR address, size_t len,
extern bool target_store_memtags (CORE_ADDR address, size_t len,
const gdb::byte_vector &tags, int type);
+extern x86_xsave_layout target_fetch_x86_xsave_layout ();
+
/* Command logging facility. */
extern void target_log_command (const char *p);