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

[Lancelot SIX <lsix@lancelotsix.com>]

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