gdb: add trailing '/' when using 'complete' with directory names

This patch contains work pulled from this previously proposed patch:

  https://inbox.sourceware.org/gdb-patches/20210213220752.32581-2-lsix@lancelotsix.com/

But has been modified by me.  Credit for the original idea and
implementation goes to Lancelot, any bugs in this new iteration belong
to me.

Consider the executable `/tmp/foo/my_exec', and if we assume `/tmp' is
empty other than the `foo' sub-directory, then currently within GDB,
if I type:

  (gdb) file /tmp/f

and then hit TAB, GDB completes this to:

  (gdb) file /tmp/foo/

notice that not only did GDB fill in the whole of `foo', but GDB also
added a trailing '/' character.  This is done within readline when the
path that was just completed is a directory.  However, if I instead
do:

  (gdb) complete file /tmp/f
  file /tmp/foo

I now see the completed directory name, but the trailing '/' is
missing.  The reason is that, in this case, the completions are not
offered via readline, but are handled entirely within GDB, and so
readline never gets the chance to add the trailing '/' character.

The above patch added filename option support to GDB, which included
completion of the filename options.  This initially suffered from the
same problem that I've outlined above, but the above patch proposed a
solution to this problem, but this solution only applied to filename
options (which have still not been added to GDB), and was mixed in
with the complete filename options support.

This patch pulls out just the fix for the trailing "/" problem, and
applies it to GDB's general filename completion.  This patch does not
add filename options to GDB, that can always be done later, but I
think this small part is itself a useful fix.

One of the biggest changes I made in this version is that I got rid of
the set_from_readline member function, instead, I now pass the value
of m_from_readline into the completion_tracker constructor.

I then moved the addition of the trailing '/' into filename_completer
so that it is applied in the general filename completion case.  I also
added a call to gdb_tilde_expand which was missing from the original
patch, I haven't tested, but I suspect that this meant that the
original patch would not add the trailing '/' if the user entered a
path starting with a tilde character.

When writing the test for this patch I ran into two problems.

The first was that the procedures in lib/completion-support.exp relied
on the command being completed for the test name.  This is fine for
many commands, but not when completing a filename, if we use the
command in this case the test name will (potentially) include the name
of the directory in which the test is being run, which means we can't
compare results between two runs of GDB from different directories.

So in this commit I've gone through completion-support.exp and added a
new (optional) testname argument to many of the procedures, this
allows me to give a unique test name, that doesn't include the path
for my new tests.

The second issue was in the procedure make_tab_completion_list_re,
this builds the completion list which is displayed after a double tab
when there are multiple possible completions.

The procedure added the regexp ' +' after each completion, and then
added another ' +' at the very end of the expected output.  So, if we
expected to match the name of two functions 'f1' and 'f2' the
generated regexp would be: 'f1 +f2 + +'.  This would match just fine,
the actual output would be: 'f1  f2  ', notice that we get two spaces
after each function name.

However, if we complete two directory names 'd1' and 'd2' then the
output will be 'd1/ d2/ '.  Notice that now we only have a single
space between each match, however, we do get the '/' added instead.

What happens is that when presenting the matches, readline always adds
the appropriate trailing character; if we performed tab completion of
'break f1' then, as 'f1' is a unique match, we'd get 'break f1 ' with
a trailing space added.  However, if we complete 'file d1' then we get
'file d1/'.  Then readline is adding a single space after each
possible match, including the last one, which accounts for the
trailing space character.

To resolve this I've simply remove the addition o the second ' +'
within make_tab_completion_list_re, for the function completion
example I gave above the expected pattern is now 'f1 +f2 +', which for
the directory case we expect 'd1/ +d2/ +', both of which work just
fine.

Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=16265
Co-Authored-By: Lancelot SIX <lsix@lancelotsix.com>
Approved-By: Tom Tromey <tom@tromey.com>
Reviewed-By: John Baldwin <jhb@FreeBSD.org>

gdb/completer.c

@@ -31,6 +31,7 @@
 #include <algorithm>
 #include "linespec.h"
 #include "cli/cli-decode.h"
