[PATCH 0/3] Improve the add-inferior completer

[PATCH 0/3] Improve the add-inferior completer

[Lancelot SIX via Gdb-patches]

This patch series aims at providing a proper completer for the
add-inferior command based on the gdb::option framework.

The first two commits add support for new option types (zuinteger and
filename) and the third implements the completer.

The filename completer is based on the one already in gdb/completer.c,
but has different behavior when it comes to handling space and quotation
characters in file names.  For the time being, I propose an independent
completer for the gdb::option framework, but if the proposed behavior is
desired for ::file_completer, I’d be happy to merge them.  I am just
unsure whether there are commands that expect to parse file names as
completed by the current completer (I do not think so).

I also have many warnings by dejagnu because test names contain file
names.  This adds quite noise but it seems inevitable given how the test
names are generated. Is this an issue?

All feedback are welcome and will be appreciated.

Lancelot SIX (3):
  gdb::option: Add support for filename option.
  gdb::option: Add support for zuinteger.
  Add completer to the add-inferior command

 gdb/cli/cli-option.c                     | 277 +++++++++++++++++++++++
 gdb/cli/cli-option.h                     |  41 ++++
 gdb/completer.c                          |   3 +
 gdb/completer.h                          |  15 ++
 gdb/inferior.c                           | 123 ++++++----
 gdb/maint-test-options.c                 |  27 ++-
 gdb/testsuite/gdb.base/completion.exp    |  11 +
 gdb/testsuite/gdb.base/options.exp       | 190 +++++++++++++---
 gdb/testsuite/lib/completion-support.exp |   1 -
 9 files changed, 613 insertions(+), 75 deletions(-)

-- 
2.29.2

[PATCH 1/3] gdb::option: Add support for filename option.

[Lancelot SIX via Gdb-patches]

This commit adds support for filename option for the gdb::option parsing
framework.  It is meant to help the user enter values that could be
parsed by built_argv.

Tab completion works as expected, i.e. similar to bash's behavior.

Considering the following tree structure:

somedir/
├── a folder
└── a file

The following completions are available:

    (gdb) maintenance test-options unknown-is-error -filename somedir/a\ <tab>
    a file    a folder/

or

    (gdb) maintenance test-options unknown-is-error -filename "somedir/a <tab>
    a file    a folder/

The complete command command also provides useful completions which are
escaped or quoted depending on what the user did input initially.

    (gdb) complete maintenance test-options unknown-is-error -filename somedir/a
    maintenance test-options unknown-is-error -filename somedir/a\ file
    maintenance test-options unknown-is-error -filename somedir/a\ folder/
    (gdb) complete maintenance test-options unknown-is-error -filename 'somedir/a
    maintenance test-options unknown-is-error -filename 'somedir/a file
    maintenance test-options unknown-is-error -filename 'somedir/a folder/

Note that since entering a path is an interactive process, the completer
will not append the closing quote after a directory name. Instead it
adds a '/' and expect the user to provide further input.

Having both of those behaviors at the same time requires that the
completer knows if it has been called by readline after the user
pressed the tab key or by some other mechanism.  In the first scenario it
should display non quoted/escaped completions so the user sees actual
filenames but complete quoted/escaped filenames.  In the second scenario
it should only provide quoted/escaped completions.  For that purpose,
this commit adds a m_from_readline property to the completion_tracker
object and initializes it accordingly to the calling context.

This filename_completer has slightly different behavior than the one
available in gdb/completer.c mainly with regard to escaping/quoting.
The original completer is kept as is to maintain stable behavior for
commands whose completer might depend on this implementation.  If this is
desirable, I’ll happily replace the existing filename completer with the
one currently proposed in the gdb::options::filename namespace instead
of having both with slightly different behaviors (but I an not sure if
users are relying on the current behavior of the filename completer).

It really feels that I have had to re-implement many existing readline
behavior and I would be glad if someone has a simpler approach to obtain
the same result.

All feedback are welcome.

gdb/ChangeLog:

	* cli/cli-option.c (union option_value): Add filename option.
	(struct option_def_and_value): Free filename in dtor.
	(gdb::option::filename::quote_filename): New function.
	(gdb::option::filename::handle_completion): New function.
	(gdb::option::filename::filename_copleter): New function.
	(gdb::option::parse_option): Add support for filename.
	(gdb::option::save_option_value_in_ctx): Add support for filename.
	(gdb::option::get_val_type_str): Add support for filename.
	(gdb::option::add_setshow_cmds_for_options): Add support for filename.
	* cli/cli-option.h (struct option_def): Add filename field.
	(struct gdb::option::filename_option_def): Create.
	* completer.c (completion_tracker::discard_completions): Reset the
	m_from_readline flag.
	(gdb_completion_word_break_characters_throw): Set the m_from_readline
	flag on the completion_tracker.
	* completer.h (class completion_tracker): Add m_from_readline fileld
	and associated getter / setters.
	* maint-test-options.c (struct test_options_opts): Add a -filename
	option.

gdb/testsuite/ChangeLog:

	* gdb.base/options.exp: Add tests for the filename option.
	* lib/completion-support.exp (make_tab_completion_list_re):
	Relax number of required spaced at the end of the pattern.
---
 gdb/cli/cli-option.c                     | 250 +++++++++++++++++++++++
 gdb/cli/cli-option.h                     |  20 ++
 gdb/completer.c                          |   3 +
 gdb/completer.h                          |  15 ++
 gdb/maint-test-options.c                 |  15 +-
 gdb/testsuite/gdb.base/options.exp       | 126 ++++++++++--
 gdb/testsuite/lib/completion-support.exp |   1 -
 7 files changed, 415 insertions(+), 15 deletions(-)

diff --git a/gdb/cli/cli-option.c b/gdb/cli/cli-option.c
index ab76f194c3d..5a6c997dcda 100644
--- a/gdb/cli/cli-option.c
+++ b/gdb/cli/cli-option.c
@@ -24,6 +24,8 @@
 #include "cli/cli-setshow.h"
 #include "command.h"
 #include <vector>
+#include <readline/readline.h>
+
 
 namespace gdb {
 namespace option {
@@ -46,6 +48,9 @@ union option_value
 
   /* For var_string options.  This is malloc-allocated.  */
   char *string;
+
+  /* For var_filename option. This is malloc-allocated.  */
+  char *filename;
 };
 
 /* Holds an options definition and its value.  */
@@ -88,6 +93,8 @@ struct option_def_and_value
       {
 	if (option.type == var_string)
 	  xfree (value->string);
+	if (option.type == var_filename)
+	  xfree (value->filename);
       }
   }
 
@@ -105,6 +112,8 @@ struct option_def_and_value
       {
 	if (option.type == var_string)
 	  value->string = nullptr;
+	if (option.type == var_filename)
+	  value->filename = nullptr;
       }
   }
 };
@@ -164,6 +173,197 @@ complete_on_options (gdb::array_view<const option_def_group> options_group,
 	}
 }
 
+namespace filename {
+
+/* Quote the filename whose name is given by TEXT using quoting char
+   QUOTECHAR. QUOTECHAR can be '\'', '"' or '\0'.
+
+   This function is meant to be used with a completion context, so the
+   closing quote is not appended (we might still be waiting for the user
+   to input more path components).  */
+
+static std::string
+quote_filename (const char *text, char quotechar)
+{
+  std::string quoted;
+  /* strlen (text) is a good first approximation of the resulting string
+     length.  */
+  quoted.reserve (strlen (text));
+
+  if (quotechar != '\0')
+    {
+      quoted.push_back (quotechar);
+      while (*text != '\0')
+	{
+	  if (*text == quotechar)
+	    quoted.push_back ('\\');
+	  quoted.push_back (*text++);
+	}
+    }
+  else
+    {
+      while (*text != '\0')
+	{
+	  /* Escape the space and quote chars.  */
+	  if (*text == ' ' || *text == '\'' || *text == '"')
+	    quoted.push_back ('\\');
+	  quoted.push_back (*text++);
+	}
+    }
+
+  return quoted;
+}
+
+/* Add the filename COMPLETION to TRACKER.  COMPLETION will be quoted
+   using QUOTECHAR, but the closing quote will be dismissed if we expect
+   more input from the user.
+
+   ONLY_COMPLETION is set to true to indicate that this function is called only
+   once (i.e. only one possible completion).  In this case, if COMPLETION is not
+   a directory, the closing quote is appended followed by a space to indicate
+   that the next option is expected.
+
+   See completer_ftype for a description of TEXT and WORD.
+
+   The following table gives examples of how COMPLETION might be passed to
+   TRACKER depending on QUOTECHAR
+
+    COMPLETION		QUOTECHAR   As registered in TRACKER
+    "/home"		'\''	    "'/home/'"
+    "/home"		'\0'	    "/home/"
+    "/home"		'"'	    "\"/home/"
+    "~/some folder"	'\''	    "'~/some folder/"
+    "~/some folder"	'\0'	    "~/some\\ folder/"
+    "~/some folder"	'"'	    "\"~/some folder/"
+    "only_compl"	'\''	    "'only_compl' "
+    "only_compl"	'\0'	    "only_compl "
+    "only_compl"	'"'	    "\"only_compl\" "
+   */
+
+static void
+handle_completion
+  (completion_tracker &tracker, std::string completion,
+   const char quotechar, bool only_completion,
+   const char *text, const char *word)
+{
+  gdb_assert (tracker.suppress_append_ws ());
+  struct stat finfo;
+  const bool isdir
+    = stat (completion.c_str (), &finfo) == 0 && S_ISDIR (finfo.st_mode);
+
+  /* Readline would add a '/' for dirs, so do is ourselves if readline is not
+     here.  */
+  if (isdir && !tracker.from_readline ())
+    completion.push_back ('/');
+
+  std::string quoted_completion = quote_filename
+    (completion.c_str (), quotechar);
+
+  /* If we only have 1 possible completion which is not a dir, then we
+     know the user will not add anything else.  We can close the quotation
+     (if any) and append a space (tracker.suppress_append_ws () is true).  */
+  if (only_completion && !isdir)
+    {
+      if (quotechar != '\0')
+        quoted_completion.push_back (quotechar);
+      quoted_completion.push_back (' ');
+    }
+
+  /* Check if we got here by the user typing
+     'complete [...] -filename ...' or '[...] -filename ...\t'
+
+     In the first case, we want the completion to show a actually
+     quotted string, while in the latter case we want to complete on
+     the quoted filename while showing the unquoted completion to the
+     user.  */
+  if (tracker.from_readline ())
+    {
+      completion_match_for_lcd match_for_lcd;
+      match_for_lcd.set_match (quoted_completion.c_str ());
+
+      tracker.add_completion
+        (std::move (gdb::unique_xmalloc_ptr<char>
+                    (xstrdup (completion.c_str ()))), &match_for_lcd,
+         text, word);
+    }
+  else
+    tracker.add_completion
+      (std::move (gdb::unique_xmalloc_ptr<char>
+		  (xstrdup (quoted_completion.c_str ()))),
+       nullptr, text, word);
+
+}
+
+/* Filename completer.  See completer_ftype for description of parameters.  */
+
+static void
+complete_filename
+  (completion_tracker &tracker, const char *text, const char *word)
+{
+  /* First call to rl_filename_completion_function should be 0, subsequent
+     calls non 0.
+
+     We also track the number of calls done to rl_filename_completion_function.
+     If it only return only one non null value a different processing should be
+     done (see handle_completion for details).  */
+  int call_count = 0;
+  std::string first_completion;
+
+  /* The logic that tells if a space is to be appended after a completion is
+     done in handle_completion, not delegated to the tracker.  */
+  tracker.set_suppress_append_ws (true);
+
+  /* Keep track of what quoting mechanism the user used so we can re-quote
+     completions accordingly.  */
+  char quotechar;
+  if (text[0] == '\'' || text[0] == '"')
+    quotechar = text[0];
+  else
+    quotechar = '\0';
+
+  const char *arg_start = text;
+  const std::string dequoted = extract_string_maybe_quoted (&arg_start);
+
+  try
+    {
+      while (true)
+	{
+	  gdb::unique_xmalloc_ptr<char> next_completion
+	    (rl_filename_completion_function
+	     (dequoted.c_str (), call_count++));
+
+	  if (next_completion == nullptr)
+	    break;
+
+	  if (call_count == 1)
+	    first_completion = next_completion.get ();
+	  else
+	    {
+	      if (call_count == 2)
+		handle_completion
+		  (tracker, first_completion, quotechar, false, text,
+		   word);
+
+	      handle_completion
+		(tracker, next_completion.get (), quotechar, false, text,
+		 word);
+	    }
+	}
+
+      /* If there was just 1 completion found, do readline job.  */
+      if (call_count == 2)
+	handle_completion
+	  (tracker, first_completion, quotechar, true, text, word);
+    }
+  catch (const gdb_exception_error &except)
+    {
+      if (except.error != MAX_COMPLETIONS_REACHED_ERROR)
+        throw;
+    }
+}
+
+} /* namespace filename */
+
 /* See cli-option.h.  */
 
 void
@@ -443,6 +643,40 @@ parse_option (gdb::array_view<const option_def_group> options_group,
 	return option_def_and_value {*match, match_ctx, val};
       }
 
+    case var_filename:
+      {
+	if (check_for_argument (args, "--"))
+	  {
+	    /* Treat "maint test-options -filename --" as if there
+	       was no argument after "-filename".  */
+	    error (_("-%s requires an argument"), match->name);
+	  }
+
+	/* We are actually parsing the input, so advance the pointer to
+	   after what have been parsed.  */
+        const char *arg_start = *args;
+	const std::string fname = extract_string_maybe_quoted (args);
+
+        /* If required, perform completion.  */
+	if (completion != nullptr)
+          {
+	    if (**args == '\0')
+              {
+                gdb::option::filename::complete_filename
+                  (completion->tracker, arg_start, arg_start);
+
+                return {};
+              }
+	  }
+
+	if (fname == "")
+	  error (_("-%s requires an argument"), match->name);
+
+	option_value val;
+	val.filename = xstrdup (fname.c_str ());
+	return option_def_and_value {*match, match_ctx, val};
+      }
+
     default:
       /* Not yet.  */
       gdb_assert_not_reached (_("option type not supported"));
@@ -606,6 +840,11 @@ save_option_value_in_ctx (gdb::optional<option_def_and_value> &ov)
 	= ov->value->string;
       ov->value->string = nullptr;
       break;
+    case var_filename:
+      *ov->option.var_address.filename (ov->option, ov->ctx)
+	= ov->value->filename;
+      ov->value->filename = nullptr;
+      break;
     default:
       gdb_assert_not_reached ("unhandled option type");
     }
@@ -680,6 +919,8 @@ get_val_type_str (const option_def &opt, std::string &buffer)
       }
     case var_string:
       return "STRING";
+    case var_filename:
+      return "FILENAME";
     default:
       return nullptr;
     }
@@ -824,6 +1065,15 @@ add_setshow_cmds_for_options (command_class cmd_class,
 				  nullptr, option.show_cmd_cb,
 				  set_list, show_list);
 	}
+      else if (option.type == var_filename)
+	{
+	  add_setshow_filename_cmd (option.name, cmd_class,
+				    option.var_address.filename (option, data),
+				    option.set_doc, option.show_doc,
+				    option.help_doc,
+				    nullptr, option.show_cmd_cb,
+				    set_list, show_list);
+	}
       else
 	gdb_assert_not_reached (_("option type not handled"));
     }
diff --git a/gdb/cli/cli-option.h b/gdb/cli/cli-option.h
index aa2ccbed5d0..fc3aca887e7 100644
--- a/gdb/cli/cli-option.h
+++ b/gdb/cli/cli-option.h
@@ -87,6 +87,7 @@ struct option_def
       int *(*integer) (const option_def &, void *ctx);
       const char **(*enumeration) (const option_def &, void *ctx);
       char **(*string) (const option_def &, void *ctx);
+      char **(*filename) (const option_def &, void *ctx);
     }
   var_address;
 
@@ -282,6 +283,25 @@ struct string_option_def : option_def
   }
 };
 
