Re: [PATCH] gdb: sim: automatically pass down sysroot

Re: [PATCH] gdb: sim: automatically pass down sysroot

[Tom Tromey]

>>>>> "Mike" == Mike Frysinger <vapier@gentoo.org> writes:

Mike> 2011-05-26  Mike Frysinger  <vapier@gentoo.org>
Mike> 	* remote-sim.c (gdbsim_open): Add the strlen of " --sysroot=" and
Mike> 	gdb_sysroot to the "len" variable.  Append both to "arg_buf".

It seems reasonable to me.  Ok.

Mike> +  strcat (arg_buf, gdb_sysroot);

It seems like this will give wrong results if the sysroot needs quoting.
But, this problem could affect other arguments here.
Offhand it seems like it would be better if this code directly built
sim_argv rather than building a string and converting it, but I don't
know if any odd issue is lurking.

Tom

Re: [PATCH] gdb: sim: automatically pass down sysroot

[Mike Frysinger]

On Fri, May 27, 2011 at 13:46, Tom Tromey wrote:
> Mike> +  strcat (arg_buf, gdb_sysroot);
>
> It seems like this will give wrong results if the sysroot needs quoting.
> But, this problem could affect other arguments here.
> Offhand it seems like it would be better if this code directly built
> sim_argv rather than building a string and converting it, but I don't
> know if any odd issue is lurking.

i thought of that, but since as you say other parts have the same
issue, and i couldnt find a helper func that would sanely take care of
things, i didnt bother
-mike

[PATCH/RFC] new argv handlers to help with sim argv building

[Mike Frysinger]

[-- Attachment #1: Type: Text/Plain, Size: 4228 bytes --]

On Friday, May 27, 2011 13:46:59 Tom Tromey wrote:
> Offhand it seems like it would be better if this code directly built
> sim_argv rather than building a string and converting it, but I don't
> know if any odd issue is lurking.

i slapped this together.  what do you think (ignoring missing docs) ?
-mike

--- libiberty/argv.c	13 Aug 2010 11:36:10 -0000	1.22
+++ libiberty/argv.c	1 Jun 2011 16:17:06 -0000
@@ -301,6 +301,56 @@ char **buildargv (const char *input)
   return (argv);
 }
 
+char **appendargv (char **argv, const char *arg, ...)
+{
+  va_list ap;
+  int argc;
+
+  for (argc = 0; argv[argc]; ++argc)
+    continue;
+
+  va_start (ap, arg);
+  while (arg)
+    {
+      argv = (char **) realloc (argv, sizeof (*argv) * (argc + 2));
+      argv[argc] = strdup (arg);
+      ++argc;
+      arg = va_arg (ap, const char *);
+    }
+  va_end (ap);
+
+  if (argv)
+    argv[argc] = NULL;
+
+  return argv;
+}
+
+char **mergeargv (char **iargv, ...)
+{
+  va_list ap;
+  char **argv = NULL;
+  int i, argc = 0;
+
+  va_start (ap, iargv);
+  while (iargv)
+    {
+      for (i = 0; iargv[i]; ++i)
+	{
+	  argv = (char **) realloc (argv, sizeof (*argv) * (argc + 2));
+	  argv[argc] = strdup (iargv[i]);
+	  ++argc;
+	}
+
+      iargv = va_arg (ap, char **);
+    }
+  va_end (ap);
+
+  if (argv)
+    argv[argc] = NULL;
+
+  return argv;
+}
+
 /*
 
 @deftypefn Extension int writeargv (const char **@var{argv}, FILE *@var{file})
--- include/libiberty.h	3 Jan 2011 21:05:50 -0000	1.63
+++ include/libiberty.h	1 Jun 2011 16:17:06 -0000
@@ -74,6 +74,9 @@ extern FILE *freopen_unlocked (const cha
 
 extern char **buildargv (const char *) ATTRIBUTE_MALLOC;
 
+extern char **appendargv (char **, const char *, ...) ATTRIBUTE_MALLOC;
+extern char **mergeargv (char **iargv, ...) ATTRIBUTE_MALLOC;
+
 /* Free a vector returned by buildargv.  */
 
 extern void freeargv (char **);