+#include "gdbsupport/gdb_tilde_expand.h"
 
 /* FIXME: This is needed because of lookup_cmd_1 ().  We should be
    calling a hook instead so we eliminate the CLI dependency.  */
@@ -225,6 +226,20 @@ 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), text, word));
     }
@@ -1474,7 +1489,8 @@ int max_completions = 200;
 
 /* See completer.h.  */
 
-completion_tracker::completion_tracker ()
+completion_tracker::completion_tracker (bool from_readline)
+  : m_from_readline (from_readline)
 {
   discard_completions ();
 }
@@ -1671,8 +1687,8 @@ make_completion_match_str (gdb::unique_xmalloc_ptr<char> &&match_name,
 completion_result
 complete (const char *line, char const **word, int *quote_char)
 {
-  completion_tracker tracker_handle_brkchars;
+  completion_tracker tracker_handle_brkchars (false);
-  completion_tracker tracker_handle_completions;
+  completion_tracker tracker_handle_completions (false);
   completion_tracker *tracker;
 
   /* The WORD should be set to the end of word to complete.  We initialize
@@ -1902,7 +1918,7 @@ gdb_completion_word_break_characters_throw ()
   /* New completion starting.  Get rid of the previous tracker and
      start afresh.  */
   delete current_completion.tracker;
-  current_completion.tracker = new completion_tracker ();
+  current_completion.tracker = new completion_tracker (true);
 
   completion_tracker &tracker = *current_completion.tracker;
 
@@ -2308,7 +2324,7 @@ gdb_rl_attempted_completion_function_throw (const char *text, int start, int end
   if (end == 0 || !current_completion.tracker->use_custom_word_point ())
     {
       delete current_completion.tracker;
-      current_completion.tracker = new completion_tracker ();
+      current_completion.tracker = new completion_tracker (true);
 
       complete_line (*current_completion.tracker, text,
 		     rl_line_buffer, rl_point);

gdb/completer.h

@@ -328,7 +328,7 @@ public:
 class completion_tracker
 {
 public:
-  completion_tracker ();
+  explicit completion_tracker (bool from_readline);
   ~completion_tracker ();
 
   DISABLE_COPY_AND_ASSIGN (completion_tracker);
@@ -423,6 +423,11 @@ public:
   completion_result build_completion_result (const char *text,
 					     int start, int end);
 
+  /* Tells if the completion task is triggered by readline.  See
+     m_from_readline.  */
+  bool from_readline () const
+  { return m_from_readline; }
+
 private:
 
   /* The type that we place into the m_entries_hash hash table.  */
@@ -512,6 +517,11 @@ private:
      track the maximum possible size of the lowest common denominator,
      which we know as each completion is added.  */
   size_t m_lowest_common_denominator_max_length = 0;
+
+  /* Indicates that the completions are to be displayed by readline
+     interactively. The 'complete' command is a way to generate completions
+     not to be displayed by readline.  */
+  bool m_from_readline;
 };
 
 /* Return a string to hand off to readline as a completion match

gdb/linespec.c

@@ -1784,7 +1784,7 @@ linespec_parse_basic (linespec_parser *parser)
       if (!parser->completion_quote_char
 	  && strcmp (PARSER_STREAM (parser), ":") == 0)
 	{
-	  completion_tracker tmp_tracker;
+	  completion_tracker tmp_tracker (false);
 	  const char *source_filename
 	    = PARSER_EXPLICIT (parser)->source_filename.get ();
 	  symbol_name_match_type match_type

gdb/testsuite/gdb.base/filename-completion.exp

@@ -0,0 +1,94 @@
+# Copyright 2024 Free Software Foundation, Inc.
+
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 3 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.
+
+# Tests for filename completion.  Create a directory tree on the test
+# machine and try completing filenames within the tree.
+
+load_lib completion-support.exp
+
+# Setup a directory tree in which completion tests can be run.  The
+# structure is:
+#
+# root/			[ DIRECTORY ]
+#   aaa/		[ DIRECTORY ]
+#   bb1/		[ DIRECTORY ]
+#   bb2/		[ DIRECTORY ]
+#   cc1/		[ DIRECTORY ]
+#   cc2			[ FILE ]
+proc setup_directory_tree {} {
+    set root [standard_output_file "root"]
+
+    remote_exec host "mkdir -p ${root}"
+    remote_exec host "mkdir -p ${root}/aaa"
+    remote_exec host "mkdir -p ${root}/bb1"
+    remote_exec host "mkdir -p ${root}/bb2"
+    remote_exec host "mkdir -p ${root}/cc1"
+    remote_exec host "touch ${root}/cc2"
+
+    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 } {
+    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"
+}
+
+gdb_start
+
+set root [setup_directory_tree]
+
+run_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.
+if {![is_remote host]} {
+
+    # The users home directory.
+    set home $::env(HOME)
+
+    # Check if ROOT is within the $HOME directory.  If it is then we can
+    # rerun the tests, but replacing the $HOME part with "~".
+    if { [string compare -length [string length $home] $root $home] == 0 } {
+	# Convert the $HOME prefix in to ~.
+	set tilde_root "~[string range $root [string length $home] end]"
+
+	with_test_prefix "with tilde" {
+	    # And rerun the tests.
+	    run_tests $tilde_root
+	}
+    }
+}

gdb/testsuite/lib/completion-support.exp

@@ -50,7 +50,6 @@ proc make_tab_completion_list_re { completion_list } {
 	append completion_list_re [string_to_regexp $c]
 	append completion_list_re $ws
     }
-    append completion_list_re $ws
 
     return $completion_list_re
 }
@@ -85,21 +84,25 @@ proc clear_input_line { test } {
 
 # Test that completing LINE with TAB completes to nothing.
 
-proc test_gdb_complete_tab_none { line } {
+proc test_gdb_complete_tab_none { line { testname "" } } {
     set line_re [string_to_regexp $line]
 
-    set test "tab complete \"$line\""
+    if { $testname eq "" } {
+	set testname "tab complete \"$line\""
+    } else {
+	set testname "tab complete: $testname"
+    }
     send_gdb "$line\t"
-    gdb_test_multiple "" "$test" {
+    gdb_test_multiple "" "$testname" {
 	-re "^$line_re$completion::bell_re$" {
-	    pass "$test"
+	    pass $gdb_test_name
 	}
 	-re "$line_re\[^ \]+ $" {
-	    fail "$test"
+	    fail $gdb_test_name
 	}
     }
 
-    clear_input_line $test
+    clear_input_line $testname
 }
 
 # Test that completing INPUT_LINE with TAB completes to
@@ -107,22 +110,26 @@ proc test_gdb_complete_tab_none { line } {
 # appended after EXPECTED_OUTPUT.  Normally that's a whitespace, but
 # in some cases it's some other character, like a colon.
 
-proc test_gdb_complete_tab_unique { input_line complete_line_re append_char_re } {
+proc test_gdb_complete_tab_unique { input_line complete_line_re append_char_re {testname ""} } {
 
-    set test "tab complete \"$input_line\""
+    if { $testname eq "" } {
+	set testname "tab complete \"$input_line\""
+    } else {
+	set testname "tab complete: $testname"
+    }
     send_gdb "$input_line\t"
     set res 1
-    gdb_test_multiple "" "$test" {
+    gdb_test_multiple "" "$testname" {
 	-re "^$complete_line_re$append_char_re$" {
-	    pass "$test"
+	    pass $gdb_test_name
 	}
 	timeout {
-	    fail "$test (timeout)"
+	    fail "$gdb_test_name (timeout)"
 	    set res -1
 	}
     }
 
-    clear_input_line $test
+    clear_input_line $testname
     return $res
 }
 
@@ -132,7 +139,8 @@ proc test_gdb_complete_tab_unique { input_line complete_line_re append_char_re }
 # to hit the max-completions limit.
 
 proc test_gdb_complete_tab_multiple { input_line add_completed_line \
-					  completion_list {max_completions 0}} {
+					  completion_list {max_completions false} \
+					  {testname ""} } {
     global gdb_prompt
 
     set input_line_re [string_to_regexp $input_line]
@@ -146,9 +154,13 @@ proc test_gdb_complete_tab_multiple { input_line add_completed_line \
 	    "\\*\\*\\* List may be truncated, max-completions reached\\. \\*\\*\\*"
     }
 
-    set test "tab complete \"$input_line\""
+    if { $testname eq "" } {
+	set testname "tab complete \"$input_line\""
+    } else {
+	set testname "tab complete: $testname"
+    }
     send_gdb "$input_line\t"
-    gdb_test_multiple "" "$test (first tab)" {
+    gdb_test_multiple "" "$testname (first tab)" {
 	-re "^${input_line_re}${completion::bell_re}$add_completed_line_re$" {
 	    send_gdb "\t"
 	    # If we auto-completed to an ambiguous prefix, we need an
@@ -159,52 +171,61 @@ proc test_gdb_complete_tab_multiple { input_line add_completed_line \
 	    } else {
 		set maybe_bell ""
 	    }
-	    gdb_test_multiple "" "$test (second tab)" {
+	    gdb_test_multiple "" "$testname (second tab)" {
 		-re "^${maybe_bell}\r\n$expected_re\r\n$gdb_prompt " {
-		    gdb_test_multiple "" "$test (second tab)" {
+		    gdb_test_multiple "" "$testname (second tab)" {
 			-re "^$input_line_re$add_completed_line_re$" {
-			    pass "$test"
+			    pass $gdb_test_name
 			}
 		    }
 		}
 		-re "${maybe_bell}\r\n.+\r\n$gdb_prompt $" {
-		    fail "$test"
+		    fail $gdb_test_name
 		}
 	    }
 	}
     }
 
-    clear_input_line $test
+    clear_input_line $testname
 }
 
 # Test that completing LINE with the complete command completes to
 # nothing.
 
-proc test_gdb_complete_cmd_none { line } {
+proc test_gdb_complete_cmd_none { line { testname "" } } {
-    gdb_test_no_output "complete $line" "cmd complete \"$line\""
+    if { $testname eq "" } {
+	set testname "cmd complete \"$line\""
+    } else {
+	set testname "cmd complete: $testname"
+    }
+    gdb_test_no_output "complete $line" $testname
 }
 
 # Test that completing LINE with the complete command completes to
 # COMPLETE_LINE_RE.
 # Returns 1 if the test passed, 0 if it failed, -1 if it timed out.
 
-proc test_gdb_complete_cmd_unique { input_line complete_line_re } {
+proc test_gdb_complete_cmd_unique { input_line complete_line_re {testname ""} } {
     global gdb_prompt
 
     set res 0
     set cmd "complete $input_line"
     set cmd_re [string_to_regexp $cmd]
-    set test "cmd complete \"$input_line\""
+    if { $testname eq "" } {
-    gdb_test_multiple $cmd $test {
+	set testname "cmd complete \"$input_line\""
+    } else {
+	set testname "cmd complete: $testname"
+    }
+    gdb_test_multiple $cmd $testname {
 	-re "^$cmd_re\r\n$complete_line_re\r\n$gdb_prompt $" {
-	    pass $test
+	    pass $gdb_test_name
 	    set res 1
 	}
 	-re "$gdb_prompt $" {
-	    fail "$test"
+	    fail $gdb_test_name
 	}
 	timeout {
-	    fail "$test (timeout)"
+	    fail "$gdb_test_name (timeout)"
 	    set res -1
 	}
     }
@@ -217,7 +238,7 @@ proc test_gdb_complete_cmd_unique { input_line complete_line_re } {
 # MAX_COMPLETIONS then we expect the completion to hit the
 # max-completions limit.
 
-proc test_gdb_complete_cmd_multiple { cmd_prefix completion_word completion_list {start_quote_char ""} {end_quote_char ""} {max_completions 0}} {
+proc test_gdb_complete_cmd_multiple { cmd_prefix completion_word completion_list {start_quote_char ""} {end_quote_char ""} {max_completions false} {testname ""} } {
     global gdb_prompt
 
     set expected_re [make_cmd_completion_list_re $cmd_prefix $completion_list $start_quote_char $end_quote_char]
@@ -229,24 +250,28 @@ proc test_gdb_complete_cmd_multiple { cmd_prefix completion_word completion_list
     }
 
     set cmd_re [string_to_regexp "complete $cmd_prefix$completion_word"]
-    set test "cmd complete \"$cmd_prefix$completion_word\""
+    if { $testname eq "" } {
-    gdb_test_multiple "complete $cmd_prefix$completion_word" $test {
+	set testname "cmd complete \"$cmd_prefix$completion_word\""
+    } else {
+	set testname "cmd complete: $testname"
+    }
+    gdb_test_multiple "complete $cmd_prefix$completion_word" $testname {
 	-re "^$cmd_re\r\n$expected_re$gdb_prompt $" {
-	    pass $test
+	    pass $gdb_test_name
 	}
 	-re "$gdb_prompt $" {
-	    fail "$test"
+	    fail $gdb_test_name
 	}
     }
 }
 
 # Test that completing LINE completes to nothing.
 
-proc test_gdb_complete_none { input_line } {
+proc test_gdb_complete_none { input_line { testname "" } } {
     if { [readline_is_used] } {
-	test_gdb_complete_tab_none $input_line
+	test_gdb_complete_tab_none $input_line $testname
     }
-    test_gdb_complete_cmd_none $input_line
+    test_gdb_complete_cmd_none $input_line $testname
 }
 
 # Test that completing INPUT_LINE completes to COMPLETE_LINE_RE.
@@ -265,7 +290,7 @@ proc test_gdb_complete_none { input_line } {
 # of a regular expression (as COMPLETE_LINE_RE).  See
 # test_gdb_complete_unique below.
 
-proc test_gdb_complete_unique_re { input_line complete_line_re {append_char " "} {max_completions 0}} {
+proc test_gdb_complete_unique_re { input_line complete_line_re {append_char " "} {max_completions false} {testname ""} } {
     set append_char_re [string_to_regexp $append_char]
 
     # Trim COMPLETE LINE, for the case we're completing a command with leading
@@ -292,14 +317,14 @@ proc test_gdb_complete_unique_re { input_line complete_line_re {append_char " "}
     # speeds up testing if necessary.
 
     set test_result [test_gdb_complete_cmd_unique $input_line\
-		$expected_output_re]
+		$expected_output_re $testname]
     if { $test_result != 1 } {
 	return $test_result
     }
 
     if { [readline_is_used] } {
 	set test_result [test_gdb_complete_tab_unique $input_line \
-		$complete_line_re $append_char_re]
+		$complete_line_re $append_char_re $testname]
     }
     return $test_result
 }
@@ -307,9 +332,9 @@ proc test_gdb_complete_unique_re { input_line complete_line_re {append_char " "}
 # Like TEST_GDB_COMPLETE_UNIQUE_RE, but COMPLETE_LINE is a string, not
 # a regular expression.
 
-proc test_gdb_complete_unique { input_line complete_line {append_char " "} {max_completions 0}} {
+proc test_gdb_complete_unique { input_line complete_line {append_char " "} {max_completions false} {testname ""} } {
     set complete_line_re [string_to_regexp $complete_line]
-    test_gdb_complete_unique_re $input_line $complete_line_re $append_char $max_completions
+    test_gdb_complete_unique_re $input_line $complete_line_re $append_char $max_completions $testname
 }
 
 # Test that completing "CMD_PREFIX + COMPLETION_WORD" adds
@@ -320,12 +345,13 @@ proc test_gdb_complete_unique { input_line complete_line {append_char " "} {max_
 
 proc test_gdb_complete_multiple {
   cmd_prefix completion_word add_completed_line completion_list
-  {start_quote_char ""} {end_quote_char ""} {max_completions 0}
+  {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
+      test_gdb_complete_tab_multiple "$cmd_prefix$completion_word" $add_completed_line $completion_list $max_completions $testname
     }
-    test_gdb_complete_cmd_multiple $cmd_prefix $completion_word $completion_list $start_quote_char $end_quote_char $max_completions
+    test_gdb_complete_cmd_multiple $cmd_prefix $completion_word $completion_list $start_quote_char $end_quote_char $max_completions $testname
 }
 
 # Test that all the substring prefixes of INPUT from [0..START) to