* [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).