gdb: make inferior::terminal a unique ptr

Message ID 20200625155517.32173-1-simon.marchi@efficios.com
State New
Headers show
Series
  • gdb: make inferior::terminal a unique ptr
Related show

Commit Message

Jose E. Marchesi via Gdb-patches June 25, 2020, 3:55 p.m.
This changes the inferior::terminal field to be a unique pointer, so its
deallocation is automatically managed.

gdb/ChangeLog:

	* inferior.h (struct inferior) <terminal>: Change type to
	gdb::unique_xmalloc_ptr<char>.
	* inferior.c (inferior::~inferior): Don't free inf->terminal.
	* infcmd.c (set_inferior_io_terminal): Don't free terminal
	field, adjust to unique pointer.
	(get_inferior_io_terminal): Adjust to unique pointer.

Change-Id: Iedb6459b4f9eeae812b0cb9d514b5707d5107cdb
---
 gdb/infcmd.c   | 6 ++----
 gdb/inferior.c | 1 -
 gdb/inferior.h | 3 ++-
 3 files changed, 4 insertions(+), 6 deletions(-)

-- 
2.27.0

Comments

Jose E. Marchesi via Gdb-patches June 25, 2020, 5:18 p.m. | #1
On Thu, Jun 25, 2020 at 10:55 AM Simon Marchi via Gdb-patches
<gdb-patches@sourceware.org> wrote:
> -    current_inferior ()->terminal = xstrdup (terminal_name);

> +    current_inferior ()->terminal.reset (xstrdup (terminal_name));


Perhaps it really should be a std::string?

Christian
Pedro Alves June 25, 2020, 5:28 p.m. | #2
On 6/25/20 6:18 PM, Christian Biesinger via Gdb-patches wrote:
> On Thu, Jun 25, 2020 at 10:55 AM Simon Marchi via Gdb-patches

> <gdb-patches@sourceware.org> wrote:

>> -    current_inferior ()->terminal = xstrdup (terminal_name);

>> +    current_inferior ()->terminal.reset (xstrdup (terminal_name));

> 

> Perhaps it really should be a std::string?


I think there's no good reason for that.  It's a string that doesn't
ever need to grow/shrink.  And then its use is to pass down to
a syscall that accepts C-style null-terminated strings:

  tty = open (inferior_thisrun_terminal, O_RDWR | O_NOCTTY);

(and printf logging)

I.e., tracking the string's size doesn't help anything.

Thanks,
Pedro Alves
Pedro Alves June 25, 2020, 5:32 p.m. | #3
On 6/25/20 4:55 PM, Simon Marchi via Gdb-patches wrote:
> This changes the inferior::terminal field to be a unique pointer, so its

> deallocation is automatically managed.

> 

> gdb/ChangeLog:

> 

> 	* inferior.h (struct inferior) <terminal>: Change type to

> 	gdb::unique_xmalloc_ptr<char>.

> 	* inferior.c (inferior::~inferior): Don't free inf->terminal.

> 	* infcmd.c (set_inferior_io_terminal): Don't free terminal

> 	field, adjust to unique pointer.

> 	(get_inferior_io_terminal): Adjust to unique pointer.

> 


LGTM.

This reminds me of this patch:

 https://github.com/palves/gdb/commit/f240d57e3e659d13a01d9dda79120e2ce6bc7e74

IMO, the code is more readable with that.  WDYT?

Thanks,
Pedro Alves
Jose E. Marchesi via Gdb-patches June 25, 2020, 5:59 p.m. | #4
On 6/25/20 2:28 PM, Pedro Alves wrote:
> On 6/25/20 6:18 PM, Christian Biesinger via Gdb-patches wrote:

>> On Thu, Jun 25, 2020 at 10:55 AM Simon Marchi via Gdb-patches

>> <gdb-patches@sourceware.org> wrote:

>>> -    current_inferior ()->terminal = xstrdup (terminal_name);

>>> +    current_inferior ()->terminal.reset (xstrdup (terminal_name));

>>

>> Perhaps it really should be a std::string?

> 

> I think there's no good reason for that.  It's a string that doesn't

> ever need to grow/shrink.  And then its use is to pass down to

> a syscall that accepts C-style null-terminated strings:


