Re: [PATCH v2 1/2] GDB: aarch64-linux: Reorganize gdb/arch/aarch64-gcs-linux.h

[Thiago Jung Bauermann <thiago.bauermann@linaro.org>]

Hello,

Luis <luis.machado.foss@gmail.com> writes:

> Hi,
>
> First, aarch64's code could really use a refactoring in terms of cleaning up the headers
> and splitting things into their own files. With that said, we don´t need to do that now,
> but would be nice to do it eventually.

Ok. I cleaned up the things you pointed out in this review, at least.

> On 14/02/2026 04:54, Thiago Jung Bauermann wrote:
>> This file implicitly depends on aarch64-linux's <asm/sigcontext.h> for
>> the definition of GCS_MAGIC and on its <bits/hwcap.h> for the definition
>> of HWCAP_GCS, but files on gdb/arch/ shouldn't depend on include files
>> specific to the architecture.
>> Regarding GCS_MAGIC and struct user_gcs, move that code section to
>> gdb/nat/aarch64-linux.h which can rely on <asm/sigcontext.h> and
>> <asm/ptrace.h>.
>> To fix the use of struct user_gcs in gdb/aarch64-linuxt-dep.c, define a
>> macro with the size of the GCS regset in aarch64-linux-tdep.h and use it
>> in aarch64-linux-tdep.c, as is done for other regsets and following a
>> suggestion from Simon Marchi.
>> Regarding HWCAP_GCS, rename it to AARCH64_HWCAP_GCS to ensure that it
>> won't conflict with any system header definition and define it
>> unconditionally, following the example of AARCH64_HWCAP_PACA.
>> Renaming HWCAP_GCS also avoids a potential (and I would say unlikely)
>> problem on non-AArch64 systems: naming conflict if by coincidence they have
>> an unrelated hardware capability bit also named HWCAP_GCS.
>> Finally, I noticed that AARCH64_HWCAP_PACA is duplicated in GDB and
>> gdbserver files, so consolidate them in the same header and rename it to
>> gdb/arch/aarch64-linux.h since it isn't just about GCS anymore.
>> ---
>> As I mentioned on v1's email thread, this patch opens a small can of worms,
>> since other gdb/arch/aarch64*.h do the same kind of thing (mostly with
>> HWCAP macros but also a few others).
>> Perhaps I'm going too far with the HWCAP_GCS renaming and the current
>> handling of it is fine?
>>   gdb/Makefile.in                               |  2 +-
>>   gdb/aarch64-linux-nat.c                       |  4 +--
>>   gdb/aarch64-linux-tdep.c                      |  9 +++---
>>   gdb/aarch64-linux-tdep.h                      |  6 ++--
>>   .../{aarch64-gcs-linux.h => aarch64-linux.h}  | 30 +++++--------------
>>   gdb/nat/aarch64-linux.h                       | 16 ++++++++++
>>   gdbserver/linux-aarch64-low.cc                |  7 ++---
>>   7 files changed, 36 insertions(+), 38 deletions(-)
>>   rename gdb/arch/{aarch64-gcs-linux.h => aarch64-linux.h} (61%)
>> diff --git a/gdb/Makefile.in b/gdb/Makefile.in
>> index 2aa95be968ac..fd47c613966a 100644
>> --- a/gdb/Makefile.in
>> +++ b/gdb/Makefile.in
>> @@ -1301,7 +1301,7 @@ HFILES_NO_SRCDIR = \
>>   	amdgpu-tdep.h \
>>   	annotate.h \
>>   	arch/aarch32.h \
>> -	arch/aarch64-gcs-linux.h \
>> +	arch/aarch64-linux.h \
>
> If there is anything feature-specific about things in aarch64-linux.h, I´d rather have a
> new featute-specific file here.

Ok. For v3 I kept arch/aarch64-gcs-linux.h and there's no
arch/aarch64-linux.h anymore.

