Re: [PATCH 1/2] gdb: Enable OS generated corefiles on systems with Intel AMX support.

[Andrew Burgess <aburgess@redhat.com>]

Thanks for addressing this issue.  I have some style issues, and one
deeper though about the test, see below.

Christina Joos <christina.joos@intel.com> writes:

> This patch addresses the issue described in
> https://sourceware.org/bugzilla/show_bug.cgi?id=34561.

Bugs should be linked using a line at the end of the commit message
(see gdb/MAINTAINERS) like:

  Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=34561

Given the link always gets added at the end like this, in the commit
message body you can just write:

  This patch addresses the issue described in PR gdb/34561.  On systems
  with Intel AMX support the .... etc ...

anyone who wants a direct link to the bug can find it in the 'Bug: '
line at the end of the commit message.

>
> On Systems with Intel AMX support the xsave size is 11008. This xsave

lower case 's' on 'Systems' please.

> size is not handled by gdb/i387-tdep.c:i387_guess_xsave_layout and is
> causing problems for corefiles generated by the linux kernel.
>
> ~~~
> $ gdb
> GNU gdb (GDB) 19.0.50.20260814-git
> Copyright (C) 2026 Free Software Foundation, Inc.
> [...]
> (gdb) core core_main_SEGV
> [New LWP 2162880 (id 1)]
> [...]
> Core was generated by `./main'.
> Program terminated with signal SIGSEGV, Segmentation fault.
> 4         *pointer = 3;
> (gdb) p $ymm0
> $1 = void
> ~~~
>
> We should be able to print the register $ymm0 (or any other register
> belonging to a feature higher than SSE).
>
> For a live debug session we can print it:
> ~~~
> Reading symbols from main...
> (gdb) start
> Temporary breakpoint 1 at 0x1131: file main.c, line 3.
> Starting program: /tmp/main
> [Thread debugging using libthread_db enabled]
> Using host libthread_db library "/lib/x86_64-linux-gnu/libthread_db.so.1".
>
> Temporary breakpoint 1, main () at main.c:3
> 3         int *pointer = 0;
> (gdb) p $ymm0
> $1 = {v16_bfloat16 = {0, 2.342e-38, 0 <repeats 14 times>}, v16_half = {0, 1.5199e-05, 0 <repeats 14 times>}, v8_float = {
>     2.34180515e-38, 0, 0, 0, 0, 0, 0, 0}, v4_double = {8.256666972292243e-317, 0, 0, 0}, v32_int8 = {0, 0, -1,
>     0 <repeats 29 times>}, v16_int16 = {0, 255, 0 <repeats 14 times>}, v8_int32 = {16711680, 0, 0, 0, 0, 0, 0, 0}, v4_int64 = {
>     16711680, 0, 0, 0}, v2_int128 = {16711680, 0}}
> [...]
> ~~~
>
> Note, that this is not reproducible for corefiles generated by the gcore

Drop the comma after 'Note'.

> command, since in such corefiles the GDB target description is
> available and gdb/i387-tdep.c:i387_fallback_xsave_layout configures
> the xsave_layout based on xcr0 derived from the target description.
>
> This patch fixes this issue by handling the new xsave size in
> i387_guess_xsave_layout.  It is necessary even though GDB does not
> support AMX yet, since we still pass the full xsave_size (11008)
> extracted from the corefile to i387_guess_xsave_layout.
> The patch does not fix i387_fallback_xsave_layout to configure the new xsave
> size.  Configuring sizeof_xsave to 2696 is fine at this point, since
> truncating to 2696 drops only the parts GDB doesn't model yet.
>
> As a quick solution for the upcoming release and CPUs supporting
> Intel AMX this patch should be sufficient.
>
> Testing:
>
> I noticed that we don't have tests for corefiles for the features AVX,
> AVX512 and PKRU in the GDB testsuite.  So this commit adds corefile tests
> for AVX, AVX512 and PKRU registers.
>
> It further fixes the tests gdb.base/coredump-filter.exp and
> gdb.arch/i386-tls-regs.exp on systems with Intel AMX support.
> With my patch the warning
> "warning: Unexpected size of section `.reg-xstate/1373786' in core file.^M"
> disappears and the tests are passing again.

Can you clean up the ^M artefact please.