I tend to agree with Christian. I suppose one positive outcome is to 
make the code less cumbersome. It's not like xstrdup is great to read 
anyway.

Are we worried about performance implications of passing std::string's 
around like this?
Pedro Alves June 25, 2020, 6:09 p.m. | #5
On 6/25/20 6:59 PM, Luis Machado wrote:
> On 6/25/20 2:28 PM, Pedro Alves wrote:

>> On 6/25/20 6:18 PM, Christian Biesinger via Gdb-patches wrote:

>>> On Thu, Jun 25, 2020 at 10:55 AM Simon Marchi via Gdb-patches

>>> <gdb-patches@sourceware.org> wrote:

>>>> -    current_inferior ()->terminal = xstrdup (terminal_name);

>>>> +    current_inferior ()->terminal.reset (xstrdup (terminal_name));

>>>

>>> Perhaps it really should be a std::string?

>>

>> I think there's no good reason for that.  It's a string that doesn't

>> ever need to grow/shrink.  And then its use is to pass down to

>> a syscall that accepts C-style null-terminated strings:

> 

> I tend to agree with Christian. I suppose one positive outcome is to make the code less cumbersome. It's not like xstrdup is great to read anyway.

> 

> Are we worried about performance implications of passing std::string's around like this?


It's more like -- why store a fat 32 bytes object in the inferior when
a pointer does just as well.  There's no code that would benefit from
making this a string, like, there's no strlen call that that would avoid.
The code that wants to consume this wants a pointer.  A method-based
interface like I pointed at in that github commit exposes a pointer.
So it would just be implementation detail.

Thanks,
Pedro Alves
Simon Marchi June 25, 2020, 6:39 p.m. | #6
On 2020-06-25 2:09 p.m., Pedro Alves wrote:
> On 6/25/20 6:59 PM, Luis Machado wrote:

>> On 6/25/20 2:28 PM, Pedro Alves wrote:

>>> On 6/25/20 6:18 PM, Christian Biesinger via Gdb-patches wrote:

>>>> On Thu, Jun 25, 2020 at 10:55 AM Simon Marchi via Gdb-patches

>>>> <gdb-patches@sourceware.org> wrote:

>>>>> -    current_inferior ()->terminal = xstrdup (terminal_name);

>>>>> +    current_inferior ()->terminal.reset (xstrdup (terminal_name));

>>>>

>>>> Perhaps it really should be a std::string?

>>>

>>> I think there's no good reason for that.  It's a string that doesn't

>>> ever need to grow/shrink.  And then its use is to pass down to

>>> a syscall that accepts C-style null-terminated strings:

>>

>> I tend to agree with Christian. I suppose one positive outcome is to make the code less cumbersome. It's not like xstrdup is great to read anyway.

>>

>> Are we worried about performance implications of passing std::string's around like this?

> 

> It's more like -- why store a fat 32 bytes object in the inferior when

> a pointer does just as well.  There's no code that would benefit from

> making this a string, like, there's no strlen call that that would avoid.

> The code that wants to consume this wants a pointer.  A method-based

> interface like I pointed at in that github commit exposes a pointer.

> So it would just be implementation detail.

> 

> Thanks,

> Pedro Alves

> 


Heh, that's more noise than I expected for this patch :)

I tend to agree with Pedro on this.  I initially tried to replace it with
a gdb::optional<std::string>, then making get_inferior_io_terminal return a
reference to that... in the end it required changing a bunch of code in a way
that didn't really make it better.  The intent of this patch is just to get rid
of the manual free-ing, which is a concrete improvement.

Simon
Simon Marchi June 25, 2020, 6:42 p.m. | #7
On 2020-06-25 1:32 p.m., Pedro Alves wrote:
> On 6/25/20 4:55 PM, Simon Marchi via Gdb-patches wrote:

>> This changes the inferior::terminal field to be a unique pointer, so its

>> deallocation is automatically managed.

>>

>> gdb/ChangeLog:

>>

>> 	* inferior.h (struct inferior) <terminal>: Change type to

>> 	gdb::unique_xmalloc_ptr<char>.

>> 	* inferior.c (inferior::~inferior): Don't free inf->terminal.

>> 	* infcmd.c (set_inferior_io_terminal): Don't free terminal

>> 	field, adjust to unique pointer.

>> 	(get_inferior_io_terminal): Adjust to unique pointer.

