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

[Luis Machado <luis.machado@arm.com>]

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