From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 10725 invoked by alias); 14 Jan 2010 13:38:03 -0000 Received: (qmail 10667 invoked by uid 22791); 14 Jan 2010 13:38:01 -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; Thu, 14 Jan 2010 13:37:47 +0000 Received: (qmail 11211 invoked from network); 14 Jan 2010 13:37:45 -0000 Received: from unknown (HELO orlando) (pedro@127.0.0.2) by mail.codesourcery.com with ESMTPA; 14 Jan 2010 13:37:45 -0000 From: Pedro Alves To: Vladimir Prus Subject: Re: Per-inferior program arguments and io terminal Date: Thu, 14 Jan 2010 13:38:00 -0000 User-Agent: KMail/1.9.10 Cc: gdb-patches@sourceware.org References: <201001131544.26408.vladimir@codesourcery.com> <201001131549.15528.pedro@codesourcery.com> <201001141227.21149.vladimir@codesourcery.com> In-Reply-To: <201001141227.21149.vladimir@codesourcery.com> MIME-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: 7bit Content-Disposition: inline Message-Id: <201001141337.43052.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/msg00377.txt.bz2 Thanks. On Thursday 14 January 2010 09:27:21, Vladimir Prus wrote: > On Wednesday 13 January 2010 18:49:15 Pedro Alves wrote: > > > > -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. > > I am not sure I follow you. tty_command, although named as FOO_command, > was not actually used as command anywhere I can see. Sure is: (gdb) tty Argument required (filename to set it to.). (gdb) help tty Set terminal for future runs of program being debugged. Usage: set inferior-tty /dev/pts/1 See: -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); ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ > Or do you refer to some other function? -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); Better choices would be: set_inferior_tty_command/show_inferior_tty_command or set_inferior_tty/show_inferior_tty. On Thursday 14 January 2010 09:27:21, Vladimir Prus wrote: > Per-inferior args and tty. and environ. > > * infcmd.c (inferior_args): Rename to ... > (inferior_args_scratch): ... this. > (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. Typo, "feed". > (run_no_args_command): Likewise. > (inferior_environ): Remove. > (run_command_1): Use environemnt of the current inferior. Typo, "environemnt". > (environtment_info, set_environment_command) Typo, "environtment_info". > (unset_environment_command, path_info, path_command): Likewise. > (_initialize_infcmd): Adjust variables. Do no init inferior_environ. Typo, "Do no". > * inferior.h (set_inferior_arg): Adjust prototype. > (struct inferior): New fields args, argc, argv, terminal, environment. > (inferior_environ): Remove declaration. > * inferior.c (free_inferior): Free new fields. > (add_inferior_silent): Initialize 'environment' field. > * main.c (captured_main): Set arguments only after the initial > inferior has been created. Set set_inferior_io_terminal, > not tty_command. > * mi/mi-main.c (mi_cmd_env_path): Use environemnt of the current > inferior. Typo, "environemnt". > (_initialize_mi_cmd_env): Adjust for disappearance of global > inferior_environ. > * solib.c (solib_find): Use environment of the current inferior. > > +static void > +notice_inferior_io_terminal_set (char *args, int from_tty, struct cmd_list_element *c) Line too long. > +{ > + /* 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. */ > + fprintf_filtered (gdb_stdout, > + _("argument list to give program being debugged when it is started is %s"), Line too long. > + get_inferior_io_terminal ()); > } > > char * > get_inferior_args (void) > { > - if (inferior_argc != 0) > + if (current_inferior ()->argc != 0) > { > - char *n, *old; > + char *n; > > - 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); > + xfree (n); > } > > - if (inferior_args == NULL) > - inferior_args = xstrdup (""); Nit. there's a mismatch between this here and set_inferior_args: > + if (current_inferior ()->args == NULL) > + current_inferior ()->args = xstrdup (""); > > - return inferior_args; > + return current_inferior ()->args; > } > > -char * > +/* Set the arguments for the current inferior. Ownership of > + NEWARGS is not transferred. */ > + > +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 : ""); This never leaves NULL args. I guess it could. > + 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; > } > > @@ -87,6 +88,10 @@ free_inferior (struct inferior *inf) > { > discard_all_inferior_continuations (inf); > inferior_free_data (inf); > + xfree (inf->args); > + xfree (inf->argv); Hmm, I was going to say that this usually leaks argv[0..argc], and you should use freeargv, but, I now see that the only caller of set_inferior_args_vector is captured_main, and the argc,argv are presently a shallow copy of `main's arguments, so definitely not ok to free this as is. This definitely needs a comment in the description of inferior->argv, expanded from the comment that was in inferior_argv. > + xfree (inf->terminal); > + free_environ (inf->environment); > xfree (inf->private); > xfree (inf); > } -- Pedro Alves