Re: [PATCH v3 06/34] Introduce interpreter factories

Simon Marchi
2016-05-18T19:20:01Z

On 16-05-06 08:34 AM, Pedro Alves wrote:
> If every UI instance has its own set of interpreters, then the current
> scheme of creating the interpreters at GDB initialization time no
> longer works. We need to create them whenever a new UI instance is
> created.
>
> The scheme implemented here has each interpreter register an factory
> callback that when called creates a new instance of a specific type.
> Then, when some code in gdb looks up an interpreter (always by name),
> if there's none yet, the factory method is called to construct one.
>
> gdb/ChangeLog:
> yyyy-mm-dd Pedro Alves <palves@redhat.com>
>
> * cli/cli-interp.c (cli_uiout): Delete, moved into ...
> (struct cli_interp): ... this new structure.
> (cli_on_normal_stop, cli_on_signal_received)
> (cli_on_end_stepping_range, cli_on_signal_exited, cli_on_exited)
> (cli_on_no_history): Use interp_ui_out.
> (cli_interpreter_init): If top level, set the cli_interp global.
> (cli_interpreter_init): Return the interp's data instead of NULL.
> (cli_interpreter_resume, cli_interpreter_exec, cli_ui_out): Adjust
> to cli_uiout being in the interpreter's data.
> (cli_interp_procs): New, factored out from _initialize_cli_interp.
> (cli_interp_factory): New function.
> (_initialize_cli_interp): Call interp_factory_register.
> * interps.c (interp_new): Add parameter 'data'. Store it.
> (struct interp_factory): New function.
> (interp_factory_p): New typedef. Define a VEC_P.
> (interpreter_factories): New global.
> (interp_factory_register): New function.
> (interp_add): Use interp_lookup_existing.
> (interp_lookup): Rename to ...
> (interp_lookup_existing): ... this. Don't check for NULL or empty
> name here.
> (interp_lookup): Reimplement.
> (interpreter_completer): Complete on registered interpreter
> factories instead of interpreters.
> * interps.h (interp_factory_func): New typedef.
> (interp_factory_register): Declare.
> (interp_new): Adjust.
> (interp_lookup): Declare.
> * mi/mi-interp.c (mi_interp_procs): New, factored out from
> _initialize_mi_interp.
> (mi_interp_factory): New function.
> * tui/tui-interp.c (tui_init): If top level, set the tui_interp
> global.
> (tui_interp_procs): New.
> (tui_interp_factory): New function.
> (_initialize_tui_interp): Call interp_factory_register.
> ---
> gdb/cli/cli-interp.c | 87 ++++++++++++++++++++++++++----------------
> gdb/interps.c | 105 +++++++++++++++++++++++++++++++++++++++++++--------
> gdb/interps.h | 21 ++++++++++-
> gdb/mi/mi-interp.c | 40 ++++++++++++--------
> gdb/tui/tui-interp.c | 38 ++++++++++++-------
> 5 files changed, 214 insertions(+), 77 deletions(-)
>
> diff --git a/gdb/cli/cli-interp.c b/gdb/cli/cli-interp.c
> index dfbd808..ac12a4c 100644
> --- a/gdb/cli/cli-interp.c
> +++ b/gdb/cli/cli-interp.c
> @@ -26,9 +26,14 @@
> #include "infrun.h"
> #include "observer.h"
>
> -/* These are the ui_out and the interpreter for the console
> - interpreter. */
> -struct ui_out *cli_uiout;
> +/* The console interpreter. */
> +struct cli_interp
> +{
> + /* The ui_out for the console interpreter. */
> + struct ui_out *cli_uiout;
> +};
> +
> +/* The interpreter for the console interpreter. */
> static struct interp *cli_interp;
>
> /* Longjmp-safe wrapper for "execute_command". */
> @@ -48,7 +53,7 @@ cli_on_normal_stop (struct bpstats *bs, int print_frame)
> if (!interp_quiet_p (cli_interp))
> {
> if (print_frame)
> - print_stop_event (cli_uiout);
> + print_stop_event (interp_ui_out (cli_interp));
> }
> }
>
> @@ -58,7 +63,7 @@ static void
> cli_on_signal_received (enum gdb_signal siggnal)
> {
> if (!interp_quiet_p (cli_interp))
> - print_signal_received_reason (cli_uiout, siggnal);
> + print_signal_received_reason (interp_ui_out (cli_interp), siggnal);
> }
>
> /* Observer for the end_stepping_range notification. */
> @@ -67,7 +72,7 @@ static void
> cli_on_end_stepping_range (void)
> {
> if (!interp_quiet_p (cli_interp))
> - print_end_stepping_range_reason (cli_uiout);
> + print_end_stepping_range_reason (interp_ui_out (cli_interp));
> }
>
> /* Observer for the signalled notification. */
> @@ -76,7 +81,7 @@ static void
> cli_on_signal_exited (enum gdb_signal siggnal)
> {
> if (!interp_quiet_p (cli_interp))
> - print_signal_exited_reason (cli_uiout, siggnal);
> + print_signal_exited_reason (interp_ui_out (cli_interp), siggnal);
> }
>
> /* Observer for the exited notification. */
> @@ -85,7 +90,7 @@ static void
> cli_on_exited (int exitstatus)
> {
> if (!interp_quiet_p (cli_interp))
> - print_exited_reason (cli_uiout, exitstatus);
> + print_exited_reason (interp_ui_out (cli_interp), exitstatus);
> }
>
> /* Observer for the no_history notification. */
> @@ -94,7 +99,7 @@ static void
> cli_on_no_history (void)
> {
> if (!interp_quiet_p (cli_interp))
> - print_no_history_reason (cli_uiout);
> + print_no_history_reason (interp_ui_out (cli_interp));
> }
>
> /* Observer for the sync_execution_done notification. */
> @@ -120,6 +125,9 @@ cli_on_command_error (void)
> static void *
> cli_interpreter_init (struct interp *self, int top_level)
> {
> + if (top_level)
> + cli_interp = self;
> +
> /* If changing this, remember to update tui-interp.c as well. */
> observer_attach_normal_stop (cli_on_normal_stop);
> observer_attach_end_stepping_range (cli_on_end_stepping_range);
> @@ -130,12 +138,13 @@ cli_interpreter_init (struct interp *self, int top_level)
> observer_attach_sync_execution_done (cli_on_sync_execution_done);
> observer_attach_command_error (cli_on_command_error);
>
> - return NULL;
> + return interp_data (self);
> }
>
> static int
> cli_interpreter_resume (void *data)
> {
> + struct cli_interp *cli = (struct cli_interp *) data;
> struct ui_file *stream;
>
> /*sync_execution = 1; */
> @@ -144,17 +153,17 @@ cli_interpreter_resume (void *data)
> previously writing to gdb_stdout, then set it to the new
> gdb_stdout afterwards. */
>
> - stream = cli_out_set_stream (cli_uiout, gdb_stdout);
> + stream = cli_out_set_stream (cli->cli_uiout, gdb_stdout);
> if (stream != gdb_stdout)
> {
> - cli_out_set_stream (cli_uiout, stream);
> + cli_out_set_stream (cli->cli_uiout, stream);
> stream = NULL;
> }
>
> gdb_setup_readline ();
>
> if (stream != NULL)
> - cli_out_set_stream (cli_uiout, gdb_stdout);
> + cli_out_set_stream (cli->cli_uiout, gdb_stdout);
>
> return 1;
> }
> @@ -169,6 +178,7 @@ cli_interpreter_suspend (void *data)
> static struct gdb_exception
> cli_interpreter_exec (void *data, const char *command_str)
> {
> + struct cli_interp *cli = (struct cli_interp *) data;
> struct ui_file *old_stream;
> struct gdb_exception result;
>
> @@ -184,9 +194,9 @@ cli_interpreter_exec (void *data, const char *command_str)
>
> It is important that it gets reset everytime, since the user
> could set gdb to use a different interpreter. */
> - old_stream = cli_out_set_stream (cli_uiout, gdb_stdout);
> - result = safe_execute_command (cli_uiout, str, 1);
> - cli_out_set_stream (cli_uiout, old_stream);
> + old_stream = cli_out_set_stream (cli->cli_uiout, gdb_stdout);
> + result = safe_execute_command (cli->cli_uiout, str, 1);
> + cli_out_set_stream (cli->cli_uiout, old_stream);
> return result;
> }
>
> @@ -222,7 +232,34 @@ safe_execute_command (struct ui_out *command_uiout, char *command, int from_tty)
> static struct ui_out *
> cli_ui_out (struct interp *self)
> {
> - return cli_uiout;
> + struct cli_interp *cli = (struct cli_interp *) interp_data (self);
> +
> + return cli->cli_uiout;
> +}
> +
> +/* The CLI interpreter's vtable. */
> +
> +static const struct interp_procs cli_interp_procs = {
> + cli_interpreter_init, /* init_proc */
> + cli_interpreter_resume, /* resume_proc */
> + cli_interpreter_suspend, /* suspend_proc */
> + cli_interpreter_exec, /* exec_proc */
> + cli_ui_out, /* ui_out_proc */
> + NULL, /* set_logging_proc */
> + cli_command_loop /* command_loop_proc */
> +};
> +
> +/* Factory for CLI interpreters. */
> +
> +static struct interp *
> +cli_interp_factory (const char *name, struct ui *ui)
> +{
> + struct cli_interp *cli = XNEW (struct cli_interp);
> +
> + /* Create a default uiout builder for the CLI. */
> + cli->cli_uiout = cli_out_new (gdb_stdout);
> +
> + return interp_new (name, &cli_interp_procs, cli);
> }
>
> /* Standard gdb initialization hook. */
> @@ -231,19 +268,5 @@ extern initialize_file_ftype _initialize_cli_interp; /* -Wmissing-prototypes */
> void
> _initialize_cli_interp (void)
> {
> - static const struct interp_procs procs = {
> - cli_interpreter_init, /* init_proc */
> - cli_interpreter_resume, /* resume_proc */
> - cli_interpreter_suspend, /* suspend_proc */
> - cli_interpreter_exec, /* exec_proc */
> - cli_ui_out, /* ui_out_proc */
> - NULL, /* set_logging_proc */
> - cli_command_loop /* command_loop_proc */
> - };
> -
> - /* Create a default uiout builder for the CLI. */
> - cli_uiout = cli_out_new (gdb_stdout);
> - cli_interp = interp_new (INTERP_CONSOLE, &procs);
> -
> - interp_add (cli_interp);
> + interp_factory_register (INTERP_CONSOLE, cli_interp_factory);
> }
> diff --git a/gdb/interps.c b/gdb/interps.c
> index 7f57132..ca8512b 100644
> --- a/gdb/interps.c
> +++ b/gdb/interps.c
> @@ -91,18 +91,20 @@ struct interp
>
> void _initialize_interpreter (void);
>
> +static struct interp *interp_lookup_existing (const char *name);
> +
> /* interp_new - This allocates space for a new interpreter,
> fills the fields from the inputs, and returns a pointer to the
> interpreter. */
> struct interp *
> -interp_new (const char *name, const struct interp_procs *procs)
> +interp_new (const char *name, const struct interp_procs *procs, void *data)
> {
> struct interp *new_interp;
>
> new_interp = XNEW (struct interp);
>
> new_interp->name = xstrdup (name);
> - new_interp->data = NULL;
> + new_interp->data = data;
> new_interp->quiet_p = 0;
> new_interp->procs = procs;
> new_interp->inited = 0;
> @@ -113,6 +115,49 @@ interp_new (const char *name, const struct interp_procs *procs)
> return new_interp;
> }
>
> +/* An interpreter factory. Maps an interpreter name to the factory
> + function that instantiates an interpreter by that name. */
> +
> +struct interp_factory
> +{
> + /* This is the name in "-i=INTERP" and "interpreter-exec INTERP". */
> + const char *name;
> +
> + /* The function that creates the interpreter. */
> + interp_factory_func func;
> +};
> +
> +typedef struct interp_factory *interp_factory_p;
> +DEF_VEC_P(interp_factory_p);
> +
> +/* The registered interpreter factories. */
> +static VEC(interp_factory_p) *interpreter_factories = NULL;
> +
> +/* See interps.h. */
> +
> +void
> +interp_factory_register (const char *name, interp_factory_func func)
> +{
> + struct interp_factory *f;
> + int ix;
> +
> + /* Assert that no factory for NAME is already registered. */
> + for (ix = 0;
> + VEC_iterate (interp_factory_p, interpreter_factories, ix, f);
> + ++ix)
> + if (strcmp (f->name, name) == 0)
> + {
> + internal_error (__FILE__, __LINE__,
> + _("interpreter factory already registered: \"%s\"\n"),
> + name);
> + }
> +
> + f = XNEW (struct interp_factory);
> + f->name = name;
> + f->func = func;
> + VEC_safe_push (interp_factory_p, interpreter_factories, f);
> +}
> +
> /* Add interpreter INTERP to the gdb interpreter list. The
> interpreter must not have previously been added. */
> void
> @@ -120,7 +165,7 @@ interp_add (struct interp *interp)
> {
> struct ui_interp_info *ui_interp = get_current_interp_info ();
>
> - gdb_assert (interp_lookup (interp->name) == NULL);
> + gdb_assert (interp_lookup_existing (interp->name) == NULL);
>
> interp->next = ui_interp->interp_list;
> ui_interp->interp_list = interp;
> @@ -219,18 +264,15 @@ interp_set (struct interp *interp, int top_level)
> return 1;
> }
>
> -/* interp_lookup - Looks up the interpreter for NAME. If no such
> - interpreter exists, return NULL, otherwise return a pointer to the
> - interpreter. */
> -struct interp *
> -interp_lookup (const char *name)
> +/* Look up the interpreter for NAME. If no such interpreter exists,
> + return NULL, otherwise return a pointer to the interpreter. */
> +
> +static struct interp *
> +interp_lookup_existing (const char *name)
> {
> struct ui_interp_info *ui_interp = get_current_interp_info ();
> struct interp *interp;
>
> - if (name == NULL || strlen (name) == 0)
> - return NULL;
> -
> for (interp = ui_interp->interp_list;
> interp != NULL;
> interp = interp->next)
> @@ -242,6 +284,37 @@ interp_lookup (const char *name)
> return NULL;
> }
>
> +/* See interps.h. */
> +
> +struct interp *
> +interp_lookup (const char *name)
> +{
> + struct ui *ui = current_ui;
> + struct interp_factory *factory;
> + struct interp *interp;
> + int ix;
> +
> + if (name == NULL || strlen (name) == 0)
> + return NULL;
> +
> + /* Only create each interpreter once per top level. */
> + interp = interp_lookup_existing (name);
> + if (interp != NULL)
> + return interp;
> +
> + for (ix = 0;
> + VEC_iterate (interp_factory_p, interpreter_factories, ix, factory);
> + ++ix)
> + if (strcmp (factory->name, name) == 0)
> + {
> + interp = factory->func (name, ui);
> + interp_add (interp);
> + return interp;
> + }
> +
> + return NULL;
> +}
> +
> /* Returns the current interpreter. */
>
> struct ui_out *
> @@ -469,15 +542,15 @@ static VEC (char_ptr) *
> interpreter_completer (struct cmd_list_element *ignore,
> const char *text, const char *word)
> {
> - struct ui_interp_info *ui_interp = get_current_interp_info ();
> + struct interp_factory *interp;
> int textlen;
> VEC (char_ptr) *matches = NULL;
> - struct interp *interp;
> + int ix;
>
> textlen = strlen (text);
> - for (interp = ui_interp->interp_list;
> - interp != NULL;
> - interp = interp->next)
> + for (ix = 0;
> + VEC_iterate (interp_factory_p, interpreter_factories, ix, interp);
> + ++ix)
> {
> if (strncmp (interp->name, text, textlen) == 0)
> {
> diff --git a/gdb/interps.h b/gdb/interps.h
> index f0badc5..3065fdf 100644
> --- a/gdb/interps.h
> +++ b/gdb/interps.h
> @@ -24,6 +24,17 @@
>
> struct ui_out;
> struct interp;
> +struct ui;
> +
> +typedef struct interp *(*interp_factory_func) (const char *interp,

Nit:

In the functions of this type ({cli,mi,tui}_interp_factory), this parameter is named
"name", so you could as well name it "name" here as well.

> + struct ui *ui);

Even in the final result (with all patches applied), none of the factories
use the ui parameter, is it expected?