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

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

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;
| +}
  

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;
| +}
  

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;