[PATCHv4,13/14] gdb: allow quoted filenames for commands that have custom completion

Message ID 351179c8d62d8dbd971679c1322757eb35ff4713.1720101827.git.aburgess@redhat.com
State New
Headers
Series Further filename completion improvements |

Checks

Context Check Description
linaro-tcwg-bot/tcwg_gdb_build--master-aarch64 success Build passed
linaro-tcwg-bot/tcwg_gdb_build--master-arm success Build passed
linaro-tcwg-bot/tcwg_gdb_check--master-arm fail Test failed
linaro-tcwg-bot/tcwg_gdb_check--master-aarch64 fail Test failed

Commit Message

Andrew Burgess July 4, 2024, 2:21 p.m. UTC
  This commit changes how GDB processes command arguments for the
following commands:

  compile file
  maint print c-tdesc
  save gdb-index

After this commit these commands will now expect their single filename
argument to be (optionally) quoted if it contains any special
characters (e.g. whit space or quotes).

If the filename does not contain any special characters then nothing
changes.  As an example:

   (gdb) save gdb-index /path/to/some/directory/

will work before and after this patch.  However, if the directory
name contains a white space then before this patch a user would write:

  (gdb) save gdb-index /path/to some/directory/

But this will now fail as GDB will consider this as two arguments,
'/path/to' and 'some/directory/'.  To pass this single directory name
a user must now do one of these:

  (gdb) save gdb-index "/path/to some/directory/"
  (gdb) save gdb-index '/path/to some/directory/'
  (gdb) save gdb-index /path/to\ some/directory/

This brings these commands into line with commands like 'file' and
'symbol-file', which have supported quoted filenames for a while.

The motivation for this change is to make handling of filename
arguments consistent throughout GDB.  We can't move to all commands
taking non-quoted filenames as the non-quoted style only allows for a
single argument.  Additionally, the non-quoted style doesn't allow for
filenames that end in white space (though this is probably pretty
rare).  So, if we want to have consistency the only choice is to move
towards supporting quote filenames.
---
 gdb/NEWS                                      |  7 +++++
 gdb/compile/compile.c                         | 11 ++++----
 gdb/doc/gdb.texinfo                           |  9 +++++++
 gdb/dwarf2/index-write.c                      |  8 +++---
 gdb/target-descriptions.c                     | 27 +++++++++----------
 .../gdb.base/filename-completion.exp          |  7 +++--
 gdb/testsuite/gdb.compile/compile.exp         |  2 +-
 7 files changed, 44 insertions(+), 27 deletions(-)
  

Comments

Eli Zaretskii July 4, 2024, 3:42 p.m. UTC | #1
> From: Andrew Burgess <aburgess@redhat.com>
> Cc: Andrew Burgess <aburgess@redhat.com>
> Date: Thu,  4 Jul 2024 15:21:08 +0100
> 
> --- a/gdb/NEWS
> +++ b/gdb/NEWS
> @@ -23,6 +23,13 @@ remove-symbol-file -a ADDRESS
>    multiple terms, e.g. 'function + 0x1000' (without quotes),
>    previously only a single term could be given.
>  
> +compile file
> +maint print c-tdesc
> +save gdb-index
> +  These commands now require their filename argument to be quoted if
> +  it contains with white space or quote characters.  If the argument
                 ^^^^
The "with" part should be removed.

Also, are white space and quotes the only special characters for these
commands?

> +  contains no special characters then quoting is not required.
              ^^^^^^^^^^^^^^^^^^^^^
I suggest to say here "no such special characters", to make absolutely
clear that you are talking about white space and quotes mentioned
earlier by their names.

Thanks.

Reviewed-By: Eli Zaretskii <eliz@gnu.org>
  
Andrew Burgess Aug. 20, 2024, 5:18 p.m. UTC | #2
Eli Zaretskii <eliz@gnu.org> writes:

