[PATCH 0/5] Cleanup and changes for file name completion

[PATCH 0/5] Cleanup and changes for file name completion

[Andrew Burgess]

I'm currently working on improving file name completion.  I have more
changes that I'm still working on, but I wanted to post what I have so
far.

There's some cleanup in here: patches #1, #4, and #5.

There's a bug fix in patch #2.

And there's some (minor) new functionality in patch #3.

Thanks,
Andrew

---

Andrew Burgess (5):
  gdb: remove skip_quoted and skip_quoted_chars
  gdb: fix bug where quote characters would become nullptr
  gdb: allow double quotes for quoting filenames
  gdb: remove some dead code from completer.c
  gdb: remove special case completion word handling for filenames

 gdb/completer.c                               | 126 ++++--------------
 gdb/completer.h                               |   7 -
 gdb/p-exp.y                                   |  26 +++-
 .../gdb.base/filename-completion.exp          |  66 ++++++---
 gdb/top.c                                     |   1 -
 5 files changed, 98 insertions(+), 128 deletions(-)


base-commit: 1b346e50485ee450e8103e4b1704b43f61bc39f7
-- 
2.25.4

[PATCH 1/5] gdb: remove skip_quoted and skip_quoted_chars

[Andrew Burgess]

The function skip_quoted_chars (completer.c) is only used by
skip_quoted (also completer.c), so could be made static.  The function
skip_quoted just calls directly to skip_quoted_chars but fills in some
default arguments.

The function skip_quoted is only used by the Pascal expression parser,
and is only used in one place.

The skip_quoted_chars function skips a single string; it either looks
for a string between matching quotes, or for a string up to a word
break character.

However, given how the Pascal expression parser calls this function,
we know that the first character will always be a single quote, in
which case skip_quoted_chars will looks for a string between matching
single quotes.

The skip_quoted_chars doesn't do any escaped character handling, it
will just stop at the next single quote character.

In this commit I propose to remove skip_quoted and skip_quoted_chars,
and replace these with a smaller function pascal_skip_string  which
I've placed in p-exp.y.  This new function only skips a string between
matching single quotes, which is exactly the use case that we need.

The benefit of this change is to remove (some) code duplication.  It
feels like skip_quoted is similar in some ways to
extract_string_maybe_quoted, however, there are some differences;
skip_quoted uses the quotes and word break characters from the
completion engine which extract_string_maybe_quoted does not.

However, I'm currently working on improving filename completion, one
part of this is that I'm looking at allowing filenames to be quoted
with single or double quotes, while the default string quoting in
GDB (for expressions) can only use single quotes.  If I do end up
allowing single and double quotes in some cases, but we retain the
single quotes only for expressions then skip_quoted starts to become a
problem, should it accept both quote types, or only one?

But given how skip_quoted is used, I can avoid worrying about this by
simply removing skip_quoted.

The Pascal tests do still pass.  The code that called skip_quoted is
called at least once in the Pascal tests (adding an abort() call
causes gdb.pascal/types.exp to fail), but I doubt the testing is
extensive.  Not sure how widely used GDB for Pascal actually is
though.
---
 gdb/completer.c | 55 -------------------------------------------------
 gdb/completer.h |  5 -----
 gdb/p-exp.y     | 26 ++++++++++++++++++++++-
 3 files changed, 25 insertions(+), 61 deletions(-)

diff --git a/gdb/completer.c b/gdb/completer.c
index 168fab74d14..8168f79de0e 100644
--- a/gdb/completer.c
+++ b/gdb/completer.c
@@ -2371,61 +2371,6 @@ gdb_rl_attempted_completion_function (const char *text, int start, int end)
   return NULL;
 }
 
-/* Skip over the possibly quoted word STR (as defined by the quote
-   characters QUOTECHARS and the word break characters BREAKCHARS).
-   Returns pointer to the location after the "word".  If either
-   QUOTECHARS or BREAKCHARS is NULL, use the same values used by the
-   completer.  */
-
-const char *
-skip_quoted_chars (const char *str, const char *quotechars,
-		   const char *breakchars)
-{
-  char quote_char = '\0';
-  const char *scan;
-
-  if (quotechars == NULL)
-    quotechars = gdb_completer_quote_characters;
-
-  if (breakchars == NULL)
-    breakchars = current_language->word_break_characters ();
-
-  for (scan = str; *scan != '\0'; scan++)
-    {
-      if (quote_char != '\0')
-	{
-	  /* Ignore everything until the matching close quote char.  */
-	  if (*scan == quote_char)
-	    {
-	      /* Found matching close quote.  */
-	      scan++;
-	      break;
-	    }
-	}
-      else if (strchr (quotechars, *scan))
-	{
-	  /* Found start of a quoted string.  */
-	  quote_char = *scan;
-	}
-      else if (strchr (breakchars, *scan))
-	{
-	  break;
-	}
-    }
-
-  return (scan);
-}
-
-/* Skip over the possibly quoted word STR (as defined by the quote
-   characters and word break characters used by the completer).
-   Returns pointer to the location after the "word".  */
-
-const char *
-skip_quoted (const char *str)
-{
-  return skip_quoted_chars (str, NULL, NULL);
-}
-
 /* Return a message indicating that the maximum number of completions
    has been reached and that there may be more.  */
 
