diff mbox

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

Message ID 20191119151417.76C002816F@gnutoolchain-gerrit.osci.io
State New, archived
Headers

Commit Message

Simon Marchi (Code Review) Nov. 19, 2019, 3:14 p.m. UTC 
  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.

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.

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(-)

Comments

Alan Hayward Nov. 20, 2019, 4:34 p.m. UTC | #1 
(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
Luis Machado Nov. 20, 2019, 4:51 p.m. UTC | #2 
Alan,

On 11/20/19 1:34 PM, Alan Hayward wrote:
> (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.

We usually make the feature names standard. GDB is responsible for 
dictating the main required features anyway, so other tools will follow 
what GDB has set. Other optional features may be provided by proprietary 
debugging stubs as needed though.

> 
> So, it’s an ok from me.
> 

Thanks! I'll push it shortly.

> 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
>
Simon Marchi Nov. 20, 2019, 4:51 p.m. UTC | #3 
On 2019-11-20 11:34 a.m., Alan Hayward wrote:
> (Gerrit appears not to be sending me account verification emails, so can’t review from there...)

There appears to be some problems with these emails, I don't know why
at the moment.  I've activated your account by hand.

Simon
Alan Hayward Nov. 21, 2019, 8:23 a.m. UTC | #4 
> On 20 Nov 2019, at 16:51, Simon Marchi <simon.marchi@polymtl.ca> wrote:

> 

> On 2019-11-20 11:34 a.m., Alan Hayward wrote:

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

> 

> There appears to be some problems with these emails, I don't know why

> at the moment.


Looks like they were working, but MS exchange quarantined them and didn’t let me know until this morning.

>  I've activated your account by hand.


Many thanks.

> 

> Simon
diff mbox

Patch

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 */