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

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

[Luis Machado (Code Review)]

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, 24 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..f37a9ac 100644
--- a/gdb/gdbserver/tdesc.c
+++ b/gdb/gdbserver/tdesc.c
@@ -186,3 +186,20 @@
   tdesc->features.emplace_back (new_feature);
   return new_feature;
 }
+
+/* See gdbsupport/tdesc.h.  */
+
+bool
+tdesc_contains_feature (const target_desc *tdesc, std::string feature)
+{
+  if (tdesc && !tdesc->features.empty ())
+    {
+      for (const tdesc_feature_up &f : tdesc->features)
+	{
+	  if (f->name.compare (feature) == 0)
+	    return true;
+	}
+    }
+
+  return false;
+}
diff --git a/gdb/gdbserver/tdesc.h b/gdb/gdbserver/tdesc.h
index b93f53c..46987ac 100644
--- a/gdb/gdbserver/tdesc.h
+++ b/gdb/gdbserver/tdesc.h
@@ -93,4 +93,10 @@
 
 const struct target_desc *current_target_desc (void);
 
+/* Return true if TDESC contains the feature described by string FEATURE.
+   If TDESC has no features or if the feature string FEATURE was not found in
+   TDESC, return false.  */
+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: 1
Gerrit-Owner: Luis Machado <luis.machado@linaro.org>
Gerrit-MessageType: newchange

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

[Luis Machado (Code Review)]

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, 24 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..440d3b9 100644
--- a/gdb/gdbserver/tdesc.c
+++ b/gdb/gdbserver/tdesc.c
@@ -186,3 +186,20 @@
   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)
+{
+  if (tdesc && !tdesc->features.empty ())
+    {
+      for (const tdesc_feature_up &f : tdesc->features)
+	{
+	  if (f->name.compare (feature) == 0)
+	    return true;
+	}
+    }
+
+  return false;
+}
diff --git a/gdb/gdbserver/tdesc.h b/gdb/gdbserver/tdesc.h
index b93f53c..46987ac 100644
--- a/gdb/gdbserver/tdesc.h
+++ b/gdb/gdbserver/tdesc.h
@@ -93,4 +93,10 @@
 
 const struct target_desc *current_target_desc (void);
 
+/* Return true if TDESC contains the feature described by string FEATURE.
+   If TDESC has no features or if the feature string FEATURE was not found in
+   TDESC, return false.  */
+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: 2
Gerrit-Owner: Luis Machado <luis.machado@linaro.org>
Gerrit-MessageType: newpatchset

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

[Simon Marchi (Code Review)]

Simon Marchi has posted comments on this change.

Change URL: https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/690
......................................................................


Patch Set 2:

(3 comments)

Here are some minor comments.  I think Alan Hayward will be in a better position to assess the correctness of the patch.

| --- gdb/gdbserver/tdesc.c
| +++ gdb/gdbserver/tdesc.c
| @@ -184,5 +184,22 @@ tdesc_create_feature (struct target_desc *tdesc, const char *name)
|  {
|    struct tdesc_feature *new_feature = new tdesc_feature (name);
|    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)

PS2, Line 193:

Pass feature as a reference.

| +{
| +  if (tdesc && !tdesc->features.empty ())

PS2, Line 195:

tdesc != nullptr

Or just assume/assert it is not NULL?  Is there a legitimate use case
for passing a NULL tdesc?

| +    {
| +      for (const tdesc_feature_up &f : tdesc->features)
| +	{
| +	  if (f->name.compare (feature) == 0)

PS2, Line 199:

This can be

 f->name == feature

| +	    return true;
| +	}
| +    }
| +
| +  return false;
| +}

-- 
Gerrit-Project: binutils-gdb
Gerrit-Branch: master
Gerrit-Change-Id: I28b782cb1677560ca9a06a1be442974b25aabae4
Gerrit-Change-Number: 690
Gerrit-PatchSet: 2
Gerrit-Owner: Luis Machado <luis.machado@linaro.org>
Gerrit-CC: Simon Marchi <simon.marchi@polymtl.ca>
Gerrit-Comment-Date: Tue, 19 Nov 2019 14:22:57 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Gerrit-MessageType: comment

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

[Luis Machado (Code Review)]

Luis Machado has posted comments on this change.

Change URL: https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/690
......................................................................


Patch Set 2:

(3 comments)

| --- gdb/gdbserver/tdesc.c
| +++ gdb/gdbserver/tdesc.c
| @@ -184,5 +184,22 @@ tdesc_create_feature (struct target_desc *tdesc, const char *name)
|  {
|    struct tdesc_feature *new_feature = new tdesc_feature (name);
|    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)

PS2, Line 193:

Thanks! I'll fix this. Getting up to speed on the cpp-ness of it!

| +{
| +  if (tdesc && !tdesc->features.empty ())

PS2, Line 195:

No use case in particular for a NULL tdesc, though i tend to think
having the check here prevents having to add such a check elsewhere.
We don't want gdbserver to die when we see a NULL tdesc for example.

Unless a NULL tdesc is a major error we don't want to allow. Then i
agree an assert would be good. Thoughts?

| +    {
| +      for (const tdesc_feature_up &f : tdesc->features)
| +	{
| +	  if (f->name.compare (feature) == 0)

PS2, Line 199:

I'll fix this.

| +	    return true;
| +	}
| +    }
| +
| +  return false;
| +}

