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

[Simon Marchi <simark@simark.ca>]

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 :).

Simon