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

Message ID cd24e42eeb1e4a51f62f3c80a25a607533effcc2.1728239729.git.aburgess@redhat.com
State New
Headers
Series Set osabi in remote target descriptions |

Checks

Context Check Description
linaro-tcwg-bot/tcwg_gdb_build--master-aarch64 success Build passed
linaro-tcwg-bot/tcwg_gdb_build--master-arm success Build passed
linaro-tcwg-bot/tcwg_gdb_check--master-arm success Test passed
linaro-tcwg-bot/tcwg_gdb_check--master-aarch64 success Test passed

Commit Message

Andrew Burgess Oct. 6, 2024, 6:37 p.m. UTC
  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.
---
 gdbserver/tdesc.cc | 16 +++++-----------
 gdbserver/tdesc.h  |  6 ++----
 2 files changed, 7 insertions(+), 15 deletions(-)
  

Comments

Tom Tromey Oct. 7, 2024, 5 p.m. UTC | #1
>>>>> "Andrew" == Andrew Burgess <aburgess@redhat.com> writes:

Andrew> Convert target_desc::arch and target_desc::osabi from 'const char*' to
Andrew> gdb::unique_xmalloc_ptr<char>.  This also allows us to remove the user
Andrew> defined ~target_desc destructor.

Andrew> I doubt it ever actually occurred, but in theory at least, there was a
Andrew> memory leak in set_tdesc_architecture and set_tdesc_osabi where the
Andrew> member variables were assigned without freeing any previous
Andrew> value... but I suspect that usually these fields are only set once.

Andrew> There should be no user visible changes after this commit.

Looks good to me, thank you.
Approved-By: Tom Tromey <tom@tromey.com>

Tom
  
Simon Marchi Oct. 8, 2024, 7:02 p.m. UTC | #2
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?

Simon
  
Andrew Burgess Oct. 9, 2024, 11:07 a.m. UTC | #3
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.

Thanks,
Andrew
  
Simon Marchi Oct. 9, 2024, 11:45 a.m. UTC | #4
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.

Simon
  
Andrew Burgess Oct. 9, 2024, 12:17 p.m. UTC | #5
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 :)

Thanks,
Andrew
  
Simon Marchi Oct. 9, 2024, 3:35 p.m. UTC | #6
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
  
Andrew Burgess Oct. 9, 2024, 3:50 p.m. UTC | #7
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
  

Patch

diff --git a/gdbserver/tdesc.cc b/gdbserver/tdesc.cc
index 3265793da96..7f11120c318 100644
--- a/gdbserver/tdesc.cc
+++ b/gdbserver/tdesc.cc
@@ -20,12 +20,6 @@ 
 
 #ifndef IN_PROCESS_AGENT
 
-target_desc::~target_desc ()
-{
-  xfree ((char *) arch);
-  xfree ((char *) osabi);
-}
-
 bool target_desc::operator== (const target_desc &other) const
 {
   if (reg_defs != other.reg_defs)
@@ -162,7 +156,7 @@  tdesc_compatible_info_arch_name (const tdesc_compatible_info_up &c_info)
 const char *
 tdesc_architecture_name (const struct target_desc *target_desc)
 {
-  return target_desc->arch;
+  return target_desc->arch.get ();
 }
 
 /* See gdbsupport/tdesc.h.  */
@@ -171,7 +165,7 @@  void
 set_tdesc_architecture (struct target_desc *target_desc,
 			const char *name)
 {
-  target_desc->arch = xstrdup (name);
+  target_desc->arch = make_unique_xstrdup (name);
 }
 
 /* See gdbsupport/tdesc.h.  */
@@ -179,7 +173,7 @@  set_tdesc_architecture (struct target_desc *target_desc,
 const char *
 tdesc_osabi_name (const struct target_desc *target_desc)
 {
-  return target_desc->osabi;
+  return target_desc->osabi.get ();
 }
 
 /* See gdbsupport/tdesc.h.  */
@@ -187,7 +181,7 @@  tdesc_osabi_name (const struct target_desc *target_desc)
 void
 set_tdesc_osabi (struct target_desc *target_desc, const char *name)
 {
-  target_desc->osabi = xstrdup (name);
+  target_desc->osabi = make_unique_xstrdup (name);
 }
 
 /* See gdbsupport/tdesc.h.  */
@@ -198,7 +192,7 @@  tdesc_get_features_xml (const target_desc *tdesc)
   /* Either .xmltarget or .features is not NULL.  */
   gdb_assert (tdesc->xmltarget != NULL
 	      || (!tdesc->features.empty ()
-		  && tdesc->arch != NULL));
+		  && tdesc_architecture_name (tdesc) != nullptr));
 
   if (tdesc->xmltarget == NULL)
     {
diff --git a/gdbserver/tdesc.h b/gdbserver/tdesc.h
index 534b8b000f5..6fc37d038bc 100644
--- a/gdbserver/tdesc.h
+++ b/gdbserver/tdesc.h
@@ -54,18 +54,16 @@  struct target_desc final : tdesc_element
   mutable const char *xmltarget = NULL;
 
   /* The value of <architecture> element in the XML, replying GDB.  */
-  const char *arch = NULL;
+  gdb::unique_xmalloc_ptr<char> arch = nullptr;
 
   /* The value of <osabi> element in the XML, replying GDB.  */
-  const char *osabi = NULL;
+  gdb::unique_xmalloc_ptr<char> osabi = nullptr;
 
 public:
   target_desc ()
     : registers_size (0)
   {}
 
-  ~target_desc ();
-
   bool operator== (const target_desc &other) const;
 
   bool operator!= (const target_desc &other) const