From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from simark.ca by simark.ca with LMTP id 7RgQBagELGBmUQAAWB0awg (envelope-from ) for ; Tue, 16 Feb 2021 12:45:12 -0500 Received: by simark.ca (Postfix, from userid 112) id 031A41EF78; Tue, 16 Feb 2021 12:45:11 -0500 (EST) X-Spam-Checker-Version: SpamAssassin 3.4.2 (2018-09-13) on simark.ca X-Spam-Level: X-Spam-Status: No, score=0.2 required=5.0 tests=DKIM_SIGNED,DKIM_VALID, DKIM_VALID_AU,MAILING_LIST_MULTI,RDNS_NONE,URIBL_BLOCKED autolearn=no autolearn_force=no version=3.4.2 Received: from sourceware.org (unknown [8.43.85.97]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by simark.ca (Postfix) with ESMTPS id D922D1E54D for ; Tue, 16 Feb 2021 12:45:10 -0500 (EST) Received: from server2.sourceware.org (localhost [IPv6:::1]) by sourceware.org (Postfix) with ESMTP id 493073851C29; Tue, 16 Feb 2021 17:45:10 +0000 (GMT) Received: from mail-wr1-x42e.google.com (mail-wr1-x42e.google.com [IPv6:2a00:1450:4864:20::42e]) by sourceware.org (Postfix) with ESMTPS id 1E3D7386189C for ; Tue, 16 Feb 2021 17:45:05 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.3.2 sourceware.org 1E3D7386189C Authentication-Results: sourceware.org; dmarc=none (p=none dis=none) header.from=embecosm.com Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=andrew.burgess@embecosm.com Received: by mail-wr1-x42e.google.com with SMTP id v1so14202566wrd.6 for ; Tue, 16 Feb 2021 09:45:05 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=embecosm.com; s=google; h=date:from:to:cc:subject:message-id:references:mime-version :content-disposition:content-transfer-encoding:in-reply-to; bh=YCD44L9QEyT6+hi236q68AHxzGkhfFQ+ROiFK32lNX8=; b=Wj6xxbmoEML2KfO4leN+aP4JA7TG763Yvpbbzr3ETeIE6CKTDoPjBsCiB5NCazrBhk A6V7dEzr++bQhcpI04faZxcJ6DggK49tm/Tr/71ixFqPHHJ00dov98NrqCahTYxW86/G uG6b3lAnOEX7FZur44Cq6jasLcsifJHnrguQP6aKI0MNFRIhuZAdLHe2+1woRWDZgluS Dix3fm3Wbggixe+iE7uK5GMN6cyf42p9Q+w3cqQDqWUV7vSemuNqLdse4yxSl2Ql0vlw rR7HBFLwMQDOLVH7OZMxyx6xly/t3z4djkvJYol+RQIqRn2SDgbA8ttyzWsXEoepET2Y 1b1g== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:content-transfer-encoding :in-reply-to; bh=YCD44L9QEyT6+hi236q68AHxzGkhfFQ+ROiFK32lNX8=; b=iYQ7rHBI9307rIXfAXavwzE2adXFLQJ8PiprPpqR2M2xRxLROVPVgE7zs+s7hxEXs1 QPG0K3AUL8ZwFXqMDpRowCft2mpsDabfn688LUw7Rnbde4VqYxQwGmLh69692hkfM2Bv jbfaX6IxTLQ9z+MbnVFnFRw4Nrqa82LqopkgIFs8zhuO9L52f29TlPghirSRQ/tOt8Lf uF4wUFiNWV9R5bJKszkpOdfaXMBC6gGX7jzJaAXtgzcJUBZro/r1tVztl6QpFtjkGsP7 yAk0hwJyOiiiK/0wyKUWBbDPGVUa/bIr5cwTbPeW9pWwd/pOaunMZN+pRCG+kdqqkhzz +7Xg== X-Gm-Message-State: AOAM5318WzzzIbGipsNt1rTnqgr5uYfiNy0QHzrge8BDoxzvOzWcPNxj 2pM4UtjVEn8KEg5s6KR3us5xbA== X-Google-Smtp-Source: ABdhPJxOfwBtPU7L0nO9X3Cb3pPSG+uFShyj/MM2/bLibAqRd1NFFrAsPL7Gpui5+CKVXvzQsz5y6g== X-Received: by 2002:a5d:44cf:: with SMTP id z15mr24707567wrr.191.1613497503754; Tue, 16 Feb 2021 09:45:03 -0800 (PST) Received: from localhost (host86-186-80-154.range86-186.btcentralplus.com. [86.186.80.154]) by smtp.gmail.com with ESMTPSA id v204sm4763887wmg.38.2021.02.16.09.45.02 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Tue, 16 Feb 2021 09:45:03 -0800 (PST) Date: Tue, 16 Feb 2021 17:45:01 +0000 From: Andrew Burgess To: Lancelot SIX Subject: Re: [PATCH 1/3] gdb::option: Add support for filename option. Message-ID: <20210216174501.GS265215@embecosm.com> References: <20210213220752.32581-1-lsix@lancelotsix.com> <20210213220752.32581-2-lsix@lancelotsix.com> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: quoted-printable In-Reply-To: <20210213220752.32581-2-lsix@lancelotsix.com> X-Operating-System: Linux/5.8.13-100.fc31.x86_64 (x86_64) X-Uptime: 17:15:09 up 69 days, 21:59, X-Editor: GNU Emacs [ http://www.gnu.org/software/emacs ] X-BeenThere: gdb-patches@sourceware.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Gdb-patches mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: gdb-patches@sourceware.org Errors-To: gdb-patches-bounces@sourceware.org Sender: "Gdb-patches" * Lancelot SIX via Gdb-patches [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. >=20 > Tab completion works as expected, i.e. similar to bash's behavior. >=20 > Considering the following tree structure: >=20 > somedir/ > =E2=94=9C=E2=94=80=E2=94=80 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. > =E2=94=94=E2=94=80=E2=94=80 a file >=20 > The following completions are available: >=20 > (gdb) maintenance test-options unknown-is-error -filename somedir/a\ = > a file a folder/ >=20 > or >=20 > (gdb) maintenance test-options unknown-is-error -filename "somedir/a = > a file a folder/ >=20 > The complete command command also provides useful completions which are > escaped or quoted depending on what the user did input initially. >=20 > (gdb) complete maintenance test-options unknown-is-error -filename so= medir/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 's= omedir/a > maintenance test-options unknown-is-error -filename 'somedir/a file > maintenance test-options unknown-is-error -filename 'somedir/a folder/ >=20 > 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. >=20 > 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. >=20 > 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=E2=80=99ll happily replace the existing filename completer w= ith 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. >=20 > 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. >=20 > All feedback are welcome. >=20 > gdb/ChangeLog: >=20 > * 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. >=20 > gdb/testsuite/ChangeLog: >=20 > * 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(-) >=20 > 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 > +#include > + > =20 > namespace gdb { > namespace option { > @@ -46,6 +48,9 @@ union option_value > =20 > /* 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; > }; > =20 > /* Holds an options definition and its value. */ > @@ -88,6 +93,8 @@ struct option_def_and_value > { > if (option.type =3D=3D var_string) > xfree (value->string); > + if (option.type =3D=3D var_filename) > + xfree (value->filename); > } > } > =20 > @@ -105,6 +112,8 @@ struct option_def_and_value > { > if (option.type =3D=3D var_string) > value->string =3D nullptr; > + if (option.type =3D=3D var_filename) > + value->filename =3D nullptr; > } > } > }; > @@ -164,6 +173,197 @@ complete_on_options (gdb::array_view options_group, > } > } > =20 > +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 !=3D '\0') > + { > + quoted.push_back (quotechar); > + while (*text !=3D '\0') > + { > + if (*text =3D=3D quotechar) > + quoted.push_back ('\\'); > + quoted.push_back (*text++); > + } > + } > + else > + { > + while (*text !=3D '\0') > + { > + /* Escape the space and quote chars. */ > + if (*text =3D=3D ' ' || *text =3D=3D '\'' || *text =3D=3D '"') > + 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 call= ed only > + once (i.e. only one possible completion). In this case, if COMPLETIO= N is not > + a directory, the closing quote is appended followed by a space to ind= icate > + 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 q= uotechar, 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 > + =3D stat (completion.c_str (), &finfo) =3D=3D 0 && S_ISDIR (finfo.st= _mode); > + > + /* Readline would add a '/' for dirs, so do is ourselves if readline i= s not > + here. */ > + if (isdir && !tracker.from_readline ()) > + completion.push_back ('/'); > + > + std::string quoted_completion =3D 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 quotati= on > + (if any) and append a space (tracker.suppress_append_ws () is true)= =2E */ > + if (only_completion && !isdir) > + { > + if (quotechar !=3D '\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 > + (xstrdup (completion.c_str ()))), &match_for_lcd, > + text, word); > + } > + else > + tracker.add_completion > + (std::move (gdb::unique_xmalloc_ptr > + (xstrdup (quoted_completion.c_str ()))), > + nullptr, text, word); > + > +} > + > +/* Filename completer. See completer_ftype for description of parameter= s. */ > + > +static void > +complete_filename > + (completion_tracker &tracker, const char *text, const char *word) > +{ > + /* First call to rl_filename_completion_function should be 0, subseque= nt > + 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_fu= nction. 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 sh= ould be > + done (see handle_completion for details). */ Typo, 'If it only returns one non null ....' > + int call_count =3D 0; > + std::string first_completion; > + > + /* The logic that tells if a space is to be appended after a completio= n 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-quo= te > + completions accordingly. */ > + char quotechar; > + if (text[0] =3D=3D '\'' || text[0] =3D=3D '"') > + quotechar =3D text[0]; > + else > + quotechar =3D '\0'; > + > + const char *arg_start =3D text; > + const std::string dequoted =3D extract_string_maybe_quoted (&arg_start= ); > + > + try > + { > + while (true) > + { > + gdb::unique_xmalloc_ptr next_completion > + (rl_filename_completion_function > + (dequoted.c_str (), call_count++)); > + > + if (next_completion =3D=3D nullptr) > + break; > + > + if (call_count =3D=3D 1) > + first_completion =3D next_completion.get (); > + else > + { > + if (call_count =3D=3D 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 =3D=3D 2) > + handle_completion > + (tracker, first_completion, quotechar, true, text, word); > + } > + catch (const gdb_exception_error &except) > + { > + if (except.error !=3D MAX_COMPLETIONS_REACHED_ERROR) > + throw; > + } > +} > + > +} /* namespace filename */ > + > /* See cli-option.h. */ > =20 > void > @@ -443,6 +643,40 @@ parse_option (gdb::array_view options_group, > return option_def_and_value {*match, match_ctx, val}; > } > =20 > + 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 =3D *args; > + const std::string fname =3D extract_string_maybe_quoted (args); > + > + /* If required, perform completion. */ > + if (completion !=3D nullptr) > + { > + if (**args =3D=3D '\0') > + { > + gdb::option::filename::complete_filename > + (completion->tracker, arg_start, arg_start); > + > + return {}; > + } > + } > + > + if (fname =3D=3D "") > + error (_("-%s requires an argument"), match->name); > + > + option_value val; > + val.filename =3D 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 &ov) > =3D ov->value->string; > ov->value->string =3D nullptr; > break; > + case var_filename: > + *ov->option.var_address.filename (ov->option, ov->ctx) > + =3D ov->value->filename; > + ov->value->filename =3D 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_cla= ss, > nullptr, option.show_cmd_cb, > set_list, show_list); > } > + else if (option.type =3D=3D 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; > =20 > @@ -282,6 +283,25 @@ struct string_option_def : option_def > } > }; > =20 > +/* A var_filename command line option. */ > + > +template > +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_ =3D nullptr, > + const char *help_doc_ =3D 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 =3D detail::get_var_address; > + } > +}; > + > /* 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 =3D false; > } > =20 > /* See completer.h. */ > @@ -2006,6 +2008,7 @@ gdb_completion_word_break_characters_throw () > start afresh. */ > delete current_completion.tracker; > current_completion.tracker =3D new completion_tracker (); > + current_completion.tracker->set_from_readline (true); > =20 > completion_tracker &tracker =3D *current_completion.tracker; > =20 > 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); > =20 > + /* Tells if the completion task is triggered by readline. > + See m from_readline. */ > + void set_from_readline (bool from_readline) > + { m_from_readline =3D from_readline; } > + > + /* Tells if the completion task is triggered by readline. > + See m_from_readline. */ > + bool from_readline () const > + { return m_from_readline; } > + > private: > =20 > /* 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 =3D 0; > + > + /* Indicates that the completions are to be displayed by readline > + interactively. The 'complete' command is a way to generate completi= ons > + not to be displayed by readline. */ > + bool m_from_readline; > }; > =20 > /* 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 =3D 0; > int zuint_unl_opt =3D 0; > char *string_opt =3D nullptr; > + char *filename_opt =3D nullptr; > =20 > test_options_opts () =3D default; > =20 > @@ -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 !=3D nullptr > ? string_opt > : ""), > + (filename_opt !=3D nullptr > + ? filename_opt > + : ""), > args); > } > }; > @@ -237,6 +242,14 @@ static const gdb::option::option_def test_options_op= tion_defs[] =3D { > nullptr, /* show_cmd_cb */ > N_("A string option."), > }, > + > + /* A filename option. */ > + gdb::option::filename_option_def { > + "filename", > + [] (test_options_opts *opts) { return &opts->filename_opt; }, > + nullptr, /* show_cmd_cb */ > + N_("A filename option."), > + }, > }; > =20 > /* 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" > } > =20 > # 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" > } > =20 > # 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" > } > =20 > # 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 =3D=3D "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 =3D=3D "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" > } > } > =20 > -# 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] =3D=3D "\"" > && [string range $str end end] =3D=3D "\"") > || ([string range $str 0 0] =3D=3D "'" > && [string range $str end end] =3D=3D "'")} { > - 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" > } > =20 > set all_options { > "-bool" > "-enum" > + "-filename" > "-flag" > "-string" > "-uinteger" > @@ -594,7 +609,7 @@ proc_with_prefix test-flag {variant} { > =20 > # 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" > =20 > # 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} { > } > } > =20 > +# 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 =3D=3D "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 filen= ames. > + 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 > } > =20 > # 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 > =20 > return $completion_list_re > } > --=20 > 2.29.2 >=20