[RFC] Support of Lion (darwin 11)

[RFC] Support of Lion (darwin 11)

[Tristan Gingold]

Hi,

with the latest mac OS X, executables are now pie by default and their load address is random.  This obviously needs to be considered by gdb, and the easiest method is to disable the randomization.

Contrary to other OS, this is done at exec (i.e. posix_spawn) time.  In order to use posix_spawn instead of exec, I added a parameter to fork_inferior.  But to do that, I first slightly changed the fork_inferior code to always use execvp.  I also did some cleanup in this function:  use alloca instead of xmalloc for argv to avoid a memory leak, and moved the code that allocate shell_command within the if (shell) block.  I also factorized the gdb_flush/_exit code used in case on error.

The remaining of the patch is somewhat trivial: adjusting all the calls of fork_inferior (I preferred to pass NULL instead of execvp, because the prototype of the later is somewhat not universal), and doing the real work for darwin.

No regressions on i386 GNU/Linux,
manually tested on Lion.

Ok for trunk ?

Tristan.

2011-09-07  Tristan Gingold  <gingold@adacore.com>

	* fork-child.c (fork_inferior): Add exec_fun parameter.
	Update comment.  Use alloca instead of xmalloc for argv.  Move
	len and shell_command declarations in the block where they are used.
	Call exec_fun or execvp.  Factorize some failure code.
	* inferior.h: Adjust prototype.
	* gnu-nat.c (gnu_create_inferior): Adjsut fork_inferior call.
	* inf-ttrace.c (inf_ttrace_create_inferior): Ditto.
	* inf-ptrace.c (inf_ptrace_create_inferior): Ditto.
	* procfs.c (procfs_create_inferior): Ditto.
	* darwin-nat.c (darwin_execvp): New function.
	(darwin_create_inferior): Use it.

diff --git a/gdb/fork-child.c b/gdb/fork-child.c
index bb173e7..ce947c4 100644
--- a/gdb/fork-child.c
+++ b/gdb/fork-child.c
@@ -115,6 +115,7 @@ escape_bang_in_quoted_argument (const char *shell_file)
    pid.  EXEC_FILE is the file to run.  ALLARGS is a string containing
    the arguments to the program.  ENV is the environment vector to
    pass.  SHELL_FILE is the shell file, or NULL if we should pick
+   one.  EXEC_FUN is the exec(2) function to use, or NULL for the default
    one.  */
 
 /* This function is NOT reentrant.  Some of the variables have been
@@ -123,12 +124,12 @@ escape_bang_in_quoted_argument (const char *shell_file)
 int
 fork_inferior (char *exec_file_arg, char *allargs, char **env,
 	       void (*traceme_fun) (void), void (*init_trace_fun) (int),
-	       void (*pre_trace_fun) (void), char *shell_file_arg)
+	       void (*pre_trace_fun) (void), char *shell_file_arg,
+               void (*exec_fun)(const char *file, char * const *argv,
+                                char * const *env))
 {
   int pid;
-  char *shell_command;
   static char default_shell_file[] = SHELL_FILE;
-  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 +163,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 +171,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));
       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 +259,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 *));
+      argv[0] = shell_file;
+      argv[1] = "-c";
+      argv[2] = shell_command;
+      argv[3] = (char *) 0;
     }
 
   /* On some systems an exec will fail if the executable is open.  */
@@ -348,29 +360,21 @@ 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>".  */
+      if (exec_fun != NULL)
+        (*exec_fun) (argv[0], argv, env);
+      else
+        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 +387,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.  */
diff --git a/gdb/gnu-nat.c b/gdb/gnu-nat.c
index 7269694..32f78be 100644
--- a/gdb/gnu-nat.c
+++ b/gdb/gnu-nat.c
@@ -2114,7 +2114,8 @@ gnu_create_inferior (struct target_ops *ops,
 
   inf_debug (inf, "creating inferior");
 
-  pid = fork_inferior (exec_file, allargs, env, trace_me, NULL, NULL, NULL);
+  pid = fork_inferior (exec_file, allargs, env, trace_me,
+                       NULL, NULL, NULL, NULL);
 
   /* Attach to the now stopped child, which is actually a shell...  */
   inf_debug (inf, "attaching to child: %d", pid);
diff --git a/gdb/inf-ptrace.c b/gdb/inf-ptrace.c
index b5e1744..110e825 100644
--- a/gdb/inf-ptrace.c
+++ b/gdb/inf-ptrace.c
@@ -134,7 +134,7 @@ inf_ptrace_create_inferior (struct target_ops *ops,
     }
 
   pid = fork_inferior (exec_file, allargs, env, inf_ptrace_me, NULL,
-		       NULL, NULL);
+		       NULL, NULL, NULL);
 
   if (! ops_already_pushed)
     discard_cleanups (back_to);