>> From: Andrew Burgess <aburgess@redhat.com>
>> Cc: Andrew Burgess <aburgess@redhat.com>
>> Date: Thu,  4 Jul 2024 15:21:08 +0100
>> 
>> --- a/gdb/NEWS
>> +++ b/gdb/NEWS
>> @@ -23,6 +23,13 @@ remove-symbol-file -a ADDRESS
>>    multiple terms, e.g. 'function + 0x1000' (without quotes),
>>    previously only a single term could be given.
>>  
>> +compile file
>> +maint print c-tdesc
>> +save gdb-index
>> +  These commands now require their filename argument to be quoted if
>> +  it contains with white space or quote characters.  If the argument
>                  ^^^^
> The "with" part should be removed.
>
> Also, are white space and quotes the only special characters for these
> commands?

Yes.  GDB doesn't interpret any other special characters for these
commands except ~ and quoting is not needed for that.

>
>> +  contains no special characters then quoting is not required.
>               ^^^^^^^^^^^^^^^^^^^^^
> I suggest to say here "no such special characters", to make absolutely
> clear that you are talking about white space and quotes mentioned
> earlier by their names.

I made this fix in the new version I posted.

Thanks for all your feedback,

Andrew



>
> Thanks.
>
> Reviewed-By: Eli Zaretskii <eliz@gnu.org>
  

Patch

diff --git a/gdb/NEWS b/gdb/NEWS
index 8808fb3e1f6..4a830a61cda 100644
--- a/gdb/NEWS
+++ b/gdb/NEWS
@@ -23,6 +23,13 @@  remove-symbol-file -a ADDRESS
   multiple terms, e.g. 'function + 0x1000' (without quotes),
   previously only a single term could be given.
 
+compile file
+maint print c-tdesc
+save gdb-index
+  These commands now require their filename argument to be quoted if
+  it contains with white space or quote characters.  If the argument
+  contains no special characters then quoting is not required.
+
 *** Changes in GDB 15
 
 * The MPX commands "show/set mpx bound" have been deprecated, as Intel
diff --git a/gdb/compile/compile.c b/gdb/compile/compile.c
index 24d911dbb10..b5aac495563 100644
--- a/gdb/compile/compile.c
+++ b/gdb/compile/compile.c
@@ -302,14 +302,13 @@  compile_file_command (const char *args, int from_tty)
   enum compile_i_scope_types scope
     = options.raw ? COMPILE_I_RAW_SCOPE : COMPILE_I_SIMPLE_SCOPE;
 
-  args = skip_spaces (args);
+  std::string filename = extract_single_filename_arg (args);
 
   /* After processing options, check whether we have a filename.  */
-  if (args == nullptr || args[0] == '\0')
+  if (filename.empty ())
     error (_("You must provide a filename for this command."));
 
-  args = skip_spaces (args);
-  std::string abspath = gdb_abspath (args);
+  std::string abspath = gdb_abspath (filename.c_str ());
   std::string buffer = string_printf ("#include \"%s\"\n", abspath.c_str ());
   eval_compile_command (NULL, buffer.c_str (), scope, NULL);
 }
@@ -327,8 +326,8 @@  compile_file_command_completer (struct cmd_list_element *ignore,
       (tracker, &text, gdb::option::PROCESS_OPTIONS_UNKNOWN_IS_ERROR, group))
     return;
 