+/* A var_filename command line option.  */
+
+template <typename Context>
+struct filename_option_def : option_def
+{
+  filename_option_def (const char *long_option_,
+		       char **(*get_var_address_cb_) (Context *),
+		       show_value_ftype *show_cmd_cb_,
+		       const char *set_doc_,
+		       const char *show_doc_ = nullptr,
+		       const char *help_doc_ = nullptr)
+    : option_def (long_option_, var_filename,
+		  (erased_get_var_address_ftype *) get_var_address_cb_,
+		  show_cmd_cb_, set_doc_, show_doc_, help_doc_)
+  {
+    var_address.enumeration = detail::get_var_address<const char *, Context>;
+  }
+};
+
 /* A group of options that all share the same context pointer to pass
    to the options' get-current-value callbacks.  */
 struct option_def_group
diff --git a/gdb/completer.c b/gdb/completer.c
index 2330ad435a8..0efc28de184 100644
--- a/gdb/completer.c
+++ b/gdb/completer.c
@@ -1622,6 +1622,8 @@ completion_tracker::discard_completions ()
 					   entry_hash_func, entry_eq_func,
 					   completion_hash_entry::deleter,
 					   xcalloc, xfree));
+
+  m_from_readline = false;
 }
 
 /* See completer.h.  */
@@ -2006,6 +2008,7 @@ gdb_completion_word_break_characters_throw ()
      start afresh.  */
   delete current_completion.tracker;
   current_completion.tracker = new completion_tracker ();
+  current_completion.tracker->set_from_readline (true);
 
   completion_tracker &tracker = *current_completion.tracker;
 
diff --git a/gdb/completer.h b/gdb/completer.h
index 240490f0c05..d9ece352547 100644
--- a/gdb/completer.h
+++ b/gdb/completer.h
@@ -405,6 +405,16 @@ class completion_tracker
   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.  */
+  void set_from_readline (bool from_readline)
+  { m_from_readline = from_readline; }
+
+  /* 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.  */
@@ -494,6 +504,11 @@ class completion_tracker
      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/maint-test-options.c b/gdb/maint-test-options.c
index 16ecd1dd7e5..93d62eecc47 100644
--- a/gdb/maint-test-options.c
+++ b/gdb/maint-test-options.c
@@ -134,6 +134,7 @@ struct test_options_opts
   unsigned int uint_opt = 0;
   int zuint_unl_opt = 0;
   char *string_opt = nullptr;
+  char *filename_opt = nullptr;
 
   test_options_opts () = default;
 
@@ -150,7 +151,8 @@ struct test_options_opts
   {
     fprintf_unfiltered (file,
 			_("-flag %d -xx1 %d -xx2 %d -bool %d "
-			  "-enum %s -uint %s -zuint-unl %s -string '%s' -- %s\n"),
+			  "-enum %s -uint %s -zuint-unl %s -string '%s' "
+			  "-filename '%s' -- %s\n"),
 			flag_opt,
 			xx1_opt,
 			xx2_opt,
@@ -165,6 +167,9 @@ struct test_options_opts
 			(string_opt != nullptr
 			 ? string_opt
 			 : ""),
+			(filename_opt != nullptr
+			 ? filename_opt
+			 : ""),
 			args);
   }
 };
@@ -237,6 +242,14 @@ static const gdb::option::option_def test_options_option_defs[] = {
     nullptr, /* show_cmd_cb */
     N_("A string option."),
   },
+
+  /* A filename option.  */
+  gdb::option::filename_option_def<test_options_opts> {
+    "filename",
+    [] (test_options_opts *opts) { return &opts->filename_opt; },
+    nullptr, /* show_cmd_cb */
+    N_("A filename option."),
+  },
 };
 
 /* Create an option_def_group for the test_options_opts options, with
diff --git a/gdb/testsuite/gdb.base/options.exp b/gdb/testsuite/gdb.base/options.exp
index 44c773c3fa3..92b298064b2 100644
--- a/gdb/testsuite/gdb.base/options.exp
+++ b/gdb/testsuite/gdb.base/options.exp
@@ -95,19 +95,19 @@ proc make_cmd {variant} {
 # test-options xxx", with no flag/option set.  OPERAND is the expected
 # operand.
 proc expect_none {operand} {
-    return "-flag 0 -xx1 0 -xx2 0 -bool 0 -enum xxx -uint 0 -zuint-unl 0 -string '' -- $operand"
+    return "-flag 0 -xx1 0 -xx2 0 -bool 0 -enum xxx -uint 0 -zuint-unl 0 -string '' -filename '' -- $operand"
 }
 
 # Return a string for the expected result of running "maint
 # test-options xxx", with -flag set.  OPERAND is the expected operand.
 proc expect_flag {operand} {
-    return "-flag 1 -xx1 0 -xx2 0 -bool 0 -enum xxx -uint 0 -zuint-unl 0 -string '' -- $operand"
+    return "-flag 1 -xx1 0 -xx2 0 -bool 0 -enum xxx -uint 0 -zuint-unl 0 -string '' -filename '' -- $operand"
 }
 
 # Return a string for the expected result of running "maint
 # test-options xxx", with -bool set.  OPERAND is the expected operand.
 proc expect_bool {operand} {
-    return "-flag 0 -xx1 0 -xx2 0 -bool 1 -enum xxx -uint 0 -zuint-unl 0 -string '' -- $operand"
+    return "-flag 0 -xx1 0 -xx2 0 -bool 1 -enum xxx -uint 0 -zuint-unl 0 -string '' -filename '' -- $operand"
 }
 
 # Return a string for the expected result of running "maint
@@ -116,31 +116,46 @@ proc expect_bool {operand} {
 # expected operand.
 proc expect_integer {option val operand} {
     if {$option == "uinteger"} {
-	return "-flag 0 -xx1 0 -xx2 0 -bool 0 -enum xxx -uint $val -zuint-unl 0 -string '' -- $operand"
+	return "-flag 0 -xx1 0 -xx2 0 -bool 0 -enum xxx -uint $val -zuint-unl 0 -string '' -filename '' -- $operand"
     } elseif {$option == "zuinteger-unlimited"} {
-	return "-flag 0 -xx1 0 -xx2 0 -bool 0 -enum xxx -uint 0 -zuint-unl $val -string '' -- $operand"
+	return "-flag 0 -xx1 0 -xx2 0 -bool 0 -enum xxx -uint 0 -zuint-unl $val -string '' -filename '' -- $operand"
     } else {
 	error "unsupported option: $option"
     }
 }
 
-# Return a string for the expected result of running "maint
-# test-options xxx", with -string set to $STR.  OPERAND is the
-# expected operand.
-proc expect_string {str operand} {
-    # Dequote the string in the expected output.
+# Helper function used to dequote string litterals in expected output.
+proc dequote_expected_string {str} {
     if { ( [string range $str 0 0] == "\""
 	   && [string range $str end end] == "\"")
 	 || ([string range $str 0 0] == "'"
 	     && [string range $str end end] == "'")} {
-	set str [string range $str 1 end-1]
+	return [string range $str 1 end-1]
+    } else {
+	return $str
     }
-    return "-flag 0 -xx1 0 -xx2 0 -bool 0 -enum xxx -uint 0 -zuint-unl 0 -string '$str' -- $operand"
+}
+
+# Return a string for the expected result of running "maint
+# test-options xxx", with -string set to $STR.  OPERAND is the
+# expected operand.
+proc expect_string {str operand} {
+    set str [dequote_expected_string $str]
+    return "-flag 0 -xx1 0 -xx2 0 -bool 0 -enum xxx -uint 0 -zuint-unl 0 -string '$str' -filename '' -- $operand"
+}
+
+# Return a string for the expected result of running "maint
+# test-options xxx", with -filename set to $FILENAME.  OPERAND is the
+# expected operand.
+proc expect_filename {filename operand} {
+    set filename [dequote_expected_string $filename]
+    return "-flag 0 -xx1 0 -xx2 0 -bool 0 -enum xxx -uint 0 -zuint-unl 0 -string '' -filename '$filename' -- $operand"
 }
 
 set all_options {
     "-bool"
     "-enum"
+    "-filename"
     "-flag"
     "-string"
     "-uinteger"
@@ -594,7 +609,7 @@ proc_with_prefix test-flag {variant} {
 
     # Extract twice the same flag, separated by one space.
     gdb_test "$cmd -xx1     -xx2 -xx1  -xx2 -xx1    -- non flags args" \
-	"-flag 0 -xx1 1 -xx2 1 -bool 0 -enum xxx -uint 0 -zuint-unl 0 -string '' -- non flags args"
+	"-flag 0 -xx1 1 -xx2 1 -bool 0 -enum xxx -uint 0 -zuint-unl 0 -string '' -filename '' -- non flags args"
 
     # Extract 2 known flags in front of unknown flags.
     gdb_test "$cmd -xx1 -xx2 -a -b -c -xx1 --" \
@@ -1013,6 +1028,90 @@ proc_with_prefix test-string {variant} {
     }
 }
 
+# filename option tests.
+proc_with_prefix test-filename {variant} {
+    global all_options
+    set tmpdir [standard_output_file ""]
+
+    set cmd [make_cmd $variant]
+
+    gdb_test "$cmd -filename --"\
+	"-filename requires an argument"
+    if {$variant == "require-delimiter"} {
+	gdb_test "$cmd -filename" [expect_none "-filename"]
+    } else {
+	gdb_test "$cmd -filename"\
+	    "-filename requires an argument"
+    }
+
+    # Check that dequoting is happening as expected (similar to string
+    # dequoting).
+    foreach_with_prefix str {
+	"STR"
+	"\"STR\""
+	"\\\"STR"
+	"'STR'"
+	"\\'STR"
+	"\"STR AAA\""
+	"'STR BBB'"
+	"\"STR 'CCC' DDD\""
+	"'STR \"EEE\" FFF'"
+	"\"STR \\\"GGG\\\" HHH\""
+	"'STR \\\'III\\\' JJJ'"
+    } {
+	gdb_test "$cmd -filename ${str} --" [expect_filename "${str}" ""]
+    }
+
+    # Create some tree structure we can use for the test.
+    set bdir "$tmpdir/testfilename"
+    file mkdir "$bdir/b"
+    file mkdir "$bdir/b c"
+    file mkdir "$bdir/b d"
+    file mkdir "$bdir/b c/d"
+    close [open "$bdir/b c/d/e" w]
+
+    # Tab completion for an escaped path.
+    test_gdb_complete_tab_multiple "$cmd -filename $bdir/b" "" {
+        "b/"
+        "b c/"
+        "b d/"
+    }
+    # Checking completions with the COMPLETE command shows escaped filenames.
+    test_gdb_complete_cmd_multiple "$cmd -filename $bdir/b" "" {
+        "/"
+        "\\ c/"
+        "\\ d/"
+    }
+    # When completing on a unique file, append a ' ' at the end.
+    test_gdb_complete_unique \
+        "$cmd -filename $bdir/b\\ c/d/" \
+        "$cmd -filename $bdir/b\\ c/d/e " ""
+
+    # Perform the same sequence with quoted filenames.
+
+    # Tab completion for a quoted path shows raw filenames.
+    test_gdb_complete_tab_multiple "$cmd -filename '$bdir/b" "" {
+        "b/"
+        "b c/"
+        "b d/"
+    }
+    # Using the COMPLETE command shows quoted filenames (without closing
+    # quote).
+    test_gdb_complete_cmd_multiple "$cmd -filename '$bdir/b" "" {
+        " c/" \
+        " d/" \
+        "/"
+    }
+    # When completing on a single option which is a file, append the
+    # closing quote as well as a space.
+    test_gdb_complete_unique \
+        "$cmd -filename '$bdir/b c/d/" \
+        "$cmd -filename '$bdir/b c/d/e' " ""
+
+    # Cleanup.
+    file delete -force "$bdir"
+}
+
 # Run the options framework tests first.
 foreach_with_prefix cmd {
     "require-delimiter"
@@ -1027,6 +1126,7 @@ foreach_with_prefix cmd {
     }
     test-enum $cmd
     test-string $cmd
+    test-filename $cmd
 }
 
 # Run the print integration tests, both as "standalone", and under
diff --git a/gdb/testsuite/lib/completion-support.exp b/gdb/testsuite/lib/completion-support.exp
index ddd3921977b..42f77887ca7 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
 }
-- 
2.29.2

Re: [PATCH 1/3] gdb::option: Add support for filename option.

[Andrew Burgess]

* Lancelot SIX via Gdb-patches <gdb-patches@sourceware.org> [2021-02-13 22:07:50 +0000]:

> This commit adds support for filename option for the gdb::option parsing
> framework.  It is meant to help the user enter values that could be
> parsed by built_argv.
> 
> Tab completion works as expected, i.e. similar to bash's behavior.
> 
> Considering the following tree structure:
> 
> somedir/
> ├── a folder

You probably want a trailing / after 'a folder' to make it clear it is
a folder, similar to how somedir/ has the trailing character.

> └── a file
> 
> The following completions are available:
> 
>     (gdb) maintenance test-options unknown-is-error -filename somedir/a\ <tab>
>     a file    a folder/
> 
> or
> 
>     (gdb) maintenance test-options unknown-is-error -filename "somedir/a <tab>
>     a file    a folder/
> 
> The complete command command also provides useful completions which are
> escaped or quoted depending on what the user did input initially.
> 
>     (gdb) complete maintenance test-options unknown-is-error -filename somedir/a
>     maintenance test-options unknown-is-error -filename somedir/a\ file
>     maintenance test-options unknown-is-error -filename somedir/a\ folder/
>     (gdb) complete maintenance test-options unknown-is-error -filename 'somedir/a
>     maintenance test-options unknown-is-error -filename 'somedir/a file
>     maintenance test-options unknown-is-error -filename 'somedir/a folder/
> 
> Note that since entering a path is an interactive process, the completer
> will not append the closing quote after a directory name. Instead it
> adds a '/' and expect the user to provide further input.
> 
> Having both of those behaviors at the same time requires that the
> completer knows if it has been called by readline after the user
> pressed the tab key or by some other mechanism.  In the first scenario it
> should display non quoted/escaped completions so the user sees actual
> filenames but complete quoted/escaped filenames.  In the second scenario
> it should only provide quoted/escaped completions.  For that purpose,
> this commit adds a m_from_readline property to the completion_tracker
> object and initializes it accordingly to the calling context.
> 
> This filename_completer has slightly different behavior than the one
> available in gdb/completer.c mainly with regard to escaping/quoting.
> The original completer is kept as is to maintain stable behavior for
> commands whose completer might depend on this implementation.  If this is
> desirable, I’ll happily replace the existing filename completer with the
> one currently proposed in the gdb::options::filename namespace instead
> of having both with slightly different behaviors (but I an not sure if
> users are relying on the current behavior of the filename
> completer).

I wonder if you could expand more on what the differences are, maybe
with some examples.  Your new code sounds better, and I don't know why
we would want to keep the existing code.

Ideally I'd like to see some examples of things that the old completer
allows us to do that would now break, or behave significantly
differently if we just changed over to your code.

> 
> It really feels that I have had to re-implement many existing readline
> behavior and I would be glad if someone has a simpler approach to obtain
> the same result.
> 
> All feedback are welcome.
> 
> gdb/ChangeLog:
> 
> 	* cli/cli-option.c (union option_value): Add filename option.
> 	(struct option_def_and_value): Free filename in dtor.
> 	(gdb::option::filename::quote_filename): New function.
> 	(gdb::option::filename::handle_completion): New function.
> 	(gdb::option::filename::filename_copleter): New function.
> 	(gdb::option::parse_option): Add support for filename.
> 	(gdb::option::save_option_value_in_ctx): Add support for filename.
> 	(gdb::option::get_val_type_str): Add support for filename.
> 	(gdb::option::add_setshow_cmds_for_options): Add support for filename.
> 	* cli/cli-option.h (struct option_def): Add filename field.
> 	(struct gdb::option::filename_option_def): Create.
> 	* completer.c (completion_tracker::discard_completions): Reset the
> 	m_from_readline flag.
> 	(gdb_completion_word_break_characters_throw): Set the m_from_readline
> 	flag on the completion_tracker.
> 	* completer.h (class completion_tracker): Add m_from_readline fileld
> 	and associated getter / setters.
> 	* maint-test-options.c (struct test_options_opts): Add a -filename
> 	option.
> 
> gdb/testsuite/ChangeLog:
> 
> 	* gdb.base/options.exp: Add tests for the filename option.
> 	* lib/completion-support.exp (make_tab_completion_list_re):
> 	Relax number of required spaced at the end of the pattern.
> ---
>  gdb/cli/cli-option.c                     | 250 +++++++++++++++++++++++
>  gdb/cli/cli-option.h                     |  20 ++
>  gdb/completer.c                          |   3 +
>  gdb/completer.h                          |  15 ++
>  gdb/maint-test-options.c                 |  15 +-
>  gdb/testsuite/gdb.base/options.exp       | 126 ++++++++++--
>  gdb/testsuite/lib/completion-support.exp |   1 -
>  7 files changed, 415 insertions(+), 15 deletions(-)
> 
> diff --git a/gdb/cli/cli-option.c b/gdb/cli/cli-option.c
> index ab76f194c3d..5a6c997dcda 100644
> --- a/gdb/cli/cli-option.c
> +++ b/gdb/cli/cli-option.c
> @@ -24,6 +24,8 @@
>  #include "cli/cli-setshow.h"
>  #include "command.h"
>  #include <vector>
> +#include <readline/readline.h>
> +
>  
>  namespace gdb {
>  namespace option {
> @@ -46,6 +48,9 @@ union option_value
>  
>    /* For var_string options.  This is malloc-allocated.  */
>    char *string;
> +
> +  /* For var_filename option. This is malloc-allocated.  */