>>

> 

> LGTM.

> 

> This reminds me of this patch:

> 

>  https://github.com/palves/gdb/commit/f240d57e3e659d13a01d9dda79120e2ce6bc7e74

> 

> IMO, the code is more readable with that.  WDYT?


I thought about doing something similar... but I limited myself to the original
intent of removing the manual free-ing.  Otherwise, it never ends, there is just
an infinity of such refactors we can do.

But I think your patch looks good.

I'll push mine now.

Simon
Tom Tromey June 25, 2020, 8:15 p.m. | #8
>>>>> "Simon" == Simon Marchi via Gdb-patches <gdb-patches@sourceware.org> writes:


Simon> -    current_inferior ()->terminal = xstrdup (terminal_name);
Simon> +    current_inferior ()->terminal.reset (xstrdup (terminal_name));

You can use make_unique_xstrdup and avoid the call to reset, in favor of
move assignment.

Tom
Simon Marchi June 25, 2020, 9:07 p.m. | #9
On 2020-06-25 4:15 p.m., Tom Tromey wrote:
>>>>>> "Simon" == Simon Marchi via Gdb-patches <gdb-patches@sourceware.org> writes:

> 

> Simon> -    current_inferior ()->terminal = xstrdup (terminal_name);

> Simon> +    current_inferior ()->terminal.reset (xstrdup (terminal_name));

> 

> You can use make_unique_xstrdup and avoid the call to reset, in favor of

> move assignment.

> 

> Tom

> 


Good catch.  I pushed the following patch that does it.


From 58373b80f3d06740fccefe735fbb390791092027 Mon Sep 17 00:00:00 2001
From: Simon Marchi <simon.marchi@efficios.com>

Date: Thu, 25 Jun 2020 17:06:18 -0400
Subject: [PATCH] gdb: use make_unique_xstrdup in set_inferior_io_terminal

gdb/ChangeLog:

	* infcmd.c (set_inferior_io_terminal): Use make_unique_xstrdup.

Change-Id: I38b6e753f58947531fe4a293d574bc27ec128f47
---
 gdb/ChangeLog | 4 ++++
 gdb/infcmd.c  | 2 +-
 2 files changed, 5 insertions(+), 1 deletion(-)

diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index d9b6a49bd105..ddb5c47d1e75 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,3 +1,7 @@
+2020-06-25  Simon Marchi  <simon.marchi@efficios.com>
+
+	* infcmd.c (set_inferior_io_terminal): Use make_unique_xstrdup.
+
 2020-06-25  Simon Marchi  <simon.marchi@efficios.com>

 	* inferior.h (struct inferior) <terminal>: Change type to
diff --git a/gdb/infcmd.c b/gdb/infcmd.c
index 48d6a91c0c24..17f7b9abeac3 100644
--- a/gdb/infcmd.c
+++ b/gdb/infcmd.c
@@ -109,7 +109,7 @@ void
 set_inferior_io_terminal (const char *terminal_name)
 {
   if (terminal_name != NULL && *terminal_name != '\0')
-    current_inferior ()->terminal.reset (xstrdup (terminal_name));
+    current_inferior ()->terminal = make_unique_xstrdup (terminal_name);
   else
     current_inferior ()->terminal = NULL;
 }
-- 
2.26.2
Pedro Alves June 25, 2020, 9:17 p.m. | #10
On 6/25/20 7:42 PM, Simon Marchi wrote:
> On 2020-06-25 1:32 p.m., Pedro Alves wrote:


>> This reminds me of this patch:

>>

>>  https://github.com/palves/gdb/commit/f240d57e3e659d13a01d9dda79120e2ce6bc7e74

>>

>> IMO, the code is more readable with that.  WDYT?

> 

> I thought about doing something similar... but I limited myself to the original

> intent of removing the manual free-ing.  Otherwise, it never ends, there is just

> an infinity of such refactors we can do.


So right you are.  That patch is part of the series to make GDB always put
the inferior on a separate session.  Someday I'll find time to finish it...

> But I think your patch looks good.

> 


I've sent it as a proper patch now:

  https://sourceware.org/pipermail/gdb-patches/2020-June/169896.html