-  word = advance_to_deprecated_filename_complete_word_point (tracker, text);
-  deprecated_filename_completer (ignore, tracker, text, word);
+  word = advance_to_filename_maybe_quoted_complete_word_point (tracker, text);
+  filename_maybe_quoted_completer (ignore, tracker, text, word);
 }
 
 /* Handle the input from the 'compile code' command.  The
diff --git a/gdb/doc/gdb.texinfo b/gdb/doc/gdb.texinfo
index 7261a5559c4..e35cf45ec31 100644
--- a/gdb/doc/gdb.texinfo
+++ b/gdb/doc/gdb.texinfo
@@ -21337,6 +21337,9 @@ 
 @smallexample
 compile file /home/user/example.c
 @end smallexample
+
+The @var{filename} argument supports escaping and quoting, see
+@ref{Filename Arguments,,Filenames As Command Arguments}.
 @end table
 
 @table @code
@@ -22783,6 +22786,9 @@ 
 @file{@var{symbol-file}.debug_names} and
 @file{@var{symbol-file}.debug_str}.  The files are created in the
 given @var{directory}.
+
+The @var{directory} argument supports escaping and quoting, see
+@ref{Filename Arguments,,Filenames As Command Arguments}.
 @end table
 
 Once you have created an index file you can merge it into your symbol
@@ -41779,6 +41785,9 @@ 
 @var{file}) must only contain a single feature.  The source file
 produced is different in this case.
 
+The @var{file} argument supports escaping and quoting, see
+@ref{Filename Arguments,,Filenames As Command Arguments}.
+
 @kindex maint print xml-tdesc
 @item maint print xml-tdesc  @r{[}@var{file}@r{]}
 Print the target description (@pxref{Target Descriptions}) as an XML
diff --git a/gdb/dwarf2/index-write.c b/gdb/dwarf2/index-write.c
index ccbaea73380..2633a69df3b 100644
--- a/gdb/dwarf2/index-write.c
+++ b/gdb/dwarf2/index-write.c
@@ -1621,8 +1621,8 @@  gdb_save_index_cmd_completer (struct cmd_list_element *ignore,
       (tracker, &text, gdb::option::PROCESS_OPTIONS_UNKNOWN_IS_OPERAND, grp))
     return;
 
-  word = advance_to_deprecated_filename_complete_word_point (tracker, text);
-  deprecated_filename_completer (ignore, tracker, text, word);
+  word = advance_to_filename_maybe_quoted_complete_word_point (tracker, text);
+  filename_maybe_quoted_completer (ignore, tracker, text, word);
 }
 
 /* Implementation of the `save gdb-index' command.
@@ -1639,10 +1639,10 @@  save_gdb_index_command (const char *args, int from_tty)
   gdb::option::process_options
     (&args, gdb::option::PROCESS_OPTIONS_UNKNOWN_IS_OPERAND, group);
 
-  if (args == nullptr || *args == '\0')
+  std::string directory = extract_single_filename_arg (args);
+  if (directory.empty ())
     error (_("usage: save gdb-index [-dwarf-5] DIRECTORY"));
 
-  std::string directory (gdb_tilde_expand (args));
   dw_index_kind index_kind
     = (opts.dwarf_5 ? dw_index_kind::DEBUG_NAMES : dw_index_kind::GDB_INDEX);
 
diff --git a/gdb/target-descriptions.c b/gdb/target-descriptions.c
index bdd7f19d60f..65d0d563e7f 100644
--- a/gdb/target-descriptions.c
+++ b/gdb/target-descriptions.c
@@ -1693,14 +1693,15 @@  static void
 maint_print_c_tdesc_cmd (const char *args, int from_tty)
 {
   const struct target_desc *tdesc;
-  const char *filename;
 
   maint_print_c_tdesc_options opts;
   auto grp = make_maint_print_c_tdesc_options_def_group (&opts);
   gdb::option::process_options
     (&args, gdb::option::PROCESS_OPTIONS_UNKNOWN_IS_ERROR, grp);
 
-  if (args == NULL)
+  std::string filename = extract_single_filename_arg (args);
+
+  if (filename.empty ())
     {
       /* Use the global target-supplied description, not the current
 	 architecture's.  This lets a GDB for one architecture generate C
@@ -1708,26 +1709,24 @@  maint_print_c_tdesc_cmd (const char *args, int from_tty)
 	 initialization code will reject the new description.  */
       target_desc_info *tdesc_info = &current_inferior ()->tdesc_info;
       tdesc = tdesc_info->tdesc;
-      filename = tdesc_info->filename.data ();
+      if (tdesc_info->filename.data () != nullptr)
+	filename = std::string (tdesc_info->filename.data ());
     }
   else
     {
       /* Use the target description from the XML file.  */
-      filename = args;
-      tdesc = file_read_description_xml (filename);
+      tdesc = file_read_description_xml (filename.c_str ());
     }
 
   if (tdesc == NULL)
     error (_("There is no target description to print."));
 
