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

Tom Tromey tromey@redhat.com
Mon Apr 27 21:38:00 GMT 2009


>>>>> "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



More information about the Gdb-patches mailing list