Two spaces after the '.' here ^^^^^

> +  char *filename;
>  };
>  
>  /* Holds an options definition and its value.  */
> @@ -88,6 +93,8 @@ struct option_def_and_value
>        {
>  	if (option.type == var_string)
>  	  xfree (value->string);
> +	if (option.type == var_filename)
> +	  xfree (value->filename);
>        }
>    }
>  
> @@ -105,6 +112,8 @@ struct option_def_and_value
>        {
>  	if (option.type == var_string)
>  	  value->string = nullptr;
> +	if (option.type == var_filename)
> +	  value->filename = nullptr;
>        }
>    }
>  };
> @@ -164,6 +173,197 @@ complete_on_options (gdb::array_view<const option_def_group> options_group,
>  	}
>  }
>  
> +namespace filename {
> +
> +/* Quote the filename whose name is given by TEXT using quoting char
> +   QUOTECHAR. QUOTECHAR can be '\'', '"' or '\0'.
> +
> +   This function is meant to be used with a completion context, so the
> +   closing quote is not appended (we might still be waiting for the user
> +   to input more path components).  */
> +
> +static std::string
> +quote_filename (const char *text, char quotechar)
> +{
> +  std::string quoted;
> +  /* strlen (text) is a good first approximation of the resulting string
> +     length.  */
> +  quoted.reserve (strlen (text));
> +
> +  if (quotechar != '\0')
> +    {
> +      quoted.push_back (quotechar);
> +      while (*text != '\0')
> +	{
> +	  if (*text == quotechar)
> +	    quoted.push_back ('\\');
> +	  quoted.push_back (*text++);
> +	}
> +    }
> +  else
> +    {
> +      while (*text != '\0')
> +	{
> +	  /* Escape the space and quote chars.  */
> +	  if (*text == ' ' || *text == '\'' || *text == '"')
> +	    quoted.push_back ('\\');
> +	  quoted.push_back (*text++);
> +	}
> +    }
> +
> +  return quoted;
> +}
> +
> +/* Add the filename COMPLETION to TRACKER.  COMPLETION will be quoted
> +   using QUOTECHAR, but the closing quote will be dismissed if we expect
> +   more input from the user.
> +
> +   ONLY_COMPLETION is set to true to indicate that this function is called only
> +   once (i.e. only one possible completion).  In this case, if COMPLETION is not
> +   a directory, the closing quote is appended followed by a space to indicate
> +   that the next option is expected.
> +
> +   See completer_ftype for a description of TEXT and WORD.
> +
> +   The following table gives examples of how COMPLETION might be passed to
> +   TRACKER depending on QUOTECHAR
> +
> +    COMPLETION		QUOTECHAR   As registered in TRACKER
> +    "/home"		'\''	    "'/home/'"
> +    "/home"		'\0'	    "/home/"
> +    "/home"		'"'	    "\"/home/"
> +    "~/some folder"	'\''	    "'~/some folder/"
> +    "~/some folder"	'\0'	    "~/some\\ folder/"
> +    "~/some folder"	'"'	    "\"~/some folder/"
> +    "only_compl"	'\''	    "'only_compl' "
> +    "only_compl"	'\0'	    "only_compl "
> +    "only_compl"	'"'	    "\"only_compl\" "
> +   */
> +
> +static void
> +handle_completion
> +  (completion_tracker &tracker, std::string completion,
> +   const char quotechar, bool only_completion,
> +   const char *text, const char *word)

The GDB style would be to place the argument list on the same line as
'handle_completion', with line breaks after a comma.  We only start
the argument list on a line of its own if one argument is so long that
it alone will break the 80 character column rule.  So you can do:

handle_completion (completion_tracker &tracker, std::string completion,
		   const char quotechar, bool only_completion,
		   const char *text, const char *word)

Which is fine, but if you had:

handle_completion (completion_tracker &tracker, std::string completion,
		   const really_long_template_type<with_template_parameter, maybe_many> quotechar,
                   bool only_completion,
		   const char *text, const char *word)

Then you'd probably (a) want to use a typedef anyway, but (b) consider
starting the argument list on a line of its own.  This is true for
function definitions, declarations, and at call sites.

> +{
> +  gdb_assert (tracker.suppress_append_ws ());
> +  struct stat finfo;
> +  const bool isdir
> +    = stat (completion.c_str (), &finfo) == 0 && S_ISDIR (finfo.st_mode);
> +
> +  /* Readline would add a '/' for dirs, so do is ourselves if readline is not
> +     here.  */
> +  if (isdir && !tracker.from_readline ())
> +    completion.push_back ('/');
> +
> +  std::string quoted_completion = quote_filename
> +    (completion.c_str (), quotechar);
> +
> +  /* If we only have 1 possible completion which is not a dir, then we
> +     know the user will not add anything else.  We can close the quotation
> +     (if any) and append a space (tracker.suppress_append_ws () is true).  */
> +  if (only_completion && !isdir)
> +    {
> +      if (quotechar != '\0')
> +        quoted_completion.push_back (quotechar);
> +      quoted_completion.push_back (' ');
> +    }
> +
> +  /* Check if we got here by the user typing
> +     'complete [...] -filename ...' or '[...] -filename ...\t'
> +
> +     In the first case, we want the completion to show a actually
> +     quotted string, while in the latter case we want to complete on
> +     the quoted filename while showing the unquoted completion to the
> +     user.  */
> +  if (tracker.from_readline ())
> +    {
> +      completion_match_for_lcd match_for_lcd;
> +      match_for_lcd.set_match (quoted_completion.c_str ());
> +
> +      tracker.add_completion
> +        (std::move (gdb::unique_xmalloc_ptr<char>
> +                    (xstrdup (completion.c_str ()))), &match_for_lcd,
> +         text, word);
> +    }
> +  else
> +    tracker.add_completion
> +      (std::move (gdb::unique_xmalloc_ptr<char>
> +		  (xstrdup (quoted_completion.c_str ()))),
> +       nullptr, text, word);
> +
> +}
> +
> +/* Filename completer.  See completer_ftype for description of parameters.  */
> +
> +static void
> +complete_filename
> +  (completion_tracker &tracker, const char *text, const char *word)
> +{
> +  /* First call to rl_filename_completion_function should be 0, subsequent
> +     calls non 0.

I found this comment a little cryptic.  It's clearly true based on
looking at the code below, but the comment doesn't help me understand
why we're passing in a count.  I'm certainly in favour of comments,
but maybe this first part could be dropped, the following seems to
provide more detail.

> +
> +     We also track the number of calls done to rl_filename_completion_function.

Maybe you could just say:

  We track the number of calls to rl_filename_completion_function,
  with the count starting at 0.

> +     If it only return only one non null value a different processing should be
> +     done (see handle_completion for details).  */

Typo, 'If it only returns one non null ....'

> +  int call_count = 0;
> +  std::string first_completion;
> +
> +  /* The logic that tells if a space is to be appended after a completion is
> +     done in handle_completion, not delegated to the tracker.  */
> +  tracker.set_suppress_append_ws (true);
> +
> +  /* Keep track of what quoting mechanism the user used so we can re-quote
> +     completions accordingly.  */
> +  char quotechar;
> +  if (text[0] == '\'' || text[0] == '"')
> +    quotechar = text[0];
> +  else
> +    quotechar = '\0';
> +
> +  const char *arg_start = text;
> +  const std::string dequoted = extract_string_maybe_quoted (&arg_start);
> +
> +  try
> +    {
> +      while (true)
> +	{
> +	  gdb::unique_xmalloc_ptr<char> next_completion
> +	    (rl_filename_completion_function
> +	     (dequoted.c_str (), call_count++));
> +
> +	  if (next_completion == nullptr)
> +	    break;
> +
> +	  if (call_count == 1)
> +	    first_completion = next_completion.get ();
> +	  else
> +	    {
> +	      if (call_count == 2)
> +		handle_completion
> +		  (tracker, first_completion, quotechar, false, text,
> +		   word);
> +
> +	      handle_completion
> +		(tracker, next_completion.get (), quotechar, false, text,
> +		 word);
> +	    }
> +	}
> +
> +      /* If there was just 1 completion found, do readline job.  */

Typo: 'readline's job.'

> +      if (call_count == 2)
> +	handle_completion
> +	  (tracker, first_completion, quotechar, true, text, word);
> +    }
> +  catch (const gdb_exception_error &except)
> +    {
> +      if (except.error != MAX_COMPLETIONS_REACHED_ERROR)
> +        throw;
> +    }
> +}
> +
> +} /* namespace filename */
> +
>  /* See cli-option.h.  */
>  
>  void
> @@ -443,6 +643,40 @@ parse_option (gdb::array_view<const option_def_group> options_group,
>  	return option_def_and_value {*match, match_ctx, val};
>        }
>  
> +    case var_filename:
> +      {
> +	if (check_for_argument (args, "--"))
> +	  {
> +	    /* Treat "maint test-options -filename --" as if there
> +	       was no argument after "-filename".  */
> +	    error (_("-%s requires an argument"), match->name);
> +	  }
> +
> +	/* We are actually parsing the input, so advance the pointer to
> +	   after what have been parsed.  */
> +        const char *arg_start = *args;
> +	const std::string fname = extract_string_maybe_quoted (args);
> +
> +        /* If required, perform completion.  */
> +	if (completion != nullptr)
> +          {
> +	    if (**args == '\0')
> +              {
> +                gdb::option::filename::complete_filename
> +                  (completion->tracker, arg_start, arg_start);
> +
> +                return {};
> +              }
> +	  }
> +
> +	if (fname == "")
> +	  error (_("-%s requires an argument"), match->name);
> +
> +	option_value val;
> +	val.filename = xstrdup (fname.c_str ());
> +	return option_def_and_value {*match, match_ctx, val};
> +      }
> +
>      default:
>        /* Not yet.  */
>        gdb_assert_not_reached (_("option type not supported"));
> @@ -606,6 +840,11 @@ save_option_value_in_ctx (gdb::optional<option_def_and_value> &ov)
>  	= ov->value->string;
>        ov->value->string = nullptr;
>        break;
> +    case var_filename:
> +      *ov->option.var_address.filename (ov->option, ov->ctx)
> +	= ov->value->filename;
> +      ov->value->filename = nullptr;
> +      break;
>      default:
>        gdb_assert_not_reached ("unhandled option type");
>      }
> @@ -680,6 +919,8 @@ get_val_type_str (const option_def &opt, std::string &buffer)
>        }
>      case var_string:
>        return "STRING";
> +    case var_filename:
> +      return "FILENAME";
>      default:
>        return nullptr;
>      }
> @@ -824,6 +1065,15 @@ add_setshow_cmds_for_options (command_class cmd_class,
>  				  nullptr, option.show_cmd_cb,
>  				  set_list, show_list);
>  	}
> +      else if (option.type == var_filename)
> +	{
> +	  add_setshow_filename_cmd (option.name, cmd_class,
> +				    option.var_address.filename (option, data),
> +				    option.set_doc, option.show_doc,
> +				    option.help_doc,
> +				    nullptr, option.show_cmd_cb,
> +				    set_list, show_list);
> +	}
>        else
>  	gdb_assert_not_reached (_("option type not handled"));
>      }
> diff --git a/gdb/cli/cli-option.h b/gdb/cli/cli-option.h
> index aa2ccbed5d0..fc3aca887e7 100644
> --- a/gdb/cli/cli-option.h
> +++ b/gdb/cli/cli-option.h
> @@ -87,6 +87,7 @@ struct option_def
>        int *(*integer) (const option_def &, void *ctx);
>        const char **(*enumeration) (const option_def &, void *ctx);
>        char **(*string) (const option_def &, void *ctx);
> +      char **(*filename) (const option_def &, void *ctx);
>      }
>    var_address;
>  
> @@ -282,6 +283,25 @@ struct string_option_def : option_def
>    }
>  };
>  
> +/* A var_filename command line option.  */
> +
> +template <typename Context>
> +struct filename_option_def : option_def
> +{
> +  filename_option_def (const char *long_option_,
> +		       char **(*get_var_address_cb_) (Context *),
> +		       show_value_ftype *show_cmd_cb_,
> +		       const char *set_doc_,
> +		       const char *show_doc_ = nullptr,
> +		       const char *help_doc_ = nullptr)
> +    : option_def (long_option_, var_filename,
> +		  (erased_get_var_address_ftype *) get_var_address_cb_,
> +		  show_cmd_cb_, set_doc_, show_doc_, help_doc_)
> +  {
> +    var_address.enumeration = detail::get_var_address<const char *, Context>;
> +  }
> +};
> +
>  /* A group of options that all share the same context pointer to pass
>     to the options' get-current-value callbacks.  */
>  struct option_def_group
> diff --git a/gdb/completer.c b/gdb/completer.c
> index 2330ad435a8..0efc28de184 100644
> --- a/gdb/completer.c
> +++ b/gdb/completer.c
> @@ -1622,6 +1622,8 @@ completion_tracker::discard_completions ()
>  					   entry_hash_func, entry_eq_func,
>  					   completion_hash_entry::deleter,
>  					   xcalloc, xfree));
> +
> +  m_from_readline = false;
>  }
>  
>  /* See completer.h.  */
> @@ -2006,6 +2008,7 @@ gdb_completion_word_break_characters_throw ()
>       start afresh.  */
>    delete current_completion.tracker;
>    current_completion.tracker = new completion_tracker ();
> +  current_completion.tracker->set_from_readline (true);
>  
>    completion_tracker &tracker = *current_completion.tracker;
>  
> diff --git a/gdb/completer.h b/gdb/completer.h
> index 240490f0c05..d9ece352547 100644
> --- a/gdb/completer.h
> +++ b/gdb/completer.h
> @@ -405,6 +405,16 @@ class completion_tracker
>    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.  */
> +  void set_from_readline (bool from_readline)
> +  { m_from_readline = from_readline; }
> +
> +  /* 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.  */
> @@ -494,6 +504,11 @@ class completion_tracker
>       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/maint-test-options.c b/gdb/maint-test-options.c
> index 16ecd1dd7e5..93d62eecc47 100644
> --- a/gdb/maint-test-options.c
> +++ b/gdb/maint-test-options.c
> @@ -134,6 +134,7 @@ struct test_options_opts
>    unsigned int uint_opt = 0;
>    int zuint_unl_opt = 0;
>    char *string_opt = nullptr;
> +  char *filename_opt = nullptr;
>  
>    test_options_opts () = default;
>  
> @@ -150,7 +151,8 @@ struct test_options_opts
>    {
>      fprintf_unfiltered (file,
>  			_("-flag %d -xx1 %d -xx2 %d -bool %d "
> -			  "-enum %s -uint %s -zuint-unl %s -string '%s' -- %s\n"),
> +			  "-enum %s -uint %s -zuint-unl %s -string '%s' "
> +			  "-filename '%s' -- %s\n"),
>  			flag_opt,
>  			xx1_opt,
>  			xx2_opt,
> @@ -165,6 +167,9 @@ struct test_options_opts
>  			(string_opt != nullptr
>  			 ? string_opt
>  			 : ""),
> +			(filename_opt != nullptr
> +			 ? filename_opt
> +			 : ""),
>  			args);
>    }
>  };
> @@ -237,6 +242,14 @@ static const gdb::option::option_def test_options_option_defs[] = {
>      nullptr, /* show_cmd_cb */
>      N_("A string option."),
>    },
> +
> +  /* A filename option.  */
> +  gdb::option::filename_option_def<test_options_opts> {
> +    "filename",
> +    [] (test_options_opts *opts) { return &opts->filename_opt; },
> +    nullptr, /* show_cmd_cb */
> +    N_("A filename option."),
> +  },
>  };
>  
>  /* Create an option_def_group for the test_options_opts options, with
> diff --git a/gdb/testsuite/gdb.base/options.exp b/gdb/testsuite/gdb.base/options.exp
> index 44c773c3fa3..92b298064b2 100644
> --- a/gdb/testsuite/gdb.base/options.exp
> +++ b/gdb/testsuite/gdb.base/options.exp
> @@ -95,19 +95,19 @@ proc make_cmd {variant} {
>  # test-options xxx", with no flag/option set.  OPERAND is the expected
>  # operand.
>  proc expect_none {operand} {
> -    return "-flag 0 -xx1 0 -xx2 0 -bool 0 -enum xxx -uint 0 -zuint-unl 0 -string '' -- $operand"
> +    return "-flag 0 -xx1 0 -xx2 0 -bool 0 -enum xxx -uint 0 -zuint-unl 0 -string '' -filename '' -- $operand"
>  }
>  
>  # Return a string for the expected result of running "maint
>  # test-options xxx", with -flag set.  OPERAND is the expected operand.
>  proc expect_flag {operand} {
> -    return "-flag 1 -xx1 0 -xx2 0 -bool 0 -enum xxx -uint 0 -zuint-unl 0 -string '' -- $operand"
> +    return "-flag 1 -xx1 0 -xx2 0 -bool 0 -enum xxx -uint 0 -zuint-unl 0 -string '' -filename '' -- $operand"
>  }
>  
>  # Return a string for the expected result of running "maint
>  # test-options xxx", with -bool set.  OPERAND is the expected operand.
>  proc expect_bool {operand} {
> -    return "-flag 0 -xx1 0 -xx2 0 -bool 1 -enum xxx -uint 0 -zuint-unl 0 -string '' -- $operand"
> +    return "-flag 0 -xx1 0 -xx2 0 -bool 1 -enum xxx -uint 0 -zuint-unl 0 -string '' -filename '' -- $operand"
>  }
>  
>  # Return a string for the expected result of running "maint
> @@ -116,31 +116,46 @@ proc expect_bool {operand} {
>  # expected operand.
>  proc expect_integer {option val operand} {
>      if {$option == "uinteger"} {
> -	return "-flag 0 -xx1 0 -xx2 0 -bool 0 -enum xxx -uint $val -zuint-unl 0 -string '' -- $operand"
> +	return "-flag 0 -xx1 0 -xx2 0 -bool 0 -enum xxx -uint $val -zuint-unl 0 -string '' -filename '' -- $operand"
>      } elseif {$option == "zuinteger-unlimited"} {
> -	return "-flag 0 -xx1 0 -xx2 0 -bool 0 -enum xxx -uint 0 -zuint-unl $val -string '' -- $operand"
> +	return "-flag 0 -xx1 0 -xx2 0 -bool 0 -enum xxx -uint 0 -zuint-unl $val -string '' -filename '' -- $operand"
>      } else {
>  	error "unsupported option: $option"
>      }
>  }
>  
> -# Return a string for the expected result of running "maint
> -# test-options xxx", with -string set to $STR.  OPERAND is the
> -# expected operand.
> -proc expect_string {str operand} {
> -    # Dequote the string in the expected output.
> +# Helper function used to dequote string litterals in expected output.
> +proc dequote_expected_string {str} {
>      if { ( [string range $str 0 0] == "\""
>  	   && [string range $str end end] == "\"")
>  	 || ([string range $str 0 0] == "'"
>  	     && [string range $str end end] == "'")} {
> -	set str [string range $str 1 end-1]
> +	return [string range $str 1 end-1]
> +    } else {
> +	return $str
>      }
> -    return "-flag 0 -xx1 0 -xx2 0 -bool 0 -enum xxx -uint 0 -zuint-unl 0 -string '$str' -- $operand"
> +}
> +
> +# Return a string for the expected result of running "maint
> +# test-options xxx", with -string set to $STR.  OPERAND is the
> +# expected operand.
> +proc expect_string {str operand} {
> +    set str [dequote_expected_string $str]
> +    return "-flag 0 -xx1 0 -xx2 0 -bool 0 -enum xxx -uint 0 -zuint-unl 0 -string '$str' -filename '' -- $operand"
> +}
> +
> +# Return a string for the expected result of running "maint
> +# test-options xxx", with -filename set to $FILENAME.  OPERAND is the
> +# expected operand.
> +proc expect_filename {filename operand} {
> +    set filename [dequote_expected_string $filename]
> +    return "-flag 0 -xx1 0 -xx2 0 -bool 0 -enum xxx -uint 0 -zuint-unl 0 -string '' -filename '$filename' -- $operand"
>  }
>  
>  set all_options {
>      "-bool"
>      "-enum"
> +    "-filename"
>      "-flag"
>      "-string"
>      "-uinteger"
> @@ -594,7 +609,7 @@ proc_with_prefix test-flag {variant} {
>  
>      # Extract twice the same flag, separated by one space.
>      gdb_test "$cmd -xx1     -xx2 -xx1  -xx2 -xx1    -- non flags args" \
> -	"-flag 0 -xx1 1 -xx2 1 -bool 0 -enum xxx -uint 0 -zuint-unl 0 -string '' -- non flags args"
> +	"-flag 0 -xx1 1 -xx2 1 -bool 0 -enum xxx -uint 0 -zuint-unl 0 -string '' -filename '' -- non flags args"
>  
>      # Extract 2 known flags in front of unknown flags.
>      gdb_test "$cmd -xx1 -xx2 -a -b -c -xx1 --" \
> @@ -1013,6 +1028,90 @@ proc_with_prefix test-string {variant} {
>      }
>  }
>  
> +# filename option tests.
> +proc_with_prefix test-filename {variant} {
> +    global all_options
> +    set tmpdir [standard_output_file ""]
> +
> +    set cmd [make_cmd $variant]
> +
> +    gdb_test "$cmd -filename --"\
> +	"-filename requires an argument"
> +    if {$variant == "require-delimiter"} {
> +	gdb_test "$cmd -filename" [expect_none "-filename"]
> +    } else {
> +	gdb_test "$cmd -filename"\
> +	    "-filename requires an argument"
> +    }
> +
> +    # Check that dequoting is happening as expected (similar to string
> +    # dequoting).
> +    foreach_with_prefix str {
> +	"STR"
> +	"\"STR\""
> +	"\\\"STR"
> +	"'STR'"
> +	"\\'STR"
> +	"\"STR AAA\""
> +	"'STR BBB'"
> +	"\"STR 'CCC' DDD\""
> +	"'STR \"EEE\" FFF'"
> +	"\"STR \\\"GGG\\\" HHH\""
> +	"'STR \\\'III\\\' JJJ'"
> +    } {
> +	gdb_test "$cmd -filename ${str} --" [expect_filename "${str}" ""]
> +    }
> +
> +    # Create some tree structure we can use for the test.
> +    set bdir "$tmpdir/testfilename"
> +    file mkdir "$bdir/b"
> +    file mkdir "$bdir/b c"
> +    file mkdir "$bdir/b d"
> +    file mkdir "$bdir/b c/d"
> +    close [open "$bdir/b c/d/e" w]
> +
> +    # Tab completion for an escaped path.
> +    test_gdb_complete_tab_multiple "$cmd -filename $bdir/b" "" {
> +        "b/"
> +        "b c/"
> +        "b d/"
> +    }

I think you will need to add a mechanism to rename these tests to
avoid including paths in the test names.

The reason this is a problem is that some folk run a baseline GDB in
one directory and a feature GDB in a different directory, and then
compare the results.  Including paths in the test names makes it hard
to compare tests.

Many dejagnu test functions take an optional test name.  These
completion functions don't, but there's no reason why they couldn't.
Then you just need to pass in a descriptive test name that doesn't
include the test path.

Thanks for looking at this.  I'd like to investigate the possibility
of merging your filename completion with the existing completion - or
at least make sure we understand why we don't want to do that.

Thanks,
Andrew

> +    # Checking completions with the COMPLETE command shows escaped filenames.
> +    test_gdb_complete_cmd_multiple "$cmd -filename $bdir/b" "" {
> +        "/"
> +        "\\ c/"
> +        "\\ d/"
> +    }
> +    # When completing on a unique file, append a ' ' at the end.
> +    test_gdb_complete_unique \
> +        "$cmd -filename $bdir/b\\ c/d/" \
> +        "$cmd -filename $bdir/b\\ c/d/e " ""
> +
> +    # Perform the same sequence with quoted filenames.
> +
> +    # Tab completion for a quoted path shows raw filenames.
> +    test_gdb_complete_tab_multiple "$cmd -filename '$bdir/b" "" {
> +        "b/"
> +        "b c/"
> +        "b d/"
> +    }
> +    # Using the COMPLETE command shows quoted filenames (without closing
> +    # quote).
> +    test_gdb_complete_cmd_multiple "$cmd -filename '$bdir/b" "" {
> +        " c/" \
> +        " d/" \
> +        "/"
> +    }
> +    # When completing on a single option which is a file, append the
> +    # closing quote as well as a space.
> +    test_gdb_complete_unique \
> +        "$cmd -filename '$bdir/b c/d/" \
> +        "$cmd -filename '$bdir/b c/d/e' " ""
> +
> +    # Cleanup.
> +    file delete -force "$bdir"
> +}
> +
>  # Run the options framework tests first.
>  foreach_with_prefix cmd {
>      "require-delimiter"
> @@ -1027,6 +1126,7 @@ foreach_with_prefix cmd {
>      }
>      test-enum $cmd
>      test-string $cmd
> +    test-filename $cmd
>  }
>  
>  # Run the print integration tests, both as "standalone", and under
> diff --git a/gdb/testsuite/lib/completion-support.exp b/gdb/testsuite/lib/completion-support.exp
> index ddd3921977b..42f77887ca7 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
>  }
> -- 
> 2.29.2
> 

Re: [PATCH 1/3] gdb::option: Add support for filename option.

[Lancelot SIX via Gdb-patches]

Le Tue, Feb 16, 2021 at 05:45:01PM +0000, Andrew Burgess a écrit :
> * Lancelot SIX via Gdb-patches <gdb-patches@sourceware.org> [2021-02-13 22:07:50 +0000]:
> 
> > This commit adds support for filename option for the gdb::option parsing
> > framework.  It is meant to help the user enter values that could be
> > parsed by built_argv.
> > 
> > Tab completion works as expected, i.e. similar to bash's behavior.
> > 
> > Considering the following tree structure:
> > 
> > somedir/
> > ├── a folder
> 
> You probably want a trailing / after 'a folder' to make it clear it is
> a folder, similar to how somedir/ has the trailing character.
> 
> > └── a file
> > 
> > The following completions are available:
> > 
> >     (gdb) maintenance test-options unknown-is-error -filename somedir/a\ <tab>
> >     a file    a folder/
> > 
> > or
> > 
> >     (gdb) maintenance test-options unknown-is-error -filename "somedir/a <tab>
> >     a file    a folder/
> > 
> > The complete command command also provides useful completions which are
> > escaped or quoted depending on what the user did input initially.
> > 
> >     (gdb) complete maintenance test-options unknown-is-error -filename somedir/a
> >     maintenance test-options unknown-is-error -filename somedir/a\ file
> >     maintenance test-options unknown-is-error -filename somedir/a\ folder/
> >     (gdb) complete maintenance test-options unknown-is-error -filename 'somedir/a
> >     maintenance test-options unknown-is-error -filename 'somedir/a file
> >     maintenance test-options unknown-is-error -filename 'somedir/a folder/
> > 
> > Note that since entering a path is an interactive process, the completer
> > will not append the closing quote after a directory name. Instead it
> > adds a '/' and expect the user to provide further input.
> > 
> > Having both of those behaviors at the same time requires that the
> > completer knows if it has been called by readline after the user
> > pressed the tab key or by some other mechanism.  In the first scenario it
> > should display non quoted/escaped completions so the user sees actual
> > filenames but complete quoted/escaped filenames.  In the second scenario
> > it should only provide quoted/escaped completions.  For that purpose,
> > this commit adds a m_from_readline property to the completion_tracker
> > object and initializes it accordingly to the calling context.
> > 
> > This filename_completer has slightly different behavior than the one
> > available in gdb/completer.c mainly with regard to escaping/quoting.
> > The original completer is kept as is to maintain stable behavior for
> > commands whose completer might depend on this implementation.  If this is
> > desirable, I’ll happily replace the existing filename completer with the
> > one currently proposed in the gdb::options::filename namespace instead
> > of having both with slightly different behaviors (but I an not sure if
> > users are relying on the current behavior of the filename
> > completer).
> 
> I wonder if you could expand more on what the differences are, maybe
> with some examples.  Your new code sounds better, and I don't know why
> we would want to keep the existing code.
> 
The current completer does not handle spaces nor any
escape mechanism.

> Ideally I'd like to see some examples of things that the old completer
> allows us to do that would now break, or behave significantly
> differently if we just changed over to your code.
> 

The only case I see where using the new completer would break things is
a command which does not use built_argv to parse the argument line:

static void
foo_command (const char *args, int from_tty)
{
  /* Handle args as a raw filename, or consider that everything that
     follows a '--' is a filename.  */
  const char *filename = args;
  foo(filename);
}