Also, as you're touching the commit message anyway, I think this would
be better written as:

  As a result of this patch, the warning:

    warning: Unexpected size of section `.reg-xstate/1373786' in core file.

  which was causing failure in gdb.base/coredump-filter.exp and
  gdb.arch/i386-tls-regs.exp on AMX systems disappears, these tests now
  pass.



> ---
>  gdb/i387-tdep.c                        |  9 +++-
>  gdb/testsuite/gdb.arch/i386-avx.c      |  6 +++
>  gdb/testsuite/gdb.arch/i386-avx.exp    | 61 ++++++++++++++++++++++----
>  gdb/testsuite/gdb.arch/i386-avx512.c   |  6 +++
>  gdb/testsuite/gdb.arch/i386-avx512.exp | 56 +++++++++++++++++++++++
>  gdb/testsuite/gdb.arch/i386-pkru.c     |  6 +++
>  gdb/testsuite/gdb.arch/i386-pkru.exp   | 39 ++++++++++++++++
>  7 files changed, 173 insertions(+), 10 deletions(-)
>
> diff --git a/gdb/i387-tdep.c b/gdb/i387-tdep.c
> index f22a5e02bbb..19429536537 100644
> --- a/gdb/i387-tdep.c
> +++ b/gdb/i387-tdep.c
> @@ -918,9 +918,14 @@ bool
>  i387_guess_xsave_layout (uint64_t xcr0, size_t xsave_size,
>  			 x86_xsave_layout &layout)
>  {
> -  if (HAS_PKRU (xcr0) && xsave_size == 2696)
> +  if (HAS_PKRU (xcr0) && (xsave_size == 2696 || xsave_size == 11008))
>      {
> -      /* Intel CPUs supporting PKRU.  */
> +      /* Intel CPUs supporting PKRU.
> +	 Note that in this case 2 possible xsave_sizes have to be handled,

The 'Note' here is not really needed.  The comment is by definition a
note.  Also, variables should be capitalised, or avoid using the
variable name maybe, like:

  In this case two possible XSAVE_SIZE values have to be handled.  As
  GDB does not yet support Intel AMX and CPUs that support this feature
  will have a different XSAVE_SIZE (11008).  If we don't handle this
  size then we will not be able to handle any xsave registers from OS
  generated core files on AMX systems.

> +	 since GDB does not support Intel AMX yet and CPUs that support
> +	 this feature still have a different xsave size (xsave_size == 11008).
> +	 Otherwise, we are not able to read xsave registers in OS generated
> +	 corefiles.  */
>        layout.avx_offset = 576;
>        layout.k_offset = 1088;
>        layout.zmm_h_offset = 1152;
> diff --git a/gdb/testsuite/gdb.arch/i386-avx.c b/gdb/testsuite/gdb.arch/i386-avx.c
> index b6cd89e5bea..7edcabcec8c 100644
> --- a/gdb/testsuite/gdb.arch/i386-avx.c
> +++ b/gdb/testsuite/gdb.arch/i386-avx.c
> @@ -18,11 +18,14 @@
>     along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
>  
>  #include <stdio.h>
> +#include <stdlib.h>
>  #include "nat/x86-cpuid.h"
>  
>  /* Align sufficient to be able to use vmovaps.  */
>  #define ALIGN 32
>  
> +volatile int should_dump_core_p = 1;
> +
>  typedef struct {
>    _Alignas (ALIGN) float f[8];
>  } v8sf_t;
> @@ -87,6 +90,9 @@ main (int argc, char **argv)
>  
>    asm ("nop"); /* first breakpoint here */
>  
> +  if (should_dump_core_p) /* Crash for OS corefile. */
> +    abort ();

We should still follow GDB/GNU style as much as possible in tests, which
means avoiding trailing comments.  Please move the comment above the
`if` line.  The exception in tests is when we use a comment to tag a
line on which we wish to place a breakpoint, e.g. the 'first breakpoint
here' line above.

I also wonder if using `abort()` here is the right approach.

You want to check the register values in the generated core file, not
just C level variables.  Making a function call runs the risk that the
callee might touch some or all of these registers.  I guess it's
unlikely for something as simple as abort(), but I wonder maybe, if
that's what's causing the problem your second patch is addressing?

Maybe it would be better to trigger the core file by reading from
address zero?  We have a testsuite proc `is_address_zero_readable` which
can be used to gate such a test.  I think it might be worth testing this
on the problematic machines you mention in the second patch to see if
this fixes the issue.

> +
>    asm (
>         "vmovaps %%ymm0, 0(%0)\n\t"
>         "vmovaps %%ymm1, 32(%0)\n\t"
> diff --git a/gdb/testsuite/gdb.arch/i386-avx.exp b/gdb/testsuite/gdb.arch/i386-avx.exp
> index 1a786ee8a51..1740045427a 100644
> --- a/gdb/testsuite/gdb.arch/i386-avx.exp
> +++ b/gdb/testsuite/gdb.arch/i386-avx.exp
> @@ -42,12 +42,26 @@ if { [gdb_compile "${srcdir}/${subdir}/${srcfile}" "${binfile}" executable \
>      return
>  }
>  
> +# Check reading registers after the first breakpoint for ymm int8 and
> +# float types.
> +proc test_regs_first_bp {} {
> +    for { set r 0 } { $r < $::nr_regs } { incr r } {
> +	gdb_test "print \$ymm$r.v8_float" \
> +	    ".. = \\{$r, $r.125, $r.25, $r.375, $r.5, $r.625, $r.75, $r.875\\}.*" \
> +	    "check float contents of %ymm$r"
> +	gdb_test "print \$ymm$r.v32_int8" \
> +	    ".. = \\{(-?${::decimal}, ){31}-?${::decimal}\\}.*" \
> +	    "check int8 contents of %ymm$r"
> +    }
> +}
>  clean_restart ${::testfile}

Add a blank line before the clean_restart line please.

>  
>  if {![runto_main]} {
>      return
>  }
>  
> +gdb_test_no_output "set should_dump_core_p=0"
> +
>  gdb_test "break [gdb_get_line_number "first breakpoint here"]" \
>  	 "Breakpoint .* at .*i386-avx.c.*" \
>  	 "set first breakpoint in main"
> @@ -59,14 +73,7 @@ if {[is_amd64_regs_target]} {
>      set nr_regs 8
>  }
>  
> -for { set r 0 } { $r < $nr_regs } { incr r } {
> -    gdb_test "print \$ymm$r.v8_float" \
> -	".. = \\{$r, $r.125, $r.25, $r.375, $r.5, $r.625, $r.75, $r.875\\}.*" \
> -	"check float contents of %ymm$r"
> -    gdb_test "print \$ymm$r.v32_int8" \
> -	".. = \\{(-?${::decimal}, ){31}-?${::decimal}\\}.*" \
> -	"check int8 contents of %ymm$r"
> -}
> +test_regs_first_bp
>  
>  for { set r 0 } { $r < $nr_regs } { incr r } {
>      gdb_test "set var \$ymm$r.v8_float\[0\] = $r + 10" "" "set %ymm$r"
> @@ -100,9 +107,47 @@ with_test_prefix "force-disable xml descriptions" {
>  	    return
>  	}
>  
> +	gdb_test_no_output "set should_dump_core_p=0"
> +
>  	# With qXfer:features:read disabled, we won't know anything
>  	# about YMM registers.
>  	gdb_test "print \$ymm0" " = void"
>  	gdb_test "print \$xmm0" "v4_float.*"
>      }
>  }
> +
> +# Restart gdb, load the corefile generated by gcore or the OS
> +# and check reading registers from the corefile.
> +proc test_corefile {core_filename} {
> +    clean_restart $::testfile
> +
> +    gdb_test "core $core_filename" "Core was generated by .*" \
> +	"load corefile"
> +
> +    test_regs_first_bp
> +}
> +
> +with_test_prefix "OS generated corefile" {
> +    set corefile [core_find $binfile]
> +    if { $corefile eq "" } {
> +	unsupported "unable to generate core file"
> +    } else {
> +	test_corefile $corefile
> +    }
> +}
> +
> +with_test_prefix "gcore corefile" {
> +    clean_restart ${::testfile}
> +    if { ![runto_main] } {
> +	return
> +    }
> +
> +    set line [gdb_get_line_number "first breakpoint here"]
> +    gdb_breakpoint $line
> +    gdb_continue_to_breakpoint "first breakpoint here" ".*$srcfile:$line.*"
> +
> +    set gcorefile $binfile.gcore
> +    if { [gdb_gcore_cmd $gcorefile "save a corefile"] } {
> +	test_corefile $gcorefile
> +    }
> +}
> diff --git a/gdb/testsuite/gdb.arch/i386-avx512.c b/gdb/testsuite/gdb.arch/i386-avx512.c
> index b1e023ce4d4..d935fab50bc 100644
> --- a/gdb/testsuite/gdb.arch/i386-avx512.c
> +++ b/gdb/testsuite/gdb.arch/i386-avx512.c
> @@ -17,8 +17,11 @@
>     You should have received a copy of the GNU General Public License
>     along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
>  
> +#include <stdlib.h>
>  #include "x86-cpuid.h"
>  
> +volatile int should_dump_core_p = 1;
> +
>  typedef struct
>  {
>    double f[8];
> @@ -233,6 +236,9 @@ main (int argc, char **argv)
>        move_zmm_data_to_reg ();
>        asm ("nop"); /* third breakpoint here  */
>  
> +      if (should_dump_core_p) /* Crash for OS corefile. */
> +	abort ();

Trailing comment again.

> +
>        /* Test script incremented values,
>  	 move back to array and check values.  */
>        move_zmm_data_to_memory ();
> diff --git a/gdb/testsuite/gdb.arch/i386-avx512.exp b/gdb/testsuite/gdb.arch/i386-avx512.exp
> index c79563366dc..6984c2280e9 100644
> --- a/gdb/testsuite/gdb.arch/i386-avx512.exp
> +++ b/gdb/testsuite/gdb.arch/i386-avx512.exp
> @@ -35,6 +35,8 @@ if {![runto_main]} {
>      return
>  }
>  
> +gdb_test_no_output "set should_dump_core_p=0"
> +
>  set supports_avx512 0
>  
>  set test "probe AVX512 support"
> @@ -181,3 +183,57 @@ gdb_test "print \$zmm0.v16_int32" "= {-1, -1, -1, -1, 0 <repeats 12 times>}"
>  if { $nr_regs >= 16 } {
>      gdb_test "print \$zmm16.v16_int32" "= {-1 <repeats 16 times>}"
>  }
> +
> +# Test reading k and zmm registers and for double and int16 types.
> +proc test_regs_corefile {} {
> +    for { set r 1 } { $r < 8 } { incr r } {
> +	gdb_test "print/x \$k$r" \
> +	    ".. = 0x[format %x $r]2[format %x $r]1" \
> +	    "check contents of %k$r"
> +    }
> +
> +    for { set r 0 } { $r < $::nr_regs } { incr r } {
> +	gdb_test "print \$zmm$r.v8_double" \
> +	    ".. = \\{$r, $r.125, $r.25, $r.375, $r.5, $r.625, $r.75, $r.875\\}.*" \
> +	    "check double contents of %zmm$r"
> +	gdb_test "print \$zmm$r.v32_int16" \
> +	    ".. = \\{(-?${::decimal}, ){31}-?${::decimal}\\}.*" \
> +	    "check int16 contents of %zmm$r"
> +    }
> +}
> +
> +# Restart gdb, load the corefile generated by gcore or the OS
> +# and check reading registers from the corefile.
> +proc test_corefile {core_filename} {
> +    clean_restart $::testfile
> +
> +    gdb_test "core $core_filename" "Core was generated by .*" \
> +	"load corefile"
> +
> +    test_regs_corefile
> +}
> +
> +with_test_prefix "OS generated corefile" {
> +    set corefile [core_find $binfile]
> +    if { $corefile eq "" } {
> +	unsupported "unable to generate core file"
> +    } else {
> +	test_corefile $corefile
> +    }
> +}
> +
> +with_test_prefix "gcore corefile" {
> +    clean_restart ${::testfile}
> +    if { ![runto_main] } {
> +	return
> +    }
> +
> +    set line [gdb_get_line_number "third breakpoint here"]
> +    gdb_breakpoint $line
> +    gdb_continue_to_breakpoint "third breakpoint here" ".*$srcfile:$line.*"
> +
> +    set gcorefile $binfile.gcore
> +    if { [gdb_gcore_cmd $gcorefile "save a corefile"] } {
> +	test_corefile $gcorefile
> +    }
> +}
> diff --git a/gdb/testsuite/gdb.arch/i386-pkru.c b/gdb/testsuite/gdb.arch/i386-pkru.c
> index 9561d1e923a..e316ca90bba 100644
> --- a/gdb/testsuite/gdb.arch/i386-pkru.c
> +++ b/gdb/testsuite/gdb.arch/i386-pkru.c
> @@ -18,12 +18,15 @@
>     along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
>  
>  #include <stddef.h>
> +#include <stdlib.h>
>  #include "x86-cpuid.h"
>  
>  #ifndef NOINLINE
>  #define NOINLINE __attribute__ ((noinline))
>  #endif
>  
> +volatile int should_dump_core_p = 1;
> +
>  unsigned int have_pkru (void) NOINLINE;
>  
>  static inline unsigned long
> @@ -83,6 +86,9 @@ main (int argc, char **argv)
>        wrpkru (wr_value);
>        asm ("nop\n\t");	/* break here 1.  */
>  
> +      if (should_dump_core_p) /* Crash for OS corefile. */
> +	abort ();

Trailing comment again.

Thanks,
Andrew