diff --git a/gdb/inf-ttrace.c b/gdb/inf-ttrace.c
index ab075db..0ab6580 100644
--- a/gdb/inf-ttrace.c
+++ b/gdb/inf-ttrace.c
@@ -650,7 +650,7 @@ inf_ttrace_create_inferior (struct target_ops *ops, char *exec_file,
   gdb_assert (inf_ttrace_vfork_ppid == -1);
 
   pid = fork_inferior (exec_file, allargs, env, inf_ttrace_me, NULL,
-		       inf_ttrace_prepare, NULL);
+		       inf_ttrace_prepare, NULL, NULL);
 
   inf_ttrace_him (ops, pid);
 }
diff --git a/gdb/inferior.h b/gdb/inferior.h
index cf747a6..f5794fe 100644
--- a/gdb/inferior.h
+++ b/gdb/inferior.h
@@ -189,7 +189,9 @@ extern void terminal_init_inferior_with_pgrp (int pgrp);
 
 extern int fork_inferior (char *, char *, char **,
 			  void (*)(void),
-			  void (*)(int), void (*)(void), char *);
+			  void (*)(int), void (*)(void), char *,
+                          void (*)(const char *,
+                                   char * const *, char * const *));
 
 
 extern void startup_inferior (int);
diff --git a/gdb/procfs.c b/gdb/procfs.c
index 917e122..871dd47 100644
--- a/gdb/procfs.c
+++ b/gdb/procfs.c
@@ -4915,7 +4915,7 @@ procfs_create_inferior (struct target_ops *ops, char *exec_file,
     }
 
   pid = fork_inferior (exec_file, allargs, env, procfs_set_exec_trap,
-		       NULL, NULL, shell_file);
+		       NULL, NULL, shell_file, NULL);
 
   procfs_init_inferior (ops, pid);
 }
diff --git a/gdb/darwin-nat.c b/gdb/darwin-nat.c
index 06a1558..4e5eb42 100644
--- a/gdb/darwin-nat.c
+++ b/gdb/darwin-nat.c
@@ -53,6 +53,7 @@
 #include <sys/proc.h>
 #include <libproc.h>
 #include <sys/syscall.h>
+#include <spawn.h>
 
 #include <mach/mach_error.h>
 #include <mach/mach_vm.h>
@@ -1507,12 +1508,47 @@ darwin_ptrace_him (int pid)
 }
 
 static void
+darwin_execvp (const char *file, char * const argv[], char * const env[])
+{
+  posix_spawnattr_t attr;
+  short flags;
+  int retval;
+
+  retval = posix_spawnattr_init (&attr);
+  if (retval != 0)
+    {
+      warning ("Couldn't initialize attributes for posix_spawn, error: %d",
+               retval);
+      return;
+    }
+
+  /* Do like execve: replace the image.  */
+  flags = POSIX_SPAWN_SETEXEC;
+
+  /* Disable ASLR.  The constant doesn't look to be available outside the
+     kernel include files.  */
+#ifndef _POSIX_SPAWN_DISABLE_ASLR
+#define _POSIX_SPAWN_DISABLE_ASLR	0x0100
+#endif
+  flags |= _POSIX_SPAWN_DISABLE_ASLR;
+  retval = posix_spawnattr_setflags (&attr, flags);
+  if (retval != 0)
+    {
+      warning ("Couldn't set the posix_spawn flags, error: %d",
+               retval);
+      return;
+    }
+
+  posix_spawnp (NULL, argv[0], NULL, &attr, argv, env);
+}
+
+static void
 darwin_create_inferior (struct target_ops *ops, char *exec_file,
 			char *allargs, char **env, int from_tty)
 {
   /* Do the hard work.  */
   fork_inferior (exec_file, allargs, env, darwin_ptrace_me, darwin_ptrace_him,
-		 darwin_pre_ptrace, NULL);
+		 darwin_pre_ptrace, NULL, darwin_execvp);
 
   /* Return now in case of error.  */
   if (ptid_equal (inferior_ptid, null_ptid))

