[v5,02/19] gdb: Store an x86_xsave_layout in i386_gdbarch_tdep.

Message ID 20230427210113.45380-3-jhb@FreeBSD.org
State New
Headers
Series Handle variable XSAVE layouts |

Commit Message

John Baldwin April 27, 2023, 9 p.m. UTC
  This structure is fetched from the current target in i386_gdbarch_init
via a new "fetch_x86_xsave_layout" target method.
---
 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(-)
  

Comments

Simon Marchi May 3, 2023, 4:22 p.m. UTC | #1
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
  
John Baldwin May 8, 2023, 4:51 p.m. UTC | #2
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
  

Patch

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;
 
@@ -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);
 
diff --git a/gdb/i386-tdep.h b/gdb/i386-tdep.h
index 642ac89b240..0586a45bd55 100644
--- a/gdb/i386-tdep.h
+++ b/gdb/i386-tdep.h
@@ -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;
 
diff --git a/gdb/target-debug.h b/gdb/target-debug.h
index acb01d47e7c..6b8983104ea 100644
--- a/gdb/target-debug.h
+++ b/gdb/target-debug.h
@@ -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 */
diff --git a/gdb/target-delegates.c b/gdb/target-delegates.c
index 57b66ce87b1..1d1f8c60f4e 100644
--- a/gdb/target-delegates.c
+++ b/gdb/target-delegates.c
@@ -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;
+}
+
diff --git a/gdb/target.c b/gdb/target.c
index 0cebecfafc3..7d9abc0d54e 100644
--- a/gdb/target.c
+++ b/gdb/target.c
@@ -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)
 {
diff --git a/gdb/target.h b/gdb/target.h
index 2dac86c394d..c459469d5eb 100644
--- a/gdb/target.h
+++ b/gdb/target.h
@@ -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);