-  if (filename == NULL)
+  if (filename.empty ())
     filename = "fetched from target";
 
-  std::string filename_after_features (filename);
-  auto loc = filename_after_features.rfind ("/features/");
-
+  auto loc = filename.rfind ("/features/");
   if (loc != std::string::npos)
-    filename_after_features = filename_after_features.substr (loc + 10);
+    filename = filename.substr (loc + 10);
 
   /* Print c files for target features instead of target descriptions,
      because c files got from target features are more flexible than the
@@ -1738,13 +1737,13 @@  maint_print_c_tdesc_cmd (const char *args, int from_tty)
 	error (_("only target descriptions with 1 feature can be used "
 		 "with -single-feature option"));
 
-      print_c_feature v (filename_after_features);
+      print_c_feature v (filename);
 
       tdesc->accept (v);
     }
   else
     {
-      print_c_tdesc v (filename_after_features);
+      print_c_tdesc v (filename);
 
       tdesc->accept (v);
     }
@@ -1762,8 +1761,8 @@  maint_print_c_tdesc_cmd_completer (struct cmd_list_element *ignore,
       (tracker, &text, gdb::option::PROCESS_OPTIONS_UNKNOWN_IS_ERROR, grp))
     return;
 
-  word = advance_to_deprecated_filename_complete_word_point (tracker, text);
-  deprecated_filename_completer (ignore, tracker, text, word);
+  word = advance_to_filename_maybe_quoted_complete_word_point (tracker, text);
+  filename_maybe_quoted_completer (ignore, tracker, text, word);
 }
 
 /* Implement the maintenance print xml-tdesc command.  */
diff --git a/gdb/testsuite/gdb.base/filename-completion.exp b/gdb/testsuite/gdb.base/filename-completion.exp
index 62fb49570a6..7ba055b9ae7 100644
--- a/gdb/testsuite/gdb.base/filename-completion.exp
+++ b/gdb/testsuite/gdb.base/filename-completion.exp
@@ -123,7 +123,10 @@  proc run_quoting_and_escaping_tests { root } {
     # Test all the commands which allow quoting of filenames, and
     # which require whitespace to be escaped in unquoted filenames.
     foreach_with_prefix cmd { file exec-file symbol-file add-symbol-file \
-			      remove-symbol-file } {
+				  remove-symbol-file \
+				  "target core" "target exec" "target tfile" \
+				  "maint print c-tdesc" "compile file" \
+				  "save gdb-index" "save gdb-index -dwarf-5" } {
 	gdb_start
 
 	# Completing 'thread apply all ...' commands uses a custom word
@@ -298,7 +301,7 @@  proc run_unquoted_tests_core { root cmd { prefix "" } } {
 proc run_unquoted_tests { root } {
     # Test all the commands which allow quoting of filenames, and
     # which require whitespace to be escaped in unquoted filenames.
-    foreach_with_prefix cmd { "maint print c-tdesc" "set logging file" \
+    foreach_with_prefix cmd { "set logging file" \
 				  "target core" "add-auto-load-safe-path" } {
 	run_unquoted_tests_core $root $cmd
     }
diff --git a/gdb/testsuite/gdb.compile/compile.exp b/gdb/testsuite/gdb.compile/compile.exp
index cd596335859..2c2e3217b19 100644
--- a/gdb/testsuite/gdb.compile/compile.exp
+++ b/gdb/testsuite/gdb.compile/compile.exp
@@ -66,7 +66,7 @@  if {[skip_compile_feature_untested]} {
 gdb_test_no_output "compile -- f = 10" \
     "test abbreviations and code delimiter"
 
-gdb_test "compile f = 10;" ".*= 10;: No such file.*" \
+gdb_test "compile f = 10;" "^Junk after filename \"=\": 10;" \
     "Test abbreviations and code collision"
 
 gdb_test_no_output "compile -r -- void _gdb_expr(){int i = 5;}" \