Re: [RFC] Support of Lion (darwin 11)

[Pedro Alves]

On Wednesday 07 September 2011 13:28:41, Tristan Gingold wrote:
> Hi,
> 
> with the latest mac OS X, executables are now pie by default and their load address is random.  This obviously needs to be considered by gdb, and the easiest method is to disable the randomization.

This leaves out attaching to already running programs.  We support PIE on
linux/svr4 now.  Would it be hard to impossible to support it on Lion?

> I also did some cleanup in this function:  use alloca instead of xmalloc for argv to avoid a memory leak, and moved the code that allocate shell_command within the if (shell) block.  I also factorized the gdb_flush/_exit code used in case on error.

How about splitting these out into a separate, preparatory patch?

-- 
Pedro Alves

Re: [RFC] Support of Lion (darwin 11)

[Pedro Alves]

On Wednesday 07 September 2011 14:09:06, Pedro Alves wrote:
> On Wednesday 07 September 2011 13:28:41, Tristan Gingold wrote:
> > Hi,
> > 
> > with the latest mac OS X, executables are now pie by default and their load address is random.  This obviously needs to be considered by gdb, and the easiest method is to disable the randomization.

I should add that being able to disable the randomization on its own is
an acceptable patch.  There's "set disable-randomization" on linux native
already to do the same thing, and it disables randomization by default.
You could re-use the same user visible setting.  It just so happens
that you'd get Lion half working for free.  :-)

> 
> This leaves out attaching to already running programs.  We support PIE on
> linux/svr4 now.  Would it be hard to impossible to support it on Lion?
> 
> > I also did some cleanup in this function:  use alloca instead of xmalloc for argv to avoid a memory leak, and moved the code that allocate shell_command within the if (shell) block.  I also factorized the gdb_flush/_exit code used in case on error.
> 
> How about splitting these out into a separate, preparatory patch?
> 
> 

-- 
Pedro Alves

Re: [RFC] Support of Lion (darwin 11)

[Tristan Gingold]

On Sep 7, 2011, at 3:09 PM, Pedro Alves wrote:

> On Wednesday 07 September 2011 13:28:41, Tristan Gingold wrote:
>> Hi,
>> 
>> with the latest mac OS X, executables are now pie by default and their load address is random.  This obviously needs to be considered by gdb, and the easiest method is to disable the randomization.
> 
> This leaves out attaching to already running programs.  We support PIE on
> linux/svr4 now.

Correct.  But handling attached PIE processes is a different problem.


>  Would it be hard to impossible to support it on Lion?

No, it isn't impossible.  It is just more difficult than PIE executables.

>> I also did some cleanup in this function:  use alloca instead of xmalloc for argv to avoid a memory leak, and moved the code that allocate shell_command within the if (shell) block.  I also factorized the gdb_flush/_exit code used in case on error.
> 
> How about splitting these out into a separate, preparatory patch?

Sure.

Tristan.

[RFA] Preliminary work in fork_inferior

[Tristan Gingold]

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;
-  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));
       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 *));
+      argv[0] = shell_file;
+      argv[1] = "-c";
+      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.  */

Re: [RFA] Preliminary work in fork_inferior

[Pedro Alves]

Thanks, this is okay.

On Friday 16 September 2011 13:28:59, Tristan Gingold wrote:
> +      /* 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));

I wouldn't mind merging the "Cannot exec" bits of both branches as well,
always printing the whole argv.

> -         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);

This obviously isn't doing what was intended though...

>           fprintf_unfiltered (gdb_stderr, "Cannot exec %s ", exec_file);

-- 
Pedro Alves

Re: [RFA] Preliminary work in fork_inferior

[Abhijit Halder]

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

Re: [RFA] Preliminary work in fork_inferior

[Tristan Gingold]

On Sep 16, 2011, at 3:19 PM, Pedro Alves wrote:

> Thanks, this is okay.
> 
> On Friday 16 September 2011 13:28:59, Tristan Gingold wrote:
>> +      /* 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));
> 
> I wouldn't mind merging the "Cannot exec" bits of both branches as well,
> always printing the whole argv.

Ok, so more cleanup.  Something like that ?

Tristan.

diff --git a/gdb/fork-child.c b/gdb/fork-child.c
index bb173e7..5dbf1f7 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;
-  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
@@ -141,6 +139,8 @@ fork_inferior (char *exec_file_arg, char *allargs, char **env,
   static char **argv;
   const char *inferior_io_terminal = get_inferior_io_terminal ();
   struct inferior *inf;
+  int i;
+  int save_errno;
 
   /* If no exec file handed to us, get it from the exec-file command
      -- with a good, common error message if none is specified.  */
