From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 6248 invoked by alias); 14 Jan 2010 15:49:28 -0000 Received: (qmail 6217 invoked by uid 22791); 14 Jan 2010 15:49:25 -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 15:49:16 +0000 Received: (qmail 31546 invoked from network); 14 Jan 2010 15:49:14 -0000 Received: from unknown (HELO orlando) (pedro@127.0.0.2) by mail.codesourcery.com with ESMTPA; 14 Jan 2010 15:49:14 -0000 From: Pedro Alves To: Vladimir Prus Subject: Re: Per-inferior program arguments and io terminal Date: Thu, 14 Jan 2010 15:49:00 -0000 User-Agent: KMail/1.9.10 Cc: gdb-patches@sourceware.org References: <201001131544.26408.vladimir@codesourcery.com> <201001141337.43052.pedro@codesourcery.com> <201001141809.17094.vladimir@codesourcery.com> In-Reply-To: <201001141809.17094.vladimir@codesourcery.com> MIME-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: 7bit Content-Disposition: inline Message-Id: <201001141549.11692.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/msg00379.txt.bz2 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