Re: [RFA] Preliminary work in fork_inferior

[Abhijit Halder <abhijit.k.halder@gmail.com>]

On Fri, Sep 16, 2011 at 5:58 PM, Tristan Gingold <gingold@adacore.com> wrote:
> 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  Tristan Gingold  <gingold@adacore.com>
>
>        * fork-child.c (fork_inferior): Update comment.
>        Use alloca instead of xmalloc for argv.  Move
>        len and shell_command declarations in the block where they are used.
>        Only call execvp.  Factorize 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, char **env,
>               void (*pre_trace_fun) (void), char *shell_file_arg)
>  {
>   int pid;
> -  char *shell_command;
>   static char default_shell_file[] = 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.
> -  int len;
>   /* Set debug_fork then attach to the child while it sleeps, to debug.  */
>   static int debug_fork = 0;
>   /* This is set to the result of setpgrp, which if vforked, will be visible
> @@ -162,16 +160,6 @@ fork_inferior (char *exec_file_arg, char *allargs, char **env,
>       shell = 1;
>     }
>
> -  /* Multiplying the length of exec_file by 4 is to account for the
> -     fact that it may expand when quoted; it is a worst-case number
> -     based on every character being '.  */
> -  len = 5 + 4 * strlen (exec_file) + 1 + strlen (allargs) + 1 + /*slop */ 12;
> -  if (exec_wrapper)
> -    len += strlen (exec_wrapper) + 1;
> -
> -  shell_command = (char *) alloca (len);
> -  shell_command[0] = '\0';
> -
>   if (!shell)
>     {
>       /* We're going to call execvp.  Create argument vector.
> @@ -180,18 +168,29 @@ fork_inferior (char *exec_file_arg, char *allargs, char **env,
>         argument.  */
>       int argc = (strlen (allargs) + 1) / 2 + 2;
>
> -      argv = (char **) xmalloc (argc * sizeof (*argv));
> +      argv = (char **) alloca (argc * sizeof (*argv));
alloca is not a portable function I guess.
>       argv[0] = exec_file;
>       breakup_args (allargs, &argv[1]);
>     }
>   else
>     {
>       /* We're going to call a shell.  */
> -
> +      char *shell_command;
> +      int len;
>       char *p;
>       int need_to_quote;
>       const int escape_bang = escape_bang_in_quoted_argument (shell_file);
>
> +      /* Multiplying the length of exec_file by 4 is to account for the
> +         fact that it may expand when quoted; it is a worst-case number
> +         based on every character being '.  */
> +      len = 5 + 4 * strlen (exec_file) + 1 + strlen (allargs) + 1 + /*slop */ 12;
> +      if (exec_wrapper)
> +        len += strlen (exec_wrapper) + 1;
> +
> +      shell_command = (char *) alloca (len);
> +      shell_command[0] = '\0';
> +
>       strcat (shell_command, "exec ");
>
>       /* Add any exec wrapper.  That may be a program name with arguments, so
> @@ -257,6 +256,16 @@ fork_inferior (char *exec_file_arg, char *allargs, char **env,
>
>       strcat (shell_command, " ");
>       strcat (shell_command, allargs);
> +
> +      /* If we decided above to start up with a shell, we exec the
> +        shell, "-c" says to interpret the next arg as a shell command
> +        to execute, and this command is "exec <target-program>
> +        <args>".  */
> +      argv = (char **) alloca (4 * sizeof (char *));
Same comment about alloca as above.
> +      argv[0] = shell_file;
> +      argv[1] = "-c";
For MinGW the -c switch will not work.
> +      argv[2] = shell_command;
> +      argv[3] = (char *) 0;
>     }
>
>   /* On some systems an exec will fail if the executable is open.  */
> @@ -348,29 +357,18 @@ fork_inferior (char *exec_file_arg, char *allargs, char **env,
>          path to find $SHELL.  Rich Pixley says so, and I agree.  */
>       environ = env;
>
> -      /* If we decided above to start up with a shell, we exec the
> -        shell, "-c" says to interpret the next arg as a shell command
> -        to execute, and this command is "exec <target-program>
> -        <args>".  */
> +      execvp (argv[0], argv);
> +
> +      /* If we get here, it's an error.  */
>       if (shell)
>        {
> -         execlp (shell_file, shell_file, "-c", shell_command, (char *) 0);
> -
> -         /* If we get here, it's an error.  */
>          fprintf_unfiltered (gdb_stderr, "Cannot exec %s: %s.\n", shell_file,
>                              safe_strerror (errno));
> -         gdb_flush (gdb_stderr);
> -         _exit (0177);
>        }
>       else
>        {
> -         /* Otherwise, we directly exec the target program with
> -            execvp.  */
>          int i;
>
> -         execvp (exec_file, argv);
> -
> -         /* If we get here, it's an error.  */
>          safe_strerror (errno);
>          fprintf_unfiltered (gdb_stderr, "Cannot exec %s ", exec_file);
>
> @@ -383,9 +381,9 @@ fork_inferior (char *exec_file_arg, char *allargs, char **env,
>              i++;
>            }
>          fprintf_unfiltered (gdb_stderr, ".\n");
> -         gdb_flush (gdb_stderr);
> -         _exit (0177);
>        }
> +      gdb_flush (gdb_stderr);
> +      _exit (0177);
>     }
>
>   /* Restore our environment in case a vforked child clob'd it.  */
>

Regards,
Abhijit Halder