Thanks,
Pedro Alves
Jose E. Marchesi via Gdb-patches June 25, 2020, 9:20 p.m. | #11
On Thu, Jun 25, 2020 at 1:09 PM Pedro Alves <pedro@palves.net> wrote:
>

> On 6/25/20 6:59 PM, Luis Machado wrote:

> > On 6/25/20 2:28 PM, Pedro Alves wrote:

> >> On 6/25/20 6:18 PM, Christian Biesinger via Gdb-patches wrote:

> >>> On Thu, Jun 25, 2020 at 10:55 AM Simon Marchi via Gdb-patches

> >>> <gdb-patches@sourceware.org> wrote:

> >>>> -    current_inferior ()->terminal = xstrdup (terminal_name);

> >>>> +    current_inferior ()->terminal.reset (xstrdup (terminal_name));

> >>>

> >>> Perhaps it really should be a std::string?

> >>

> >> I think there's no good reason for that.  It's a string that doesn't

> >> ever need to grow/shrink.  And then its use is to pass down to

> >> a syscall that accepts C-style null-terminated strings:

> >

> > I tend to agree with Christian. I suppose one positive outcome is to make the code less cumbersome. It's not like xstrdup is great to read anyway.

> >

> > Are we worried about performance implications of passing std::string's around like this?

>

> It's more like -- why store a fat 32 bytes object in the inferior when

> a pointer does just as well.  There's no code that would benefit from

> making this a string, like, there's no strlen call that that would avoid.

> The code that wants to consume this wants a pointer.  A method-based

> interface like I pointed at in that github commit exposes a pointer.

> So it would just be implementation detail.


Well, if the string is usually short, std::string can avoid a memory
allocation...

But I guess it comes down to a philosophical question on whether
xstrdup/unique_xmalloc_ptr or std::string is more elegant.

Christian
Tom Tromey June 25, 2020, 9:45 p.m. | #12
Simon> Good catch.  I pushed the following patch that does it.

Thanks.  IMO it's good to avoid 'reset' when possible.

Tom

Patch

diff --git a/gdb/infcmd.c b/gdb/infcmd.c
index 42b050d3c4e..48d6a91c0c2 100644
--- a/gdb/infcmd.c
+++ b/gdb/infcmd.c
@@ -108,10 +108,8 @@  int stopped_by_random_signal;
 void 
 set_inferior_io_terminal (const char *terminal_name)
 {
-  xfree (current_inferior ()->terminal);
-
   if (terminal_name != NULL && *terminal_name != '\0')
-    current_inferior ()->terminal = xstrdup (terminal_name);
+    current_inferior ()->terminal.reset (xstrdup (terminal_name));
   else
     current_inferior ()->terminal = NULL;
 }
@@ -119,7 +117,7 @@  set_inferior_io_terminal (const char *terminal_name)
 const char *
 get_inferior_io_terminal (void)
 {
-  return current_inferior ()->terminal;
+  return current_inferior ()->terminal.get ();
 }
 
 static void
diff --git a/gdb/inferior.c b/gdb/inferior.c
index 2f4ced0788d..d3bece029dd 100644
--- a/gdb/inferior.c
+++ b/gdb/inferior.c
@@ -79,7 +79,6 @@  inferior::~inferior ()
   discard_all_inferior_continuations (inf);
   inferior_free_data (inf);
   xfree (inf->args);
-  xfree (inf->terminal);
   target_desc_info_free (inf->tdesc_info);
 }
 
diff --git a/gdb/inferior.h b/gdb/inferior.h
index 95af474eede..5002b0b8b3d 100644
--- a/gdb/inferior.h
+++ b/gdb/inferior.h
@@ -52,6 +52,7 @@  struct thread_info;
 #include "symfile-add-flags.h"
 #include "gdbsupport/refcounted-object.h"
 #include "gdbsupport/forward-scope-exit.h"
+#include "gdbsupport/gdb_unique_ptr.h"
 
 #include "gdbsupport/common-inferior.h"
 #include "gdbthread.h"
@@ -456,7 +457,7 @@  class inferior : public refcounted_object
   gdb::unique_xmalloc_ptr<char> cwd;
 
   /* The name of terminal device to use for I/O.  */
-  char *terminal = NULL;
+  gdb::unique_xmalloc_ptr<char> terminal;
 
   /* The terminal state as set by the last target_terminal::terminal_*
      call.  */