From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 21275 invoked by alias); 16 Sep 2011 14:44:01 -0000 Received: (qmail 21260 invoked by uid 22791); 16 Sep 2011 14:43:59 -0000 X-SWARE-Spam-Status: No, hits=-1.3 required=5.0 tests=AWL,BAYES_50,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,FREEMAIL_FROM,RCVD_IN_DNSWL_LOW,TW_CL,TW_RG X-Spam-Check-By: sourceware.org Received: from mail-ww0-f43.google.com (HELO mail-ww0-f43.google.com) (74.125.82.43) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Fri, 16 Sep 2011 14:43:42 +0000 Received: by wwf27 with SMTP id 27so4661343wwf.12 for ; Fri, 16 Sep 2011 07:43:41 -0700 (PDT) MIME-Version: 1.0 Received: by 10.216.137.76 with SMTP id x54mr829538wei.44.1316184220823; Fri, 16 Sep 2011 07:43:40 -0700 (PDT) Received: by 10.216.93.81 with HTTP; Fri, 16 Sep 2011 07:43:40 -0700 (PDT) In-Reply-To: <8B49B563-BE0B-452C-8956-31E00A880D96@adacore.com> References: <201109071409.06452.pedro@codesourcery.com> <8B49B563-BE0B-452C-8956-31E00A880D96@adacore.com> Date: Fri, 16 Sep 2011 14:47:00 -0000 Message-ID: Subject: Re: [RFA] Preliminary work in fork_inferior From: Abhijit Halder To: Tristan Gingold Cc: "gdb-patches@sourceware.org ml" , Pedro Alves Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: quoted-printable 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: 2011-09/txt/msg00302.txt.bz2 On Fri, Sep 16, 2011 at 5:58 PM, Tristan Gingold wrot= e: > Hi, > > this patch is both a cleanup and a preliminary work for Lion. > > It fixes a few weirdness: > - shell_command was always allocated even if not used > - argv was xmalloc'ed but never free > - gdb_flush/_exit sequence for exec failure was duplicated > > The preliminary work consists in calling execvp wether or not a shell is = executed. > > No regressions on GNU/Linux i386 > > Ok for trunk ? > > Tristan. > > 2011-09-07 =A0Tristan Gingold =A0 > > =A0 =A0 =A0 =A0* fork-child.c (fork_inferior): Update comment. > =A0 =A0 =A0 =A0Use alloca instead of xmalloc for argv. =A0Move > =A0 =A0 =A0 =A0len and shell_command declarations in the block where they= are used. > =A0 =A0 =A0 =A0Only call execvp. =A0Factorize some failure code. > > diff --git a/gdb/fork-child.c b/gdb/fork-child.c > index bb173e7..e937aec 100644 > --- a/gdb/fork-child.c > +++ b/gdb/fork-child.c > @@ -126,9 +126,7 @@ fork_inferior (char *exec_file_arg, char *allargs, ch= ar **env, > =A0 =A0 =A0 =A0 =A0 =A0 =A0 void (*pre_trace_fun) (void), char *shell_fil= e_arg) > =A0{ > =A0 int pid; > - =A0char *shell_command; > =A0 static char default_shell_file[] =3D SHELL_FILE; I don't know whether this is relevant, but I think the SHELL_FILE macro is defined as "/bin/sh" and this will not work for MinGW. Some similar comments were popped up during the review of my patch for adding pipe command in GDB. I'm relating this to that. > - =A0int len; > =A0 /* Set debug_fork then attach to the child while it sleeps, to debug.= =A0*/ > =A0 static int debug_fork =3D 0; > =A0 /* This is set to the result of setpgrp, which if vforked, will be vi= sible > @@ -162,16 +160,6 @@ fork_inferior (char *exec_file_arg, char *allargs, c= har **env, > =A0 =A0 =A0 shell =3D 1; > =A0 =A0 } > > - =A0/* Multiplying the length of exec_file by 4 is to account for the > - =A0 =A0 fact that it may expand when quoted; it is a worst-case number > - =A0 =A0 based on every character being '. =A0*/ > - =A0len =3D 5 + 4 * strlen (exec_file) + 1 + strlen (allargs) + 1 + /*sl= op */ 12; > - =A0if (exec_wrapper) > - =A0 =A0len +=3D strlen (exec_wrapper) + 1; > - > - =A0shell_command =3D (char *) alloca (len); > - =A0shell_command[0] =3D '\0'; > - > =A0 if (!shell) > =A0 =A0 { > =A0 =A0 =A0 /* We're going to call execvp. =A0Create argument vector. > @@ -180,18 +168,29 @@ fork_inferior (char *exec_file_arg, char *allargs, = char **env, > =A0 =A0 =A0 =A0 argument. =A0*/ > =A0 =A0 =A0 int argc =3D (strlen (allargs) + 1) / 2 + 2; > > - =A0 =A0 =A0argv =3D (char **) xmalloc (argc * sizeof (*argv)); > + =A0 =A0 =A0argv =3D (char **) alloca (argc * sizeof (*argv)); alloca is not a portable function I guess. > =A0 =A0 =A0 argv[0] =3D exec_file; > =A0 =A0 =A0 breakup_args (allargs, &argv[1]); > =A0 =A0 } > =A0 else > =A0 =A0 { > =A0 =A0 =A0 /* We're going to call a shell. =A0*/ > - > + =A0 =A0 =A0char *shell_command; > + =A0 =A0 =A0int len; > =A0 =A0 =A0 char *p; > =A0 =A0 =A0 int need_to_quote; > =A0 =A0 =A0 const int escape_bang =3D escape_bang_in_quoted_argument (she= ll_file); > > + =A0 =A0 =A0/* Multiplying the length of exec_file by 4 is to account fo= r the > + =A0 =A0 =A0 =A0 fact that it may expand when quoted; it is a worst-case= number > + =A0 =A0 =A0 =A0 based on every character being '. =A0*/ > + =A0 =A0 =A0len =3D 5 + 4 * strlen (exec_file) + 1 + strlen (allargs) + = 1 + /*slop */ 12; > + =A0 =A0 =A0if (exec_wrapper) > + =A0 =A0 =A0 =A0len +=3D strlen (exec_wrapper) + 1; > + > + =A0 =A0 =A0shell_command =3D (char *) alloca (len); > + =A0 =A0 =A0shell_command[0] =3D '\0'; > + > =A0 =A0 =A0 strcat (shell_command, "exec "); > > =A0 =A0 =A0 /* Add any exec wrapper. =A0That may be a program name with a= rguments, so > @@ -257,6 +256,16 @@ fork_inferior (char *exec_file_arg, char *allargs, c= har **env, > > =A0 =A0 =A0 strcat (shell_command, " "); > =A0 =A0 =A0 strcat (shell_command, allargs); > + > + =A0 =A0 =A0/* If we decided above to start up with a shell, we exec the > + =A0 =A0 =A0 =A0shell, "-c" says to interpret the next arg as a shell co= mmand > + =A0 =A0 =A0 =A0to execute, and this command is "exec > + =A0 =A0 =A0 =A0". =A0*/ > + =A0 =A0 =A0argv =3D (char **) alloca (4 * sizeof (char *)); Same comment about alloca as above. > + =A0 =A0 =A0argv[0] =3D shell_file; > + =A0 =A0 =A0argv[1] =3D "-c"; For MinGW the -c switch will not work. > + =A0 =A0 =A0argv[2] =3D shell_command; > + =A0 =A0 =A0argv[3] =3D (char *) 0; > =A0 =A0 } > > =A0 /* On some systems an exec will fail if the executable is open. =A0*/ > @@ -348,29 +357,18 @@ fork_inferior (char *exec_file_arg, char *allargs, = char **env, > =A0 =A0 =A0 =A0 =A0path to find $SHELL. =A0Rich Pixley says so, and I agr= ee. =A0*/ > =A0 =A0 =A0 environ =3D env; > > - =A0 =A0 =A0/* If we decided above to start up with a shell, we exec the > - =A0 =A0 =A0 =A0shell, "-c" says to interpret the next arg as a shell co= mmand > - =A0 =A0 =A0 =A0to execute, and this command is "exec > - =A0 =A0 =A0 =A0". =A0*/ > + =A0 =A0 =A0execvp (argv[0], argv); > + > + =A0 =A0 =A0/* If we get here, it's an error. =A0*/ > =A0 =A0 =A0 if (shell) > =A0 =A0 =A0 =A0{ > - =A0 =A0 =A0 =A0 execlp (shell_file, shell_file, "-c", shell_command, (c= har *) 0); > - > - =A0 =A0 =A0 =A0 /* If we get here, it's an error. =A0*/ > =A0 =A0 =A0 =A0 =A0fprintf_unfiltered (gdb_stderr, "Cannot exec %s: %s.\n= ", shell_file, > =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0safe_strerror = (errno)); > - =A0 =A0 =A0 =A0 gdb_flush (gdb_stderr); > - =A0 =A0 =A0 =A0 _exit (0177); > =A0 =A0 =A0 =A0} > =A0 =A0 =A0 else > =A0 =A0 =A0 =A0{ > - =A0 =A0 =A0 =A0 /* Otherwise, we directly exec the target program with > - =A0 =A0 =A0 =A0 =A0 =A0execvp. =A0*/ > =A0 =A0 =A0 =A0 =A0int i; > > - =A0 =A0 =A0 =A0 execvp (exec_file, argv); > - > - =A0 =A0 =A0 =A0 /* If we get here, it's an error. =A0*/ > =A0 =A0 =A0 =A0 =A0safe_strerror (errno); > =A0 =A0 =A0 =A0 =A0fprintf_unfiltered (gdb_stderr, "Cannot exec %s ", exe= c_file); > > @@ -383,9 +381,9 @@ fork_inferior (char *exec_file_arg, char *allargs, ch= ar **env, > =A0 =A0 =A0 =A0 =A0 =A0 =A0i++; > =A0 =A0 =A0 =A0 =A0 =A0} > =A0 =A0 =A0 =A0 =A0fprintf_unfiltered (gdb_stderr, ".\n"); > - =A0 =A0 =A0 =A0 gdb_flush (gdb_stderr); > - =A0 =A0 =A0 =A0 _exit (0177); > =A0 =A0 =A0 =A0} > + =A0 =A0 =A0gdb_flush (gdb_stderr); > + =A0 =A0 =A0_exit (0177); > =A0 =A0 } > > =A0 /* Restore our environment in case a vforked child clob'd it. =A0*/ > Regards, Abhijit Halder