From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 22396 invoked by alias); 13 Jan 2010 15:49:25 -0000 Received: (qmail 22372 invoked by uid 22791); 13 Jan 2010 15:49:22 -0000 X-SWARE-Spam-Status: No, hits=-2.5 required=5.0 tests=AWL,BAYES_00,SPF_PASS X-Spam-Check-By: sourceware.org Received: from mail.codesourcery.com (HELO mail.codesourcery.com) (38.113.113.100) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Wed, 13 Jan 2010 15:49:17 +0000 Received: (qmail 4327 invoked from network); 13 Jan 2010 15:49:15 -0000 Received: from unknown (HELO orlando) (pedro@127.0.0.2) by mail.codesourcery.com with ESMTPA; 13 Jan 2010 15:49:15 -0000 From: Pedro Alves To: gdb-patches@sourceware.org Subject: Re: Per-inferior program arguments and io terminal Date: Wed, 13 Jan 2010 15:49:00 -0000 User-Agent: KMail/1.9.10 Cc: Vladimir Prus References: <201001131544.26408.vladimir@codesourcery.com> In-Reply-To: <201001131544.26408.vladimir@codesourcery.com> MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Content-Disposition: inline Message-Id: <201001131549.15528.pedro@codesourcery.com> X-IsSubscribed: yes 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 X-SW-Source: 2010-01/txt/msg00348.txt.bz2 Thanks. IMO, it doesn't make sense to have per-inferior args, but not per-inferior environ. We should give the environ the same treatment now, so that we don't end up with a per-inferior args in 7.1, and per-inferior args and environ in post 7.1. On Wednesday 13 January 2010 12:44:26, Vladimir Prus wrote: > commit 62c539aad84bf40129de8c8e1232af7c332885b1 > Author: Vladimir Prus > Date: Mon Dec 21 15:28:03 2009 +0300 > > Per-inferior args and tty. > > * infcmd.c (inferior_args): Rename to ... > (inferior_args_sctach): ... this. Typo. > (inferior_io_terminal): Rename to ... > (inferior_io_terminal_scratch): ... this. > (inferior_argc, inferior_argv): Remove. > (set_inferior_io_terminal, get_inferior_io_terminal): Store > inside current_inferior(). > (notice_inferior_io_terminal_set, show_inferior_io_terminal): New. > (get_inferior_args, set_inferior_args): Store inside > current_inferior(). > (set_inferior_args_vector, notice_args_set): Likewise. > (tty_command): Remove. > (run_command_1): Don't free old args, as they are feed by > set_inferior_arg now. > (run_no_args_command): Likewise. > (_initialize_infcmd): Adjust variables. > * inferior.h (set_inferior_arg): Adjust prototype. > (struct inferior): New fields args, argc, argv, terminal. > * main.c (captured_main): Set arguments only after the initial > inferior has been created. Set set_inferior_io_terminal, > not tty_command. > > diff --git a/gdb/infcmd.c b/gdb/infcmd.c > index 21a2233..f0804b8 100644 > --- a/gdb/infcmd.c > +++ b/gdb/infcmd.c > @@ -126,20 +126,17 @@ void _initialize_infcmd (void); > #define ERROR_NO_INFERIOR \ > if (!target_has_execution) error (_("The program is not being run.")); > > -/* String containing arguments to give to the program, separated by spaces. > - Empty string (pointer to '\0') means no args. */ > +/* Scratch area where string containing arguments to give to the program will be > + stored by 'set args'. As soon as anything is stored, notice_args_set will > + move it into per-inferior storage. Arguments are separated by spaces. Empty > + string (pointer to '\0') means no args. */ > > -static char *inferior_args; > +static char *inferior_args_scratch; > > -/* The inferior arguments as a vector. If INFERIOR_ARGC is nonzero, > - then we must compute INFERIOR_ARGS from this (via the target). */ > +/* Scratch area where 'set inferior-tty' will store user-provided value. > + We'll immediate copy it into per-inferior storage. */ > > -static int inferior_argc; > -static char **inferior_argv; > - > -/* File name for default use for standard in/out in the inferior. */ > - > -static char *inferior_io_terminal; > +static char *inferior_io_terminal_scratch; > > /* Pid of our debugged inferior, or 0 if no inferior now. > Since various parts of infrun.c test this to see whether there is a program > @@ -176,64 +173,74 @@ struct gdb_environ *inferior_environ; > void > set_inferior_io_terminal (const char *terminal_name) > { > - if (inferior_io_terminal) > - xfree (inferior_io_terminal); > - > - if (!terminal_name) > - inferior_io_terminal = NULL; > - else > - inferior_io_terminal = xstrdup (terminal_name); > + xfree (current_inferior ()->terminal); > + current_inferior ()->terminal = terminal_name ? xstrdup (terminal_name) : 0; > } > > const char * > get_inferior_io_terminal (void) > { > - return inferior_io_terminal; > + return current_inferior ()->terminal; > +} > + > +static void > +notice_inferior_io_terminal_set (char *args, int from_tty, struct cmd_list_element *c) > +{ > + /* CLI has assigned the user-provided value to inferior_io_terminal_scratch. > + Now route it to current inferior. */ > + set_inferior_io_terminal (inferior_io_terminal_scratch); > +} > + > +static void > +show_inferior_io_terminal (struct ui_file *file, int from_tty, > + struct cmd_list_element *c, const char *value) > +{ > + /* Note that we ignore the passed-in value in favor of computing it > + directly. */ > + deprecated_show_value_hack (file, from_tty, c, get_inferior_io_terminal ()); > } Please don't add more deprecated_show_value_hack uses. It's really a nasty hack (it's in cli/cli-setshow.c). It has this gem: /* Print doc minus "show" at start. */ print_doc_line (gdb_stdout, c->doc + 5); This is obviously busted for i18n. Please write something like this instead: fprintf_filtered (gdb_stdout, _("..."), get_inferior_io_terminal ()); > > char * > get_inferior_args (void) > { > - if (inferior_argc != 0) > + if (current_inferior ()->argc != 0) > { > char *n, *old; > > - n = construct_inferior_arguments (inferior_argc, inferior_argv); > - old = set_inferior_args (n); > - xfree (old); > + n = construct_inferior_arguments (current_inferior ()->argc, > + current_inferior ()->argv); > + set_inferior_args (n); > } > > - if (inferior_args == NULL) > - inferior_args = xstrdup (""); > + if (current_inferior ()->args == NULL) > + current_inferior ()->args = xstrdup (""); > > - return inferior_args; > + return current_inferior ()->args; > } > > -char * > +void > set_inferior_args (char *newargs) > { > - char *saved_args = inferior_args; > - > - inferior_args = newargs; > - inferior_argc = 0; > - inferior_argv = 0; > - > - return saved_args; > + xfree (current_inferior ()->args); > + current_inferior ()->args = xstrdup (newargs ? newargs : ""); > + current_inferior ()->argc = 0; > + current_inferior ()->argv = 0; > } > > void > set_inferior_args_vector (int argc, char **argv) > { > - inferior_argc = argc; > - inferior_argv = argv; > + current_inferior ()->argc = argc; > + current_inferior ()->argv = argv; > } > > /* Notice when `set args' is run. */ > static void > notice_args_set (char *args, int from_tty, struct cmd_list_element *c) > { > - inferior_argc = 0; > - inferior_argv = 0; > + /* CLI has assigned the user-provided value to inferior_args_scratch. > + Now route it to current inferior. */ > + set_inferior_args (inferior_args_scratch); > } > > /* Notice when `show args' is run. */ > @@ -369,15 +376,6 @@ strip_bg_char (char **args) > return 0; > } > > -void > -tty_command (char *file, int from_tty) > -{ > - if (file == 0) > - error_no_arg (_("terminal name for running target process")); > - > - set_inferior_io_terminal (file); > -} If you're removing this, please delete the declaration in inferior.h as well. A generic comment: it is good (and the common practice) to name the FOO command callbacks as "FOO_command" or "set_FOO_command"/"show_FOO_command", so that we can find a command implementation easily by just guessing and grepping. > - > /* Common actions to take after creating any sort of inferior, by any > means (running, attaching, connecting, et cetera). The target > should be stopped. */ > @@ -536,10 +534,7 @@ run_command_1 (char *args, int from_tty, int tbreak_at_main) > > /* If there were other args, beside '&', process them. */ > if (args) > - { > - char *old_args = set_inferior_args (xstrdup (args)); > - xfree (old_args); > - } > + set_inferior_args (xstrdup (args)); There's inconsistency in the `args' memory managent. set_inferior_args already xstrdups the incoming argument. What should stay? Can we have a short comment in set_inferior_args's decription mentioning who's supposed to manage the args' memory? > } > > if (from_tty) > @@ -594,8 +589,7 @@ run_command (char *args, int from_tty) > static void > run_no_args_command (char *args, int from_tty) > { > - char *old_args = set_inferior_args (xstrdup ("")); > - xfree (old_args); > + set_inferior_args (xstrdup ("")); > } > likewise. > > @@ -2646,14 +2640,17 @@ _initialize_infcmd (void) > > /* add the filename of the terminal connected to inferior I/O */ > add_setshow_filename_cmd ("inferior-tty", class_run, > - &inferior_io_terminal, _("\ > + &inferior_io_terminal_scratch, _("\ > Set terminal for future runs of program being debugged."), _("\ > Show terminal for future runs of program being debugged."), _("\ > -Usage: set inferior-tty /dev/pts/1"), NULL, NULL, &setlist, &showlist); > +Usage: set inferior-tty /dev/pts/1"), > + notice_inferior_io_terminal_set, > + show_inferior_io_terminal, > + &setlist, &showlist); > add_com_alias ("tty", "set inferior-tty", class_alias, 0); > > add_setshow_optional_filename_cmd ("args", class_run, > - &inferior_args, _("\ > + &inferior_args_scratch, _("\ > Set argument list to give program being debugged when it is started."), _("\ > Show argument list to give program being debugged when it is started."), _("\ > Follow this command with any number of args, to be passed to the program."), > diff --git a/gdb/inferior.h b/gdb/inferior.h > index 9798ef1..048fc11 100644 > --- a/gdb/inferior.h > +++ b/gdb/inferior.h > @@ -261,7 +261,7 @@ extern void attach_command (char *, int); > > extern char *get_inferior_args (void); > > -extern char *set_inferior_args (char *); > +extern void set_inferior_args (char *); > > extern void set_inferior_args_vector (int, char **); > > @@ -427,6 +427,18 @@ struct inferior > /* The program space bound to this inferior. */ > struct program_space *pspace; > > + /* The arguments string to use when running. */ > + char *args; > + > + /* The size of elements in argv. */ > + int argc; > + > + /* The vector version of arguments. */ > + char **argv; > + > + /* The name of terminal device to use for I/O. */ > + char *terminal; > + These are all leaking when the inferior is deleted. > /* See the definition of stop_kind above. */ > enum stop_kind stop_soon; > > diff --git a/gdb/main.c b/gdb/main.c > index 0cc0f75..e9f3857 100644 > --- a/gdb/main.c > +++ b/gdb/main.c > @@ -631,54 +631,6 @@ extern int gdbtk_test (char *); > use_windows = 0; > } > > - if (set_args) > - { > - /* The remaining options are the command-line options for the > - inferior. The first one is the sym/exec file, and the rest > - are arguments. */ > - if (optind >= argc) > - { > - fprintf_unfiltered (gdb_stderr, > - _("%s: `--args' specified but no program specified\n"), > - argv[0]); > - exit (1); > - } > - symarg = argv[optind]; > - execarg = argv[optind]; > - ++optind; > - set_inferior_args_vector (argc - optind, &argv[optind]); > - } > - else > - { > - /* OK, that's all the options. */ > - > - /* The first argument, if specified, is the name of the > - executable. */ > - if (optind < argc) > - { > - symarg = argv[optind]; > - execarg = argv[optind]; > - optind++; > - } > - > - /* If the user hasn't already specified a PID or the name of a > - core file, then a second optional argument is allowed. If > - present, this argument should be interpreted as either a > - PID or a core file, whichever works. */ > - if (pidarg == NULL && corearg == NULL && optind < argc) > - { > - pid_or_core_arg = argv[optind]; > - optind++; > - } > - > - /* Any argument left on the command line is unexpected and > - will be ignored. Inform the user. */ > - if (optind < argc) > - fprintf_unfiltered (gdb_stderr, _("\ > -Excess command line arguments ignored. (%s%s)\n"), > - argv[optind], > - (optind == argc - 1) ? "" : " ..."); > - } > if (batch) > quiet = 1; > } > @@ -687,6 +639,57 @@ Excess command line arguments ignored. (%s%s)\n"), > control of the console via the deprecated_init_ui_hook (). */ > gdb_init (argv[0]); > > + /* Now that gdb_init has created the initial inferior, we're in position > + to set args for that inferior. */ > + if (set_args) > + { > + /* The remaining options are the command-line options for the > + inferior. The first one is the sym/exec file, and the rest > + are arguments. */ > + if (optind >= argc) > + { > + fprintf_unfiltered (gdb_stderr, > + _("%s: `--args' specified but no program specified\n"), > + argv[0]); > + exit (1); > + } > + symarg = argv[optind]; > + execarg = argv[optind]; > + ++optind; > + set_inferior_args_vector (argc - optind, &argv[optind]); > + } > + else > + { > + /* OK, that's all the options. */ > + > + /* The first argument, if specified, is the name of the > + executable. */ > + if (optind < argc) > + { > + symarg = argv[optind]; > + execarg = argv[optind]; > + optind++; > + } > + > + /* If the user hasn't already specified a PID or the name of a > + core file, then a second optional argument is allowed. If > + present, this argument should be interpreted as either a > + PID or a core file, whichever works. */ > + if (pidarg == NULL && corearg == NULL && optind < argc) > + { > + pid_or_core_arg = argv[optind]; > + optind++; > + } > + > + /* Any argument left on the command line is unexpected and > + will be ignored. Inform the user. */ > + if (optind < argc) > + fprintf_unfiltered (gdb_stderr, _("\ > +Excess command line arguments ignored. (%s%s)\n"), > + argv[optind], > + (optind == argc - 1) ? "" : " ..."); > + } > + > /* Lookup gdbinit files. Note that the gdbinit file name may be overriden > during file initialization, so get_init_files should be called after > gdb_init. */ > @@ -840,7 +843,7 @@ Can't attach to process and specify a core file at the same time.")); > } > > if (ttyarg != NULL) > - catch_command_errors (tty_command, ttyarg, !batch, RETURN_MASK_ALL); > + set_inferior_io_terminal (ttyarg); > > /* Error messages should no longer be distinguished with extra output. */ > error_pre_print = NULL;