Re: [PATCH 2/2] gdb, gdbserver: Add Intel APX register support

[Keith Seitz <keiths@redhat.com>]

Hi,

On 6/2/26 4:11 AM, Christina Schimpe wrote:
> From: "H.J. Lu" <hjl.tools@gmail.com>
> 
> Intel(R) Advanced Performance Extensions (APX) doubles the number of
> general-purpose registers (GPRs) from 16 to 32 (r16-r31).
> 
> Those new 64-bit Extended GPRs (EGPRs) do not change the size of the
> XSAVE area, as they take up the space left behind by the deprecated
> MPX registers.
> 
> In contrast to the legacy GPRS, EGPRs are not enabled by default in 64-bit
> mode but are xcr0 enabled based on APX_F Intel APX state.  Define
> X86_XSTATE_APX_F to support this new configuration.
> 
> Add gdb and gdbserver registers support for those new 64-bit EGPRs.
> Also add byte, word and dword pseudo register support.

This patch largely looks good, but I do have a question (and some
minor formatting nits) below...

> diff --git a/gdb/i387-tdep.c b/gdb/i387-tdep.c
> index f33f5ad046e..2b58375eefb 100644
> --- a/gdb/i387-tdep.c
> +++ b/gdb/i387-tdep.c
> @@ -920,6 +948,7 @@ i387_guess_xsave_layout (uint64_t xcr0, size_t xsave_size,
>       {
>         /* Intel CPUs supporting PKRU.  */
>         layout.avx_offset = 576;
> +      layout.apx_offset = 960;
>         layout.k_offset = 1088;
>         layout.zmm_h_offset = 1152;
>         layout.zmm_offset = 1664;
> @@ -938,6 +967,7 @@ i387_guess_xsave_layout (uint64_t xcr0, size_t xsave_size,
>       {
>         /* Intel CPUs supporting AVX512.  */
>         layout.avx_offset = 576;
> +      layout.apx_offset = 960;
>         layout.k_offset = 1088;
>         layout.zmm_h_offset = 1152;
>         layout.zmm_offset = 1664;
> @@ -968,6 +998,7 @@ i387_fallback_xsave_layout (uint64_t xcr0)
>       {
>         /* Intel CPUs supporting PKRU.  */
>         layout.avx_offset = 576;
> +      layout.apx_offset = 960;
>         layout.k_offset = 1088;
>         layout.zmm_h_offset = 1152;
>         layout.zmm_offset = 1664;
> @@ -978,6 +1009,7 @@ i387_fallback_xsave_layout (uint64_t xcr0)
>       {
>         /* Intel CPUs supporting AVX512.  */
>         layout.avx_offset = 576;
> +      layout.apx_offset = 960;
>         layout.k_offset = 1088;
>         layout.zmm_h_offset = 1152;
>         layout.zmm_offset = 1664;

I have a (probably naive) question about 'apx_layout' in the various
supported configurations. As patched, this member is set when XCR0
has a bit set for PKRU or AVX512 support. Does this mean that EGPR
reads from core files with only "plain" AVX support will be incorrect
(at offest 0 instead of 960)? AFAICT, the set of features described
by XCR0 are independent of each other, e.g., having PKRU does not
mean that APX is enabled/supported.

This may be a paper-only issue, but perhaps a little defensive
programming might be in order?

> diff --git a/gdb/testsuite/gdb.arch/amd64-apx-corefile.exp b/gdb/testsuite/gdb.arch/amd64-apx-corefile.exp
> new file mode 100644
> index 00000000000..6b357a06395
> --- /dev/null
> +++ b/gdb/testsuite/gdb.arch/amd64-apx-corefile.exp
> @@ -0,0 +1,87 @@
> +# Copyright 2024 Free Software Foundation, Inc.
> +
> +# This program is free software; you can redistribute it and/or modify
> +# it under the terms of the GNU General Public License as published by
> +# the Free Software Foundation; either version 3 of the License, or
> +# (at your option) any later version.
> +#
> +# This program is distributed in the hope that it will be useful,
> +# but WITHOUT ANY WARRANTY; without even the implied warranty of
> +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> +# GNU General Public License for more details.
> +#
> +# You should have received a copy of the GNU General Public License
> +# along with this program.  If not, see <http://www.gnu.org/licenses/>.
> +
> +# Please email any bugs, comments, and/or additions to this file to:
> +# bug-gdb@gnu.org
> +
> +# Test apx corefiles.
> +
> +require allow_apx_tests
> +
> +standard_testfile amd64-apx.c
> +
> +if { [prepare_for_testing "failed to prepare" ${testfile} ${srcfile} \
> +	{debug additional_flags=-mapxf}] } {
> +    return -1
> +}
> +
> +set nr_regs 16
> +set egpr(1) r16
> +set egpr(2) r17
> +set egpr(3) r18
> +set egpr(4) r19
> +set egpr(5) r20
> +set egpr(6) r21
> +set egpr(7) r22
> +set egpr(8) r23
> +set egpr(9) r24
> +set egpr(10) r25
> +set egpr(11) r26
> +set egpr(12) r27
> +set egpr(13) r28
> +set egpr(14) r29
> +set egpr(15) r30
> +set egpr(16) r31
> +
> +proc test_apx_corefiles {setting} {
> +    global egpr nr_regs
> +
> +    clean_restart "$::testfile"
> +    if { ![runto_main] } {
> +	continue
> +    }
> +
> +    set line [gdb_get_line_number "break here"]
> +    gdb_breakpoint $line
> +    gdb_continue_to_breakpoint "break here" ".*$::srcfile:$line.*"
> +
> +    gdb_test_no_output "maint set gcore xml-target-description ${setting}"
> +
> +    set gcorefile "${::binfile}_${setting}.gcore"
> +    if { ![gdb_gcore_cmd $gcorefile "save a corefile"] } {
> +	return -1
> +    }
> +
> +    # Now restart gdb and load the corefile.
> +    clean_restart "$::testfile"
> +
> +    gdb_test "core ${gcorefile}" \
> +	"Core was generated by .*" "re-load generated corefile"
> +
> +    with_test_prefix "test egpr after corefile load" {
> +	for { set r 1 } { $r <= $nr_regs  } { incr r } {
> +	set hexr [format %x [expr $r-1]]
                                   ^^^^

This expression (and others) are not enclosed in braces. If you run
"pre-commit run --all-files" (which is essentially what
gdb.src/pre-commit.exp does), you'll see

gdb/testsuite/gdb.arch/amd64-apx-corefile.exp:75:28: expression with 
substitutions should be enclosed by braces [unbraced-expr]
gdb/testsuite/gdb.arch/amd64-apx.exp:40:31: expression with 
substitutions should be enclosed by braces [unbraced-expr]
gdb/testsuite/gdb.arch/amd64-apx.exp:101:20: expression with 
substitutions should be enclosed by braces [unbraced-expr]

> +	gdb_test "print/z \$$egpr($r)" \
> +	    "= 0x00000000${hexr}4${hexr}3${hexr}2${hexr}1"
> +	}
> +    }
> +}
> +
> +test_apx_corefiles "on"
> +
> +# Test the code path without the NT_GDB_TDESC note
> +with_test_prefix "no NT_GDB_TDESC" {
> +    test_apx_corefiles "off"
> +}
> diff --git a/gdbserver/i387-fp.cc b/gdbserver/i387-fp.cc
> index 164b666979f..346f30c637b 100644
> --- a/gdbserver/i387-fp.cc
> +++ b/gdbserver/i387-fp.cc
> @@ -312,6 +317,10 @@ i387_cache_to_xsave (struct regcache *regcache, void *buf)
>         if ((clear_bv & X86_XSTATE_PKRU))
>   	for (i = 0; i < num_pkeys_registers; i++)
>   	  memset (fp->pkru_space () + i * 4, 0, 4);
> +
> +     if (amd64 && (clear_bv & X86_XSTATE_APX_F) != 0)
> +	for (i = 0; i < num_apx_registers; i++)
> +	  memset (fp->apx_space () + i * 8, 0, 8);
>       }

Could you double-check the indentation of this block? It looks like
the "if (amd64...)" line is mis-indented?

>     /* Check if any x87 registers are changed.  */
> @@ -465,6 +474,23 @@ i387_cache_to_xsave (struct regcache *regcache, void *buf)
>   	}
>       }
>   
> +  /* Check if any APX registers are changed.  */
> +  if (amd64 && (x86_xstate_bv & X86_XSTATE_APX_F)!= 0)

Missing a space before "!= 0)".

> +    {
> +      int r16_regnum = find_regno (regcache->tdesc, "r16");
> +
> +      for (i = 0; i < num_apx_registers; i++)
> +	{
> +	  collect_register (regcache, i + r16_regnum, raw);
> +	  p = fp->apx_space () + i * 8;
> +	  if (memcmp (raw, p, 8) != 0)
> +	    {
> +	      xstate_bv |= X86_XSTATE_APX_F;
> +	      memcpy (p, raw, 8);
> +	    }
> +	}
> +    }
> +
>     if (x86_xstate_bv & X86_XSTATE_X87)
>       {
>         collect_register_by_name (regcache, "fioff", raw);
Thanks!

Keith