Other than that, using a completer that handles space escaping or
quoting would only improve things.

I have not tracked down usages of the current completer to make sure no
command ingests the filename without any sort of processing.  I’ll do that
before sending a v2 of this patch series.

> > 
> > It really feels that I have had to re-implement many existing readline
> > behavior and I would be glad if someone has a simpler approach to obtain
> > the same result.
> > 
> > All feedback are welcome.
> > 
> > gdb/ChangeLog:
> > 
> > 	* cli/cli-option.c (union option_value): Add filename option.
> > 	(struct option_def_and_value): Free filename in dtor.
> > 	(gdb::option::filename::quote_filename): New function.
> > 	(gdb::option::filename::handle_completion): New function.
> > 	(gdb::option::filename::filename_copleter): New function.
> > 	(gdb::option::parse_option): Add support for filename.
> > 	(gdb::option::save_option_value_in_ctx): Add support for filename.
> > 	(gdb::option::get_val_type_str): Add support for filename.
> > 	(gdb::option::add_setshow_cmds_for_options): Add support for filename.
> > 	* cli/cli-option.h (struct option_def): Add filename field.
> > 	(struct gdb::option::filename_option_def): Create.
> > 	* completer.c (completion_tracker::discard_completions): Reset the
> > 	m_from_readline flag.
> > 	(gdb_completion_word_break_characters_throw): Set the m_from_readline
> > 	flag on the completion_tracker.
> > 	* completer.h (class completion_tracker): Add m_from_readline fileld
> > 	and associated getter / setters.
> > 	* maint-test-options.c (struct test_options_opts): Add a -filename
> > 	option.
> > 
> > gdb/testsuite/ChangeLog:
> > 
> > 	* gdb.base/options.exp: Add tests for the filename option.
> > 	* lib/completion-support.exp (make_tab_completion_list_re):
> > 	Relax number of required spaced at the end of the pattern.
> > ---
> >  gdb/cli/cli-option.c                     | 250 +++++++++++++++++++++++
> >  gdb/cli/cli-option.h                     |  20 ++
> >  gdb/completer.c                          |   3 +
> >  gdb/completer.h                          |  15 ++
> >  gdb/maint-test-options.c                 |  15 +-
> >  gdb/testsuite/gdb.base/options.exp       | 126 ++++++++++--
> >  gdb/testsuite/lib/completion-support.exp |   1 -
> >  7 files changed, 415 insertions(+), 15 deletions(-)
> > 
> > diff --git a/gdb/cli/cli-option.c b/gdb/cli/cli-option.c
> > index ab76f194c3d..5a6c997dcda 100644
> > --- a/gdb/cli/cli-option.c
> > +++ b/gdb/cli/cli-option.c
> > @@ -24,6 +24,8 @@
> >  #include "cli/cli-setshow.h"
> >  #include "command.h"
> >  #include <vector>
> > +#include <readline/readline.h>
> > +
> >  
> >  namespace gdb {
> >  namespace option {
> > @@ -46,6 +48,9 @@ union option_value
> >  
> >    /* For var_string options.  This is malloc-allocated.  */
> >    char *string;
> > +
> > +  /* For var_filename option. This is malloc-allocated.  */
> 
> Two spaces after the '.' here ^^^^^
> 
> > +  char *filename;
> >  };
> >  
> >  /* Holds an options definition and its value.  */
> > @@ -88,6 +93,8 @@ struct option_def_and_value
> >        {
> >  	if (option.type == var_string)
> >  	  xfree (value->string);
> > +	if (option.type == var_filename)
> > +	  xfree (value->filename);
> >        }
> >    }
> >  
> > @@ -105,6 +112,8 @@ struct option_def_and_value
> >        {
> >  	if (option.type == var_string)
> >  	  value->string = nullptr;
> > +	if (option.type == var_filename)
> > +	  value->filename = nullptr;
> >        }
> >    }
> >  };
> > @@ -164,6 +173,197 @@ complete_on_options (gdb::array_view<const option_def_group> options_group,
> >  	}
> >  }
> >  
> > +namespace filename {
> > +
> > +/* Quote the filename whose name is given by TEXT using quoting char
> > +   QUOTECHAR. QUOTECHAR can be '\'', '"' or '\0'.
> > +
> > +   This function is meant to be used with a completion context, so the
> > +   closing quote is not appended (we might still be waiting for the user
> > +   to input more path components).  */
> > +
> > +static std::string
> > +quote_filename (const char *text, char quotechar)
> > +{
> > +  std::string quoted;
> > +  /* strlen (text) is a good first approximation of the resulting string
> > +     length.  */
> > +  quoted.reserve (strlen (text));
> > +
> > +  if (quotechar != '\0')
> > +    {
> > +      quoted.push_back (quotechar);
> > +      while (*text != '\0')
> > +	{
> > +	  if (*text == quotechar)
> > +	    quoted.push_back ('\\');
> > +	  quoted.push_back (*text++);
> > +	}
> > +    }
> > +  else
> > +    {
> > +      while (*text != '\0')
> > +	{
> > +	  /* Escape the space and quote chars.  */
> > +	  if (*text == ' ' || *text == '\'' || *text == '"')
> > +	    quoted.push_back ('\\');
> > +	  quoted.push_back (*text++);
> > +	}
> > +    }
> > +
> > +  return quoted;
> > +}
> > +
> > +/* Add the filename COMPLETION to TRACKER.  COMPLETION will be quoted
> > +   using QUOTECHAR, but the closing quote will be dismissed if we expect
> > +   more input from the user.
> > +
> > +   ONLY_COMPLETION is set to true to indicate that this function is called only
> > +   once (i.e. only one possible completion).  In this case, if COMPLETION is not
> > +   a directory, the closing quote is appended followed by a space to indicate
> > +   that the next option is expected.
> > +
> > +   See completer_ftype for a description of TEXT and WORD.
> > +
> > +   The following table gives examples of how COMPLETION might be passed to
> > +   TRACKER depending on QUOTECHAR
> > +
> > +    COMPLETION		QUOTECHAR   As registered in TRACKER
> > +    "/home"		'\''	    "'/home/'"
> > +    "/home"		'\0'	    "/home/"
> > +    "/home"		'"'	    "\"/home/"
> > +    "~/some folder"	'\''	    "'~/some folder/"
> > +    "~/some folder"	'\0'	    "~/some\\ folder/"
> > +    "~/some folder"	'"'	    "\"~/some folder/"
> > +    "only_compl"	'\''	    "'only_compl' "
> > +    "only_compl"	'\0'	    "only_compl "
> > +    "only_compl"	'"'	    "\"only_compl\" "
> > +   */
> > +
> > +static void
> > +handle_completion
> > +  (completion_tracker &tracker, std::string completion,
> > +   const char quotechar, bool only_completion,
> > +   const char *text, const char *word)
> 
> The GDB style would be to place the argument list on the same line as
> 'handle_completion', with line breaks after a comma.  We only start
> the argument list on a line of its own if one argument is so long that
> it alone will break the 80 character column rule.  So you can do:
> 
> handle_completion (completion_tracker &tracker, std::string completion,
> 		   const char quotechar, bool only_completion,
> 		   const char *text, const char *word)
> 
> Which is fine, but if you had:
> 
> handle_completion (completion_tracker &tracker, std::string completion,
> 		   const really_long_template_type<with_template_parameter, maybe_many> quotechar,
>                    bool only_completion,
> 		   const char *text, const char *word)
> 
> Then you'd probably (a) want to use a typedef anyway, but (b) consider
> starting the argument list on a line of its own.  This is true for
> function definitions, declarations, and at call sites.
> 

Noted. Thanks for the explanations.

> > +{
> > +  gdb_assert (tracker.suppress_append_ws ());
> > +  struct stat finfo;
> > +  const bool isdir
> > +    = stat (completion.c_str (), &finfo) == 0 && S_ISDIR (finfo.st_mode);
> > +
> > +  /* Readline would add a '/' for dirs, so do is ourselves if readline is not
> > +     here.  */
> > +  if (isdir && !tracker.from_readline ())
> > +    completion.push_back ('/');
> > +
> > +  std::string quoted_completion = quote_filename
> > +    (completion.c_str (), quotechar);
> > +
> > +  /* If we only have 1 possible completion which is not a dir, then we
> > +     know the user will not add anything else.  We can close the quotation
> > +     (if any) and append a space (tracker.suppress_append_ws () is true).  */
> > +  if (only_completion && !isdir)
> > +    {
> > +      if (quotechar != '\0')
> > +        quoted_completion.push_back (quotechar);
> > +      quoted_completion.push_back (' ');
> > +    }
> > +
> > +  /* Check if we got here by the user typing
> > +     'complete [...] -filename ...' or '[...] -filename ...\t'
> > +
> > +     In the first case, we want the completion to show a actually
> > +     quotted string, while in the latter case we want to complete on
> > +     the quoted filename while showing the unquoted completion to the
> > +     user.  */
> > +  if (tracker.from_readline ())
> > +    {
> > +      completion_match_for_lcd match_for_lcd;
> > +      match_for_lcd.set_match (quoted_completion.c_str ());
> > +
> > +      tracker.add_completion
> > +        (std::move (gdb::unique_xmalloc_ptr<char>
> > +                    (xstrdup (completion.c_str ()))), &match_for_lcd,
> > +         text, word);
> > +    }
> > +  else
> > +    tracker.add_completion
> > +      (std::move (gdb::unique_xmalloc_ptr<char>
> > +		  (xstrdup (quoted_completion.c_str ()))),
> > +       nullptr, text, word);
> > +
> > +}
> > +
> > +/* Filename completer.  See completer_ftype for description of parameters.  */
> > +
> > +static void
> > +complete_filename
> > +  (completion_tracker &tracker, const char *text, const char *word)
> > +{
> > +  /* First call to rl_filename_completion_function should be 0, subsequent
> > +     calls non 0.
> 
> I found this comment a little cryptic.  It's clearly true based on
> looking at the code below, but the comment doesn't help me understand
> why we're passing in a count.  I'm certainly in favour of comments,
> but maybe this first part could be dropped, the following seems to
> provide more detail.
> 

It comes down to readline keeping some state between two calls.  The
first time you call it, you want readline to open the directory to list
its content and return the first item, and have subsequent calls just
return one more item.

I’ll update the comment.

> > +
> > +     We also track the number of calls done to rl_filename_completion_function.
> 
> Maybe you could just say:
> 
>   We track the number of calls to rl_filename_completion_function,
>   with the count starting at 0.
> 
> > +     If it only return only one non null value a different processing should be
> > +     done (see handle_completion for details).  */
> 
> Typo, 'If it only returns one non null ....'
> 
> > +  int call_count = 0;
> > +  std::string first_completion;
> > +
> > +  /* The logic that tells if a space is to be appended after a completion is
> > +     done in handle_completion, not delegated to the tracker.  */
> > +  tracker.set_suppress_append_ws (true);
> > +
> > +  /* Keep track of what quoting mechanism the user used so we can re-quote
> > +     completions accordingly.  */
> > +  char quotechar;
> > +  if (text[0] == '\'' || text[0] == '"')
> > +    quotechar = text[0];
> > +  else
> > +    quotechar = '\0';
> > +
> > +  const char *arg_start = text;
> > +  const std::string dequoted = extract_string_maybe_quoted (&arg_start);
> > +
> > +  try
> > +    {
> > +      while (true)
> > +	{
> > +	  gdb::unique_xmalloc_ptr<char> next_completion
> > +	    (rl_filename_completion_function
> > +	     (dequoted.c_str (), call_count++));
> > +
> > +	  if (next_completion == nullptr)
> > +	    break;
> > +
> > +	  if (call_count == 1)
> > +	    first_completion = next_completion.get ();
> > +	  else
> > +	    {
> > +	      if (call_count == 2)
> > +		handle_completion
> > +		  (tracker, first_completion, quotechar, false, text,
> > +		   word);
> > +
> > +	      handle_completion
> > +		(tracker, next_completion.get (), quotechar, false, text,
> > +		 word);
> > +	    }
> > +	}
> > +
> > +      /* If there was just 1 completion found, do readline job.  */
> 
> Typo: 'readline's job.'
> 
> > +      if (call_count == 2)
> > +	handle_completion
> > +	  (tracker, first_completion, quotechar, true, text, word);
> > +    }
> > +  catch (const gdb_exception_error &except)
> > +    {
> > +      if (except.error != MAX_COMPLETIONS_REACHED_ERROR)
> > +        throw;
> > +    }
> > +}
> > +
> > +} /* namespace filename */
> > +
> >  /* See cli-option.h.  */
> >  
> >  void
> > @@ -443,6 +643,40 @@ parse_option (gdb::array_view<const option_def_group> options_group,
> >  	return option_def_and_value {*match, match_ctx, val};
> >        }
> >  
> > +    case var_filename:
> > +      {
> > +	if (check_for_argument (args, "--"))
> > +	  {
> > +	    /* Treat "maint test-options -filename --" as if there
> > +	       was no argument after "-filename".  */
> > +	    error (_("-%s requires an argument"), match->name);
> > +	  }
> > +
> > +	/* We are actually parsing the input, so advance the pointer to
> > +	   after what have been parsed.  */
> > +        const char *arg_start = *args;
> > +	const std::string fname = extract_string_maybe_quoted (args);
> > +
> > +        /* If required, perform completion.  */
> > +	if (completion != nullptr)
> > +          {
> > +	    if (**args == '\0')
> > +              {
> > +                gdb::option::filename::complete_filename
> > +                  (completion->tracker, arg_start, arg_start);
> > +
> > +                return {};
> > +              }
> > +	  }
> > +
> > +	if (fname == "")
> > +	  error (_("-%s requires an argument"), match->name);
> > +
> > +	option_value val;
> > +	val.filename = xstrdup (fname.c_str ());
> > +	return option_def_and_value {*match, match_ctx, val};
> > +      }
> > +
> >      default:
> >        /* Not yet.  */
> >        gdb_assert_not_reached (_("option type not supported"));
> > @@ -606,6 +840,11 @@ save_option_value_in_ctx (gdb::optional<option_def_and_value> &ov)
> >  	= ov->value->string;
> >        ov->value->string = nullptr;
> >        break;
> > +    case var_filename:
> > +      *ov->option.var_address.filename (ov->option, ov->ctx)
> > +	= ov->value->filename;
> > +      ov->value->filename = nullptr;
> > +      break;
> >      default:
> >        gdb_assert_not_reached ("unhandled option type");
> >      }
> > @@ -680,6 +919,8 @@ get_val_type_str (const option_def &opt, std::string &buffer)
> >        }
> >      case var_string:
> >        return "STRING";
> > +    case var_filename:
> > +      return "FILENAME";
> >      default:
> >        return nullptr;
> >      }
> > @@ -824,6 +1065,15 @@ add_setshow_cmds_for_options (command_class cmd_class,
> >  				  nullptr, option.show_cmd_cb,
> >  				  set_list, show_list);
> >  	}
> > +      else if (option.type == var_filename)
> > +	{
> > +	  add_setshow_filename_cmd (option.name, cmd_class,
> > +				    option.var_address.filename (option, data),
> > +				    option.set_doc, option.show_doc,
> > +				    option.help_doc,
> > +				    nullptr, option.show_cmd_cb,
> > +				    set_list, show_list);
> > +	}
> >        else
> >  	gdb_assert_not_reached (_("option type not handled"));
> >      }
> > diff --git a/gdb/cli/cli-option.h b/gdb/cli/cli-option.h
> > index aa2ccbed5d0..fc3aca887e7 100644
> > --- a/gdb/cli/cli-option.h
> > +++ b/gdb/cli/cli-option.h
> > @@ -87,6 +87,7 @@ struct option_def
> >        int *(*integer) (const option_def &, void *ctx);
> >        const char **(*enumeration) (const option_def &, void *ctx);
> >        char **(*string) (const option_def &, void *ctx);
> > +      char **(*filename) (const option_def &, void *ctx);
> >      }
> >    var_address;
> >  
> > @@ -282,6 +283,25 @@ struct string_option_def : option_def
> >    }
> >  };
> >  
> > +/* A var_filename command line option.  */
> > +
> > +template <typename Context>
> > +struct filename_option_def : option_def
> > +{
> > +  filename_option_def (const char *long_option_,
> > +		       char **(*get_var_address_cb_) (Context *),
> > +		       show_value_ftype *show_cmd_cb_,
> > +		       const char *set_doc_,
> > +		       const char *show_doc_ = nullptr,
> > +		       const char *help_doc_ = nullptr)
> > +    : option_def (long_option_, var_filename,
> > +		  (erased_get_var_address_ftype *) get_var_address_cb_,
> > +		  show_cmd_cb_, set_doc_, show_doc_, help_doc_)
> > +  {
> > +    var_address.enumeration = detail::get_var_address<const char *, Context>;
> > +  }
> > +};
> > +
> >  /* A group of options that all share the same context pointer to pass
> >     to the options' get-current-value callbacks.  */
> >  struct option_def_group
> > diff --git a/gdb/completer.c b/gdb/completer.c
> > index 2330ad435a8..0efc28de184 100644
> > --- a/gdb/completer.c
> > +++ b/gdb/completer.c
> > @@ -1622,6 +1622,8 @@ completion_tracker::discard_completions ()
> >  					   entry_hash_func, entry_eq_func,
> >  					   completion_hash_entry::deleter,
> >  					   xcalloc, xfree));
> > +
> > +  m_from_readline = false;
> >  }
> >  
> >  /* See completer.h.  */
> > @@ -2006,6 +2008,7 @@ gdb_completion_word_break_characters_throw ()
> >       start afresh.  */
> >    delete current_completion.tracker;
> >    current_completion.tracker = new completion_tracker ();
> > +  current_completion.tracker->set_from_readline (true);
> >  
> >    completion_tracker &tracker = *current_completion.tracker;
> >  
> > diff --git a/gdb/completer.h b/gdb/completer.h
> > index 240490f0c05..d9ece352547 100644
> > --- a/gdb/completer.h
> > +++ b/gdb/completer.h
> > @@ -405,6 +405,16 @@ class completion_tracker
> >    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.  */
> > +  void set_from_readline (bool from_readline)
> > +  { m_from_readline = from_readline; }
> > +
> > +  /* 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.  */
> > @@ -494,6 +504,11 @@ class completion_tracker
> >       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/maint-test-options.c b/gdb/maint-test-options.c
> > index 16ecd1dd7e5..93d62eecc47 100644
> > --- a/gdb/maint-test-options.c
> > +++ b/gdb/maint-test-options.c
> > @@ -134,6 +134,7 @@ struct test_options_opts
> >    unsigned int uint_opt = 0;
> >    int zuint_unl_opt = 0;
> >    char *string_opt = nullptr;
> > +  char *filename_opt = nullptr;
> >  
> >    test_options_opts () = default;
> >  
> > @@ -150,7 +151,8 @@ struct test_options_opts
> >    {
> >      fprintf_unfiltered (file,
> >  			_("-flag %d -xx1 %d -xx2 %d -bool %d "
> > -			  "-enum %s -uint %s -zuint-unl %s -string '%s' -- %s\n"),
> > +			  "-enum %s -uint %s -zuint-unl %s -string '%s' "
> > +			  "-filename '%s' -- %s\n"),
> >  			flag_opt,
> >  			xx1_opt,
> >  			xx2_opt,
> > @@ -165,6 +167,9 @@ struct test_options_opts
> >  			(string_opt != nullptr
> >  			 ? string_opt
> >  			 : ""),
> > +			(filename_opt != nullptr
> > +			 ? filename_opt
> > +			 : ""),
> >  			args);
> >    }
> >  };
> > @@ -237,6 +242,14 @@ static const gdb::option::option_def test_options_option_defs[] = {
> >      nullptr, /* show_cmd_cb */
> >      N_("A string option."),
> >    },
> > +
> > +  /* A filename option.  */
> > +  gdb::option::filename_option_def<test_options_opts> {
> > +    "filename",
> > +    [] (test_options_opts *opts) { return &opts->filename_opt; },
> > +    nullptr, /* show_cmd_cb */
> > +    N_("A filename option."),
> > +  },
> >  };
> >  
> >  /* Create an option_def_group for the test_options_opts options, with
> > diff --git a/gdb/testsuite/gdb.base/options.exp b/gdb/testsuite/gdb.base/options.exp
> > index 44c773c3fa3..92b298064b2 100644
> > --- a/gdb/testsuite/gdb.base/options.exp
> > +++ b/gdb/testsuite/gdb.base/options.exp
> > @@ -95,19 +95,19 @@ proc make_cmd {variant} {
> >  # test-options xxx", with no flag/option set.  OPERAND is the expected
> >  # operand.
> >  proc expect_none {operand} {
> > -    return "-flag 0 -xx1 0 -xx2 0 -bool 0 -enum xxx -uint 0 -zuint-unl 0 -string '' -- $operand"
> > +    return "-flag 0 -xx1 0 -xx2 0 -bool 0 -enum xxx -uint 0 -zuint-unl 0 -string '' -filename '' -- $operand"
> >  }
> >  
> >  # Return a string for the expected result of running "maint
> >  # test-options xxx", with -flag set.  OPERAND is the expected operand.
> >  proc expect_flag {operand} {
> > -    return "-flag 1 -xx1 0 -xx2 0 -bool 0 -enum xxx -uint 0 -zuint-unl 0 -string '' -- $operand"
> > +    return "-flag 1 -xx1 0 -xx2 0 -bool 0 -enum xxx -uint 0 -zuint-unl 0 -string '' -filename '' -- $operand"
> >  }
> >  
> >  # Return a string for the expected result of running "maint
> >  # test-options xxx", with -bool set.  OPERAND is the expected operand.
> >  proc expect_bool {operand} {
> > -    return "-flag 0 -xx1 0 -xx2 0 -bool 1 -enum xxx -uint 0 -zuint-unl 0 -string '' -- $operand"
> > +    return "-flag 0 -xx1 0 -xx2 0 -bool 1 -enum xxx -uint 0 -zuint-unl 0 -string '' -filename '' -- $operand"
> >  }
> >  
> >  # Return a string for the expected result of running "maint
> > @@ -116,31 +116,46 @@ proc expect_bool {operand} {
> >  # expected operand.
> >  proc expect_integer {option val operand} {
> >      if {$option == "uinteger"} {
> > -	return "-flag 0 -xx1 0 -xx2 0 -bool 0 -enum xxx -uint $val -zuint-unl 0 -string '' -- $operand"
> > +	return "-flag 0 -xx1 0 -xx2 0 -bool 0 -enum xxx -uint $val -zuint-unl 0 -string '' -filename '' -- $operand"
> >      } elseif {$option == "zuinteger-unlimited"} {
> > -	return "-flag 0 -xx1 0 -xx2 0 -bool 0 -enum xxx -uint 0 -zuint-unl $val -string '' -- $operand"
> > +	return "-flag 0 -xx1 0 -xx2 0 -bool 0 -enum xxx -uint 0 -zuint-unl $val -string '' -filename '' -- $operand"
> >      } else {
> >  	error "unsupported option: $option"
> >      }
> >  }
> >  
> > -# Return a string for the expected result of running "maint
> > -# test-options xxx", with -string set to $STR.  OPERAND is the
> > -# expected operand.
> > -proc expect_string {str operand} {
> > -    # Dequote the string in the expected output.
> > +# Helper function used to dequote string litterals in expected output.
> > +proc dequote_expected_string {str} {
> >      if { ( [string range $str 0 0] == "\""
> >  	   && [string range $str end end] == "\"")
> >  	 || ([string range $str 0 0] == "'"
> >  	     && [string range $str end end] == "'")} {
> > -	set str [string range $str 1 end-1]
> > +	return [string range $str 1 end-1]
> > +    } else {
> > +	return $str
> >      }
> > -    return "-flag 0 -xx1 0 -xx2 0 -bool 0 -enum xxx -uint 0 -zuint-unl 0 -string '$str' -- $operand"
> > +}
> > +
> > +# Return a string for the expected result of running "maint
> > +# test-options xxx", with -string set to $STR.  OPERAND is the
> > +# expected operand.
> > +proc expect_string {str operand} {
> > +    set str [dequote_expected_string $str]
> > +    return "-flag 0 -xx1 0 -xx2 0 -bool 0 -enum xxx -uint 0 -zuint-unl 0 -string '$str' -filename '' -- $operand"
> > +}
> > +
> > +# Return a string for the expected result of running "maint
> > +# test-options xxx", with -filename set to $FILENAME.  OPERAND is the
> > +# expected operand.
> > +proc expect_filename {filename operand} {
> > +    set filename [dequote_expected_string $filename]
> > +    return "-flag 0 -xx1 0 -xx2 0 -bool 0 -enum xxx -uint 0 -zuint-unl 0 -string '' -filename '$filename' -- $operand"
> >  }
> >  
> >  set all_options {
> >      "-bool"
> >      "-enum"
> > +    "-filename"
> >      "-flag"
> >      "-string"
> >      "-uinteger"
> > @@ -594,7 +609,7 @@ proc_with_prefix test-flag {variant} {
> >  
> >      # Extract twice the same flag, separated by one space.
> >      gdb_test "$cmd -xx1     -xx2 -xx1  -xx2 -xx1    -- non flags args" \
> > -	"-flag 0 -xx1 1 -xx2 1 -bool 0 -enum xxx -uint 0 -zuint-unl 0 -string '' -- non flags args"
> > +	"-flag 0 -xx1 1 -xx2 1 -bool 0 -enum xxx -uint 0 -zuint-unl 0 -string '' -filename '' -- non flags args"
> >  
> >      # Extract 2 known flags in front of unknown flags.
> >      gdb_test "$cmd -xx1 -xx2 -a -b -c -xx1 --" \
> > @@ -1013,6 +1028,90 @@ proc_with_prefix test-string {variant} {
> >      }
> >  }
> >  
> > +# filename option tests.
> > +proc_with_prefix test-filename {variant} {
> > +    global all_options
> > +    set tmpdir [standard_output_file ""]
> > +
> > +    set cmd [make_cmd $variant]
> > +
> > +    gdb_test "$cmd -filename --"\
> > +	"-filename requires an argument"
> > +    if {$variant == "require-delimiter"} {
> > +	gdb_test "$cmd -filename" [expect_none "-filename"]
> > +    } else {
> > +	gdb_test "$cmd -filename"\
> > +	    "-filename requires an argument"
> > +    }
> > +
> > +    # Check that dequoting is happening as expected (similar to string
> > +    # dequoting).
> > +    foreach_with_prefix str {
> > +	"STR"
> > +	"\"STR\""
> > +	"\\\"STR"
> > +	"'STR'"
> > +	"\\'STR"
> > +	"\"STR AAA\""
> > +	"'STR BBB'"
> > +	"\"STR 'CCC' DDD\""
> > +	"'STR \"EEE\" FFF'"
> > +	"\"STR \\\"GGG\\\" HHH\""
> > +	"'STR \\\'III\\\' JJJ'"
> > +    } {
> > +	gdb_test "$cmd -filename ${str} --" [expect_filename "${str}" ""]
> > +    }
> > +
> > +    # Create some tree structure we can use for the test.
> > +    set bdir "$tmpdir/testfilename"
> > +    file mkdir "$bdir/b"
> > +    file mkdir "$bdir/b c"
> > +    file mkdir "$bdir/b d"
> > +    file mkdir "$bdir/b c/d"
> > +    close [open "$bdir/b c/d/e" w]
> > +
> > +    # Tab completion for an escaped path.
> > +    test_gdb_complete_tab_multiple "$cmd -filename $bdir/b" "" {
> > +        "b/"
> > +        "b c/"
> > +        "b d/"
> > +    }
> 
> I think you will need to add a mechanism to rename these tests to
> avoid including paths in the test names.
> 
> The reason this is a problem is that some folk run a baseline GDB in
> one directory and a feature GDB in a different directory, and then
> compare the results.  Including paths in the test names makes it hard
> to compare tests.
> 
> Many dejagnu test functions take an optional test name.  These
> completion functions don't, but there's no reason why they couldn't.
> Then you just need to pass in a descriptive test name that doesn't
> include the test path.

OK, I’ll do that. Thanks for the explanations.

> 
> Thanks for looking at this.  I'd like to investigate the possibility
> of merging your filename completion with the existing completion - or
> at least make sure we understand why we don't want to do that.

I planned to do that ultimatly.  I’ll go over all the uses of the
current implementation and evaluate if changing it would cause any
problem.

I’ll also need to make sure I get all subtleties associated with
custom_word_point right before merging the completer.  I am not sure if
it makes any difference in this use case, but I need to make sure of
that.

Thanks for the feedback,
Lancelot.

> 
> Thanks,
> Andrew
> 
> > +    # Checking completions with the COMPLETE command shows escaped filenames.
> > +    test_gdb_complete_cmd_multiple "$cmd -filename $bdir/b" "" {
> > +        "/"
> > +        "\\ c/"
> > +        "\\ d/"
> > +    }
> > +    # When completing on a unique file, append a ' ' at the end.
> > +    test_gdb_complete_unique \
> > +        "$cmd -filename $bdir/b\\ c/d/" \
> > +        "$cmd -filename $bdir/b\\ c/d/e " ""
> > +
> > +    # Perform the same sequence with quoted filenames.
> > +
> > +    # Tab completion for a quoted path shows raw filenames.
> > +    test_gdb_complete_tab_multiple "$cmd -filename '$bdir/b" "" {
> > +        "b/"
> > +        "b c/"
> > +        "b d/"
> > +    }
> > +    # Using the COMPLETE command shows quoted filenames (without closing
> > +    # quote).
> > +    test_gdb_complete_cmd_multiple "$cmd -filename '$bdir/b" "" {
> > +        " c/" \
> > +        " d/" \
> > +        "/"
> > +    }
> > +    # When completing on a single option which is a file, append the
> > +    # closing quote as well as a space.
> > +    test_gdb_complete_unique \
> > +        "$cmd -filename '$bdir/b c/d/" \
> > +        "$cmd -filename '$bdir/b c/d/e' " ""
> > +
> > +    # Cleanup.
> > +    file delete -force "$bdir"
> > +}
> > +
> >  # Run the options framework tests first.
> >  foreach_with_prefix cmd {
> >      "require-delimiter"
> > @@ -1027,6 +1126,7 @@ foreach_with_prefix cmd {
> >      }
> >      test-enum $cmd
> >      test-string $cmd
> > +    test-filename $cmd
> >  }
> >  
> >  # Run the print integration tests, both as "standalone", and under
> > diff --git a/gdb/testsuite/lib/completion-support.exp b/gdb/testsuite/lib/completion-support.exp
> > index ddd3921977b..42f77887ca7 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
> >  }
> > -- 
> > 2.29.2
> > 

-- 
Lancelot SIX

Re: [PATCH 1/3] gdb::option: Add support for filename option.

[Andrew Burgess]

* Lancelot SIX <lsix@lancelotsix.com> [2021-02-16 18:52:45 +0000]:

> Le Tue, Feb 16, 2021 at 05:45:01PM +0000, Andrew Burgess a écrit :
> > * Lancelot SIX via Gdb-patches <gdb-patches@sourceware.org> [2021-02-13 22:07:50 +0000]:
> > 
> > > This commit adds support for filename option for the gdb::option parsing
> > > framework.  It is meant to help the user enter values that could be
> > > parsed by built_argv.
> > > 
> > > Tab completion works as expected, i.e. similar to bash's behavior.
> > > 
> > > Considering the following tree structure:
> > > 
> > > somedir/
> > > ├── a folder
> > 
> > You probably want a trailing / after 'a folder' to make it clear it is
> > a folder, similar to how somedir/ has the trailing character.
> > 
> > > └── a file
> > > 
> > > The following completions are available:
> > > 
> > >     (gdb) maintenance test-options unknown-is-error -filename somedir/a\ <tab>
> > >     a file    a folder/
> > > 
> > > or
> > > 
> > >     (gdb) maintenance test-options unknown-is-error -filename "somedir/a <tab>
> > >     a file    a folder/
> > > 
> > > The complete command command also provides useful completions which are
> > > escaped or quoted depending on what the user did input initially.
> > > 
> > >     (gdb) complete maintenance test-options unknown-is-error -filename somedir/a
> > >     maintenance test-options unknown-is-error -filename somedir/a\ file
> > >     maintenance test-options unknown-is-error -filename somedir/a\ folder/
> > >     (gdb) complete maintenance test-options unknown-is-error -filename 'somedir/a
> > >     maintenance test-options unknown-is-error -filename 'somedir/a file
> > >     maintenance test-options unknown-is-error -filename 'somedir/a folder/
> > > 
> > > Note that since entering a path is an interactive process, the completer
> > > will not append the closing quote after a directory name. Instead it
> > > adds a '/' and expect the user to provide further input.
> > > 
> > > Having both of those behaviors at the same time requires that the
> > > completer knows if it has been called by readline after the user
> > > pressed the tab key or by some other mechanism.  In the first scenario it
> > > should display non quoted/escaped completions so the user sees actual
> > > filenames but complete quoted/escaped filenames.  In the second scenario
> > > it should only provide quoted/escaped completions.  For that purpose,
> > > this commit adds a m_from_readline property to the completion_tracker
> > > object and initializes it accordingly to the calling context.
> > > 
> > > This filename_completer has slightly different behavior than the one
> > > available in gdb/completer.c mainly with regard to escaping/quoting.
> > > The original completer is kept as is to maintain stable behavior for
> > > commands whose completer might depend on this implementation.  If this is
> > > desirable, I’ll happily replace the existing filename completer with the
> > > one currently proposed in the gdb::options::filename namespace instead
> > > of having both with slightly different behaviors (but I an not sure if
> > > users are relying on the current behavior of the filename
> > > completer).
> > 
> > I wonder if you could expand more on what the differences are, maybe
> > with some examples.  Your new code sounds better, and I don't know why
> > we would want to keep the existing code.
> > 
> The current completer does not handle spaces nor any
> escape mechanism.
> 
> > Ideally I'd like to see some examples of things that the old completer
> > allows us to do that would now break, or behave significantly
> > differently if we just changed over to your code.
> > 
> 
> The only case I see where using the new completer would break things is
> a command which does not use built_argv to parse the argument line:
> 
> static void
> foo_command (const char *args, int from_tty)
> {
>   /* Handle args as a raw filename, or consider that everything that
>      follows a '--' is a filename.  */
>   const char *filename = args;
>   foo(filename);
> }
> 
> Other than that, using a completer that handles space escaping or
> quoting would only improve things.
> 
> I have not tracked down usages of the current completer to make sure no
> command ingests the filename without any sort of processing.  I’ll do that
> before sending a v2 of this patch series.

