From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 21489 invoked by alias); 27 Apr 2009 21:38:33 -0000 Received: (qmail 21480 invoked by uid 22791); 27 Apr 2009 21:38:32 -0000 X-SWARE-Spam-Status: No, hits=-2.0 required=5.0 tests=AWL,BAYES_00,J_CHICKENPOX_43,SPF_HELO_PASS,SPF_PASS X-Spam-Check-By: sourceware.org Received: from mx2.redhat.com (HELO mx2.redhat.com) (66.187.237.31) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Mon, 27 Apr 2009 21:38:21 +0000 Received: from int-mx2.corp.redhat.com (int-mx2.corp.redhat.com [172.16.27.26]) by mx2.redhat.com (8.13.8/8.13.8) with ESMTP id n3RLcAxS008668; Mon, 27 Apr 2009 17:38:10 -0400 Received: from ns3.rdu.redhat.com (ns3.rdu.redhat.com [10.11.255.199]) by int-mx2.corp.redhat.com (8.13.1/8.13.1) with ESMTP id n3RLc9Io024902; Mon, 27 Apr 2009 17:38:10 -0400 Received: from opsy.redhat.com (vpn-13-172.rdu.redhat.com [10.11.13.172]) by ns3.rdu.redhat.com (8.13.8/8.13.8) with ESMTP id n3RLc7m3031787; Mon, 27 Apr 2009 17:38:08 -0400 Received: by opsy.redhat.com (Postfix, from userid 500) id AFF8F3785BE; Mon, 27 Apr 2009 15:38:06 -0600 (MDT) To: Denis PILAT Cc: gdb-patches Subject: Re: [Fwd: Re: Patch to support spaces in filenames & paths] References: <4981BA17.2090501@st.com> From: Tom Tromey Reply-To: tromey@redhat.com Date: Mon, 27 Apr 2009 21:38:00 -0000 In-Reply-To: <4981BA17.2090501@st.com> (Denis PILAT's message of "Thu\, 29 Jan 2009 15\:15\:51 +0100") Message-ID: User-Agent: Gnus/5.11 (Gnus v5.11) Emacs/22.2 (gnu/linux) MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org X-SW-Source: 2009-04/txt/msg00742.txt.bz2 >>>>> "Denis" == Denis PILAT 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> * 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