RE: [RFC PATCH v2 2/5] core: Support fetching TARGET_OBJECT_X86_XSAVE_LAYOUT from architectures.

["George, Jini Susan" <JiniSusan.George@amd.com>]

[Public]

Thanks for making these changes, John. I am not a gdb maintainer, but I am inclined to be in favor of these changes till we have modifications in the kernel to store the xsave layout indicators in the coredump. AFAIK, yes, AMD CPUs never implemented MPX, and hence the gap wrt the xsave layout of Intel. (There is an additional unaccounted gap of 128 bytes too). I am not sure as to why there is no space reserved in the standard form for MPX, but it is for AVX-512 in CPUs where AVX-512 is not implemented. I am trying to find this out, but at this point, I don't have answers for this.

Are these changes catering to corefile debugging only ? If so, are you planning to make changes for live debugging ? If not, could you please let me know ? If you plan to proceed with the corresponding changes for Linux, I would be interested in looking at that patch too.

One comment that I have is for the code in i387_set_xsave_layout(). Since the hardware provides a finer granularity in setting the individual xfeature mask bits for MPX and AVX-512, I am wondering if it might be better to consider more scenarios like:

...
else if  (HAS_AVX512(xcr0) && xsave_size == 2432) /* AMD CPUs with AVX-512 */
...
else if (HAS_AVX512_ZMM_HI(xcr0) && xsave_size == 1408)
...
else if  (HAS_AVX512_ZMM_KREGS(xcr0) && xsave_size == 896)
...
etc.

