Re: Per-inferior program arguments and io terminal

[Pedro Alves <pedro@codesourcery.com>]

On Thursday 14 January 2010 15:09:17, Vladimir Prus wrote:
> On Thursday 14 January 2010 16:37:42 Pedro Alves wrote:
> > Better choices would be: set_inferior_tty_command/show_inferior_tty_command
> > or set_inferior_tty/show_inferior_tty.
> 
> Done. Also done for args function.

Thanks.

> > > @@ -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.
> 
> I've added a comment.

Thanks, but not correct yet...

> @@ -87,6 +88,10 @@ free_inferior (struct inferior *inf)
>  {
>    discard_all_inferior_continuations (inf);
>    inferior_free_data (inf);
> +  xfree (inf->args);
> +  xfree (inf->argv);
> +  xfree (inf->terminal);
> +  free_environ (inf->environment);
>    xfree (inf->private);

Maybe I confused you with saying "shallow" copy.  This
argv comes from a direct `argv' pointer copy:

 void
 set_inferior_args_vector (int argc, char **argv)
 {
  current_inferior ()->argc = argc;
  current_inferior ()->argv = argv;
 }

This is only called from captured_main, like so:

 set_inferior_args_vector (argc - optind, &argv[optind]);

So at best, you'd need to `xfree (&argv[-optind])', but
even that would be broken.  Say, let's ignore that offset.  argv
in that context is a pointer copy coming from:

static int
captured_main (void *data)
{
  struct captured_main_args *context = data;
  int argc = context->argc;
  char **argv = context->argv;

and this context->argv itself comes from:

int
main (int argc, char **argv)
{
  struct captured_main_args args;
  memset (&args, 0, sizeof args);
  args.argc = argc;
  args.argv = argv;
  ^^^^^^^^^^^^^^^^
  args.use_windows = 0;
  args.interpreter_p = INTERP_CONSOLE;
  return gdb_main (&args);
}

so, freeing that would be equivalent to:

 int
 main (int argc, char **argv)
 {
    free (argv);
 }

which is not kosher (try it).   You should _not_ free inferior->argv.

Actually, I'll try it myself:

> ./gdb -q --args /home/pedro/gdb/tests/threads --args foo bar
(gdb) clone-inferior
Added inferior 2.
(gdb) inferior 1
[Switching to inferior 1 [process 0] (/home/pedro/gdb/tests/threads)]
(gdb) inferior 2
[Switching to inferior 2 [process 0] (/home/pedro/gdb/tests/threads)]
(gdb) remove-inferior 1
*** glibc detected *** ./gdb: munmap_chunk(): invalid pointer: 0x00007fff9d5b4fe8 ***
======= Backtrace: =========
/lib/libc.so.6(cfree+0x1b6)[0x7f7f0e86cd46]
./gdb(xfree+0x1c)[0x462f1e]
:
Cancelled (core dumped)

And since you now made me try the patch... :-)

>qr
Warning: trailing whitespace in lines 230,238 of gdb/solib.c
Warning: trailing whitespace in lines 664,673,683 of gdb/main.c
Warning: trailing whitespace in line 266 of gdb/mi/mi-cmd-env.c
Warning: trailing whitespace in lines 129,186,200,213,2653,2654,2655 of gdb/infcmd.c
Refreshed patch per-inferior-args3.diff

-- 
Pedro Alves