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

["Schimpe, Christina" <christina.schimpe@intel.com>]

Hi Keith, 

Thank you for the review. 
This is my feedback for your remaining comments of this patch.

> -----Original Message-----
> From: Keith Seitz <keiths@redhat.com>
> Sent: Donnerstag, 18. Juni 2026 19:34
> To: Schimpe, Christina <christina.schimpe@intel.com>; gdb-
> patches@sourceware.org
> Cc: hjl.tools@gmail.com
> Subject: Re: [PATCH 2/2] gdb, gdbserver: Add Intel APX register support
> 
> 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]

Ah, yes - I should have checked that. Will fix.

> > +	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?

Indeed, will fix. 

> >     /* 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)".

Will fix.

> > +    {
> > +      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

Christina
Intel Deutschland GmbH

Registered Address: Dornacher Strasse 1, 85622 Feldkirchen, Germany
Tel: +49 89 991 430, www.intel.de
Managing Directors: Harry Demas, Jeffrey Schneiderman, Yin Chong Sorrell
Chairperson of the Supervisory Board: Nicole Lau
Registered Seat: Munich
Commercial Register: Amtsgericht Muenchen HRB 186928