(though I don't know under what circumstances, you can have only certain components of AVX-512 or MPX turned on) -- but this would be in line with the rest of the code changes. We also might be able to make the code simpler since the post MPX xsave extended feature offsets (from opmask onwards) of AMD would be at an offset which is 256 bytes lesser than the corresponding ones of Intel.

AMD processors based on the Bulldozer architecture have the Light Weight Profiling regs placed at offset 832 in the xsave area -- but I believe we can safely disregard that.

Thanks,
Jini.

>>-----Original Message-----
>>From: Gdb-patches <gdb-patches-
>>bounces+jigeorge=amd.com@sourceware.org> On Behalf Of John Baldwin
>>Sent: Friday, March 18, 2022 12:06 AM
>>To: gdb-patches@sourceware.org
>>Cc: Willgerodt@sourceware.org
>>Subject: [RFC PATCH v2 2/5] core: Support fetching
>>TARGET_OBJECT_X86_XSAVE_LAYOUT from architectures.
>>
>>[CAUTION: External Email]
>>
>>Add gdbarch_core_xfer_x86_xsave_layout to fetch the x86 XSAVE layout
>>structure from a core file.
>>
>>Current OS's do not export the offsets of XSAVE state components in core
>>dumps, so provide an i387_set_xsave_layout helper function to set offsets
>>based on known combinations of XCR0 masks and total state sizes.  Eventually
>>when core dumps do contain this information this function should only be used
>>as a fall back for older core dumps.
>>---
>> gdb/corelow.c             | 22 +++++++++++++++++
>> gdb/gdbarch-components.py | 13 ++++++++++
>> gdb/gdbarch-gen.h         | 10 ++++++++
>> gdb/gdbarch.c             | 32 ++++++++++++++++++++++++
>> gdb/i387-tdep.c           | 51 +++++++++++++++++++++++++++++++++++++++
>> gdb/i387-tdep.h           |  7 ++++++
>> 6 files changed, 135 insertions(+)
>>
>>diff --git a/gdb/corelow.c b/gdb/corelow.c index 001c4f147fc..71bfdcff9dd
>>100644
>>--- a/gdb/corelow.c
>>+++ b/gdb/corelow.c
>>@@ -987,6 +987,28 @@ core_target::xfer_partial (enum target_object object,
>>const char *annex,
>>        }
>>       return TARGET_XFER_E_IO;
>>
>>+    case TARGET_OBJECT_X86_XSAVE_LAYOUT:
>>+      if (readbuf)
>>+       {
>>+         if (m_core_gdbarch != nullptr
>>+             && gdbarch_core_xfer_x86_xsave_layout_p (m_core_gdbarch))
>>+           {
>>+             LONGEST l = gdbarch_core_xfer_x86_xsave_layout  (m_core_gdbarch,
>>+                                                              readbuf, offset,
>>+                                                              len);
>>+
>>+             if (l >= 0)
>>+               {
>>+                 *xfered_len = l;
>>+                 if (l == 0)
>>+                   return TARGET_XFER_EOF;
>>+                 else
>>+                   return TARGET_XFER_OK;
>>+               }
>>+           }
>>+       }
>>+      return TARGET_XFER_E_IO;
>>+
>>     default:
>>       return this->beneath ()->xfer_partial (object, annex, readbuf,
>>                                             writebuf, offset, len, diff --git a/gdb/gdbarch-
>>components.py b/gdb/gdbarch-components.py index
>>c820ddae764..99eaca7f7e2 100644
>>--- a/gdb/gdbarch-components.py
>>+++ b/gdb/gdbarch-components.py
>>@@ -1584,6 +1584,19 @@ of bytes read (zero indicates EOF, a negative value
>>indicates failure).
>>     invalid=True,
>> )
>>
>>+Method(
>>+    comment="""
>>+Read offset OFFSET of TARGET_OBJECT_X86_XSAVE_LAYOUT from core file
>>+into buffer READBUF with length LEN.  Return the number of bytes read
>>+(zero indicates EOF, a negative value indicates failure).
>>+""",
>>+    type="LONGEST",
>>+    name="core_xfer_x86_xsave_layout",
>>+    params=[("gdb_byte *", "readbuf"), ("ULONGEST", "offset"), ("ULONGEST",
>>"len")],
>>+    predicate=True,
>>+    invalid=True,
>>+)
>>+
>> Value(
>>     comment="""
>> BFD target to use when generating a core file.
>>diff --git a/gdb/gdbarch-gen.h b/gdb/gdbarch-gen.h index
>>7a8721328ab..82292f9c954 100644
>>--- a/gdb/gdbarch-gen.h
>>+++ b/gdb/gdbarch-gen.h
>>@@ -921,6 +921,16 @@ typedef LONGEST (gdbarch_core_xfer_siginfo_ftype)
>>(struct gdbarch *gdbarch, gdb_  extern LONGEST gdbarch_core_xfer_siginfo
>>(struct gdbarch *gdbarch, gdb_byte *readbuf, ULONGEST offset, ULONGEST
>>len);  extern void set_gdbarch_core_xfer_siginfo (struct gdbarch *gdbarch,
>>gdbarch_core_xfer_siginfo_ftype *core_xfer_siginfo);
>>
>>+/* Read offset OFFSET of TARGET_OBJECT_X86_XSAVE_LAYOUT from core file
>>+   into buffer READBUF with length LEN.  Return the number of bytes read
>>+   (zero indicates EOF, a negative value indicates failure). */
>>+
>>+extern bool gdbarch_core_xfer_x86_xsave_layout_p (struct gdbarch
>>+*gdbarch);
>>+
>>+typedef LONGEST (gdbarch_core_xfer_x86_xsave_layout_ftype) (struct
>>+gdbarch *gdbarch, gdb_byte *readbuf, ULONGEST offset, ULONGEST len);
>>+extern LONGEST gdbarch_core_xfer_x86_xsave_layout (struct gdbarch
>>+*gdbarch, gdb_byte *readbuf, ULONGEST offset, ULONGEST len); extern
>>+void set_gdbarch_core_xfer_x86_xsave_layout (struct gdbarch *gdbarch,
>>+gdbarch_core_xfer_x86_xsave_layout_ftype *core_xfer_x86_xsave_layout);
>>+
>> /* BFD target to use when generating a core file. */
>>
>> extern bool gdbarch_gcore_bfd_target_p (struct gdbarch *gdbarch); diff --git
>>a/gdb/gdbarch.c b/gdb/gdbarch.c index 9fdcf1505fe..e8681d8930b 100644
>>--- a/gdb/gdbarch.c
>>+++ b/gdb/gdbarch.c
>>@@ -176,6 +176,7 @@ struct gdbarch
>>   gdbarch_core_pid_to_str_ftype *core_pid_to_str;
>>   gdbarch_core_thread_name_ftype *core_thread_name;
>>   gdbarch_core_xfer_siginfo_ftype *core_xfer_siginfo;
>>+  gdbarch_core_xfer_x86_xsave_layout_ftype *core_xfer_x86_xsave_layout;
>>   const char * gcore_bfd_target;
>>   int vtable_function_descriptors;
>>   int vbit_in_delta;
>>@@ -532,6 +533,7 @@ verify_gdbarch (struct gdbarch *gdbarch)
>>   /* Skip verify of core_pid_to_str, has predicate.  */
>>   /* Skip verify of core_thread_name, has predicate.  */
>>   /* Skip verify of core_xfer_siginfo, has predicate.  */
>>+  /* Skip verify of core_xfer_x86_xsave_layout, has predicate.  */
>>   /* Skip verify of gcore_bfd_target, has predicate.  */
>>   /* Skip verify of vtable_function_descriptors, invalid_p == 0 */
>>   /* Skip verify of vbit_in_delta, invalid_p == 0 */ @@ -1126,6 +1128,12 @@
>>gdbarch_dump (struct gdbarch *gdbarch, struct ui_file *file)
>>   fprintf_filtered (file,
>>                       "gdbarch_dump: core_xfer_siginfo = <%s>\n",
>>                       host_address_to_string (gdbarch->core_xfer_siginfo));
>>+  fprintf_filtered (file,
>>+                      "gdbarch_dump: gdbarch_core_xfer_x86_xsave_layout_p() =
>>%d\n",
>>+                      gdbarch_core_xfer_x86_xsave_layout_p (gdbarch));
>>+ fprintf_filtered (file,
>>+                      "gdbarch_dump: core_xfer_x86_xsave_layout = <%s>\n",
>>+                      host_address_to_string
>>+ (gdbarch->core_xfer_x86_xsave_layout));
>>   fprintf_filtered (file,
>>                       "gdbarch_dump: gdbarch_gcore_bfd_target_p() = %d\n",
>>                       gdbarch_gcore_bfd_target_p (gdbarch)); @@ -3864,6 +3872,30
>>@@ set_gdbarch_core_xfer_siginfo (struct gdbarch *gdbarch,
>>   gdbarch->core_xfer_siginfo = core_xfer_siginfo;  }
>>
>>+bool
>>+gdbarch_core_xfer_x86_xsave_layout_p (struct gdbarch *gdbarch) {
>>+  gdb_assert (gdbarch != NULL);
>>+  return gdbarch->core_xfer_x86_xsave_layout != NULL; }
>>+
>>+LONGEST
>>+gdbarch_core_xfer_x86_xsave_layout (struct gdbarch *gdbarch, gdb_byte
>>+*readbuf, ULONGEST offset, ULONGEST len) {
>>+  gdb_assert (gdbarch != NULL);
>>+  gdb_assert (gdbarch->core_xfer_x86_xsave_layout != NULL);
>>+  if (gdbarch_debug >= 2)
>>+    fprintf_unfiltered (gdb_stdlog, "gdbarch_core_xfer_x86_xsave_layout
>>+called\n");
>>+  return gdbarch->core_xfer_x86_xsave_layout (gdbarch, readbuf, offset,
>>+len); }
>>+
>>+void
>>+set_gdbarch_core_xfer_x86_xsave_layout (struct gdbarch *gdbarch,
>>+
>>+gdbarch_core_xfer_x86_xsave_layout_ftype core_xfer_x86_xsave_layout) {
>>+  gdbarch->core_xfer_x86_xsave_layout = core_xfer_x86_xsave_layout; }
>>+
>> bool
>> gdbarch_gcore_bfd_target_p (struct gdbarch *gdbarch)  { diff --git a/gdb/i387-
>>tdep.c b/gdb/i387-tdep.c index 2f0b6509457..83df8a7fb0f 100644
>>--- a/gdb/i387-tdep.c
>>+++ b/gdb/i387-tdep.c
>>@@ -898,6 +898,57 @@ static int xsave_pkeys_offset[] =
>>   (xsave + xsave_pkeys_offset[regnum - I387_PKRU_REGNUM (tdep)])
>>
>>
>>+/* See i387-tdep.h.  */
>>+
>>+void
>>+i387_set_xsave_layout (uint64_t xcr0, size_t xsave_size,
>>+                      x86_xsave_layout *layout) {
>>+  layout->sizeof_xsave = xsave_size;
>>+  if (HAS_PKRU(xcr0) && xsave_size == 2696)
>>+    {
>>+      /* Intel CPUs supporting PKRU.  */
>>+      layout->avx_offset = 576;
>>+      layout->bndregs_offset = 960;
>>+      layout->bndcfg_offset = 1024;
>>+      layout->avx512_k_offset = 1088;
>>+      layout->avx512_zmm_h_offset = 1152;
>>+      layout->avx512_zmm_offset = 1664;
>>+      layout->pkru_offset = 2688;
>>+    }
>>+  else if (HAS_PKRU(xcr0) && xsave_size == 2440)
>>+    {
>>+      /* AMD Ryzen 9 CPUs supporting PKRU.  */
>>+      layout->avx_offset = 576;
>>+      layout->avx512_k_offset = 832;
>>+      layout->avx512_zmm_h_offset = 896;
>>+      layout->avx512_zmm_offset = 1408;
>>+      layout->pkru_offset = 2432;
>>+    }
>>+  else if (HAS_AVX512(xcr0) && xsave_size == 2688)
>>+    {
>>+      /* Intel CPUs supporting AVX512.  */
>>+      layout->avx_offset = 576;
>>+      layout->bndregs_offset = 960;
>>+      layout->bndcfg_offset = 1024;
>>+      layout->avx512_k_offset = 1088;
>>+      layout->avx512_zmm_h_offset = 1152;
>>+      layout->avx512_zmm_offset = 1664;
>>+    }
>>+  else if (HAS_MPX(xcr0) && xsave_size == 1088)
>>+    {
>>+      /* Intel CPUs supporting MPX.  */
>>+      layout->avx_offset = 576;
>>+      layout->bndregs_offset = 960;
>>+      layout->bndcfg_offset = 1024;
>>+    }
>>+  else if (HAS_AVX(xcr0) && xsave_size == 832)
>>+    {
>>+      /* Intel and AMD CPUs supporting AVX.  */
>>+      layout->avx_offset = 576;
>>+    }
>>+}
>>+
>> /* Extract from XSAVE a bitset of the features that are available on the
>>    target, but which have not yet been enabled.  */
>>
>>diff --git a/gdb/i387-tdep.h b/gdb/i387-tdep.h index 698ff2ee206..4e1403aa753
>>100644
>>--- a/gdb/i387-tdep.h
>>+++ b/gdb/i387-tdep.h
>>@@ -25,6 +25,7 @@ struct frame_info;
>> struct regcache;
>> struct type;
>> struct ui_file;
>>+struct x86_xsave_layout;
>>
>> /* Number of i387 floating point registers.  */  #define I387_NUM_REGS  16
>>@@ -138,6 +139,12 @@ extern void i387_collect_fsave (const struct regcache
>>*regcache, int regnum,  extern void i387_supply_fxsave (struct regcache
>>*regcache, int regnum,
>>                                const void *fxsave);
>>
>>+/* Select an XSAVE layout based on the XCR0 bitmask and total XSAVE
>>+   extended state size.  */
>>+
>>+extern void i387_set_xsave_layout (uint64_t xcr0, size_t xsave_size,
>>+                                  x86_xsave_layout *layout);
>>+
>> /* Similar to i387_supply_fxsave, but use XSAVE extended state.  */
>>
>> extern void i387_supply_xsave (struct regcache *regcache, int regnum,
>>--
>>2.34.1