Based on the above explanation then it sounds like moving to your
completer globally.  It should be fairly easy I'd think to track down
which commands register with the old completer, and from there check
how they process the filename.

If any of them need fixing then I'd do that as a separate patch (in
the same series) first.

Then introduce your new completer and switch everyone over to it.

Thanks,
Andrew

[PATCH 2/3] gdb::option: Add support for zuinteger.

[Lancelot SIX via Gdb-patches]

This patch adds support for options of type var_zuinteger in the
gdb::option framework.

gdb/ChangeLog:

	* cli/cli-option.c (union option_value): Add zuinteger field.
	(parse_option): Add parsing of zuinteger field.
	(save_option_value_in_ctx): Handle var_zuinteger.
	(get_val_type_str): Handle var_zuinteger.
	(add_setshow_cmds_for_options): Handle var_zuinteger.
	* cli/cli-option.h (struct option_def): Add var_address.zuinteger.
	(struct zuinteger_option_def): Add.
	* maint-test-options.c (struct test_options_opts): Add zuint_opt.

gdb/testsuite/ChangeLog:

	* gdb.base/options.exp: Add tests for -zuinteger.
---
 gdb/cli/cli-option.c               | 27 ++++++++++
 gdb/cli/cli-option.h               | 21 ++++++++
 gdb/maint-test-options.c           | 14 +++++-
 gdb/testsuite/gdb.base/options.exp | 80 +++++++++++++++++++-----------
 4 files changed, 111 insertions(+), 31 deletions(-)

