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

[Simon Marchi <simon dot marchi at ericsson dot com>]

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?