@@ -162,16 +162,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 +170,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));
       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 +258,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 *));
+      argv[0] = shell_file;
+      argv[1] = "-c";
+      argv[2] = shell_command;
+      argv[3] = (char *) 0;
     }
 
   /* On some systems an exec will fail if the executable is open.  */
@@ -348,44 +359,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>".  */
-      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);
-
-	  i = 1;
-	  while (argv[i] != NULL)
-	    {
-	      if (i != 1)
-		fprintf_unfiltered (gdb_stderr, " ");
-	      fprintf_unfiltered (gdb_stderr, "%s", argv[i]);
-	      i++;
-	    }
-	  fprintf_unfiltered (gdb_stderr, ".\n");
-	  gdb_flush (gdb_stderr);
-	  _exit (0177);
-	}
+      execvp (argv[0], argv);
+
+      /* If we get here, it's an error.  */
+      save_errno = errno;
+      fprintf_unfiltered (gdb_stderr, "Cannot exec %s", exec_file);
+      for (i = 1; argv[i] != NULL; i++)
+	fprintf_unfiltered (gdb_stderr, " %s", argv[i]);
+      fprintf_unfiltered (gdb_stderr, ".\n");
+      fprintf_unfiltered (gdb_stderr, "Error: %s\n",
+			  safe_strerror (save_errno));
+      gdb_flush (gdb_stderr);
+      _exit (0177);
     }
 
   /* Restore our environment in case a vforked child clob'd it.  */

Re: [RFA] Preliminary work in fork_inferior

[Tristan Gingold]

On Sep 16, 2011, at 4:43 PM, Abhijit Halder wrote:

> On Fri, Sep 16, 2011 at 5:58 PM, Tristan Gingold <gingold@adacore.com> wrote:
[…]
>> --- 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.

Well, given that this file deals with spawning via [v]fork+exec it won't work at all for MinGW :-)

>> -      argv = (char **) xmalloc (argc * sizeof (*argv));
>> +      argv = (char **) alloca (argc * sizeof (*argv));
> alloca is not a portable function I guess.

Although alloca is not part of ANSI-C, it is heavily used by GNU software and available almost everywhere.
There is even a replacement alloca function in libiberty but I am pretty sure it wasn't used for a long time.

Tristan.

Re: [RFA] Preliminary work in fork_inferior

[Pedro Alves]

On Friday 16 September 2011 15:47:19, Tristan Gingold wrote:
> 
> On Sep 16, 2011, at 3:19 PM, Pedro Alves wrote:
> 
> > Thanks, this is okay.
> > 
> > On Friday 16 September 2011 13:28:59, Tristan Gingold wrote:
> >> +      /* 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));
> > 
> > I wouldn't mind merging the "Cannot exec" bits of both branches as well,
> > always printing the whole argv.
> 
> Ok, so more cleanup.  Something like that ?

Looks greak, thanks!  Please apply.

-- 
Pedro Alves

Re: [RFA] Preliminary work in fork_inferior

[Tristan Gingold]

On Sep 16, 2011, at 4:57 PM, Pedro Alves wrote:

> On Friday 16 September 2011 15:47:19, Tristan Gingold wrote:
>> 
>> On Sep 16, 2011, at 3:19 PM, Pedro Alves wrote:
>> 
>>> Thanks, this is okay.
>>> 
>>> On Friday 16 September 2011 13:28:59, Tristan Gingold wrote:
>>>> +      /* 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));
>>> 
>>> I wouldn't mind merging the "Cannot exec" bits of both branches as well,
>>> always printing the whole argv.
>> 
>> Ok, so more cleanup.  Something like that ?
> 
> Looks greak, thanks!  Please apply.

Thanks, now committed.

Tristan.