diff --git a/gdb/cli/cli-option.c b/gdb/cli/cli-option.c
index 5a6c997dcda..a36e899739c 100644
--- a/gdb/cli/cli-option.c
+++ b/gdb/cli/cli-option.c
@@ -40,6 +40,9 @@ union option_value
   /* For var_uinteger options.  */
   unsigned int uinteger;
 
+  /* For var_zuinteger options.  */
+  unsigned int zuinteger;
+
   /* For var_zuinteger_unlimited options.  */
   int integer;
 
@@ -593,6 +596,15 @@ parse_option (gdb::array_view<const option_def_group> options_group,
 	    return option_def_and_value {*match, match_ctx, val};
 	  }
       }
+    case var_zuinteger:
+      {
+        /* No completion available.  We cannot display NUMBER as a
+	   convinience for the user since there would be only one
+	   option and readline would complete it.  */
+	option_value val;
+	val.zuinteger = parse_cli_var_uinteger (match->type, args, false);
+	return option_def_and_value {*match, match_ctx, val};
+      }
     case var_enum:
       {
 	if (completion != nullptr)
@@ -827,6 +839,10 @@ save_option_value_in_ctx (gdb::optional<option_def_and_value> &ov)
       *ov->option.var_address.uinteger (ov->option, ov->ctx)
 	= ov->value->uinteger;
       break;
+    case var_zuinteger:
+      *ov->option.var_address.zuinteger (ov->option, ov->ctx)
+	= ov->value->zuinteger;
+      break;
     case var_zuinteger_unlimited:
       *ov->option.var_address.integer (ov->option, ov->ctx)
 	= ov->value->integer;
@@ -906,6 +922,8 @@ get_val_type_str (const option_def &opt, std::string &buffer)
     case var_uinteger:
     case var_zuinteger_unlimited:
       return "NUMBER|unlimited";
+    case var_zuinteger:
+      return "NUMBER";
     case var_enum:
       {
 	buffer = "";
@@ -1036,6 +1054,15 @@ add_setshow_cmds_for_options (command_class cmd_class,
 				    nullptr, option.show_cmd_cb,
 				    set_list, show_list);
 	}
+      else if (option.type == var_zuinteger)
+	{
+	  add_setshow_zuinteger_cmd (option.name, cmd_class,
+				     option.var_address.zuinteger (option, data),
+				     option.set_doc, option.show_doc,
+				     option.help_doc,
+				     nullptr, option.show_cmd_cb,
+				     set_list, show_list);
+	}
       else if (option.type == var_zuinteger_unlimited)
 	{
 	  add_setshow_zuinteger_unlimited_cmd
diff --git a/gdb/cli/cli-option.h b/gdb/cli/cli-option.h
index fc3aca887e7..b4dc2c36ce7 100644
--- a/gdb/cli/cli-option.h
+++ b/gdb/cli/cli-option.h
@@ -84,6 +84,7 @@ struct option_def
     {
       bool *(*boolean) (const option_def &, void *ctx);
       unsigned int *(*uinteger) (const option_def &, void *ctx);
+      unsigned int *(*zuinteger) (const option_def &, void *ctx);
       int *(*integer) (const option_def &, void *ctx);
       const char **(*enumeration) (const option_def &, void *ctx);
       char **(*string) (const option_def &, void *ctx);
@@ -221,6 +222,26 @@ struct uinteger_option_def : option_def
   }
 };
 
+/* A var_zuinteger command line option.  */
+
+template<typename Context>
+struct zuinteger_option_def : option_def
+{
+  zuinteger_option_def (const char *long_option_,
+			unsigned int *(*get_var_address_cb_) (Context *),
+			show_value_ftype *show_cmd_cb_,
+			const char *set_doc_,
+			const char *show_doc_ = nullptr,
+			const char *help_doc_ = nullptr)
+    : option_def (long_option_, var_zuinteger,
+		  (erased_get_var_address_ftype *) get_var_address_cb_,
+		  show_cmd_cb_,
+		  set_doc_, show_doc_, help_doc_)
+  {
+    var_address.uinteger = detail::get_var_address<unsigned int, Context>;
+  }
+};
+
 /* A var_zuinteger_unlimited command line option.  */
 
 template<typename Context>
diff --git a/gdb/maint-test-options.c b/gdb/maint-test-options.c
index 93d62eecc47..1e7145b3767 100644
--- a/gdb/maint-test-options.c
+++ b/gdb/maint-test-options.c
@@ -132,6 +132,7 @@ struct test_options_opts
   bool boolean_opt = false;
   const char *enum_opt = test_options_enum_values_xxx;
   unsigned int uint_opt = 0;
+  unsigned int zuint_opt = 0;
   int zuint_unl_opt = 0;
   char *string_opt = nullptr;
   char *filename_opt = nullptr;
@@ -151,7 +152,7 @@ struct test_options_opts
   {
     fprintf_unfiltered (file,
 			_("-flag %d -xx1 %d -xx2 %d -bool %d "
-			  "-enum %s -uint %s -zuint-unl %s -string '%s' "
+			  "-enum %s -uint %s -zuint %s -zuint-unl %s -string '%s' "
 			  "-filename '%s' -- %s\n"),
 			flag_opt,
 			xx1_opt,
@@ -161,6 +162,7 @@ struct test_options_opts
 			(uint_opt == UINT_MAX
 			 ? "unlimited"
 			 : pulongest (uint_opt)),
+			pulongest (zuint_opt),
 			(zuint_unl_opt == -1
 			 ? "unlimited"
 			 : plongest (zuint_unl_opt)),
@@ -225,6 +227,16 @@ static const gdb::option::option_def test_options_option_defs[] = {
     N_("A help doc that spawns\nmultiple lines."),
   },
 
+  /* A zuinteger option.  */
+  gdb::option::zuinteger_option_def<test_options_opts> {
+    "zuinteger",
+    [] (test_options_opts *opts) { return &opts->zuint_opt; },
+    nullptr, /* show_cmd_cb */
+    N_("A zuinteger option."),
+    nullptr, /* show_doc */
+    nullptr,
+  },
+
   /* A zuinteger_unlimited option.  */
   gdb::option::zuinteger_unlimited_option_def<test_options_opts> {
     "zuinteger-unlimited",
diff --git a/gdb/testsuite/gdb.base/options.exp b/gdb/testsuite/gdb.base/options.exp
index 92b298064b2..3fcbd68a4e5 100644
--- a/gdb/testsuite/gdb.base/options.exp
+++ b/gdb/testsuite/gdb.base/options.exp
@@ -95,19 +95,19 @@ proc make_cmd {variant} {
 # test-options xxx", with no flag/option set.  OPERAND is the expected
 # operand.
 proc expect_none {operand} {
-    return "-flag 0 -xx1 0 -xx2 0 -bool 0 -enum xxx -uint 0 -zuint-unl 0 -string '' -filename '' -- $operand"
+    return "-flag 0 -xx1 0 -xx2 0 -bool 0 -enum xxx -uint 0 -zuint 0 -zuint-unl 0 -string '' -filename '' -- $operand"
 }
 
 # Return a string for the expected result of running "maint
 # test-options xxx", with -flag set.  OPERAND is the expected operand.
 proc expect_flag {operand} {
-    return "-flag 1 -xx1 0 -xx2 0 -bool 0 -enum xxx -uint 0 -zuint-unl 0 -string '' -filename '' -- $operand"
+    return "-flag 1 -xx1 0 -xx2 0 -bool 0 -enum xxx -uint 0 -zuint 0 -zuint-unl 0 -string '' -filename '' -- $operand"
 }
 
 # Return a string for the expected result of running "maint
 # test-options xxx", with -bool set.  OPERAND is the expected operand.
 proc expect_bool {operand} {
-    return "-flag 0 -xx1 0 -xx2 0 -bool 1 -enum xxx -uint 0 -zuint-unl 0 -string '' -filename '' -- $operand"
+    return "-flag 0 -xx1 0 -xx2 0 -bool 1 -enum xxx -uint 0 -zuint 0 -zuint-unl 0 -string '' -filename '' -- $operand"
 }
 
 # Return a string for the expected result of running "maint
@@ -116,9 +116,11 @@ proc expect_bool {operand} {
 # expected operand.
 proc expect_integer {option val operand} {
     if {$option == "uinteger"} {
-	return "-flag 0 -xx1 0 -xx2 0 -bool 0 -enum xxx -uint $val -zuint-unl 0 -string '' -filename '' -- $operand"
+	return "-flag 0 -xx1 0 -xx2 0 -bool 0 -enum xxx -uint $val -zuint 0 -zuint-unl 0 -string '' -filename '' -- $operand"
+    } elseif {$option == "zuinteger"} {
+	return "-flag 0 -xx1 0 -xx2 0 -bool 0 -enum xxx -uint 0 -zuint $val -zuint-unl 0 -string '' -filename '' -- $operand"
     } elseif {$option == "zuinteger-unlimited"} {
-	return "-flag 0 -xx1 0 -xx2 0 -bool 0 -enum xxx -uint 0 -zuint-unl $val -string '' -filename '' -- $operand"
+	return "-flag 0 -xx1 0 -xx2 0 -bool 0 -enum xxx -uint 0 -zuint 0 -zuint-unl $val -string '' -filename '' -- $operand"
     } else {
 	error "unsupported option: $option"
     }
@@ -141,7 +143,7 @@ proc dequote_expected_string {str} {
 # expected operand.
 proc expect_string {str operand} {
     set str [dequote_expected_string $str]
-    return "-flag 0 -xx1 0 -xx2 0 -bool 0 -enum xxx -uint 0 -zuint-unl 0 -string '$str' -filename '' -- $operand"
+    return "-flag 0 -xx1 0 -xx2 0 -bool 0 -enum xxx -uint 0 -zuint 0 -zuint-unl 0 -string '$str' -filename '' -- $operand"
 }
 
 # Return a string for the expected result of running "maint
@@ -149,7 +151,7 @@ proc expect_string {str operand} {
 # expected operand.
 proc expect_filename {filename operand} {
     set filename [dequote_expected_string $filename]
-    return "-flag 0 -xx1 0 -xx2 0 -bool 0 -enum xxx -uint 0 -zuint-unl 0 -string '' -filename '$filename' -- $operand"
+    return "-flag 0 -xx1 0 -xx2 0 -bool 0 -enum xxx -uint 0 -zuint 0 -zuint-unl 0 -string '' -filename '$filename' -- $operand"
 }
 
 set all_options {
@@ -161,6 +163,7 @@ set all_options {
     "-uinteger"
     "-xx1"
     "-xx2"
+    "-zuinteger"
     "-zuinteger-unlimited"
 }
 
@@ -609,7 +612,7 @@ proc_with_prefix test-flag {variant} {
 
     # Extract twice the same flag, separated by one space.
     gdb_test "$cmd -xx1     -xx2 -xx1  -xx2 -xx1    -- non flags args" \
-	"-flag 0 -xx1 1 -xx2 1 -bool 0 -enum xxx -uint 0 -zuint-unl 0 -string '' -filename '' -- non flags args"
+	"-flag 0 -xx1 1 -xx2 1 -bool 0 -enum xxx -uint 0 -zuint 0 -zuint-unl 0 -string '' -filename '' -- non flags args"
 
     # Extract 2 known flags in front of unknown flags.
     gdb_test "$cmd -xx1 -xx2 -a -b -c -xx1 --" \
@@ -827,39 +830,49 @@ proc_with_prefix test-boolean {variant} {
 }
 
 # Uinteger option tests.  OPTION is which integer option we're
-# testing.  Can be "uinteger" or "zuinteger-unlimited".
+# testing.  Can be "uinteger", "zuinteger" or "zuinteger-unlimited".
 proc_with_prefix test-uinteger {variant option} {
     global all_options
 
     set cmd "[make_cmd $variant] -$option"
 
-    # Test completing a uinteger option:
-    res_test_gdb_complete_multiple \
-	"1 [expect_none ""]" \
-	"$cmd " "" "" {
-	"NUMBER"
-	"unlimited"
-    }
+    # zuinteger have no completion. NUMBER is not present since it
+    # would be the only option available and therefore completed on.
+    if {$option != "zuinteger"} {
 
-    # NUMBER above is just a placeholder, make sure we don't complete
-    # it as a valid option.
-    res_test_gdb_complete_none \
-	"1 [expect_none "NU"]" \
-	"$cmd NU"
+	# Test completing a uinteger option:
+	res_test_gdb_complete_multiple \
+	    "1 [expect_none ""]" \
+	    "$cmd " "" "" {
+	    "NUMBER"
+	    "unlimited"
+	}
 
-    # "unlimited" is valid though.
-    res_test_gdb_complete_unique \
-	"1 [expect_none "u"]" \
-	"$cmd u" \
-	"$cmd unlimited"
+	# NUMBER above is just a placeholder, make sure we don't complete
+	# it as a valid option.
+	res_test_gdb_complete_none \
+	    "1 [expect_none "NU"]" \
+	    "$cmd NU"
+
+	# "unlimited" is valid though.
+	res_test_gdb_complete_unique \
+	    "1 [expect_none "u"]" \
+	    "$cmd u" \
+	    "$cmd unlimited"
+    }
 
     # Basic smoke test of accepted / not accepted values.
     gdb_test "$cmd 1 -- 999" [expect_integer $option "1" "999"]
-    gdb_test "$cmd unlimited -- 999" \
-	[expect_integer $option "unlimited" "999"]
+    if {$option != "zuinteger"} {
+        gdb_test "$cmd unlimited -- 999" \
+	    [expect_integer $option "unlimited" "999"]
+    }
     if {$option == "zuinteger-unlimited"} {
 	gdb_test "$cmd -1 --" [expect_integer $option "unlimited" ""]
 	gdb_test "$cmd 0 --" [expect_integer $option "0" ""]
+    } elseif {$option == "zuinteger"} {
+	gdb_test "$cmd 0 --" [expect_integer $option "0" ""]
+	gdb_test "$cmd -1 --" "integer -1 out of range"
     } else {
 	gdb_test "$cmd -1 --" "integer -1 out of range"
 	gdb_test "$cmd 0 --" [expect_integer $option "unlimited" ""]
@@ -870,7 +883,7 @@ proc_with_prefix test-uinteger {variant option} {
 	"Expected integer at: unlimitedx --"
 
     # Don't offer completions until we're past the
-    # -uinteger/-zuinteger-unlimited argument.
+    # -uinteger/-zuinteger/-zuinteger-unlimited argument.
     res_test_gdb_complete_none \
 	"1 [expect_none ""]" \
 	"$cmd 1"
@@ -890,6 +903,13 @@ proc_with_prefix test-uinteger {variant option} {
 		"1 [expect_none ""]" \
 		"$cmd $value"
 	}
+    } elseif {$option == "zuinteger"} {
+	# -1 is invalid uinteger.
+	foreach value {"-1" "-1 "} {
+	    res_test_gdb_complete_none \
+		"1 [expect_none ""]" \
+		"$cmd $value"
+	}
     } else {
 	# -1 is valid for zuinteger-unlimited.
 	res_test_gdb_complete_none \
@@ -1121,7 +1141,7 @@ foreach_with_prefix cmd {
     test-misc $cmd
     test-flag $cmd
     test-boolean $cmd
-    foreach subcmd {"uinteger" "zuinteger-unlimited" } {
+    foreach subcmd {"uinteger" "zuinteger" "zuinteger-unlimited" } {
 	test-uinteger $cmd $subcmd
     }
     test-enum $cmd
-- 
2.29.2

[PATCH 3/3] Add completer to the add-inferior command

[Lancelot SIX via Gdb-patches]

This commit adds a completer to the add-inferior command.  Argument
parsing is also updated to benefit from what is offered by the
gdb::option framework.

gdb/Changelog:

	* inferior.c (struct add_inferior_options): Add struct.
	(add_inferior_options_defs): Create.
	(make_add_inferior_options_def_group): Create.
	(add_inferior_completer): Create.
	(add_inferior_command): Use gdb::option parsing for arguments.
	(initialize_inferiors): Use new completer for add-inferior
	command.

gdb/testsuite/ChangeLog:

	* gdb.base/completion.exp: Test add-completion completion.
---
 gdb/inferior.c                        | 123 ++++++++++++++++++--------
 gdb/testsuite/gdb.base/completion.exp |  11 +++
 2 files changed, 96 insertions(+), 38 deletions(-)

diff --git a/gdb/inferior.c b/gdb/inferior.c
index 49f869a4c78..6993d043a57 100644
--- a/gdb/inferior.c
+++ b/gdb/inferior.c
@@ -788,63 +788,110 @@ switch_to_inferior_and_push_target (inferior *new_inf,
     printf_filtered (_("Added inferior %d\n"), new_inf->num);
 }
 
+/* Describes the options for the add-inferior command.  */
+
+struct add_inferior_options
+{
+    /* Number new inferiors to create.  */
+    unsigned int copies = 1;
+
+    /* Instructs to start the inferior with no target connected.  */
+    bool no_connection = false;
+
+    /* Path to the file name of the executable to use as main program.  */
+    char *exec = nullptr;
+
+    ~add_inferior_options ()
+      {
+        if (exec != nullptr)
+          {
+            xfree (exec);
+            exec = nullptr;
+          }
+      }
+};
+
+/* Definition of options for the 'add-inferior' command.  */
+
+static const gdb::option::option_def add_inferior_options_defs[] = {
+  gdb::option::zuinteger_option_def<add_inferior_options> {
+      "copies",
+      [] (add_inferior_options *opt) { return &opt->copies; },
+      nullptr, /* show_cmd_cb */
+      nullptr, /* set_doc */
+      nullptr, /* show_doc */
+      nullptr, /* help_doc */
+  },
+
+  gdb::option::flag_option_def<add_inferior_options> {
+      "no-connection",
+      [] (add_inferior_options *opt) { return &opt->no_connection; },
+      nullptr, /* set_doc */
+      nullptr, /* help_doc */
+  },
+
+  gdb::option::filename_option_def<add_inferior_options> {
+      "exec",
+      [] (add_inferior_options *opt) { return &opt->exec; },
+      nullptr, /* show_cmd_cb */
+      nullptr, /* set_doc */
+      nullptr, /* show_doc */
+      nullptr, /* help_doc */
+  },
+
+};
+
+static gdb::option::option_def_group
+make_add_inferior_options_def_group (struct add_inferior_options *opts)
+{
+  return {{add_inferior_options_defs}, opts};
+}
+
+/* Completer for the add-inferior command.  */
+
+static void
+add_inferior_completer (struct cmd_list_element *cmd,
+			completion_tracker &tracker,
+			const char *text, const char * /*word*/)
+{
+  const auto group = make_add_inferior_options_def_group (nullptr);
+  if (gdb::option::complete_options
+      (tracker, &text, gdb::option::PROCESS_OPTIONS_UNKNOWN_IS_OPERAND, group))
+    return;
+}
+
 /* add-inferior [-copies N] [-exec FILENAME] [-no-connection] */
 
 static void
 add_inferior_command (const char *args, int from_tty)
 {
-  int i, copies = 1;
+  int i;
   gdb::unique_xmalloc_ptr<char> exec;
   symfile_add_flags add_flags = 0;
-  bool no_connection = false;
+
+  add_inferior_options opts;
+  const gdb::option::option_def_group group
+    = make_add_inferior_options_def_group (&opts);
+  gdb::option::process_options
+    (&args, gdb::option::PROCESS_OPTIONS_UNKNOWN_IS_ERROR, group);
 
   if (from_tty)
     add_flags |= SYMFILE_VERBOSE;
 
-  if (args)
-    {
-      gdb_argv built_argv (args);
-
-      for (char **argv = built_argv.get (); *argv != NULL; argv++)
-	{
-	  if (**argv == '-')
-	    {
-	      if (strcmp (*argv, "-copies") == 0)
-		{
-		  ++argv;
-		  if (!*argv)
-		    error (_("No argument to -copies"));
-		  copies = parse_and_eval_long (*argv);
-		}
-	      else if (strcmp (*argv, "-no-connection") == 0)
-		no_connection = true;
-	      else if (strcmp (*argv, "-exec") == 0)
-		{
-		  ++argv;
-		  if (!*argv)
-		    error (_("No argument to -exec"));
-		  exec.reset (tilde_expand (*argv));
-		}
-	    }
-	  else
-	    error (_("Invalid argument"));
-	}
-    }
-
   inferior *orginf = current_inferior ();
 
   scoped_restore_current_pspace_and_thread restore_pspace_thread;
 
-  for (i = 0; i < copies; ++i)
+  for (i = 0; i < opts.copies; ++i)
     {
       inferior *inf = add_inferior_with_spaces ();
 
-      switch_to_inferior_and_push_target (inf, no_connection, orginf);
+      switch_to_inferior_and_push_target (inf, opts.no_connection, orginf);
 
-      if (exec != NULL)
+      if (opts.filename != nullptr)
 	{
-	  exec_file_attach (exec.get (), from_tty);
-	  symbol_file_add_main (exec.get (), add_flags);
+	  exec_file_attach (opts.filename, from_tty);
+	  symbol_file_add_main (opts.filename, add_flags);
 	}
     }
 }
@@ -997,7 +1044,7 @@ as main program.\n\
 By default, the new inferior inherits the current inferior's connection.\n\
 If -no-connection is specified, the new inferior begins with\n\
 no target connection yet."));
-  set_cmd_completer (c, filename_completer);
+  set_cmd_completer_handle_brkchars (c, add_inferior_completer);
 
   add_com ("remove-inferiors", no_class, remove_inferior_command, _("\
 Remove inferior ID (or list of IDs).\n\
diff --git a/gdb/testsuite/gdb.base/completion.exp b/gdb/testsuite/gdb.base/completion.exp
index 076134cdc33..c83aa8994e2 100644
--- a/gdb/testsuite/gdb.base/completion.exp
+++ b/gdb/testsuite/gdb.base/completion.exp
@@ -980,3 +980,14 @@ test_gdb_complete_unique "xxx_yyy_" "xxx_yyy_zzz"
 gdb_test_no_output "alias set aaa_bbb_ccc=set debug"
 gdb_test_no_output "maint deprecate set aaa_bbb_ccc"
 test_gdb_complete_unique "set aaa_bbb_" "set aaa_bbb_ccc"
+
+# Test the completer of the add-inferior command.
+gdb_test_no_output "set max-completions unlimited"
+test_gdb_complete_multiple "add-inferior " "-" "" {
+    "-copies"
+    "-exec"
+    "-no-connection"
+}
+test_gdb_complete_unique \
+    "add-inferior -copies 2 -exec some\\ file -n" \
+    "add-inferior -copies 2 -exec some\\ file -no-connection"
-- 
2.29.2

Re: [PATCH 3/3] Add completer to the add-inferior command

[Andrew Burgess]

* Lancelot SIX via Gdb-patches <gdb-patches@sourceware.org> [2021-02-13 22:07:52 +0000]:

> This commit adds a completer to the add-inferior command.  Argument
> parsing is also updated to benefit from what is offered by the
> gdb::option framework.
> 
> gdb/Changelog:
> 
> 	* inferior.c (struct add_inferior_options): Add struct.
> 	(add_inferior_options_defs): Create.
> 	(make_add_inferior_options_def_group): Create.
> 	(add_inferior_completer): Create.
> 	(add_inferior_command): Use gdb::option parsing for arguments.
> 	(initialize_inferiors): Use new completer for add-inferior
> 	command.
> 
> gdb/testsuite/ChangeLog:
> 
> 	* gdb.base/completion.exp: Test add-completion completion.
> ---
>  gdb/inferior.c                        | 123 ++++++++++++++++++--------
>  gdb/testsuite/gdb.base/completion.exp |  11 +++
>  2 files changed, 96 insertions(+), 38 deletions(-)
> 
> diff --git a/gdb/inferior.c b/gdb/inferior.c
> index 49f869a4c78..6993d043a57 100644
> --- a/gdb/inferior.c
> +++ b/gdb/inferior.c
> @@ -788,63 +788,110 @@ switch_to_inferior_and_push_target (inferior *new_inf,
>      printf_filtered (_("Added inferior %d\n"), new_inf->num);
>  }
>  
> +/* Describes the options for the add-inferior command.  */
> +
> +struct add_inferior_options
> +{
> +    /* Number new inferiors to create.  */
> +    unsigned int copies = 1;
> +
> +    /* Instructs to start the inferior with no target connected.  */
> +    bool no_connection = false;
> +
> +    /* Path to the file name of the executable to use as main program.  */
> +    char *exec = nullptr;
> +
> +    ~add_inferior_options ()
> +      {
> +        if (exec != nullptr)
> +          {
> +            xfree (exec);
> +            exec = nullptr;
> +          }
> +      }
> +};
> +
> +/* Definition of options for the 'add-inferior' command.  */
> +
> +static const gdb::option::option_def add_inferior_options_defs[] = {
> +  gdb::option::zuinteger_option_def<add_inferior_options> {
> +      "copies",
> +      [] (add_inferior_options *opt) { return &opt->copies; },
> +      nullptr, /* show_cmd_cb */
> +      nullptr, /* set_doc */
> +      nullptr, /* show_doc */
> +      nullptr, /* help_doc */
> +  },
> +
> +  gdb::option::flag_option_def<add_inferior_options> {
> +      "no-connection",
> +      [] (add_inferior_options *opt) { return &opt->no_connection; },
> +      nullptr, /* set_doc */
> +      nullptr, /* help_doc */
> +  },
> +
> +  gdb::option::filename_option_def<add_inferior_options> {
> +      "exec",
> +      [] (add_inferior_options *opt) { return &opt->exec; },
> +      nullptr, /* show_cmd_cb */
> +      nullptr, /* set_doc */
> +      nullptr, /* show_doc */
> +      nullptr, /* help_doc */
> +  },
> +
> +};
> +
> +static gdb::option::option_def_group
> +make_add_inferior_options_def_group (struct add_inferior_options *opts)
> +{
> +  return {{add_inferior_options_defs}, opts};
> +}
> +
> +/* Completer for the add-inferior command.  */
> +
> +static void
> +add_inferior_completer (struct cmd_list_element *cmd,
> +			completion_tracker &tracker,
> +			const char *text, const char * /*word*/)
> +{
> +  const auto group = make_add_inferior_options_def_group (nullptr);
> +  if (gdb::option::complete_options
> +      (tracker, &text, gdb::option::PROCESS_OPTIONS_UNKNOWN_IS_OPERAND, group))
> +    return;
> +}
> +
>  /* add-inferior [-copies N] [-exec FILENAME] [-no-connection] */
>  
>  static void
>  add_inferior_command (const char *args, int from_tty)
>  {
> -  int i, copies = 1;
> +  int i;
>    gdb::unique_xmalloc_ptr<char> exec;
>    symfile_add_flags add_flags = 0;
> -  bool no_connection = false;
> +
> +  add_inferior_options opts;
> +  const gdb::option::option_def_group group
> +    = make_add_inferior_options_def_group (&opts);
> +  gdb::option::process_options
> +    (&args, gdb::option::PROCESS_OPTIONS_UNKNOWN_IS_ERROR, group);
>  
>    if (from_tty)
>      add_flags |= SYMFILE_VERBOSE;
>  
> -  if (args)
> -    {
> -      gdb_argv built_argv (args);
> -
> -      for (char **argv = built_argv.get (); *argv != NULL; argv++)
> -	{
> -	  if (**argv == '-')
> -	    {
> -	      if (strcmp (*argv, "-copies") == 0)
> -		{
> -		  ++argv;
> -		  if (!*argv)
> -		    error (_("No argument to -copies"));
> -		  copies = parse_and_eval_long (*argv);
> -		}
> -	      else if (strcmp (*argv, "-no-connection") == 0)
> -		no_connection = true;
> -	      else if (strcmp (*argv, "-exec") == 0)
> -		{
> -		  ++argv;
> -		  if (!*argv)
> -		    error (_("No argument to -exec"));
> -		  exec.reset (tilde_expand (*argv));
> -		}
> -	    }
> -	  else
> -	    error (_("Invalid argument"));
> -	}
> -    }
> -
>    inferior *orginf = current_inferior ();
>  
>    scoped_restore_current_pspace_and_thread restore_pspace_thread;
>  
> -  for (i = 0; i < copies; ++i)
> +  for (i = 0; i < opts.copies; ++i)
>      {
>        inferior *inf = add_inferior_with_spaces ();
>  
> -      switch_to_inferior_and_push_target (inf, no_connection, orginf);
> +      switch_to_inferior_and_push_target (inf, opts.no_connection, orginf);
>  
> -      if (exec != NULL)
> +      if (opts.filename != nullptr)
>  	{
> -	  exec_file_attach (exec.get (), from_tty);
> -	  symbol_file_add_main (exec.get (), add_flags);
> +	  exec_file_attach (opts.filename, from_tty);
> +	  symbol_file_add_main (opts.filename, add_flags);

This patch doesn't build for me as opts.filename is unknown.  Did you
mean opts.exec maybe?

Thanks,
Andrew



>  	}
>      }
>  }
> @@ -997,7 +1044,7 @@ as main program.\n\
>  By default, the new inferior inherits the current inferior's connection.\n\
>  If -no-connection is specified, the new inferior begins with\n\
>  no target connection yet."));
> -  set_cmd_completer (c, filename_completer);
> +  set_cmd_completer_handle_brkchars (c, add_inferior_completer);
>  
>    add_com ("remove-inferiors", no_class, remove_inferior_command, _("\
>  Remove inferior ID (or list of IDs).\n\
> diff --git a/gdb/testsuite/gdb.base/completion.exp b/gdb/testsuite/gdb.base/completion.exp
> index 076134cdc33..c83aa8994e2 100644
> --- a/gdb/testsuite/gdb.base/completion.exp
> +++ b/gdb/testsuite/gdb.base/completion.exp
> @@ -980,3 +980,14 @@ test_gdb_complete_unique "xxx_yyy_" "xxx_yyy_zzz"
>  gdb_test_no_output "alias set aaa_bbb_ccc=set debug"
>  gdb_test_no_output "maint deprecate set aaa_bbb_ccc"
>  test_gdb_complete_unique "set aaa_bbb_" "set aaa_bbb_ccc"
> +
> +# Test the completer of the add-inferior command.
> +gdb_test_no_output "set max-completions unlimited"
> +test_gdb_complete_multiple "add-inferior " "-" "" {
> +    "-copies"
> +    "-exec"
> +    "-no-connection"
> +}
> +test_gdb_complete_unique \
> +    "add-inferior -copies 2 -exec some\\ file -n" \
> +    "add-inferior -copies 2 -exec some\\ file -no-connection"
> -- 
> 2.29.2
> 

Re: [PATCH 3/3] Add completer to the add-inferior command

[Lancelot SIX via Gdb-patches]

> 
> This patch doesn't build for me as opts.filename is unknown.  Did you
> mean opts.exec maybe?
> 
> Thanks,
> Andrew
> 
> 
> 

Indeed… I was sure I rebuilt everything after this refactoring, but
apparently not.

Sorry for that.