diff mbox series

[v5,06/16,gdbserver/aarch64] refactor: Adjust expedited registers dynamically

Message ID 20230907152018.1031257-7-luis.machado@arm.com
State New
Headers
Series SME support for AArch64 gdb/gdbserver on Linux |

Commit Message

Luis Machado Sept. 7, 2023, 3:20 p.m. UTC 
  Instead of using static arrays, build the list of expedited registers
dynamically using a std::vector.

This refactor shouldn't cause any user-visible changes.

Regression-tested for aarch64-linux Ubuntu 22.04/20.04.
---
 gdbserver/linux-aarch64-tdesc.cc | 21 ++++++++++++++-------
 1 file changed, 14 insertions(+), 7 deletions(-)

Comments

Simon Marchi Sept. 8, 2023, 3:35 p.m. UTC | #1 
On 9/7/23 11:20, Luis Machado via Gdb-patches wrote:
> Instead of using static arrays, build the list of expedited registers
> dynamically using a std::vector.
> 
> This refactor shouldn't cause any user-visible changes.
> 
> Regression-tested for aarch64-linux Ubuntu 22.04/20.04.
> ---
>  gdbserver/linux-aarch64-tdesc.cc | 21 ++++++++++++++-------
>  1 file changed, 14 insertions(+), 7 deletions(-)
> 
> diff --git a/gdbserver/linux-aarch64-tdesc.cc b/gdbserver/linux-aarch64-tdesc.cc
> index 633134955e5..3c60e1a4db0 100644
> --- a/gdbserver/linux-aarch64-tdesc.cc
> +++ b/gdbserver/linux-aarch64-tdesc.cc
> @@ -30,6 +30,8 @@
>  /* All possible aarch64 target descriptors.  */
>  static std::unordered_map<aarch64_features, target_desc *> tdesc_aarch64_map;
>  
> +static std::vector<const char *> expedited_registers;
> +
>  /* Create the aarch64 target description.  */
>  
>  const target_desc *
> @@ -44,15 +46,20 @@ aarch64_linux_read_description (const aarch64_features &features)
>    if (tdesc == NULL)
>      {
>        tdesc = aarch64_create_target_description (features);
> +      expedited_registers.clear ();
> +
> +      /* Configure the expedited registers.  By default we include x29, sp and
> +	 pc.  */
> +      expedited_registers.push_back ("x29");
> +      expedited_registers.push_back ("sp");
> +      expedited_registers.push_back ("pc");
> +
> +      if (features.vq > 0)
> +	expedited_registers.push_back ("vg");
>  
> -      static const char *expedite_regs_aarch64[] = { "x29", "sp", "pc", NULL };
> -      static const char *expedite_regs_aarch64_sve[] = { "x29", "sp", "pc",
> -							 "vg", NULL };
> +      expedited_registers.push_back (nullptr);
>  
> -      if (features.vq == 0)
> -	init_target_desc (tdesc, expedite_regs_aarch64);
> -      else
> -	init_target_desc (tdesc, expedite_regs_aarch64_sve);
> +      init_target_desc (tdesc, (const char **) expedited_registers.data ());

It seems weird to have a single std::vector instance.  What happens if
multiple target descriptions are created?  Won't the second mess up the
data of the first one?

Simon
Luis Machado Sept. 8, 2023, 4 p.m. UTC | #2 
On 9/8/23 16:35, Simon Marchi wrote:
> On 9/7/23 11:20, Luis Machado via Gdb-patches wrote:
>> Instead of using static arrays, build the list of expedited registers
>> dynamically using a std::vector.
>>
>> This refactor shouldn't cause any user-visible changes.
>>
>> Regression-tested for aarch64-linux Ubuntu 22.04/20.04.
>> ---
>>  gdbserver/linux-aarch64-tdesc.cc | 21 ++++++++++++++-------
>>  1 file changed, 14 insertions(+), 7 deletions(-)
>>
>> diff --git a/gdbserver/linux-aarch64-tdesc.cc b/gdbserver/linux-aarch64-tdesc.cc
>> index 633134955e5..3c60e1a4db0 100644
>> --- a/gdbserver/linux-aarch64-tdesc.cc
>> +++ b/gdbserver/linux-aarch64-tdesc.cc
>> @@ -30,6 +30,8 @@
>>  /* All possible aarch64 target descriptors.  */
>>  static std::unordered_map<aarch64_features, target_desc *> tdesc_aarch64_map;
>>  
>> +static std::vector<const char *> expedited_registers;
>> +
>>  /* Create the aarch64 target description.  */
>>  
>>  const target_desc *
>> @@ -44,15 +46,20 @@ aarch64_linux_read_description (const aarch64_features &features)
>>    if (tdesc == NULL)
>>      {
>>        tdesc = aarch64_create_target_description (features);
>> +      expedited_registers.clear ();
>> +
>> +      /* Configure the expedited registers.  By default we include x29, sp and
>> +	 pc.  */
>> +      expedited_registers.push_back ("x29");
>> +      expedited_registers.push_back ("sp");
>> +      expedited_registers.push_back ("pc");
>> +
>> +      if (features.vq > 0)
>> +	expedited_registers.push_back ("vg");
>>  
>> -      static const char *expedite_regs_aarch64[] = { "x29", "sp", "pc", NULL };
>> -      static const char *expedite_regs_aarch64_sve[] = { "x29", "sp", "pc",
>> -							 "vg", NULL };
>> +      expedited_registers.push_back (nullptr);
>>  
>> -      if (features.vq == 0)
>> -	init_target_desc (tdesc, expedite_regs_aarch64);
>> -      else
>> -	init_target_desc (tdesc, expedite_regs_aarch64_sve);
>> +      init_target_desc (tdesc, (const char **) expedited_registers.data ());
> 
> It seems weird to have a single std::vector instance.  What happens if
> multiple target descriptions are created?  Won't the second mess up the
> data of the first one?

Yeah. Well spotted. I think I'll need to have a map-based entry for the expedited registers, because init_target_desc
just copies the pointer instead of the contents.

Let me think about that.

Thanks,
Luis
Simon Marchi Sept. 8, 2023, 4:52 p.m. UTC | #3 
On 9/8/23 12:00, Luis Machado wrote:
> On 9/8/23 16:35, Simon Marchi wrote:
>> On 9/7/23 11:20, Luis Machado via Gdb-patches wrote:
>>> Instead of using static arrays, build the list of expedited registers
>>> dynamically using a std::vector.
>>>
>>> This refactor shouldn't cause any user-visible changes.
>>>
>>> Regression-tested for aarch64-linux Ubuntu 22.04/20.04.
>>> ---
>>>  gdbserver/linux-aarch64-tdesc.cc | 21 ++++++++++++++-------
>>>  1 file changed, 14 insertions(+), 7 deletions(-)
>>>
>>> diff --git a/gdbserver/linux-aarch64-tdesc.cc b/gdbserver/linux-aarch64-tdesc.cc
>>> index 633134955e5..3c60e1a4db0 100644
>>> --- a/gdbserver/linux-aarch64-tdesc.cc
>>> +++ b/gdbserver/linux-aarch64-tdesc.cc
>>> @@ -30,6 +30,8 @@
>>>  /* All possible aarch64 target descriptors.  */
>>>  static std::unordered_map<aarch64_features, target_desc *> tdesc_aarch64_map;
>>>  
>>> +static std::vector<const char *> expedited_registers;
>>> +
>>>  /* Create the aarch64 target description.  */
>>>  
>>>  const target_desc *
>>> @@ -44,15 +46,20 @@ aarch64_linux_read_description (const aarch64_features &features)
>>>    if (tdesc == NULL)
>>>      {
>>>        tdesc = aarch64_create_target_description (features);
>>> +      expedited_registers.clear ();
>>> +
>>> +      /* Configure the expedited registers.  By default we include x29, sp and
>>> +	 pc.  */
>>> +      expedited_registers.push_back ("x29");
>>> +      expedited_registers.push_back ("sp");
>>> +      expedited_registers.push_back ("pc");
>>> +
>>> +      if (features.vq > 0)
>>> +	expedited_registers.push_back ("vg");
>>>  
>>> -      static const char *expedite_regs_aarch64[] = { "x29", "sp", "pc", NULL };
>>> -      static const char *expedite_regs_aarch64_sve[] = { "x29", "sp", "pc",
>>> -							 "vg", NULL };
>>> +      expedited_registers.push_back (nullptr);
>>>  
>>> -      if (features.vq == 0)
>>> -	init_target_desc (tdesc, expedite_regs_aarch64);
>>> -      else
>>> -	init_target_desc (tdesc, expedite_regs_aarch64_sve);
>>> +      init_target_desc (tdesc, (const char **) expedited_registers.data ());
>>
>> It seems weird to have a single std::vector instance.  What happens if
>> multiple target descriptions are created?  Won't the second mess up the
>> data of the first one?
> 
> Yeah. Well spotted. I think I'll need to have a map-based entry for the expedited registers, because init_target_desc
> just copies the pointer instead of the contents.

This can probably be changed, to make the target desc hold a vector.

Simon
diff mbox series

Patch

diff --git a/gdbserver/linux-aarch64-tdesc.cc b/gdbserver/linux-aarch64-tdesc.cc
index 633134955e5..3c60e1a4db0 100644
--- a/gdbserver/linux-aarch64-tdesc.cc
+++ b/gdbserver/linux-aarch64-tdesc.cc
@@ -30,6 +30,8 @@ 
 /* All possible aarch64 target descriptors.  */
 static std::unordered_map<aarch64_features, target_desc *> tdesc_aarch64_map;
 
+static std::vector<const char *> expedited_registers;
+
 /* Create the aarch64 target description.  */
 
 const target_desc *
@@ -44,15 +46,20 @@  aarch64_linux_read_description (const aarch64_features &features)
   if (tdesc == NULL)
     {
       tdesc = aarch64_create_target_description (features);
+      expedited_registers.clear ();
+
+      /* Configure the expedited registers.  By default we include x29, sp and
+	 pc.  */
+      expedited_registers.push_back ("x29");
+      expedited_registers.push_back ("sp");
+      expedited_registers.push_back ("pc");
+
+      if (features.vq > 0)
+	expedited_registers.push_back ("vg");
 
-      static const char *expedite_regs_aarch64[] = { "x29", "sp", "pc", NULL };
-      static const char *expedite_regs_aarch64_sve[] = { "x29", "sp", "pc",
-							 "vg", NULL };
+      expedited_registers.push_back (nullptr);
 
-      if (features.vq == 0)
-	init_target_desc (tdesc, expedite_regs_aarch64);
-      else
-	init_target_desc (tdesc, expedite_regs_aarch64_sve);
+      init_target_desc (tdesc, (const char **) expedited_registers.data ());
 
       tdesc_aarch64_map[features] = tdesc;
     }