[PATCH 0/6] Further filename completion improvements

[PATCH 0/6] Further filename completion improvements

[Andrew Burgess]

This series continues my efforts to improve filename completion.

After this series I believe GDB will be ready to add filename options
(to the option sub-system), though I haven't done that in this series,
I want to see if there are any objections to the direction I'm going
in.

But even without the filename options, this series adds some nice
improvements: GDB's filename completion (both TAB and 'complete'
command) now handles escaped whitespace and quotes correctly, and the
'complete' command now correctly displays filenames that require
escaping, and better handles the trailing quote when a 'complete'
command returns a directory.

---

Andrew Burgess (6):
  gdb: improve escaping when completing filenames
  gdb: move display of completion results into completion_result class
  gdb: simplify completion_result::print_matches
  gdb: add match formatter mechanism for 'complete' command output
  gdb: apply escaping to filenames in 'complete' results
  gdb: improve gdb_rl_find_completion_word for quoted words

 gdb/cli/cli-cmds.c                            |  26 +-
 gdb/completer.c                               | 389 +++++++++++++++---
 gdb/completer.h                               |  47 ++-
 .../gdb.base/filename-completion.exp          | 118 +++++-
 4 files changed, 494 insertions(+), 86 deletions(-)


base-commit: 56f703d39d6f4793ba73b2364a4ea052e8ad755d
-- 
2.25.4

[PATCH 1/6] gdb: improve escaping when completing filenames

[Andrew Burgess]

This commit is a step towards improving filename quoting when
completing filenames.

I've struggled a bit trying to split this series into chunks.  There's
a lot of dependencies between different parts of the completion
system, and trying to get this working correctly is pretty messy.

This first step is really about implementing 3 readline hooks:

  rl_char_is_quoted_p
    - Is a particular character quoted within readline's input buffer?
  rl_filename_dequoting_function
    - Remove quoting characters from a filename.
  rl_filename_quoting_function
    - Add quoting characters to a filename.

See 'info readline' for more details.

There's still a bunch of stuff that doesn't work after this commit,
mostly around the 'complete' command which of course doesn't go
through readline, so doesn't benefit from all of these new functions
yet, I'll add some of this in a later commit.

Tab completion is now slightly improved though, it is possible to
tab-complete a filename that includes a double or single quote, either
in an unquoted string or within a string surrounded by single or
double quotes, backslash escaping is used when necessary.

There are some additional tests to cover the new functionality.
---
 gdb/completer.c                               | 163 +++++++++++++++++-
 .../gdb.base/filename-completion.exp          |  34 ++++
 2 files changed, 194 insertions(+), 3 deletions(-)

diff --git a/gdb/completer.c b/gdb/completer.c
index 8e34e30f46b..4cda5f3a383 100644
--- a/gdb/completer.c
+++ b/gdb/completer.c
@@ -204,6 +204,153 @@ noop_completer (struct cmd_list_element *ignore,
 {
 }
 
+/* Return 1 if the character at EINDEX in STRING is quoted (there is an
+   unclosed quoted string), or if the character at EINDEX is quoted by a
+   backslash.  */
+
+static int
+gdb_completer_file_name_char_is_quoted (char *string, int eindex)
+{
+  for (int i = 0; i <= eindex && string[i] != '\0'; )
+    {
+      char c = string[i];
+
+      if (c == '\\')
+	{
+	  /* The backslash itself is not quoted.  */
+	  if (i >= eindex)
+	    return 0;
+	  ++i;
+	  /* But the next character is.  */
+	  if (i >= eindex)
+	    return 1;
+	  if (string[i] == '\0')
+	    return 0;
+	  ++i;
+	  continue;
+	}
+      else if (strchr (rl_completer_quote_characters, c) != nullptr)
+	{
+	  /* This assumes that extract_string_maybe_quoted can handle a
+	     string quoted with character C.  Currently this is true as the
+	     only characters we put in rl_completer_quote_characters are
+	     single and/or double quotes, both of which
+	     extract_string_maybe_quoted can handle.  */
+	  const char *tmp = &string[i];
+	  (void) extract_string_maybe_quoted (&tmp);
+	  i = tmp - string;
+
+	  /* Consider any character within the string we just skipped over
+	     as quoted, though this might not be completely correct; the
+	     opening and closing quotes are not themselves quoted.  But so
+	     far this doesn't seem to have caused any issues.  */
+	  if (i >= eindex)
+	    return 1;
+	}
+      else
+	++i;
+    }
+
+  return 0;
+}
+
+/* Removing character escaping from FILENAME.  QUOTE_CHAR is the quote
+   character around FILENAME or the null-character if there is no quoting
+   around FILENAME.  */
+
+static char *
+gdb_completer_file_name_dequote (char *filename, int quote_char)
+{
+  std::string tmp;
+
+  if (quote_char == '\'')
+    {
+      /* There is no backslash escaping within a single quoted string.  In
+	 this case we can just return the input string.  */
+      tmp = filename;
+    }
+  else if (quote_char == '"')
+    {
+      /* Remove escaping from a double quoted string.  */
+      for (const char *input = filename;
+	   *input != '\0';
+	   ++input)
+	{
+	  if (input[0] == '\\'
+	      && input[1] != '\0'
+	      && strchr ("\"\\", input[1]) != nullptr)
+	    ++input;
+	  tmp += *input;
+	}
+    }
+  else
+    {
+      /* Remove escaping from an unquoted string.  */
+      for (const char *input = filename;
+	   *input != '\0';
+	   ++input)
+	{
+	  /* We allow anything to be escaped in an unquoted string.  */
+	  if (*input == '\\')
+	    {
+	      ++input;
+	      if (*input == '\0')
+		break;
+	    }
+
+	  tmp += *input;
+	}
+    }
+
+  return strdup (tmp.c_str ());
+}
+
+/* Apply character escaping to the file name in TEXT.  QUOTE_PTR points to
+   the quote character surrounding TEXT, or points to the null-character if
+   there are no quotes around TEXT.  MATCH_TYPE will be one of the readline
+   constants SINGLE_MATCH or MULTI_MATCH depending on if there is one or
+   many completions.  */
+
+static char *
+gdb_completer_file_name_quote (char *text, int match_type, char *quote_ptr)
+{
+  std::string str;
+
+  if (*quote_ptr == '\'')
+    {
+      /* There is no backslash escaping permitted within a single quoted
+	 string, so in this case we can just return the input sting.  */
+      str = text;
+    }
+  else if (*quote_ptr == '"')
+    {
+      /* Add escaping for a double quoted filename.  */
+      for (const char *input = text;
+	   *input != '\0';
+	   ++input)
+	{
+	  if (strchr ("\"\\", *input) != nullptr)
+	    str += '\\';
+	  str += *input;
+	}
+    }
+  else
+    {
+      /* Add escaping for an unquoted filename.  */
+      for (const char *input = text;
+	   *input != '\0';
+	   ++input)
+	{
+	  if (strchr (" \t\n\\\"'", *input)
+	      != nullptr)
+	    str += '\\';
+	  str += *input;
+	}
+    }
+
+  return strdup (str.c_str ());
+}
+
 /* Complete on filenames.  */
 
 void
@@ -211,6 +358,7 @@ filename_completer (struct cmd_list_element *ignore,
 		    completion_tracker &tracker,
 		    const char *text, const char *word)
 {
+  rl_char_is_quoted_p = gdb_completer_file_name_char_is_quoted;
   rl_completer_quote_characters = gdb_completer_file_name_quote_characters;
 
   int subsequent_name = 0;
@@ -262,6 +410,7 @@ filename_completer_handle_brkchars (struct cmd_list_element *ignore,
     (gdb_completer_file_name_break_characters);
 
   rl_completer_quote_characters = gdb_completer_file_name_quote_characters;
+  rl_char_is_quoted_p = gdb_completer_file_name_char_is_quoted;
 }
 
 /* Find the bounds of the current word for completion purposes, and
@@ -1261,6 +1410,7 @@ complete_line_internal_1 (completion_tracker &tracker,
      completing file names then we can switch to the file name quote
      character set (i.e., both single- and double-quotes).  */
   rl_completer_quote_characters = gdb_completer_expression_quote_characters;
+  rl_char_is_quoted_p = nullptr;
 
   /* Decide whether to complete on a list of gdb commands or on
      symbols.  */
@@ -2153,9 +2303,11 @@ completion_tracker::build_completion_result (const char *text,
   /* Build replacement word, based on the LCD.  */
 
   recompute_lowest_common_denominator ();
-  match_list[0]
-    = expand_preserving_ws (text, end - start,
-			    m_lowest_common_denominator);
+  if (rl_filename_completion_desired)
+    match_list[0] = xstrdup (m_lowest_common_denominator);
+  else
+    match_list[0]
+      = expand_preserving_ws (text, end - start, m_lowest_common_denominator);
 
   if (m_lowest_common_denominator_unique)
     {
@@ -3018,6 +3170,11 @@ _initialize_completer ()
   rl_attempted_completion_function = gdb_rl_attempted_completion_function;
   set_rl_completer_word_break_characters (default_word_break_characters ());
 
+  /* Setup readline globals relating to filename completion.  */
+  rl_filename_quote_characters = " \t\n\\\"'";
+  rl_filename_dequoting_function = gdb_completer_file_name_dequote;
+  rl_filename_quoting_function = gdb_completer_file_name_quote;
+
   add_setshow_zuinteger_unlimited_cmd ("max-completions", no_class,
 				       &max_completions, _("\
 Set maximum number of completion candidates."), _("\
diff --git a/gdb/testsuite/gdb.base/filename-completion.exp b/gdb/testsuite/gdb.base/filename-completion.exp
index b700977cec5..f23e8671f40 100644
--- a/gdb/testsuite/gdb.base/filename-completion.exp
+++ b/gdb/testsuite/gdb.base/filename-completion.exp
@@ -40,6 +40,9 @@ proc setup_directory_tree {} {
     remote_exec host "touch \"${root}/aaa/aa bb\""
     remote_exec host "touch \"${root}/aaa/aa cc\""
 
+    remote_exec host "touch \"${root}/bb1/aa\\\"bb\""
+    remote_exec host "touch \"${root}/bb1/aa'bb\""
+
     return $root
 }
 
@@ -89,6 +92,37 @@ proc run_tests { root } {
 		    "aa cc"
 		} "" "${qc}" false \
 		"expand filenames containing spaces"
+
+	    test_gdb_complete_multiple "file ${qc}${root}/bb1/" \
+		"a" "a" {
+		    "aa\"bb"
+		    "aa'bb"
+		} "" "${qc}" false \
+		"expand filenames containing quotes"
+	} else {
+	    set sp "\\ "
+
+	    test_gdb_complete_tab_multiple "file ${qc}${root}/aaa/a" \
+		"a${sp}" {
+		    "aa bb"
+		    "aa cc"
+		} false \
+		"expand filenames containing spaces"
+
+	    test_gdb_complete_tab_multiple "file ${qc}${root}/bb1/a" \
+		"a" {
+		    "aa\"bb"
+		    "aa'bb"
+		} false \
+		"expand filenames containing quotes"
+
+	    test_gdb_complete_tab_unique "file ${qc}${root}/bb1/aa\\\"" \
+		"file ${qc}${root}/bb1/aa\\\\\"bb${qc}" " " \
+	    "expand unique filename containing double quotes"
+
+	    test_gdb_complete_tab_unique "file ${qc}${root}/bb1/aa\\'" \
+		"file ${qc}${root}/bb1/aa\\\\'bb${qc}" " " \
+		"expand unique filename containing single quote"
 	}
     }
 }
-- 
2.25.4

Re: [PATCH 1/6] gdb: improve escaping when completing filenames

[Lancelot SIX]

Hi Andrew,

Thanks for working on that!

I have a couple of really minor comments below.

On Fri, Mar 29, 2024 at 11:42:27AM +0000, Andrew Burgess wrote:
> --- a/gdb/completer.c
> +++ b/gdb/completer.c
> @@ -204,6 +204,153 @@ noop_completer (struct cmd_list_element *ignore,
>  {
>  }
>  
> +/* Return 1 if the character at EINDEX in STRING is quoted (there is an
> +   unclosed quoted string), or if the character at EINDEX is quoted by a
> +   backslash.  */
> +
> +static int
> +gdb_completer_file_name_char_is_quoted (char *string, int eindex)
> +{
> +  for (int i = 0; i <= eindex && string[i] != '\0'; )
> +    {
> +      char c = string[i];
> +
> +      if (c == '\\')
> +	{
> +	  /* The backslash itself is not quoted.  */
> +	  if (i >= eindex)
> +	    return 0;
> +	  ++i;
> +	  /* But the next character is.  */
> +	  if (i >= eindex)
> +	    return 1;
> +	  if (string[i] == '\0')
> +	    return 0;
> +	  ++i;
> +	  continue;
> +	}
> +      else if (strchr (rl_completer_quote_characters, c) != nullptr)
> +	{
> +	  /* This assumes that extract_string_maybe_quoted can handle a
> +	     string quoted with character C.  Currently this is true as the
> +	     only characters we put in rl_completer_quote_characters are
> +	     single and/or double quotes, both of which
> +	     extract_string_maybe_quoted can handle.  */

Maybe it is worth asserting that assumption

    gdb_assert (c == '"' || c == '\'');

> +	  const char *tmp = &string[i];
> +	  (void) extract_string_maybe_quoted (&tmp);
> +	  i = tmp - string;
> +
> +	  /* Consider any character within the string we just skipped over
> +	     as quoted, though this might not be completely correct; the
> +	     opening and closing quotes are not themselves quoted.  But so
> +	     far this doesn't seem to have caused any issues.  */
> +	  if (i >= eindex)
> +	    return 1;
> +	}
> +      else
> +	++i;
> +    }
> +
> +  return 0;
> +}
> +
> +/* Removing character escaping from FILENAME.  QUOTE_CHAR is the quote
> +   character around FILENAME or the null-character if there is no quoting
> +   around FILENAME.  */
> +
> +static char *
> +gdb_completer_file_name_dequote (char *filename, int quote_char)
> +{
> +  std::string tmp;
> +
> +  if (quote_char == '\'')
> +    {
> +      /* There is no backslash escaping within a single quoted string.  In
> +	 this case we can just return the input string.  */
> +      tmp = filename;
> +    }
> +  else if (quote_char == '"')
> +    {
> +      /* Remove escaping from a double quoted string.  */
> +      for (const char *input = filename;
> +	   *input != '\0';
> +	   ++input)
> +	{
> +	  if (input[0] == '\\'
> +	      && input[1] != '\0'
> +	      && strchr ("\"\\", input[1]) != nullptr)
> +	    ++input;
> +	  tmp += *input;
> +	}
> +    }
> +  else
> +    {

It might be overkill, but I would probably add "gdb_assert (quote_char ==
'\0')".

> +      /* Remove escaping from an unquoted string.  */
> +      for (const char *input = filename;
> +	   *input != '\0';
> +	   ++input)
> +	{
> +	  /* We allow anything to be escaped in an unquoted string.  */
> +	  if (*input == '\\')
> +	    {
> +	      ++input;
> +	      if (*input == '\0')
> +		break;
> +	    }
> +
> +	  tmp += *input;
> +	}
> +    }
> +
> +  return strdup (tmp.c_str ());
> +}
> +
> +/* Apply character escaping to the file name in TEXT.  QUOTE_PTR points to
> +   the quote character surrounding TEXT, or points to the null-character if
> +   there are no quotes around TEXT.  MATCH_TYPE will be one of the readline
> +   constants SINGLE_MATCH or MULTI_MATCH depending on if there is one or
> +   many completions.  */
> +
> +static char *
> +gdb_completer_file_name_quote (char *text, int match_type, char *quote_ptr)

The match_type argument is never used, maybe mark it with
[[maybe_unused]]?

> +{
> +  std::string str;
> +
> +  if (*quote_ptr == '\'')
> +    {
> +      /* There is no backslash escaping permitted within a single quoted
> +	 string, so in this case we can just return the input sting.  */
> +      str = text;
> +    }
> +  else if (*quote_ptr == '"')
> +    {
> +      /* Add escaping for a double quoted filename.  */
> +      for (const char *input = text;
> +	   *input != '\0';
> +	   ++input)
> +	{
> +	  if (strchr ("\"\\", *input) != nullptr)
> +	    str += '\\';
> +	  str += *input;
> +	}
> +    }
> +  else
> +    {

Similarly, I’d add a "gdb_assert (*quote_ptr == '\0')".

> +      /* Add escaping for an unquoted filename.  */
> +      for (const char *input = text;
> +	   *input != '\0';
> +	   ++input)
> +	{
> +	  if (strchr (" \t\n\\\"'", *input)
> +	      != nullptr)
> +	    str += '\\';
> +	  str += *input;
> +	}
> +    }
> +
> +  return strdup (str.c_str ());
> +}
> +

Best,
Lancelot.

[PATCH 2/6] gdb: move display of completion results into completion_result class

[Andrew Burgess]

When using the 'complete' command to complete file names we have some
problems.  The suggested completion should include any required
escaping, so if there is a filename '/tmp/aa"bb' (without the single
quotes), then this should be displayed in the completion output like:

  (gdb) complete file /tmp/aa
  file /tmp/aa\"bb

This doesn't currently happen.  We already have some code in
filename_completer (completer.c) that handles adding the trailing '/'
character if the completion result is a directory, so we might be
tempted to add any needed escaping in that function, however if we do
then we run into a problem.

If there are multiple completion results from a 'complete' command,
then in complete_command (cli/cli-cmds.c) we sort the completion
results prior to printing them.

If those results have already had the escaping added then the sort
will be done on the text including the escape characters.  This
means that results from the 'complete' command will appear in a
different order than readline would present them; readline sorts the
results and then adds and required escaping.

I think that the 'complete' command should behave the same as
readline, sort the entries and then add the escaping.  This means that
we need to sort before adding the escaping.

There is a second problem when using the 'complete' command with file
names, that is trailing quote characters.  The addition of a trailing
quote character is a bit complex due (I think) to the structure of
readline itself.

Adding the quote character currently occurs in two places in GDB.  In
completion_tracker::build_completion_result (completer.c) we add the
trailing quote in the case where we only have a single result, and in
complete_command (cli/cli-cmds.c) we add the trailing quote character
if we have more than one result.

With these two cases we ensure that the 'complete' command always adds
a trailing quote (when necessary) to the results it displays.

However, I think that for filename completion we don't always want a
trailing quote.  Consider if we have a file '/tmp/xxx/foo.c' (without
the quotes), and then we do:

  (gdb) complete file /tmp/xx

What should the result of this be?  Well, if we use TAB completion
like this:

  (gdb) file /tmp/xx<TAB>
  (gdb) file /tmp/xxx/

then readline completes the directory 'xxx/', but doesn't try to
complete the filename within the xxx/ directory until we press <TAB>
again.  So I think that the results of the 'complete' command should
share this behaviour:

  (gdb) complete file /tmp/xx
  file /tmp/xxx/

And this is what we get right now, but what if the user adds some
opening quotes?  Right now we get this:

  (gdb) complete file "/tmp/xx
  file "/tmp/xxx/

Which I think is correct, there's no trailing quote added.  This is
because in completion_tracker::build_completion_result the
completion_tracker object doesn't know that the double quote is the
quote_char().  However, this causes a problem, if we do this:

  (gdb) complete file "/tmp/xxx/f
  file "/tmp/xxx/foo.c

We're still missing the trailing quote, even though in this case, when
we've expanded a complete filename, adding the trailing quote would be
the correct thing to do.  The cause is, of course, the same, the
completion_tracker is unaware of what the quote character is, and so
doesn't add the quote when needed.

However, the problem gets worse, if we create an additional file
'/tmp/xxa/bar.c', notice that this is in a different directory.  So
now when we do this:

  (gdb) complete file "/tmp/xx
  file "/tmp/xxa/"
  file "/tmp/xxx/"

Now the trailing quote has been added even though we haven't completed
a full filename.  This is because in complete_command (cli/cli-cmds.c)
we do know what the quote character is, and the trailing quote is
always added.

There are multiple problems here, but at least part of the problem
will be solved by making the result printing in complete_command
smarter.  My plan is to allow different completion functions to modify
how the 'complete' results are printed.

This commit doesn't solve any of the problems listed above.  Instead
this commit is just a refactor.  I've moved the result printing logic
out of complete_command and created a new function
completion_result::print_matches.  And that's it.  Nothing has
functionally changed yet, that will come in later commits when the
print_matches function is made smarter.

There should be no user visible changes after this commit.
---
 gdb/cli/cli-cmds.c | 26 +-------------------------
 gdb/completer.c    | 33 +++++++++++++++++++++++++++++++++
 gdb/completer.h    | 13 +++++++++++++
 3 files changed, 47 insertions(+), 25 deletions(-)

diff --git a/gdb/cli/cli-cmds.c b/gdb/cli/cli-cmds.c
index df11f956245..0b621f65917 100644
--- a/gdb/cli/cli-cmds.c
+++ b/gdb/cli/cli-cmds.c
@@ -424,31 +424,7 @@ complete_command (const char *arg, int from_tty)
     {
       std::string arg_prefix (arg, word - arg);
 
-      if (result.number_matches == 1)
-	printf_unfiltered ("%s%s\n", arg_prefix.c_str (), result.match_list[0]);
-      else
-	{
-	  result.sort_match_list ();
-
-	  for (size_t i = 0; i < result.number_matches; i++)
-	    {
-	      printf_unfiltered ("%s%s",
-				 arg_prefix.c_str (),
-				 result.match_list[i + 1]);
-	      if (quote_char)
-		printf_unfiltered ("%c", quote_char);
-	      printf_unfiltered ("\n");
-	    }
-	}
-
-      if (result.number_matches == max_completions)
-	{
-	  /* ARG_PREFIX and WORD are included in the output so that emacs
-	     will include the message in the output.  */
-	  printf_unfiltered (_("%s%s %s\n"),
-			     arg_prefix.c_str (), word,
-			     get_max_completions_reached_message ());
-	}
+      result.print_matches (arg_prefix, word, quote_char);
     }
 }
 
diff --git a/gdb/completer.c b/gdb/completer.c
index 4cda5f3a383..9b4041da01a 100644
--- a/gdb/completer.c
+++ b/gdb/completer.c
@@ -2443,6 +2443,39 @@ completion_result::reset_match_list ()
     }
 }
 
+/* See completer.h  */
+
+void
+completion_result::print_matches (const std::string &prefix,
+				  const char *word, int quote_char)
+{
+  if (this->number_matches == 1)
+    printf_unfiltered ("%s%s\n", prefix.c_str (), this->match_list[0]);
+  else
+    {
+      this->sort_match_list ();
+
+      for (size_t i = 0; i < this->number_matches; i++)
+	{
+	  printf_unfiltered ("%s%s", prefix.c_str (),
+			     this->match_list[i + 1]);
+	  if (quote_char)
+	    printf_unfiltered ("%c", quote_char);
+	  printf_unfiltered ("\n");
+	}
+    }
+
+  if (this->number_matches == max_completions)
+    {
+      /* PREFIX and WORD are included in the output so that emacs will
+	 include the message in the output.  */
+      printf_unfiltered (_("%s%s %s\n"),
+			 prefix.c_str (), word,
+			 get_max_completions_reached_message ());
+    }
+
+}
+
 /* Helper for gdb_rl_attempted_completion_function, which does most of
    the work.  This is called by readline to build the match list array
    and to determine the lowest common denominator.  The real matches
diff --git a/gdb/completer.h b/gdb/completer.h
index 98a12f3907c..4419c8f6d30 100644
--- a/gdb/completer.h
+++ b/gdb/completer.h
@@ -268,6 +268,19 @@ struct completion_result
   /* Sort the match list.  */
   void sort_match_list ();
 
+  /* Called to display all matches (used by the 'complete' command).
+     PREFIX is everything before the completion word.  WORD is the word
+     being completed, this is only used if we reach the maximum number of
+     completions, otherwise, each line of output consists of PREFIX
+     followed by one of the possible completion words.
+
+     The QUOTE_CHAR is appended after each possible completion word and
+     should be the quote character that appears before the completion word,
+     or the null-character if there is no quote before the completion
+     word.  */
+  void print_matches (const std::string &prefix, const char *word,
+		      int quote_char);
+
 private:
   /* Destroy the match list array and its contents.  */
   void reset_match_list ();
-- 
2.25.4

Re: [PATCH 2/6] gdb: move display of completion results into completion_result class

[Eli Zaretskii]

> From: Andrew Burgess <aburgess@redhat.com>
> Cc: Andrew Burgess <aburgess@redhat.com>
> Date: Fri, 29 Mar 2024 11:42:28 +0000
> 
> When using the 'complete' command to complete file names we have some
> problems.  The suggested completion should include any required
> escaping, so if there is a filename '/tmp/aa"bb' (without the single
> quotes), then this should be displayed in the completion output like:
> 
>   (gdb) complete file /tmp/aa
>   file /tmp/aa\"bb

Why should it be displayed with the backslash?  And would the
completed name, if passed to a command, also have the backslash added?
If so, it could cause some commands to fail; basically, you are adding
shell file-name semantics into commands that don't work via the shell.

So I'm not sure I understand what is being intended here, and I'm
afraid you will uncover a lot of problems as you go with this (as you
already discovered).

Apologies if I'm missing something important here.

Re: [PATCH 2/6] gdb: move display of completion results into completion_result class

[Lancelot SIX]

On Fri, Mar 29, 2024 at 03:14:07PM +0300, Eli Zaretskii wrote:
> > From: Andrew Burgess <aburgess@redhat.com>
> > Cc: Andrew Burgess <aburgess@redhat.com>
> > Date: Fri, 29 Mar 2024 11:42:28 +0000
> > 
> > When using the 'complete' command to complete file names we have some
> > problems.  The suggested completion should include any required
> > escaping, so if there is a filename '/tmp/aa"bb' (without the single
> > quotes), then this should be displayed in the completion output like:
> > 
> >   (gdb) complete file /tmp/aa
> >   file /tmp/aa\"bb
> 
> Why should it be displayed with the backslash?  And would the
> completed name, if passed to a command, also have the backslash added?
> If so, it could cause some commands to fail; basically, you are adding
> shell file-name semantics into commands that don't work via the shell.

Hi,

The "file" command currently expects "shell file-name semantics":

    $ gdb -q
    (gdb) file test/aa bb/a.out
    test/aa: No such file or directory.
    (gdb) file "test/aa bb/a.out"
    Reading symbols from test/aa bb/a.out...
    (gdb) file test/aa\ bb/a.out
    Load new symbol table from "test/aa bb/a.out"? (y or n) y
    Reading symbols from test/aa bb/a.out...
    (gdb)

The problem Andrew is trying to solve is that the current completer for
this command completes to something that is not a valid input.

Moreover, the completer as it is currently is is not entirely functional
for commands that expect "raw" filenames either.  If you take the same
directory structure, it can complete up to "test/aa bb", but can’t
complete the directory:

    $ gdb -q
    (gdb) complete file test/aa
    file test/aa bb
    (gdb) complete file test/aa bb
    (gdb) complete file test/aa bb/
    (gdb)

Where I do agree with you is that GDB is inconsistent: some command
expect "shell escaped filenames", and some do not.  For example, "set
logging file" will take its argument verbatim and use that:

    (gdb) set logging file "/tmp/test/aa bb/file.txt"
    (gdb) show logging file
    The current logfile is ""/tmp/test/aa bb/file.txt"".
    (gdb) set logging file /tmp/test/aa\ bb/file.txt
    (gdb) show logging file
    The current logfile is "/tmp/test/aa\ bb/file.txt".

Part of the issue you are raising is that both commands use the same
completer, so if the completer works for some commands, it will be wrong
for the others.  I have not reached the end of this series so I am not
sure if this is addressed (I do not expect it is), but I guess we should
either:
1) have 2 filename completers, and make sure that each function using a
   filename completer uses the appropriate one, or
2) have GDB be consistent regarding how commands consume filenames.

I would prefer consistency, but implementing 2 would strictly speaking
be an interface breaking change, so it needs careful consideration.

My experience is that I tend to use "file" / "add-symbol-file" more than
other commands dealing with filenames, so I would prefer to have a
completer that works for those.  Also, the current completer does not
completely work for commands that consume "raw" filenames anyway, so
having a completer that works properly for some commands rather than for
none sounds appealing to me.  That being said, this is just my 2 cent, I
am looking forward to hearing other people’s thoughts on the subject.

Best,
Lancelot.

> 
> So I'm not sure I understand what is being intended here, and I'm
> afraid you will uncover a lot of problems as you go with this (as you
> already discovered).
> 
> Apologies if I'm missing something important here.

Re: [PATCH 2/6] gdb: move display of completion results into completion_result class

[Eli Zaretskii]

> Date: Sat, 30 Mar 2024 23:30:38 +0000
> From: Lancelot SIX <lsix@lancelotsix.com>
> Cc: Andrew Burgess <aburgess@redhat.com>, gdb-patches@sourceware.org
> 
> On Fri, Mar 29, 2024 at 03:14:07PM +0300, Eli Zaretskii wrote:
> > > From: Andrew Burgess <aburgess@redhat.com>
> > > Cc: Andrew Burgess <aburgess@redhat.com>
> > > Date: Fri, 29 Mar 2024 11:42:28 +0000
> > > 
> > > When using the 'complete' command to complete file names we have some
> > > problems.  The suggested completion should include any required
> > > escaping, so if there is a filename '/tmp/aa"bb' (without the single
> > > quotes), then this should be displayed in the completion output like:
> > > 
> > >   (gdb) complete file /tmp/aa
> > >   file /tmp/aa\"bb
> > 
> > Why should it be displayed with the backslash?  And would the
> > completed name, if passed to a command, also have the backslash added?
> > If so, it could cause some commands to fail; basically, you are adding
> > shell file-name semantics into commands that don't work via the shell.
> 
> Hi,
> 
> The "file" command currently expects "shell file-name semantics":

For starters, this fact, if it is true, is not really documented.

>     $ gdb -q
>     (gdb) file test/aa bb/a.out
>     test/aa: No such file or directory.
>     (gdb) file "test/aa bb/a.out"
>     Reading symbols from test/aa bb/a.out...
>     (gdb) file test/aa\ bb/a.out
>     Load new symbol table from "test/aa bb/a.out"? (y or n) y
>     Reading symbols from test/aa bb/a.out...
>     (gdb)

Next, if 'file' indeed expects shell file-name semantics, then the
question is: which shell?  Should the above behave differently on, for
example, MS-Windows, since file-name quoting there works differently?
For example, escaping whitespace with a backslash will not work on
Windows.

Also, Andrew used the 'file' command in his examples, but completion
in GDB is used in many other commands, including those who don't
necessarily expect/need shell file-name semantics.

And last, but not least: the manual says about the 'complete' command:

     This is intended for use by GNU Emacs.

So one other thing we should consider is how Emacs handles these cases
and what it expects from GDB in response.

> The problem Andrew is trying to solve is that the current completer for
> this command completes to something that is not a valid input.

My point is that we need to have a clear idea what is a "valid input"
before we decide whether the current behavior is invalid, and if so,
how to fix it.  IOW, we should step back and discuss the valid
behavior first.  Andrew's patch series started from postulating a
certain desired result without any such discussion, and I think we
should discuss these aspects before we decide what is the desired
result.

> That being said, this is just my 2 cent, I am looking forward to
> hearing other people’s thoughts on the subject.

Same here.

Thanks.

Re: [PATCH 2/6] gdb: move display of completion results into completion_result class

[Andrew Burgess]

Eli Zaretskii <eliz@gnu.org> writes:

>> Date: Sat, 30 Mar 2024 23:30:38 +0000
>> From: Lancelot SIX <lsix@lancelotsix.com>
>> Cc: Andrew Burgess <aburgess@redhat.com>, gdb-patches@sourceware.org
>> 
>> On Fri, Mar 29, 2024 at 03:14:07PM +0300, Eli Zaretskii wrote:
>> > > From: Andrew Burgess <aburgess@redhat.com>
>> > > Cc: Andrew Burgess <aburgess@redhat.com>
>> > > Date: Fri, 29 Mar 2024 11:42:28 +0000
>> > > 
>> > > When using the 'complete' command to complete file names we have some
>> > > problems.  The suggested completion should include any required
>> > > escaping, so if there is a filename '/tmp/aa"bb' (without the single
>> > > quotes), then this should be displayed in the completion output like:
>> > > 
>> > >   (gdb) complete file /tmp/aa
>> > >   file /tmp/aa\"bb
>> > 
>> > Why should it be displayed with the backslash?  And would the
>> > completed name, if passed to a command, also have the backslash added?
>> > If so, it could cause some commands to fail; basically, you are adding
>> > shell file-name semantics into commands that don't work via the shell.
>> 
>> Hi,
>> 
>> The "file" command currently expects "shell file-name semantics":
>
> For starters, this fact, if it is true, is not really documented.

Agreed.  This can be improved.

