[binutils-gdb] gdb: add trailing '/' when using 'complete' with directory names

[binutils-gdb] gdb: add trailing '/' when using 'complete' with directory names

[Andrew Burgess]

https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;h=3b9ff5d9009d587aa76ec7bbaab4439e26a52c50

commit 3b9ff5d9009d587aa76ec7bbaab4439e26a52c50
Author: Andrew Burgess <aburgess@redhat.com>
Date:   Tue Jan 2 17:08:30 2024 +0000

    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>

Diff:
---
 gdb/completer.c                                |  26 ++++--
 gdb/completer.h                                |  12 ++-
 gdb/linespec.c                                 |   2 +-
 gdb/testsuite/gdb.base/filename-completion.exp |  94 ++++++++++++++++++++
 gdb/testsuite/lib/completion-support.exp       | 116 +++++++++++++++----------
 5 files changed, 198 insertions(+), 52 deletions(-)

diff --git a/gdb/completer.c b/gdb/completer.c
index d69ddcceca9..8e2f031f13a 100644
--- a/gdb/completer.c
+++ b/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_completions;
+  completion_tracker tracker_handle_brkchars (false);
+  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);
diff --git a/gdb/completer.h b/gdb/completer.h
index d96c4b515d3..4faef4ee80b 100644
--- a/gdb/completer.h
+++ b/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
diff --git a/gdb/linespec.c b/gdb/linespec.c
index 76a93d7a35b..7be65c8017d 100644
--- a/gdb/linespec.c
+++ b/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
diff --git a/gdb/testsuite/gdb.base/filename-completion.exp b/gdb/testsuite/gdb.base/filename-completion.exp
new file mode 100644
index 00000000000..20ca227f96d
--- /dev/null
+++ b/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
+	}
+    }
+}
diff --git a/gdb/testsuite/lib/completion-support.exp b/gdb/testsuite/lib/completion-support.exp
index 3196b04d04f..0103d6b63bb 100644
--- a/gdb/testsuite/lib/completion-support.exp
+++ b/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 } {
-    gdb_test_no_output "complete $line" "cmd complete \"$line\""
+proc test_gdb_complete_cmd_none { line { testname "" } } {
+    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\""
-    gdb_test_multiple $cmd $test {
+    if { $testname eq "" } {
+	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\""
-    gdb_test_multiple "complete $cmd_prefix$completion_word" $test {
+    if { $testname eq "" } {
+	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