[RFC] Support of quoted string in argument of dump, restore and append commands.

[Denis PILAT <denis dot pilat at st dot com>]

Hello everyone,

In the actual implementation of dump/restore/append command, I found it 
was impossible to manage files that contains spaces.
Whereas some other commands like file, symbol-file, ...etc.. accept the 
handling of quoted arguments, dump/restore/append do not.

You'll find attached the file cli-dump.patch that implements all the 
above, allowing quoting arguments to these commands so allowing having 
spaces in file names and in any arguments. This implementation uses 
buildargv.

Then using restore command, I found that the "binary" argument was not 
documented in the "help restore command", I add it and change the 
gdb.base/help.exp as well.

Then I discovered that the gdb.base/dump.exp use this kind of command:
dump srec mem FILE START STOP
where the only the last argument can contains spaces. That is a little 
bit strange, why START should be a space less expression and STOP could 
contain whatever you want ?
Same remark for all similar command and for
restore FILE "binary" OFFSET START END
In the actual implementation, only END argument could contains complex 
expressions with spaces. Of course the actual testsuite has taken that 
into account and make complex expression only for the last argument.


My implementation treats all arguments with same rules, I'm using 
buildargv function that's pretty well for that. If user want to use 
spaces in any argument, he must use double or single quotes.
I add checking about the number of arguments and if user overpass it, 
gdb prints "Too much arguments." and does not execute the command.

Please give me some feedback on what I did,
I can also adopt the policy to parse the last argument like is was 
before, considering that all stuff after N-1 argument is to be 
considered as the last one.
May be I can add some more test in the testsuite as well ?

As you will see, I've removed lot of dead code in cli-dumps.c, may be 
you'd like that to be part of another patch !

Waiting for your feedback
Cheers

Denis PILAT / STMicroelectronics
2008-12-01  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.
	(fopen_with_cleanup): use of buildargv for argument parsing.
	(dump_memory_to_file, restore_command): Likewise.
	(_initialize_cli_dump): add "binary" option for restore command 
	help message.


2008-12-02  Denis Pilat  <denis.pilat@st.com>

	* gdb.base/help.exp: add "binary" option for restore command.
	* gdb.base/dump.exp: quote the last argument of the "dump array 
	as mem, srec, expressions" test.


Index: cli/cli-dump.h
===================================================================
RCS file: /cvs/src/src/gdb/cli/cli-dump.h,v
retrieving revision 1.6
diff -u -p -r1.6 cli-dump.h
--- cli/cli-dump.h	1 Jan 2008 22:53:14 -0000	1.6
+++ cli/cli-dump.h	2 Dec 2008 09:10:07 -0000
@@ -25,14 +25,6 @@ extern void add_dump_command (char *name
 			      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
Index: cli/cli-dump.c
===================================================================
RCS file: /cvs/src/src/gdb/cli/cli-dump.c,v
retrieving revision 1.29
diff -u -p -r1.29 cli-dump.c
--- cli/cli-dump.c	28 Oct 2008 15:22:13 -0000	1.29
+++ cli/cli-dump.c	2 Dec 2008 09:10:08 -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;
 
-  /* Open the file.  */
-  filename = scan_filename_with_cleanup (&cmd, NULL);
+  argv = 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')
+  /* Find the second address.  */
+  hi_exp = argv[2];
+  if (hi_exp == NULL)
     error (_("Missing stop address."));
-  hi_exp = cmd;
 
   lo = parse_and_eval_address (lo_exp);
   hi = parse_and_eval_address (hi_exp);
   if (hi <= lo)
     error (_("Invalid memory address range (start >= end)."));
+
+  if (argv[3] != NULL)
+    error (_("Too much arguments."));
+
   count = hi - lo;
 
   /* FIXME: Should use read_memory_partial() and a magic blocking
@@ -273,16 +215,25 @@ dump_value_to_file (char *cmd, char *mod
   struct cleanup *old_cleanups = make_cleanup (null_cleanup, NULL);
   struct value *val;
   char *filename;
+  char ** argv;
+
+  argv = buildargv (cmd);
+  make_cleanup_freeargv (argv);
 
   /* Open the file.  */
-  filename = scan_filename_with_cleanup (&cmd, NULL);
+  if (argv == NULL || argv[0] == NULL)
+    error (_("Missing filename."));
+  filename = tilde_expand (argv[0]);
+  make_cleanup (xfree, filename);
 
   /* Find the value.  */
-  if (cmd == NULL || *cmd == '\0')
+  if (argv[1] == NULL)
     error (_("No value to %s."), *mode == 'a' ? "append" : "dump");
-  val = parse_and_eval (cmd);
+  val = parse_and_eval (argv[1]);
   if (val == NULL)
     error (_("Invalid expression."));
+  if (argv[2] != NULL)
+    error (_("Too much arguments."));
 
   /* Have everything.  Open/write the data.  */
   if (file_format == NULL || strcmp (file_format, "binary") == 0)
@@ -559,6 +510,7 @@ restore_command (char *args, int from_tt
   char *filename;
   struct callback_data data;
   bfd *ibfd;
+  char **argv;
   int binary_flag = 0;
 
   if (!target_has_execution)
@@ -568,43 +520,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 = 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."));
-	    }
-	}
+  /* Look for filename argument (required).  */
+  if (argv == NULL || argv[0] == NULL)
+    error (_("Missing filename."));
+  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) 
+    {
+      int arg_num = 1;
+      /* Look for "binary" flag (optional).  */
+      if (strcmp (argv[arg_num], "binary") == 0)
+        {
+          binary_flag = 1;
+          arg_num++;
+        }
+      /* Look for offset argument (optional) and parse it.  */
+      if (argv[arg_num] != NULL)
+        {
+          data.load_offset = 
+            parse_and_eval_long (argv[arg_num++]);
+          /* Look for start address argument (optional) and parse it.  */
+          if (argv[arg_num] != NULL)
+            {
+              data.load_start = 
+                parse_and_eval_long (argv[arg_num++]);
+              /* Look for end address argument (optional) and parse it.  */
+              if (argv[arg_num] != NULL)
+                {
+                  data.load_end = parse_and_eval_long (argv[arg_num++]);
+                  if (data.load_end <= data.load_start)
+                    error (_("Start must be less than end."));
+                  if (argv[arg_num] != NULL)
+                    error (_("Too much arguments."));
+                }
+            }
+        }
     }
 
   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 +738,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: testsuite/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
--- testsuite/gdb.base/help.exp	16 Nov 2008 18:03:25 -0000	1.31
+++ testsuite/gdb.base/help.exp	2 Dec 2008 09:10:08 -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
Index: testsuite/gdb.base/dump.exp
===================================================================
RCS file: /cvs/src/src/gdb/testsuite/gdb.base/dump.exp,v
retrieving revision 1.14
diff -u -p -r1.14 dump.exp
--- testsuite/gdb.base/dump.exp	6 Aug 2008 12:52:07 -0000	1.14
+++ testsuite/gdb.base/dump.exp	2 Dec 2008 09:10:08 -0000
@@ -190,7 +190,7 @@ make_dump_file "dump tekhex mem intstr2.
 
 # test complex expressions
 make_dump_file \
-	"dump srec mem intarr3.srec &intarray \(char *\) &intarray + sizeof intarray" \
+	"dump srec mem intarr3.srec &intarray \"\(char *\) &intarray + sizeof intarray\"" \
 	"dump array as mem, srec, expressions"