[Fwd: Re: Patch to support spaces in filenames & paths]

[Fwd: Re: Patch to support spaces in filenames & paths]

[Denis PILAT]

[-- Attachment #1: Type: text/plain, Size: 7 bytes --]

Ping !

[-- Attachment #2: Re: Patch to support spaces in filenames & paths.eml --]
[-- Type: message/rfc822, Size: 14910 bytes --]

[-- Attachment #2.1.1: Type: text/plain, Size: 1929 bytes --]


Daniel Jacobowitz wrote:
> On Wed, Dec 10, 2008 at 04:31:48PM +0100, Denis PILAT wrote:
>   
>> I made some experiments with parse_to_comma_and_eval, but I think I must  
>> be sure to understand what you need before going further in my  
>> implementation, and summarize the situation
>>     
>
> parse_to_comma_and_eval is only required for functions that take more
> than one expression.  This doesn't, so it doesn't apply.
>
> I see that buildargv is going to be a problem if the last argument is
> a free-form expression.  I'm not entirely sure what to do about that.
> Maybe a GDB-local version that extracts one argument string at a time
> and returns that, plus a pointer to the next unparsed bit of the
> command line.
>
> The important thing, in my opinion, is to use the same handling of
> quotes and spaces that buildargv does for consistency with other
> commands, like "file".
>
> Maybe we can solve this later and just use buildargv.  Compare with
> add_symbol_file_command which takes an address.
>
>   
Hi Daniel,

I'm fine with what you said.
I read your comments and produce a new patch that uses gdb_buildargv (as 
Pedro suggested), inspired from add_symbol_file_command as well.
That fixes the problem of last argument that could be a free form. Now 
all the last arguments, are concatenate into a single one.
It avoids as well any possible regression since it behaves exactly the 
same as the previous gdb, except that it accepts arguments with spaces 
like file command does.
And it consistent with most of commands that accept spaces in their 
arguments.

I also removed lot of dead code in cli/cli-dump.c and fopen_with_cleanup 
is now used only locally, so removed from cli-dump.h.

Dump restore and append commands are impacted by this patch.

As the "binary" option was missing from the help message, I add it as 
well into help.exp in a separate patch.

Patch are attached.
Ok for commit ?

Denis



[-- Attachment #2.1.2: cli-dump.patch --]
[-- Type: text/plain, Size: 9404 bytes --]

2009-01-05  Denis Pilat  <denis.pilat@st.com>

	* cli/cli-dump.h (scan_filename_with_cleanup, skip_spaces,
	fopen_with_cleanup, parse_and_eval_with_error,
	scan_expression_with_cleanup): Remove.
	* cli/cli-dump.c: Likewise.
	(dump_memory_to_file): use of gdb_buildargv for argument parsing.
	Concatenate last arguments on the command line.
	(dump_value_to_file, restore_command): Likewise.
	(_initialize_cli_dump): add "binary" option for the help message of 
	restore command.


Index: cli/cli-dump.c
===================================================================
RCS file: /cvs/src/src/gdb/cli/cli-dump.c,v
retrieving revision 1.30
diff -u -p -r1.30 cli-dump.c
--- cli/cli-dump.c	3 Jan 2009 05:57:54 -0000	1.30
+++ cli/cli-dump.c	5 Jan 2009 14:14:36 -0000
@@ -33,75 +33,6 @@
 
 #define XMALLOC(TYPE) ((TYPE*) xmalloc (sizeof (TYPE)))
 
-
-char *
-skip_spaces (char *chp)
-{
-  if (chp == NULL)
-    return NULL;
-  while (isspace (*chp))
-    chp++;
-  return chp;
-}
-
-char *
-scan_expression_with_cleanup (char **cmd, const char *def)
-{
-  if ((*cmd) == NULL || (**cmd) == '\0')
-    {
-      char *exp = xstrdup (def);
-      make_cleanup (xfree, exp);
-      return exp;
-    }
-  else
-    {
-      char *exp;
-      char *end;
-
-      end = (*cmd) + strcspn (*cmd, " \t");
-      exp = savestring ((*cmd), end - (*cmd));
-      make_cleanup (xfree, exp);
-      (*cmd) = skip_spaces (end);
-      return exp;
-    }
-}
-
-
-char *
-scan_filename_with_cleanup (char **cmd, const char *defname)
-{
-  char *filename;
-  char *fullname;
-
-  /* FIXME: Need to get the ``/a(ppend)'' flag from somewhere.  */
-
-  /* File.  */
-  if ((*cmd) == NULL)
-    {
-      if (defname == NULL)
-	error (_("Missing filename."));
-      filename = xstrdup (defname);
-      make_cleanup (xfree, filename);
-    }
-  else
-    {
-      /* FIXME: should parse a possibly quoted string.  */
-      char *end;
-
-      (*cmd) = skip_spaces (*cmd);
-      end = *cmd + strcspn (*cmd, " \t");
-      filename = savestring ((*cmd), end - (*cmd));
-      make_cleanup (xfree, filename);
-      (*cmd) = skip_spaces (end);
-    }
-  gdb_assert (filename != NULL);
-
-  fullname = tilde_expand (filename);
-  make_cleanup (xfree, fullname);
-  
-  return fullname;
-}
-
 FILE *
 fopen_with_cleanup (const char *filename, const char *mode)
 {
@@ -222,24 +153,35 @@ dump_memory_to_file (char *cmd, char *mo
   char *lo_exp;
   char *hi_exp;
   int len;
+  char ** argv;
+  int argcnt;
 
-  /* Open the file.  */
-  filename = scan_filename_with_cleanup (&cmd, NULL);
+  argv = gdb_buildargv (cmd);
+  make_cleanup_freeargv (argv);
 
+  /* Open the file.  */
+  if (argv == NULL || argv[0] == NULL)
+    error (_("Missing filename."));
+  filename = tilde_expand (argv[0]);
+  make_cleanup (xfree, filename);
+  
   /* Find the low address.  */
-  if (cmd == NULL || *cmd == '\0')
+  lo_exp = argv[1];
+  if (lo_exp == NULL)
     error (_("Missing start address."));
-  lo_exp = scan_expression_with_cleanup (&cmd, NULL);
 
   /* Find the second address - rest of line.  */
-  if (cmd == NULL || *cmd == '\0')
+  hi_exp = argv[2];
+  if (hi_exp == NULL)
     error (_("Missing stop address."));
-  hi_exp = cmd;
+  for (argcnt = 3; argv[argcnt] != NULL; argcnt++)
+    hi_exp = concat (hi_exp, " ", argv[argcnt], (char *)NULL);
 
   lo = parse_and_eval_address (lo_exp);
   hi = parse_and_eval_address (hi_exp);
   if (hi <= lo)
     error (_("Invalid memory address range (start >= end)."));
+
   count = hi - lo;
 
   /* FIXME: Should use read_memory_partial() and a magic blocking
@@ -273,14 +215,28 @@ dump_value_to_file (char *cmd, char *mod
   struct cleanup *old_cleanups = make_cleanup (null_cleanup, NULL);
   struct value *val;
   char *filename;
+  char ** argv;
+  char *strvalue;
+  int argcnt;
 
-  /* Open the file.  */
-  filename = scan_filename_with_cleanup (&cmd, NULL);
+  argv = gdb_buildargv (cmd);
+  make_cleanup_freeargv (argv);
 
-  /* Find the value.  */
-  if (cmd == NULL || *cmd == '\0')
+  /* Open the file.  */
+  if (argv == NULL || argv[0] == NULL)
+    error (_("Missing filename."));
+  filename = tilde_expand (argv[0]);
+  make_cleanup (xfree, filename);
+
+  /* Find the value - rest of line.  */
+  strvalue = argv[1];
+  if (strvalue == NULL)
     error (_("No value to %s."), *mode == 'a' ? "append" : "dump");
-  val = parse_and_eval (cmd);
+
+  for (argcnt = 2; argv[argcnt] != NULL; argcnt++)
+    strvalue = concat (strvalue, " ", argv[argcnt], (char *)NULL);
+
+  val = parse_and_eval (strvalue);
   if (val == NULL)
     error (_("Invalid expression."));
 
@@ -559,8 +515,14 @@ restore_command (char *args, int from_tt
   char *filename;
   struct callback_data data;
   bfd *ibfd;
-  int binary_flag = 0;
-
+  char **argv;
+  int binary_flag;
+  char *arg;
+  int arg_num;
+  char *end_addr = NULL;
+  filename = NULL;
+  binary_flag = 0;
+  
   if (!target_has_execution)
     noprocess ();
 
@@ -568,43 +530,54 @@ restore_command (char *args, int from_tt
   data.load_start  = 0;
   data.load_end    = 0;
 
-  /* Parse the input arguments.  First is filename (required). */
-  filename = scan_filename_with_cleanup (&args, NULL);
-  if (args != NULL && *args != '\0')
-    {
-      char *binary_string = "binary";
+  argv = gdb_buildargv (args);
+  make_cleanup_freeargv (argv);
 
-      /* Look for optional "binary" flag.  */
-      if (strncmp (args, binary_string, strlen (binary_string)) == 0)
-	{
-	  binary_flag = 1;
-	  args += strlen (binary_string);
-	  args = skip_spaces (args);
-	}
-      /* Parse offset (optional). */
-      if (args != NULL && *args != '\0')
-      data.load_offset = 
-	parse_and_eval_long (scan_expression_with_cleanup (&args, NULL));
-      if (args != NULL && *args != '\0')
-	{
-	  /* Parse start address (optional). */
-	  data.load_start = 
-	    parse_and_eval_long (scan_expression_with_cleanup (&args, NULL));
-	  if (args != NULL && *args != '\0')
-	    {
-	      /* Parse end address (optional). */
-	      data.load_end = parse_and_eval_long (args);
-	      if (data.load_end <= data.load_start)
-		error (_("Start must be less than end."));
-	    }
-	}
+  /* At least filename argument is required.  */
+  if (argv == NULL || argv[0] == NULL)
+    error (_("Missing filename as the first argument."));
+
+  /* The first argument is the file name.  */
+  filename = tilde_expand (argv[0]);
+  make_cleanup (xfree, filename);
+
+  /* Look for optionals arguments from here: "binary", offset, 
+   start and end address.  */
+  if (argv[1] != NULL && strcmp (argv[1], "binary") == 0) 
+        binary_flag = 1;
+
+  arg_num = 1 + binary_flag;
+  for (arg = argv[arg_num]; arg != NULL; arg = argv[++arg_num])
+    {
+      /* parse offset argument (optional).  */
+      if (arg_num == 1 + binary_flag)
+          data.load_offset = parse_and_eval_long (arg);
+      
+      /* parse start address argument (optional).  */
+      else if (arg_num == 2 + binary_flag)
+        data.load_start = parse_and_eval_long (arg);
+  
+      /* Rest of line is considered as end address optional argument.  */
+      else if (arg_num == 3 + binary_flag )
+        end_addr = arg;
+      else if (arg_num > 3 + binary_flag)
+        end_addr = concat (end_addr, " ", arg, (char *)NULL);
+    }      
+
+  /* If end address argument is given, parse it and test it's greater than 
+     the start address.  */
+  if (end_addr != NULL)
+    {
+      data.load_end = parse_and_eval_long (end_addr);
+      if (data.load_end <= data.load_start)
+        error (_("Start must be less than end."));
     }
 
   if (info_verbose)
     printf_filtered ("Restore file %s offset 0x%lx start 0x%lx end 0x%lx\n",
-		     filename, (unsigned long) data.load_offset, 
-		     (unsigned long) data.load_start, 
-		     (unsigned long) data.load_end);
+                     filename, (unsigned long) data.load_offset, 
+                     (unsigned long) data.load_start, 
+                     (unsigned long) data.load_end);
 
   if (binary_flag)
     {
@@ -775,7 +748,8 @@ to the specified FILE in raw target orde
 
   c = add_com ("restore", class_vars, restore_command, _("\
 Restore the contents of FILE to target memory.\n\
-Arguments are FILE OFFSET START END where all except FILE are optional.\n\
+Arguments are FILE \"binary\" OFFSET START END\n\
+where all except FILE are optional.\n\
 OFFSET will be added to the base address of the file (default zero).\n\
 If START and END are given, only the file contents within that range\n\
 (file relative) will be restored to target memory."));
Index: cli/cli-dump.h
===================================================================
RCS file: /cvs/src/src/gdb/cli/cli-dump.h,v
retrieving revision 1.7
diff -u -p -r1.7 cli-dump.h
--- cli/cli-dump.h	3 Jan 2009 05:57:54 -0000	1.7
+++ cli/cli-dump.h	5 Jan 2009 14:14:36 -0000
@@ -24,15 +24,4 @@ extern void add_dump_command (char *name
 			      void (*func) (char *args, char *mode),
 			      char *descr);
 
-/* Utilities for doing the dump.  */
-extern char *scan_filename_with_cleanup (char **cmd, const char *defname);
-
-extern char *scan_expression_with_cleanup (char **cmd, const char *defname);
-
-extern FILE *fopen_with_cleanup (const char *filename, const char *mode);
-
-extern char *skip_spaces (char *inp);
-
-extern struct value *parse_and_eval_with_error (char *exp, const char *fmt, ...) ATTR_FORMAT (printf, 2, 3);
-
 #endif

[-- Attachment #2.1.3: help.exp.patch --]
[-- Type: text/plain, Size: 1738 bytes --]

2009-01-05  Denis Pilat  <denis.pilat@st.com>

	* gdb.base/help.exp: add "binary" option for restore command.

Index: gdb.base/help.exp
===================================================================
RCS file: /cvs/src/src/gdb/testsuite/gdb.base/help.exp,v
retrieving revision 1.31
diff -u -p -r1.31 help.exp
--- gdb.base/help.exp	16 Nov 2008 18:03:25 -0000	1.31
+++ gdb.base/help.exp	5 Jan 2009 14:02:44 -0000
@@ -355,7 +355,7 @@ gdb_test "help run" "Start debugged prog
 # test help rbreak
 gdb_test "help rbreak" "Set a breakpoint for all functions matching REGEXP\." "help rbreak"
 # test help restore
-gdb_test "help restore" "Restore the contents of FILE to target memory\.\[\r\n\]+Arguments are FILE OFFSET START END where all except FILE are optional\.\[\r\n\]+OFFSET will be added to the base address of the file \\(default zero\\)\.\[\r\n\]+If START and END are given, only the file contents within that range\[\r\n\]+\\(file relative\\) will be restored to target memory\."
+gdb_test "help restore" "Restore the contents of FILE to target memory\.\[\r\n\]+Arguments are FILE \"binary\" OFFSET START END\[\r\n\]+where all except FILE are optional\.\[\r\n\]+OFFSET will be added to the base address of the file \\(default zero\\)\.\[\r\n\]+If START and END are given, only the file contents within that range\[\r\n\]+\\(file relative\\) will be restored to target memory\."
 # test help return
 gdb_test "help return" "Make selected stack frame return to its caller\.\[\r\n\]+Control remains in the debugger, but when you continue\[\r\n\]+execution will resume in the frame above the one now selected\.\[\r\n\]+If an argument is given, it is an expression for the value to return\." "help return"
 # test help reverse-search

Re: [Fwd: Re: Patch to support spaces in filenames & paths]

[Tom Tromey]

>>>>> "Denis" == Denis PILAT <denis.pilat@st.com> writes:

Denis> Ping !

Sorry for the delay on this.

I read both the patches for this issue, plus the thread on the mailing
list.

Denis> I read your comments and produce a new patch that uses gdb_buildargv
Denis> (as Pedro suggested), inspired from add_symbol_file_command as well.

I don't like this approach.

With the current patch, we will accept a quoted expression.  This
seems very ugly to me.  I wouldn't be surprised if completion failed
in this case.  And, given that I would not support a new command using
a quoting scheme like this, it seems odd to allow one via refactoring.

I think this problem can be addressed to some degree by more carefully
parsing the address arguments.

Second, concatenating the final arguments using a space is not
completely correct, because it does not preserve the meaning of all
possible expressions.  This is a pedantic point, since such
expressions are unlikely to be used here.


In the long term, I would like to see gdb move toward a more uniform
approach to parsing its arguments, and to allowing full expressions in
more places.  I don't think gdb_buildargv can be the foundation for
that, though.


Since there only seems to be a single argument that has a problem
here, how about what Daniel suggested: a function that uses the same
quoting rules as gdb_buildargv, but which only peels off a single
argument?

A few notes on the patch itself follow.

Denis> I also removed lot of dead code in cli/cli-dump.c and
Denis> fopen_with_cleanup is now used only locally, so removed from
Denis> cli-dump.h.

In this case, make the function static as well.

Denis> Dump restore and append commands are impacted by this patch.

I think this may need a documentation update.

Denis> @@ -559,8 +515,14 @@ restore_command (char *args, int from_tt
Denis>    char *filename;
Denis>    struct callback_data data;
Denis>    bfd *ibfd;
Denis> -  int binary_flag = 0;
Denis> -
Denis> +  char **argv;
Denis> +  int binary_flag;
Denis> +  char *arg;
Denis> +  int arg_num;
Denis> +  char *end_addr = NULL;
Denis> +  filename = NULL;
Denis> +  binary_flag = 0;
Denis> +  
Denis>    if (!target_has_execution)
Denis>      noprocess ();
 
Denis> @@ -568,43 +530,54 @@ restore_command (char *args, int from_tt
Denis>    data.load_start  = 0;
Denis>    data.load_end    = 0;
 
Denis> -  /* Parse the input arguments.  First is filename (required). */
Denis> -  filename = scan_filename_with_cleanup (&args, NULL);
Denis> -  if (args != NULL && *args != '\0')
Denis> -    {
Denis> -      char *binary_string = "binary";
Denis> +  argv = gdb_buildargv (args);
Denis> +  make_cleanup_freeargv (argv);
 
Denis> -      /* Look for optional "binary" flag.  */
Denis> -      if (strncmp (args, binary_string, strlen (binary_string)) == 0)
Denis> -	{
Denis> -	  binary_flag = 1;
Denis> -	  args += strlen (binary_string);
Denis> -	  args = skip_spaces (args);
Denis> -	}
Denis> -      /* Parse offset (optional). */
Denis> -      if (args != NULL && *args != '\0')
Denis> -      data.load_offset = 
Denis> -	parse_and_eval_long (scan_expression_with_cleanup (&args, NULL));
Denis> -      if (args != NULL && *args != '\0')
Denis> -	{
Denis> -	  /* Parse start address (optional). */
Denis> -	  data.load_start = 
Denis> -	    parse_and_eval_long (scan_expression_with_cleanup (&args, NULL));
Denis> -	  if (args != NULL && *args != '\0')
Denis> -	    {
Denis> -	      /* Parse end address (optional). */
Denis> -	      data.load_end = parse_and_eval_long (args);
Denis> -	      if (data.load_end <= data.load_start)
Denis> -		error (_("Start must be less than end."));
Denis> -	    }
Denis> -	}
Denis> +  /* At least filename argument is required.  */
Denis> +  if (argv == NULL || argv[0] == NULL)
Denis> +    error (_("Missing filename as the first argument."));
Denis> +
Denis> +  /* The first argument is the file name.  */
Denis> +  filename = tilde_expand (argv[0]);
Denis> +  make_cleanup (xfree, filename);
Denis> +
Denis> +  /* Look for optionals arguments from here: "binary", offset, 
Denis> +   start and end address.  */
Denis> +  if (argv[1] != NULL && strcmp (argv[1], "binary") == 0) 
Denis> +        binary_flag = 1;
Denis> +
Denis> +  arg_num = 1 + binary_flag;
Denis> +  for (arg = argv[arg_num]; arg != NULL; arg = argv[++arg_num])
Denis> +    {
Denis> +      /* parse offset argument (optional).  */
Denis> +      if (arg_num == 1 + binary_flag)
Denis> +          data.load_offset = parse_and_eval_long (arg);
Denis> +      
Denis> +      /* parse start address argument (optional).  */
Denis> +      else if (arg_num == 2 + binary_flag)
Denis> +        data.load_start = parse_and_eval_long (arg);
Denis> +  
Denis> +      /* Rest of line is considered as end address optional argument.  */
Denis> +      else if (arg_num == 3 + binary_flag )
Denis> +        end_addr = arg;
Denis> +      else if (arg_num > 3 + binary_flag)
Denis> +        end_addr = concat (end_addr, " ", arg, (char *)NULL);
Denis> +    }      
Denis> +
Denis> +  /* If end address argument is given, parse it and test it's greater than 
Denis> +     the start address.  */
Denis> +  if (end_addr != NULL)
Denis> +    {
Denis> +      data.load_end = parse_and_eval_long (end_addr);
Denis> +      if (data.load_end <= data.load_start)
Denis> +        error (_("Start must be less than end."));
Denis>      }
 
Denis>    if (info_verbose)
Denis>      printf_filtered ("Restore file %s offset 0x%lx start 0x%lx end 0x%lx\n",
Denis> -		     filename, (unsigned long) data.load_offset, 
Denis> -		     (unsigned long) data.load_start, 
Denis> -		     (unsigned long) data.load_end);
Denis> +                     filename, (unsigned long) data.load_offset, 
Denis> +                     (unsigned long) data.load_start, 
Denis> +                     (unsigned long) data.load_end);
 
Denis>    if (binary_flag)
Denis>      {
Denis> @@ -775,7 +748,8 @@ to the specified FILE in raw target orde
 
Denis>    c = add_com ("restore", class_vars, restore_command, _("\
Denis>  Restore the contents of FILE to target memory.\n\
Denis> -Arguments are FILE OFFSET START END where all except FILE are optional.\n\
Denis> +Arguments are FILE \"binary\" OFFSET START END\n\
Denis> +where all except FILE are optional.\n\
Denis>  OFFSET will be added to the base address of the file (default zero).\n\
Denis>  If START and END are given, only the file contents within that range\n\
Denis>  (file relative) will be restored to target memory."));
Denis> Index: cli/cli-dump.h
Denis> ===================================================================
Denis> RCS file: /cvs/src/src/gdb/cli/cli-dump.h,v
Denis> retrieving revision 1.7
Denis> diff -u -p -r1.7 cli-dump.h
Denis> --- cli/cli-dump.h	3 Jan 2009 05:57:54 -0000	1.7
Denis> +++ cli/cli-dump.h	5 Jan 2009 14:14:36 -0000
Denis> @@ -24,15 +24,4 @@ extern void add_dump_command (char *name
Denis>  			      void (*func) (char *args, char *mode),
Denis>  			      char *descr);
 
Denis> -/* Utilities for doing the dump.  */
Denis> -extern char *scan_filename_with_cleanup (char **cmd, const char *defname);
Denis> -
Denis> -extern char *scan_expression_with_cleanup (char **cmd, const char *defname);
Denis> -
Denis> -extern FILE *fopen_with_cleanup (const char *filename, const char *mode);
Denis> -
Denis> -extern char *skip_spaces (char *inp);
Denis> -
Denis> -extern struct value *parse_and_eval_with_error (char *exp, const char *fmt, ...) ATTR_FORMAT (printf, 2, 3);
Denis> -
Denis>  #endif
Denis> 2009-01-05  Denis Pilat  <denis.pilat@st.com>

Denis> 	* gdb.base/help.exp: add "binary" option for restore command.

Denis> Index: gdb.base/help.exp
Denis> ===================================================================
Denis> RCS file: /cvs/src/src/gdb/testsuite/gdb.base/help.exp,v
Denis> retrieving revision 1.31
Denis> diff -u -p -r1.31 help.exp
Denis> --- gdb.base/help.exp	16 Nov 2008 18:03:25 -0000	1.31
Denis> +++ gdb.base/help.exp	5 Jan 2009 14:02:44 -0000
Denis> @@ -355,7 +355,7 @@ gdb_test "help run" "Start debugged prog
Denis>  # test help rbreak
Denis>  gdb_test "help rbreak" "Set a breakpoint for all functions matching REGEXP\." "help rbreak"
Denis>  # test help restore
Denis> -gdb_test "help restore" "Restore the contents of FILE to target memory\.\[\r\n\]+Arguments are FILE OFFSET START END where all except FILE are optional\.\[\r\n\]+OFFSET will be added to the base address of the file \\(default zero\\)\.\[\r\n\]+If START and END are given, only the file contents within that range\[\r\n\]+\\(file relative\\) will be restored to target memory\."
Denis> +gdb_test "help restore" "Restore the contents of FILE to target memory\.\[\r\n\]+Arguments are FILE \"binary\" OFFSET START END\[\r\n\]+where all except FILE are optional\.\[\r\n\]+OFFSET will be added to the base address of the file \\(default zero\\)\.\[\r\n\]+If START and END are given, only the file contents within that range\[\r\n\]+\\(file relative\\) will be restored to target memory\."
Denis>  # test help return
Denis>  gdb_test "help return" "Make selected stack frame return to its caller\.\[\r\n\]+Control remains in the debugger, but when you continue\[\r\n\]+execution will resume in the frame above the one now selected\.\[\r\n\]+If an argument is given, it is an expression for the value to return\." "help return"
Denis>  # test help reverse-search
Denis> ----------


Tom

Re: [Fwd: Re: Patch to support spaces in filenames & paths]

[Tom Tromey]

>>>>> "Denis" == Denis PILAT <denis.pilat@st.com> writes:

Oops, I mistyped and sent a draft before I had finished.
This is the same note with one minor addition at the end.

Denis> Ping !

Sorry for the delay on this.

I read both the patches for this issue, plus the thread on the mailing
list.

Denis> I read your comments and produce a new patch that uses gdb_buildargv
Denis> (as Pedro suggested), inspired from add_symbol_file_command as well.

I don't like this approach.

With the current patch, we will accept a quoted expression.  This
seems very ugly to me.  I wouldn't be surprised if completion failed
in this case.  And, given that I would not support a new command using
a quoting scheme like this, it seems odd to allow one via refactoring.

I think this problem can be addressed to some degree by more carefully
parsing the address arguments.

Second, concatenating the final arguments using a space is not
completely correct, because it does not preserve the meaning of all
possible expressions.  This is a pedantic point, since such
expressions are unlikely to be used here.


In the long term, I would like to see gdb move toward a more uniform
approach to parsing its arguments, and to allowing full expressions in
more places.  I don't think gdb_buildargv can be the foundation for
that, though.


Since there only seems to be a single argument that has a problem
here, how about what Daniel suggested: a function that uses the same
quoting rules as gdb_buildargv, but which only peels off a single
argument?

A few notes on the patch itself follow.

Denis> I also removed lot of dead code in cli/cli-dump.c and
Denis> fopen_with_cleanup is now used only locally, so removed from
Denis> cli-dump.h.

In this case, make the function static as well.

Denis> Dump restore and append commands are impacted by this patch.

I think this may need a documentation update.

Denis> @@ -559,8 +515,14 @@ restore_command (char *args, int from_tt

[...]

Denis> +  argv = gdb_buildargv (args);
Denis> +  make_cleanup_freeargv (argv);
 
You have to save the first cleanup in a function and then either "do"
or "discard" it before any return.

Tom