>>   	arch/aarch64.h \
>>   	arch/aarch64-insn.h \
>>   	arch/aarch64-mte.h \
>> diff --git a/gdb/aarch64-linux-nat.c b/gdb/aarch64-linux-nat.c
>> index 028de981588b..b2dd192a7b56 100644
>> --- a/gdb/aarch64-linux-nat.c
>> +++ b/gdb/aarch64-linux-nat.c
>> @@ -51,7 +51,7 @@
>>   #include "gdb_proc_service.h"
>>   #include "arch-utils.h"
>>   -#include "arch/aarch64-gcs-linux.h"
>> +#include "arch/aarch64-linux.h"
>
> Likewise above.
>
>>   #include "arch/aarch64-mte-linux.h"
>>     #include "nat/aarch64-mte-linux-ptrace.h"
>> @@ -1013,7 +1013,7 @@ aarch64_linux_nat_target::read_description ()
>>        active or not.  */
>>     features.vq = aarch64_sve_get_vq (tid);
>>     features.pauth = hwcap & AARCH64_HWCAP_PACA;
>> -  features.gcs = features.gcs_linux = hwcap & HWCAP_GCS;
>> +  features.gcs = features.gcs_linux = hwcap & AARCH64_HWCAP_GCS;
>>     features.mte = hwcap2 & HWCAP2_MTE;
>>     features.tls = aarch64_tls_register_count (tid);
>>     /* SME feature check.  */
>> diff --git a/gdb/aarch64-linux-tdep.c b/gdb/aarch64-linux-tdep.c
>> index b85c25ecae1d..33e1b411ff1d 100644
>> --- a/gdb/aarch64-linux-tdep.c
>> +++ b/gdb/aarch64-linux-tdep.c
>> @@ -51,7 +51,7 @@
>>   #include "record-full.h"
>>   #include "linux-record.h"
>>   -#include "arch/aarch64-gcs-linux.h"
>> +#include "arch/aarch64-linux.h"
>
> Likewise here.
>
>>   #include "arch/aarch64-mte.h">   #include "arch/aarch64-mte-linux.h"
>>   #include "arch/aarch64-scalable-linux.h"
>> @@ -1734,8 +1734,9 @@ aarch64_linux_iterate_over_regset_sections (struct gdbarch *gdbarch,
>>   	  gcs_regmap, regcache_supply_regset, regcache_collect_regset
>>   	};
>>   -      cb (".reg-aarch-gcs", sizeof (user_gcs), sizeof (user_gcs),
>> -	  &aarch64_linux_gcs_regset, "GCS registers", cb_data);
>> +      cb (".reg-aarch-gcs", AARCH64_LINUX_SIZEOF_GCS_REGSET,
>> +	  AARCH64_LINUX_SIZEOF_GCS_REGSET, &aarch64_linux_gcs_regset,
>> +	  "GCS registers", cb_data);
>>       }
>>   }
>>   @@ -1761,7 +1762,7 @@ aarch64_linux_core_read_description (struct gdbarch *gdbarch,
>>        length.  */
>>     features.vq = aarch64_linux_core_read_vq_from_sections (gdbarch, abfd);
>>     features.pauth = hwcap & AARCH64_HWCAP_PACA;
>> -  features.gcs = features.gcs_linux = hwcap & HWCAP_GCS;
>> +  features.gcs = features.gcs_linux = hwcap & AARCH64_HWCAP_GCS;
>>     features.mte = hwcap2 & HWCAP2_MTE;
>>     features.fpmr = hwcap2 & HWCAP2_FPMR;
>>   diff --git a/gdb/aarch64-linux-tdep.h b/gdb/aarch64-linux-tdep.h
>> index e926687ff6eb..f247d6aa72cd 100644
>> --- a/gdb/aarch64-linux-tdep.h
>> +++ b/gdb/aarch64-linux-tdep.h
>> @@ -39,10 +39,10 @@
>>   /* The MTE regset consists of a 64-bit register.  */
>>   #define AARCH64_LINUX_SIZEOF_MTE_REGSET (8)
>>   +/* The GCS regset consists of 3 64-bit registers.  */
>> +#define AARCH64_LINUX_SIZEOF_GCS_REGSET (3 * 8)
>> +
>
> The above is the kind of feature-specific data we should put into
> arch/aarch64-linux-<feature>.[c|h]. At the moment not all of it follows this particular
> pattern, and that's annoying. But we should strive to clean it up.

Ok, in v3 I moved this constant to gdb/arch/aarch64-gcs-linux.h.

>>   extern const struct regset aarch64_linux_gregset;
>>   extern const struct regset aarch64_linux_fpregset;
>>   -/* Matches HWCAP_PACA in kernel header arch/arm64/include/uapi/asm/hwcap.h.  */
>> -#define AARCH64_HWCAP_PACA (1 << 30)
>> -
>
> The above is older code, and that bit is likely available out there. We should do the same
> treatment to it as the others. That is, if it is not defined, define it. Otherwise use
> whatever is defined.

Ok. In v3 I added a patch renaming this to HWCAP_PACA and moving it to
gdb/arch/aarch64-pauth-linux.h.

>>   #endif /* GDB_AARCH64_LINUX_TDEP_H */
>> diff --git a/gdb/arch/aarch64-gcs-linux.h b/gdb/arch/aarch64-linux.h
>> similarity index 61%
>> rename from gdb/arch/aarch64-gcs-linux.h
>> rename to gdb/arch/aarch64-linux.h
>> index b31fc32daa03..a33c082a0318 100644
>> --- a/gdb/arch/aarch64-gcs-linux.h
>> +++ b/gdb/arch/aarch64-linux.h
>> @@ -1,4 +1,4 @@
>> -/* Common Linux target-dependent definitions for AArch64 GCS
>> +/* Common Linux target-dependent definitions for AArch64 running Linux.
>>   >      Copyright (C) 2025-2026 Free Software Foundation, Inc.
>>   @@ -17,28 +17,12 @@
>>      You should have received a copy of the GNU General Public License
>>      along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
>>   -#ifndef GDB_ARCH_AARCH64_GCS_LINUX_H
>> -#define GDB_ARCH_AARCH64_GCS_LINUX_H
>> -
>> -#include <stdint.h>
>> +#ifndef GDB_ARCH_AARCH64_LINUX_H
>> +#define GDB_ARCH_AARCH64_LINUX_H
>>   +/* Matches HWCAP_PACA in kernel header arch/arm64/include/uapi/asm/hwcap.h.  */
>> +#define AARCH64_HWCAP_PACA (1 << 30)
>
> We should either move all of the definitions to arch/aarch64-linux.h or move them to their
> own separate feature files. See mte/sve/sme for instance.

I chose the separate feature files option, as you suggested below.

>>   /* Feature check for Guarded Control Stack.  */
>> -#ifndef HWCAP_GCS
>> -#define HWCAP_GCS (1ULL << 32)
>> -#endif
>> -
>> -/* Make sure we only define these if the kernel header doesn't.  */
>> -#ifndef GCS_MAGIC
>> -
>> -/* GCS state (NT_ARM_GCS).  */
>> -
>> -struct user_gcs
>> -{
>> -  uint64_t features_enabled;
>> -  uint64_t features_locked;
>> -  uint64_t gcspr_el0;
>> -};
>> -
>> -#endif /* GCS_MAGIC */
>> +#define AARCH64_HWCAP_GCS (1ULL << 32)
>>   -#endif /* GDB_ARCH_AARCH64_GCS_LINUX_H */
>> +#endif /* GDB_ARCH_AARCH64_LINUX_H */
>> diff --git a/gdb/nat/aarch64-linux.h b/gdb/nat/aarch64-linux.h
>> index bb5381eba3ef..f7c09aa112bf 100644
>> --- a/gdb/nat/aarch64-linux.h
>> +++ b/gdb/nat/aarch64-linux.h
>> @@ -19,7 +19,9 @@
>>   #ifndef GDB_NAT_AARCH64_LINUX_H
>>   #define GDB_NAT_AARCH64_LINUX_H
>>   +#include <stdint.h>
>>   #include <signal.h>
>> +#include <asm/ptrace.h>
>>     /* Defines ps_err_e, struct ps_prochandle.  */
>>   #include "gdb_proc_service.h"
>> @@ -113,6 +115,20 @@ typedef struct compat_siginfo
>>   #define cpt_si_band _sifields._sigpoll._band
>>   #define cpt_si_fd _sifields._sigpoll._fd
>>   +/* Make sure we only define these if the kernel header doesn't.  */
>> +#ifndef GCS_MAGIC
>> +
>> +/* GCS state (NT_ARM_GCS).  */
>> +
>> +struct user_gcs
>> +{
>> +  uint64_t features_enabled;
>> +  uint64_t features_locked;
>> +  uint64_t gcspr_el0;
>> +};
>> +
>> +#endif /* GCS_MAGIC */
>> +
>
> To keep things clean I´d define the above in a separare file for the feature. See
> gdb/nat/aarch64-mte-linux-ptrace.[h|c] for instance.

Ok. In v3 I created gdb/nat/aarch64-gcs-linux-ptrace.h and moved the
struct user_gcs definition to it.

>>   void aarch64_siginfo_from_compat_siginfo (siginfo_t *to,
>>   					    compat_siginfo_t *from);
>>   void aarch64_compat_siginfo_from_siginfo (compat_siginfo_t *to,
>> diff --git a/gdbserver/linux-aarch64-low.cc b/gdbserver/linux-aarch64-low.cc
>> index b19e605f55d6..60ea50f609cc 100644
>> --- a/gdbserver/linux-aarch64-low.cc
>> +++ b/gdbserver/linux-aarch64-low.cc
>> @@ -39,7 +39,7 @@
>>     #include "gdb_proc_service.h"
>>   #include "arch/aarch64.h"
>> -#include "arch/aarch64-gcs-linux.h"
>> +#include "arch/aarch64-linux.h"
>
> See the comment below about having per-feature files. I think having per-feature files is
> less work at this point.

Agreed.

>>   #include "arch/aarch64-mte-linux.h"
>>   #include "arch/aarch64-scalable-linux.h"
>>   #include "linux-aarch32-tdesc.h"
>> @@ -984,9 +984,6 @@ aarch64_adjust_register_sets (const struct aarch64_features &features)
>>       }
>>   }
>>   -/* Matches HWCAP_PACA in kernel header arch/arm64/include/uapi/asm/hwcap.h.  */
>> -#define AARCH64_HWCAP_PACA (1 << 30)
>> -
>>   /* Implementation of linux target ops method "low_arch_setup".  */
>>     void
>> @@ -1009,7 +1006,7 @@ aarch64_target::low_arch_setup ()
>>         /* A-profile MTE is 64-bit only.  */
>>         features.mte = linux_get_hwcap2 (pid, 8) & HWCAP2_MTE;
>>         features.tls = aarch64_tls_register_count (tid);
>> -      features.gcs = features.gcs_linux = linux_get_hwcap (pid, 8) & HWCAP_GCS;
>> +      features.gcs = features.gcs_linux = linux_get_hwcap (pid, 8) & AARCH64_HWCAP_GCS;
>>         features.fpmr = linux_get_hwcap2 (pid, 8) & HWCAP2_FPMR;
>>           /* Scalable Matrix Extension feature and size check.  */
>
> Checking arch/aarch64.h, HWCAP2_FPMR looks out of place as well. So yeah, overall these
> files need a bit of a cleanup.

Ok. In v3 I added a patch moving it to gdb/arch/aarch64-fpmr-linux.h.

-- 
Thiago