Re: Per-inferior program arguments and io terminal

[Pedro Alves <pedro at codesourcery dot com>]

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 <vladimir@codesourcery.com>
> 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;