From: Luis Machado <luis.machado@arm.com>
To: John Baldwin <jhb@FreeBSD.org>, gdb-patches@sourceware.org
Subject: Re: [PATCH] Use aarch64_features to describe register features in target descriptions.
Date: Wed, 18 May 2022 15:23:53 +0100 [thread overview]
Message-ID: <34e14ff6-8158-620b-6255-6e08b2fff567@arm.com> (raw)
In-Reply-To: <157dad7b-adae-6769-f947-ef4448b99aaf@FreeBSD.org>
Hi!
On 5/18/22 14:59, John Baldwin wrote:
> On 5/5/22 11:46 AM, John Baldwin wrote:
>> Replace the sve bool member of aarch64_features with a vq member that
>> holds the vector quotient. It is zero if SVE is not present.
>>
>> Add std::hash<> specialization and operator== so that aarch64_features
>> can be used as a key with std::unordered_map<>.
>>
>> Change the various functions that create or lookup aarch64 target
>> descriptions to accept a const aarch64_features object rather than a
>> growing number of arguments.
>>
>> Replace the multi-dimension tdesc_aarch64_list arrays used to cache
>> target descriptions with unordered_maps indexed by aarch64_feature.
>
> Ping? (Sorry, I thought I had cc'd Luis on this when I sent it, but it seems
> like it was only sent to the list)
Sorry I didn't notice this earlier. Sometimes the inbound server drops my copy of the e-mail and it
only makes its way to the list.
>
>> ---
>> gdb/aarch64-fbsd-nat.c | 5 +++--
>> gdb/aarch64-fbsd-tdep.c | 5 ++++-
>> gdb/aarch64-linux-nat.c | 10 +++++----
>> gdb/aarch64-linux-tdep.c | 11 +++++----
>> gdb/aarch64-tdep.c | 21 +++++++++++-------
>> gdb/aarch64-tdep.h | 3 +--
>> gdb/arch/aarch64.c | 13 +++++------
>> gdb/arch/aarch64.h | 38 ++++++++++++++++++++++++--------
>> gdbserver/linux-aarch64-ipa.cc | 4 ++--
>> gdbserver/linux-aarch64-low.cc | 11 ++++-----
>> gdbserver/linux-aarch64-tdesc.cc | 18 +++++++--------
>> gdbserver/linux-aarch64-tdesc.h | 6 +++--
>> gdbserver/netbsd-aarch64-low.cc | 2 +-
>> 13 files changed, 89 insertions(+), 58 deletions(-)
>>
>> diff --git a/gdb/aarch64-fbsd-nat.c b/gdb/aarch64-fbsd-nat.c
>> index 910bf5bb190..fb7a29b5afb 100644
>> --- a/gdb/aarch64-fbsd-nat.c
>> +++ b/gdb/aarch64-fbsd-nat.c
>> @@ -149,8 +149,9 @@ aarch64_fbsd_nat_target::store_registers (struct regcache *regcache,
>> const struct target_desc *
>> aarch64_fbsd_nat_target::read_description ()
>> {
>> - bool tls = have_regset (inferior_ptid, NT_ARM_TLS) != 0;
>> - return aarch64_read_description (0, false, false, tls);
>> + aarch64_features features;
>> + features.tls = have_regset (inferior_ptid, NT_ARM_TLS) != 0;
>> + return aarch64_read_description (features);
>> }
>> #ifdef HAVE_DBREG
>> diff --git a/gdb/aarch64-fbsd-tdep.c b/gdb/aarch64-fbsd-tdep.c
>> index fdf0795b9bf..891546b3c64 100644
>> --- a/gdb/aarch64-fbsd-tdep.c
>> +++ b/gdb/aarch64-fbsd-tdep.c
>> @@ -178,7 +178,10 @@ aarch64_fbsd_core_read_description (struct gdbarch *gdbarch,
>> {
>> asection *tls = bfd_get_section_by_name (abfd, ".reg-aarch-tls");
>> - return aarch64_read_description (0, false, false, tls != nullptr);
>> + aarch64_features features;
>> + features.tls = tls != nullptr;
>> +
>> + return aarch64_read_description (features);
>> }
>> /* Implement the get_thread_local_address gdbarch method. */
>> diff --git a/gdb/aarch64-linux-nat.c b/gdb/aarch64-linux-nat.c
>> index 10b0ca10984..9cde72b247b 100644
>> --- a/gdb/aarch64-linux-nat.c
>> +++ b/gdb/aarch64-linux-nat.c
>> @@ -709,11 +709,13 @@ aarch64_linux_nat_target::read_description ()
>> CORE_ADDR hwcap = linux_get_hwcap (this);
>> CORE_ADDR hwcap2 = linux_get_hwcap2 (this);
>> - bool pauth_p = hwcap & AARCH64_HWCAP_PACA;
>> - bool mte_p = hwcap2 & HWCAP2_MTE;
>> + aarch64_features features;
>> + features.vq = aarch64_sve_get_vq (tid);
>> + features.pauth = hwcap & AARCH64_HWCAP_PACA;
>> + features.mte = hwcap2 & HWCAP2_MTE;
>> + features.tls = true;
>> - return aarch64_read_description (aarch64_sve_get_vq (tid), pauth_p, mte_p,
>> - true);
>> + return aarch64_read_description (features);
>> }
>> /* Convert a native/host siginfo object, into/from the siginfo in the
>> diff --git a/gdb/aarch64-linux-tdep.c b/gdb/aarch64-linux-tdep.c
>> index dbebcd4f0e0..453692df2f4 100644
>> --- a/gdb/aarch64-linux-tdep.c
>> +++ b/gdb/aarch64-linux-tdep.c
>> @@ -779,10 +779,13 @@ aarch64_linux_core_read_description (struct gdbarch *gdbarch,
>> CORE_ADDR hwcap = linux_get_hwcap (target);
>> CORE_ADDR hwcap2 = linux_get_hwcap2 (target);
>> - bool pauth_p = hwcap & AARCH64_HWCAP_PACA;
>> - bool mte_p = hwcap2 & HWCAP2_MTE;
>> - return aarch64_read_description (aarch64_linux_core_read_vq (gdbarch, abfd),
>> - pauth_p, mte_p, tls != nullptr);
>> + aarch64_features features;
>> + features.vq = aarch64_linux_core_read_vq (gdbarch, abfd);
>> + features.pauth = hwcap & AARCH64_HWCAP_PACA;
>> + features.mte = hwcap2 & HWCAP2_MTE;
>> + features.tls = tls != nullptr;
>> +
>> + return aarch64_read_description (features);
>> }
>> /* Implementation of `gdbarch_stap_is_single_operand', as defined in
>> diff --git a/gdb/aarch64-tdep.c b/gdb/aarch64-tdep.c
>> index 9d06ebfe27c..09fe8ae2fdb 100644
>> --- a/gdb/aarch64-tdep.c
>> +++ b/gdb/aarch64-tdep.c
>> @@ -52,13 +52,14 @@
>> #include "opcode/aarch64.h"
>> #include <algorithm>
>> +#include <unordered_map>
>> /* A Homogeneous Floating-Point or Short-Vector Aggregate may have at most
>> four members. */
>> #define HA_MAX_NUM_FLDS 4
>> /* All possible aarch64 target descriptors. */
>> -static target_desc *tdesc_aarch64_list[AARCH64_MAX_SVE_VQ + 1][2/*pauth*/][2 /* mte */][2 /* tls */];
>> +static std::unordered_map <aarch64_features, target_desc *> tdesc_aarch64_map;
>> /* The standard register names, and all the valid aliases for them. */
>> static const struct
>> @@ -3332,18 +3333,18 @@ aarch64_displaced_step_hw_singlestep (struct gdbarch *gdbarch)
>> TLS_P indicates the presence of the Thread Local Storage feature. */
>> const target_desc *
>> -aarch64_read_description (uint64_t vq, bool pauth_p, bool mte_p, bool tls_p)
>> +aarch64_read_description (const aarch64_features &features)
>> {
>> - if (vq > AARCH64_MAX_SVE_VQ)
>> - error (_("VQ is %" PRIu64 ", maximum supported value is %d"), vq,
>> + if (features.vq > AARCH64_MAX_SVE_VQ)
>> + error (_("VQ is %" PRIu64 ", maximum supported value is %d"), features.vq,
>> AARCH64_MAX_SVE_VQ);
>> - struct target_desc *tdesc = tdesc_aarch64_list[vq][pauth_p][mte_p][tls_p];
>> + struct target_desc *tdesc = tdesc_aarch64_map[features];
>> if (tdesc == NULL)
>> {
>> - tdesc = aarch64_create_target_description (vq, pauth_p, mte_p, tls_p);
>> - tdesc_aarch64_list[vq][pauth_p][mte_p][tls_p] = tdesc;
>> + tdesc = aarch64_create_target_description (features);
>> + tdesc_aarch64_map[features] = tdesc;
>> }
>> return tdesc;
>> @@ -3450,7 +3451,11 @@ aarch64_gdbarch_init (struct gdbarch_info info, struct gdbarch_list *arches)
>> value. */
>> const struct target_desc *tdesc = info.target_desc;
>> if (!tdesc_has_registers (tdesc) || vq != aarch64_get_tdesc_vq (tdesc))
>> - tdesc = aarch64_read_description (vq, false, false, false);
>> + {
>> + aarch64_features features;
>> + features.vq = vq;
>> + tdesc = aarch64_read_description (features);
>> + }
>> gdb_assert (tdesc);
>> feature_core = tdesc_find_feature (tdesc,"org.gnu.gdb.aarch64.core");
>> diff --git a/gdb/aarch64-tdep.h b/gdb/aarch64-tdep.h
>> index e4cdebb6311..3b9f67d6344 100644
>> --- a/gdb/aarch64-tdep.h
>> +++ b/gdb/aarch64-tdep.h
>> @@ -121,8 +121,7 @@ struct aarch64_gdbarch_tdep : gdbarch_tdep
>> }
>> };
>> -const target_desc *aarch64_read_description (uint64_t vq, bool pauth_p,
>> - bool mte_p, bool tls_p);
>> +const target_desc *aarch64_read_description (const aarch64_features &features);
>> extern int aarch64_process_record (struct gdbarch *gdbarch,
>> struct regcache *regcache, CORE_ADDR addr);
>> diff --git a/gdb/arch/aarch64.c b/gdb/arch/aarch64.c
>> index 733a3fd6d2a..0f73286f145 100644
>> --- a/gdb/arch/aarch64.c
>> +++ b/gdb/arch/aarch64.c
>> @@ -29,8 +29,7 @@
>> /* See arch/aarch64.h. */
>> target_desc *
>> -aarch64_create_target_description (uint64_t vq, bool pauth_p, bool mte_p,
>> - bool tls_p)
>> +aarch64_create_target_description (const aarch64_features &features)
>> {
>> target_desc_up tdesc = allocate_target_description ();
>> @@ -42,19 +41,19 @@ aarch64_create_target_description (uint64_t vq, bool pauth_p, bool mte_p,
>> regnum = create_feature_aarch64_core (tdesc.get (), regnum);
>> - if (vq == 0)
>> + if (features.vq == 0)
>> regnum = create_feature_aarch64_fpu (tdesc.get (), regnum);
>> else
>> - regnum = create_feature_aarch64_sve (tdesc.get (), regnum, vq);
>> + regnum = create_feature_aarch64_sve (tdesc.get (), regnum, features.vq);
>> - if (pauth_p)
>> + if (features.pauth)
>> regnum = create_feature_aarch64_pauth (tdesc.get (), regnum);
>> /* Memory tagging extension registers. */
>> - if (mte_p)
>> + if (features.mte)
>> regnum = create_feature_aarch64_mte (tdesc.get (), regnum);
>> - if (tls_p)
>> + if (features.tls)
>> regnum = create_feature_aarch64_tls (tdesc.get (), regnum);
>> return tdesc.release ();
>> diff --git a/gdb/arch/aarch64.h b/gdb/arch/aarch64.h
>> index 8496a0341f7..72ec4193eba 100644
>> --- a/gdb/arch/aarch64.h
>> +++ b/gdb/arch/aarch64.h
>> @@ -26,23 +26,43 @@
>> used to select register sets. */
>> struct aarch64_features
>> {
>> - bool sve = false;
>> + /* A non zero VQ value indicates both the presence of SVE and the
>> + Vector Quotient - the number of 128bit chunks in an SVE Z
>> + register. */
>> + uint64_t vq = 0;
>> +
>> bool pauth = false;
>> bool mte = false;
>> bool tls = false;
>> };
>> -/* Create the aarch64 target description. A non zero VQ value indicates both
>> - the presence of SVE and the Vector Quotient - the number of 128bit chunks in
>> - an SVE Z register. HAS_PAUTH_P indicates the presence of the PAUTH
>> - feature.
>> +inline bool operator==(const aarch64_features &lhs, const aarch64_features &rhs)
>> +{
>> + return lhs.vq == rhs.vq
>> + && lhs.pauth == rhs.pauth
>> + && lhs.mte == rhs.mte
>> + && lhs.tls == rhs.tls;
>> +}
>> - MTE_P indicates the presence of the Memory Tagging Extension feature.
>> +template<>
>> +struct std::hash<aarch64_features>
>> +{
>> + std::size_t operator()(const aarch64_features &features) const noexcept
>> + {
>> + std::size_t h;
>> - TLS_P indicates the presence of the Thread Local Storage feature. */
>> + h = features.vq;
>> + h = h << 1 | features.pauth;
>> + h = h << 1 | features.mte;
>> + h = h << 1 | features.tls;
>> + return h;
>> + }
>> +};
>> -target_desc *aarch64_create_target_description (uint64_t vq, bool has_pauth_p,
>> - bool mte_p, bool tls_p);
>> +/* Create the aarch64 target description. */
>> +
>> +target_desc *
>> + aarch64_create_target_description (const aarch64_features &features);
>> /* Register numbers of various important registers.
>> Note that on SVE, the Z registers reuse the V register numbers and the V
>> diff --git a/gdbserver/linux-aarch64-ipa.cc b/gdbserver/linux-aarch64-ipa.cc
>> index dc907d3ff88..918f85f6ea9 100644
>> --- a/gdbserver/linux-aarch64-ipa.cc
>> +++ b/gdbserver/linux-aarch64-ipa.cc
>> @@ -152,7 +152,7 @@ get_raw_reg (const unsigned char *raw_regs, int regnum)
>> const struct target_desc *
>> get_ipa_tdesc (int idx)
>> {
>> - return aarch64_linux_read_description (0, false, false, false);
>> + return aarch64_linux_read_description ({});
>> }
>> /* Allocate buffer for the jump pads. The branch instruction has a reach
>> @@ -205,5 +205,5 @@ void
>> initialize_low_tracepoint (void)
>> {
>> /* SVE, pauth, MTE and TLS not yet supported. */
>> - aarch64_linux_read_description (0, false, false, false);
>> + aarch64_linux_read_description ({});
>> }
>> diff --git a/gdbserver/linux-aarch64-low.cc b/gdbserver/linux-aarch64-low.cc
>> index ba0a810e0f9..db508696261 100644
>> --- a/gdbserver/linux-aarch64-low.cc
>> +++ b/gdbserver/linux-aarch64-low.cc
>> @@ -779,11 +779,11 @@ aarch64_adjust_register_sets (const struct aarch64_features &features)
>> break;
>> case NT_FPREGSET:
>> /* This is unavailable when SVE is present. */
>> - if (!features.sve)
>> + if (features.vq == 0)
>> regset->size = sizeof (struct user_fpsimd_state);
>> break;
>> case NT_ARM_SVE:
>> - if (features.sve)
>> + if (features.vq > 0)
>> regset->size = SVE_PT_SIZE (AARCH64_MAX_SVE_VQ, SVE_PT_REGS_SVE);
>> break;
>> case NT_ARM_PAC_MASK:
>> @@ -824,17 +824,14 @@ aarch64_target::low_arch_setup ()
>> {
>> struct aarch64_features features;
>> - uint64_t vq = aarch64_sve_get_vq (tid);
>> - features.sve = (vq > 0);
>> + features.vq = aarch64_sve_get_vq (tid);
>> /* A-profile PAC is 64-bit only. */
>> features.pauth = linux_get_hwcap (8) & AARCH64_HWCAP_PACA;
>> /* A-profile MTE is 64-bit only. */
>> features.mte = linux_get_hwcap2 (8) & HWCAP2_MTE;
>> features.tls = true;
>> - current_process ()->tdesc
>> - = aarch64_linux_read_description (vq, features.pauth, features.mte,
>> - features.tls);
>> + current_process ()->tdesc = aarch64_linux_read_description (features);
>> /* Adjust the register sets we should use for this particular set of
>> features. */
>> diff --git a/gdbserver/linux-aarch64-tdesc.cc b/gdbserver/linux-aarch64-tdesc.cc
>> index be96612d571..93f2bedde0d 100644
>> --- a/gdbserver/linux-aarch64-tdesc.cc
>> +++ b/gdbserver/linux-aarch64-tdesc.cc
>> @@ -25,36 +25,36 @@
>> #include "arch/aarch64.h"
>> #include "linux-aarch32-low.h"
>> #include <inttypes.h>
>> +#include <unordered_map>
>> /* All possible aarch64 target descriptors. */
>> -struct target_desc *tdesc_aarch64_list[AARCH64_MAX_SVE_VQ + 1][2/*pauth*/][2 /* mte */][2 /* tls */];
>> +static std::unordered_map<aarch64_features, target_desc *> tdesc_aarch64_map;
>> /* Create the aarch64 target description. */
>> const target_desc *
>> -aarch64_linux_read_description (uint64_t vq, bool pauth_p, bool mte_p,
>> - bool tls_p)
>> +aarch64_linux_read_description (const aarch64_features &features)
>> {
>> - if (vq > AARCH64_MAX_SVE_VQ)
>> - error (_("VQ is %" PRIu64 ", maximum supported value is %d"), vq,
>> + if (features.vq > AARCH64_MAX_SVE_VQ)
>> + error (_("VQ is %" PRIu64 ", maximum supported value is %d"), features.vq,
>> AARCH64_MAX_SVE_VQ);
>> - struct target_desc *tdesc = tdesc_aarch64_list[vq][pauth_p][mte_p][tls_p];
>> + struct target_desc *tdesc = tdesc_aarch64_map[features];
>> if (tdesc == NULL)
>> {
>> - tdesc = aarch64_create_target_description (vq, pauth_p, mte_p, tls_p);
>> + tdesc = aarch64_create_target_description (features);
>> static const char *expedite_regs_aarch64[] = { "x29", "sp", "pc", NULL };
>> static const char *expedite_regs_aarch64_sve[] = { "x29", "sp", "pc",
>> "vg", NULL };
>> - if (vq == 0)
>> + if (features.vq == 0)
>> init_target_desc (tdesc, expedite_regs_aarch64);
>> else
>> init_target_desc (tdesc, expedite_regs_aarch64_sve);
>> - tdesc_aarch64_list[vq][pauth_p][mte_p][tls_p] = tdesc;
>> + tdesc_aarch64_map[features] = tdesc;
>> }
>> return tdesc;
>> diff --git a/gdbserver/linux-aarch64-tdesc.h b/gdbserver/linux-aarch64-tdesc.h
>> index 4ab658447a2..30bcd24d13d 100644
>> --- a/gdbserver/linux-aarch64-tdesc.h
>> +++ b/gdbserver/linux-aarch64-tdesc.h
>> @@ -20,7 +20,9 @@
>> #ifndef GDBSERVER_LINUX_AARCH64_TDESC_H
>> #define GDBSERVER_LINUX_AARCH64_TDESC_H
>> -const target_desc * aarch64_linux_read_description (uint64_t vq, bool pauth_p,
>> - bool mte_p, bool tls_p);
>> +#include "arch/aarch64.h"
>> +
>> +const target_desc *
>> + aarch64_linux_read_description (const aarch64_features &features);
>> #endif /* GDBSERVER_LINUX_AARCH64_TDESC_H */
>> diff --git a/gdbserver/netbsd-aarch64-low.cc b/gdbserver/netbsd-aarch64-low.cc
>> index b371e599232..f8447b0d1ee 100644
>> --- a/gdbserver/netbsd-aarch64-low.cc
>> +++ b/gdbserver/netbsd-aarch64-low.cc
>> @@ -96,7 +96,7 @@ void
>> netbsd_aarch64_target::low_arch_setup ()
>> {
>> target_desc *tdesc
>> - = aarch64_create_target_description (0, false, false, false);
>> + = aarch64_create_target_description ({});
>> static const char *expedite_regs_aarch64[] = { "x29", "sp", "pc", NULL };
>> init_target_desc (tdesc, expedite_regs_aarch64);
>
>
I was considering something less C++ish, but this patch looks great to me. I think it cleans things up quite nicely.
Thanks for addressing this.
next prev parent reply other threads:[~2022-05-18 14:24 UTC|newest]
Thread overview: 6+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-05-05 18:46 John Baldwin
2022-05-18 13:59 ` John Baldwin
2022-05-18 14:23 ` Luis Machado [this message]
2022-05-20 11:52 ` Luis Machado
2022-05-20 16:39 ` John Baldwin
2022-05-23 9:29 ` Luis Machado
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=34e14ff6-8158-620b-6255-6e08b2fff567@arm.com \
--to=luis.machado@arm.com \
--cc=gdb-patches@sourceware.org \
--cc=jhb@FreeBSD.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).