--- gdb/remote-sim.c	1 Jun 2011 15:29:07 -0000	1.103
+++ gdb/remote-sim.c	1 Jun 2011 16:17:06 -0000
@@ -664,7 +664,7 @@ static void
 gdbsim_open (char *args, int from_tty)
 {
   int len;
-  char *arg_buf;
+  char **argv;
   struct sim_inferior_data *sim_data;
   SIM_DESC gdbsim_desc;
 
@@ -681,44 +681,43 @@ gdbsim_open (char *args, int from_tty)
   if (gdbsim_is_open)
     unpush_target (&gdbsim_ops);
 
-  len = (7 + 1			/* gdbsim */
-	 + strlen (" -E little")
-	 + strlen (" --architecture=xxxxxxxxxx")
-	 + strlen (" --sysroot=") + strlen (gdb_sysroot) +
-	 + (args ? strlen (args) : 0)
-	 + 50) /* slack */ ;
-  arg_buf = (char *) alloca (len);
-  strcpy (arg_buf, "gdbsim");	/* 7 */
+  argv = buildargv ("gdbsim");
+
   /* Specify the byte order for the target when it is explicitly
      specified by the user (not auto detected).  */
   switch (selected_byte_order ())
     {
     case BFD_ENDIAN_BIG:
-      strcat (arg_buf, " -E big");
+      argv = appendargv (argv, "-E", "big", NULL);
       break;
     case BFD_ENDIAN_LITTLE:
-      strcat (arg_buf, " -E little");
+      argv = appendargv (argv, "-E", "little", NULL);
       break;
     case BFD_ENDIAN_UNKNOWN:
       break;
     }
+
   /* Specify the architecture of the target when it has been
      explicitly specified */
   if (selected_architecture_name () != NULL)
-    {
-      strcat (arg_buf, " --architecture=");
-      strcat (arg_buf, selected_architecture_name ());
-    }
+    argv = appendargv (argv, "--architecture",
+			selected_architecture_name (), NULL);
+
   /* Pass along gdb's concept of the sysroot.  */
-  strcat (arg_buf, " --sysroot=");
-  strcat (arg_buf, gdb_sysroot);
+  argv = appendargv (argv, "--sysroot", gdb_sysroot, NULL);
+
   /* finally, any explicit args */
   if (args)
     {
-      strcat (arg_buf, " ");	/* 1 */
-      strcat (arg_buf, args);
+      char **uargv;
+
+      uargv = gdb_buildargv (args);
+      sim_argv = mergeargv (argv, uargv, NULL);
+      freeargv (argv);
+      freeargv (uargv);
     }
-  sim_argv = gdb_buildargv (arg_buf);
+  else
+    sim_argv = argv;
 
   init_callbacks ();
   gdbsim_desc = sim_open (SIM_OPEN_DEBUG, &gdb_callback, exec_bfd, sim_argv);

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH/RFC] new argv handlers to help with sim argv building

[Tom Tromey]

>>>>> "Mike" == Mike Frysinger <vapier@gentoo.org> writes:

Mike> i slapped this together.  what do you think (ignoring missing docs) ?

It seems reasonable to me overall, though see below.

Mike> +char **appendargv (char **argv, const char *arg, ...)

Wrong formatting.

The canonical source for libiberty is gcc, so changes have to go there.

Mike> +  argv = appendargv (argv, "--sysroot", gdb_sysroot, NULL);
[...]
Mike> +      uargv = gdb_buildargv (args);
Mike> +      sim_argv = mergeargv (argv, uargv, NULL);
Mike> +      freeargv (argv);

If there is only one argument potentially needing quoting from
buildargv, and we have to do splitting and merging as well, then it
seems like it would be simpler to just add a quoting function and use
that.

Tom

Re: [PATCH/RFC] new argv handlers to help with sim argv building

[Mike Frysinger]

[-- Attachment #1: Type: Text/Plain, Size: 1032 bytes --]

On Friday, June 03, 2011 12:56:24 Tom Tromey wrote:
> >>>>> "Mike" == Mike Frysinger <vapier@gentoo.org> writes:
> Mike> +char **appendargv (char **argv, const char *arg, ...)
> 
> Wrong formatting.

i dont know what you mean

> The canonical source for libiberty is gcc, so changes have to go there.

yes, but i'm just bouncing the idea right now

> Mike> +  argv = appendargv (argv, "--sysroot", gdb_sysroot, NULL);
> [...]
> Mike> +      uargv = gdb_buildargv (args);
> Mike> +      sim_argv = mergeargv (argv, uargv, NULL);
> Mike> +      freeargv (argv);
> 
> If there is only one argument potentially needing quoting from
> buildargv, and we have to do splitting and merging as well, then it
> seems like it would be simpler to just add a quoting function and use
> that.

quoting opens a whole can of worms i dont want to think about.  simply 
sticking quotes around gdb_sysroot doesnt solve the whole problem.  what if 
the sysroot has quotes in it ?  i'd prefer to stick in the argv world.
-mike

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 836 bytes --]