-- 
Gerrit-Project: binutils-gdb
Gerrit-Branch: master
Gerrit-Change-Id: I28b782cb1677560ca9a06a1be442974b25aabae4
Gerrit-Change-Number: 690
Gerrit-PatchSet: 2
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-Comment-Date: Tue, 19 Nov 2019 14:29:55 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Simon Marchi <simon.marchi@polymtl.ca>
Gerrit-MessageType: comment

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

[Luis Machado (Code Review)]

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, 24 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..20e6263 100644
--- a/gdb/gdbserver/tdesc.c
+++ b/gdb/gdbserver/tdesc.c
@@ -186,3 +186,20 @@
   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)
+{
+  if (tdesc != nullptr && !tdesc->features.empty ())
+    {
+      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..d74c6b0 100644
--- a/gdb/gdbserver/tdesc.h
+++ b/gdb/gdbserver/tdesc.h
@@ -93,4 +93,10 @@
 
 const struct target_desc *current_target_desc (void);
 
+/* Return true if TDESC contains the feature described by string FEATURE.
+   If TDESC has no features or if the feature string FEATURE was not found in
+   TDESC, return false.  */
+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: 3
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

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

[Simon Marchi (Code Review)]

Simon Marchi has posted comments on this change.

Change URL: https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/690
......................................................................


Patch Set 3:

(1 comment)

| --- gdb/gdbserver/tdesc.c
| +++ gdb/gdbserver/tdesc.c
| @@ -186,3 +186,19 @@ tdesc_create_feature (struct target_desc *tdesc, const char *name)
|    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)
| +{
| +  if (tdesc && !tdesc->features.empty ())

PS2, Line 195:

> No use case in particular for a NULL tdesc, though i tend to think having the check here prevents having to add such a check elsewhere. We don't want gdbserver to die when we see a NULL tdesc for example.
> 
> Unless a NULL tdesc is a major error we don't want to allow. Then i agree an assert would be good. Thoughts?

I don't know if it's possible to execute with no target description in
gdbserver, or if we always have one.

regcache->tdesc is initialized in init_register_cache.  I checked the
various paths that could be called, and it appears to me like it's
always non-NULL.  For example:

- the new_register_cache call at server.c:4110, passes
current_target_desc.  current_target_desc returns a default target
description at worst
- get_thread_regcache passes a tdesc, which it asserts to be not null
- in tracepoint.c, we call get_ipa_tdesc, which is target-specific.  I
did a quick scan of the architectures, and it looks like they always
return something non-NULL.

I believe that it's not possible to get a NULL tdesc here, so I would
vote for making it an assert.

| +    {
| +      for (const tdesc_feature_up &f : tdesc->features)
| +	{
| +	  if (f->name.compare (feature) == 0)
| +	    return true;
| +	}
| +    }
| +
| +  return false;

-- 
Gerrit-Project: binutils-gdb
Gerrit-Branch: master
Gerrit-Change-Id: I28b782cb1677560ca9a06a1be442974b25aabae4
Gerrit-Change-Number: 690
Gerrit-PatchSet: 3
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-Comment-Date: Tue, 19 Nov 2019 15:02:49 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Luis Machado <luis.machado@linaro.org>
Comment-In-Reply-To: Simon Marchi <simon.marchi@polymtl.ca>
Gerrit-MessageType: comment

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

[Luis Machado (Code Review)]

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



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

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

[Alan Hayward]

(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

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

[Luis Machado]

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
> 

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

[Alan Hayward]

> 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

[pushed] Improve target description check for SVE in gdbserver

[Sourceware to Gerrit sync (Code Review)]

Sourceware to Gerrit sync has submitted this change.

Change URL: https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/690
......................................................................

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-20  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.

Change-Id: I28b782cb1677560ca9a06a1be442974b25aabae4
---
M gdb/gdbserver/ChangeLog
M gdb/gdbserver/linux-aarch64-low.c
M gdb/gdbserver/tdesc.c
M gdb/gdbserver/tdesc.h
4 files changed, 29 insertions(+), 1 deletion(-)


diff --git a/gdb/gdbserver/ChangeLog b/gdb/gdbserver/ChangeLog
index fde6abb..a5da6b5 100644
--- a/gdb/gdbserver/ChangeLog
+++ b/gdb/gdbserver/ChangeLog
@@ -1,3 +1,10 @@
+2019-11-20  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.
+
 2019-11-15  Christian Biesinger  <cbiesinger@google.com>
 
 	* Makefile.in: Add safe-strerror.c.
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: 5
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: merged

[pushed] Improve target description check for SVE in gdbserver

[Sourceware to Gerrit sync (Code Review)]

The original change was created by Luis Machado.

Change URL: https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/690
......................................................................

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-20  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.

Change-Id: I28b782cb1677560ca9a06a1be442974b25aabae4
---
M gdb/gdbserver/ChangeLog
M gdb/gdbserver/linux-aarch64-low.c
M gdb/gdbserver/tdesc.c
M gdb/gdbserver/tdesc.h
4 files changed, 29 insertions(+), 1 deletion(-)



diff --git a/gdb/gdbserver/ChangeLog b/gdb/gdbserver/ChangeLog
index fde6abb..a5da6b5 100644
--- a/gdb/gdbserver/ChangeLog
+++ b/gdb/gdbserver/ChangeLog
@@ -1,3 +1,10 @@
+2019-11-20  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.
+
 2019-11-15  Christian Biesinger  <cbiesinger@google.com>
 
 	* Makefile.in: Add safe-strerror.c.
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: 5
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