Re: [PATCH 1/3] gdbserver: make arch and osabi names gdb::unique_xmalloc_ptr<char>

[Andrew Burgess <aburgess@redhat.com>]

Simon Marchi <simark@simark.ca> writes:

> On 2024-10-09 08:17, Andrew Burgess wrote:
>> Simon Marchi <simark@simark.ca> writes:
>> 
>>> On 2024-10-09 07:07, Andrew Burgess wrote:
>>>> Simon Marchi <simark@simark.ca> writes:
>>>>
>>>>> On 2024-10-06 14:37, Andrew Burgess wrote:
>>>>>> Convert target_desc::arch and target_desc::osabi from 'const char*' to
>>>>>> gdb::unique_xmalloc_ptr<char>.  This also allows us to remove the user
>>>>>> defined ~target_desc destructor.
>>>>>>
>>>>>> I doubt it ever actually occurred, but in theory at least, there was a
>>>>>> memory leak in set_tdesc_architecture and set_tdesc_osabi where the
>>>>>> member variables were assigned without freeing any previous
>>>>>> value... but I suspect that usually these fields are only set once.
>>>>>>
>>>>>> There should be no user visible changes after this commit.
>>>>>
>>>>> Could they be std::string instead?
>>>>
>>>> I considered this, but that would, I think, require wider reaching
>>>> changes as the API to add the osabi to the returned tdesc passes the
>>>> information via 'const char *'.  This approach added the memory safety
>>>> without requiring any further changes.
>>>>
>>>> If you'd prefer std::string be used then let me know and I'll update
>>>> things.
>>>
>>> Up to you, I don't mind, it can be done later.
>> 
>> I'll leave it then if that's OK.
>> 
>> My reason is that, if the osbi member variable changes to std::string
>> then it makes sense that tdesc_osabi_name return 'const std::string &'.
>> Which means that tdesc_osabi_name in GDB will also need to return 'const
>> std::string &', but on the GDB side we store 'enum gdb_osabi' rather
>> than the actual string ... so now that code needs updating too.
>> 
>> I only wanted to fix this so that I could call set_tdesc_osbi without
>> leaking memory, and gdb::unique_xmalloc_ptr<char>, which we use in loads
>> of places, achieves that, and leaves the API untouched.
>> 
>> But I'm not against changing to std::string if someone's keen :)
>
> Yeah, it makes sense to take smaller, simpler steps.  I might do it once
> your series is merged, to scratch my itch :).

Sounds good.

I'll hold off merging until you've had a chance to check out v2 w.r.t
the switch to use the 'enum gdb_osabi' instead of passing strings.

Thanks,
Andrew