diff --git a/gdb/completer.h b/gdb/completer.h
index f0b9e68e120..f604a95011f 100644
--- a/gdb/completer.h
+++ b/gdb/completer.h
@@ -662,11 +662,6 @@ extern void complete_expression (completion_tracker &tracker,
 extern void complete_nested_command_line (completion_tracker &tracker,
 					  const char *text);
 
-extern const char *skip_quoted_chars (const char *, const char *,
-				      const char *);
-
-extern const char *skip_quoted (const char *);
-
 /* Called from command completion function to skip over /FMT
    specifications, allowing the rest of the line to be completed.  Returns
    true if the /FMT is at the end of the current line and there is nothing
diff --git a/gdb/p-exp.y b/gdb/p-exp.y
index 2b5eb6f3026..726166a7a5e 100644
--- a/gdb/p-exp.y
+++ b/gdb/p-exp.y
@@ -76,6 +76,8 @@ static void yyerror (const char *);
 
 static char *uptok (const char *, int);
 
+static const char *pascal_skip_string (const char *str);
+
 using namespace expr;
 %}
 
@@ -1041,6 +1043,28 @@ uptok (const char *tokstart, int namelen)
   return uptokstart;
 }
 
+/* Skip over a Pascal string.  STR must point to the opening single quote
+   character.  This function returns a pointer to the character after the
+   closing single quote character.
+
+   This function does not support embedded, escaped single quotes, which
+   is done by placing two consecutive single quotes into a string.
+   Support for this would be easy to add, but this function is only used
+   from the Python expression parser, and if we did skip over escaped
+   quotes then the rest of the expression parser wouldn't handle them
+   correctly.  */
+static const char *
+pascal_skip_string (const char *str)
+{
+  gdb_assert (*str == '\'');
+
+  do
+    ++str;
+  while (*str != '\0' && *str != '\'');
+
+  return str;
+}
+
 /* Read one token, getting characters through lexptr.  */
 
 static int
@@ -1119,7 +1143,7 @@ yylex (void)
       c = *pstate->lexptr++;
       if (c != '\'')
 	{
-	  namelen = skip_quoted (tokstart) - tokstart;
+	  namelen = pascal_skip_string (tokstart) - tokstart;
 	  if (namelen > 2)
 	    {
 	      pstate->lexptr = tokstart + namelen;
-- 
2.25.4

[PATCH 2/5] gdb: fix bug where quote characters would become nullptr

[Andrew Burgess]

In gdb_completion_word_break_characters_throw, after calling
complete_line_internal, if the completion function chose to use a
custom word point then we set rl_completer_quote_characters to NULL.

However, nowhere do we set rl_completer_quote_characters back to its
default value, which is setup in init_main (top.c).

An example of something that uses a custom word point for its
completion is 'thread apply all ...'.

An example of something that relies on rl_completer_quote_characters
would be completion of a quoted filename that contains white space.

Consider this shell and GDB session.  The <TAB> markers indicate where
I've used tab to trigger completion:

  $ mkdir /tmp/aaa\ bbb
  $ touch /tmp/aaa\ bbb/xx\ 11
  $ touch /tmp/aaa\ bbb/xx\ 22
  $ gdb -q
  (gdb) file '/tmp/aaa bbb/xx<TAB><TAB>
  xx 11  xx 22
  (gdb) thread apply all hel<TAB>
  (gdb) thread apply all help
  (gdb) file '/tmp/aaa bbb/xx<TAB><TAB>

First I create a directory structure which uses white space within
file and directory names.  Then within GDB I use the 'file' command
and use a single quote to quote the filename.  When I tab complete GDB
correctly offers the two files within the directory '/tmp/aaa bbb/'.

This works because rl_completer_quote_characters contains the single
quote, and so readline knows that it is trying to complete the string
that starts after the single quote: /tmp/aaa bbb/xx

Next I invoke the completer for the 'thread apply all' command, to do
this I type 'thread apply all hel' and hit tab, this expands to the
one completion 'thread apply all help'.  We can run this command or
not, it doesn't matter (there are no threads, so we'll get no output).

Now I repeat the original 'file' completion.  This time though I don't
get offered any completions.

The reason is that the 'thread apply all' completer set
rl_completer_quote_characters to nullptr.  Now, when readline tries to
figure out the word to complete it doesn't see the single quote as the
start of a quoted word, so instead readline falls back to the word
break characters, and in this case spots the white space.  As a result
readline tries to complete the string 'bbb/xx' which obviously doesn't
have any completions.

By setting rl_completer_quote_characters each time completion is
invoked this problem is resolved and the second 'file' command
completes as expected.

I've extended gdb.base/filename-completion.exp to also test with
quoted filenames, and added a 'thread apply all' completion at the
start to expose this bug.

As setting of rl_completer_quote_characters is now all done in the
completer.c file the function get_gdb_completer_quote_characters()
could be made static.  However, as this function is only used one time
to initialise rl_completer_quote_characters, I've instead just deleted
get_gdb_completer_quote_characters() and used
gdb_completer_quote_characters directly.
---
 gdb/completer.c                               | 10 +--
 gdb/completer.h                               |  2 -
 .../gdb.base/filename-completion.exp          | 66 +++++++++++++------
 gdb/top.c                                     |  1 -
 4 files changed, 49 insertions(+), 30 deletions(-)

diff --git a/gdb/completer.c b/gdb/completer.c
index 8168f79de0e..44da6548eb4 100644
--- a/gdb/completer.c
+++ b/gdb/completer.c
@@ -181,13 +181,6 @@ static const char gdb_completer_file_name_break_characters[] =
    sequences as strings.  */
 static const char gdb_completer_quote_characters[] = "'";
 \f
-/* Accessor for some completer data that may interest other files.  */
-
-const char *
-get_gdb_completer_quote_characters (void)
-{
-  return gdb_completer_quote_characters;
-}
 
 /* This can be used for functions which don't want to complete on
    symbols but don't want to complete on anything else either.  */
@@ -1269,6 +1262,9 @@ complete_line_internal_1 (completion_tracker &tracker,
   set_rl_completer_word_break_characters
     (current_language->word_break_characters ());
 
+  /* Likewise for the quote characters.  */
+  rl_completer_quote_characters = gdb_completer_quote_characters;
+
   /* Decide whether to complete on a list of gdb commands or on
      symbols.  */
   tmp_command = (char *) alloca (point + 1);
diff --git a/gdb/completer.h b/gdb/completer.h
index f604a95011f..0a95ced8a80 100644
--- a/gdb/completer.h
+++ b/gdb/completer.h
@@ -619,8 +619,6 @@ extern void reggroup_completer (struct cmd_list_element *,
 				completion_tracker &tracker,
 				const char *, const char *);
 
-extern const char *get_gdb_completer_quote_characters (void);
-
 extern char *gdb_completion_word_break_characters (void);
 
 /* Set the word break characters array to BREAK_CHARS.  This function
diff --git a/gdb/testsuite/gdb.base/filename-completion.exp b/gdb/testsuite/gdb.base/filename-completion.exp
index 20ca227f96d..7d8ab1a3350 100644
--- a/gdb/testsuite/gdb.base/filename-completion.exp
+++ b/gdb/testsuite/gdb.base/filename-completion.exp
@@ -37,6 +37,9 @@ proc setup_directory_tree {} {
     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\""
+
     return $root
 }
 
@@ -45,26 +48,49 @@ proc setup_directory_tree {} {
 # 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 } {
-    test_gdb_complete_none "file ${root}/xx" \
-	"expand a non-existent filename"
-
-    test_gdb_complete_unique "file ${root}/a" \
-	"file ${root}/aaa/" "" false \
-	"expand a unique filename"
-
-    test_gdb_complete_multiple "file ${root}/" \
-	"b" "b" {
-	    "bb1/"
-	    "bb2/"
-	} "" "" false \
-	"expand multiple directory names"
-
-    test_gdb_complete_multiple "file ${root}/" \
-	"c" "c" {
-	    "cc1/"
-	    "cc2"
-	} "" "" false \
-	"expand mixed directory and file names"
+
+    # 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" \
+	    "expand a non-existent filename"
+
+	test_gdb_complete_unique "file ${qc}${root}/a" \
+	    "file ${qc}${root}/aaa/" "" false \
+	    "expand a unique filename"
+
+	test_gdb_complete_multiple "file ${qc}${root}/" \
+	    "b" "b" {
+		"bb1/"
+		"bb2/"
+	    } "" "${qc}" false \
+	    "expand multiple directory names"
+
+	test_gdb_complete_multiple "file ${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 "file ${qc}${root}/aaa/" \
+		"a" "a${sp}" {
+		    "aa bb"
+		    "aa cc"
+		} "" "${qc}" false \
+		"expand filenames containing spaces"
+	}
+    }
 }
 
 gdb_start
diff --git a/gdb/top.c b/gdb/top.c
index fb15c719564..543d6cdfbc0 100644
--- a/gdb/top.c
+++ b/gdb/top.c
@@ -2150,7 +2150,6 @@ init_main (void)
   rl_completion_word_break_hook = gdb_completion_word_break_characters;
   rl_attempted_completion_function = gdb_rl_attempted_completion_function;
   set_rl_completer_word_break_characters (default_word_break_characters ());
-  rl_completer_quote_characters = get_gdb_completer_quote_characters ();
   rl_completion_display_matches_hook = cli_display_match_list;
   rl_readline_name = "gdb";
   rl_terminal_name = getenv ("TERM");
-- 
2.25.4

[PATCH 3/5] gdb: allow double quotes for quoting filenames

[Andrew Burgess]

Currently GDB only supports using single quotes for quoting things,
the reason for this, as explained in completer.c (next to the variable
gdb_completer_expression_quote_characters) is that double quoted
strings need to be treated differently by the C expression parser.

But for filenames I don't believe this restriction holds.  The file
names as passed to things like the 'file' command are not passing
through the C expression parser, so it seems like we should be fine to
allow double quotes for quoting in this case.

And so, this commit extends GDB to allow double quotes for quoting
filenames.  Maybe in future we might be able to allow double quote
quoting in additional places, but this seems enough for now.

The testing has been extended to cover double quotes in addition to
the existing single quote testing.

This change does a number of things:

 1. Set rl_completer_quote_characters in filename_completer and
 filename_completer_handle_brkchars, this overrides the default which
 is set in complete_line_internal_1,

 2. In advance_to_completion_word we now take a set of quote
 characters as a parameter, the two callers
 advance_to_expression_complete_word_point and
 advance_to_filename_complete_word_point now pass in the required set
 of quote characters,

 3. In completion_find_completion_word we now use the currently active
 set of quote characters, this means we'll use
 gdb_completer_expression_quote_characters or
 gdb_completer_file_name_quote_characters depending on what type of
 things we are completing.
---
 gdb/completer.c                               | 30 +++++++++++++------
 .../gdb.base/filename-completion.exp          |  2 +-
 2 files changed, 22 insertions(+), 10 deletions(-)

diff --git a/gdb/completer.c b/gdb/completer.c
index 44da6548eb4..198d7893a6f 100644
--- a/gdb/completer.c
+++ b/gdb/completer.c
@@ -179,7 +179,12 @@ static const char gdb_completer_file_name_break_characters[] =
 /* Characters that can be used to quote completion strings.  Note that
    we can't include '"' because the gdb C parser treats such quoted
    sequences as strings.  */
-static const char gdb_completer_quote_characters[] = "'";
+static const char gdb_completer_expression_quote_characters[] = "'";
+
+/* Characters that can be used to quote file names.  We do allow double
+   quotes in this set as file names are now passed through the C
+   expression parser.  */
+static const char gdb_completer_file_name_quote_characters[] = "'\"";
 \f
 
 /* This can be used for functions which don't want to complete on
@@ -199,9 +204,9 @@ filename_completer (struct cmd_list_element *ignore,
 		    completion_tracker &tracker,
 		    const char *text, const char *word)
 {
-  int subsequent_name;
+  rl_completer_quote_characters = gdb_completer_file_name_quote_characters;
 
-  subsequent_name = 0;
+  int subsequent_name = 0;
   while (1)
     {
       gdb::unique_xmalloc_ptr<char> p_rl
@@ -256,6 +261,8 @@ filename_completer_handle_brkchars (struct cmd_list_element *ignore,
 {
   set_rl_completer_word_break_characters
     (gdb_completer_file_name_break_characters);
+
+  rl_completer_quote_characters = gdb_completer_file_name_quote_characters;
 }
 
 /* Find the bounds of the current word for completion purposes, and
@@ -401,12 +408,13 @@ gdb_rl_find_completion_word (struct gdb_rl_completion_word_info *info,
 static const char *
 advance_to_completion_word (completion_tracker &tracker,
 			    const char *word_break_characters,
+			    const char *quote_characters,
 			    const char *text)
 {
   gdb_rl_completion_word_info info;
 
   info.word_break_characters = word_break_characters;
-  info.quote_characters = gdb_completer_quote_characters;
+  info.quote_characters = quote_characters;
   info.basic_quote_characters = rl_basic_quote_characters;
 
   int delimiter;
@@ -431,7 +439,8 @@ advance_to_expression_complete_word_point (completion_tracker &tracker,
 					   const char *text)
 {
   const char *brk_chars = current_language->word_break_characters ();
-  return advance_to_completion_word (tracker, brk_chars, text);
+  const char *quote_chars = gdb_completer_expression_quote_characters;
+  return advance_to_completion_word (tracker, brk_chars, quote_chars, text);
 }
 
 /* See completer.h.  */
@@ -441,7 +450,8 @@ advance_to_filename_complete_word_point (completion_tracker &tracker,
 					 const char *text)
 {
   const char *brk_chars = gdb_completer_file_name_break_characters;
-  return advance_to_completion_word (tracker, brk_chars, text);
+  const char *quote_chars = gdb_completer_file_name_quote_characters;
+  return advance_to_completion_word (tracker, brk_chars, quote_chars, text);
 }
 
 /* See completer.h.  */
@@ -1262,8 +1272,10 @@ complete_line_internal_1 (completion_tracker &tracker,
   set_rl_completer_word_break_characters
     (current_language->word_break_characters ());
 
-  /* Likewise for the quote characters.  */
-  rl_completer_quote_characters = gdb_completer_quote_characters;
+  /* Likewise for the quote characters.  If we later find out that we are
+     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;
 
   /* Decide whether to complete on a list of gdb commands or on
      symbols.  */
@@ -1988,7 +2000,7 @@ completion_find_completion_word (completion_tracker &tracker, const char *text,
   gdb_rl_completion_word_info info;
 
   info.word_break_characters = rl_completer_word_break_characters;
-  info.quote_characters = gdb_completer_quote_characters;
+  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);
diff --git a/gdb/testsuite/gdb.base/filename-completion.exp b/gdb/testsuite/gdb.base/filename-completion.exp
index 7d8ab1a3350..b700977cec5 100644
--- a/gdb/testsuite/gdb.base/filename-completion.exp
+++ b/gdb/testsuite/gdb.base/filename-completion.exp
@@ -56,7 +56,7 @@ proc run_tests { root } {
 	"thread apply all help" " " false \
 	"complete a 'thread apply all' command"
 
-    foreach_with_prefix qc [list "" "'"] {
+    foreach_with_prefix qc [list "" "'" "\""] {
 	test_gdb_complete_none "file ${qc}${root}/xx" \
 	    "expand a non-existent filename"
 
-- 
2.25.4

[PATCH 4/5] gdb: remove some dead code from completer.c

[Andrew Burgess]

In completer.c there is some code that is surrounded with '#if 0',
this code:

  #if 0
    /* There is no way to do this just long enough to affect quote
       inserting without also affecting the next completion.  This
       should be fixed in readline.  FIXME.  */
    /* Ensure that readline does the right thing
       with respect to inserting quotes.  */
    rl_completer_word_break_characters = "";
  #endif

This code, in some form, and always defined out, has been around since
the original import of GDB.  Though the comment hints at what the
problem might be, it's not really clear what the issue is.  And
completion within GDB has moved on a long way since this code was
written ... but not used.

I'm proposing that we just remove this code.

If/when a problem comes up then we can look at how to solve it.  Maybe
this code would be the answer ... but also, I suspect, given all the
changes ... maybe not.  I'm not sure carrying around this code for
another 20+ years adds much value.

There should be no user visible changes after this commit.
---
 gdb/completer.c | 8 --------
 1 file changed, 8 deletions(-)

diff --git a/gdb/completer.c b/gdb/completer.c
index 198d7893a6f..9c89aa43810 100644
--- a/gdb/completer.c
+++ b/gdb/completer.c
@@ -241,14 +241,6 @@ filename_completer (struct cmd_list_element *ignore,
       tracker.add_completion
 	(make_completion_match_str (std::move (p_rl), text, word));
     }
-#if 0
-  /* There is no way to do this just long enough to affect quote
-     inserting without also affecting the next completion.  This
-     should be fixed in readline.  FIXME.  */
-  /* Ensure that readline does the right thing
-     with respect to inserting quotes.  */
-  rl_completer_word_break_characters = "";
-#endif
 }
 
 /* The corresponding completer_handle_brkchars
-- 
2.25.4

[PATCH 5/5] gdb: remove special case completion word handling for filenames

[Andrew Burgess]

Eli,

CC-ing you directly as the work I'm touching was originally done by
you, see:

  https://sourceware.org/pipermail/gdb-patches/2001-February/007313.html

The original email mentions 4 cases:

  - completion on a file name in a list of file names didn't work;
  - GDB would not always append a slash if the completion is a directory;
  - completion failed when the file name had non-file-name characters,
    such as redirection, around it;
  - on DOS/Windows, completion would fail with files with a drive letter.

I believe I've tested the first 3 of these and they all seem to work
fine still, but I don't have a working Windows machine on which I can
test case #4.  I just wanted to bring this change to your attention in
case you wanted to build/test this and check that completion around
drive letters was still working as expected.

---

This commit removes some code which is special casing the filename
completion logic.  The code in question relates to finding the
beginning of the completion word and was first introduced, or modified
into its existing form in commit 7830cf6fb9571c3357b1a0 (from 2001).

The code being removed moved the start of the completion word backward
until a character in gdb_completer_file_name_break_characters was
found, or until we reached the end of the actual command.

However, I doubt that this is needed any more.  The filename completer
has a corresponding filename_completer_handle_brkchars function which
provides gdb_completer_file_name_break_characters as the word break
characters to readline, and also sets rl_completer_quote_characters.
As such, I would expect readline to be able to correctly find the
start of the completion word.

There is one change which I've needed to make as a consequence of
removing the above code, and I think this is a bug fix.

In complete_line_internal_normal_command we initialised temporary
variable P to the CMD_ARGS; this is the complete text after the
command name.  Meanwhile, complete_line_internal_normal_command also
accepts an argument WORD, which is the completion word that readline
found for us.

In the code I removed P was updated, it was first set to WORD, and
then moved backwards to the "new" start of the completion word.

But notice, the default for P is the complete command argument text,
and only if we are performing filename completion do we modify P to be
the completion word.

We then passed P through to the actual commands completion function.

If we are doing anything other than filename completion then the value
of P passed is the complete argument text.

If we are doing filename completion then the value of P passed is the
completion word.

Thus in filename_completer we get two arguments TEXT and WORD, the
TEXT argument gets the value of P which is the "new" completion word,
while WORD is the completion word that readline calculated.

However, after I removed the special case in
complete_line_internal_normal_command, the temporary P is removed, we
now always pass through the complete argument text where P was
previous used.

As such, filename_completer now receives TEXT as the complete argument
text, and WORD as the readline calculated completion word.

Previously in filename_completer we actually tried to generate
completions based on TEXT, which due to the special case, was the
completion word.  But after my change this is no longer the case.  So
I've updated filename_completer to generate completions based on WORD,
the readline calculated completion word.

If I'm correct, then I don't expect to see any user visible changes
after this commit.
---
 gdb/completer.c | 25 ++++---------------------
 1 file changed, 4 insertions(+), 21 deletions(-)

diff --git a/gdb/completer.c b/gdb/completer.c
index 9c89aa43810..b78946d66dd 100644
--- a/gdb/completer.c
+++ b/gdb/completer.c
@@ -210,7 +210,7 @@ filename_completer (struct cmd_list_element *ignore,
   while (1)
     {
       gdb::unique_xmalloc_ptr<char> p_rl
-	(rl_filename_completion_function (text, subsequent_name));
+	(rl_filename_completion_function (word, subsequent_name));
       if (p_rl == NULL)
 	break;
       /* We need to set subsequent_name to a non-zero value before the
@@ -239,7 +239,7 @@ filename_completer (struct cmd_list_element *ignore,
 	}
 
       tracker.add_completion
-	(make_completion_match_str (std::move (p_rl), text, word));
+	(make_completion_match_str (std::move (p_rl), word, word));
     }
 }
 
@@ -1193,23 +1193,6 @@ complete_line_internal_normal_command (completion_tracker &tracker,
 				       complete_line_internal_reason reason,
 				       struct cmd_list_element *c)
 {
-  const char *p = cmd_args;
-
-  if (c->completer == filename_completer)
-    {
-      /* Many commands which want to complete on file names accept
-	 several file names, as in "run foo bar >>baz".  So we don't
-	 want to complete the entire text after the command, just the
-	 last word.  To this end, we need to find the beginning of the
-	 file name by starting at `word' and going backwards.  */
-      for (p = word;
-	   p > command
-	     && strchr (gdb_completer_file_name_break_characters,
-			p[-1]) == NULL;
-	   p--)
-	;
-    }
-
   if (reason == handle_brkchars)
     {
       completer_handle_brkchars_ftype *brkchars_fn;
@@ -1223,11 +1206,11 @@ complete_line_internal_normal_command (completion_tracker &tracker,
 	       (c->completer));
 	}
 
-      brkchars_fn (c, tracker, p, word);
+      brkchars_fn (c, tracker, cmd_args, word);
     }
 
   if (reason != handle_brkchars && c->completer != NULL)
-    (*c->completer) (c, tracker, p, word);
+    (*c->completer) (c, tracker, cmd_args, word);
 }
 
 /* Internal function used to handle completions.
-- 
2.25.4

Re: [PATCH 5/5] gdb: remove special case completion word handling for filenames

[Eli Zaretskii]

> From: Andrew Burgess <aburgess@redhat.com>
> Cc: Andrew Burgess <aburgess@redhat.com>,
> 	Eli Zaretskii <eliz@gnu.org>
> Date: Tue, 16 Jan 2024 21:24:02 +0000
> 
> Eli,
> 
> CC-ing you directly as the work I'm touching was originally done by
> you, see:
> 
>   https://sourceware.org/pipermail/gdb-patches/2001-February/007313.html
> 
> The original email mentions 4 cases:
> 
>   - completion on a file name in a list of file names didn't work;
>   - GDB would not always append a slash if the completion is a directory;
>   - completion failed when the file name had non-file-name characters,
>     such as redirection, around it;
>   - on DOS/Windows, completion would fail with files with a drive letter.
> 
> I believe I've tested the first 3 of these and they all seem to work
> fine still, but I don't have a working Windows machine on which I can
> test case #4.  I just wanted to bring this change to your attention in
> case you wanted to build/test this and check that completion around
> drive letters was still working as expected.

Thanks.  I won't have enough time for this soon enough, unfortunately.
Maybe someone else could do that?  I've taken the liberty of CC'ing
Hannes, in the hope that he could find some time for this soon.

Re: [PATCH 5/5] gdb: remove special case completion word handling for filenames

[Hannes Domani]

 Am Mittwoch, 17. Januar 2024, 13:09:38 MEZ hat Eli Zaretskii <eliz@gnu.org> Folgendes geschrieben:

> > From: Andrew Burgess <aburgess@redhat.com>
>
> > Cc: Andrew Burgess <aburgess@redhat.com>,
> >     Eli Zaretskii <eliz@gnu.org>
> > Date: Tue, 16 Jan 2024 21:24:02 +0000
> >
> > Eli,
> >
> > CC-ing you directly as the work I'm touching was originally done by
> > you, see:
> >
> >  https://sourceware.org/pipermail/gdb-patches/2001-February/007313.html
> >
> > The original email mentions 4 cases:
> >
> >  - completion on a file name in a list of file names didn't work;
> >  - GDB would not always append a slash if the completion is a directory;
> >  - completion failed when the file name had non-file-name characters,
> >    such as redirection, around it;
> >  - on DOS/Windows, completion would fail with files with a drive letter.
> >
> > I believe I've tested the first 3 of these and they all seem to work
> > fine still, but I don't have a working Windows machine on which I can
> > test case #4.  I just wanted to bring this change to your attention in
> > case you wanted to build/test this and check that completion around
> > drive letters was still working as expected.
>
>
> Thanks.  I won't have enough time for this soon enough, unfortunately.
> Maybe someone else could do that?  I've taken the liberty of CC'ing
> Hannes, in the hope that he could find some time for this soon.


Not sure if this is exactly what you had in mind, but I tested
a few things with the patches applied, and saw no problems.

Absolute path:

(gdb) complete file c:/gdb/build64/gdb-git-python3/g
file c:/gdb/build64/gdb-git-python3/gdb/
file c:/gdb/build64/gdb-git-python3/gdbserver/
file c:/gdb/build64/gdb-git-python3/gdbsupport/
file c:/gdb/build64/gdb-git-python3/gnulib/
(gdb) complete file "c:/gdb/build64/gdb-git-python3/g
file "c:/gdb/build64/gdb-git-python3/gdb/"
file "c:/gdb/build64/gdb-git-python3/gdbserver/"
file "c:/gdb/build64/gdb-git-python3/gdbsupport/"
file "c:/gdb/build64/gdb-git-python3/gnulib/"

Absolute path on the same drive:

(gdb) complete file /gdb/build64/gdb-git-python3/g
file /gdb/build64/gdb-git-python3/gdb/
file /gdb/build64/gdb-git-python3/gdbserver/
file /gdb/build64/gdb-git-python3/gdbsupport/
file /gdb/build64/gdb-git-python3/gnulib/
(gdb) complete file "/gdb/build64/gdb-git-python3/g
file "/gdb/build64/gdb-git-python3/gdb/"
file "/gdb/build64/gdb-git-python3/gdbserver/"
file "/gdb/build64/gdb-git-python3/gdbsupport/"
file "/gdb/build64/gdb-git-python3/gnulib/"

Relative path:

(gdb) pwd
Working directory C:\gdb\build64\gdb-git-python3.
(gdb) complete file g
file gdb/
file gdbserver/
file gdbsupport/
file gnulib/
(gdb) complete file "g
file "gdb/"
file "gdbserver/"
file "gdbsupport/"
file "gnulib/"

And filename-completion.exp has no fails:

Running /c/src/repos/binutils-gdb.git/gdb/testsuite/gdb.base/filename-completion.exp ...

                === gdb Summary ===

# of expected passes            15

Re: [PATCH 5/5] gdb: remove special case completion word handling for filenames

[Eli Zaretskii]

> Date: Wed, 17 Jan 2024 16:29:39 +0000 (UTC)
> From: Hannes Domani <ssbssa@yahoo.de>
> Cc: "gdb-patches@sourceware.org" <gdb-patches@sourceware.org>
> 
>  Am Mittwoch, 17. Januar 2024, 13:09:38 MEZ hat Eli Zaretskii <eliz@gnu.org> Folgendes geschrieben:
> 
> > Thanks.  I won't have enough time for this soon enough, unfortunately.
> > Maybe someone else could do that?  I've taken the liberty of CC'ing
> > Hannes, in the hope that he could find some time for this soon.
> 
> 
> Not sure if this is exactly what you had in mind, but I tested
> a few things with the patches applied, and saw no problems.

Thanks, I think this means file-name completion on Windows is unharmed
by Andrew's changes.

Re: [PATCH 5/5] gdb: remove special case completion word handling for filenames

[Andrew Burgess]

Hannes Domani <ssbssa@yahoo.de> writes:

>  Am Mittwoch, 17. Januar 2024, 13:09:38 MEZ hat Eli Zaretskii <eliz@gnu.org> Folgendes geschrieben:
>
>> > From: Andrew Burgess <aburgess@redhat.com>
>>
>> > Cc: Andrew Burgess <aburgess@redhat.com>,
>> >     Eli Zaretskii <eliz@gnu.org>
>> > Date: Tue, 16 Jan 2024 21:24:02 +0000
>> >
>> > Eli,
>> >
>> > CC-ing you directly as the work I'm touching was originally done by
>> > you, see:
>> >
>> >  https://sourceware.org/pipermail/gdb-patches/2001-February/007313.html
>> >
>> > The original email mentions 4 cases:
>> >
>> >  - completion on a file name in a list of file names didn't work;
>> >  - GDB would not always append a slash if the completion is a directory;
>> >  - completion failed when the file name had non-file-name characters,
>> >    such as redirection, around it;
>> >  - on DOS/Windows, completion would fail with files with a drive letter.
>> >
>> > I believe I've tested the first 3 of these and they all seem to work
>> > fine still, but I don't have a working Windows machine on which I can
>> > test case #4.  I just wanted to bring this change to your attention in
>> > case you wanted to build/test this and check that completion around
>> > drive letters was still working as expected.
>>
>>
>> Thanks.  I won't have enough time for this soon enough, unfortunately.
>> Maybe someone else could do that?  I've taken the liberty of CC'ing
>> Hannes, in the hope that he could find some time for this soon.
>
>
> Not sure if this is exactly what you had in mind, but I tested
> a few things with the patches applied, and saw no problems.

Thanks for doing this.  I didn't think this would have broken anything,
but without a Windows machine to test on I'm always a little nervous
messing with this sort of code.  I feel better knowing that at least
I've not completely broken things :)

Thanks again,
Andrew

>
> Absolute path:
>
> (gdb) complete file c:/gdb/build64/gdb-git-python3/g
> file c:/gdb/build64/gdb-git-python3/gdb/
> file c:/gdb/build64/gdb-git-python3/gdbserver/
> file c:/gdb/build64/gdb-git-python3/gdbsupport/
> file c:/gdb/build64/gdb-git-python3/gnulib/
> (gdb) complete file "c:/gdb/build64/gdb-git-python3/g
> file "c:/gdb/build64/gdb-git-python3/gdb/"
> file "c:/gdb/build64/gdb-git-python3/gdbserver/"
> file "c:/gdb/build64/gdb-git-python3/gdbsupport/"
> file "c:/gdb/build64/gdb-git-python3/gnulib/"
>
> Absolute path on the same drive:
>
> (gdb) complete file /gdb/build64/gdb-git-python3/g
> file /gdb/build64/gdb-git-python3/gdb/
> file /gdb/build64/gdb-git-python3/gdbserver/
> file /gdb/build64/gdb-git-python3/gdbsupport/
> file /gdb/build64/gdb-git-python3/gnulib/
> (gdb) complete file "/gdb/build64/gdb-git-python3/g
> file "/gdb/build64/gdb-git-python3/gdb/"
> file "/gdb/build64/gdb-git-python3/gdbserver/"
> file "/gdb/build64/gdb-git-python3/gdbsupport/"
> file "/gdb/build64/gdb-git-python3/gnulib/"
>
> Relative path:
>
> (gdb) pwd
> Working directory C:\gdb\build64\gdb-git-python3.
> (gdb) complete file g
> file gdb/
> file gdbserver/
> file gdbsupport/
> file gnulib/
> (gdb) complete file "g
> file "gdb/"
> file "gdbserver/"
> file "gdbsupport/"
> file "gnulib/"
>
> And filename-completion.exp has no fails:
>
> Running /c/src/repos/binutils-gdb.git/gdb/testsuite/gdb.base/filename-completion.exp ...
>
>                 === gdb Summary ===
>
> # of expected passes            15

[PATCHv2 0/7] Cleanup and changes for file name completion

[Andrew Burgess]

In v2:

  + Rebased onto current upstream/master and retested,

  + Added two additional clean up patches #6 and #7.

---

Andrew Burgess (7):
  gdb: remove skip_quoted and skip_quoted_chars
  gdb: fix bug where quote characters would become nullptr
  gdb: allow double quotes for quoting filenames
  gdb: remove some dead code from completer.c
  gdb: remove special case completion word handling for filenames
  gdb/completion: make completion_find_completion_word static
  gdb: move more completion setup into completer.c

 gdb/completer.c                               | 168 +++++++-----------
 gdb/completer.h                               |  29 ---
 gdb/p-exp.y                                   |  26 ++-
 .../gdb.base/filename-completion.exp          |  66 ++++---
 gdb/top.c                                     |   4 -
 5 files changed, 131 insertions(+), 162 deletions(-)


base-commit: f08311ceb1ba4e19eab7070e676416337455a074
-- 
2.25.4

[PATCHv2 1/7] gdb: remove skip_quoted and skip_quoted_chars

[Andrew Burgess]

The function skip_quoted_chars (completer.c) is only used by
skip_quoted (also completer.c), so could be made static.  The function
skip_quoted just calls directly to skip_quoted_chars but fills in some
default arguments.

The function skip_quoted is only used by the Pascal expression parser,
and is only used in one place.

The skip_quoted_chars function skips a single string; it either looks
for a string between matching quotes, or for a string up to a word
break character.

However, given how the Pascal expression parser calls this function,
we know that the first character will always be a single quote, in
which case skip_quoted_chars will looks for a string between matching
single quotes.

The skip_quoted_chars doesn't do any escaped character handling, it
will just stop at the next single quote character.

In this commit I propose to remove skip_quoted and skip_quoted_chars,
and replace these with a smaller function pascal_skip_string  which
I've placed in p-exp.y.  This new function only skips a string between
matching single quotes, which is exactly the use case that we need.

The benefit of this change is to remove (some) code duplication.  It
feels like skip_quoted is similar in some ways to
extract_string_maybe_quoted, however, there are some differences;
skip_quoted uses the quotes and word break characters from the
completion engine which extract_string_maybe_quoted does not.

However, I'm currently working on improving filename completion, one
part of this is that I'm looking at allowing filenames to be quoted
with single or double quotes, while the default string quoting in
GDB (for expressions) can only use single quotes.  If I do end up
allowing single and double quotes in some cases, but we retain the
single quotes only for expressions then skip_quoted starts to become a
problem, should it accept both quote types, or only one?

But given how skip_quoted is used, I can avoid worrying about this by
simply removing skip_quoted.

The Pascal tests do still pass.  The code that called skip_quoted is
called at least once in the Pascal tests (adding an abort() call
causes gdb.pascal/types.exp to fail), but I doubt the testing is
extensive.  Not sure how widely used GDB for Pascal actually is
though.
---
 gdb/completer.c | 55 -------------------------------------------------
 gdb/completer.h |  5 -----
 gdb/p-exp.y     | 26 ++++++++++++++++++++++-
 3 files changed, 25 insertions(+), 61 deletions(-)

diff --git a/gdb/completer.c b/gdb/completer.c
index 168fab74d14..8168f79de0e 100644
--- a/gdb/completer.c
+++ b/gdb/completer.c
@@ -2371,61 +2371,6 @@ gdb_rl_attempted_completion_function (const char *text, int start, int end)
   return NULL;
 }
 
-/* Skip over the possibly quoted word STR (as defined by the quote
-   characters QUOTECHARS and the word break characters BREAKCHARS).
-   Returns pointer to the location after the "word".  If either
-   QUOTECHARS or BREAKCHARS is NULL, use the same values used by the
-   completer.  */
-
-const char *
-skip_quoted_chars (const char *str, const char *quotechars,
-		   const char *breakchars)
-{
-  char quote_char = '\0';
-  const char *scan;
-
-  if (quotechars == NULL)
-    quotechars = gdb_completer_quote_characters;
-
-  if (breakchars == NULL)
-    breakchars = current_language->word_break_characters ();
-
-  for (scan = str; *scan != '\0'; scan++)
-    {
-      if (quote_char != '\0')
-	{
-	  /* Ignore everything until the matching close quote char.  */
-	  if (*scan == quote_char)
-	    {
-	      /* Found matching close quote.  */
-	      scan++;
-	      break;
-	    }
-	}
-      else if (strchr (quotechars, *scan))
-	{
-	  /* Found start of a quoted string.  */
-	  quote_char = *scan;
-	}
-      else if (strchr (breakchars, *scan))
-	{
-	  break;
-	}
-    }
-
-  return (scan);
-}
-
-/* Skip over the possibly quoted word STR (as defined by the quote
-   characters and word break characters used by the completer).
-   Returns pointer to the location after the "word".  */
-
-const char *
-skip_quoted (const char *str)
-{
-  return skip_quoted_chars (str, NULL, NULL);
-}
-
 /* Return a message indicating that the maximum number of completions
    has been reached and that there may be more.  */
 
diff --git a/gdb/completer.h b/gdb/completer.h
index f0b9e68e120..f604a95011f 100644
--- a/gdb/completer.h
+++ b/gdb/completer.h
@@ -662,11 +662,6 @@ extern void complete_expression (completion_tracker &tracker,
 extern void complete_nested_command_line (completion_tracker &tracker,
 					  const char *text);
 
-extern const char *skip_quoted_chars (const char *, const char *,
-				      const char *);
-
-extern const char *skip_quoted (const char *);
-
 /* Called from command completion function to skip over /FMT
    specifications, allowing the rest of the line to be completed.  Returns
    true if the /FMT is at the end of the current line and there is nothing
diff --git a/gdb/p-exp.y b/gdb/p-exp.y
index ea7eb8c8d7e..bfb1cad60a7 100644
--- a/gdb/p-exp.y
+++ b/gdb/p-exp.y
@@ -76,6 +76,8 @@ static void yyerror (const char *);
 
 static char *uptok (const char *, int);
 
+static const char *pascal_skip_string (const char *str);
+
 using namespace expr;
 %}
 
@@ -1042,6 +1044,28 @@ uptok (const char *tokstart, int namelen)
   return uptokstart;
 }
 
+/* Skip over a Pascal string.  STR must point to the opening single quote
+   character.  This function returns a pointer to the character after the
+   closing single quote character.
+
+   This function does not support embedded, escaped single quotes, which
+   is done by placing two consecutive single quotes into a string.
+   Support for this would be easy to add, but this function is only used
+   from the Python expression parser, and if we did skip over escaped
+   quotes then the rest of the expression parser wouldn't handle them
+   correctly.  */
+static const char *
+pascal_skip_string (const char *str)
+{
+  gdb_assert (*str == '\'');
+
+  do
+    ++str;
+  while (*str != '\0' && *str != '\'');
+
+  return str;
+}
+
 /* Read one token, getting characters through lexptr.  */
 
 static int
@@ -1120,7 +1144,7 @@ yylex (void)
       c = *pstate->lexptr++;
       if (c != '\'')
 	{
-	  namelen = skip_quoted (tokstart) - tokstart;
+	  namelen = pascal_skip_string (tokstart) - tokstart;
 	  if (namelen > 2)
 	    {
 	      pstate->lexptr = tokstart + namelen;
-- 
2.25.4

[PATCHv2 2/7] gdb: fix bug where quote characters would become nullptr

[Andrew Burgess]

In gdb_completion_word_break_characters_throw, after calling
complete_line_internal, if the completion function chose to use a
custom word point then we set rl_completer_quote_characters to NULL.

However, nowhere do we set rl_completer_quote_characters back to its
default value, which is setup in init_main (top.c).

An example of something that uses a custom word point for its
completion is 'thread apply all ...'.

An example of something that relies on rl_completer_quote_characters
would be completion of a quoted filename that contains white space.

Consider this shell and GDB session.  The <TAB> markers indicate where
I've used tab to trigger completion:

  $ mkdir /tmp/aaa\ bbb
  $ touch /tmp/aaa\ bbb/xx\ 11
  $ touch /tmp/aaa\ bbb/xx\ 22
  $ gdb -q
  (gdb) file '/tmp/aaa bbb/xx<TAB><TAB>
  xx 11  xx 22
  (gdb) thread apply all hel<TAB>
  (gdb) thread apply all help
  (gdb) file '/tmp/aaa bbb/xx<TAB><TAB>

First I create a directory structure which uses white space within
file and directory names.  Then within GDB I use the 'file' command
and use a single quote to quote the filename.  When I tab complete GDB
correctly offers the two files within the directory '/tmp/aaa bbb/'.

This works because rl_completer_quote_characters contains the single
quote, and so readline knows that it is trying to complete the string
that starts after the single quote: /tmp/aaa bbb/xx

Next I invoke the completer for the 'thread apply all' command, to do
this I type 'thread apply all hel' and hit tab, this expands to the
one completion 'thread apply all help'.  We can run this command or
not, it doesn't matter (there are no threads, so we'll get no output).

Now I repeat the original 'file' completion.  This time though I don't
get offered any completions.

The reason is that the 'thread apply all' completer set
rl_completer_quote_characters to nullptr.  Now, when readline tries to
figure out the word to complete it doesn't see the single quote as the
start of a quoted word, so instead readline falls back to the word
break characters, and in this case spots the white space.  As a result
readline tries to complete the string 'bbb/xx' which obviously doesn't
have any completions.

By setting rl_completer_quote_characters each time completion is
invoked this problem is resolved and the second 'file' command
completes as expected.

I've extended gdb.base/filename-completion.exp to also test with
quoted filenames, and added a 'thread apply all' completion at the
start to expose this bug.

As setting of rl_completer_quote_characters is now all done in the
completer.c file the function get_gdb_completer_quote_characters()
could be made static.  However, as this function is only used one time
to initialise rl_completer_quote_characters, I've instead just deleted
get_gdb_completer_quote_characters() and used
gdb_completer_quote_characters directly.
---
 gdb/completer.c                               | 10 +--
 gdb/completer.h                               |  2 -
 .../gdb.base/filename-completion.exp          | 66 +++++++++++++------
 gdb/top.c                                     |  1 -
 4 files changed, 49 insertions(+), 30 deletions(-)

diff --git a/gdb/completer.c b/gdb/completer.c
index 8168f79de0e..44da6548eb4 100644
--- a/gdb/completer.c
+++ b/gdb/completer.c
@@ -181,13 +181,6 @@ static const char gdb_completer_file_name_break_characters[] =
    sequences as strings.  */
 static const char gdb_completer_quote_characters[] = "'";
 \f
-/* Accessor for some completer data that may interest other files.  */
-
-const char *
-get_gdb_completer_quote_characters (void)
-{
-  return gdb_completer_quote_characters;
-}
 
 /* This can be used for functions which don't want to complete on
    symbols but don't want to complete on anything else either.  */
@@ -1269,6 +1262,9 @@ complete_line_internal_1 (completion_tracker &tracker,
   set_rl_completer_word_break_characters
     (current_language->word_break_characters ());
 
+  /* Likewise for the quote characters.  */
+  rl_completer_quote_characters = gdb_completer_quote_characters;
+
   /* Decide whether to complete on a list of gdb commands or on
      symbols.  */
   tmp_command = (char *) alloca (point + 1);
diff --git a/gdb/completer.h b/gdb/completer.h
index f604a95011f..0a95ced8a80 100644
--- a/gdb/completer.h
+++ b/gdb/completer.h
@@ -619,8 +619,6 @@ extern void reggroup_completer (struct cmd_list_element *,
 				completion_tracker &tracker,
 				const char *, const char *);
 
-extern const char *get_gdb_completer_quote_characters (void);
-
 extern char *gdb_completion_word_break_characters (void);
 
 /* Set the word break characters array to BREAK_CHARS.  This function
diff --git a/gdb/testsuite/gdb.base/filename-completion.exp b/gdb/testsuite/gdb.base/filename-completion.exp
index 20ca227f96d..7d8ab1a3350 100644
--- a/gdb/testsuite/gdb.base/filename-completion.exp
+++ b/gdb/testsuite/gdb.base/filename-completion.exp
@@ -37,6 +37,9 @@ proc setup_directory_tree {} {
     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\""
+
     return $root
 }
 
@@ -45,26 +48,49 @@ proc setup_directory_tree {} {
 # 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 } {
-    test_gdb_complete_none "file ${root}/xx" \
-	"expand a non-existent filename"
-
-    test_gdb_complete_unique "file ${root}/a" \
-	"file ${root}/aaa/" "" false \
-	"expand a unique filename"
-
-    test_gdb_complete_multiple "file ${root}/" \
-	"b" "b" {
-	    "bb1/"
-	    "bb2/"
-	} "" "" false \
-	"expand multiple directory names"
-
-    test_gdb_complete_multiple "file ${root}/" \
-	"c" "c" {
-	    "cc1/"
-	    "cc2"
-	} "" "" false \
-	"expand mixed directory and file names"
+
+    # 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" \
+	    "expand a non-existent filename"
+
+	test_gdb_complete_unique "file ${qc}${root}/a" \
+	    "file ${qc}${root}/aaa/" "" false \
+	    "expand a unique filename"
+
+	test_gdb_complete_multiple "file ${qc}${root}/" \
+	    "b" "b" {
+		"bb1/"
+		"bb2/"
+	    } "" "${qc}" false \
+	    "expand multiple directory names"
+
+	test_gdb_complete_multiple "file ${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 "file ${qc}${root}/aaa/" \
+		"a" "a${sp}" {
+		    "aa bb"
+		    "aa cc"
+		} "" "${qc}" false \
+		"expand filenames containing spaces"
+	}
+    }
 }
 
 gdb_start
diff --git a/gdb/top.c b/gdb/top.c
index cf7d3a913ba..8df684ec8b4 100644
--- a/gdb/top.c
+++ b/gdb/top.c
@@ -2142,7 +2142,6 @@ init_main (void)
   rl_completion_word_break_hook = gdb_completion_word_break_characters;
   rl_attempted_completion_function = gdb_rl_attempted_completion_function;
   set_rl_completer_word_break_characters (default_word_break_characters ());
-  rl_completer_quote_characters = get_gdb_completer_quote_characters ();
   rl_completion_display_matches_hook = cli_display_match_list;
   rl_readline_name = "gdb";
   rl_terminal_name = getenv ("TERM");
-- 
2.25.4

[PATCHv2 3/7] gdb: allow double quotes for quoting filenames

[Andrew Burgess]

Currently GDB only supports using single quotes for quoting things,
the reason for this, as explained in completer.c (next to the variable
gdb_completer_expression_quote_characters) is that double quoted
strings need to be treated differently by the C expression parser.

But for filenames I don't believe this restriction holds.  The file
names as passed to things like the 'file' command are not passing
through the C expression parser, so it seems like we should be fine to
allow double quotes for quoting in this case.

And so, this commit extends GDB to allow double quotes for quoting
filenames.  Maybe in future we might be able to allow double quote
quoting in additional places, but this seems enough for now.

The testing has been extended to cover double quotes in addition to
the existing single quote testing.

This change does a number of things:

 1. Set rl_completer_quote_characters in filename_completer and
 filename_completer_handle_brkchars, this overrides the default which
 is set in complete_line_internal_1,

 2. In advance_to_completion_word we now take a set of quote
 characters as a parameter, the two callers
 advance_to_expression_complete_word_point and
 advance_to_filename_complete_word_point now pass in the required set
 of quote characters,

 3. In completion_find_completion_word we now use the currently active
 set of quote characters, this means we'll use
 gdb_completer_expression_quote_characters or
 gdb_completer_file_name_quote_characters depending on what type of
 things we are completing.
---
 gdb/completer.c                               | 34 +++++++++++++------
 .../gdb.base/filename-completion.exp          |  2 +-
 2 files changed, 24 insertions(+), 12 deletions(-)

diff --git a/gdb/completer.c b/gdb/completer.c
index 44da6548eb4..cefac605a33 100644
--- a/gdb/completer.c
+++ b/gdb/completer.c
@@ -176,10 +176,15 @@ static const char gdb_completer_file_name_break_characters[] =
   " \t\n*|\"';:?><";
 #endif
 
-/* Characters that can be used to quote completion strings.  Note that
-   we can't include '"' because the gdb C parser treats such quoted
+/* Characters that can be used to quote expressions.  Note that we can't
+   include '"' (double quote) because the gdb C parser treats such quoted
    sequences as strings.  */
-static const char gdb_completer_quote_characters[] = "'";
+static const char gdb_completer_expression_quote_characters[] = "'";
+
+/* Characters that can be used to quote file names.  We do allow '"'
+   (double quotes) in this set as file names are not passed through the C
+   expression parser.  */
+static const char gdb_completer_file_name_quote_characters[] = "'\"";
 \f
 
 /* This can be used for functions which don't want to complete on
@@ -199,9 +204,9 @@ filename_completer (struct cmd_list_element *ignore,
 		    completion_tracker &tracker,
 		    const char *text, const char *word)
 {
-  int subsequent_name;
+  rl_completer_quote_characters = gdb_completer_file_name_quote_characters;
 
-  subsequent_name = 0;
+  int subsequent_name = 0;
   while (1)
     {
       gdb::unique_xmalloc_ptr<char> p_rl
@@ -256,6 +261,8 @@ filename_completer_handle_brkchars (struct cmd_list_element *ignore,
 {
   set_rl_completer_word_break_characters
     (gdb_completer_file_name_break_characters);
+
+  rl_completer_quote_characters = gdb_completer_file_name_quote_characters;
 }
 
 /* Find the bounds of the current word for completion purposes, and
@@ -401,12 +408,13 @@ gdb_rl_find_completion_word (struct gdb_rl_completion_word_info *info,
 static const char *
 advance_to_completion_word (completion_tracker &tracker,
 			    const char *word_break_characters,
+			    const char *quote_characters,
 			    const char *text)
 {
   gdb_rl_completion_word_info info;
 
   info.word_break_characters = word_break_characters;
-  info.quote_characters = gdb_completer_quote_characters;
+  info.quote_characters = quote_characters;
   info.basic_quote_characters = rl_basic_quote_characters;
 
   int delimiter;
@@ -431,7 +439,8 @@ advance_to_expression_complete_word_point (completion_tracker &tracker,
 					   const char *text)
 {
   const char *brk_chars = current_language->word_break_characters ();
-  return advance_to_completion_word (tracker, brk_chars, text);
+  const char *quote_chars = gdb_completer_expression_quote_characters;
+  return advance_to_completion_word (tracker, brk_chars, quote_chars, text);
 }
 
 /* See completer.h.  */
@@ -441,7 +450,8 @@ advance_to_filename_complete_word_point (completion_tracker &tracker,
 					 const char *text)
 {
   const char *brk_chars = gdb_completer_file_name_break_characters;
-  return advance_to_completion_word (tracker, brk_chars, text);
+  const char *quote_chars = gdb_completer_file_name_quote_characters;
+  return advance_to_completion_word (tracker, brk_chars, quote_chars, text);
 }
 
 /* See completer.h.  */
@@ -1262,8 +1272,10 @@ complete_line_internal_1 (completion_tracker &tracker,
   set_rl_completer_word_break_characters
     (current_language->word_break_characters ());
 
-  /* Likewise for the quote characters.  */
-  rl_completer_quote_characters = gdb_completer_quote_characters;
+  /* Likewise for the quote characters.  If we later find out that we are
+     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;
 
   /* Decide whether to complete on a list of gdb commands or on
      symbols.  */
@@ -1988,7 +2000,7 @@ completion_find_completion_word (completion_tracker &tracker, const char *text,
   gdb_rl_completion_word_info info;
 
   info.word_break_characters = rl_completer_word_break_characters;
-  info.quote_characters = gdb_completer_quote_characters;
+  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);
diff --git a/gdb/testsuite/gdb.base/filename-completion.exp b/gdb/testsuite/gdb.base/filename-completion.exp
index 7d8ab1a3350..b700977cec5 100644
--- a/gdb/testsuite/gdb.base/filename-completion.exp
+++ b/gdb/testsuite/gdb.base/filename-completion.exp
@@ -56,7 +56,7 @@ proc run_tests { root } {
 	"thread apply all help" " " false \
 	"complete a 'thread apply all' command"
 
-    foreach_with_prefix qc [list "" "'"] {
+    foreach_with_prefix qc [list "" "'" "\""] {
 	test_gdb_complete_none "file ${qc}${root}/xx" \
 	    "expand a non-existent filename"
 
-- 
2.25.4

[PATCHv2 4/7] gdb: remove some dead code from completer.c

[Andrew Burgess]

In completer.c there is some code that is surrounded with '#if 0',
this code:

  #if 0
    /* There is no way to do this just long enough to affect quote
       inserting without also affecting the next completion.  This
       should be fixed in readline.  FIXME.  */
    /* Ensure that readline does the right thing
       with respect to inserting quotes.  */
    rl_completer_word_break_characters = "";
  #endif

This code, in some form, and always defined out, has been around since
the original import of GDB.  Though the comment hints at what the
problem might be, it's not really clear what the issue is.  And
completion within GDB has moved on a long way since this code was
written ... but not used.

I'm proposing that we just remove this code.

If/when a problem comes up then we can look at how to solve it.  Maybe
this code would be the answer ... but also, I suspect, given all the
changes ... maybe not.  I'm not sure carrying around this code for
another 20+ years adds much value.

There should be no user visible changes after this commit.
---
 gdb/completer.c | 8 --------
 1 file changed, 8 deletions(-)

diff --git a/gdb/completer.c b/gdb/completer.c
index cefac605a33..330c39598c5 100644
--- a/gdb/completer.c
+++ b/gdb/completer.c
@@ -241,14 +241,6 @@ filename_completer (struct cmd_list_element *ignore,
       tracker.add_completion
 	(make_completion_match_str (std::move (p_rl), text, word));
     }
-#if 0
-  /* There is no way to do this just long enough to affect quote
-     inserting without also affecting the next completion.  This
-     should be fixed in readline.  FIXME.  */
-  /* Ensure that readline does the right thing
-     with respect to inserting quotes.  */
-  rl_completer_word_break_characters = "";
-#endif
 }
 
 /* The corresponding completer_handle_brkchars
-- 
2.25.4

[PATCHv2 5/7] gdb: remove special case completion word handling for filenames

[Andrew Burgess]

This commit removes some code which is special casing the filename
completion logic.  The code in question relates to finding the
beginning of the completion word and was first introduced, or modified
into its existing form in commit 7830cf6fb9571c3357b1a0 (from 2001).

The code being removed moved the start of the completion word backward
until a character in gdb_completer_file_name_break_characters was
found, or until we reached the end of the actual command.

However, I doubt that this is needed any more.  The filename completer
has a corresponding filename_completer_handle_brkchars function which
provides gdb_completer_file_name_break_characters as the word break
characters to readline, and also sets rl_completer_quote_characters.
As such, I would expect readline to be able to correctly find the
start of the completion word.

There is one change which I've needed to make as a consequence of
removing the above code, and I think this is a bug fix.

In complete_line_internal_normal_command we initialised temporary
variable P to the CMD_ARGS; this is the complete text after the
command name.  Meanwhile, complete_line_internal_normal_command also
accepts an argument WORD, which is the completion word that readline
found for us.

In the code I removed P was updated, it was first set to WORD, and
then moved backwards to the "new" start of the completion word.

But notice, the default for P is the complete command argument text,
and only if we are performing filename completion do we modify P to be
the completion word.

We then passed P through to the actual commands completion function.

If we are doing anything other than filename completion then the value
of P passed is the complete argument text.

If we are doing filename completion then the value of P passed is the
completion word.

Thus in filename_completer we get two arguments TEXT and WORD, the
TEXT argument gets the value of P which is the "new" completion word,
while WORD is the completion word that readline calculated.

However, after I removed the special case in
complete_line_internal_normal_command, the temporary P is removed, we
now always pass through the complete argument text where P was
previous used.

As such, filename_completer now receives TEXT as the complete argument
text, and WORD as the readline calculated completion word.

Previously in filename_completer we actually tried to generate
completions based on TEXT, which due to the special case, was the
completion word.  But after my change this is no longer the case.  So
I've updated filename_completer to generate completions based on WORD,
the readline calculated completion word.

If I'm correct, then I don't expect to see any user visible changes
after this commit.
---
 gdb/completer.c | 25 ++++---------------------
 1 file changed, 4 insertions(+), 21 deletions(-)

diff --git a/gdb/completer.c b/gdb/completer.c
index 330c39598c5..04354120621 100644
--- a/gdb/completer.c
+++ b/gdb/completer.c
@@ -210,7 +210,7 @@ filename_completer (struct cmd_list_element *ignore,
   while (1)
     {
       gdb::unique_xmalloc_ptr<char> p_rl
-	(rl_filename_completion_function (text, subsequent_name));
+	(rl_filename_completion_function (word, subsequent_name));
       if (p_rl == NULL)
 	break;
       /* We need to set subsequent_name to a non-zero value before the
@@ -239,7 +239,7 @@ filename_completer (struct cmd_list_element *ignore,
 	}
 
       tracker.add_completion
-	(make_completion_match_str (std::move (p_rl), text, word));
+	(make_completion_match_str (std::move (p_rl), word, word));
     }
 }
 
@@ -1193,23 +1193,6 @@ complete_line_internal_normal_command (completion_tracker &tracker,
 				       complete_line_internal_reason reason,
 				       struct cmd_list_element *c)
 {
-  const char *p = cmd_args;
-
-  if (c->completer == filename_completer)
-    {
-      /* Many commands which want to complete on file names accept
-	 several file names, as in "run foo bar >>baz".  So we don't
-	 want to complete the entire text after the command, just the
-	 last word.  To this end, we need to find the beginning of the
-	 file name by starting at `word' and going backwards.  */
-      for (p = word;
-	   p > command
-	     && strchr (gdb_completer_file_name_break_characters,
-			p[-1]) == NULL;
-	   p--)
-	;
-    }
-
   if (reason == handle_brkchars)
     {
       completer_handle_brkchars_ftype *brkchars_fn;
@@ -1223,11 +1206,11 @@ complete_line_internal_normal_command (completion_tracker &tracker,
 	       (c->completer));
 	}
 
-      brkchars_fn (c, tracker, p, word);
+      brkchars_fn (c, tracker, cmd_args, word);
     }
 
   if (reason != handle_brkchars && c->completer != NULL)
-    (*c->completer) (c, tracker, p, word);
+    (*c->completer) (c, tracker, cmd_args, word);
 }
 
 /* Internal function used to handle completions.
-- 
2.25.4

[PATCHv2 6/7] gdb/completion: make completion_find_completion_word static

[Andrew Burgess]

I noticed that completion_find_completion_word is only used within
completer.c, so lets make it static.

There should be no user visible changes after this commit.
---
 gdb/completer.c | 14 ++++++++++++--
 gdb/completer.h | 11 -----------
 2 files changed, 12 insertions(+), 13 deletions(-)

diff --git a/gdb/completer.c b/gdb/completer.c
index 04354120621..2fee90503db 100644
--- a/gdb/completer.c
+++ b/gdb/completer.c
@@ -46,6 +46,11 @@
 
 #include "completer.h"
 
+/* Forward declarations.  */
+static const char *completion_find_completion_word (completion_tracker &tracker,
+						    const char *text,
+						    int *quote_char);
+
 /* See completer.h.  */
 
 class completion_tracker::completion_hash_entry
@@ -1955,9 +1960,14 @@ gdb_completion_word_break_characters ()
   return NULL;
 }
 
-/* See completer.h.  */
+/* Find the bounds of the word in TEXT for completion purposes, and return
+   a pointer to the end of the word.  Calls the completion machinery for a
+   handle_brkchars phase (using TRACKER) to figure out the right work break
+   characters for the command in TEXT.  QUOTE_CHAR, if non-null, is set to
+   the opening quote character if we found an unclosed quoted substring,
+   '\0' otherwise.  */
 
-const char *
+static const char *
 completion_find_completion_word (completion_tracker &tracker, const char *text,
 				 int *quote_char)
 {
diff --git a/gdb/completer.h b/gdb/completer.h
index 0a95ced8a80..9011ba778a7 100644
--- a/gdb/completer.h
+++ b/gdb/completer.h
@@ -556,17 +556,6 @@ extern void complete_line (completion_tracker &tracker,
 extern completion_result
   complete (const char *line, char const **word, int *quote_char);
 
-/* Find the bounds of the word in TEXT for completion purposes, and
-   return a pointer to the end of the word.  Calls the completion
-   machinery for a handle_brkchars phase (using TRACKER) to figure out
-   the right work break characters for the command in TEXT.
-   QUOTE_CHAR, if non-null, is set to the opening quote character if
-   we found an unclosed quoted substring, '\0' otherwise.  */
-extern const char *completion_find_completion_word (completion_tracker &tracker,
-						    const char *text,
-						    int *quote_char);
-
-
 /* Assuming TEXT is an expression in the current language, find the
    completion word point for TEXT, emulating the algorithm readline
    uses to find the word point, using the current language's word
-- 
2.25.4

[PATCHv2 7/7] gdb: move more completion setup into completer.c

[Andrew Burgess]

Move more setup of the readline global state relating to tab
completion into completer.c out of top.c.

Lots of the readline setup is done in init_main (top.c).  This commit
moves those bits of initialisation that relate to completion, and
which are only set the one time, into completer.c.  This does mean
that readline initialisation is now done in multiple locations, some
in init_main (top.c) and some in completer.c, but I think this is OK.
The work done in init_main is the general readline setup.

I think making static what can be made static, and having it all in
one file, makes things easier to reason about.  So I'm OK with having
this split initialisation.

The only completion related thing which is still setup in top.c is
rl_completion_display_matches_hook.  I've left this where it is for
now as rl_completion_display_matches_hook is also updated in the tui
code, and the display hook functions are not in completer.c anyway, so
moving this initialisation to completer.c would not allow anything
else to be made static.

There should be no user visible changes after this commit.
---
 gdb/completer.c | 24 +++++++++++++++++++-----
 gdb/completer.h | 11 -----------
 gdb/top.c       |  3 ---
 3 files changed, 19 insertions(+), 19 deletions(-)

diff --git a/gdb/completer.c b/gdb/completer.c
index 2fee90503db..8e34e30f46b 100644
--- a/gdb/completer.c
+++ b/gdb/completer.c
@@ -51,6 +51,8 @@ static const char *completion_find_completion_word (completion_tracker &tracker,
 						    const char *text,
 						    int *quote_char);
 
+static void set_rl_completer_word_break_characters (const char *break_chars);
+
 /* See completer.h.  */
 
 class completion_tracker::completion_hash_entry
@@ -1113,9 +1115,12 @@ expression_completer (struct cmd_list_element *ignore,
   complete_expression (tracker, text, word);
 }
 
-/* See definition in completer.h.  */
+/* Set the word break characters array to BREAK_CHARS.  This function is
+   useful as const-correct alternative to direct assignment to
+   rl_completer_word_break_characters, which is "char *", not "const
+   char *".  */
 
-void
+static void
 set_rl_completer_word_break_characters (const char *break_chars)
 {
   rl_completer_word_break_characters = (char *) break_chars;
@@ -1940,8 +1945,12 @@ gdb_completion_word_break_characters_throw ()
   return (char *) rl_completer_word_break_characters;
 }
 
-char *
-gdb_completion_word_break_characters ()
+/* Get the list of chars that are considered as word breaks for the current
+   command.  This function does not throw any exceptions and is called from
+   readline.  See gdb_completion_word_break_characters_throw for details.  */
+
+static char *
+gdb_completion_word_break_characters () noexcept
 {
   /* New completion starting.  */
   current_completion.aborted = false;
@@ -2336,7 +2345,7 @@ gdb_rl_attempted_completion_function_throw (const char *text, int start, int end
    hook.  Wrapper around gdb_rl_attempted_completion_function_throw
    that catches C++ exceptions, which can't cross readline.  */
 
-char **
+static char **
 gdb_rl_attempted_completion_function (const char *text, int start, int end)
 {
   /* Restore globals that might have been tweaked in
@@ -3004,6 +3013,11 @@ void _initialize_completer ();
 void
 _initialize_completer ()
 {
+  /* Setup some readline completion globals.  */
+  rl_completion_word_break_hook = gdb_completion_word_break_characters;
+  rl_attempted_completion_function = gdb_rl_attempted_completion_function;
+  set_rl_completer_word_break_characters (default_word_break_characters ());
+
   add_setshow_zuinteger_unlimited_cmd ("max-completions", no_class,
 				       &max_completions, _("\
 Set maximum number of completion candidates."), _("\
diff --git a/gdb/completer.h b/gdb/completer.h
index 9011ba778a7..98a12f3907c 100644
--- a/gdb/completer.h
+++ b/gdb/completer.h
@@ -569,9 +569,6 @@ const char *advance_to_expression_complete_word_point
 extern const char *advance_to_filename_complete_word_point
   (completion_tracker &tracker, const char *text);
 
-extern char **gdb_rl_attempted_completion_function (const char *text,
-						    int start, int end);
-
 extern void noop_completer (struct cmd_list_element *,
 			    completion_tracker &tracker,
 			    const char *, const char *);
@@ -608,14 +605,6 @@ extern void reggroup_completer (struct cmd_list_element *,
 				completion_tracker &tracker,
 				const char *, const char *);
 
-extern char *gdb_completion_word_break_characters (void);
-
-/* Set the word break characters array to BREAK_CHARS.  This function
-   is useful as const-correct alternative to direct assignment to
-   rl_completer_word_break_characters, which is "char *",
-   not "const char *".  */
-extern void set_rl_completer_word_break_characters (const char *break_chars);
-
 /* Get the matching completer_handle_brkchars_ftype function for FN.
    FN is one of the core completer functions above (filename,
    location, symbol, etc.).  This function is useful for cases when
diff --git a/gdb/top.c b/gdb/top.c
index 8df684ec8b4..0aeeb987832 100644
--- a/gdb/top.c
+++ b/gdb/top.c
@@ -2139,9 +2139,6 @@ init_main (void)
   write_history_p = 0;
 
   /* Setup important stuff for command line editing.  */
-  rl_completion_word_break_hook = gdb_completion_word_break_characters;
-  rl_attempted_completion_function = gdb_rl_attempted_completion_function;
-  set_rl_completer_word_break_characters (default_word_break_characters ());
   rl_completion_display_matches_hook = cli_display_match_list;
   rl_readline_name = "gdb";
   rl_terminal_name = getenv ("TERM");
-- 
2.25.4

Re: [PATCHv2 0/7] Cleanup and changes for file name completion

[Andrew Burgess]

Andrew Burgess <aburgess@redhat.com> writes:

> In v2:
>
>   + Rebased onto current upstream/master and retested,
>
>   + Added two additional clean up patches #6 and #7.

I've taken the liberty of pushing this series.  If there is any
post-commit feedback then please just let me know, happy to address any
concerns.

Thanks,
Andrew