Re: [review v4] [AArch64, SVE] Improve target description check for SVE in gdbserver

[Alan Hayward <Alan dot Hayward at arm dot com>]

(Gerrit appears not to be sending me account verification emails, so can’t review from there...)


> On 19 Nov 2019, at 15:14, Luis Machado (Code Review) <gerrit@gnutoolchain-gerrit.osci.io> wrote:
> 
> Change URL: https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/690
> ......................................................................
> 
> [AArch64, SVE] Improve target description check for SVE in gdbserver
> 
> The current code checks for the presence of a SVE target description by
> comparing the number of registers.  This is a bit fragile since the number
> of registers can change whenever we add new sets. Like PAC, for example.
> 

Yes, agreed.

> If the comparison breaks, then we're left with SVE registers in the
> description, but gdbserver doesn't send the registers to GDB, which in
> turn displays stale information to the user.
> 
> The following patch changes the check to use the SVE feature string instead,
> which hopefully should be more stable.

My only concern here is if it’s possible to end up with a tdesc without the correct feature name.
If this was GDB connected to a not-gdbserver remote stub, then yes, I think that’s possible.
However, given this is gdbserver, I think it should always be correctly creating the tdesc with the feature names.

So, it’s an ok from me.

Alan.

> 
> gdb/gdbserver/ChangeLog:
> 
> 2019-11-18  Luis Machado  <luis.machado@linaro.org>
> 
> 	* linux-aarch64-low.c (is_sve_tdesc): Check against target feature
> 	instead of register count.
> 	* tdesc.c (tdesc_contains_feature): New function.
> 	* tdesc.h (tdesc_contains_feature): New prototype.
> 
> Signed-off-by: Luis Machado <luis.machado@linaro.org>
> Change-Id: I28b782cb1677560ca9a06a1be442974b25aabae4
> ---
> M gdb/gdbserver/linux-aarch64-low.c
> M gdb/gdbserver/tdesc.c
> M gdb/gdbserver/tdesc.h
> 3 files changed, 22 insertions(+), 1 deletion(-)
> 
> 
> 
> diff --git a/gdb/gdbserver/linux-aarch64-low.c b/gdb/gdbserver/linux-aarch64-low.c
> index 87a21a0..9e234e0 100644
> --- a/gdb/gdbserver/linux-aarch64-low.c
> +++ b/gdb/gdbserver/linux-aarch64-low.c
> @@ -83,7 +83,7 @@
> {
>   struct regcache *regcache = get_thread_regcache (current_thread, 0);
> 
> -  return regcache->tdesc->reg_defs.size () == AARCH64_SVE_NUM_REGS;
> +  return tdesc_contains_feature (regcache->tdesc, "org.gnu.gdb.aarch64.sve");
> }
> 
> static void
> diff --git a/gdb/gdbserver/tdesc.c b/gdb/gdbserver/tdesc.c
> index 92a0a60..817e69a 100644
> --- a/gdb/gdbserver/tdesc.c
> +++ b/gdb/gdbserver/tdesc.c
> @@ -186,3 +186,19 @@
>   tdesc->features.emplace_back (new_feature);
>   return new_feature;
> }
> +
> +/* See gdbsupport/tdesc.h.  */
> +
> +bool
> +tdesc_contains_feature (const target_desc *tdesc, const std::string &feature)
> +{
> +  gdb_assert (tdesc != nullptr);
> +
> +  for (const tdesc_feature_up &f : tdesc->features)
> +    {
> +      if (f->name == feature)
> +	return true;
> +    }
> +
> +  return false;
> +}
> diff --git a/gdb/gdbserver/tdesc.h b/gdb/gdbserver/tdesc.h
> index b93f53c..da21cda 100644
> --- a/gdb/gdbserver/tdesc.h
> +++ b/gdb/gdbserver/tdesc.h
> @@ -93,4 +93,9 @@
> 
> const struct target_desc *current_target_desc (void);
> 
> +/* Return true if TDESC contains the feature described by string FEATURE.
> +   Return false otherwise.  */
> +bool tdesc_contains_feature (const target_desc *tdesc,
> +			     const std::string &feature);
> +
> #endif /* GDBSERVER_TDESC_H */
> 
> -- 
> Gerrit-Project: binutils-gdb
> Gerrit-Branch: master
> Gerrit-Change-Id: I28b782cb1677560ca9a06a1be442974b25aabae4
> Gerrit-Change-Number: 690
> Gerrit-PatchSet: 4
> Gerrit-Owner: Luis Machado <luis.machado@linaro.org>
> Gerrit-Reviewer: Luis Machado <luis.machado@linaro.org>
> Gerrit-CC: Simon Marchi <simon.marchi@polymtl.ca>
> Gerrit-MessageType: newpatchset