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

Simon Marchi via Gdb-patches
2023-09-08T16:53:01Z

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