From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 107645 invoked by alias); 18 May 2016 19:20:01 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org Received: (qmail 107599 invoked by uid 89); 18 May 2016 19:19:59 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-1.9 required=5.0 tests=BAYES_00,SPF_PASS autolearn=ham version=3.3.2 spammy=afterwards, 1206, 1136, 857 X-HELO: usplmg21.ericsson.net Received: from usplmg21.ericsson.net (HELO usplmg21.ericsson.net) (198.24.6.65) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with (AES256-SHA encrypted) ESMTPS; Wed, 18 May 2016 19:19:57 +0000 Received: from EUSAAHC001.ericsson.se (Unknown_Domain [147.117.188.75]) by usplmg21.ericsson.net (Symantec Mail Security) with SMTP id 5E.FF.03614.820CC375; Wed, 18 May 2016 21:19:04 +0200 (CEST) Received: from [142.133.110.144] (147.117.188.8) by smtp-am.internal.ericsson.com (147.117.188.77) with Microsoft SMTP Server id 14.3.248.2; Wed, 18 May 2016 15:19:46 -0400 Subject: Re: [PATCH v3 06/34] Introduce interpreter factories To: Pedro Alves , References: <1462538104-19109-1-git-send-email-palves@redhat.com> <1462538104-19109-7-git-send-email-palves@redhat.com> From: Simon Marchi Message-ID: <573CC052.6000502@ericsson.com> Date: Wed, 18 May 2016 19:20:00 -0000 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Thunderbird/38.6.0 MIME-Version: 1.0 In-Reply-To: <1462538104-19109-7-git-send-email-palves@redhat.com> Content-Type: text/plain; charset="windows-1252" Content-Transfer-Encoding: 7bit X-IsSubscribed: yes X-SW-Source: 2016-05/txt/msg00313.txt.bz2 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 > > * 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?