>
>>     $ gdb -q
>>     (gdb) file test/aa bb/a.out
>>     test/aa: No such file or directory.
>>     (gdb) file "test/aa bb/a.out"
>>     Reading symbols from test/aa bb/a.out...
>>     (gdb) file test/aa\ bb/a.out
>>     Load new symbol table from "test/aa bb/a.out"? (y or n) y
>>     Reading symbols from test/aa bb/a.out...
>>     (gdb)
>
> Next, if 'file' indeed expects shell file-name semantics, then the
> question is: which shell?  Should the above behave differently on, for
> example, MS-Windows, since file-name quoting there works differently?
> For example, escaping whitespace with a backslash will not work on
> Windows.

I think calling this "shell file-name semantics" makes this change seem
worse than it is.  If we want to call it anything, then it should be
called gdb-shell semantics.

That is, this is how GDB quotes filenames.

This syntax isn't getting forwarded to any system shell, it remains
entirely within GDB, so I don't think we'd need to change the behaviour
for MS-Windows vs GNU/Linux.

> Also, Andrew used the 'file' command in his examples, but completion
> in GDB is used in many other commands, including those who don't
> necessarily expect/need shell file-name semantics.

These changes only impact filename completion, anything that doesn't
specifically invoke the filename completion function will not change its
behaviour.  If you see any examples of this then that is 100% a bug in
this work.

> And last, but not least: the manual says about the 'complete' command:
>
>      This is intended for use by GNU Emacs.
>
> So one other thing we should consider is how Emacs handles these cases
> and what it expects from GDB in response.

Happy to test such a thing.  Can you point me at any instruction/guides
for how to trigger completion via emacs?

My hope is that this change will fix things rather than break them.
Previously the 'complete' command would output a partial completion that
was invalid, that is, if the user passes emacs a short string and then
triggers completion, GDB would provide a longer string which was
invalid.  If emacs then feeds that longs string back to GDB the command
isn't going to work.  After this change that should no longer be the
case.

>
>> The problem Andrew is trying to solve is that the current completer for
>> this command completes to something that is not a valid input.
>
> My point is that we need to have a clear idea what is a "valid input"
> before we decide whether the current behavior is invalid, and if so,
> how to fix it.

I do disagree with the "decide whether the current behaviour is invalid"
part of this text.  Completion of files containing whitespace doesn't
work right now, so I'd suggest the current behaviour is self-evidently
invalid.  But ...

>                 IOW, we should step back and discuss the valid
> behavior first.

I'm totally happy to take suggestions on what the working behaviour
should look like.

>                  Andrew's patch series started from postulating a
> certain desired result without any such discussion, and I think we
> should discuss these aspects before we decide what is the desired
> result.

Sure.  My experience of proposing changes without patches is that folk
say, "Sure, sounds good, but how _exactly_ will it work.  What _exactly_
will it look like."  And honestly, I'm just not smart enough to answer
those questions without writing the code first.

I'm ALWAYS willing to rework changes based on feedback.  Please don't
think, just because I posted a patch that I'm saying it _has_ to be this
way.

That said, you email raises the idea concerns, but I'm still not clear
exactly _what_ your concerns are (other than emacs compatibility which
has been covered above).

I think there's something else which has been missed from this
discussion, which is super important when we talk about what the
"correct" behaviour should be:  I'm changing _completion_, I'm not
changing how GDB actually reads files names.

Right now if we want to pass a file containing whitespace to the 'file'
command then a user can either quote the filename, or escape the
whitespace.  That is _current_ behaviour, and is not something I'm
touching in this series.

This series makes the completion engine offer completions using those
schemes: if the user didn't include an opening quote then the whitespace
will be escaped.  If the filename did have an opening quote then no
escaping is necessary.  As such, it would seem that unless we want to
change how GDB currently parses filenames[*] then what completion should
do is pretty much prescribed by GDB's parsing rules...

I'll see if I can figure out the emacs thing, but if you do have some
hints that would be great.

Thanks,
Andrew




>
>> That being said, this is just my 2 cent, I am looking forward to
>> hearing other people’s thoughts on the subject.
>
> Same here.
>
> Thanks.

Re: [PATCH 2/6] gdb: move display of completion results into completion_result class

[Eli Zaretskii]

> From: Andrew Burgess <aburgess@redhat.com>
> Cc: gdb-patches@sourceware.org
> Date: Fri, 12 Apr 2024 18:24:31 +0100
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> > Next, if 'file' indeed expects shell file-name semantics, then the
> > question is: which shell?  Should the above behave differently on, for
> > example, MS-Windows, since file-name quoting there works differently?
> > For example, escaping whitespace with a backslash will not work on
> > Windows.
> 
> I think calling this "shell file-name semantics" makes this change seem
> worse than it is.  If we want to call it anything, then it should be
> called gdb-shell semantics.

What is "gdb-shell"?  Saying "gdb-shell semantics" is only useful if
the term "gdb-shell" is known and understood by many.  Otherwise, we
should not use this terminology, because it doesn't explain anything.

> > And last, but not least: the manual says about the 'complete' command:
> >
> >      This is intended for use by GNU Emacs.
> >
> > So one other thing we should consider is how Emacs handles these cases
> > and what it expects from GDB in response.
> 
> Happy to test such a thing.  Can you point me at any instruction/guides
> for how to trigger completion via emacs?

Invoke either "M-x gdb" or "M-x gud-gdb" from Emacs, and then, after
the debugging session starts, type TAB to complete file names in
commands that accept file names.

> My hope is that this change will fix things rather than break them.
> Previously the 'complete' command would output a partial completion that
> was invalid, that is, if the user passes emacs a short string and then
> triggers completion, GDB would provide a longer string which was
> invalid.  If emacs then feeds that longs string back to GDB the command
> isn't going to work.  After this change that should no longer be the
> case.

I hope you are right.  Let's see.  In general, Emacs doesn't need any
quoting when it receives file names from a sub-process (in this case,
from GDB).

> >> The problem Andrew is trying to solve is that the current completer for
> >> this command completes to something that is not a valid input.
> >
> > My point is that we need to have a clear idea what is a "valid input"
> > before we decide whether the current behavior is invalid, and if so,
> > how to fix it.
> 
> I do disagree with the "decide whether the current behaviour is invalid"
> part of this text.  Completion of files containing whitespace doesn't
> work right now, so I'd suggest the current behaviour is self-evidently
> invalid.  But ...
> 
> >                 IOW, we should step back and discuss the valid
> > behavior first.
> 
> I'm totally happy to take suggestions on what the working behaviour
> should look like.

I made one alternative suggestion about completing a file name that
begins with a quote, for example.  Whether it is a good suggestion
depends on what we want the behavior to be, so that should preferably
be discussed before deciding on the solution.

Thanks.

Re: [PATCH 2/6] gdb: move display of completion results into completion_result class

[Andrew Burgess]

Eli Zaretskii <eliz@gnu.org> writes:

>> From: Andrew Burgess <aburgess@redhat.com>
>> Cc: gdb-patches@sourceware.org
>> Date: Fri, 12 Apr 2024 18:24:31 +0100
>> 
>> Eli Zaretskii <eliz@gnu.org> writes:
>> 
>> > Next, if 'file' indeed expects shell file-name semantics, then the
>> > question is: which shell?  Should the above behave differently on, for
>> > example, MS-Windows, since file-name quoting there works differently?
>> > For example, escaping whitespace with a backslash will not work on
>> > Windows.
>> 
>> I think calling this "shell file-name semantics" makes this change seem
>> worse than it is.  If we want to call it anything, then it should be
>> called gdb-shell semantics.
>
> What is "gdb-shell"?  Saying "gdb-shell semantics" is only useful if
> the term "gdb-shell" is known and understood by many.  Otherwise, we
> should not use this terminology, because it doesn't explain anything.

It's a name I made up for the thing you type at after a (gdb) prompt.

The point is, it's not a "shell", it's GDB.  It has its own rules.

>
>> > And last, but not least: the manual says about the 'complete' command:
>> >
>> >      This is intended for use by GNU Emacs.
>> >
>> > So one other thing we should consider is how Emacs handles these cases
>> > and what it expects from GDB in response.
>> 
>> Happy to test such a thing.  Can you point me at any instruction/guides
>> for how to trigger completion via emacs?
>
> Invoke either "M-x gdb" or "M-x gud-gdb" from Emacs, and then, after
> the debugging session starts, type TAB to complete file names in
> commands that accept file names.

Thank you.  I got this working and did some tests, I'll write up my
results at the end of this email.

>
>> My hope is that this change will fix things rather than break them.
>> Previously the 'complete' command would output a partial completion that
>> was invalid, that is, if the user passes emacs a short string and then
>> triggers completion, GDB would provide a longer string which was
>> invalid.  If emacs then feeds that longs string back to GDB the command
>> isn't going to work.  After this change that should no longer be the
>> case.
>
> I hope you are right.  Let's see.  In general, Emacs doesn't need any
> quoting when it receives file names from a sub-process (in this case,
> from GDB).

See below.

>
>> >> The problem Andrew is trying to solve is that the current completer for
>> >> this command completes to something that is not a valid input.
>> >
>> > My point is that we need to have a clear idea what is a "valid input"
>> > before we decide whether the current behavior is invalid, and if so,
>> > how to fix it.
>> 
>> I do disagree with the "decide whether the current behaviour is invalid"
>> part of this text.  Completion of files containing whitespace doesn't
>> work right now, so I'd suggest the current behaviour is self-evidently
>> invalid.  But ...
>> 
>> >                 IOW, we should step back and discuss the valid
>> > behavior first.
>> 
>> I'm totally happy to take suggestions on what the working behaviour
>> should look like.
>
> I made one alternative suggestion about completing a file name that
> begins with a quote, for example.  Whether it is a good suggestion
> depends on what we want the behavior to be, so that should preferably
> be discussed before deciding on the solution.

OK.  But right now GDB uses readline for its shell/session.  And
readline completion works in a particular way.  We (GDB) don't get much
say over that unless we want to start seriously hacking on readline.

These patches aren't me coming up with some radical new approach and
trying to push it onto GDB.  This is just me connecting up readline and
GDB a little more fully, and letting readline do its thing.  How tab
completion works is really a readline feature.

Now where I have "invented" things, to some degree, is with the
'complete' command, which, as you point out, emacs uses.  My assumption
was that the 'complete' was used like this:

  1. User types a partial command in emacs buffer,

  2. User invokes completion,

  3. Emacs grabs the partial command and sends it to GDB's 'complete'
  command,

  4. GDB returns a set of partial completions,

  5. Emacs presents each completion to the user, and allows the user to
  select one,

  6. Emacs inserts the selected text into the buffer, extending the
  partially completed command,

  7. User hits <return> and the command is executed.

Having had a dig through gdb-mi.el and gud.el (from the emacs source) I
think I was pretty much correct with my assumptions.

And all the changes I've made to the 'complete' command have been
intended to support this use case, e.g. adding the escapes to the
'complete' command output.  Notice that readline by default does NOT add
the escaping, e.g.:

    (gdb) file /tmp/qqq/aa<tab>
        => file /tmp/qqq/aa\ <tab><tab>
    aa bb/ aa cc/

Here the ' ' in 'aa bb/' and 'aa cc/' is not escaped.  However, I made
sure that if I do:

    (gdb) complete file /tmp/eee/aa
    file /tmp/eee/aa\ bb/
    file /tmp/eee/aa\ cc/
    (gdb)

Then the whitespace is escaped, because if it wasn't and emacs just
dropped the text in without the escaping, then GDB would see two
separate words and do the wrong thing.

OK, so now the bad news.

My patched GDB doesn't "just work" with emacs.

Remember though, an unpatched GDB doesn't work either when trying to
complete a filename with spaces in, so it's not that I've made things
worse.

