* [PATCH 0/3] Minor improvements to 'interp' @ 2022-06-20 13:46 Tom Tromey 2022-06-20 13:46 ` [PATCH 1/3] Use unique_xmalloc_ptr in interp Tom Tromey ` (2 more replies) 0 siblings, 3 replies; 6+ messages in thread From: Tom Tromey @ 2022-06-20 13:46 UTC (permalink / raw) To: gdb-patches This series removes some manual memory management from 'interp', and also makes some recompilations a bit faster by reducing dependencies on interps.h. Regression tested on x86-64 Fedora 34. Tom ^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH 1/3] Use unique_xmalloc_ptr in interp 2022-06-20 13:46 [PATCH 0/3] Minor improvements to 'interp' Tom Tromey @ 2022-06-20 13:46 ` Tom Tromey 2022-06-20 13:46 ` [PATCH 2/3] Move mi_interpreter to mi-interp.h Tom Tromey 2022-06-20 13:46 ` [PATCH 3/3] Use std::string for interpreter_p Tom Tromey 2 siblings, 0 replies; 6+ messages in thread From: Tom Tromey @ 2022-06-20 13:46 UTC (permalink / raw) To: gdb-patches; +Cc: Tom Tromey This changes interp::m_name to be a unique_xmalloc_ptr, removing some manual memory management. It also cleans up the initialization of the 'inited' member, and moves the 'private:' and 'public:' keywords to their proper spots. --- gdb/interps.c | 4 +--- gdb/interps.h | 10 +++++----- 2 files changed, 6 insertions(+), 8 deletions(-) diff --git a/gdb/interps.c b/gdb/interps.c index 44002ff2cb5..0c440e78685 100644 --- a/gdb/interps.c +++ b/gdb/interps.c @@ -79,14 +79,12 @@ static struct interp *interp_lookup_existing (struct ui *ui, const char *name); interp::interp (const char *name) - : m_name (xstrdup (name)) + : m_name (make_unique_xstrdup (name)) { - this->inited = false; } interp::~interp () { - xfree (m_name); } /* An interpreter factory. Maps an interpreter name to the factory diff --git a/gdb/interps.h b/gdb/interps.h index 330c1ba6615..e393b08c962 100644 --- a/gdb/interps.h +++ b/gdb/interps.h @@ -78,20 +78,20 @@ class interp const char *name () const { - return m_name; + return m_name.get (); } - /* This is the name in "-i=" and "set interpreter". */ private: - char *m_name; + /* This is the name in "-i=" and "set interpreter". */ + gdb::unique_xmalloc_ptr<char> m_name; +public: /* Interpreters are stored in a linked list, this is the next one... */ -public: struct interp *next; /* Has the init method been run? */ - bool inited; + bool inited = false; }; /* Look up the interpreter for NAME, creating one if none exists yet. -- 2.34.1 ^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH 2/3] Move mi_interpreter to mi-interp.h 2022-06-20 13:46 [PATCH 0/3] Minor improvements to 'interp' Tom Tromey 2022-06-20 13:46 ` [PATCH 1/3] Use unique_xmalloc_ptr in interp Tom Tromey @ 2022-06-20 13:46 ` Tom Tromey 2022-06-20 13:46 ` [PATCH 3/3] Use std::string for interpreter_p Tom Tromey 2 siblings, 0 replies; 6+ messages in thread From: Tom Tromey @ 2022-06-20 13:46 UTC (permalink / raw) To: gdb-patches; +Cc: Tom Tromey I noticed that touching interps.h caused a lot of recompilation. I tracked this down to mi-common.h including this file. This patch moves the MI interpreter to mi-interp.h, which cuts down on recompilation when modifying interps.h. --- gdb/mi/mi-common.h | 50 ---------------------------------------------- gdb/mi/mi-interp.h | 50 ++++++++++++++++++++++++++++++++++++++++++++++ gdb/mi/mi-main.c | 2 +- 3 files changed, 51 insertions(+), 51 deletions(-) diff --git a/gdb/mi/mi-common.h b/gdb/mi/mi-common.h index 0c254baf8b4..6970d41ae08 100644 --- a/gdb/mi/mi-common.h +++ b/gdb/mi/mi-common.h @@ -19,10 +19,6 @@ #ifndef MI_MI_COMMON_H #define MI_MI_COMMON_H -#include "interps.h" - -struct mi_console_file; - /* Represents the reason why GDB is sending an asynchronous command to the front end. NOTE: When modifing this, don't forget to update gdb.texinfo! */ @@ -52,50 +48,4 @@ enum async_reply_reason const char *async_reason_lookup (enum async_reply_reason reason); -/* An MI interpreter. */ - -class mi_interp final : public interp -{ -public: - mi_interp (const char *name) - : interp (name) - {} - - void init (bool top_level) override; - void resume () override; - void suspend () override; - gdb_exception exec (const char *command_str) override; - ui_out *interp_ui_out () override; - void set_logging (ui_file_up logfile, bool logging_redirect, - bool debug_redirect) override; - void pre_command_loop () override; - - /* MI's output channels */ - mi_console_file *out; - mi_console_file *err; - mi_console_file *log; - mi_console_file *targ; - mi_console_file *event_channel; - - /* Raw console output. */ - struct ui_file *raw_stdout; - - /* Raw logfile output. */ - struct ui_file *raw_stdlog; - - /* Save the original value of raw_stdout and raw_stdlog here when logging, and - the file which we need to delete, so we can restore correctly when - done. */ - struct ui_file *saved_raw_stdout; - struct ui_file *saved_raw_stdlog; - struct ui_file *saved_raw_file_to_delete; - - - /* MI's builder. */ - struct ui_out *mi_uiout; - - /* MI's CLI builder (wraps OUT). */ - struct ui_out *cli_uiout; -}; - #endif /* MI_MI_COMMON_H */ diff --git a/gdb/mi/mi-interp.h b/gdb/mi/mi-interp.h index 53369f2eea7..adf6eaffef4 100644 --- a/gdb/mi/mi-interp.h +++ b/gdb/mi/mi-interp.h @@ -20,6 +20,56 @@ #ifndef MI_MI_INTERP_H #define MI_MI_INTERP_H +#include "interps.h" + +struct mi_console_file; + +/* An MI interpreter. */ + +class mi_interp final : public interp +{ +public: + mi_interp (const char *name) + : interp (name) + {} + + void init (bool top_level) override; + void resume () override; + void suspend () override; + gdb_exception exec (const char *command_str) override; + ui_out *interp_ui_out () override; + void set_logging (ui_file_up logfile, bool logging_redirect, + bool debug_redirect) override; + void pre_command_loop () override; + + /* MI's output channels */ + mi_console_file *out; + mi_console_file *err; + mi_console_file *log; + mi_console_file *targ; + mi_console_file *event_channel; + + /* Raw console output. */ + struct ui_file *raw_stdout; + + /* Raw logfile output. */ + struct ui_file *raw_stdlog; + + /* Save the original value of raw_stdout and raw_stdlog here when logging, and + the file which we need to delete, so we can restore correctly when + done. */ + struct ui_file *saved_raw_stdout; + struct ui_file *saved_raw_stdlog; + struct ui_file *saved_raw_file_to_delete; + + + /* MI's builder. */ + struct ui_out *mi_uiout; + + /* MI's CLI builder (wraps OUT). */ + struct ui_out *cli_uiout; +}; + /* Output the shared object attributes to UIOUT. */ void mi_output_solib_attribs (ui_out *uiout, struct so_list *solib); diff --git a/gdb/mi/mi-main.c b/gdb/mi/mi-main.c index 18707bf62e7..68868e49e99 100644 --- a/gdb/mi/mi-main.c +++ b/gdb/mi/mi-main.c @@ -40,7 +40,7 @@ #include "regcache.h" #include "frame.h" #include "mi-main.h" -#include "mi-common.h" +#include "mi-interp.h" #include "language.h" #include "valprint.h" #include "osdata.h" -- 2.34.1 ^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH 3/3] Use std::string for interpreter_p 2022-06-20 13:46 [PATCH 0/3] Minor improvements to 'interp' Tom Tromey 2022-06-20 13:46 ` [PATCH 1/3] Use unique_xmalloc_ptr in interp Tom Tromey 2022-06-20 13:46 ` [PATCH 2/3] Move mi_interpreter to mi-interp.h Tom Tromey @ 2022-06-20 13:46 ` Tom Tromey 2022-06-21 12:11 ` Andrew Burgess 2 siblings, 1 reply; 6+ messages in thread From: Tom Tromey @ 2022-06-20 13:46 UTC (permalink / raw) To: gdb-patches; +Cc: Tom Tromey The global interpreter_p is a manually-managed 'char *'. This patch changes it to be a std::string instead, and removes some erroneous comments. --- gdb/interps.c | 11 ++--------- gdb/main.c | 24 +++++++++--------------- gdb/main.h | 2 +- gdb/tui/tui-interp.c | 9 +++------ 4 files changed, 15 insertions(+), 31 deletions(-) diff --git a/gdb/interps.c b/gdb/interps.c index 0c440e78685..3a9c590b8c8 100644 --- a/gdb/interps.c +++ b/gdb/interps.c @@ -168,15 +168,8 @@ interp_set (struct interp *interp, bool top_level) if (top_level) ui_interp->top_level_interpreter = interp; - /* We use interpreter_p for the "set interpreter" variable, so we need - to make sure we have a malloc'ed copy for the set command to free. */ - if (interpreter_p != NULL - && strcmp (interp->name (), interpreter_p) != 0) - { - xfree (interpreter_p); - - interpreter_p = xstrdup (interp->name ()); - } + if (interpreter_p != interp->name ()) + interpreter_p = interp->name (); /* Run the init proc. */ if (!interp->inited) diff --git a/gdb/main.c b/gdb/main.c index ec2b7b01752..8c97987956b 100644 --- a/gdb/main.c +++ b/gdb/main.c @@ -56,10 +56,8 @@ #include "observable.h" #include "serial.h" -/* The selected interpreter. This will be used as a set command - variable, so it should always be malloc'ed - since - do_setshow_command will free it. */ -char *interpreter_p; +/* The selected interpreter. */ +std::string interpreter_p; /* System root path, used to find libraries etc. */ std::string gdb_sysroot; @@ -729,7 +727,7 @@ captured_main_1 (struct captured_main_args *context) this captured main, or one specified by the user at start up, or the console. Initialize the interpreter to the one requested by the application. */ - interpreter_p = xstrdup (context->interpreter_p); + interpreter_p = context->interpreter_p; /* Parse arguments and options. */ { @@ -866,8 +864,7 @@ captured_main_1 (struct captured_main_args *context) case OPT_TUI: /* --tui is equivalent to -i=tui. */ #ifdef TUI - xfree (interpreter_p); - interpreter_p = xstrdup (INTERP_TUI); + interpreter_p = INTERP_TUI; #else error (_("%s: TUI mode is not supported"), gdb_program_name); #endif @@ -877,14 +874,12 @@ captured_main_1 (struct captured_main_args *context) actually useful, and if it is, what it should do. */ #ifdef GDBTK /* --windows is equivalent to -i=insight. */ - xfree (interpreter_p); - interpreter_p = xstrdup (INTERP_INSIGHT); + interpreter_p = INTERP_INSIGHT; #endif break; case OPT_NOWINDOWS: /* -nw is equivalent to -i=console. */ - xfree (interpreter_p); - interpreter_p = xstrdup (INTERP_CONSOLE); + interpreter_p = INTERP_CONSOLE; break; case 'f': annotation_level = 1; @@ -950,8 +945,7 @@ captured_main_1 (struct captured_main_args *context) } #endif /* GDBTK */ case 'i': - xfree (interpreter_p); - interpreter_p = xstrdup (optarg); + interpreter_p = optarg; break; case 'd': dirarg.push_back (optarg); @@ -1133,7 +1127,7 @@ captured_main_1 (struct captured_main_args *context) GDB retain the old MI1 interpreter startup behavior. Output the copyright message before the interpreter is installed. That way it isn't encapsulated in MI output. */ - if (!quiet && strcmp (interpreter_p, INTERP_MI1) == 0) + if (!quiet && interpreter_p == INTERP_MI1) { /* Print all the junk at the top, with trailing "..." if we are about to read a symbol file (possibly slowly). */ @@ -1147,7 +1141,7 @@ captured_main_1 (struct captured_main_args *context) /* Install the default UI. All the interpreters should have had a look at things by now. Initialize the default interpreter. */ - set_top_level_interpreter (interpreter_p); + set_top_level_interpreter (interpreter_p.c_str ()); /* FIXME: cagney/2003-02-03: The big hack (part 2 of 2) that lets GDB retain the old MI1 interpreter startup behavior. Output the diff --git a/gdb/main.h b/gdb/main.h index 60b3569438e..7cdd93f3420 100644 --- a/gdb/main.h +++ b/gdb/main.h @@ -36,7 +36,7 @@ extern int batch_silent; extern int batch_flag; /* * The name of the interpreter if specified on the command line. */ -extern char *interpreter_p; +extern std::string interpreter_p; /* From mingw-hdep.c, used by main.c. */ diff --git a/gdb/tui/tui-interp.c b/gdb/tui/tui-interp.c index e867441afb0..b4faede8ce6 100644 --- a/gdb/tui/tui-interp.c +++ b/gdb/tui/tui-interp.c @@ -343,14 +343,11 @@ _initialize_tui_interp () { interp_factory_register (INTERP_TUI, tui_interp_factory); - if (interpreter_p && strcmp (interpreter_p, INTERP_TUI) == 0) + if (interpreter_p == INTERP_TUI) tui_start_enabled = true; - if (interpreter_p && strcmp (interpreter_p, INTERP_CONSOLE) == 0) - { - xfree (interpreter_p); - interpreter_p = xstrdup (INTERP_TUI); - } + if (interpreter_p == INTERP_CONSOLE) + interpreter_p = INTERP_TUI; /* If changing this, remember to update cli-interp.c as well. */ gdb::observers::normal_stop.attach (tui_on_normal_stop, "tui-interp"); -- 2.34.1 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 3/3] Use std::string for interpreter_p 2022-06-20 13:46 ` [PATCH 3/3] Use std::string for interpreter_p Tom Tromey @ 2022-06-21 12:11 ` Andrew Burgess 2022-06-22 19:50 ` Tom Tromey 0 siblings, 1 reply; 6+ messages in thread From: Andrew Burgess @ 2022-06-21 12:11 UTC (permalink / raw) To: Tom Tromey via Gdb-patches, gdb-patches; +Cc: Tom Tromey Tom Tromey via Gdb-patches <gdb-patches@sourceware.org> writes: > The global interpreter_p is a manually-managed 'char *'. This patch > changes it to be a std::string instead, and removes some erroneous > comments. I took a look through this series, and it all looks good to me. I don't like the name interpreter_p - the _p says "predicate" to me, but I that's not your fault, so can be fixed some other day. Thanks, Andrew > --- > gdb/interps.c | 11 ++--------- > gdb/main.c | 24 +++++++++--------------- > gdb/main.h | 2 +- > gdb/tui/tui-interp.c | 9 +++------ > 4 files changed, 15 insertions(+), 31 deletions(-) > > diff --git a/gdb/interps.c b/gdb/interps.c > index 0c440e78685..3a9c590b8c8 100644 > --- a/gdb/interps.c > +++ b/gdb/interps.c > @@ -168,15 +168,8 @@ interp_set (struct interp *interp, bool top_level) > if (top_level) > ui_interp->top_level_interpreter = interp; > > - /* We use interpreter_p for the "set interpreter" variable, so we need > - to make sure we have a malloc'ed copy for the set command to free. */ > - if (interpreter_p != NULL > - && strcmp (interp->name (), interpreter_p) != 0) > - { > - xfree (interpreter_p); > - > - interpreter_p = xstrdup (interp->name ()); > - } > + if (interpreter_p != interp->name ()) > + interpreter_p = interp->name (); > > /* Run the init proc. */ > if (!interp->inited) > diff --git a/gdb/main.c b/gdb/main.c > index ec2b7b01752..8c97987956b 100644 > --- a/gdb/main.c > +++ b/gdb/main.c > @@ -56,10 +56,8 @@ > #include "observable.h" > #include "serial.h" > > -/* The selected interpreter. This will be used as a set command > - variable, so it should always be malloc'ed - since > - do_setshow_command will free it. */ > -char *interpreter_p; > +/* The selected interpreter. */ > +std::string interpreter_p; > > /* System root path, used to find libraries etc. */ > std::string gdb_sysroot; > @@ -729,7 +727,7 @@ captured_main_1 (struct captured_main_args *context) > this captured main, or one specified by the user at start up, or > the console. Initialize the interpreter to the one requested by > the application. */ > - interpreter_p = xstrdup (context->interpreter_p); > + interpreter_p = context->interpreter_p; > > /* Parse arguments and options. */ > { > @@ -866,8 +864,7 @@ captured_main_1 (struct captured_main_args *context) > case OPT_TUI: > /* --tui is equivalent to -i=tui. */ > #ifdef TUI > - xfree (interpreter_p); > - interpreter_p = xstrdup (INTERP_TUI); > + interpreter_p = INTERP_TUI; > #else > error (_("%s: TUI mode is not supported"), gdb_program_name); > #endif > @@ -877,14 +874,12 @@ captured_main_1 (struct captured_main_args *context) > actually useful, and if it is, what it should do. */ > #ifdef GDBTK > /* --windows is equivalent to -i=insight. */ > - xfree (interpreter_p); > - interpreter_p = xstrdup (INTERP_INSIGHT); > + interpreter_p = INTERP_INSIGHT; > #endif > break; > case OPT_NOWINDOWS: > /* -nw is equivalent to -i=console. */ > - xfree (interpreter_p); > - interpreter_p = xstrdup (INTERP_CONSOLE); > + interpreter_p = INTERP_CONSOLE; > break; > case 'f': > annotation_level = 1; > @@ -950,8 +945,7 @@ captured_main_1 (struct captured_main_args *context) > } > #endif /* GDBTK */ > case 'i': > - xfree (interpreter_p); > - interpreter_p = xstrdup (optarg); > + interpreter_p = optarg; > break; > case 'd': > dirarg.push_back (optarg); > @@ -1133,7 +1127,7 @@ captured_main_1 (struct captured_main_args *context) > GDB retain the old MI1 interpreter startup behavior. Output the > copyright message before the interpreter is installed. That way > it isn't encapsulated in MI output. */ > - if (!quiet && strcmp (interpreter_p, INTERP_MI1) == 0) > + if (!quiet && interpreter_p == INTERP_MI1) > { > /* Print all the junk at the top, with trailing "..." if we are > about to read a symbol file (possibly slowly). */ > @@ -1147,7 +1141,7 @@ captured_main_1 (struct captured_main_args *context) > > /* Install the default UI. All the interpreters should have had a > look at things by now. Initialize the default interpreter. */ > - set_top_level_interpreter (interpreter_p); > + set_top_level_interpreter (interpreter_p.c_str ()); > > /* FIXME: cagney/2003-02-03: The big hack (part 2 of 2) that lets > GDB retain the old MI1 interpreter startup behavior. Output the > diff --git a/gdb/main.h b/gdb/main.h > index 60b3569438e..7cdd93f3420 100644 > --- a/gdb/main.h > +++ b/gdb/main.h > @@ -36,7 +36,7 @@ extern int batch_silent; > extern int batch_flag; > > /* * The name of the interpreter if specified on the command line. */ > -extern char *interpreter_p; > +extern std::string interpreter_p; > > /* From mingw-hdep.c, used by main.c. */ > > diff --git a/gdb/tui/tui-interp.c b/gdb/tui/tui-interp.c > index e867441afb0..b4faede8ce6 100644 > --- a/gdb/tui/tui-interp.c > +++ b/gdb/tui/tui-interp.c > @@ -343,14 +343,11 @@ _initialize_tui_interp () > { > interp_factory_register (INTERP_TUI, tui_interp_factory); > > - if (interpreter_p && strcmp (interpreter_p, INTERP_TUI) == 0) > + if (interpreter_p == INTERP_TUI) > tui_start_enabled = true; > > - if (interpreter_p && strcmp (interpreter_p, INTERP_CONSOLE) == 0) > - { > - xfree (interpreter_p); > - interpreter_p = xstrdup (INTERP_TUI); > - } > + if (interpreter_p == INTERP_CONSOLE) > + interpreter_p = INTERP_TUI; > > /* If changing this, remember to update cli-interp.c as well. */ > gdb::observers::normal_stop.attach (tui_on_normal_stop, "tui-interp"); > -- > 2.34.1 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 3/3] Use std::string for interpreter_p 2022-06-21 12:11 ` Andrew Burgess @ 2022-06-22 19:50 ` Tom Tromey 0 siblings, 0 replies; 6+ messages in thread From: Tom Tromey @ 2022-06-22 19:50 UTC (permalink / raw) To: Andrew Burgess; +Cc: Tom Tromey via Gdb-patches, Tom Tromey >>>>> "Andrew" == Andrew Burgess <aburgess@redhat.com> writes: Andrew> Tom Tromey via Gdb-patches <gdb-patches@sourceware.org> writes: >> The global interpreter_p is a manually-managed 'char *'. This patch >> changes it to be a std::string instead, and removes some erroneous >> comments. Andrew> I took a look through this series, and it all looks good to me. Thanks. Andrew> I don't like the name interpreter_p - the _p says "predicate" to me, but Andrew> I that's not your fault, so can be fixed some other day. Yeah. I think maybe it means "pointer"? Not sure. I looked at removing this entirely, and it's almost possible except for the hacks in tui-interp.c: interp_factory_register (INTERP_TUI, tui_interp_factory); if (interpreter_p == INTERP_TUI) tui_start_enabled = true; if (interpreter_p == INTERP_CONSOLE) interpreter_p = INTERP_TUI; I don't really understand why the TUI is considered a distinct interpreter at all. That seems weird to me. For one thing, I found it doesn't work properly with "new-ui" (I filed a bug about this). Tom ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2022-06-22 19:50 UTC | newest] Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2022-06-20 13:46 [PATCH 0/3] Minor improvements to 'interp' Tom Tromey 2022-06-20 13:46 ` [PATCH 1/3] Use unique_xmalloc_ptr in interp Tom Tromey 2022-06-20 13:46 ` [PATCH 2/3] Move mi_interpreter to mi-interp.h Tom Tromey 2022-06-20 13:46 ` [PATCH 3/3] Use std::string for interpreter_p Tom Tromey 2022-06-21 12:11 ` Andrew Burgess 2022-06-22 19:50 ` Tom Tromey
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).