I tracked at least one bug down to this function in gud.el:

  (defun gud-gdb-completion-at-point ()
    "Return the data to complete the GDB command before point."
    (let ((end (point))
          (start
           (save-excursion
             (skip-chars-backward "^ " (comint-line-beginning-position))
             (point))))
      ;; FIXME: `gud-gdb-run-command-fetch-lines' has some nasty side-effects on
      ;; the buffer (via `gud-delete-prompt-marker'): it removes the prompt and
      ;; then re-adds it later, thus messing up markers and overlays along the
      ;; way (bug#18282).
      ;; We use an "insert-before" marker for `start', since it's typically right
      ;; after the prompt, which works around the problem, but is a hack (and
      ;; comes with other downsides, e.g. if completion adds text at `start').
      (list (copy-marker start t) end
            (completion-table-dynamic
             (apply-partially gud-gdb-completion-function
                              (buffer-substring (comint-line-beginning-position)
                                                start))))))
                                                
This is trying to figure out everything before the completion word on
the current line and doing the apply-partial with this context on
gud-gdb-completion-function.

So if we are completing 'file /tmp/abc<tab>' then we'd end up doing:

  (apply-partial gud-gdb-completion-function "file ")

The completion context is calculated as running from the start of the
line up to 'start', which is calculated in the 'let'.  And 'start' is
calculated by starting from 'point' and moving backward to the first
whitespace.

That's not going to work for completing any line that contains a space.

I think this function would need to get smarter to understand about
escaping and quoting.

And, finally, some good news.

If a filename _doesn't_ include any spaces, then as far as I can tell,
from my limited testing, everything works fine in emacs, and is now
improved.

On an unpatched GDB, given a file /tmp/qqq/f1, within emacs:

  (gdb) file /tmp/qq<tab>
     => file /tmp/qqq<tab>
     *Sole Completion*

That's it.  At this point emacs will not help me complete this at all.
I have to manually add the '/' and only then will emacs offer the next
level of completion.

Compare this to plain GDB:

  (gdb) file /tmp/qq<tab>
     => file /tmp/qqq/<tag>
     => file /tmp/qqq/f1

With a patched GDB, emacs is now as good as plain GDB.  This is thanks
to me adding the trailing '/' character in the 'complete' command
output.

And on the subject of quotes.  With an unpatched GDB emacs is perfectly
happy completing a file containing quotes (well, as happy as it is with
an unquoted file name), e.g.:

  (gdb) file "/tmp/qq<tab>
     => file "/tmp/qqq

But, when I get to the final file completion, emacs doesn't add the
trailing quote, so:

  (gdb) file "/tmp/qqq/f<tab>
    =>  file "/tmp/qqq/f1

In contrast and *unpatched* GDB at the CLI will add the final quote.

With a patched GDB, emacs is just as happy handling the quotes, only now
it adds the final trailing quote on, just like GDB's CLI:

  (gdb) file "/tmp/qqq/f<tab>
    =>  file "/tmp/qqq/f1"

OK, so summary of where we're at:

  + I think we're restricted to doing file completion at the GDB CLI the
    readline way.  I think trying to force readline to do things
    differently is just going to create maintenance overhead.  Of
    course, if someone wants to support such a thing, I love to see
    their plan.  But for now, I think we should keep it simple, do
    things the readline way.  Of course, if there's real problems that
    this causes then maybe we have to do things differently.  Let me
    know if any problems are found,

  + The complete command changes, as far as I can tell, only improve the
    emacs experience for filenames that _don't_ include white space.
    Nothing appears to be broken, but my testing has been limited.
    Please report any issues found,

  + For filenames that include white space, sadly, emacs doesn't "just
    work".  I've found one issue with the emacs code that isn't helping,
    but I suspect there's likely to be others beyond that.  That said,
    this is NOT a regression.  In fact, I think emacs is getting further
    through the completion process now (GDB is now sending it actual
    completions rather than junk), it's just emacs has never seen
    completions like this before (previous GDB would just send junk), so
    it's not really a surprise that this has exposed some bugs.  I'm
    willing to commit to creating emacs bugs for the completion features
    if/when this patch is merged so that these things can be addressed
    on the emacs side,

  + We are agreed that there's clearly a documentation issue.  However,
    I would argue that the documentation is missing for current
    functionality (i.e. the current rules for quoting and escaping file
    names).  We wouldn't document this stuff as part of "command
    completion", that doesn't really make sense I think.  I will start
    working on a separate patch to try and improve the documentation for
    how file names are quoted / escaped currently,

  + Lancelot has highlighted an issue with filename completion within
    'set' values, I haven't looked into that yet, but will do.

Are there any concerns that I haven't addressed?  I don't want to miss
anything that's concerning you.

Thanks,
Andrew

Re: [PATCH 2/6] gdb: move display of completion results into completion_result class

[Eli Zaretskii]

> From: Andrew Burgess <aburgess@redhat.com>
> Cc: lsix@lancelotsix.com, gdb-patches@sourceware.org
> Date: Fri, 12 Apr 2024 23:20:33 +0100
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> > What is "gdb-shell"?  Saying "gdb-shell semantics" is only useful if
> > the term "gdb-shell" is known and understood by many.  Otherwise, we
> > should not use this terminology, because it doesn't explain anything.
> 
> It's a name I made up for the thing you type at after a (gdb) prompt.
> 
> The point is, it's not a "shell", it's GDB.  It has its own rules.

Do we have those rules spelled out somewhere?  In the context of this
discussion, the pertinent rules are those for quoting in file names.
Are they documented?  And does this patch series modify those rules in
some way that users will need to know?

I guess this part:

>   + We are agreed that there's clearly a documentation issue.  However,
>     I would argue that the documentation is missing for current
>     functionality (i.e. the current rules for quoting and escaping file
>     names).  We wouldn't document this stuff as part of "command
>     completion", that doesn't really make sense I think.  I will start
>     working on a separate patch to try and improve the documentation for
>     how file names are quoted / escaped currently,

means the answer is NO.  So yes, it would be good to document those
rules in the manual (it would also be useful when someone would like
to fix the bugs in Emacs you mention below).

> OK, so now the bad news.
> 
> My patched GDB doesn't "just work" with emacs.
> 
> Remember though, an unpatched GDB doesn't work either when trying to
> complete a filename with spaces in, so it's not that I've made things
> worse.
> 
> I tracked at least one bug down to this function in gud.el:
> 
>   (defun gud-gdb-completion-at-point ()
>     "Return the data to complete the GDB command before point."
>     (let ((end (point))
>           (start
>            (save-excursion
>              (skip-chars-backward "^ " (comint-line-beginning-position))
>              (point))))
>       ;; FIXME: `gud-gdb-run-command-fetch-lines' has some nasty side-effects on
>       ;; the buffer (via `gud-delete-prompt-marker'): it removes the prompt and
>       ;; then re-adds it later, thus messing up markers and overlays along the
>       ;; way (bug#18282).
>       ;; We use an "insert-before" marker for `start', since it's typically right
>       ;; after the prompt, which works around the problem, but is a hack (and
>       ;; comes with other downsides, e.g. if completion adds text at `start').
>       (list (copy-marker start t) end
>             (completion-table-dynamic
>              (apply-partially gud-gdb-completion-function
>                               (buffer-substring (comint-line-beginning-position)
>                                                 start))))))
>                                                 
> This is trying to figure out everything before the completion word on
> the current line and doing the apply-partial with this context on
> gud-gdb-completion-function.
> 
> So if we are completing 'file /tmp/abc<tab>' then we'd end up doing:
> 
>   (apply-partial gud-gdb-completion-function "file ")
> 
> The completion context is calculated as running from the start of the
> line up to 'start', which is calculated in the 'let'.  And 'start' is
> calculated by starting from 'point' and moving backward to the first
> whitespace.
> 
> That's not going to work for completing any line that contains a space.
> 
> I think this function would need to get smarter to understand about
> escaping and quoting.

Yes.  Would you mind submitting a bug report about that, using the
command "M-x report-emacs-bug"?

> And, finally, some good news.
> 
> If a filename _doesn't_ include any spaces, then as far as I can tell,
> from my limited testing, everything works fine in emacs, and is now
> improved.
> 
> On an unpatched GDB, given a file /tmp/qqq/f1, within emacs:
> 
>   (gdb) file /tmp/qq<tab>
>      => file /tmp/qqq<tab>
>      *Sole Completion*
> 
> That's it.  At this point emacs will not help me complete this at all.
> I have to manually add the '/' and only then will emacs offer the next
> level of completion.
> 
> Compare this to plain GDB:
> 
>   (gdb) file /tmp/qq<tab>
>      => file /tmp/qqq/<tag>
>      => file /tmp/qqq/f1
> 
> With a patched GDB, emacs is now as good as plain GDB.  This is thanks
> to me adding the trailing '/' character in the 'complete' command
> output.
> 
> And on the subject of quotes.  With an unpatched GDB emacs is perfectly
> happy completing a file containing quotes (well, as happy as it is with
> an unquoted file name), e.g.:
> 
>   (gdb) file "/tmp/qq<tab>
>      => file "/tmp/qqq
> 
> But, when I get to the final file completion, emacs doesn't add the
> trailing quote, so:
> 
>   (gdb) file "/tmp/qqq/f<tab>
>     =>  file "/tmp/qqq/f1
> 
> In contrast and *unpatched* GDB at the CLI will add the final quote.
> 
> With a patched GDB, emacs is just as happy handling the quotes, only now
> it adds the final trailing quote on, just like GDB's CLI:
> 
>   (gdb) file "/tmp/qqq/f<tab>
>     =>  file "/tmp/qqq/f1"

SGTM, thanks.

> Are there any concerns that I haven't addressed?  I don't want to miss
> anything that's concerning you.

No, I don't think you've missed anything.  Thanks for working on this
important feature (I happen to be one of those completion junkies, so
any improvements in this area are welcome here).

Re: [PATCH 2/6] gdb: move display of completion results into completion_result class

[Andrew Burgess]

Eli Zaretskii <eliz@gnu.org> writes:

>> From: Andrew Burgess <aburgess@redhat.com>
>> Cc: lsix@lancelotsix.com, gdb-patches@sourceware.org
>> Date: Fri, 12 Apr 2024 23:20:33 +0100
>> 
>> OK, so now the bad news.
>> 
>> My patched GDB doesn't "just work" with emacs.
>> 
>> Remember though, an unpatched GDB doesn't work either when trying to
>> complete a filename with spaces in, so it's not that I've made things
>> worse.
>> 
>> I tracked at least one bug down to this function in gud.el:
>> 
>>   (defun gud-gdb-completion-at-point ()
>>     "Return the data to complete the GDB command before point."
>>     (let ((end (point))
>>           (start
>>            (save-excursion
>>              (skip-chars-backward "^ " (comint-line-beginning-position))
>>              (point))))
>>       ;; FIXME: `gud-gdb-run-command-fetch-lines' has some nasty side-effects on
>>       ;; the buffer (via `gud-delete-prompt-marker'): it removes the prompt and
>>       ;; then re-adds it later, thus messing up markers and overlays along the
>>       ;; way (bug#18282).
>>       ;; We use an "insert-before" marker for `start', since it's typically right
>>       ;; after the prompt, which works around the problem, but is a hack (and
>>       ;; comes with other downsides, e.g. if completion adds text at `start').
>>       (list (copy-marker start t) end
>>             (completion-table-dynamic
>>              (apply-partially gud-gdb-completion-function
>>                               (buffer-substring (comint-line-beginning-position)
>>                                                 start))))))
>>                                                 
>> This is trying to figure out everything before the completion word on
>> the current line and doing the apply-partial with this context on
>> gud-gdb-completion-function.
>> 
>> So if we are completing 'file /tmp/abc<tab>' then we'd end up doing:
>> 
>>   (apply-partial gud-gdb-completion-function "file ")
>> 
>> The completion context is calculated as running from the start of the
>> line up to 'start', which is calculated in the 'let'.  And 'start' is
>> calculated by starting from 'point' and moving backward to the first
>> whitespace.
>> 
>> That's not going to work for completing any line that contains a space.
>> 
>> I think this function would need to get smarter to understand about
>> escaping and quoting.
>
> Yes.  Would you mind submitting a bug report about that, using the
> command "M-x report-emacs-bug"?

I absolutely will file the bug report once this series (or whatever this
series becomes) is merged.  However, I wanted to see if there were
additional bugs hiding in the emacs code, these might indicate changes
that need to be made (or not made) on the GDB side.

So the good news is, that with a little hacking I managed to get emacs
working!

Disclaimer: My emacs lisp knowledge is pretty weak, so please consider
this a proof of concept rather than code I'm actually suggesting should
be merged into emacs.  When I file the bug I'll include this code again,
but by adding it here we can hopefully see what needs to change.

The new and updated emacs code is at the end of this email if anyone
wants to play with emacs and this new completion.  Drop this new code
into a file, load the emacs 'gdb' mode, then eval the new code.  This
adds some new functions, and replaces one existing function.

With this in place I now get completion for filenames containing
whitespace.  Given this directory tree:

  /tmp/eee/
  ├── aa bb
  │   ├── f1
  │   └── f2
  └── aa cc
      ├── g1
      └── g2

  (gdb) file /tmp/eee/aa<TAB>
    =>  file /tmp/eee/aa\ <TAB>
  Possible completions are:		<--- These are offered in a new buffer.
  /tmp/eee/aa\ bb/			<--- I select this entry.
  /tmp/eee/aa\ cc/
    => file /tmp/eee/aa\ bb/<TAB>
    => file /tmp/eee/aa\ bb/f<TAB>
  Possible completions are:		<--- These are offered in a new buffer.
  /tmp/eee/aa\ bb/f1
  /tmp/eee/aa\ bb/f2			<--- I select this entry.
    => file /tmp/eee/aa\ bb/f2<ENTER>
  ... gdb tries to load the file ...

And if also works with quoting:

  (gdb) file "/tmp/eee/a<TAB>
    =>  file "/tmp/eee/aa <TAB>
  Possible completions are:		<--- These are offered in a new buffer.
  "/tmp/eee/aa bb/
  "/tmp/eee/aa cc/			<--- I select this entry.
    =>  file "/tmp/eee/aa cc/<TAB>
    =>  file "/tmp/eee/aa cc/g<TAB>
  Possible completions are:		<--- These are offered in a new buffer.
  "/tmp/eee/aa cc/g1"			<--- I select this entry.
  "/tmp/eee/aa cc/g2"
    =>  file "/tmp/eee/aa cc/g1"<ENTER>
 ... gdb tries to load the file ...

As I said, I'm not claiming that emacs is "fixed", but I think I've
shown what needs to be done to get this working, and also I've
satisfied myself that everything can be made to work.

Thanks,
Andrew



---

;; New function: Return true if the character at POS is escaped, that
;; is, is the character as POS preceeded by a backslash.
(defun gud-gdb-is-char-escaped (pos)
  (char-equal ?\\ (char-before pos)))

;; New function: Move point forward as skip-chars-forward does,
;; passing CHARS and LIMIT to skip-chars-forward.  However, after
;; calling skip-chars-forward, if the character we are looking at is
;; escaped (with a backslash) then skip that character, and call
;; skip-chars-forward again.
(defun gud-gdb-skip-to-unquoted (chars limit)
  (while (and (< (point) limit)
              (progn
                (skip-chars-forward chars limit)
                (if (gud-gdb-is-char-escaped (point))
                    (progn
                      (forward-char)
                      t)
                  nil)))))

;; New function: Move point forward skipping over a 'word'.  A word
;; can be quoted, starts with a single or double quote, in which case
;; the corresponding quote marks the end of the word.  Or a word can
;; be unquoted, in which case the word ends at the next white space.
;;
;; Never move forward past LIMIT.
;;
;; In an unquoted word white space can be escaped.
;;
;; In a double quoted word, double quotes can be escaped.
(defun gud-gdb-forward-maybe-quoted-word (limit)
  (cond ((char-equal ?\' (char-after))
         (progn
           (forward-char)
           (skip-chars-forward "^'" limit)))
        ((char-equal ?\" (char-after))
         (progn
           (forward-char)
           (gud-gdb-skip-to-unquoted "^\"" limit)))
        (t
         (progn
           (gud-gdb-skip-to-unquoted "^[:space:]" limit)))))

;; Assuming point is at the start of a line, return the position for
;; the start of a completion word.  The point will be moved by calling
;; this function.
;;
;; Never search past LIMIT.
;;
;; The completion word is the last word on the line, the word might be
;; quoted though.
(defun gud-gdb-find-completion-word (limit)
  (let ((completion-word-start nil))
    (while (< (point) limit)
      (setf completion-word-start (point))
      (gud-gdb-forward-maybe-quoted-word limit)
      (skip-chars-forward " " limit))

    completion-word-start))

;; This replaces an existing emacs function.  I've changed the logic
;; for finding the completion word.
(defun gud-gdb-completion-at-point ()
  "Return the data to complete the GDB command before point."
  (let* ((end (point))
        (start
         (save-excursion
           (goto-char (comint-line-beginning-position))
           (gud-gdb-find-completion-word end))))
    ;; FIXME: `gud-gdb-run-command-fetch-lines' has some nasty side-effects on
    ;; the buffer (via `gud-delete-prompt-marker'): it removes the prompt and
    ;; then re-adds it later, thus messing up markers and overlays along the
    ;; way (bug#18282).
    ;; We use an "insert-before" marker for `start', since it's typically right
    ;; after the prompt, which works around the problem, but is a hack (and
    ;; comes with other downsides, e.g. if completion adds text at `start').
    (list (copy-marker start t) end
          (completion-table-dynamic
           (apply-partially gud-gdb-completion-function
                            (buffer-substring (comint-line-beginning-position)
                                              start))))))

Re: [PATCH 2/6] gdb: move display of completion results into completion_result class

[Eli Zaretskii]

> From: Andrew Burgess <aburgess@redhat.com>
> Cc: lsix@lancelotsix.com, gdb-patches@sourceware.org
> Date: Sat, 13 Apr 2024 10:09:52 +0100
> 
> > Yes.  Would you mind submitting a bug report about that, using the
> > command "M-x report-emacs-bug"?
> 
> I absolutely will file the bug report once this series (or whatever this
> series becomes) is merged.  However, I wanted to see if there were
> additional bugs hiding in the emacs code, these might indicate changes
> that need to be made (or not made) on the GDB side.
> 
> So the good news is, that with a little hacking I managed to get emacs
> working!
> 
> Disclaimer: My emacs lisp knowledge is pretty weak, so please consider
> this a proof of concept rather than code I'm actually suggesting should
> be merged into emacs.  When I file the bug I'll include this code again,
> but by adding it here we can hopefully see what needs to change.
> 
> The new and updated emacs code is at the end of this email if anyone
> wants to play with emacs and this new completion.  Drop this new code
> into a file, load the emacs 'gdb' mode, then eval the new code.  This
> adds some new functions, and replaces one existing function.
> 
> With this in place I now get completion for filenames containing
> whitespace.  Given this directory tree:
> 
>   /tmp/eee/
>   ├── aa bb
>   │   ├── f1
>   │   └── f2
>   └── aa cc
>       ├── g1
>       └── g2
> 
>   (gdb) file /tmp/eee/aa<TAB>
>     =>  file /tmp/eee/aa\ <TAB>
>   Possible completions are:		<--- These are offered in a new buffer.
>   /tmp/eee/aa\ bb/			<--- I select this entry.
>   /tmp/eee/aa\ cc/
>     => file /tmp/eee/aa\ bb/<TAB>
>     => file /tmp/eee/aa\ bb/f<TAB>
>   Possible completions are:		<--- These are offered in a new buffer.
>   /tmp/eee/aa\ bb/f1
>   /tmp/eee/aa\ bb/f2			<--- I select this entry.
>     => file /tmp/eee/aa\ bb/f2<ENTER>
>   ... gdb tries to load the file ...
> 
> And if also works with quoting:
> 
>   (gdb) file "/tmp/eee/a<TAB>
>     =>  file "/tmp/eee/aa <TAB>
>   Possible completions are:		<--- These are offered in a new buffer.
>   "/tmp/eee/aa bb/
>   "/tmp/eee/aa cc/			<--- I select this entry.
>     =>  file "/tmp/eee/aa cc/<TAB>
>     =>  file "/tmp/eee/aa cc/g<TAB>
>   Possible completions are:		<--- These are offered in a new buffer.
>   "/tmp/eee/aa cc/g1"			<--- I select this entry.
>   "/tmp/eee/aa cc/g2"
>     =>  file "/tmp/eee/aa cc/g1"<ENTER>
>  ... gdb tries to load the file ...
> 
> As I said, I'm not claiming that emacs is "fixed", but I think I've
> shown what needs to be done to get this working, and also I've
> satisfied myself that everything can be made to work.

Thanks, please include these patches with your bug report, so that our
completion experts could review them.

Re: [PATCH 2/6] gdb: move display of completion results into completion_result class

[Andrew Burgess]

Lancelot SIX <lsix@lancelotsix.com> writes:

> On Fri, Mar 29, 2024 at 03:14:07PM +0300, Eli Zaretskii wrote:
>> > From: Andrew Burgess <aburgess@redhat.com>
>> > Cc: Andrew Burgess <aburgess@redhat.com>
>> > Date: Fri, 29 Mar 2024 11:42:28 +0000
>> > 
>> > When using the 'complete' command to complete file names we have some
>> > problems.  The suggested completion should include any required
>> > escaping, so if there is a filename '/tmp/aa"bb' (without the single
>> > quotes), then this should be displayed in the completion output like:
>> > 
>> >   (gdb) complete file /tmp/aa
>> >   file /tmp/aa\"bb
>> 
>> Why should it be displayed with the backslash?  And would the
>> completed name, if passed to a command, also have the backslash added?
>> If so, it could cause some commands to fail; basically, you are adding
>> shell file-name semantics into commands that don't work via the shell.
>
> Hi,
>
> The "file" command currently expects "shell file-name semantics":
>
>     $ gdb -q
>     (gdb) file test/aa bb/a.out
>     test/aa: No such file or directory.
>     (gdb) file "test/aa bb/a.out"
>     Reading symbols from test/aa bb/a.out...
>     (gdb) file test/aa\ bb/a.out
>     Load new symbol table from "test/aa bb/a.out"? (y or n) y
>     Reading symbols from test/aa bb/a.out...
>     (gdb)
>
> The problem Andrew is trying to solve is that the current completer for
> this command completes to something that is not a valid input.
>
> Moreover, the completer as it is currently is is not entirely functional
> for commands that expect "raw" filenames either.  If you take the same
> directory structure, it can complete up to "test/aa bb", but can’t
> complete the directory:
>
>     $ gdb -q
>     (gdb) complete file test/aa
>     file test/aa bb
>     (gdb) complete file test/aa bb
>     (gdb) complete file test/aa bb/
>     (gdb)
>
> Where I do agree with you is that GDB is inconsistent: some command
> expect "shell escaped filenames", and some do not.  For example, "set
> logging file" will take its argument verbatim and use that:
>
>     (gdb) set logging file "/tmp/test/aa bb/file.txt"
>     (gdb) show logging file
>     The current logfile is ""/tmp/test/aa bb/file.txt"".
>     (gdb) set logging file /tmp/test/aa\ bb/file.txt
>     (gdb) show logging file
>     The current logfile is "/tmp/test/aa\ bb/file.txt".
>
> Part of the issue you are raising is that both commands use the same
> completer, so if the completer works for some commands, it will be wrong
> for the others.  I have not reached the end of this series so I am not
> sure if this is addressed (I do not expect it is), but I guess we should
> either:
> 1) have 2 filename completers, and make sure that each function using a
>    filename completer uses the appropriate one, or
> 2) have GDB be consistent regarding how commands consume filenames.
>
> I would prefer consistency, but implementing 2 would strictly speaking
> be an interface breaking change, so it needs careful consideration.

I wasn't aware of this difference.  I would also rather just have
consistent parsing.

The 'set logging file' command gets away with this parsing because the
filename is always the last thing on the command line, but for other
commands this is not always the case.

We could probably arrange a mostly backward compatible solution for this
case.  I'll see if I can spot any other problem cases.

Thanks,
Andrew

[PATCH 3/6] gdb: simplify completion_result::print_matches

[Andrew Burgess]

Simplify completion_result::print_matches by removing one of the code
paths.  Now, every time we call ::print_matches we always add the
trailing quote.

Previously, when using the 'complete' command, if there was only one
result then trailing quote was added in ::build_completion_result, but
when we had multiple results the trailing quote was added in
::print_matches.  As a consequence, ::print_matches had to understand
not to add the trailing quote for the single result case.

After this commit we don't add the trailing quote in
::build_completion_result, instead ::print_matches always adds the
trailing quote, which makes ::print_matches simpler.

However, there is a slight problem.  When completion is being driven
by readline, and not by the 'complete' command, we still need to
manually add the trailing quote in the single result case, and as the
printing is done by readline we can't add the quote at the time of
printing, and so, in ::build_completion_result, we still add the
trailing quote, but only when completion is being done for readline.

And this does cause a small problem.  When completing a filename, if
the completion results in a directory name then, when using the
'complete' command, GDB should not be adding a trailing quote.  For
example, if we have the file /tmp/xxx/foo.c, then what we should see
is this:

  (gdb) complete file '/tmp/xx
  file 'tmp/xxx/

But what we actually see after this commit is this:

  (gdb) complete file '/tmp/xx
  file 'tmp/xxx/'

Previously we didn't get the trailing quote in this case, as when
there is only a single result, the quote was added in
::build_completion_result, and for filename completion, GDB didn't
know what the quote character was in ::build_completion_result, so no
quote was added.  Now that the trailing quote is always added in
::print_matches, and GDB does know the quote character at this point,
so we are now getting the trailing quote, which is not correct.

This is a regression, but really, GDB is now broken in a consistent
way, if we create the file /tmp/xxa/bar.c, then previously if we did
this:

  (gdb) complete file '/tmp/xx
  file '/tmp/xxa/'
  file '/tmp/xxx/'

Notice how we get the trailing quote in this case, this is the before
patch behaviour, and is also wrong.

A later commit will fix things so that the trailing quote is not added
in this filename completion case, but for now I'm going to accept this
small regression.

This change in behaviour caused some failures in one of the completion
tests, I've tweaked the test case to expect the trailing quote as part
of this commit, but will revert this in a later commit in this series.

I've also added an extra test for when the 'complete' command does
complete to a single complete filename, in which case the trailing
quote is expected.
---
 gdb/completer.c                               | 62 +++++++++----------
 .../gdb.base/filename-completion.exp          | 17 ++++-
 2 files changed, 46 insertions(+), 33 deletions(-)

diff --git a/gdb/completer.c b/gdb/completer.c
index 9b4041da01a..2b3972213d8 100644
--- a/gdb/completer.c
+++ b/gdb/completer.c
@@ -2311,23 +2311,30 @@ completion_tracker::build_completion_result (const char *text,
 
   if (m_lowest_common_denominator_unique)
     {
-      /* We don't rely on readline appending the quote char as
-	 delimiter as then readline wouldn't append the ' ' after the
-	 completion.  */
-      char buf[2] = { (char) quote_char () };
-
-      match_list[0] = reconcat (match_list[0], match_list[0],
-				buf, (char *) NULL);
-      match_list[1] = NULL;
-
-      /* If the tracker wants to, or we already have a space at the
-	 end of the match, tell readline to skip appending
-	 another.  */
-      char *match = match_list[0];
-      bool completion_suppress_append
-	= (suppress_append_ws ()
-	   || (match[0] != '\0'
-	       && match[strlen (match) - 1] == ' '));
+      bool completion_suppress_append;
+
+      if (from_readline ())
+	{
+	  /* We don't rely on readline appending the quote char as
+	     delimiter as then readline wouldn't append the ' ' after the
+	     completion.  */
+	  char buf[2] = { (char) quote_char () };
+
+	  match_list[0] = reconcat (match_list[0], match_list[0], buf,
+				    (char *) nullptr);
+
+	  /* If the tracker wants to, or we already have a space at the end
+	     of the match, tell readline to skip appending another.  */
+	  char *match = match_list[0];
+	  completion_suppress_append
+	    = (suppress_append_ws ()
+	       || (match[0] != '\0'
+		   && match[strlen (match) - 1] == ' '));
+	}
+      else
+	completion_suppress_append = false;
+
+      match_list[1] = nullptr;
 
       return completion_result (match_list, 1, completion_suppress_append);
     }
@@ -2449,21 +2456,14 @@ void
 completion_result::print_matches (const std::string &prefix,
 				  const char *word, int quote_char)
 {
-  if (this->number_matches == 1)
-    printf_unfiltered ("%s%s\n", prefix.c_str (), this->match_list[0]);
-  else
-    {
-      this->sort_match_list ();
+  this->sort_match_list ();
 
-      for (size_t i = 0; i < this->number_matches; i++)
-	{
-	  printf_unfiltered ("%s%s", prefix.c_str (),
-			     this->match_list[i + 1]);
-	  if (quote_char)
-	    printf_unfiltered ("%c", quote_char);
-	  printf_unfiltered ("\n");
-	}
-    }
+  char buf[2] = { (char) quote_char, '\0' };
+  size_t off = this->number_matches == 1 ? 0 : 1;
+
+  for (size_t i = 0; i < this->number_matches; i++)
+    printf_unfiltered ("%s%s%s\n", prefix.c_str (),
+		       this->match_list[i + off], buf);
 
   if (this->number_matches == max_completions)
     {
diff --git a/gdb/testsuite/gdb.base/filename-completion.exp b/gdb/testsuite/gdb.base/filename-completion.exp
index f23e8671f40..66e5f411795 100644
--- a/gdb/testsuite/gdb.base/filename-completion.exp
+++ b/gdb/testsuite/gdb.base/filename-completion.exp
@@ -63,8 +63,21 @@ proc run_tests { root } {
 	test_gdb_complete_none "file ${qc}${root}/xx" \
 	    "expand a non-existent filename"
 
-	test_gdb_complete_unique "file ${qc}${root}/a" \
-	    "file ${qc}${root}/aaa/" "" false \
+	# The following test is split into separate cmd and tab calls as the
+	# cmd versions will add a closing quote.  It shouldn't be doing
+	# this; this will be fixed in a later commit.
+	test_gdb_complete_cmd_unique "file ${qc}${root}/a" \
+	    "file ${qc}${root}/aaa/${qc}" \
+	    "expand a unique directory name"
+
+	if { [readline_is_used] } {
+	    test_gdb_complete_tab_unique "file ${qc}${root}/a" \
+		"file ${qc}${root}/aaa/" "" \
+		"expand a unique directory name"
+	}
+
+	test_gdb_complete_unique "file ${qc}${root}/cc2" \
+	    "file ${qc}${root}/cc2${qc}" " " false \
 	    "expand a unique filename"
 
 	test_gdb_complete_multiple "file ${qc}${root}/" \
-- 
2.25.4

Re: [PATCH 3/6] gdb: simplify completion_result::print_matches

[Lancelot SIX]

Hi Andlew,

I have a couple of minor comment below.

> diff --git a/gdb/completer.c b/gdb/completer.c
> index 9b4041da01a..2b3972213d8 100644
> --- a/gdb/completer.c
> +++ b/gdb/completer.c
> @@ -2311,23 +2311,30 @@ completion_tracker::build_completion_result (const char *text,
>  
>    if (m_lowest_common_denominator_unique)
>      {
> -      /* We don't rely on readline appending the quote char as
> -	 delimiter as then readline wouldn't append the ' ' after the
> -	 completion.  */
> -      char buf[2] = { (char) quote_char () };
> -
> -      match_list[0] = reconcat (match_list[0], match_list[0],
> -				buf, (char *) NULL);
> -      match_list[1] = NULL;
> -
> -      /* If the tracker wants to, or we already have a space at the
> -	 end of the match, tell readline to skip appending
> -	 another.  */
> -      char *match = match_list[0];
> -      bool completion_suppress_append
> -	= (suppress_append_ws ()
> -	   || (match[0] != '\0'
> -	       && match[strlen (match) - 1] == ' '));
> +      bool completion_suppress_append;
> +
> +      if (from_readline ())
> +	{
> +	  /* We don't rely on readline appending the quote char as
> +	     delimiter as then readline wouldn't append the ' ' after the
> +	     completion.  */
> +	  char buf[2] = { (char) quote_char () };

To be consistent with what is used below, "{ (char) quote_char (), '\0' }"

> +
> +	  match_list[0] = reconcat (match_list[0], match_list[0], buf,
> +				    (char *) nullptr);
> +
> +	  /* If the tracker wants to, or we already have a space at the end
> +	     of the match, tell readline to skip appending another.  */
> +	  char *match = match_list[0];
> +	  completion_suppress_append
> +	    = (suppress_append_ws ()
> +	       || (match[0] != '\0'
> +		   && match[strlen (match) - 1] == ' '));
> +	}
> +      else
> +	completion_suppress_append = false;
> +
> +      match_list[1] = nullptr;
>  
>        return completion_result (match_list, 1, completion_suppress_append);
>      }
> @@ -2449,21 +2456,14 @@ void
>  completion_result::print_matches (const std::string &prefix,
>  				  const char *word, int quote_char)
>  {
> -  if (this->number_matches == 1)
> -    printf_unfiltered ("%s%s\n", prefix.c_str (), this->match_list[0]);
> -  else
> -    {
> -      this->sort_match_list ();
> +  this->sort_match_list ();
>  
> -      for (size_t i = 0; i < this->number_matches; i++)
> -	{
> -	  printf_unfiltered ("%s%s", prefix.c_str (),
> -			     this->match_list[i + 1]);
> -	  if (quote_char)
> -	    printf_unfiltered ("%c", quote_char);
> -	  printf_unfiltered ("\n");
> -	}
> -    }
> +  char buf[2] = { (char) quote_char, '\0' };
> +  size_t off = this->number_matches == 1 ? 0 : 1;
> +
> +  for (size_t i = 0; i < this->number_matches; i++)
> +    printf_unfiltered ("%s%s%s\n", prefix.c_str (),
> +		       this->match_list[i + off], buf);
>  
>    if (this->number_matches == max_completions)
>      {
> diff --git a/gdb/testsuite/gdb.base/filename-completion.exp b/gdb/testsuite/gdb.base/filename-completion.exp
> index f23e8671f40..66e5f411795 100644
> --- a/gdb/testsuite/gdb.base/filename-completion.exp
> +++ b/gdb/testsuite/gdb.base/filename-completion.exp
> @@ -63,8 +63,21 @@ proc run_tests { root } {
>  	test_gdb_complete_none "file ${qc}${root}/xx" \
>  	    "expand a non-existent filename"
>  
> -	test_gdb_complete_unique "file ${qc}${root}/a" \
> -	    "file ${qc}${root}/aaa/" "" false \
> +	# The following test is split into separate cmd and tab calls as the
> +	# cmd versions will add a closing quote.  It shouldn't be doing
> +	# this; this will be fixed in a later commit.

Instead of testing the "wrong" behavior, if you intend to fix that in a
later patch, I’d be tempted to prefer to just use "setup_kfail".

> +	test_gdb_complete_cmd_unique "file ${qc}${root}/a" \
> +	    "file ${qc}${root}/aaa/${qc}" \
> +	    "expand a unique directory name"
> +
> +	if { [readline_is_used] } {
> +	    test_gdb_complete_tab_unique "file ${qc}${root}/a" \
> +		"file ${qc}${root}/aaa/" "" \
> +		"expand a unique directory name"
> +	}
> +
> +	test_gdb_complete_unique "file ${qc}${root}/cc2" \
> +	    "file ${qc}${root}/cc2${qc}" " " false \
>  	    "expand a unique filename"
>  
>  	test_gdb_complete_multiple "file ${qc}${root}/" \
> -- 
> 2.25.4
> 

Best,
Lancelot.

[PATCH 4/6] gdb: add match formatter mechanism for 'complete' command output

[Andrew Burgess]

This commit solves the problem that was made worse in the previous
commit.  When completing a filename with the 'complete' command GDB
will always add a trailing quote character, even if the completion is
a directory name, in which case it would be better if the trailing
quote was not added.  Consider:

  (gdb) complete file '/tmp/xx
  file '/tmp/xxx/'

The completion offered here is really only a partial completion, we've
completed up to the end of the next directory name, but, until we have
a filename then the completion is not finished and the trailing quote
should not be added.

This would match the readline behaviour, e.g.:

  (gdb) file '/tmp/xx<TAB>
  (gdb) file '/tmp/xxx/

In this case readline completes the directory name, but doesn't add
the trailing quote character.

To achieve this, in this commit, I've added a new function pointer
member variable completion_result::m_match_formatter.  This contains a
pointer to a callback function which is used by the 'complete' command
to format each completion result.

The default behaviour of this callback function is to just append the
quote character (the character from before the completion string) to
the end of the completion result.

However, for filename completion we override the default value of
m_match_formatter, this new function checks if the completion result
is a directory or not.  If the completion result is a directory then
the closing quote is not added, instead we add a trailing '/'
character.

The code to add a trailing '/' character already exists within the
filename_completer function.  This is no longer needed in this
location, instead this code is move into the formatter callback.

Tests are updated to handle the changes in functionality.
---
 gdb/completer.c                               | 92 ++++++++++++++-----
 gdb/completer.h                               | 42 ++++++++-
 .../gdb.base/filename-completion.exp          | 66 +++++++++----
 3 files changed, 155 insertions(+), 45 deletions(-)

diff --git a/gdb/completer.c b/gdb/completer.c
index 2b3972213d8..785fb09b4d7 100644
--- a/gdb/completer.c
+++ b/gdb/completer.c
@@ -351,6 +351,34 @@ gdb_completer_file_name_quote (char *text, int match_type, char *quote_ptr)
   return strdup (str.c_str ());
 }
 
+/* The function is used to update the completion word MATCH before
+   displaying it to the user in the 'complete' command output, this
+   particular function is only used when MATCH has been supplied by the
+   filename_completer function (and so MATCH is a filename or directory
+   name).
+
+   This function checks to see if the completion word MATCH is a directory,
+   in which case a trailing "/" (forward-slash) is added, otherwise
+   QUOTE_CHAR is added as a trailing quote.
+
+   Return the updated completion word as a string.  */
+
+static std::string
+filename_match_formatter (const char *match, char quote_char)
+{
+  std::string expanded = gdb_tilde_expand (match);
+  struct stat finfo;
+  const bool isdir = (stat (expanded.c_str (), &finfo) == 0
+		      && S_ISDIR (finfo.st_mode));
+  std::string result (match);
+  if (isdir)
+    result += "/";
+  else
+    result += quote_char;
+
+  return result;
+}
+
 /* Complete on filenames.  */
 
 void
@@ -361,6 +389,8 @@ filename_completer (struct cmd_list_element *ignore,
   rl_char_is_quoted_p = gdb_completer_file_name_char_is_quoted;
   rl_completer_quote_characters = gdb_completer_file_name_quote_characters;
 
+  tracker.set_match_format_func (filename_match_formatter);
+
   int subsequent_name = 0;
   while (1)
     {
@@ -379,20 +409,6 @@ filename_completer (struct cmd_list_element *ignore,
       if (p[strlen (p) - 1] == '~')
 	continue;
 
-      /* Readline appends a trailing '/' if the completion is a
-	 directory.  If this completion request originated from outside
-	 readline (e.g. GDB's 'complete' command), then we append the
-	 trailing '/' ourselves now.  */
-      if (!tracker.from_readline ())
-	{
-	  std::string expanded = gdb_tilde_expand (p_rl.get ());
-	  struct stat finfo;
-	  const bool isdir = (stat (expanded.c_str (), &finfo) == 0
-			      && S_ISDIR (finfo.st_mode));
-	  if (isdir)
-	    p_rl.reset (concat (p_rl.get (), "/", nullptr));
-	}
-
       tracker.add_completion
 	(make_completion_match_str (std::move (p_rl), word, word));
     }
@@ -1630,10 +1646,25 @@ int max_completions = 200;
 /* Initial size of the table.  It automagically grows from here.  */
 #define INITIAL_COMPLETION_HTAB_SIZE 200
 
+/* The function is used to update the completion word MATCH before
+   displaying it to the user in the 'complete' command output.  This
+   default function is used in all cases except those where a completion
+   function overrides this function by calling set_match_format_func.
+
+   This function returns MATCH with QUOTE_CHAR appended.  If QUOTE_CHAR is
+   the null-character then the returned string will just contain MATCH.  */
+
+static std::string
+default_match_formatter (const char *match, char quote_char)
+{
+  return std::string (match) + quote_char;
+}
+
 /* See completer.h.  */
 
 completion_tracker::completion_tracker (bool from_readline)
-  : m_from_readline (from_readline)
+  : m_from_readline (from_readline),
+    m_match_format_func (default_match_formatter)
 {
   discard_completions ();
 }
@@ -2336,7 +2367,8 @@ completion_tracker::build_completion_result (const char *text,
 
       match_list[1] = nullptr;
 
-      return completion_result (match_list, 1, completion_suppress_append);
+      return completion_result (match_list, 1, completion_suppress_append,
+				m_match_format_func);
     }
   else
     {
@@ -2373,7 +2405,8 @@ completion_tracker::build_completion_result (const char *text,
       htab_traverse_noresize (m_entries_hash.get (), func, &builder);
       match_list[builder.index] = NULL;
 
-      return completion_result (match_list, builder.index - 1, false);
+      return completion_result (match_list, builder.index - 1, false,
+				m_match_format_func);
     }
 }
 
@@ -2381,17 +2414,20 @@ completion_tracker::build_completion_result (const char *text,
 
 completion_result::completion_result ()
   : match_list (NULL), number_matches (0),
-    completion_suppress_append (false)
+    completion_suppress_append (false),
+    m_match_formatter (nullptr)
 {}
 
 /* See completer.h  */
 
 completion_result::completion_result (char **match_list_,
 				      size_t number_matches_,
-				      bool completion_suppress_append_)
+				      bool completion_suppress_append_,
+				      match_format_func_t match_formatter_)
   : match_list (match_list_),
     number_matches (number_matches_),
-    completion_suppress_append (completion_suppress_append_)
+    completion_suppress_append (completion_suppress_append_),
+    m_match_formatter (match_formatter_)
 {}
 
 /* See completer.h  */
@@ -2405,10 +2441,12 @@ completion_result::~completion_result ()
 
 completion_result::completion_result (completion_result &&rhs) noexcept
   : match_list (rhs.match_list),
-    number_matches (rhs.number_matches)
+    number_matches (rhs.number_matches),
+    m_match_formatter (rhs.m_match_formatter)
 {
   rhs.match_list = NULL;
   rhs.number_matches = 0;
+  rhs.m_match_formatter = nullptr;
 }
 
 /* See completer.h  */
@@ -2458,12 +2496,18 @@ completion_result::print_matches (const std::string &prefix,
 {
   this->sort_match_list ();
 
-  char buf[2] = { (char) quote_char, '\0' };
   size_t off = this->number_matches == 1 ? 0 : 1;
 
   for (size_t i = 0; i < this->number_matches; i++)
-    printf_unfiltered ("%s%s%s\n", prefix.c_str (),
-		       this->match_list[i + off], buf);
+    {
+      gdb_assert (this->m_match_formatter != nullptr);
+      std::string formatted_match
+	= this->m_match_formatter (this->match_list[i + off],
+				   (char) quote_char);
+
+      printf_unfiltered ("%s%s\n", prefix.c_str (),
+			 formatted_match.c_str ());
+    }
 
   if (this->number_matches == max_completions)
     {
diff --git a/gdb/completer.h b/gdb/completer.h
index 4419c8f6d30..8965ecacc44 100644
--- a/gdb/completer.h
+++ b/gdb/completer.h
@@ -247,12 +247,24 @@ struct completion_match_result
 
 struct completion_result
 {
+  /* The type of a function that is used to format completion results when
+     using the 'complete' command.  MATCH is the completion word to be
+     printed, and QUOTE_CHAR is a trailing quote character to (possibly)
+     add at the end of MATCH.  QUOTE_CHAR can be the null-character in
+     which case no trailing quote should be added.
+
+     Return the possibly modified completion match word which should be
+     presented to the user.  */
+  using match_format_func_t = std::string (*) (const char *match,
+					       char quote_char);
+
   /* Create an empty result.  */
   completion_result ();
 
   /* Create a result.  */
   completion_result (char **match_list, size_t number_matches,
-		     bool completion_suppress_append);
+		     bool completion_suppress_append,
+		     match_format_func_t match_format_func);
 
   /* Destroy a result.  */
   ~completion_result ();
@@ -274,10 +286,15 @@ struct completion_result
      completions, otherwise, each line of output consists of PREFIX
      followed by one of the possible completion words.
 
-     The QUOTE_CHAR is appended after each possible completion word and
-     should be the quote character that appears before the completion word,
-     or the null-character if there is no quote before the completion
-     word.  */
+     The QUOTE_CHAR is usually appended after each possible completion
+     word and should be the quote character that appears before the
+     completion word, or the null-character if there is no quote before
+     the completion word.
+
+     The QUOTE_CHAR is not always appended to the completion output.  For
+     example, filename completions will not append QUOTE_CHAR if the
+     completion is a directory name.  This is all handled by calling this
+     function.  */
   void print_matches (const std::string &prefix, const char *word,
 		      int quote_char);
 
@@ -305,6 +322,12 @@ struct completion_result
   /* Whether readline should suppress appending a whitespace, when
      there's only one possible completion.  */
   bool completion_suppress_append;
+
+private:
+  /* A function which formats a single completion match ready for display
+     as part of the 'complete' command output.  Different completion
+     functions can set different formatter functions.  */
+  match_format_func_t m_match_formatter;
 };
 
 /* Object used by completers to build a completion match list to hand
@@ -441,6 +464,11 @@ class completion_tracker
   bool from_readline () const
   { return m_from_readline; }
 
+  /* Set the function used to format the completion word before displaying
+     it to the user to F, this is used by the 'complete' command.  */
+  void set_match_format_func (completion_result::match_format_func_t f)
+  { m_match_format_func = f; }
+
 private:
 
   /* The type that we place into the m_entries_hash hash table.  */
@@ -535,6 +563,10 @@ class completion_tracker
      interactively. The 'complete' command is a way to generate completions
      not to be displayed by readline.  */
   bool m_from_readline;
+
+  /* The function used to format the completion word before it is printed
+     in the 'complete' command output.  */
+  completion_result::match_format_func_t m_match_format_func;
 };
 
 /* Return a string to hand off to readline as a completion match
diff --git a/gdb/testsuite/gdb.base/filename-completion.exp b/gdb/testsuite/gdb.base/filename-completion.exp
index 66e5f411795..d7c99e1340d 100644
--- a/gdb/testsuite/gdb.base/filename-completion.exp
+++ b/gdb/testsuite/gdb.base/filename-completion.exp
@@ -46,6 +46,49 @@ proc setup_directory_tree {} {
     return $root
 }
 
+# This proc started as a copy of test_gdb_complete_multiple, however, this
+# version does some extra work.  See the original test_gdb_complete_multiple
+# for a description of all the arguments.
+#
+# When using the 'complete' command with filenames, GDB will add a trailing
+# quote for filenames, and a trailing "/" for directory names.  As the
+# trailing "/" is also added in the tab-completion output the
+# COMPLETION_LIST will include the "/" character, but the trailing quote is
+# only added when using the 'complete' command.
+#
+# Pass the trailing quote will be passed as END_QUOTE_CHAR, this proc will
+# run the tab completion test, and will then add the trailing quote to those
+# entries in COMPLETION_LIST that don't have a trailing "/" before running
+# the 'complete' command test.
+proc test_gdb_complete_filename_multiple {
+  cmd_prefix completion_word add_completed_line completion_list
+  {start_quote_char ""} {end_quote_char ""} {max_completions false}
+  {testname ""}
+} {
+    if { [readline_is_used] } {
+	test_gdb_complete_tab_multiple "$cmd_prefix$completion_word" \
+	    $add_completed_line $completion_list $max_completions $testname
+    }
+
+    if { $start_quote_char eq "" && $end_quote_char ne "" } {
+	set updated_completion_list {}
+
+	foreach entry $completion_list {
+	    if {[string range $entry end end] ne "/"} {
+		set entry $entry$end_quote_char
+	    }
+	    lappend updated_completion_list $entry
+	}
+
+	set completion_list $updated_completion_list
+	set end_quote_char ""
+    }
+
+    test_gdb_complete_cmd_multiple $cmd_prefix $completion_word \
+	$completion_list $start_quote_char $end_quote_char $max_completions \
+	$testname
+}
+
 # Run filename completetion tests.  ROOT is the base directory as
 # returned from setup_directory_tree, though, if ROOT is a
 # sub-directory of the user's home directory ROOT might have been
@@ -63,31 +106,22 @@ proc run_tests { root } {
 	test_gdb_complete_none "file ${qc}${root}/xx" \
 	    "expand a non-existent filename"
 
-	# The following test is split into separate cmd and tab calls as the
-	# cmd versions will add a closing quote.  It shouldn't be doing
-	# this; this will be fixed in a later commit.
-	test_gdb_complete_cmd_unique "file ${qc}${root}/a" \
-	    "file ${qc}${root}/aaa/${qc}" \
+	test_gdb_complete_unique "file ${qc}${root}/a" \
+	    "file ${qc}${root}/aaa/" "" false \
 	    "expand a unique directory name"
 
-	if { [readline_is_used] } {
-	    test_gdb_complete_tab_unique "file ${qc}${root}/a" \
-		"file ${qc}${root}/aaa/" "" \
-		"expand a unique directory name"
-	}
-
 	test_gdb_complete_unique "file ${qc}${root}/cc2" \
 	    "file ${qc}${root}/cc2${qc}" " " false \
 	    "expand a unique filename"
 
-	test_gdb_complete_multiple "file ${qc}${root}/" \
+	test_gdb_complete_filename_multiple "file ${qc}${root}/" \
 	    "b" "b" {
 		"bb1/"
 		"bb2/"
-	    } "" "${qc}" false \
+	    } "" "" false \
 	    "expand multiple directory names"
 
-	test_gdb_complete_multiple "file ${qc}${root}/" \
+	test_gdb_complete_filename_multiple "file ${qc}${root}/" \
 	    "c" "c" {
 		"cc1/"
 		"cc2"
@@ -99,14 +133,14 @@ proc run_tests { root } {
 	if { $qc ne "" } {
 	    set sp " "
 
-	    test_gdb_complete_multiple "file ${qc}${root}/aaa/" \
+	    test_gdb_complete_filename_multiple "file ${qc}${root}/aaa/" \
 		"a" "a${sp}" {
 		    "aa bb"
 		    "aa cc"
 		} "" "${qc}" false \
 		"expand filenames containing spaces"
 
-	    test_gdb_complete_multiple "file ${qc}${root}/bb1/" \
+	    test_gdb_complete_filename_multiple "file ${qc}${root}/bb1/" \
 		"a" "a" {
 		    "aa\"bb"
 		    "aa'bb"
-- 
2.25.4

Re: [PATCH 4/6] gdb: add match formatter mechanism for 'complete' command output

[Lancelot SIX]

Hi Andrew,

A couple of comments below.

On Fri, Mar 29, 2024 at 11:42:30AM +0000, Andrew Burgess wrote:
> This commit solves the problem that was made worse in the previous
> commit.  When completing a filename with the 'complete' command GDB
> will always add a trailing quote character, even if the completion is
> a directory name, in which case it would be better if the trailing
> quote was not added.  Consider:
> 
>   (gdb) complete file '/tmp/xx
>   file '/tmp/xxx/'
> 
> The completion offered here is really only a partial completion, we've
> completed up to the end of the next directory name, but, until we have
> a filename then the completion is not finished and the trailing quote
> should not be added.
> 
> This would match the readline behaviour, e.g.:
> 
>   (gdb) file '/tmp/xx<TAB>
>   (gdb) file '/tmp/xxx/
> 
> In this case readline completes the directory name, but doesn't add
> the trailing quote character.
> 
> To achieve this, in this commit, I've added a new function pointer
> member variable completion_result::m_match_formatter.  This contains a
> pointer to a callback function which is used by the 'complete' command
> to format each completion result.
> 
> The default behaviour of this callback function is to just append the
> quote character (the character from before the completion string) to
> the end of the completion result.
> 
> However, for filename completion we override the default value of
> m_match_formatter, this new function checks if the completion result
> is a directory or not.  If the completion result is a directory then
> the closing quote is not added, instead we add a trailing '/'
> character.
> 
> The code to add a trailing '/' character already exists within the
> filename_completer function.  This is no longer needed in this
> location, instead this code is move into the formatter callback.
> 
> Tests are updated to handle the changes in functionality.
> ---
>  gdb/completer.c                               | 92 ++++++++++++++-----
>  gdb/completer.h                               | 42 ++++++++-
>  .../gdb.base/filename-completion.exp          | 66 +++++++++----
>  3 files changed, 155 insertions(+), 45 deletions(-)
> 
> diff --git a/gdb/completer.c b/gdb/completer.c
> index 2b3972213d8..785fb09b4d7 100644
> --- a/gdb/completer.c
> +++ b/gdb/completer.c
> @@ -351,6 +351,34 @@ gdb_completer_file_name_quote (char *text, int match_type, char *quote_ptr)
>    return strdup (str.c_str ());
>  }
>  
> +/* The function is used to update the completion word MATCH before
> +   displaying it to the user in the 'complete' command output, this
> +   particular function is only used when MATCH has been supplied by the
> +   filename_completer function (and so MATCH is a filename or directory
> +   name).
> +
> +   This function checks to see if the completion word MATCH is a directory,
> +   in which case a trailing "/" (forward-slash) is added, otherwise
> +   QUOTE_CHAR is added as a trailing quote.
> +
> +   Return the updated completion word as a string.  */
> +
> +static std::string
> +filename_match_formatter (const char *match, char quote_char)
> +{
> +  std::string expanded = gdb_tilde_expand (match);
> +  struct stat finfo;
> +  const bool isdir = (stat (expanded.c_str (), &finfo) == 0
> +		      && S_ISDIR (finfo.st_mode));

This could be "std::filesystem::is_derictory (expanded)".  Not sure if
there is a strong benefit though.

> +  std::string result (match);
> +  if (isdir)
> +    result += "/";
> +  else
> +    result += quote_char;
> +
> +  return result;
> +}
> +
>  /* Complete on filenames.  */
>  
>  void
> @@ -361,6 +389,8 @@ filename_completer (struct cmd_list_element *ignore,
>    rl_char_is_quoted_p = gdb_completer_file_name_char_is_quoted;
>    rl_completer_quote_characters = gdb_completer_file_name_quote_characters;
>  
> +  tracker.set_match_format_func (filename_match_formatter);
> +
>    int subsequent_name = 0;
>    while (1)
>      {
> @@ -379,20 +409,6 @@ filename_completer (struct cmd_list_element *ignore,
>        if (p[strlen (p) - 1] == '~')
>  	continue;
>  
> -      /* Readline appends a trailing '/' if the completion is a
> -	 directory.  If this completion request originated from outside
> -	 readline (e.g. GDB's 'complete' command), then we append the
> -	 trailing '/' ourselves now.  */
> -      if (!tracker.from_readline ())
> -	{
> -	  std::string expanded = gdb_tilde_expand (p_rl.get ());
> -	  struct stat finfo;
> -	  const bool isdir = (stat (expanded.c_str (), &finfo) == 0
> -			      && S_ISDIR (finfo.st_mode));
> -	  if (isdir)
> -	    p_rl.reset (concat (p_rl.get (), "/", nullptr));
> -	}
> -
>        tracker.add_completion
>  	(make_completion_match_str (std::move (p_rl), word, word));
>      }
> @@ -1630,10 +1646,25 @@ int max_completions = 200;
>  /* Initial size of the table.  It automagically grows from here.  */
>  #define INITIAL_COMPLETION_HTAB_SIZE 200
>  
> +/* The function is used to update the completion word MATCH before
> +   displaying it to the user in the 'complete' command output.  This
> +   default function is used in all cases except those where a completion
> +   function overrides this function by calling set_match_format_func.
> +
> +   This function returns MATCH with QUOTE_CHAR appended.  If QUOTE_CHAR is
> +   the null-character then the returned string will just contain MATCH.  */
> +
> +static std::string
> +default_match_formatter (const char *match, char quote_char)
> +{
> +  return std::string (match) + quote_char;
> +}
> +
>  /* See completer.h.  */
>  
>  completion_tracker::completion_tracker (bool from_readline)
> -  : m_from_readline (from_readline)
> +  : m_from_readline (from_readline),
> +    m_match_format_func (default_match_formatter)
>  {
>    discard_completions ();
>  }
> @@ -2336,7 +2367,8 @@ completion_tracker::build_completion_result (const char *text,
>  
>        match_list[1] = nullptr;
>  
> -      return completion_result (match_list, 1, completion_suppress_append);
> +      return completion_result (match_list, 1, completion_suppress_append,
> +				m_match_format_func);
>      }
>    else
>      {
> @@ -2373,7 +2405,8 @@ completion_tracker::build_completion_result (const char *text,
>        htab_traverse_noresize (m_entries_hash.get (), func, &builder);
>        match_list[builder.index] = NULL;
>  
> -      return completion_result (match_list, builder.index - 1, false);
> +      return completion_result (match_list, builder.index - 1, false,
> +				m_match_format_func);
>      }
>  }
>  
> @@ -2381,17 +2414,20 @@ completion_tracker::build_completion_result (const char *text,
>  
>  completion_result::completion_result ()
>    : match_list (NULL), number_matches (0),
> -    completion_suppress_append (false)
> +    completion_suppress_append (false),
> +    m_match_formatter (nullptr)

Is there a reason not to initialize this with default_match_formatter?

Ensuring that we can not assign nullptr to the m_match_formatter field
could avoid the need of checking that this field is not nullptr before
using it, and more importantly it makes it easier to track down an issue
if someone tries to set it to nullptr at some point.

We could also define match_fonmat_func_t to be

    using match_format_func_t
      = std::function<std::stirng (const char *match, char quote_char)>;

and have

    match_format_func_t m_match_formatter;

In which case I do not think we can have a non-valid callable stored in
_match_formatter.  That being said, std::function is not a light
abstraction (as far a I know), so I can understand if you would prefer
not to go that route.

>  {}
>  
>  /* See completer.h  */
>  
>  completion_result::completion_result (char **match_list_,
>  				      size_t number_matches_,
> -				      bool completion_suppress_append_)
> +				      bool completion_suppress_append_,
> +				      match_format_func_t match_formatter_)
>    : match_list (match_list_),
>      number_matches (number_matches_),
> -    completion_suppress_append (completion_suppress_append_)
> +    completion_suppress_append (completion_suppress_append_),
> +    m_match_formatter (match_formatter_)
>  {}
>  
>  /* See completer.h  */
> @@ -2405,10 +2441,12 @@ completion_result::~completion_result ()
>  
>  completion_result::completion_result (completion_result &&rhs) noexcept
>    : match_list (rhs.match_list),
> -    number_matches (rhs.number_matches)
> +    number_matches (rhs.number_matches),
> +    m_match_formatter (rhs.m_match_formatter)
>  {
>    rhs.match_list = NULL;
>    rhs.number_matches = 0;
> +  rhs.m_match_formatter = nullptr;

Similarly, we might want to reset to a legal value here (i.e.
default_match_formatter).

>  }
>  
>  /* See completer.h  */
> @@ -2458,12 +2496,18 @@ completion_result::print_matches (const std::string &prefix,
>  {
>    this->sort_match_list ();
>  
> -  char buf[2] = { (char) quote_char, '\0' };
>    size_t off = this->number_matches == 1 ? 0 : 1;
>  
>    for (size_t i = 0; i < this->number_matches; i++)
> -    printf_unfiltered ("%s%s%s\n", prefix.c_str (),
> -		       this->match_list[i + off], buf);
> +    {
> +      gdb_assert (this->m_match_formatter != nullptr);
> +      std::string formatted_match
> +	= this->m_match_formatter (this->match_list[i + off],
> +				   (char) quote_char);
> +
> +      printf_unfiltered ("%s%s\n", prefix.c_str (),
> +			 formatted_match.c_str ());
> +    }
>  
>    if (this->number_matches == max_completions)
>      {
> diff --git a/gdb/completer.h b/gdb/completer.h
> index 4419c8f6d30..8965ecacc44 100644
> --- a/gdb/completer.h
> +++ b/gdb/completer.h
> @@ -247,12 +247,24 @@ struct completion_match_result
>  
>  struct completion_result
>  {
> +  /* The type of a function that is used to format completion results when
> +     using the 'complete' command.  MATCH is the completion word to be
> +     printed, and QUOTE_CHAR is a trailing quote character to (possibly)
> +     add at the end of MATCH.  QUOTE_CHAR can be the null-character in
> +     which case no trailing quote should be added.
> +
> +     Return the possibly modified completion match word which should be
> +     presented to the user.  */
> +  using match_format_func_t = std::string (*) (const char *match,
> +					       char quote_char);
> +
>    /* Create an empty result.  */
>    completion_result ();
>  
>    /* Create a result.  */
>    completion_result (char **match_list, size_t number_matches,
> -		     bool completion_suppress_append);
> +		     bool completion_suppress_append,
> +		     match_format_func_t match_format_func);
>  
>    /* Destroy a result.  */
>    ~completion_result ();
> @@ -274,10 +286,15 @@ struct completion_result
>       completions, otherwise, each line of output consists of PREFIX
>       followed by one of the possible completion words.
>  
> -     The QUOTE_CHAR is appended after each possible completion word and
> -     should be the quote character that appears before the completion word,
> -     or the null-character if there is no quote before the completion
> -     word.  */
> +     The QUOTE_CHAR is usually appended after each possible completion
> +     word and should be the quote character that appears before the
> +     completion word, or the null-character if there is no quote before
> +     the completion word.
> +
> +     The QUOTE_CHAR is not always appended to the completion output.  For
> +     example, filename completions will not append QUOTE_CHAR if the
> +     completion is a directory name.  This is all handled by calling this
> +     function.  */
>    void print_matches (const std::string &prefix, const char *word,
>  		      int quote_char);
>  
> @@ -305,6 +322,12 @@ struct completion_result
>    /* Whether readline should suppress appending a whitespace, when
>       there's only one possible completion.  */
>    bool completion_suppress_append;
> +
> +private:
> +  /* A function which formats a single completion match ready for display
> +     as part of the 'complete' command output.  Different completion
> +     functions can set different formatter functions.  */
> +  match_format_func_t m_match_formatter;
>  };
>  
>  /* Object used by completers to build a completion match list to hand
> @@ -441,6 +464,11 @@ class completion_tracker
>    bool from_readline () const
>    { return m_from_readline; }
>  
> +  /* Set the function used to format the completion word before displaying
> +     it to the user to F, this is used by the 'complete' command.  */
> +  void set_match_format_func (completion_result::match_format_func_t f)
> +  { m_match_format_func = f; }

If you prefer to keep a plain function pointer, I’d have an assertion
here to ensure the field is not set to nullptr (just to make it easier
to track down invalid usage).

Best,
Lancelot.

> +
>  private:
>  
>    /* The type that we place into the m_entries_hash hash table.  */
> @@ -535,6 +563,10 @@ class completion_tracker
>       interactively. The 'complete' command is a way to generate completions
>       not to be displayed by readline.  */
>    bool m_from_readline;
> +
> +  /* The function used to format the completion word before it is printed
> +     in the 'complete' command output.  */
> +  completion_result::match_format_func_t m_match_format_func;
>  };
>  
>  /* Return a string to hand off to readline as a completion match
> diff --git a/gdb/testsuite/gdb.base/filename-completion.exp b/gdb/testsuite/gdb.base/filename-completion.exp
> index 66e5f411795..d7c99e1340d 100644
> --- a/gdb/testsuite/gdb.base/filename-completion.exp
> +++ b/gdb/testsuite/gdb.base/filename-completion.exp
> @@ -46,6 +46,49 @@ proc setup_directory_tree {} {
>      return $root
>  }
>  
> +# This proc started as a copy of test_gdb_complete_multiple, however, this
> +# version does some extra work.  See the original test_gdb_complete_multiple
> +# for a description of all the arguments.
> +#
> +# When using the 'complete' command with filenames, GDB will add a trailing
> +# quote for filenames, and a trailing "/" for directory names.  As the
> +# trailing "/" is also added in the tab-completion output the
> +# COMPLETION_LIST will include the "/" character, but the trailing quote is
> +# only added when using the 'complete' command.
> +#
> +# Pass the trailing quote will be passed as END_QUOTE_CHAR, this proc will
> +# run the tab completion test, and will then add the trailing quote to those
> +# entries in COMPLETION_LIST that don't have a trailing "/" before running
> +# the 'complete' command test.
> +proc test_gdb_complete_filename_multiple {
> +  cmd_prefix completion_word add_completed_line completion_list
> +  {start_quote_char ""} {end_quote_char ""} {max_completions false}
> +  {testname ""}
> +} {
> +    if { [readline_is_used] } {
> +	test_gdb_complete_tab_multiple "$cmd_prefix$completion_word" \
> +	    $add_completed_line $completion_list $max_completions $testname
> +    }
> +
> +    if { $start_quote_char eq "" && $end_quote_char ne "" } {
> +	set updated_completion_list {}
> +
> +	foreach entry $completion_list {
> +	    if {[string range $entry end end] ne "/"} {
> +		set entry $entry$end_quote_char
> +	    }
> +	    lappend updated_completion_list $entry
> +	}
> +
> +	set completion_list $updated_completion_list
> +	set end_quote_char ""
> +    }
> +
> +    test_gdb_complete_cmd_multiple $cmd_prefix $completion_word \
> +	$completion_list $start_quote_char $end_quote_char $max_completions \
> +	$testname
> +}
> +
>  # Run filename completetion tests.  ROOT is the base directory as
>  # returned from setup_directory_tree, though, if ROOT is a
>  # sub-directory of the user's home directory ROOT might have been
> @@ -63,31 +106,22 @@ proc run_tests { root } {
>  	test_gdb_complete_none "file ${qc}${root}/xx" \
>  	    "expand a non-existent filename"
>  
> -	# The following test is split into separate cmd and tab calls as the
> -	# cmd versions will add a closing quote.  It shouldn't be doing
> -	# this; this will be fixed in a later commit.
> -	test_gdb_complete_cmd_unique "file ${qc}${root}/a" \
> -	    "file ${qc}${root}/aaa/${qc}" \
> +	test_gdb_complete_unique "file ${qc}${root}/a" \
> +	    "file ${qc}${root}/aaa/" "" false \
>  	    "expand a unique directory name"
>  
> -	if { [readline_is_used] } {
> -	    test_gdb_complete_tab_unique "file ${qc}${root}/a" \
> -		"file ${qc}${root}/aaa/" "" \
> -		"expand a unique directory name"
> -	}
> -
>  	test_gdb_complete_unique "file ${qc}${root}/cc2" \
>  	    "file ${qc}${root}/cc2${qc}" " " false \
>  	    "expand a unique filename"
>  
> -	test_gdb_complete_multiple "file ${qc}${root}/" \
> +	test_gdb_complete_filename_multiple "file ${qc}${root}/" \
>  	    "b" "b" {
>  		"bb1/"
>  		"bb2/"
> -	    } "" "${qc}" false \
> +	    } "" "" false \
>  	    "expand multiple directory names"
>  
> -	test_gdb_complete_multiple "file ${qc}${root}/" \
> +	test_gdb_complete_filename_multiple "file ${qc}${root}/" \
>  	    "c" "c" {
>  		"cc1/"
>  		"cc2"
> @@ -99,14 +133,14 @@ proc run_tests { root } {
>  	if { $qc ne "" } {
>  	    set sp " "
>  
> -	    test_gdb_complete_multiple "file ${qc}${root}/aaa/" \
> +	    test_gdb_complete_filename_multiple "file ${qc}${root}/aaa/" \
>  		"a" "a${sp}" {
>  		    "aa bb"
>  		    "aa cc"
>  		} "" "${qc}" false \
>  		"expand filenames containing spaces"
>  
> -	    test_gdb_complete_multiple "file ${qc}${root}/bb1/" \
> +	    test_gdb_complete_filename_multiple "file ${qc}${root}/bb1/" \
>  		"a" "a" {
>  		    "aa\"bb"
>  		    "aa'bb"
> -- 
> 2.25.4
> 

Re: [PATCH 4/6] gdb: add match formatter mechanism for 'complete' command output

[Eli Zaretskii]

> Date: Sat, 30 Mar 2024 23:49:18 +0000
> From: Lancelot SIX <lsix@lancelotsix.com>
> Cc: gdb-patches@sourceware.org
> 
> >   (gdb) complete file '/tmp/xx
> >   file '/tmp/xxx/'
> > 
> > The completion offered here is really only a partial completion, we've
> > completed up to the end of the next directory name, but, until we have
> > a filename then the completion is not finished and the trailing quote
> > should not be added.
> > 
> > This would match the readline behaviour, e.g.:
> > 
> >   (gdb) file '/tmp/xx<TAB>
> >   (gdb) file '/tmp/xxx/
> > 
> > In this case readline completes the directory name, but doesn't add
> > the trailing quote character.

Btw, what readline does is not the only useful behavior.  An
alternative would be this:

  (gdb) file '/tmp/xx<TAB>
   => (gdb) file '/tmp/xx/'
  (gdb) file '/tmp/xx/'a<TAB>
   => (gdb) file '/tmp/xx/abcd'

That is, allow to type after the closing quote, and remove the closing
quote before trying to complete.  This way, the user can use
'/tmp/xx/' if that is what she needs, or continue typing and
completing if not.

Re: [PATCH 4/6] gdb: add match formatter mechanism for 'complete' command output

[Andrew Burgess]

Eli Zaretskii <eliz@gnu.org> writes:

>> Date: Sat, 30 Mar 2024 23:49:18 +0000
>> From: Lancelot SIX <lsix@lancelotsix.com>
>> Cc: gdb-patches@sourceware.org
>> 
>> >   (gdb) complete file '/tmp/xx
>> >   file '/tmp/xxx/'
>> > 
>> > The completion offered here is really only a partial completion, we've
>> > completed up to the end of the next directory name, but, until we have
>> > a filename then the completion is not finished and the trailing quote
>> > should not be added.
>> > 
>> > This would match the readline behaviour, e.g.:
>> > 
>> >   (gdb) file '/tmp/xx<TAB>
>> >   (gdb) file '/tmp/xxx/
>> > 
>> > In this case readline completes the directory name, but doesn't add
>> > the trailing quote character.
>
> Btw, what readline does is not the only useful behavior.  An
> alternative would be this:
>
>   (gdb) file '/tmp/xx<TAB>
>    => (gdb) file '/tmp/xx/'
>   (gdb) file '/tmp/xx/'a<TAB>
>    => (gdb) file '/tmp/xx/abcd'
>
> That is, allow to type after the closing quote, and remove the closing
> quote before trying to complete.  This way, the user can use
> '/tmp/xx/' if that is what she needs, or continue typing and
> completing if not.

That does sound better.  I think there are still things that would need
figuring out.  For example, what if the user didn't do the second tab
completion, but just kept typing.

  (gdb) file '/tmp/xx<TAB>
	=> (gdb) file '/tmp/xx/'
  (gdb) file '/tmp/xx/'abcd<ENTER>
	=> ?? What would this do?

What if there really was a file /tmp/xx/'abcd ? What happens then?  But
I do think this sounds like an interesting idea.  But...

... I don't think I want to start making such sweeping changes to
readline, or how GDB uses readline.  Or maybe that would require moving
off readline completely?

My goal with this work is to support '-filename PATH' command options
using GDB's option framework.  Adding such options really calls for good
filename completion.  [ NOTE: I'm not adding those options in this
series, that would be the next project. ]

Ideally I want to work with GDB as it is right now, i.e. how does GDB
work right now, and how does GDB parse filenames right now, and have
filename completion work with those constraints.

Thanks,
Andrew

Re: [PATCH 4/6] gdb: add match formatter mechanism for 'complete' command output

[Eli Zaretskii]

> From: Andrew Burgess <aburgess@redhat.com>
> Cc: gdb-patches@sourceware.org
> Date: Fri, 12 Apr 2024 18:42:12 +0100
> 
> >   (gdb) file '/tmp/xx<TAB>
> >    => (gdb) file '/tmp/xx/'
> >   (gdb) file '/tmp/xx/'a<TAB>
> >    => (gdb) file '/tmp/xx/abcd'
> >
> > That is, allow to type after the closing quote, and remove the closing
> > quote before trying to complete.  This way, the user can use
> > '/tmp/xx/' if that is what she needs, or continue typing and
> > completing if not.
> 
> That does sound better.  I think there are still things that would need
> figuring out.  For example, what if the user didn't do the second tab
> completion, but just kept typing.
> 
>   (gdb) file '/tmp/xx<TAB>
> 	=> (gdb) file '/tmp/xx/'
>   (gdb) file '/tmp/xx/'abcd<ENTER>
> 	=> ?? What would this do?
> 
> What if there really was a file /tmp/xx/'abcd ? What happens then?

ENTER does the same as TAB, and then submits the result.  So GDB will
see '/tmp/xx/abcd' in that case.

Re: [PATCH 4/6] gdb: add match formatter mechanism for 'complete' command output

[Andrew Burgess]

Eli Zaretskii <eliz@gnu.org> writes:

>> From: Andrew Burgess <aburgess@redhat.com>
>> Cc: gdb-patches@sourceware.org
>> Date: Fri, 12 Apr 2024 18:42:12 +0100
>> 
>> >   (gdb) file '/tmp/xx<TAB>
>> >    => (gdb) file '/tmp/xx/'
>> >   (gdb) file '/tmp/xx/'a<TAB>
>> >    => (gdb) file '/tmp/xx/abcd'
>> >
>> > That is, allow to type after the closing quote, and remove the closing
>> > quote before trying to complete.  This way, the user can use
>> > '/tmp/xx/' if that is what she needs, or continue typing and
>> > completing if not.
>> 
>> That does sound better.  I think there are still things that would need
>> figuring out.  For example, what if the user didn't do the second tab
>> completion, but just kept typing.
>> 
>>   (gdb) file '/tmp/xx<TAB>
>> 	=> (gdb) file '/tmp/xx/'
>>   (gdb) file '/tmp/xx/'abcd<ENTER>
>> 	=> ?? What would this do?
>> 
>> What if there really was a file /tmp/xx/'abcd ? What happens then?
>
> ENTER does the same as TAB, and then submits the result.  So GDB will
> see '/tmp/xx/abcd' in that case.

This all sounds great.  Do you know of any other readline based projects
that do completion like this?

My concern here is that, as I understand readline, this isn't going to
be easy to implement.  And even if we could (somehow) convince readline
to do this, it's going to result in a bunch of extra complexity compared
to just having readline do things the way it wants to.

And no, GDB doesn't have to use readline, but I'd rather not have to
throw out readline just so we can have working filename completion.

I'm not trying to be dismissive here.  This started as "Andrew's come up
with a design, he should have discussed it first."  And I'm trying to
work out if this is a serious, we should do it this other way.  Or if
this is just "See, there are other ways it could be done, you should
have discussed it first"?

To both of these my answer would be, I've not really invented anything
here, I've just connected up GDB with readline, and what we get is the
readline way of doing things.

Maybe then you know that readline can be made to do things differently
(hence me asking for examples).  Or maybe you're suggesting we should
forget readline and do things as you suggest ... I'm just trying to
figure out what you ideal end result is here.

Thanks,
Andrew

Re: [PATCH 4/6] gdb: add match formatter mechanism for 'complete' command output

[Eli Zaretskii]

> From: Andrew Burgess <aburgess@redhat.com>
> Cc: lsix@lancelotsix.com, gdb-patches@sourceware.org
> Date: Fri, 12 Apr 2024 23:29:02 +0100
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> >>   (gdb) file '/tmp/xx<TAB>
> >> 	=> (gdb) file '/tmp/xx/'
> >>   (gdb) file '/tmp/xx/'abcd<ENTER>
> >> 	=> ?? What would this do?
> >> 
> >> What if there really was a file /tmp/xx/'abcd ? What happens then?
> >
> > ENTER does the same as TAB, and then submits the result.  So GDB will
> > see '/tmp/xx/abcd' in that case.
> 
> This all sounds great.  Do you know of any other readline based projects
> that do completion like this?

No, not off the top of my head.

Ironically, the Windows shell cmd.exe does behave like that.

> Maybe then you know that readline can be made to do things differently
> (hence me asking for examples).  Or maybe you're suggesting we should
> forget readline and do things as you suggest ... I'm just trying to
> figure out what you ideal end result is here.

If Readline doesn't want to go this way, I think in the long run GDB
should develop its own completion machinery.  But I'm okay with
leaving that alone for now.

[PATCH 5/6] gdb: apply escaping to filenames in 'complete' results

[Andrew Burgess]

Building on the mechanism added in the previous commit(s), this commit
applies escaping to filenames in the 'complete' command output.
Consider a file: /tmp/xxx/aa"bb -- that is a filename that contains a
double quote, currently the 'complete' command output looks like this:

  (gdb) complete file /tmp/xxx/a
  file /tmp/xxx/aa"bb

Notice that the double quote in the output is not escaped.  If we
passed this same output back to GDB then the double quote will be
treated as the start of a string.

After this commit then the output looks like this:

  (gdb) complete file /tmp/xxx/a
  file /tmp/xxx/aa\"bb

The double quote is now escaped.  If we feed this output back to GDB
then GDB will treat this as a single filename that contains a double
quote, exactly what we want.

To achieve this I've done a little refactoring, splitting out the core
of gdb_completer_file_name_quote, and then added a new call from the
filename_match_formatter function.

There are updates to the tests to cover this new functionality.
---
 gdb/completer.c                               | 35 ++++++--
 .../gdb.base/filename-completion.exp          | 89 +++++++++++--------
 2 files changed, 80 insertions(+), 44 deletions(-)

diff --git a/gdb/completer.c b/gdb/completer.c
index 785fb09b4d7..710c8c206cb 100644
--- a/gdb/completer.c
+++ b/gdb/completer.c
@@ -305,24 +305,24 @@ gdb_completer_file_name_dequote (char *filename, int quote_char)
   return strdup (tmp.c_str ());
 }
 
-/* Apply character escaping to the file name in TEXT.  QUOTE_PTR points to
-   the quote character surrounding TEXT, or points to the null-character if
-   there are no quotes around TEXT.  MATCH_TYPE will be one of the readline
-   constants SINGLE_MATCH or MULTI_MATCH depending on if there is one or
-   many completions.  */
+/* Apply character escaping to the filename in TEXT and return a newly
+   allocated buffer containing the possibly updated filename.
+
+   QUOTE_CHAR is the quote character surrounding TEXT, or the
+   null-character if there are no quotes around TEXT.  */
 
 static char *
-gdb_completer_file_name_quote (char *text, int match_type, char *quote_ptr)
+gdb_completer_file_name_quote_1 (const char *text, char quote_char)
 {
   std::string str;
 
-  if (*quote_ptr == '\'')
+  if (quote_char == '\'')
     {
       /* There is no backslash escaping permitted within a single quoted
 	 string, so in this case we can just return the input sting.  */
       str = text;
     }
-  else if (*quote_ptr == '"')
+  else if (quote_char == '"')
     {
       /* Add escaping for a double quoted filename.  */
       for (const char *input = text;
@@ -351,6 +351,18 @@ gdb_completer_file_name_quote (char *text, int match_type, char *quote_ptr)
   return strdup (str.c_str ());
 }
 
+/* Apply character escaping to the filename in TEXT.  QUOTE_PTR points to
+   the quote character surrounding TEXT, or points to the null-character if
+   there are no quotes around TEXT.  MATCH_TYPE will be one of the readline
+   constants SINGLE_MATCH or MULTI_MATCH depending on if there is one or
+   many completions.  */
+
+static char *
+gdb_completer_file_name_quote (char *text, int match_type, char *quote_ptr)
+{
+  return gdb_completer_file_name_quote_1 (text, *quote_ptr);
+}
+
 /* The function is used to update the completion word MATCH before
    displaying it to the user in the 'complete' command output, this
    particular function is only used when MATCH has been supplied by the
@@ -370,7 +382,12 @@ filename_match_formatter (const char *match, char quote_char)
   struct stat finfo;
   const bool isdir = (stat (expanded.c_str (), &finfo) == 0
 		      && S_ISDIR (finfo.st_mode));
-  std::string result (match);
+
+  gdb::unique_xmalloc_ptr<char> quoted_match
+    (gdb_completer_file_name_quote_1 (match, quote_char));
+
+  std::string result (quoted_match.get ());
+
   if (isdir)
     result += "/";
   else
diff --git a/gdb/testsuite/gdb.base/filename-completion.exp b/gdb/testsuite/gdb.base/filename-completion.exp
index d7c99e1340d..f8a48269528 100644
--- a/gdb/testsuite/gdb.base/filename-completion.exp
+++ b/gdb/testsuite/gdb.base/filename-completion.exp
@@ -70,10 +70,22 @@ proc test_gdb_complete_filename_multiple {
 	    $add_completed_line $completion_list $max_completions $testname
     }
 
-    if { $start_quote_char eq "" && $end_quote_char ne "" } {
+    if { $start_quote_char eq "" } {
 	set updated_completion_list {}
 
 	foreach entry $completion_list {
+	    # If ENTRY is quoted with double quotes, then any double
+	    # quotes within the entry need to be escaped.
+	    if { $end_quote_char eq "\"" } {
+		regsub -all "\"" $entry "\\\"" entry
+	    }
+
+	    if { $end_quote_char eq "" } {
+		regsub -all " " $entry "\\ " entry
+		regsub -all "\"" $entry "\\\"" entry
+		regsub -all "'" $entry "\\'" entry
+	    }
+
 	    if {[string range $entry end end] ne "/"} {
 		set entry $entry$end_quote_char
 	    }
@@ -128,47 +140,54 @@ proc run_tests { root } {
 	    } "" "${qc}" false \
 	    "expand mixed directory and file names"
 
-	# GDB does not currently escape word break characters
-	# (e.g. white space) correctly in unquoted filenames.
 	if { $qc ne "" } {
 	    set sp " "
-
-	    test_gdb_complete_filename_multiple "file ${qc}${root}/aaa/" \
-		"a" "a${sp}" {
-		    "aa bb"
-		    "aa cc"
-		} "" "${qc}" false \
-		"expand filenames containing spaces"
-
-	    test_gdb_complete_filename_multiple "file ${qc}${root}/bb1/" \
-		"a" "a" {
-		    "aa\"bb"
-		    "aa'bb"
-		} "" "${qc}" false \
-		"expand filenames containing quotes"
 	} else {
 	    set sp "\\ "
+	}
 
-	    test_gdb_complete_tab_multiple "file ${qc}${root}/aaa/a" \
-		"a${sp}" {
-		    "aa bb"
-		    "aa cc"
-		} false \
-		"expand filenames containing spaces"
-
-	    test_gdb_complete_tab_multiple "file ${qc}${root}/bb1/a" \
-		"a" {
-		    "aa\"bb"
-		    "aa'bb"
-		} false \
-		"expand filenames containing quotes"
-
-	    test_gdb_complete_tab_unique "file ${qc}${root}/bb1/aa\\\"" \
-		"file ${qc}${root}/bb1/aa\\\\\"bb${qc}" " " \
+	if { $qc eq "'" } {
+	    set dq "\""
+	    set dq_re "\""
+	} else {
+	    set dq "\\\""
+	    set dq_re "\\\\\""
+	}
+
+	test_gdb_complete_filename_multiple "file ${qc}${root}/aaa/" \
+	    "a" "a${sp}" {
+		"aa bb"
+		"aa cc"
+	    } "" "${qc}" false \
+	    "expand filenames containing spaces"
+
+	test_gdb_complete_filename_multiple "file ${qc}${root}/bb1/" \
+	    "a" "a" {
+		"aa\"bb"
+		"aa'bb"
+	    } "" "${qc}" false \
+	    "expand filenames containing quotes"
+
+	test_gdb_complete_tab_unique "file ${qc}${root}/bb1/aa${dq}" \
+	    "file ${qc}${root}/bb1/aa${dq_re}bb${qc}" " " \
 	    "expand unique filename containing double quotes"
 
-	    test_gdb_complete_tab_unique "file ${qc}${root}/bb1/aa\\'" \
-		"file ${qc}${root}/bb1/aa\\\\'bb${qc}" " " \
+	# It is not possible to include a single quote character
+	# within a single quoted string.  However, GDB does not do
+	# anything smart if a user tries to do this.  Avoid testing
+	# this case.  Maybe in the future we'll figure a way to avoid
+	# this situation.
+	if { $qc ne "'" } {
+	    if { $qc eq "" } {
+		set sq "\\'"
+		set sq_re "\\\\'"
+	    } else {
+		set sq "'"
+		set sq_re "'"
+	    }
+
+	    test_gdb_complete_tab_unique "file ${qc}${root}/bb1/aa${sq}" \
+		"file ${qc}${root}/bb1/aa${sq_re}bb${qc}" " " \
 		"expand unique filename containing single quote"
 	}
     }
-- 
2.25.4

[PATCH 6/6] gdb: improve gdb_rl_find_completion_word for quoted words

[Andrew Burgess]

The function gdb_rl_find_completion_word is very similar to the
readline function _rl_find_completion_word, but was either an older
version of that function, or was trimmed when copying to remove code
which was considered unnecessary.

We maintain this copy because the _rl_find_completion_word function is
not part of the public readline API, and we need to replicate the
functionality of that function as part of the 'complete' command.

Within gdb_rl_find_completion_word when looking for the completion
word, if we don't find a unclosed quoted string (which would become
the completion word) then we scan backwards looking for a word break
character.  For example, given:

  (gdb) complete file /tmp/foo

There is no unclosed quoted string so we end up scanning backwards
from the end looking for a word break character.  In this case the
space after 'file' and before '/tmp/foo' is found, so '/tmp/foo'
becomes the completion word.

However, given this:

  (gdb) complete file /tmp/foo\"

There is still no unclosed quoted string, however, when we can
backwards the '"' (double quotes) are treated as a word break
character, and so we end up using the empty string as the completion
word.

The readline function _rl_find_completion_word avoids this mistake by
using the rl_char_is_quoted_p hook.  This function will return true
for the double quote character as it is preceded by a backslash.  An
earlier commit in this series supplied a rl_char_is_quoted_p function
for the filename completion case, however, gdb_rl_find_completion_word
doesn't call rl_char_is_quoted_p so this doesn't help for the
'complete' case.

In this commit I've copied the code to call rl_char_is_quoted_p from
_rl_find_completion_word into gdb_rl_find_completion_word.

This half solves the problem.  In the case:

  (gdb) complete file /tmp/foo\"

We do now try to complete on the string '/tmp/foo\"', however, when we
reach filename_completer we call back into readline to actually
perform filename completion.  However, at this point the WORD variable
points to a string that still contains the backslash.  The backslash
isn't part of the actual filename, that's just an escape character.

Our expectation is that readline will remove the backslash when
looking for matching filenames.  However, readline contains an
optimisation to avoid unnecessary work trying to remove escape
characters.

The readline variable rl_completion_found_quote is set in the readline
function gen_completion_matches before the generation of completion
matches.  This variable is set to true (non-zero) if there is (or
might be) escape characters within the completion word.

The function rl_filename_completion_function, which generates the
filename matches, only removes escape characters when
rl_completion_found_quote is true.  When GDB generates completions
through readline (e.g. tab completion) then rl_completion_found_quote
is set correctly.

But when we use the 'complete' command we don't pass through readline,
and so gen_completion_matches is never called and
rl_completion_found_quote is not set.  In this case when we call
rl_filename_completion_function readline doesn't remove the escapes
from the completion word, and so in our case above, readline looks for
completions of the exact filename '/tmp/foo\"', that is, the filename
including the backslash.

To work around this problem I've added a new flag to our function
gdb_rl_find_completion_word which is set true when we find any quoting
or escaping.  This matches what readline does.

Then in the 'complete' function we can set rl_completion_found_quote
prior to generating completion matches.

With this done the 'complete' command now works correctly when trying
to complete filenames that contain escaped word break characters.  The
tests have been updated accordingly.
---
 gdb/completer.c                               | 60 +++++++++++++++----
 .../gdb.base/filename-completion.exp          | 12 ++--
 2 files changed, 54 insertions(+), 18 deletions(-)

diff --git a/gdb/completer.c b/gdb/completer.c
index 710c8c206cb..924fd08b469 100644
--- a/gdb/completer.c
+++ b/gdb/completer.c
@@ -49,7 +49,8 @@
 /* Forward declarations.  */
 static const char *completion_find_completion_word (completion_tracker &tracker,
 						    const char *text,
-						    int *quote_char);
+						    int *quote_char,
+						    bool *found_any_quoting);
 
 static void set_rl_completer_word_break_characters (const char *break_chars);
 
@@ -458,7 +459,9 @@ filename_completer_handle_brkchars (struct cmd_list_element *ignore,
    boundaries of the current word.  QC, if non-null, is set to the
    opening quote character if we found an unclosed quoted substring,
    '\0' otherwise.  DP, if non-null, is set to the value of the
-   delimiter character that caused a word break.  */
+   delimiter character that caused a word break.  FOUND_ANY_QUOTING, if
+   non-null, is set to true if we found any quote characters (single or
+   double quotes, or a backslash) while finding the completion word.  */
 
 struct gdb_rl_completion_word_info
 {
@@ -469,7 +472,7 @@ struct gdb_rl_completion_word_info
 
 static const char *
 gdb_rl_find_completion_word (struct gdb_rl_completion_word_info *info,
-			     int *qc, int *dp,
+			     int *qc, int *dp, bool *found_any_quoting,
 			     const char *line_buffer)
 {
   int scan, end, delimiter, pass_next, isbrk;
@@ -491,6 +494,7 @@ gdb_rl_find_completion_word (struct gdb_rl_completion_word_info *info,
   end = point;
   delimiter = 0;
   quote_char = '\0';
+  bool found_quote = false;
 
   brkchars = info->word_break_characters;
 
@@ -516,6 +520,7 @@ gdb_rl_find_completion_word (struct gdb_rl_completion_word_info *info,
 	  if (quote_char != '\'' && line_buffer[scan] == '\\')
 	    {
 	      pass_next = 1;
+	      found_quote = true;
 	      continue;
 	    }
 
@@ -536,6 +541,7 @@ gdb_rl_find_completion_word (struct gdb_rl_completion_word_info *info,
 	      /* Found start of a quoted substring.  */
 	      quote_char = line_buffer[scan];
 	      point = scan + 1;
+	      found_quote = true;
 	    }
 	}
     }
@@ -549,8 +555,22 @@ gdb_rl_find_completion_word (struct gdb_rl_completion_word_info *info,
 	{
 	  scan = line_buffer[point];
 
-	  if (strchr (brkchars, scan) != 0)
-	    break;
+	  if (strchr (brkchars, scan) == 0)
+	    continue;
+
+	  /* Call the application-specific function to tell us whether
+	     this word break character is quoted and should be skipped.
+	     The const_cast is needed here to comply with the readline
+	     API.  The only function we register for rl_char_is_quoted_p
+	     treats the input buffer as 'const', so we're OK.  */
+	  if (rl_char_is_quoted_p != nullptr && found_quote
+	      && (*rl_char_is_quoted_p) (const_cast<char *> (line_buffer),
+					 point))
+	    continue;
+
+	  /* Convoluted code, but it avoids an n^2 algorithm with calls
+	     to char_is_quoted.  */
+	  break;
 	}
     }
 
@@ -574,6 +594,8 @@ gdb_rl_find_completion_word (struct gdb_rl_completion_word_info *info,
 	}
     }
 
+  if (found_any_quoting != nullptr)
+    *found_any_quoting = found_quote;
   if (qc != NULL)
     *qc = quote_char;
   if (dp != NULL)
@@ -600,7 +622,7 @@ advance_to_completion_word (completion_tracker &tracker,
 
   int delimiter;
   const char *start
-    = gdb_rl_find_completion_word (&info, NULL, &delimiter, text);
+    = gdb_rl_find_completion_word (&info, nullptr, &delimiter, nullptr, text);
 
   tracker.advance_custom_word_point_by (start - text);
 
@@ -673,7 +695,8 @@ complete_nested_command_line (completion_tracker &tracker, const char *text)
 
   int quote_char = '\0';
   const char *word = completion_find_completion_word (tracker, text,
-						      &quote_char);
+						      &quote_char,
+						      nullptr);
 
   if (tracker.use_custom_word_point ())
     {
@@ -1891,8 +1914,11 @@ complete (const char *line, char const **word, int *quote_char)
 
   try
     {
+      bool found_any_quoting = false;
+
       *word = completion_find_completion_word (tracker_handle_brkchars,
-					      line, quote_char);
+					       line, quote_char,
+					       &found_any_quoting);
 
       /* Completers that provide a custom word point in the
 	 handle_brkchars phase also compute their completions then.
@@ -1902,6 +1928,12 @@ complete (const char *line, char const **word, int *quote_char)
 	tracker = &tracker_handle_brkchars;
       else
 	{
+	  /* Setting this global matches what readline does within
+	     gen_completion_matches.  We need this set correctly in case
+	     our completion function calls back into readline to perform
+	     completion (e.g. filename_completer does this).  */
+	  rl_completion_found_quote = found_any_quoting;
+
 	  complete_line (tracker_handle_completions, *word, line, strlen (line));
 	  tracker = &tracker_handle_completions;
 	}
@@ -2176,7 +2208,7 @@ gdb_completion_word_break_characters () noexcept
 
 static const char *
 completion_find_completion_word (completion_tracker &tracker, const char *text,
-				 int *quote_char)
+				 int *quote_char, bool *found_any_quoting)
 {
   size_t point = strlen (text);
 
@@ -2186,6 +2218,13 @@ completion_find_completion_word (completion_tracker &tracker, const char *text,
     {
       gdb_assert (tracker.custom_word_point () > 0);
       *quote_char = tracker.quote_char ();
+      /* This isn't really correct, we're ignoring the case where we found
+	 a backslash escaping a character.  However, this isn't an issue
+	 right now as we only rely on *FOUND_ANY_QUOTING being set when
+	 performing filename completion, which doesn't go through this
+	 path.  */
+      if (found_any_quoting != nullptr)
+	*found_any_quoting = *quote_char != '\0';
       return text + tracker.custom_word_point ();
     }
 
@@ -2195,7 +2234,8 @@ completion_find_completion_word (completion_tracker &tracker, const char *text,
   info.quote_characters = rl_completer_quote_characters;
   info.basic_quote_characters = rl_basic_quote_characters;
 
-  return gdb_rl_find_completion_word (&info, quote_char, NULL, text);
+  return gdb_rl_find_completion_word (&info, quote_char, nullptr,
+				      found_any_quoting, text);
 }
 
 /* See completer.h.  */
diff --git a/gdb/testsuite/gdb.base/filename-completion.exp b/gdb/testsuite/gdb.base/filename-completion.exp
index f8a48269528..fd4c407ac23 100644
--- a/gdb/testsuite/gdb.base/filename-completion.exp
+++ b/gdb/testsuite/gdb.base/filename-completion.exp
@@ -148,10 +148,8 @@ proc run_tests { root } {
 
 	if { $qc eq "'" } {
 	    set dq "\""
-	    set dq_re "\""
 	} else {
 	    set dq "\\\""
-	    set dq_re "\\\\\""
 	}
 
 	test_gdb_complete_filename_multiple "file ${qc}${root}/aaa/" \
@@ -168,8 +166,8 @@ proc run_tests { root } {
 	    } "" "${qc}" false \
 	    "expand filenames containing quotes"
 
-	test_gdb_complete_tab_unique "file ${qc}${root}/bb1/aa${dq}" \
-	    "file ${qc}${root}/bb1/aa${dq_re}bb${qc}" " " \
+	test_gdb_complete_unique "file ${qc}${root}/bb1/aa${dq}" \
+	    "file ${qc}${root}/bb1/aa${dq}bb${qc}" " " false \
 	    "expand unique filename containing double quotes"
 
 	# It is not possible to include a single quote character
@@ -180,14 +178,12 @@ proc run_tests { root } {
 	if { $qc ne "'" } {
 	    if { $qc eq "" } {
 		set sq "\\'"
-		set sq_re "\\\\'"
 	    } else {
 		set sq "'"
-		set sq_re "'"
 	    }
 
-	    test_gdb_complete_tab_unique "file ${qc}${root}/bb1/aa${sq}" \
-		"file ${qc}${root}/bb1/aa${sq_re}bb${qc}" " " \
+	    test_gdb_complete_unique "file ${qc}${root}/bb1/aa${sq}" \
+		"file ${qc}${root}/bb1/aa${sq}bb${qc}" " " false \
 		"expand unique filename containing single quote"
 	}
     }
-- 
2.25.4

[PATCHv2 0/8] Further filename completion improvements

[Andrew Burgess]

In V2:

  - Patches #1 and #2 are new in this iteration.  Patches #3 to #8 are
    the patches from V1 rebased onto these two new patches,

  - Patch #1 adds documentation for the formatting of filename
    arguments, this tries to explain the two diffent ways that GDB
    expects filename arguments to be formatted.

  - Patch #2 addresses the problem that Lancelot pointed out: some
    commands don't expect filename arguments to be quoted, or to
    contain escapes.  In this patch I split the filename completion in
    two so the two different filename argument formats are handled
    separately...

  - This clears the way for the rest of the series, which updates how
    completion works for those filename arguments that do accept
    quoting and escaping,

  - Patches #3 to #8 are in principle the same as in V1, but there
    were some changes after rebasing onto the new patch #2.

---

Andrew Burgess (8):
  gdb/doc: document how filename arguments are formatted
  gdb: split apart two different types of filename completion
  gdb: improve escaping when completing filenames
  gdb: move display of completion results into completion_result class
  gdb: simplify completion_result::print_matches
  gdb: add match formatter mechanism for 'complete' command output
  gdb: apply escaping to filenames in 'complete' results
  gdb: improve gdb_rl_find_completion_word for quoted words

 gdb/auto-load.c                               |   4 +-
 gdb/breakpoint.c                              |   5 +-
 gdb/cli/cli-cmds.c                            |  34 +-
 gdb/cli/cli-decode.c                          |  12 +-
 gdb/cli/cli-dump.c                            |   6 +-
 gdb/compile/compile.c                         |   3 +-
 gdb/completer.c                               | 501 +++++++++++++++---
 gdb/completer.h                               |  79 ++-
 gdb/corefile.c                                |   4 +-
 gdb/corelow.c                                 |   4 +-
 gdb/doc/gdb.texinfo                           |  66 +++
 gdb/dwarf2/index-write.c                      |   3 +-
 gdb/exec.c                                    |   7 +-
 gdb/guile/scm-cmd.c                           |   2 +-
 gdb/infcmd.c                                  |  15 +-
 gdb/inferior.c                                |   2 +-
 gdb/jit.c                                     |   2 +-
 gdb/python/py-cmd.c                           |   2 +-
 gdb/record-full.c                             |   5 +-
 gdb/record.c                                  |   2 +-
 gdb/skip.c                                    |   2 +-
 gdb/source.c                                  |   3 +-
 gdb/symfile.c                                 |   7 +-
 gdb/target-descriptions.c                     |   7 +-
 gdb/target.c                                  |   4 +-
 gdb/target.h                                  |   9 +-
 .../gdb.base/filename-completion.exp          | 257 +++++++--
 gdb/tracectf.c                                |   3 +-
 gdb/tracefile-tfile.c                         |   4 +-
 29 files changed, 857 insertions(+), 197 deletions(-)


base-commit: 7a59cf956369336eb9346196a85976e4042019f5
-- 
2.25.4

[PATCHv2 1/8] gdb/doc: document how filename arguments are formatted

[Andrew Burgess]

In the following commits I intend to improve GDB's filename
completion.  However, how filenames should be completed is a little
complex because GDB is not consistent with how it expects filename
arguments to be formatted.

This commit documents the current state of GDB when it comes to
formatting filename arguments.

Currently GDB will not correctly complete filenames inline with this
documentation; GDB will either fail to complete, or complete
incorrectly (i.e. the result of completion will not then be accepted
by GDB).  However, later commits in this series will fix completion.
---
 gdb/doc/gdb.texinfo | 66 +++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 66 insertions(+)

diff --git a/gdb/doc/gdb.texinfo b/gdb/doc/gdb.texinfo
index 82a617e9ad3..abceb9b111b 100644
--- a/gdb/doc/gdb.texinfo
+++ b/gdb/doc/gdb.texinfo
@@ -1752,6 +1752,7 @@
 * Command Syntax::              How to give commands to @value{GDBN}
 * Command Settings::            How to change default behavior of commands
 * Completion::                  Command completion
+* Filename Arguments::		Filenames As Command Arguments
 * Command Options::             Command options
 * Help::                        How to ask @value{GDBN} for help
 @end menu
@@ -2123,6 +2124,56 @@
 @}
 @end smallexample
 
+@node Filename Arguments
+@section Filenames As Command Arguments
+
+When passing filenames (or directory names) as arguments to a command,
+if the filename argument does not include any whitespace, double
+quotes, or single quotes, then for all commands the filename can be
+written as a simple string, for example:
+
+@smallexample
+(@value{GDBP}) file /path/to/some/file
+@end smallexample
+
+If the filename does include whitespace, double quotes, or single
+quotes then @value{GDBN} has two approaches for how these filenames
+should be formatted, which format to use depends on which command is
+being used.
+
+Most @value{GDBN} commands don't require, or support, quoting and
+escaping.  These commands treat any text after the command name, that
+is not a command option (@pxref{Command Options}), as the filename,
+even if the filename contains whitespace or quote characters.  In the
+following example the user is adding @w{@file{/path/that contains/two
+spaces/}} to the auto-load safe-path (@pxref{add-auto-load-safe-path}):
+
+@smallexample
+(@value{GDBP}) add-auto-load-safe-path /path/that contains/two spaces/
+@end smallexample
+
+A small number of commands require that filenames containing
+whitespace or quote characters are either quoted, or have the special
+characters escaped with a backslash.  Commands that support this style
+are marked as such in the manual, any command not marked as accepting
+quoting and escaping of its filename argument, does not accept this
+filename argument style.
+
+For example, to load the file @file{/path/with spaces/to/a file} with
+the @code{file} command (@pxref{Files, ,Commands to Specify Files}),
+you can escape the whitespace characters with a backslash:
+
+@smallexample
+(@value{GDBP}) file /path/with\ spaces/to/a\ file
+@end smallexample
+
+Alternatively the entire filename can be wrapped in quotes, in which
+case no backlsashes are needed, for example:
+
+@smallexample
+(@value{GDBP}) file "/path/with spaces/to/a file"
+@end smallexample
+
 @node Command Options
 @section Command options
 
@@ -21615,6 +21666,9 @@
 to run.  You can change the value of this variable, for both @value{GDBN}
 and your program, using the @code{path} command.
 
+The @var{filename} argument supports escaping and quoting, see
+@ref{Filename Arguments,,Filenames As Command Arguments}.
+
 @cindex unlinked object files
 @cindex patching object files
 You can load unlinked object @file{.o} files into @value{GDBN} using
@@ -21637,6 +21691,9 @@
 if necessary to locate your program.  Omitting @var{filename} means to
 discard information on the executable file.
 
+The @var{filename} argument supports escaping and quoting, see
+@ref{Filename Arguments,,Filenames As Command Arguments}.
+
 @kindex symbol-file
 @item symbol-file @r{[} @var{filename} @r{[} -o @var{offset} @r{]]}
 Read symbol table information from file @var{filename}.  @env{PATH} is
@@ -21660,6 +21717,9 @@
 @code{symbol-file} does not repeat if you press @key{RET} again after
 executing it once.
 
+The @var{filename} argument supports escaping and quoting, see
+@ref{Filename Arguments,,Filenames As Command Arguments}.
+
 When @value{GDBN} is configured for a particular environment, it
 understands debugging information in whatever format is the standard
 generated for that environment; you may use either a @sc{gnu} compiler, or
@@ -21754,6 +21814,9 @@
 @code{add-symbol-file} command any number of times; the new symbol data
 thus read is kept in addition to the old.
 
+The @var{filename} argument supports escaping and quoting, see
+@ref{Filename Arguments,,Filenames As Command Arguments}.
+
 Changes can be reverted using the command @code{remove-symbol-file}.
 
 @cindex relocatable object files, reading symbols from
@@ -21813,6 +21876,9 @@
 
 @code{remove-symbol-file} does not repeat if you press @key{RET} after using it.
 
+The @var{filename} argument supports escaping and quoting, see
+@ref{Filename Arguments,,Filenames As Command Arguments}.
+
 @kindex add-symbol-file-from-memory
 @cindex @code{syscall DSO}
 @cindex load symbols from memory
-- 
2.25.4

Re: [PATCHv2 1/8] gdb/doc: document how filename arguments are formatted

[Eli Zaretskii]

> From: Andrew Burgess <aburgess@redhat.com>
> Cc: Andrew Burgess <aburgess@redhat.com>,
> 	Lancelot SIX <lsix@lancelotsix.com>,
> 	Eli Zaretskii <eliz@gnu.org>
> Date: Sat, 20 Apr 2024 10:10:01 +0100
> 
> In the following commits I intend to improve GDB's filename
> completion.  However, how filenames should be completed is a little
> complex because GDB is not consistent with how it expects filename
> arguments to be formatted.
> 
> This commit documents the current state of GDB when it comes to
> formatting filename arguments.
> 
> Currently GDB will not correctly complete filenames inline with this
> documentation; GDB will either fail to complete, or complete
> incorrectly (i.e. the result of completion will not then be accepted
> by GDB).  However, later commits in this series will fix completion.
> ---
>  gdb/doc/gdb.texinfo | 66 +++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 66 insertions(+)

Thanks, a few comments below.

> +@node Filename Arguments
> +@section Filenames As Command Arguments

Perhaps add some index entry here?  Like this, for example:

  @cindex file names, quoting and escaping
  
> +If the filename does include whitespace, double quotes, or single
> +quotes then @value{GDBN} has two approaches for how these filenames
         ^
A comma missing there.

> +should be formatted, which format to use depends on which command is
> +being used.        ^

I'd replace that comma with a semi-colon.

> +Alternatively the entire filename can be wrapped in quotes, in which
> +case no backlsashes are needed, for example:
> +
> +@smallexample
> +(@value{GDBP}) file "/path/with spaces/to/a file"
> +@end smallexample

is quoting "like this" the only supported style of quoting, or do we
also support quoting 'like this'?  If the latter, we should say so in
the manual.  I would also suggest to describe how to quote file names
with embedded quote characters, instead of leaving this to the
reader's imagination and creativity.

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

Re: [PATCHv2 1/8] gdb/doc: document how filename arguments are formatted

[Andrew Burgess]

Eli,

Thanks for your feedback.

I've addressed the issues you pointed out, and have added some
additional text at the end to better describe the escaping of quote
characters and of backslash itself.

Let me know what you think.

Thanks,
Andrew

---

commit 4d1a140481d8e7262d631cf10756a0d698f75477
Author: Andrew Burgess <aburgess@redhat.com>
Date:   Wed Apr 17 14:47:49 2024 +0100

    gdb/doc: document how filename arguments are formatted
    
    In the following commits I intend to improve GDB's filename
    completion.  However, how filenames should be completed is a little
    complex because GDB is not consistent with how it expects filename
    arguments to be formatted.
    
    This commit documents the current state of GDB when it comes to
    formatting filename arguments.
    
    Currently GDB will not correctly complete filenames inline with this
    documentation; GDB will either fail to complete, or complete
    incorrectly (i.e. the result of completion will not then be accepted
    by GDB).  However, later commits in this series will fix completion.
    
    Reviewed-By: Eli Zaretskii <eliz@gnu.org>

diff --git a/gdb/doc/gdb.texinfo b/gdb/doc/gdb.texinfo
index b2e9faac82d..4ba77b56b8f 100644
--- a/gdb/doc/gdb.texinfo
+++ b/gdb/doc/gdb.texinfo
@@ -1752,6 +1752,7 @@
 * Command Syntax::              How to give commands to @value{GDBN}
 * Command Settings::            How to change default behavior of commands
 * Completion::                  Command completion
+* Filename Arguments::		Filenames As Command Arguments
 * Command Options::             Command options
 * Help::                        How to ask @value{GDBN} for help
 @end menu
@@ -2123,6 +2124,68 @@
 @}
 @end smallexample
 
+@node Filename Arguments
+@section Filenames As Command Arguments
+@cindex file names, quoting and escaping
+
+When passing filenames (or directory names) as arguments to a command,
+if the filename argument does not include any whitespace, double
+quotes, or single quotes, then for all commands the filename can be
+written as a simple string, for example:
+
+@smallexample
+(@value{GDBP}) file /path/to/some/file
+@end smallexample
+
+If the filename does include whitespace, double quotes, or single
+quotes, then @value{GDBN} has two approaches for how these filenames
+should be formatted; which format to use depends on which command is
+being used.
+
+Most @value{GDBN} commands don't require, or support, quoting and
+escaping.  These commands treat any text after the command name, that
+is not a command option (@pxref{Command Options}), as the filename,
+even if the filename contains whitespace or quote characters.  In the
+following example the user is adding @w{@file{/path/that contains/two
+spaces/}} to the auto-load safe-path (@pxref{add-auto-load-safe-path}):
+
+@smallexample
+(@value{GDBP}) add-auto-load-safe-path /path/that contains/two spaces/
+@end smallexample
+
+A small number of commands require that filenames containing
+whitespace or quote characters are either quoted, or have the special
+characters escaped with a backslash.  Commands that support this style
+are marked as such in the manual, any command not marked as accepting
+quoting and escaping of its filename argument, does not accept this
+filename argument style.
+
+For example, to load the file @file{/path/with spaces/to/a file} with
+the @code{file} command (@pxref{Files, ,Commands to Specify Files}),
+you can escape the whitespace characters with a backslash:
+
+@smallexample
+(@value{GDBP}) file /path/with\ spaces/to/a\ file
+@end smallexample
+
+Alternatively the entire filename can be wrapped in either single or
+double quotes, in which case no backlsashes are needed, for example:
+
+@smallexample
+(@value{GDBP}) symbol-file "/path/with spaces/to/a file"
+(@value{GDBP}) exec-file '/path/with spaces/to/a file'
+@end smallexample
+
+It is possible to include a quote character within a quoted filename
+by escaping it with a backslash, for example, within a filename
+surrounded by double quotes, a double quote character should be
+escaped with a backslash, but a single quote character should not be
+escaped.  Within a single quoted string a single quote character needs
+to be escaped, but a double quote character does not.
+
+A literal backslash character can also be included by escaping it with
+a backslash.
+
 @node Command Options
 @section Command options
 
@@ -21615,6 +21678,9 @@
 to run.  You can change the value of this variable, for both @value{GDBN}
 and your program, using the @code{path} command.
 
+The @var{filename} argument supports escaping and quoting, see
+@ref{Filename Arguments,,Filenames As Command Arguments}.
+
 @cindex unlinked object files
 @cindex patching object files
 You can load unlinked object @file{.o} files into @value{GDBN} using
@@ -21637,6 +21703,9 @@
 if necessary to locate your program.  Omitting @var{filename} means to
 discard information on the executable file.
 
+The @var{filename} argument supports escaping and quoting, see
+@ref{Filename Arguments,,Filenames As Command Arguments}.
+
 @kindex symbol-file
 @item symbol-file @r{[} @var{filename} @r{[} -o @var{offset} @r{]]}
 Read symbol table information from file @var{filename}.  @env{PATH} is
@@ -21660,6 +21729,9 @@
 @code{symbol-file} does not repeat if you press @key{RET} again after
 executing it once.
 
+The @var{filename} argument supports escaping and quoting, see
+@ref{Filename Arguments,,Filenames As Command Arguments}.
+
 When @value{GDBN} is configured for a particular environment, it
 understands debugging information in whatever format is the standard
 generated for that environment; you may use either a @sc{gnu} compiler, or
@@ -21754,6 +21826,9 @@
 @code{add-symbol-file} command any number of times; the new symbol data
 thus read is kept in addition to the old.
 
+The @var{filename} argument supports escaping and quoting, see
+@ref{Filename Arguments,,Filenames As Command Arguments}.
+
 Changes can be reverted using the command @code{remove-symbol-file}.
 
 @cindex relocatable object files, reading symbols from
@@ -21813,6 +21888,9 @@
 
 @code{remove-symbol-file} does not repeat if you press @key{RET} after using it.
 
+The @var{filename} argument supports escaping and quoting, see
+@ref{Filename Arguments,,Filenames As Command Arguments}.
+
 @kindex add-symbol-file-from-memory
 @cindex @code{syscall DSO}
 @cindex load symbols from memory

Re: [PATCHv2 1/8] gdb/doc: document how filename arguments are formatted

[Eli Zaretskii]

> From: Andrew Burgess <aburgess@redhat.com>
> Cc: gdb-patches@sourceware.org, lsix@lancelotsix.com
> Date: Sat, 27 Apr 2024 11:01:23 +0100
> 
> 
> Eli,
> 
> Thanks for your feedback.
> 
> I've addressed the issues you pointed out, and have added some
> additional text at the end to better describe the escaping of quote
> characters and of backslash itself.

Thanks.

> +For example, to load the file @file{/path/with spaces/to/a file} with
                                 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
This should be in @w, because white space is significant.

OK with that nit fixed.

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

Re: [PATCHv2 1/8] gdb/doc: document how filename arguments are formatted

[Andrew Burgess]

Eli Zaretskii <eliz@gnu.org> writes:

>> From: Andrew Burgess <aburgess@redhat.com>
>> Cc: gdb-patches@sourceware.org, lsix@lancelotsix.com
>> Date: Sat, 27 Apr 2024 11:01:23 +0100
>> 
>> 
>> Eli,
>> 
>> Thanks for your feedback.
>> 
>> I've addressed the issues you pointed out, and have added some
>> additional text at the end to better describe the escaping of quote
>> characters and of backslash itself.
>
> Thanks.
>
>> +For example, to load the file @file{/path/with spaces/to/a file} with
>                                  ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> This should be in @w, because white space is significant.
>
> OK with that nit fixed.

I added the missing @w and pushed just this patch for now.

Thanks,
Andrew

[PATCHv2 2/8] gdb: split apart two different types of filename completion

[Andrew Burgess]

Unfortunately we have two different types of filename completion in
GDB.

The majority of commands have what I call unquoted filename
completion, this is for commands like 'set logging file ...', 'target
core ...', and 'add-auto-load-safe-path ...'.  For these commands
everything after the command name (that is not a command option) is
treated as a single filename.  If the filename contains white space
then this does not need to be escaped, nor does the filename need to
be quoted.  In fact, the filename argument is not de-quoted, and does
not have any escaping removed, so if a user does try to add such
things, they will be treated as part of the filename.  As an example:

  (gdb) target core "/path/that contains/some white space"

Will look for a directory calls '"' (double quotes) in the local
directory.

A small number of commands do de-quote and remove escapes from
filename arguments.  These command accept what I call quoted and
escaped filenames.  Right now these are the commands that specify the
file for GDB to debug, so:

  file
  exec-file
  symbol-file
  add-symbol-file
  remove-symbol-file

As an example of this in action:

  (gdb) file "/path/that contains/some white space"

In this case GDB would load the file:

  /path/that contains/some white space

Current filename completion always assumes that filenames can be
quoted, though escaping doesn't work in completion right now.  But the
assumption that quoting is allowed is clearly wrong.

This commit splits filename completion into two.  There is now a
filename completer for unquoted filenames, and a second completer to
handle quoted and escaped filenames.

To handle completion of unquoted filenames the completion needs to be
performed during the brkchars phase of completion, which is why almost
every place we use filename_completer has to change in this commit.

I've also renamed the 'filename_completer' function to reflect that it
now handles filenames that can be quoted.

The filename completion test has been extended to cover more cases.

The 'advance_to_filename_complete_word_point' function is no longer
used after this commit and so I've deleted it.
---
 gdb/auto-load.c                               |   4 +-
 gdb/breakpoint.c                              |   5 +-
 gdb/cli/cli-cmds.c                            |   8 +-
 gdb/cli/cli-decode.c                          |  12 +-
 gdb/cli/cli-dump.c                            |   6 +-
 gdb/compile/compile.c                         |   3 +-
 gdb/completer.c                               |  61 ++++---
 gdb/completer.h                               |  29 +++-
 gdb/corefile.c                                |   4 +-
 gdb/corelow.c                                 |   4 +-
 gdb/dwarf2/index-write.c                      |   3 +-
 gdb/exec.c                                    |   7 +-
 gdb/guile/scm-cmd.c                           |   2 +-
 gdb/infcmd.c                                  |  15 +-
 gdb/inferior.c                                |   2 +-
 gdb/jit.c                                     |   2 +-
 gdb/python/py-cmd.c                           |   2 +-
 gdb/record-full.c                             |   5 +-
 gdb/record.c                                  |   2 +-
 gdb/skip.c                                    |   2 +-
 gdb/source.c                                  |   3 +-
 gdb/symfile.c                                 |   7 +-
 gdb/target-descriptions.c                     |   7 +-
 gdb/target.c                                  |   4 +-
 gdb/target.h                                  |   9 +-
 .../gdb.base/filename-completion.exp          | 156 +++++++++++++-----
 gdb/tracectf.c                                |   3 +-
 gdb/tracefile-tfile.c                         |   4 +-
 28 files changed, 248 insertions(+), 123 deletions(-)

diff --git a/gdb/auto-load.c b/gdb/auto-load.c
index db6d6ae0f73..0d57bf92fb7 100644
--- a/gdb/auto-load.c
+++ b/gdb/auto-load.c
@@ -1630,7 +1630,7 @@ This option has security implications for untrusted inferiors."),
 See the commands 'set auto-load safe-path' and 'show auto-load safe-path' to\n\
 access the current full list setting."),
 		 &cmdlist);
-  set_cmd_completer (cmd, filename_completer);
+  set_cmd_completer_handle_brkchars (cmd, filename_completer_handle_brkchars);
 
   cmd = add_cmd ("add-auto-load-scripts-directory", class_support,
 		 add_auto_load_dir,
@@ -1639,7 +1639,7 @@ access the current full list setting."),
 See the commands 'set auto-load scripts-directory' and\n\
 'show auto-load scripts-directory' to access the current full list setting."),
 		 &cmdlist);
-  set_cmd_completer (cmd, filename_completer);
+  set_cmd_completer_handle_brkchars (cmd, filename_completer_handle_brkchars);
 
   add_setshow_boolean_cmd ("auto-load", class_maintenance,
 			   &debug_auto_load, _("\
diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
index 6d8adc62664..946b44df11c 100644
--- a/gdb/breakpoint.c
+++ b/gdb/breakpoint.c
@@ -15093,14 +15093,15 @@ This includes all types of breakpoints (breakpoints, watchpoints,\n\
 catchpoints, tracepoints).  Use the 'source' command in another debug\n\
 session to restore them."),
 	       &save_cmdlist);
-  set_cmd_completer (c, filename_completer);
+  set_cmd_completer_handle_brkchars (c, filename_completer_handle_brkchars);
 
   cmd_list_element *save_tracepoints_cmd
     = add_cmd ("tracepoints", class_trace, save_tracepoints_command, _("\
 Save current tracepoint definitions as a script.\n\
 Use the 'source' command in another debug session to restore them."),
 	       &save_cmdlist);
-  set_cmd_completer (save_tracepoints_cmd, filename_completer);
+  set_cmd_completer_handle_brkchars (save_tracepoints_cmd,
+				     filename_completer_handle_brkchars);
 
   c = add_com_alias ("save-tracepoints", save_tracepoints_cmd, class_trace, 0);
   deprecate_cmd (c, "save tracepoints");
diff --git a/gdb/cli/cli-cmds.c b/gdb/cli/cli-cmds.c
index 3afe2178199..0a537ca9661 100644
--- a/gdb/cli/cli-cmds.c
+++ b/gdb/cli/cli-cmds.c
@@ -2619,7 +2619,7 @@ The debugger's current working directory specifies where scripts and other\n\
 files that can be loaded by GDB are located.\n\
 In order to change the inferior's current working directory, the recommended\n\
 way is to use the \"set cwd\" command."), &cmdlist);
-  set_cmd_completer (c, filename_completer);
+  set_cmd_completer_handle_brkchars (c, filename_completer_handle_brkchars);
 
   add_com ("echo", class_support, echo_command, _("\
 Print a constant string.  Give string as argument.\n\
@@ -2785,7 +2785,7 @@ the previous command number shown."),
     = add_com ("shell", class_support, shell_command, _("\
 Execute the rest of the line as a shell command.\n\
 With no arguments, run an inferior shell."));
-  set_cmd_completer (shell_cmd, filename_completer);
+  set_cmd_completer_handle_brkchars (shell_cmd, filename_completer_handle_brkchars);
 
   add_com_alias ("!", shell_cmd, class_support, 0);
 
@@ -2874,7 +2874,7 @@ you must type \"disassemble 'foo.c'::bar\" and not \"disassemble foo.c:bar\"."))
 
   c = add_com ("make", class_support, make_command, _("\
 Run the ``make'' program using the rest of the line as arguments."));
-  set_cmd_completer (c, filename_completer);
+  set_cmd_completer_handle_brkchars (c, filename_completer_handle_brkchars);
   c = add_cmd ("user", no_class, show_user, _("\
 Show definitions of non-python/scheme user defined commands.\n\
 Argument is the name of the user defined command.\n\
@@ -2958,5 +2958,5 @@ Note that the file \"%s\" is read automatically in this way\n\
 when GDB is started."), GDBINIT).release ();
   c = add_cmd ("source", class_support, source_command,
 	       source_help_text, &cmdlist);
-  set_cmd_completer (c, filename_completer);
+  set_cmd_completer_handle_brkchars (c, filename_completer_handle_brkchars);
 }
diff --git a/gdb/cli/cli-decode.c b/gdb/cli/cli-decode.c
index d9a2ab40575..a48b18bbd87 100644
--- a/gdb/cli/cli-decode.c
+++ b/gdb/cli/cli-decode.c
@@ -866,7 +866,8 @@ add_setshow_filename_cmd (const char *name, enum command_class theclass,
 					 nullptr, nullptr, set_func,
 					 show_func, set_list, show_list);
 
-  set_cmd_completer (commands.set, filename_completer);
+  set_cmd_completer_handle_brkchars (commands.set,
+				     filename_completer_handle_brkchars);
 
   return commands;
 }
@@ -890,7 +891,8 @@ add_setshow_filename_cmd (const char *name, command_class theclass,
 						 nullptr, show_func, set_list,
 						 show_list);
 
-  set_cmd_completer (cmds.set, filename_completer);
+  set_cmd_completer_handle_brkchars (cmds.set,
+				     filename_completer_handle_brkchars);
 
   return cmds;
 }
@@ -1015,7 +1017,8 @@ add_setshow_optional_filename_cmd (const char *name, enum command_class theclass
 					 nullptr, nullptr, set_func, show_func,
 					 set_list, show_list);
 
-  set_cmd_completer (commands.set, filename_completer);
+  set_cmd_completer_handle_brkchars (commands.set,
+				     filename_completer_handle_brkchars);
 
   return commands;
 }
@@ -1039,7 +1042,8 @@ add_setshow_optional_filename_cmd (const char *name, command_class theclass,
 				       set_func, get_func, nullptr, show_func,
 				       set_list,show_list);
 
-  set_cmd_completer (cmds.set, filename_completer);
+  set_cmd_completer_handle_brkchars (cmds.set,
+				     filename_completer_handle_brkchars);
 
   return cmds;
 }
diff --git a/gdb/cli/cli-dump.c b/gdb/cli/cli-dump.c
index 9b44c6edcf2..aaacdd6ede1 100644
--- a/gdb/cli/cli-dump.c
+++ b/gdb/cli/cli-dump.c
@@ -348,7 +348,7 @@ add_dump_command (const char *name,
   struct dump_context *d;
 
   c = add_cmd (name, all_commands, descr, &dump_cmdlist);
-  c->completer =  filename_completer;
+  c->completer_handle_brkchars = filename_completer_handle_brkchars;
   d = XNEW (struct dump_context);
   d->func = func;
   d->mode = FOPEN_WB;
@@ -356,7 +356,7 @@ add_dump_command (const char *name,
   c->func = call_dump_func;
 
   c = add_cmd (name, all_commands, descr, &append_cmdlist);
-  c->completer =  filename_completer;
+  c->completer_handle_brkchars = filename_completer_handle_brkchars;
   d = XNEW (struct dump_context);
   d->func = func;
   d->mode = FOPEN_AB;
@@ -705,6 +705,6 @@ Arguments are FILE OFFSET START END 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."));
-  c->completer = filename_completer;
+  set_cmd_completer_handle_brkchars (c, filename_completer_handle_brkchars);
   /* FIXME: completers for other commands.  */
 }
diff --git a/gdb/compile/compile.c b/gdb/compile/compile.c
index 2d97a1b2005..062418b3791 100644
--- a/gdb/compile/compile.c
+++ b/gdb/compile/compile.c
@@ -327,8 +327,7 @@ compile_file_command_completer (struct cmd_list_element *ignore,
       (tracker, &text, gdb::option::PROCESS_OPTIONS_UNKNOWN_IS_ERROR, group))
     return;
 
-  word = advance_to_filename_complete_word_point (tracker, text);
-  filename_completer (ignore, tracker, text, word);
+  filename_completer_handle_brkchars (ignore, tracker, text, word);
 }
 
 /* Handle the input from the 'compile code' command.  The
diff --git a/gdb/completer.c b/gdb/completer.c
index 171d1ca8c0e..c300e42f588 100644
--- a/gdb/completer.c
+++ b/gdb/completer.c
@@ -203,15 +203,15 @@ noop_completer (struct cmd_list_element *ignore,
 {
 }
 
-/* Complete on filenames.  */
+/* Generate filename completions of WORD, storing the completions into
+   TRACKER.  This is used for generating completions for commands that
+   only accept unquoted filenames as well as for commands that accept
+   quoted and escaped filenames.  */
 
-void
-filename_completer (struct cmd_list_element *ignore,
-		    completion_tracker &tracker,
-		    const char *text, const char *word)
+static void
+filename_completer_generate_completions (completion_tracker &tracker,
+					 const char *word)
 {
-  rl_completer_quote_characters = gdb_completer_file_name_quote_characters;
-
   int subsequent_name = 0;
   while (1)
     {
@@ -249,13 +249,23 @@ filename_completer (struct cmd_list_element *ignore,
     }
 }
 
-/* The corresponding completer_handle_brkchars
-   implementation.  */
+/* Complete on filenames.  */
+
+void
+filename_maybe_quoted_completer (struct cmd_list_element *ignore,
+				 completion_tracker &tracker,
+				 const char *text, const char *word)
+{
+  rl_completer_quote_characters = gdb_completer_file_name_quote_characters;
+  filename_completer_generate_completions (tracker, word);
+}
+
+/* The corresponding completer_handle_brkchars implementation.  */
 
 static void
-filename_completer_handle_brkchars (struct cmd_list_element *ignore,
-				    completion_tracker &tracker,
-				    const char *text, const char *word)
+filename_maybe_quoted_completer_handle_brkchars
+	(struct cmd_list_element *ignore, completion_tracker &tracker,
+	 const char *text, const char *word)
 {
   set_rl_completer_word_break_characters
     (gdb_completer_file_name_break_characters);
@@ -263,6 +273,18 @@ filename_completer_handle_brkchars (struct cmd_list_element *ignore,
   rl_completer_quote_characters = gdb_completer_file_name_quote_characters;
 }
 
+/* See completer.h.  */
+
+void
+filename_completer_handle_brkchars
+	(struct cmd_list_element *ignore, completion_tracker &tracker,
+	 const char *text, const char *word)
+{
+  gdb_assert (word == nullptr);
+  tracker.set_use_custom_word_point (true);
+  filename_completer_generate_completions (tracker, text);
+}
+
 /* Find the bounds of the current word for completion purposes, and
    return a pointer to the end of the word.  This mimics (and is a
    modified version of) readline's _rl_find_completion_word internal
@@ -443,17 +465,6 @@ advance_to_expression_complete_word_point (completion_tracker &tracker,
 
 /* See completer.h.  */
 
-const char *
-advance_to_filename_complete_word_point (completion_tracker &tracker,
-					 const char *text)
-{
-  const char *brk_chars = gdb_completer_file_name_break_characters;
-  const char *quote_chars = gdb_completer_file_name_quote_characters;
-  return advance_to_completion_word (tracker, brk_chars, quote_chars, text);
-}
-
-/* See completer.h.  */
-
 bool
 completion_tracker::completes_to_completion_word (const char *word)
 {
@@ -1877,8 +1888,8 @@ default_completer_handle_brkchars (struct cmd_list_element *ignore,
 completer_handle_brkchars_ftype *
 completer_handle_brkchars_func_for_completer (completer_ftype *fn)
 {
-  if (fn == filename_completer)
-    return filename_completer_handle_brkchars;
+  if (fn == filename_maybe_quoted_completer)
+    return filename_maybe_quoted_completer_handle_brkchars;
 
   if (fn == location_completer)
     return location_completer_handle_brkchars;
diff --git a/gdb/completer.h b/gdb/completer.h
index 98a12f3907c..fa21156bd1f 100644
--- a/gdb/completer.h
+++ b/gdb/completer.h
@@ -563,12 +563,6 @@ extern completion_result
 const char *advance_to_expression_complete_word_point
   (completion_tracker &tracker, const char *text);
 
-/* Assuming TEXT is an filename, find the completion word point for
-   TEXT, emulating the algorithm readline uses to find the word
-   point.  */
-extern const char *advance_to_filename_complete_word_point
-  (completion_tracker &tracker, const char *text);
-
 extern void noop_completer (struct cmd_list_element *,
 			    completion_tracker &tracker,
 			    const char *, const char *);
@@ -577,6 +571,29 @@ extern void filename_completer (struct cmd_list_element *,
 				completion_tracker &tracker,
 				const char *, const char *);
 
+/* Filename completer that can be registered for the brkchars phase of
+   completion.  This should be used by commands that don't allow the
+   filename to be quoted, and whitespace does not need to be escaped.
+
+   NOTE: If you are considering using this function as your commands
+   completer, then consider updating your function to use gdb_argv or
+   extract_string_maybe_quoted to allow for possibly quoted filenames
+   instead.  You would then use filename_maybe_quoted_completer for
+   filename completion.  The benefit of this is that in the future it is
+   possible to add additional arguments to your new command.  */
+
+extern void filename_completer_handle_brkchars
+	(struct cmd_list_element *ignore, completion_tracker &tracker,
+	 const char *text, const char *word);
+
+/* Filename completer for commands where the filename argument can be
+   quoted, and whitespace within an unquoted filename should be escaped
+   with a backslash.  */
+
+extern void filename_maybe_quoted_completer (struct cmd_list_element *,
+					     completion_tracker &tracker,
+					     const char *, const char *);
+
 extern void expression_completer (struct cmd_list_element *,
 				  completion_tracker &tracker,
 				  const char *, const char *);
diff --git a/gdb/corefile.c b/gdb/corefile.c
index 16cd60f7106..04a1021ee2b 100644
--- a/gdb/corefile.c
+++ b/gdb/corefile.c
@@ -404,9 +404,9 @@ Use FILE as core dump for examining memory and registers.\n\
 Usage: core-file FILE\n\
 No arg means have no core file.  This command has been superseded by the\n\
 `target core' and `detach' commands."), &cmdlist);
-  set_cmd_completer (core_file_cmd, filename_completer);
+  set_cmd_completer_handle_brkchars (core_file_cmd,
+				     filename_completer_handle_brkchars);
 
-  
   set_show_commands set_show_gnutarget
     = add_setshow_string_noescape_cmd ("gnutarget", class_files,
 				       &gnutarget_string, _("\
diff --git a/gdb/corelow.c b/gdb/corelow.c
index f4e8273d962..ff66d808120 100644
--- a/gdb/corelow.c
+++ b/gdb/corelow.c
@@ -1509,7 +1509,9 @@ void _initialize_corelow ();
 void
 _initialize_corelow ()
 {
-  add_target (core_target_info, core_target_open, filename_completer);
+  struct cmd_list_element *c
+    = add_target (core_target_info, core_target_open);
+  set_cmd_completer_handle_brkchars (c, filename_completer_handle_brkchars);
   add_cmd ("core-file-backed-mappings", class_maintenance,
 	   maintenance_print_core_file_backed_mappings,
 	   _("Print core file's file-backed mappings."),
diff --git a/gdb/dwarf2/index-write.c b/gdb/dwarf2/index-write.c
index 3f812285995..4f79c42bbf7 100644
--- a/gdb/dwarf2/index-write.c
+++ b/gdb/dwarf2/index-write.c
@@ -1620,8 +1620,7 @@ gdb_save_index_cmd_completer (struct cmd_list_element *ignore,
       (tracker, &text, gdb::option::PROCESS_OPTIONS_UNKNOWN_IS_OPERAND, grp))
     return;
 
-  word = advance_to_filename_complete_word_point (tracker, text);
-  filename_completer (ignore, tracker, text, word);
+  filename_completer_handle_brkchars (ignore, tracker, text, word);
 }
 
 /* Implementation of the `save gdb-index' command.
diff --git a/gdb/exec.c b/gdb/exec.c
index 98ad81fb99a..5210e6bc4ea 100644
--- a/gdb/exec.c
+++ b/gdb/exec.c
@@ -1067,14 +1067,14 @@ and it is the program executed when you use the `run' command.\n\
 If FILE cannot be found as specified, your execution directory path\n\
 ($PATH) is searched for a command of that name.\n\
 No arg means to have no executable file and no symbols."), &cmdlist);
-  set_cmd_completer (c, filename_completer);
+  set_cmd_completer (c, filename_maybe_quoted_completer);
 
   c = add_cmd ("exec-file", class_files, exec_file_command, _("\
 Use FILE as program for getting contents of pure memory.\n\
 If FILE cannot be found as specified, your execution directory path\n\
 is searched for a command of that name.\n\
 No arg means have no executable file."), &cmdlist);
-  set_cmd_completer (c, filename_completer);
+  set_cmd_completer (c, filename_maybe_quoted_completer);
 
   add_com ("section", class_files, set_section_command, _("\
 Change the base address of section SECTION of the exec file to ADDR.\n\
@@ -1112,5 +1112,6 @@ will be loaded as well."),
 			show_exec_file_mismatch_command,
 			&setlist, &showlist);
 
-  add_target (exec_target_info, exec_target_open, filename_completer);
+  c = add_target (exec_target_info, exec_target_open);
+  set_cmd_completer_handle_brkchars (c, filename_completer_handle_brkchars);
 }
diff --git a/gdb/guile/scm-cmd.c b/gdb/guile/scm-cmd.c
index d75d2b63c8c..037813250a9 100644
--- a/gdb/guile/scm-cmd.c
+++ b/gdb/guile/scm-cmd.c
@@ -110,7 +110,7 @@ struct cmdscm_completer
 static const struct cmdscm_completer cmdscm_completers[] =
 {
   { "COMPLETE_NONE", noop_completer },
-  { "COMPLETE_FILENAME", filename_completer },
+  { "COMPLETE_FILENAME", filename_maybe_quoted_completer },
   { "COMPLETE_LOCATION", location_completer },
   { "COMPLETE_COMMAND", command_completer },
   { "COMPLETE_SYMBOL", symbol_completer },
diff --git a/gdb/infcmd.c b/gdb/infcmd.c
index 10a964a90d7..568dfe3693b 100644
--- a/gdb/infcmd.c
+++ b/gdb/infcmd.c
@@ -3113,7 +3113,8 @@ Follow this command with any number of args, to be passed to the program."),
 				       get_args_value,
 				       show_args_command,
 				       &setlist, &showlist);
-  set_cmd_completer (args_set_show.set, filename_completer);
+  set_cmd_completer_handle_brkchars (args_set_show.set,
+				     filename_completer_handle_brkchars);
 
   auto cwd_set_show
     = add_setshow_string_noescape_cmd ("cwd", class_run, _("\
@@ -3129,7 +3130,8 @@ working directory."),
 				       set_cwd_value, get_inferior_cwd,
 				       show_cwd_command,
 				       &setlist, &showlist);
-  set_cmd_completer (cwd_set_show.set, filename_completer);
+  set_cmd_completer_handle_brkchars (cwd_set_show.set,
+				     filename_completer_handle_brkchars);
 
   c = add_cmd ("environment", no_class, environment_info, _("\
 The environment to give the program, or one variable's value.\n\
@@ -3163,7 +3165,7 @@ This path is equivalent to the $PATH shell variable.  It is a list of\n\
 directories, separated by colons.  These directories are searched to find\n\
 fully linked executable files and separately compiled object files as \
 needed."));
-  set_cmd_completer (c, filename_completer);
+  set_cmd_completer_handle_brkchars (c, filename_completer_handle_brkchars);
 
   c = add_cmd ("paths", no_class, path_info, _("\
 Current search path for finding object files.\n\
@@ -3313,18 +3315,19 @@ Specifying -a and an ignore count simultaneously is an error."));
     = add_com ("run", class_run, run_command, _("\
 Start debugged program.\n"
 RUN_ARGS_HELP));
-  set_cmd_completer (run_cmd, filename_completer);
+  set_cmd_completer_handle_brkchars (run_cmd,
+				     filename_completer_handle_brkchars);
   add_com_alias ("r", run_cmd, class_run, 1);
 
   c = add_com ("start", class_run, start_command, _("\
 Start the debugged program stopping at the beginning of the main procedure.\n"
 RUN_ARGS_HELP));
-  set_cmd_completer (c, filename_completer);
+  set_cmd_completer_handle_brkchars (c, filename_completer_handle_brkchars);
 
   c = add_com ("starti", class_run, starti_command, _("\
 Start the debugged program stopping at the first instruction.\n"
 RUN_ARGS_HELP));
-  set_cmd_completer (c, filename_completer);
+  set_cmd_completer_handle_brkchars (c, filename_completer_handle_brkchars);
 
   add_com ("interrupt", class_run, interrupt_command,
 	   _("Interrupt the execution of the debugged program.\n\
diff --git a/gdb/inferior.c b/gdb/inferior.c
index 4e1d789d1ba..c883f020585 100644
--- a/gdb/inferior.c
+++ b/gdb/inferior.c
@@ -1112,7 +1112,7 @@ as main program.\n\
 By default, the new inferior inherits the current inferior's connection.\n\
 If -no-connection is specified, the new inferior begins with\n\
 no target connection yet."));
-  set_cmd_completer (c, filename_completer);
+  set_cmd_completer_handle_brkchars (c, filename_completer_handle_brkchars);
 
   add_com ("remove-inferiors", no_class, remove_inferior_command, _("\
 Remove inferior ID (or list of IDs).\n\
diff --git a/gdb/jit.c b/gdb/jit.c
index 3843b84b0e6..aa0b5289d36 100644
--- a/gdb/jit.c
+++ b/gdb/jit.c
@@ -1327,7 +1327,7 @@ Usage: jit-reader-load FILE\n\
 Try to load file FILE as a debug info reader (and unwinder) for\n\
 JIT compiled code.  The file is loaded from " JIT_READER_DIR ",\n\
 relocated relative to the GDB executable if required."));
-      set_cmd_completer (c, filename_completer);
+      set_cmd_completer_handle_brkchars (c, filename_completer_handle_brkchars);
 
       c = add_com ("jit-reader-unload", no_class,
 		   jit_reader_unload_command, _("\
diff --git a/gdb/python/py-cmd.c b/gdb/python/py-cmd.c
index f18a8e8eaa9..dec11f75c08 100644
--- a/gdb/python/py-cmd.c
+++ b/gdb/python/py-cmd.c
@@ -39,7 +39,7 @@ struct cmdpy_completer
 static const struct cmdpy_completer completers[] =
 {
   { "COMPLETE_NONE", noop_completer },
-  { "COMPLETE_FILENAME", filename_completer },
+  { "COMPLETE_FILENAME", filename_maybe_quoted_completer },
   { "COMPLETE_LOCATION", location_completer },
   { "COMPLETE_COMMAND", command_completer },
   { "COMPLETE_SYMBOL", symbol_completer },
diff --git a/gdb/record-full.c b/gdb/record-full.c
index 2e67cf5b428..c13b5b8d644 100644
--- a/gdb/record-full.c
+++ b/gdb/record-full.c
@@ -2888,12 +2888,13 @@ _initialize_record_full ()
 	       _("Restore the execution log from a file.\n\
 Argument is filename.  File must be created with 'record save'."),
 	       &record_full_cmdlist);
-  set_cmd_completer (record_full_restore_cmd, filename_completer);
+  set_cmd_completer_handle_brkchars (record_full_restore_cmd,
+				     filename_completer_handle_brkchars);
 
   /* Deprecate the old version without "full" prefix.  */
   c = add_alias_cmd ("restore", record_full_restore_cmd, class_obscure, 1,
 		     &record_cmdlist);
-  set_cmd_completer (c, filename_completer);
+  set_cmd_completer_handle_brkchars (c, filename_completer_handle_brkchars);
   deprecate_cmd (c, "record full restore");
 
   add_setshow_prefix_cmd ("full", class_support,
diff --git a/gdb/record.c b/gdb/record.c
index 5b1093dd12e..daa3d5bd1e3 100644
--- a/gdb/record.c
+++ b/gdb/record.c
@@ -820,7 +820,7 @@ A size of \"unlimited\" means unlimited lines.  The default is 10."),
 Usage: record save [FILENAME]\n\
 Default filename is 'gdb_record.PROCESS_ID'."),
 	       &record_cmdlist);
-  set_cmd_completer (c, filename_completer);
+  set_cmd_completer_handle_brkchars (c, filename_completer_handle_brkchars);
 
   cmd_list_element *record_delete_cmd
     =  add_cmd ("delete", class_obscure, cmd_record_delete,
diff --git a/gdb/skip.c b/gdb/skip.c
index f2818eccb34..c932598d243 100644
--- a/gdb/skip.c
+++ b/gdb/skip.c
@@ -683,7 +683,7 @@ Ignore a file while stepping.\n\
 Usage: skip file [FILE-NAME]\n\
 If no filename is given, ignore the current file."),
 	       &skiplist);
-  set_cmd_completer (c, filename_completer);
+  set_cmd_completer_handle_brkchars (c, filename_completer_handle_brkchars);
 
   c = add_cmd ("function", class_breakpoint, skip_function_command, _("\
 Ignore a function while stepping.\n\
diff --git a/gdb/source.c b/gdb/source.c
index 432301e2a71..7c0cea08aa3 100644
--- a/gdb/source.c
+++ b/gdb/source.c
@@ -1916,7 +1916,8 @@ directory in which the source file was compiled into object code.\n\
 With no argument, reset the search path to $cdir:$cwd, the default."),
 	       &cmdlist);
 
-  set_cmd_completer (directory_cmd, filename_completer);
+  set_cmd_completer_handle_brkchars (directory_cmd,
+				     filename_completer_handle_brkchars);
 
   add_setshow_optional_filename_cmd ("directories",
 				     class_files,
diff --git a/gdb/symfile.c b/gdb/symfile.c
index 2a7d41dc974..b9c21d33ac0 100644
--- a/gdb/symfile.c
+++ b/gdb/symfile.c
@@ -3870,7 +3870,7 @@ Usage: symbol-file [-readnow | -readnever] [-o OFF] FILE\n\
 OFF is an optional offset which is added to each section address.\n\
 The `file' command can also load symbol tables, as well as setting the file\n\
 to execute.\n" READNOW_READNEVER_HELP), &cmdlist);
-  set_cmd_completer (c, filename_completer);
+  set_cmd_completer (c, filename_maybe_quoted_completer);
 
   c = add_cmd ("add-symbol-file", class_files, add_symbol_file_command, _("\
 Load symbols from FILE, assuming FILE has been dynamically loaded.\n\
@@ -3884,7 +3884,7 @@ OFF is an optional offset which is added to the default load addresses\n\
 of all sections for which no other address was specified.\n"
 READNOW_READNEVER_HELP),
 	       &cmdlist);
-  set_cmd_completer (c, filename_completer);
+  set_cmd_completer (c, filename_maybe_quoted_completer);
 
   c = add_cmd ("remove-symbol-file", class_files,
 	       remove_symbol_file_command, _("\
@@ -3894,6 +3894,7 @@ Usage: remove-symbol-file FILENAME\n\
 The file to remove can be identified by its filename or by an address\n\
 that lies within the boundaries of this symbol file in memory."),
 	       &cmdlist);
+  set_cmd_completer (c, filename_maybe_quoted_completer);
 
   c = add_cmd ("load", class_files, load_command, _("\
 Dynamically load FILE into the running program.\n\
@@ -3902,7 +3903,7 @@ Usage: load [FILE] [OFFSET]\n\
 An optional load OFFSET may also be given as a literal address.\n\
 When OFFSET is provided, FILE must also be provided.  FILE can be provided\n\
 on its own."), &cmdlist);
-  set_cmd_completer (c, filename_completer);
+  set_cmd_completer_handle_brkchars (c, filename_completer_handle_brkchars);
 
   cmd_list_element *overlay_cmd
     = add_basic_prefix_cmd ("overlay", class_support,
diff --git a/gdb/target-descriptions.c b/gdb/target-descriptions.c
index 8aca5cf719b..dead63823d1 100644
--- a/gdb/target-descriptions.c
+++ b/gdb/target-descriptions.c
@@ -1762,8 +1762,7 @@ 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_filename_complete_word_point (tracker, text);
-  filename_completer (ignore, tracker, text, word);
+  filename_completer_handle_brkchars (ignore, tracker, text, word);
 }
 
 /* Implement the maintenance print xml-tdesc command.  */
@@ -1947,7 +1946,7 @@ that feature within an already existing target_desc object."), grp);
   cmd = add_cmd ("xml-tdesc", class_maintenance, maint_print_xml_tdesc_cmd, _("\
 Print the current target description as an XML file."),
 		 &maintenanceprintlist);
-  set_cmd_completer (cmd, filename_completer);
+  set_cmd_completer_handle_brkchars (cmd, filename_completer_handle_brkchars);
 
   cmd = add_cmd ("xml-descriptions", class_maintenance,
 		 maintenance_check_xml_descriptions, _("\
@@ -1956,5 +1955,5 @@ Check the target descriptions created in GDB equal the descriptions\n\
 created from XML files in the directory.\n\
 The parameter is the directory name."),
 		 &maintenancechecklist);
-  set_cmd_completer (cmd, filename_completer);
+  set_cmd_completer_handle_brkchars (cmd, filename_completer_handle_brkchars);
 }
diff --git a/gdb/target.c b/gdb/target.c
index 107a84b3ca1..949daf82b0a 100644
--- a/gdb/target.c
+++ b/gdb/target.c
@@ -829,7 +829,7 @@ open_target (const char *args, int from_tty, struct cmd_list_element *command)
 
 /* See target.h.  */
 
-void
+struct cmd_list_element *
 add_target (const target_info &t, target_open_ftype *func,
 	    completer_ftype *completer)
 {
@@ -853,6 +853,8 @@ information on the arguments for a particular protocol, type\n\
   c->func = open_target;
   if (completer != NULL)
     set_cmd_completer (c, completer);
+
+  return c;
 }
 
 /* See target.h.  */
diff --git a/gdb/target.h b/gdb/target.h
index c9eaff16346..fe8a9c7a60b 100644
--- a/gdb/target.h
+++ b/gdb/target.h
@@ -2370,11 +2370,12 @@ typedef void target_open_ftype (const char *args, int from_tty);
 
 /* Add the target described by INFO to the list of possible targets
    and add a new command 'target $(INFO->shortname)'.  Set COMPLETER
-   as the command's completer if not NULL.  */
+   as the command's completer if not NULL.  Return the new target
+   command.  */
 
-extern void add_target (const target_info &info,
-			target_open_ftype *func,
-			completer_ftype *completer = NULL);
+extern struct cmd_list_element *add_target
+	(const target_info &info, target_open_ftype *func,
+	 completer_ftype *completer = NULL);
 
 /* Adds a command ALIAS for the target described by INFO and marks it
    deprecated.  This is useful for maintaining backwards compatibility
diff --git a/gdb/testsuite/gdb.base/filename-completion.exp b/gdb/testsuite/gdb.base/filename-completion.exp
index b700977cec5..1ccaaff9afc 100644
--- a/gdb/testsuite/gdb.base/filename-completion.exp
+++ b/gdb/testsuite/gdb.base/filename-completion.exp
@@ -23,8 +23,16 @@ load_lib completion-support.exp
 #
 # root/			[ DIRECTORY ]
 #   aaa/		[ DIRECTORY ]
+#     aa bb		[ FILE ]
+#     aa cc		[ FILE ]
+#   aaa/		[ DIRECTORY ]
 #   bb1/		[ DIRECTORY ]
 #   bb2/		[ DIRECTORY ]
+#     dir 1/		[ DIRECTORY ]
+#       unique file	[ FILE ]
+#     dir 2/		[ DIRECTORY ]
+#       file 1		[ FILE ]
+#       file 2		[ FILE ]
 #   cc1/		[ DIRECTORY ]
 #   cc2			[ FILE ]
 proc setup_directory_tree {} {
@@ -36,68 +44,139 @@ proc setup_directory_tree {} {
     remote_exec host "mkdir -p ${root}/bb2"
     remote_exec host "mkdir -p ${root}/cc1"
     remote_exec host "touch ${root}/cc2"
-
     remote_exec host "touch \"${root}/aaa/aa bb\""
     remote_exec host "touch \"${root}/aaa/aa cc\""
+    remote_exec host "mkdir -p \"${root}/bb2/dir 1\""
+    remote_exec host "mkdir -p \"${root}/bb2/dir 2\""
+    remote_exec host "touch \"${root}/bb2/dir 1/unique file\""
+    remote_exec host "touch \"${root}/bb2/dir 2/file 1\""
+    remote_exec host "touch \"${root}/bb2/dir 2/file 2\""
 
     return $root
 }
 
-# Run filename completetion tests.  ROOT is the base directory as
-# returned from setup_directory_tree, though, if ROOT is a
-# sub-directory of the user's home directory ROOT might have been
-# modified to replace the $HOME prefix with a single "~" character.
-proc run_tests { root } {
-
-    # Completing 'thread apply all ...' commands uses a custom word
-    # point.  At one point we had a bug where doing this would break
-    # completion of quoted filenames that contained white space.
-    test_gdb_complete_unique "thread apply all hel" \
-	"thread apply all help" " " false \
-	"complete a 'thread apply all' command"
-
-    foreach_with_prefix qc [list "" "'" "\""] {
-	test_gdb_complete_none "file ${qc}${root}/xx" \
+# Run filename completetion tests for those command that accept quoting and
+# escaping of the filename argument.
+#
+# ROOT is the base directory as returned from setup_directory_tree, though,
+# if ROOT is a sub-directory of the user's home directory ROOT might have
+# been modified to replace the $HOME prefix with a single "~" character.
+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 } {
+	gdb_start
+
+	# Completing 'thread apply all ...' commands uses a custom word
+	# point.  At one point we had a bug where doing this would break
+	# completion of quoted filenames that contained white space.
+	test_gdb_complete_unique "thread apply all hel" \
+	    "thread apply all help" " " false \
+	    "complete a 'thread apply all' command"
+
+	foreach_with_prefix qc [list "" "'" "\""] {
+	    test_gdb_complete_none "$cmd ${qc}${root}/xx" \
+		"expand a non-existent filename"
+
+	    test_gdb_complete_unique "$cmd ${qc}${root}/a" \
+		"$cmd ${qc}${root}/aaa/" "" false \
+		"expand a unique filename"
+
+	    test_gdb_complete_multiple "$cmd ${qc}${root}/" \
+		"b" "b" {
+		    "bb1/"
+		    "bb2/"
+		} "" "${qc}" false \
+		"expand multiple directory names"
+
+	    test_gdb_complete_multiple "$cmd ${qc}${root}/" \
+		"c" "c" {
+		    "cc1/"
+		    "cc2"
+		} "" "${qc}" false \
+		"expand mixed directory and file names"
+
+	    # GDB does not currently escape word break characters
+	    # (e.g. white space) correctly in unquoted filenames.
+	    if { $qc ne "" } {
+		set sp " "
+
+		test_gdb_complete_multiple "$cmd ${qc}${root}/aaa/" \
+		    "a" "a${sp}" {
+			"aa bb"
+			"aa cc"
+		    } "" "${qc}" false \
+		    "expand filenames containing spaces"
+	    }
+	}
+
+	gdb_exit
+    }
+}
+
+# Run filename completetion tests for a sample of commands that take an
+# unquoted, unescaped filename as an argument.  Only a sample of commands
+# are (currently) tested as there's a lot of commands that accept this style
+# of filename argument.
+#
+# ROOT is the base directory as returned from setup_directory_tree, though,
+# if ROOT is a sub-directory of the user's home directory ROOT might have
+# been modified to replace the $HOME prefix with a single "~" character.
+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 { "set logging file" "target core" \
+				  "add-auto-load-safe-path" } {
+	gdb_start
+
+	test_gdb_complete_none "$cmd ${root}/xx" \
 	    "expand a non-existent filename"
 
-	test_gdb_complete_unique "file ${qc}${root}/a" \
-	    "file ${qc}${root}/aaa/" "" false \
+	test_gdb_complete_unique "$cmd ${root}/a" \
+	    "$cmd ${root}/aaa/" "" false \
 	    "expand a unique filename"
 
-	test_gdb_complete_multiple "file ${qc}${root}/" \
+	test_gdb_complete_unique "$cmd ${root}/bb2/dir 1/uni" \
+	    "$cmd ${root}/bb2/dir 1/unique file" " " false \
+	    "expand a unique filename containing whitespace"
+
+	test_gdb_complete_multiple "$cmd ${root}/" \
 	    "b" "b" {
 		"bb1/"
 		"bb2/"
-	    } "" "${qc}" false \
+	    } "" "" false \
 	    "expand multiple directory names"
 
-	test_gdb_complete_multiple "file ${qc}${root}/" \
+	test_gdb_complete_multiple "$cmd ${root}/" \
 	    "c" "c" {
 		"cc1/"
 		"cc2"
-	    } "" "${qc}" false \
+	    } "" "" false \
 	    "expand mixed directory and file names"
 
-	# GDB does not currently escape word break characters
-	# (e.g. white space) correctly in unquoted filenames.
-	if { $qc ne "" } {
-	    set sp " "
-
-	    test_gdb_complete_multiple "file ${qc}${root}/aaa/" \
-		"a" "a${sp}" {
-		    "aa bb"
-		    "aa cc"
-		} "" "${qc}" false \
-		"expand filenames containing spaces"
-	}
+	test_gdb_complete_multiple "$cmd ${root}/aaa/" \
+	    "a" "a " {
+		"aa bb"
+		"aa cc"
+	    } "" "" false \
+	    "expand filenames containing spaces"
+
+	test_gdb_complete_multiple "$cmd ${root}/bb2/dir 2/" \
+	    "fi" "le " {
+		"file 1"
+		"file 2"
+	    } "" "" false \
+	    "expand filenames containing spaces in path"
+
+	gdb_exit
     }
 }
 
-gdb_start
-
 set root [setup_directory_tree]
 
-run_tests $root
+run_quoting_and_escaping_tests $root
+run_unquoted_tests $root
 
 # This test relies on using the $HOME directory.  We could make this
 # work for remote hosts, but right now, this isn't supported.
@@ -114,7 +193,8 @@ if {![is_remote host]} {
 
 	with_test_prefix "with tilde" {
 	    # And rerun the tests.
-	    run_tests $tilde_root
+	    run_quoting_and_escaping_tests $tilde_root
+	    run_unquoted_tests $tilde_root
 	}
     }
 }
diff --git a/gdb/tracectf.c b/gdb/tracectf.c
index 282a8250ac1..94b8a283ac6 100644
--- a/gdb/tracectf.c
+++ b/gdb/tracectf.c
@@ -1721,6 +1721,7 @@ void
 _initialize_ctf ()
 {
 #if HAVE_LIBBABELTRACE
-  add_target (ctf_target_info, ctf_target_open, filename_completer);
+  struct cmd_list_element *c = add_target (ctf_target_info, ctf_target_open);
+  set_cmd_completer_handle_brkchars (c, filename_completer_handle_brkchars);
 #endif
 }
diff --git a/gdb/tracefile-tfile.c b/gdb/tracefile-tfile.c
index 79af963b049..14909daa1cc 100644
--- a/gdb/tracefile-tfile.c
+++ b/gdb/tracefile-tfile.c
@@ -1119,5 +1119,7 @@ void _initialize_tracefile_tfile ();
 void
 _initialize_tracefile_tfile ()
 {
-  add_target (tfile_target_info, tfile_target_open, filename_completer);
+  struct cmd_list_element *c =
+    add_target (tfile_target_info, tfile_target_open);
+  set_cmd_completer_handle_brkchars (c, filename_completer_handle_brkchars);
 }
-- 
2.25.4

[PATCHv2 3/8] gdb: improve escaping when completing filenames

[Andrew Burgess]

This improves quoting and escaping when completing filenames for
commands that allow filenames to be quoted and escaped.

I've struggled a bit trying to split this series into chunks.  There's
a lot of dependencies between different parts of the completion
system, and trying to get this working correctly is pretty messy.

This first step is really about implementing 3 readline hooks:

  rl_char_is_quoted_p
    - Is a particular character quoted within readline's input buffer?
  rl_filename_dequoting_function
    - Remove quoting characters from a filename.
  rl_filename_quoting_function
    - Add quoting characters to a filename.

See 'info readline' for full details, but with these hooks connected
up, readline (on behalf of GDB) should do a better job inserting
backslash escapes when completing filenames.

There's still a bunch of stuff that doesn't work after this commit,
mostly around the 'complete' command which of course doesn't go
through readline, so doesn't benefit from all of these new functions
yet, I'll add some of this in a later commit.

Tab completion is now slightly improved though, it is possible to
tab-complete a filename that includes a double or single quote, either
in an unquoted string or within a string surrounded by single or
double quotes, backslash escaping is used when necessary.

There are some additional tests to cover the new functionality.
---
 gdb/completer.c                               | 169 +++++++++++++++++-
 .../gdb.base/filename-completion.exp          |  36 +++-
 2 files changed, 201 insertions(+), 4 deletions(-)

diff --git a/gdb/completer.c b/gdb/completer.c
index c300e42f588..396b1e0136b 100644
--- a/gdb/completer.c
+++ b/gdb/completer.c
@@ -203,6 +203,159 @@ noop_completer (struct cmd_list_element *ignore,
 {
 }
 
+/* Return 1 if the character at EINDEX in STRING is quoted (there is an
+   unclosed quoted string), or if the character at EINDEX is quoted by a
+   backslash.  */
+
+static int
+gdb_completer_file_name_char_is_quoted (char *string, int eindex)
+{
+  for (int i = 0; i <= eindex && string[i] != '\0'; )
+    {
+      char c = string[i];
+
+      if (c == '\\')
+	{
+	  /* The backslash itself is not quoted.  */
+	  if (i >= eindex)
+	    return 0;
+	  ++i;
+	  /* But the next character is.  */
+	  if (i >= eindex)
+	    return 1;
+	  if (string[i] == '\0')
+	    return 0;
+	  ++i;
+	  continue;
+	}
+      else if (strchr (rl_completer_quote_characters, c) != nullptr)
+	{
+	  /* This assumes that extract_string_maybe_quoted can handle a
+	     string quoted with character C.  Currently this is true as the
+	     only characters we put in rl_completer_quote_characters are
+	     single and/or double quotes, both of which
+	     extract_string_maybe_quoted can handle.  */
+	  gdb_assert (c == '"' || c == '\'');
+	  const char *tmp = &string[i];
+	  (void) extract_string_maybe_quoted (&tmp);
+	  i = tmp - string;
+
+	  /* Consider any character within the string we just skipped over
+	     as quoted, though this might not be completely correct; the
+	     opening and closing quotes are not themselves quoted.  But so
+	     far this doesn't seem to have caused any issues.  */
+	  if (i >= eindex)
+	    return 1;
+	}
+      else
+	++i;
+    }
+
+  return 0;
+}
+
+/* Removing character escaping from FILENAME.  QUOTE_CHAR is the quote
+   character around FILENAME or the null-character if there is no quoting
+   around FILENAME.  */
+
+static char *
+gdb_completer_file_name_dequote (char *filename, int quote_char)
+{
+  std::string tmp;
+
+  if (quote_char == '\'')
+    {
+      /* There is no backslash escaping within a single quoted string.  In
+	 this case we can just return the input string.  */
+      tmp = filename;
+    }
+  else if (quote_char == '"')
+    {
+      /* Remove escaping from a double quoted string.  */
+      for (const char *input = filename;
+	   *input != '\0';
+	   ++input)
+	{
+	  if (input[0] == '\\'
+	      && input[1] != '\0'
+	      && strchr ("\"\\", input[1]) != nullptr)
+	    ++input;
+	  tmp += *input;
+	}
+    }
+  else
+    {
+      gdb_assert (quote_char == '\0');
+
+      /* Remove escaping from an unquoted string.  */
+      for (const char *input = filename;
+	   *input != '\0';
+	   ++input)
+	{
+	  /* We allow anything to be escaped in an unquoted string.  */
+	  if (*input == '\\')
+	    {
+	      ++input;
+	      if (*input == '\0')
+		break;
+	    }
+
+	  tmp += *input;
+	}
+    }
+
+  return strdup (tmp.c_str ());
+}
+
+/* Apply character escaping to the file name in TEXT.  QUOTE_PTR points to
+   the quote character surrounding TEXT, or points to the null-character if
+   there are no quotes around TEXT.  MATCH_TYPE will be one of the readline
+   constants SINGLE_MATCH or MULTI_MATCH depending on if there is one or
+   many completions.  */
+
+static char *
+gdb_completer_file_name_quote (char *text, int match_type ATTRIBUTE_UNUSED,
+			       char *quote_ptr)
+{
+  std::string str;
+
+  if (*quote_ptr == '\'')
+    {
+      /* There is no backslash escaping permitted within a single quoted
+	 string, so in this case we can just return the input sting.  */
+      str = text;
+    }
+  else if (*quote_ptr == '"')
+    {
+      /* Add escaping for a double quoted filename.  */
+      for (const char *input = text;
+	   *input != '\0';
+	   ++input)
+	{
+	  if (strchr ("\"\\", *input) != nullptr)
+	    str += '\\';
+	  str += *input;
+	}
+    }
+  else
+    {
+      gdb_assert (*quote_ptr == '\0');
+
+      /* Add escaping for an unquoted filename.  */
+      for (const char *input = text;
+	   *input != '\0';
+	   ++input)
+	{
+	  if (strchr (" \t\n\\\"'", *input)
+	      != nullptr)
+	    str += '\\';
+	  str += *input;
+	}
+    }
+
+  return strdup (str.c_str ());
+}
+
 /* Generate filename completions of WORD, storing the completions into
    TRACKER.  This is used for generating completions for commands that
    only accept unquoted filenames as well as for commands that accept
@@ -256,6 +409,7 @@ filename_maybe_quoted_completer (struct cmd_list_element *ignore,
 				 completion_tracker &tracker,
 				 const char *text, const char *word)
 {
+  rl_char_is_quoted_p = gdb_completer_file_name_char_is_quoted;
   rl_completer_quote_characters = gdb_completer_file_name_quote_characters;
   filename_completer_generate_completions (tracker, word);
 }
@@ -271,6 +425,7 @@ filename_maybe_quoted_completer_handle_brkchars
     (gdb_completer_file_name_break_characters);
 
   rl_completer_quote_characters = gdb_completer_file_name_quote_characters;
+  rl_char_is_quoted_p = gdb_completer_file_name_char_is_quoted;
 }
 
 /* See completer.h.  */
@@ -1271,6 +1426,7 @@ complete_line_internal_1 (completion_tracker &tracker,
      completing file names then we can switch to the file name quote
      character set (i.e., both single- and double-quotes).  */
   rl_completer_quote_characters = gdb_completer_expression_quote_characters;
+  rl_char_is_quoted_p = nullptr;
 
   /* Decide whether to complete on a list of gdb commands or on
      symbols.  */
@@ -2163,9 +2319,11 @@ completion_tracker::build_completion_result (const char *text,
   /* Build replacement word, based on the LCD.  */
 
   recompute_lowest_common_denominator ();
-  match_list[0]
-    = expand_preserving_ws (text, end - start,
-			    m_lowest_common_denominator);
+  if (rl_filename_completion_desired)
+    match_list[0] = xstrdup (m_lowest_common_denominator);
+  else
+    match_list[0]
+      = expand_preserving_ws (text, end - start, m_lowest_common_denominator);
 
   if (m_lowest_common_denominator_unique)
     {
@@ -3028,6 +3186,11 @@ _initialize_completer ()
   rl_attempted_completion_function = gdb_rl_attempted_completion_function;
   set_rl_completer_word_break_characters (default_word_break_characters ());
 
+  /* Setup readline globals relating to filename completion.  */
+  rl_filename_quote_characters = " \t\n\\\"'";
+  rl_filename_dequoting_function = gdb_completer_file_name_dequote;
+  rl_filename_quoting_function = gdb_completer_file_name_quote;
+
   add_setshow_zuinteger_unlimited_cmd ("max-completions", no_class,
 				       &max_completions, _("\
 Set maximum number of completion candidates."), _("\
diff --git a/gdb/testsuite/gdb.base/filename-completion.exp b/gdb/testsuite/gdb.base/filename-completion.exp
index 1ccaaff9afc..275140ffd9d 100644
--- a/gdb/testsuite/gdb.base/filename-completion.exp
+++ b/gdb/testsuite/gdb.base/filename-completion.exp
@@ -52,6 +52,9 @@ proc setup_directory_tree {} {
     remote_exec host "touch \"${root}/bb2/dir 2/file 1\""
     remote_exec host "touch \"${root}/bb2/dir 2/file 2\""
 
+    remote_exec host "touch \"${root}/bb1/aa\\\"bb\""
+    remote_exec host "touch \"${root}/bb1/aa'bb\""
+
     return $root
 }
 
@@ -102,12 +105,43 @@ proc run_quoting_and_escaping_tests { root } {
 	    if { $qc ne "" } {
 		set sp " "
 
-		test_gdb_complete_multiple "$cmd ${qc}${root}/aaa/" \
+		test_gdb_complete_multiple "file ${qc}${root}/aaa/" \
 		    "a" "a${sp}" {
 			"aa bb"
 			"aa cc"
 		    } "" "${qc}" false \
 		    "expand filenames containing spaces"
+
+		test_gdb_complete_multiple "file ${qc}${root}/bb1/" \
+		    "a" "a" {
+			"aa\"bb"
+			"aa'bb"
+		    } "" "${qc}" false \
+		    "expand filenames containing quotes"
+	    } else {
+		set sp "\\ "
+
+		test_gdb_complete_tab_multiple "file ${qc}${root}/aaa/a" \
+		    "a${sp}" {
+			"aa bb"
+			"aa cc"
+		    } false \
+		    "expand filenames containing spaces"
+
+		test_gdb_complete_tab_multiple "file ${qc}${root}/bb1/a" \
+		    "a" {
+			"aa\"bb"
+			"aa'bb"
+		    } false \
+		    "expand filenames containing quotes"
+
+		test_gdb_complete_tab_unique "file ${qc}${root}/bb1/aa\\\"" \
+		    "file ${qc}${root}/bb1/aa\\\\\"bb${qc}" " " \
+		    "expand unique filename containing double quotes"
+
+		test_gdb_complete_tab_unique "file ${qc}${root}/bb1/aa\\'" \
+		    "file ${qc}${root}/bb1/aa\\\\'bb${qc}" " " \
+		    "expand unique filename containing single quote"
 	    }
 	}
 
-- 
2.25.4

[PATCHv2 4/8] gdb: move display of completion results into completion_result class

[Andrew Burgess]

This commit moves the printing of the 'complete' command results out
of the 'complete_command' function.  The printing is now done in a new
member function 'completion_result::print_matches'.  At this point,
this is entirely a refactor.

The motivation for this refactor is how 'complete' should print the
completion of filename arguments.  In some cases the filename results
need to have escaping added to the output.  This escaping needs to be
done immediately prior to printing the result as adding too early will
result in multiple 'complete' results potentially being sorted
incorrectly.  See the subsequent commits for more details.

There should be no user visible changes after this commit.
---
 gdb/cli/cli-cmds.c | 26 +-------------------------
 gdb/completer.c    | 33 +++++++++++++++++++++++++++++++++
 gdb/completer.h    | 13 +++++++++++++
 3 files changed, 47 insertions(+), 25 deletions(-)

diff --git a/gdb/cli/cli-cmds.c b/gdb/cli/cli-cmds.c
index 0a537ca9661..efde7e18f44 100644
--- a/gdb/cli/cli-cmds.c
+++ b/gdb/cli/cli-cmds.c
@@ -423,31 +423,7 @@ complete_command (const char *arg, int from_tty)
     {
       std::string arg_prefix (arg, word - arg);
 
-      if (result.number_matches == 1)
-	printf_unfiltered ("%s%s\n", arg_prefix.c_str (), result.match_list[0]);
-      else
-	{
-	  result.sort_match_list ();
-
-	  for (size_t i = 0; i < result.number_matches; i++)
-	    {
-	      printf_unfiltered ("%s%s",
-				 arg_prefix.c_str (),
-				 result.match_list[i + 1]);
-	      if (quote_char)
-		printf_unfiltered ("%c", quote_char);
-	      printf_unfiltered ("\n");
-	    }
-	}
-
-      if (result.number_matches == max_completions)
-	{
-	  /* ARG_PREFIX and WORD are included in the output so that emacs
-	     will include the message in the output.  */
-	  printf_unfiltered (_("%s%s %s\n"),
-			     arg_prefix.c_str (), word,
-			     get_max_completions_reached_message ());
-	}
+      result.print_matches (arg_prefix, word, quote_char);
     }
 }
 
diff --git a/gdb/completer.c b/gdb/completer.c
index 396b1e0136b..af1c09b45b1 100644
--- a/gdb/completer.c
+++ b/gdb/completer.c
@@ -2459,6 +2459,39 @@ completion_result::reset_match_list ()
     }
 }
 
+/* See completer.h  */
+
+void
+completion_result::print_matches (const std::string &prefix,
+				  const char *word, int quote_char)
+{
+  if (this->number_matches == 1)
+    printf_unfiltered ("%s%s\n", prefix.c_str (), this->match_list[0]);
+  else
+    {
+      this->sort_match_list ();
+
+      for (size_t i = 0; i < this->number_matches; i++)
+	{
+	  printf_unfiltered ("%s%s", prefix.c_str (),
+			     this->match_list[i + 1]);
+	  if (quote_char)
+	    printf_unfiltered ("%c", quote_char);
+	  printf_unfiltered ("\n");
+	}
+    }
+
+  if (this->number_matches == max_completions)
+    {
+      /* PREFIX and WORD are included in the output so that emacs will
+	 include the message in the output.  */
+      printf_unfiltered (_("%s%s %s\n"),
+			 prefix.c_str (), word,
+			 get_max_completions_reached_message ());
+    }
+
+}
+
 /* Helper for gdb_rl_attempted_completion_function, which does most of
    the work.  This is called by readline to build the match list array
    and to determine the lowest common denominator.  The real matches
diff --git a/gdb/completer.h b/gdb/completer.h
index fa21156bd1f..200d8a9b3af 100644
--- a/gdb/completer.h
+++ b/gdb/completer.h
@@ -268,6 +268,19 @@ struct completion_result
   /* Sort the match list.  */
   void sort_match_list ();
 
+  /* Called to display all matches (used by the 'complete' command).
+     PREFIX is everything before the completion word.  WORD is the word
+     being completed, this is only used if we reach the maximum number of
+     completions, otherwise, each line of output consists of PREFIX
+     followed by one of the possible completion words.
+
+     The QUOTE_CHAR is appended after each possible completion word and
+     should be the quote character that appears before the completion word,
+     or the null-character if there is no quote before the completion
+     word.  */
+  void print_matches (const std::string &prefix, const char *word,
+		      int quote_char);
+
 private:
   /* Destroy the match list array and its contents.  */
   void reset_match_list ();
-- 
2.25.4

[PATCHv2 5/8] gdb: simplify completion_result::print_matches

[Andrew Burgess]

Simplify completion_result::print_matches by removing one of the code
paths.  Now, every time we call ::print_matches we always add the
trailing quote.

Previously, when using the 'complete' command, if there was only one
result then trailing quote was added in ::build_completion_result, but
when we had multiple results the trailing quote was added in
::print_matches.  As a consequence, ::print_matches had to understand
not to add the trailing quote for the single result case.

After this commit we don't add the trailing quote in
::build_completion_result, instead ::print_matches always adds the
trailing quote, which makes ::print_matches simpler.

However, there is a slight problem.  When completion is being driven
by readline, and not by the 'complete' command, we still need to
manually add the trailing quote in the single result case, and as the
printing is done by readline we can't add the quote at the time of
printing, and so, in ::build_completion_result, we still add the
trailing quote, but only when completion is being done for readline.

And this does cause a small problem.  When completing a filename, if
the completion results in a directory name then, when using the
'complete' command, GDB should not be adding a trailing quote.  For
example, if we have the file /tmp/xxx/foo.c, then what we should see
is this:

  (gdb) complete file '/tmp/xx
  file 'tmp/xxx/

But what we actually see after this commit is this:

  (gdb) complete file '/tmp/xx
  file 'tmp/xxx/'

Previously we didn't get the trailing quote in this case, as when
there is only a single result, the quote was added in
::build_completion_result, and for filename completion, GDB didn't
know what the quote character was in ::build_completion_result, so no
quote was added.  Now that the trailing quote is always added in
::print_matches, and GDB does know the quote character at this point,
so we are now getting the trailing quote, which is not correct.

This is a regression, but really, GDB is now broken in a consistent
way, if we create the file /tmp/xxa/bar.c, then previously if we did
this:

  (gdb) complete file '/tmp/xx
  file '/tmp/xxa/'
  file '/tmp/xxx/'

Notice how we get the trailing quote in this case, this is the before
patch behaviour, and is also wrong.

A later commit will fix things so that the trailing quote is not added
in this filename completion case, but for now I'm going to accept this
small regression.

This change in behaviour caused some failures in one of the completion
tests, I've tweaked the test case to expect the trailing quote as part
of this commit, but will revert this in a later commit in this series.

I've also added an extra test for when the 'complete' command does
complete to a single complete filename, in which case the trailing
quote is expected.
---
 gdb/completer.c                               | 62 +++++++++----------
 .../gdb.base/filename-completion.exp          | 21 ++++++-
 2 files changed, 50 insertions(+), 33 deletions(-)

diff --git a/gdb/completer.c b/gdb/completer.c
index af1c09b45b1..4ba45a35f8f 100644
--- a/gdb/completer.c
+++ b/gdb/completer.c
@@ -2327,23 +2327,30 @@ completion_tracker::build_completion_result (const char *text,
 
   if (m_lowest_common_denominator_unique)
     {
-      /* We don't rely on readline appending the quote char as
-	 delimiter as then readline wouldn't append the ' ' after the
-	 completion.  */
-      char buf[2] = { (char) quote_char () };
-
-      match_list[0] = reconcat (match_list[0], match_list[0],
-				buf, (char *) NULL);
-      match_list[1] = NULL;
-
-      /* If the tracker wants to, or we already have a space at the
-	 end of the match, tell readline to skip appending
-	 another.  */
-      char *match = match_list[0];
-      bool completion_suppress_append
-	= (suppress_append_ws ()
-	   || (match[0] != '\0'
-	       && match[strlen (match) - 1] == ' '));
+      bool completion_suppress_append;
+
+      if (from_readline ())
+	{
+	  /* We don't rely on readline appending the quote char as
+	     delimiter as then readline wouldn't append the ' ' after the
+	     completion.  */
+	  char buf[2] = { (char) quote_char (), '\0' };
+
+	  match_list[0] = reconcat (match_list[0], match_list[0], buf,
+				    (char *) nullptr);
+
+	  /* If the tracker wants to, or we already have a space at the end
+	     of the match, tell readline to skip appending another.  */
+	  char *match = match_list[0];
+	  completion_suppress_append
+	    = (suppress_append_ws ()
+	       || (match[0] != '\0'
+		   && match[strlen (match) - 1] == ' '));
+	}
+      else
+	completion_suppress_append = false;
+
+      match_list[1] = nullptr;
 
       return completion_result (match_list, 1, completion_suppress_append);
     }
@@ -2465,21 +2472,14 @@ void
 completion_result::print_matches (const std::string &prefix,
 				  const char *word, int quote_char)
 {
-  if (this->number_matches == 1)
-    printf_unfiltered ("%s%s\n", prefix.c_str (), this->match_list[0]);
-  else
-    {
-      this->sort_match_list ();
+  this->sort_match_list ();
 
-      for (size_t i = 0; i < this->number_matches; i++)
-	{
-	  printf_unfiltered ("%s%s", prefix.c_str (),
-			     this->match_list[i + 1]);
-	  if (quote_char)
-	    printf_unfiltered ("%c", quote_char);
-	  printf_unfiltered ("\n");
-	}
-    }
+  char buf[2] = { (char) quote_char, '\0' };
+  size_t off = this->number_matches == 1 ? 0 : 1;
+
+  for (size_t i = 0; i < this->number_matches; i++)
+    printf_unfiltered ("%s%s%s\n", prefix.c_str (),
+		       this->match_list[i + off], buf);
 
   if (this->number_matches == max_completions)
     {
diff --git a/gdb/testsuite/gdb.base/filename-completion.exp b/gdb/testsuite/gdb.base/filename-completion.exp
index 275140ffd9d..3e3b9b29ba4 100644
--- a/gdb/testsuite/gdb.base/filename-completion.exp
+++ b/gdb/testsuite/gdb.base/filename-completion.exp
@@ -82,8 +82,25 @@ proc run_quoting_and_escaping_tests { root } {
 	    test_gdb_complete_none "$cmd ${qc}${root}/xx" \
 		"expand a non-existent filename"
 
-	    test_gdb_complete_unique "$cmd ${qc}${root}/a" \
-		"$cmd ${qc}${root}/aaa/" "" false \
+	    # The following test is split into separate cmd and tab calls
+	    # so we can xfail the cmd version.  The cmd version will add a
+	    # closing quote, it shouldn't be doing this.  This will be
+	    # fixed in a later commit.
+	    if { $qc ne "" } {
+		setup_xfail "*-*-*"
+	    }
+	    test_gdb_complete_cmd_unique "file ${qc}${root}/a" \
+		"file ${qc}${root}/aaa/" \
+		"expand a unique directory name"
+
+	    if { [readline_is_used] } {
+		test_gdb_complete_tab_unique "file ${qc}${root}/a" \
+		    "file ${qc}${root}/aaa/" "" \
+		    "expand a unique directory name"
+	    }
+
+	    test_gdb_complete_unique "file ${qc}${root}/cc2" \
+		"file ${qc}${root}/cc2${qc}" " " false \
 		"expand a unique filename"
 
 	    test_gdb_complete_multiple "$cmd ${qc}${root}/" \
-- 
2.25.4

[PATCHv2 6/8] gdb: add match formatter mechanism for 'complete' command output

[Andrew Burgess]

This commit solves a problem that existed prior to the previous
commit, but the previous commit made more common.

When completing a filename with the 'complete' command GDB will always
add a trailing quote character, even if the completion is a directory
name, in which case it would be better if the trailing quote was not
added.  Consider:

  (gdb) complete file '/tmp/xx
  file '/tmp/xxx/'

The completion offered here is really only a partial completion, we've
completed up to the end of the next directory name, but, until we have
a filename then the completion is not finished and the trailing quote
should not be added.

This would match the readline behaviour, e.g.:

  (gdb) file '/tmp/xx<TAB>
  (gdb) file '/tmp/xxx/

In this case readline completes the directory name, but doesn't add
the trailing quote character.

Remember that the 'complete' command is intended for tools like
e.g. emacs in order that they can emulate GDB's standard readline
completion when implementing a CLI of their own.  As such, not adding
the trailing quote in this case matches the readline behaviour, and
seems like the right way to go.

To achieve this, I've added a new function pointer member variable
completion_result::m_match_formatter.  This contains a pointer to a
callback function which is used by the 'complete' command to format
each result.

The default behaviour of this callback function is to just append the
quote character (the character from before the completion string) to
the end of the completion result.  This matches the current behaviour.

However, for filename completion we override the default value of
m_match_formatter, this new function checks if the completion result
is a directory or not.  If the completion result is a directory then
the closing quote is not added, instead we add a trailing '/'
character.

The code to add a trailing '/' character already exists within the
filename_completer function.  This is no longer needed in this
location, instead this code is moved into the formatter callback.

Tests are updated to handle the changes in functionality, this removes
an xfail added in the previous commit.
---
 gdb/completer.c                               | 92 ++++++++++++++-----
 gdb/completer.h                               | 45 ++++++++-
 .../gdb.base/filename-completion.exp          | 68 ++++++++++----
 3 files changed, 156 insertions(+), 49 deletions(-)

diff --git a/gdb/completer.c b/gdb/completer.c
index 4ba45a35f8f..ee016cdc7f7 100644
--- a/gdb/completer.c
+++ b/gdb/completer.c
@@ -45,6 +45,8 @@
 
 #include "completer.h"
 
+#include <filesystem>
+
 /* Forward declarations.  */
 static const char *completion_find_completion_word (completion_tracker &tracker,
 						    const char *text,
@@ -356,6 +358,28 @@ gdb_completer_file_name_quote (char *text, int match_type ATTRIBUTE_UNUSED,
   return strdup (str.c_str ());
 }
 
+/* The function is used to update the completion word MATCH before
+   displaying it to the user in the 'complete' command output.  This
+   function is only used for formatting filename or directory names.
+
+   This function checks to see if the completion word MATCH is a directory,
+   in which case a trailing "/" (forward-slash) is added, otherwise
+   QUOTE_CHAR is added as a trailing quote.
+
+   Return the updated completion word as a string.  */
+
+static std::string
+filename_match_formatter (const char *match, char quote_char)
+{
+  std::string result (match);
+  if (std::filesystem::is_directory (gdb_tilde_expand (match)))
+    result += "/";
+  else
+    result += quote_char;
+
+  return result;
+}
+
 /* Generate filename completions of WORD, storing the completions into
    TRACKER.  This is used for generating completions for commands that
    only accept unquoted filenames as well as for commands that accept
@@ -365,6 +389,8 @@ static void
 filename_completer_generate_completions (completion_tracker &tracker,
 					 const char *word)
 {
+  tracker.set_match_format_func (filename_match_formatter);
+
   int subsequent_name = 0;
   while (1)
     {
@@ -383,20 +409,6 @@ filename_completer_generate_completions (completion_tracker &tracker,
       if (p[strlen (p) - 1] == '~')
 	continue;
 
-      /* Readline appends a trailing '/' if the completion is a
-	 directory.  If this completion request originated from outside
-	 readline (e.g. GDB's 'complete' command), then we append the
-	 trailing '/' ourselves now.  */
-      if (!tracker.from_readline ())
-	{
-	  std::string expanded = gdb_tilde_expand (p_rl.get ());
-	  struct stat finfo;
-	  const bool isdir = (stat (expanded.c_str (), &finfo) == 0
-			      && S_ISDIR (finfo.st_mode));
-	  if (isdir)
-	    p_rl.reset (concat (p_rl.get (), "/", nullptr));
-	}
-
       tracker.add_completion
 	(make_completion_match_str (std::move (p_rl), word, word));
     }
@@ -1646,10 +1658,25 @@ int max_completions = 200;
 /* Initial size of the table.  It automagically grows from here.  */
 #define INITIAL_COMPLETION_HTAB_SIZE 200
 
+/* The function is used to update the completion word MATCH before
+   displaying it to the user in the 'complete' command output.  This
+   default function is used in all cases except those where a completion
+   function overrides this function by calling set_match_format_func.
+
+   This function returns MATCH with QUOTE_CHAR appended.  If QUOTE_CHAR is
+   the null-character then the returned string will just contain MATCH.  */
+
+static std::string
+default_match_formatter (const char *match, char quote_char)
+{
+  return std::string (match) + quote_char;
+}
+
 /* See completer.h.  */
 
 completion_tracker::completion_tracker (bool from_readline)
-  : m_from_readline (from_readline)
+  : m_from_readline (from_readline),
+    m_match_format_func (default_match_formatter)
 {
   discard_completions ();
 }
@@ -2352,7 +2379,8 @@ completion_tracker::build_completion_result (const char *text,
 
       match_list[1] = nullptr;
 
-      return completion_result (match_list, 1, completion_suppress_append);
+      return completion_result (match_list, 1, completion_suppress_append,
+				m_match_format_func);
     }
   else
     {
@@ -2389,7 +2417,8 @@ completion_tracker::build_completion_result (const char *text,
       htab_traverse_noresize (m_entries_hash.get (), func, &builder);
       match_list[builder.index] = NULL;
 
-      return completion_result (match_list, builder.index - 1, false);
+      return completion_result (match_list, builder.index - 1, false,
+				m_match_format_func);
     }
 }
 
@@ -2397,18 +2426,23 @@ completion_tracker::build_completion_result (const char *text,
 
 completion_result::completion_result ()
   : match_list (NULL), number_matches (0),
-    completion_suppress_append (false)
+    completion_suppress_append (false),
+    m_match_formatter (default_match_formatter)
 {}
 
 /* See completer.h  */
 
 completion_result::completion_result (char **match_list_,
 				      size_t number_matches_,
-				      bool completion_suppress_append_)
+				      bool completion_suppress_append_,
+				      match_format_func_t match_formatter_)
   : match_list (match_list_),
     number_matches (number_matches_),
-    completion_suppress_append (completion_suppress_append_)
-{}
+    completion_suppress_append (completion_suppress_append_),
+    m_match_formatter (match_formatter_)
+{
+  gdb_assert (m_match_formatter != nullptr);
+}
 
 /* See completer.h  */
 
@@ -2421,10 +2455,12 @@ completion_result::~completion_result ()
 
 completion_result::completion_result (completion_result &&rhs) noexcept
   : match_list (rhs.match_list),
-    number_matches (rhs.number_matches)
+    number_matches (rhs.number_matches),
+    m_match_formatter (rhs.m_match_formatter)
 {
   rhs.match_list = NULL;
   rhs.number_matches = 0;
+  rhs.m_match_formatter = default_match_formatter;
 }
 
 /* See completer.h  */
@@ -2474,12 +2510,18 @@ completion_result::print_matches (const std::string &prefix,
 {
   this->sort_match_list ();
 
-  char buf[2] = { (char) quote_char, '\0' };
   size_t off = this->number_matches == 1 ? 0 : 1;
 
   for (size_t i = 0; i < this->number_matches; i++)
-    printf_unfiltered ("%s%s%s\n", prefix.c_str (),
-		       this->match_list[i + off], buf);
+    {
+      gdb_assert (this->m_match_formatter != nullptr);
+      std::string formatted_match
+	= this->m_match_formatter (this->match_list[i + off],
+				   (char) quote_char);
+
+      printf_unfiltered ("%s%s\n", prefix.c_str (),
+			 formatted_match.c_str ());
+    }
 
   if (this->number_matches == max_completions)
     {
diff --git a/gdb/completer.h b/gdb/completer.h
index 200d8a9b3af..71303e2ae9a 100644
--- a/gdb/completer.h
+++ b/gdb/completer.h
@@ -247,12 +247,24 @@ struct completion_match_result
 
 struct completion_result
 {
+  /* The type of a function that is used to format completion results when
+     using the 'complete' command.  MATCH is the completion word to be
+     printed, and QUOTE_CHAR is a trailing quote character to (possibly)
+     add at the end of MATCH.  QUOTE_CHAR can be the null-character in
+     which case no trailing quote should be added.
+
+     Return the possibly modified completion match word which should be
+     presented to the user.  */
+  using match_format_func_t = std::string (*) (const char *match,
+					       char quote_char);
+
   /* Create an empty result.  */
   completion_result ();
 
   /* Create a result.  */
   completion_result (char **match_list, size_t number_matches,
-		     bool completion_suppress_append);
+		     bool completion_suppress_append,
+		     match_format_func_t match_format_func);
 
   /* Destroy a result.  */
   ~completion_result ();
@@ -274,10 +286,15 @@ struct completion_result
      completions, otherwise, each line of output consists of PREFIX
      followed by one of the possible completion words.
 
-     The QUOTE_CHAR is appended after each possible completion word and
-     should be the quote character that appears before the completion word,
-     or the null-character if there is no quote before the completion
-     word.  */
+     The QUOTE_CHAR is usually appended after each possible completion
+     word and should be the quote character that appears before the
+     completion word, or the null-character if there is no quote before
+     the completion word.
+
+     The QUOTE_CHAR is not always appended to the completion output.  For
+     example, filename completions will not append QUOTE_CHAR if the
+     completion is a directory name.  This is all handled by calling this
+     function.  */
   void print_matches (const std::string &prefix, const char *word,
 		      int quote_char);
 
@@ -305,6 +322,12 @@ struct completion_result
   /* Whether readline should suppress appending a whitespace, when
      there's only one possible completion.  */
   bool completion_suppress_append;
+
+private:
+  /* A function which formats a single completion match ready for display
+     as part of the 'complete' command output.  Different completion
+     functions can set different formatter functions.  */
+  match_format_func_t m_match_formatter;
 };
 
 /* Object used by completers to build a completion match list to hand
@@ -441,6 +464,14 @@ class completion_tracker
   bool from_readline () const
   { return m_from_readline; }
 
+  /* Set the function used to format the completion word before displaying
+     it to the user to F, this is used by the 'complete' command.  */
+  void set_match_format_func (completion_result::match_format_func_t f)
+  {
+    gdb_assert (f != nullptr);
+    m_match_format_func = f;
+  }
+
 private:
 
   /* The type that we place into the m_entries_hash hash table.  */
@@ -535,6 +566,10 @@ class completion_tracker
      interactively. The 'complete' command is a way to generate completions
      not to be displayed by readline.  */
   bool m_from_readline;
+
+  /* The function used to format the completion word before it is printed
+     in the 'complete' command output.  */
+  completion_result::match_format_func_t m_match_format_func;
 };
 
 /* Return a string to hand off to readline as a completion match
diff --git a/gdb/testsuite/gdb.base/filename-completion.exp b/gdb/testsuite/gdb.base/filename-completion.exp
index 3e3b9b29ba4..0467d5c425e 100644
--- a/gdb/testsuite/gdb.base/filename-completion.exp
+++ b/gdb/testsuite/gdb.base/filename-completion.exp
@@ -58,6 +58,49 @@ proc setup_directory_tree {} {
     return $root
 }
 
+# This proc started as a copy of test_gdb_complete_multiple, however, this
+# version does some extra work.  See the original test_gdb_complete_multiple
+# for a description of all the arguments.
+#
+# When using the 'complete' command with filenames, GDB will add a trailing
+# quote for filenames, and a trailing "/" for directory names.  As the
+# trailing "/" is also added in the tab-completion output the
+# COMPLETION_LIST will include the "/" character, but the trailing quote is
+# only added when using the 'complete' command.
+#
+# Pass the trailing quote will be passed as END_QUOTE_CHAR, this proc will
+# run the tab completion test, and will then add the trailing quote to those
+# entries in COMPLETION_LIST that don't have a trailing "/" before running
+# the 'complete' command test.
+proc test_gdb_complete_filename_multiple {
+  cmd_prefix completion_word add_completed_line completion_list
+  {start_quote_char ""} {end_quote_char ""} {max_completions false}
+  {testname ""}
+} {
+    if { [readline_is_used] } {
+	test_gdb_complete_tab_multiple "$cmd_prefix$completion_word" \
+	    $add_completed_line $completion_list $max_completions $testname
+    }
+
+    if { $start_quote_char eq "" && $end_quote_char ne "" } {
+	set updated_completion_list {}
+
+	foreach entry $completion_list {
+	    if {[string range $entry end end] ne "/"} {
+		set entry $entry$end_quote_char
+	    }
+	    lappend updated_completion_list $entry
+	}
+
+	set completion_list $updated_completion_list
+	set end_quote_char ""
+    }
+
+    test_gdb_complete_cmd_multiple $cmd_prefix $completion_word \
+	$completion_list $start_quote_char $end_quote_char $max_completions \
+	$testname
+}
+
 # Run filename completetion tests for those command that accept quoting and
 # escaping of the filename argument.
 #
@@ -82,35 +125,22 @@ proc run_quoting_and_escaping_tests { root } {
 	    test_gdb_complete_none "$cmd ${qc}${root}/xx" \
 		"expand a non-existent filename"
 
-	    # The following test is split into separate cmd and tab calls
-	    # so we can xfail the cmd version.  The cmd version will add a
-	    # closing quote, it shouldn't be doing this.  This will be
-	    # fixed in a later commit.
-	    if { $qc ne "" } {
-		setup_xfail "*-*-*"
-	    }
-	    test_gdb_complete_cmd_unique "file ${qc}${root}/a" \
-		"file ${qc}${root}/aaa/" \
+	    test_gdb_complete_unique "file ${qc}${root}/a" \
+		"file ${qc}${root}/aaa/" "" false \
 		"expand a unique directory name"
 
-	    if { [readline_is_used] } {
-		test_gdb_complete_tab_unique "file ${qc}${root}/a" \
-		    "file ${qc}${root}/aaa/" "" \
-		    "expand a unique directory name"
-	    }
-
 	    test_gdb_complete_unique "file ${qc}${root}/cc2" \
 		"file ${qc}${root}/cc2${qc}" " " false \
 		"expand a unique filename"
 
-	    test_gdb_complete_multiple "$cmd ${qc}${root}/" \
+	    test_gdb_complete_filename_multiple "$cmd ${qc}${root}/" \
 		"b" "b" {
 		    "bb1/"
 		    "bb2/"
 		} "" "${qc}" false \
 		"expand multiple directory names"
 
-	    test_gdb_complete_multiple "$cmd ${qc}${root}/" \
+	    test_gdb_complete_filename_multiple "$cmd ${qc}${root}/" \
 		"c" "c" {
 		    "cc1/"
 		    "cc2"
@@ -122,14 +152,14 @@ proc run_quoting_and_escaping_tests { root } {
 	    if { $qc ne "" } {
 		set sp " "
 
-		test_gdb_complete_multiple "file ${qc}${root}/aaa/" \
+		test_gdb_complete_filename_multiple "file ${qc}${root}/aaa/" \
 		    "a" "a${sp}" {
 			"aa bb"
 			"aa cc"
 		    } "" "${qc}" false \
 		    "expand filenames containing spaces"
 
-		test_gdb_complete_multiple "file ${qc}${root}/bb1/" \
+		test_gdb_complete_filename_multiple "file ${qc}${root}/bb1/" \
 		    "a" "a" {
 			"aa\"bb"
 			"aa'bb"
-- 
2.25.4

[PATCHv2 7/8] gdb: apply escaping to filenames in 'complete' results

[Andrew Burgess]

Building on the mechanism added in the previous commit(s), this commit
applies escaping to filenames in the 'complete' command output.
Consider a file: /tmp/xxx/aa"bb -- that is a filename that contains a
double quote, currently the 'complete' command output looks like this:

  (gdb) complete file /tmp/xxx/a
  file /tmp/xxx/aa"bb

Notice that the double quote in the output is not escaped.  If we
passed this same output back to GDB then the double quote will be
treated as the start of a string.

After this commit then the output looks like this:

  (gdb) complete file /tmp/xxx/a
  file /tmp/xxx/aa\"bb

The double quote is now escaped.  If we feed this output back to GDB
then GDB will treat this as a single filename that contains a double
quote, exactly what we want.

To achieve this I've done a little refactoring, splitting out the core
of gdb_completer_file_name_quote, and then added a new call from the
filename_match_formatter function.

There are updates to the tests to cover this new functionality.
---
 gdb/completer.c                               |  98 ++++++++++++++---
 .../gdb.base/filename-completion.exp          | 100 +++++++++++-------
 2 files changed, 144 insertions(+), 54 deletions(-)

diff --git a/gdb/completer.c b/gdb/completer.c
index ee016cdc7f7..9c14e6a10ce 100644
--- a/gdb/completer.c
+++ b/gdb/completer.c
@@ -309,25 +309,24 @@ gdb_completer_file_name_dequote (char *filename, int quote_char)
   return strdup (tmp.c_str ());
 }
 
-/* Apply character escaping to the file name in TEXT.  QUOTE_PTR points to
-   the quote character surrounding TEXT, or points to the null-character if
-   there are no quotes around TEXT.  MATCH_TYPE will be one of the readline
-   constants SINGLE_MATCH or MULTI_MATCH depending on if there is one or
-   many completions.  */
+/* Apply character escaping to the filename in TEXT and return a newly
+   allocated buffer containing the possibly updated filename.
+
+   QUOTE_CHAR is the quote character surrounding TEXT, or the
+   null-character if there are no quotes around TEXT.  */
 
 static char *
-gdb_completer_file_name_quote (char *text, int match_type ATTRIBUTE_UNUSED,
-			       char *quote_ptr)
+gdb_completer_file_name_quote_1 (const char *text, char quote_char)
 {
   std::string str;
 
-  if (*quote_ptr == '\'')
+  if (quote_char == '\'')
     {
       /* There is no backslash escaping permitted within a single quoted
 	 string, so in this case we can just return the input sting.  */
       str = text;
     }
-  else if (*quote_ptr == '"')
+  else if (quote_char == '"')
     {
       /* Add escaping for a double quoted filename.  */
       for (const char *input = text;
@@ -341,7 +340,7 @@ gdb_completer_file_name_quote (char *text, int match_type ATTRIBUTE_UNUSED,
     }
   else
     {
-      gdb_assert (*quote_ptr == '\0');
+      gdb_assert (quote_char == '\0');
 
       /* Add escaping for an unquoted filename.  */
       for (const char *input = text;
@@ -358,6 +357,19 @@ gdb_completer_file_name_quote (char *text, int match_type ATTRIBUTE_UNUSED,
   return strdup (str.c_str ());
 }
 
+/* Apply character escaping to the filename in TEXT.  QUOTE_PTR points to
+   the quote character surrounding TEXT, or points to the null-character if
+   there are no quotes around TEXT.  MATCH_TYPE will be one of the readline
+   constants SINGLE_MATCH or MULTI_MATCH depending on if there is one or
+   many completions.  */
+
+static char *
+gdb_completer_file_name_quote (char *text, int match_type ATTRIBUTE_UNUSED,
+			       char *quote_ptr)
+{
+  return gdb_completer_file_name_quote_1 (text, *quote_ptr);
+}
+
 /* The function is used to update the completion word MATCH before
    displaying it to the user in the 'complete' command output.  This
    function is only used for formatting filename or directory names.
@@ -366,12 +378,28 @@ gdb_completer_file_name_quote (char *text, int match_type ATTRIBUTE_UNUSED,
    in which case a trailing "/" (forward-slash) is added, otherwise
    QUOTE_CHAR is added as a trailing quote.
 
+   When ADD_ESCAPES is true any special characters (e.g. whitespace,
+   quotes) will be escaped with a backslash.  See
+   gdb_completer_file_name_quote_1 for full details on escaping.  When
+   ADD_ESCAPES is false then no escaping will be added and MATCH (with the
+   correct trailing character) will be used unmodified.
+
    Return the updated completion word as a string.  */
 
 static std::string
-filename_match_formatter (const char *match, char quote_char)
+filename_match_formatter_1 (const char *match, char quote_char,
+			    bool add_escapes)
 {
-  std::string result (match);
+  std::string result;
+  if (add_escapes)
+    {
+      gdb::unique_xmalloc_ptr<char> quoted_match
+	(gdb_completer_file_name_quote_1 (match, quote_char));
+      result = quoted_match.get ();
+    }
+  else
+    result = match;
+
   if (std::filesystem::is_directory (gdb_tilde_expand (match)))
     result += "/";
   else
@@ -380,16 +408,52 @@ filename_match_formatter (const char *match, char quote_char)
   return result;
 }
 
+/* The formatting function used to format the results of a 'complete'
+   command when the result is a filename, but the filename should not have
+   any escape characters added.  Most commands that accept a filename don't
+   expect the filename to be quoted or to contain escape characters.
+
+   See filename_match_formatter_1 for more argument details.  */
+
+static std::string
+filename_unquoted_match_formatter (const char *match, char quote_char)
+{
+  return filename_match_formatter_1 (match, quote_char, false);
+}
+
+/* The formatting function used to format the results of a 'complete'
+   command when the result is a filename, and the filename should have any
+   special character (e.g. whitespace, quotes) within it escaped with a
+   backslash.  A limited number of commands accept this style of filename
+   argument.
+
+   See filename_match_formatter_1 for more argument details.  */
+
+static std::string
+filename_maybe_quoted_match_formatter (const char *match, char quote_char)
+{
+  return filename_match_formatter_1 (match, quote_char, true);
+}
+
 /* Generate filename completions of WORD, storing the completions into
    TRACKER.  This is used for generating completions for commands that
    only accept unquoted filenames as well as for commands that accept
-   quoted and escaped filenames.  */
+   quoted and escaped filenames.
+
+   When QUOTE_MATCHES is true TRACKER will be given a match formatter
+   function which will add escape characters (if needed) in the results.
+   When QUOTE_MATCHES is false the match formatter provided will not add
+   any escaping to the results.  */
 
 static void
 filename_completer_generate_completions (completion_tracker &tracker,
-					 const char *word)
+					 const char *word,
+					 bool quote_matches)
 {
-  tracker.set_match_format_func (filename_match_formatter);
+  if (quote_matches)
+    tracker.set_match_format_func (filename_maybe_quoted_match_formatter);
+  else
+    tracker.set_match_format_func (filename_unquoted_match_formatter);
 
   int subsequent_name = 0;
   while (1)
@@ -423,7 +487,7 @@ filename_maybe_quoted_completer (struct cmd_list_element *ignore,
 {
   rl_char_is_quoted_p = gdb_completer_file_name_char_is_quoted;
   rl_completer_quote_characters = gdb_completer_file_name_quote_characters;
-  filename_completer_generate_completions (tracker, word);
+  filename_completer_generate_completions (tracker, word, true);
 }
 
 /* The corresponding completer_handle_brkchars implementation.  */
@@ -449,7 +513,7 @@ filename_completer_handle_brkchars
 {
   gdb_assert (word == nullptr);
   tracker.set_use_custom_word_point (true);
-  filename_completer_generate_completions (tracker, text);
+  filename_completer_generate_completions (tracker, text, false);
 }
 
 /* Find the bounds of the current word for completion purposes, and
diff --git a/gdb/testsuite/gdb.base/filename-completion.exp b/gdb/testsuite/gdb.base/filename-completion.exp
index 0467d5c425e..3ded82431c8 100644
--- a/gdb/testsuite/gdb.base/filename-completion.exp
+++ b/gdb/testsuite/gdb.base/filename-completion.exp
@@ -82,10 +82,22 @@ proc test_gdb_complete_filename_multiple {
 	    $add_completed_line $completion_list $max_completions $testname
     }
 
-    if { $start_quote_char eq "" && $end_quote_char ne "" } {
+    if { $start_quote_char eq "" } {
 	set updated_completion_list {}
 
 	foreach entry $completion_list {
+	    # If ENTRY is quoted with double quotes, then any double
+	    # quotes within the entry need to be escaped.
+	    if { $end_quote_char eq "\"" } {
+		regsub -all "\"" $entry "\\\"" entry
+	    }
+
+	    if { $end_quote_char eq "" } {
+		regsub -all " " $entry "\\ " entry
+		regsub -all "\"" $entry "\\\"" entry
+		regsub -all "'" $entry "\\'" entry
+	    }
+
 	    if {[string range $entry end end] ne "/"} {
 		set entry $entry$end_quote_char
 	    }
@@ -147,47 +159,61 @@ proc run_quoting_and_escaping_tests { root } {
 		} "" "${qc}" false \
 		"expand mixed directory and file names"
 
-	    # GDB does not currently escape word break characters
-	    # (e.g. white space) correctly in unquoted filenames.
 	    if { $qc ne "" } {
 		set sp " "
-
-		test_gdb_complete_filename_multiple "file ${qc}${root}/aaa/" \
-		    "a" "a${sp}" {
-			"aa bb"
-			"aa cc"
-		    } "" "${qc}" false \
-		    "expand filenames containing spaces"
-
-		test_gdb_complete_filename_multiple "file ${qc}${root}/bb1/" \
-		    "a" "a" {
-			"aa\"bb"
-			"aa'bb"
-		    } "" "${qc}" false \
-		    "expand filenames containing quotes"
 	    } else {
 		set sp "\\ "
+	    }
+
+	    if { $qc eq "'" } {
+		set dq "\""
+		set dq_re "\""
+	    } else {
+		set dq "\\\""
+		set dq_re "\\\\\""
+	    }
+
+	    test_gdb_complete_filename_multiple "file ${qc}${root}/bb2/" \
+		"d" "ir${sp}" {
+		    "dir 1/"
+		    "dir 2/"
+		} "" "${qc}" false \
+		"expand multiple directory names containing spaces"
 
-		test_gdb_complete_tab_multiple "file ${qc}${root}/aaa/a" \
-		    "a${sp}" {
-			"aa bb"
-			"aa cc"
-		    } false \
-		    "expand filenames containing spaces"
-
-		test_gdb_complete_tab_multiple "file ${qc}${root}/bb1/a" \
-		    "a" {
-			"aa\"bb"
-			"aa'bb"
-		    } false \
-		    "expand filenames containing quotes"
-
-		test_gdb_complete_tab_unique "file ${qc}${root}/bb1/aa\\\"" \
-		    "file ${qc}${root}/bb1/aa\\\\\"bb${qc}" " " \
-		    "expand unique filename containing double quotes"
-
-		test_gdb_complete_tab_unique "file ${qc}${root}/bb1/aa\\'" \
-		    "file ${qc}${root}/bb1/aa\\\\'bb${qc}" " " \
+	    test_gdb_complete_filename_multiple "file ${qc}${root}/aaa/" \
+		"a" "a${sp}" {
+		    "aa bb"
+		    "aa cc"
+		} "" "${qc}" false \
+		"expand filenames containing spaces"
+
+	    test_gdb_complete_filename_multiple "file ${qc}${root}/bb1/" \
+		"a" "a" {
+		    "aa\"bb"
+		    "aa'bb"
+		} "" "${qc}" false \
+		"expand filenames containing quotes"
+
+	    test_gdb_complete_tab_unique "file ${qc}${root}/bb1/aa${dq}" \
+		"file ${qc}${root}/bb1/aa${dq_re}bb${qc}" " " \
+		"expand unique filename containing double quotes"
+
+	    # It is not possible to include a single quote character
+	    # within a single quoted string.  However, GDB does not do
+	    # anything smart if a user tries to do this.  Avoid testing
+	    # this case.  Maybe in the future we'll figure a way to avoid
+	    # this situation.
+	    if { $qc ne "'" } {
+		if { $qc eq "" } {
+		    set sq "\\'"
+		    set sq_re "\\\\'"
+		} else {
+		    set sq "'"
+		    set sq_re "'"
+		}
+
+		test_gdb_complete_tab_unique "file ${qc}${root}/bb1/aa${sq}" \
+		    "file ${qc}${root}/bb1/aa${sq_re}bb${qc}" " " \
 		    "expand unique filename containing single quote"
 	    }
 	}
-- 
2.25.4

[PATCHv2 8/8] gdb: improve gdb_rl_find_completion_word for quoted words

[Andrew Burgess]

The function gdb_rl_find_completion_word is very similar to the
readline function _rl_find_completion_word, but was either an older
version of that function, or was trimmed when copying to remove code
which was considered unnecessary.

We maintain this copy because the _rl_find_completion_word function is
not part of the public readline API, and we need to replicate the
functionality of that function as part of the 'complete' command.

Within gdb_rl_find_completion_word when looking for the completion
word, if we don't find a unclosed quoted string (which would become
the completion word) then we scan backwards looking for a word break
character.  For example, given:

  (gdb) complete file /tmp/foo

There is no unclosed quoted string so we end up scanning backwards
from the end looking for a word break character.  In this case the
space after 'file' and before '/tmp/foo' is found, so '/tmp/foo'
becomes the completion word.

However, given this:

  (gdb) complete file /tmp/foo\"

There is still no unclosed quoted string, however, when we can
backwards the '"' (double quotes) are treated as a word break
character, and so we end up using the empty string as the completion
word.

The readline function _rl_find_completion_word avoids this mistake by
using the rl_char_is_quoted_p hook.  This function will return true
for the double quote character as it is preceded by a backslash.  An
earlier commit in this series supplied a rl_char_is_quoted_p function
for the filename completion case, however, gdb_rl_find_completion_word
doesn't call rl_char_is_quoted_p so this doesn't help for the
'complete' case.

In this commit I've copied the code to call rl_char_is_quoted_p from
_rl_find_completion_word into gdb_rl_find_completion_word.

This half solves the problem.  In the case:

  (gdb) complete file /tmp/foo\"

We do now try to complete on the string '/tmp/foo\"', however, when we
reach filename_completer we call back into readline to actually
perform filename completion.  However, at this point the WORD variable
points to a string that still contains the backslash.  The backslash
isn't part of the actual filename, that's just an escape character.

Our expectation is that readline will remove the backslash when
looking for matching filenames.  However, readline contains an
optimisation to avoid unnecessary work trying to remove escape
characters.

The readline variable rl_completion_found_quote is set in the readline
function gen_completion_matches before the generation of completion
matches.  This variable is set to true (non-zero) if there is (or
might be) escape characters within the completion word.

The function rl_filename_completion_function, which generates the
filename matches, only removes escape characters when
rl_completion_found_quote is true.  When GDB generates completions
through readline (e.g. tab completion) then rl_completion_found_quote
is set correctly.

But when we use the 'complete' command we don't pass through readline,
and so gen_completion_matches is never called and
rl_completion_found_quote is not set.  In this case when we call
rl_filename_completion_function readline doesn't remove the escapes
from the completion word, and so in our case above, readline looks for
completions of the exact filename '/tmp/foo\"', that is, the filename
including the backslash.

To work around this problem I've added a new flag to our function
gdb_rl_find_completion_word which is set true when we find any quoting
or escaping.  This matches what readline does.

Then in the 'complete' function we can set rl_completion_found_quote
prior to generating completion matches.

With this done the 'complete' command now works correctly when trying
to complete filenames that contain escaped word break characters.  The
tests have been updated accordingly.
---
 gdb/completer.c                               | 60 +++++++++++++++----
 .../gdb.base/filename-completion.exp          | 12 ++--
 2 files changed, 54 insertions(+), 18 deletions(-)

diff --git a/gdb/completer.c b/gdb/completer.c
index 9c14e6a10ce..6a3428e2bbd 100644
--- a/gdb/completer.c
+++ b/gdb/completer.c
@@ -50,7 +50,8 @@
 /* Forward declarations.  */
 static const char *completion_find_completion_word (completion_tracker &tracker,
 						    const char *text,
-						    int *quote_char);
+						    int *quote_char,
+						    bool *found_any_quoting);
 
 static void set_rl_completer_word_break_characters (const char *break_chars);
 
@@ -528,7 +529,9 @@ filename_completer_handle_brkchars
    boundaries of the current word.  QC, if non-null, is set to the
    opening quote character if we found an unclosed quoted substring,
    '\0' otherwise.  DP, if non-null, is set to the value of the
-   delimiter character that caused a word break.  */
+   delimiter character that caused a word break.  FOUND_ANY_QUOTING, if
+   non-null, is set to true if we found any quote characters (single or
+   double quotes, or a backslash) while finding the completion word.  */
 
 struct gdb_rl_completion_word_info
 {
@@ -539,7 +542,7 @@ struct gdb_rl_completion_word_info
 
 static const char *
 gdb_rl_find_completion_word (struct gdb_rl_completion_word_info *info,
-			     int *qc, int *dp,
+			     int *qc, int *dp, bool *found_any_quoting,
 			     const char *line_buffer)
 {
   int scan, end, delimiter, pass_next, isbrk;
@@ -561,6 +564,7 @@ gdb_rl_find_completion_word (struct gdb_rl_completion_word_info *info,
   end = point;
   delimiter = 0;
   quote_char = '\0';
+  bool found_quote = false;
 
   brkchars = info->word_break_characters;
 
@@ -586,6 +590,7 @@ gdb_rl_find_completion_word (struct gdb_rl_completion_word_info *info,
 	  if (quote_char != '\'' && line_buffer[scan] == '\\')
 	    {
 	      pass_next = 1;
+	      found_quote = true;
 	      continue;
 	    }
 
@@ -606,6 +611,7 @@ gdb_rl_find_completion_word (struct gdb_rl_completion_word_info *info,
 	      /* Found start of a quoted substring.  */
 	      quote_char = line_buffer[scan];
 	      point = scan + 1;
+	      found_quote = true;
 	    }
 	}
     }
@@ -619,8 +625,22 @@ gdb_rl_find_completion_word (struct gdb_rl_completion_word_info *info,
 	{
 	  scan = line_buffer[point];
 
-	  if (strchr (brkchars, scan) != 0)
-	    break;
+	  if (strchr (brkchars, scan) == 0)
+	    continue;
+
+	  /* Call the application-specific function to tell us whether
+	     this word break character is quoted and should be skipped.
+	     The const_cast is needed here to comply with the readline
+	     API.  The only function we register for rl_char_is_quoted_p
+	     treats the input buffer as 'const', so we're OK.  */
+	  if (rl_char_is_quoted_p != nullptr && found_quote
+	      && (*rl_char_is_quoted_p) (const_cast<char *> (line_buffer),
+					 point))
+	    continue;
+
+	  /* Convoluted code, but it avoids an n^2 algorithm with calls
+	     to char_is_quoted.  */
+	  break;
 	}
     }
 
@@ -644,6 +664,8 @@ gdb_rl_find_completion_word (struct gdb_rl_completion_word_info *info,
 	}
     }
 
+  if (found_any_quoting != nullptr)
+    *found_any_quoting = found_quote;
   if (qc != NULL)
     *qc = quote_char;
   if (dp != NULL)
@@ -670,7 +692,7 @@ advance_to_completion_word (completion_tracker &tracker,
 
   int delimiter;
   const char *start
-    = gdb_rl_find_completion_word (&info, NULL, &delimiter, text);
+    = gdb_rl_find_completion_word (&info, nullptr, &delimiter, nullptr, text);
 
   tracker.advance_custom_word_point_by (start - text);
 
@@ -732,7 +754,8 @@ complete_nested_command_line (completion_tracker &tracker, const char *text)
 
   int quote_char = '\0';
   const char *word = completion_find_completion_word (tracker, text,
-						      &quote_char);
+						      &quote_char,
+						      nullptr);
 
   if (tracker.use_custom_word_point ())
     {
@@ -1950,8 +1973,11 @@ complete (const char *line, char const **word, int *quote_char)
 
   try
     {
+      bool found_any_quoting = false;
+
       *word = completion_find_completion_word (tracker_handle_brkchars,
-					      line, quote_char);
+					       line, quote_char,
+					       &found_any_quoting);
 
       /* Completers that provide a custom word point in the
 	 handle_brkchars phase also compute their completions then.
@@ -1961,6 +1987,12 @@ complete (const char *line, char const **word, int *quote_char)
 	tracker = &tracker_handle_brkchars;
       else
 	{
+	  /* Setting this global matches what readline does within
+	     gen_completion_matches.  We need this set correctly in case
+	     our completion function calls back into readline to perform
+	     completion (e.g. filename_completer does this).  */
+	  rl_completion_found_quote = found_any_quoting;
+
 	  complete_line (tracker_handle_completions, *word, line, strlen (line));
 	  tracker = &tracker_handle_completions;
 	}
@@ -2235,7 +2267,7 @@ gdb_completion_word_break_characters () noexcept
 
 static const char *
 completion_find_completion_word (completion_tracker &tracker, const char *text,
-				 int *quote_char)
+				 int *quote_char, bool *found_any_quoting)
 {
   size_t point = strlen (text);
 
@@ -2245,6 +2277,13 @@ completion_find_completion_word (completion_tracker &tracker, const char *text,
     {
       gdb_assert (tracker.custom_word_point () > 0);
       *quote_char = tracker.quote_char ();
+      /* This isn't really correct, we're ignoring the case where we found
+	 a backslash escaping a character.  However, this isn't an issue
+	 right now as we only rely on *FOUND_ANY_QUOTING being set when
+	 performing filename completion, which doesn't go through this
+	 path.  */
+      if (found_any_quoting != nullptr)
+	*found_any_quoting = *quote_char != '\0';
       return text + tracker.custom_word_point ();
     }
 
@@ -2254,7 +2293,8 @@ completion_find_completion_word (completion_tracker &tracker, const char *text,
   info.quote_characters = rl_completer_quote_characters;
   info.basic_quote_characters = rl_basic_quote_characters;
 
-  return gdb_rl_find_completion_word (&info, quote_char, NULL, text);
+  return gdb_rl_find_completion_word (&info, quote_char, nullptr,
+				      found_any_quoting, text);
 }
 
 /* See completer.h.  */
diff --git a/gdb/testsuite/gdb.base/filename-completion.exp b/gdb/testsuite/gdb.base/filename-completion.exp
index 3ded82431c8..78f85ace8a1 100644
--- a/gdb/testsuite/gdb.base/filename-completion.exp
+++ b/gdb/testsuite/gdb.base/filename-completion.exp
@@ -167,10 +167,8 @@ proc run_quoting_and_escaping_tests { root } {
 
 	    if { $qc eq "'" } {
 		set dq "\""
-		set dq_re "\""
 	    } else {
 		set dq "\\\""
-		set dq_re "\\\\\""
 	    }
 
 	    test_gdb_complete_filename_multiple "file ${qc}${root}/bb2/" \
@@ -194,8 +192,8 @@ proc run_quoting_and_escaping_tests { root } {
 		} "" "${qc}" false \
 		"expand filenames containing quotes"
 
-	    test_gdb_complete_tab_unique "file ${qc}${root}/bb1/aa${dq}" \
-		"file ${qc}${root}/bb1/aa${dq_re}bb${qc}" " " \
+	    test_gdb_complete_unique "file ${qc}${root}/bb1/aa${dq}" \
+		"file ${qc}${root}/bb1/aa${dq}bb${qc}" " " false \
 		"expand unique filename containing double quotes"
 
 	    # It is not possible to include a single quote character
@@ -206,14 +204,12 @@ proc run_quoting_and_escaping_tests { root } {
 	    if { $qc ne "'" } {
 		if { $qc eq "" } {
 		    set sq "\\'"
-		    set sq_re "\\\\'"
 		} else {
 		    set sq "'"
-		    set sq_re "'"
 		}
 
-		test_gdb_complete_tab_unique "file ${qc}${root}/bb1/aa${sq}" \
-		    "file ${qc}${root}/bb1/aa${sq_re}bb${qc}" " " \
+		test_gdb_complete_unique "file ${qc}${root}/bb1/aa${sq}" \
+		    "file ${qc}${root}/bb1/aa${sq}bb${qc}" " " false \
 		    "expand unique filename containing single quote"
 	    }
 	}
-- 
2.25.4