[PATCH 1/5] Fix test_gdb_complete_tab_multiple race

[PATCH 1/5] Fix test_gdb_complete_tab_multiple race

[Pedro Alves]

Running 'make check-read1 TESTS="gdb.base/options.exp"' revealed a
race in test_gdb_complete_tab_multiple.  There's a gdb_test_multiple
call that expects a prompt in the middle of the regexp.  That's racy
because gdb_test_multiple includes a built-in FAIL pattern for the
prompt, which may match if gdb is slow enough to produce the rest of
the output after the prompt.

Fix this in the usual way of splitting the matching in two.

gdb/testsuite/ChangeLog:
yyyy-mm-dd  Pedro Alves  <palves@redhat.com>

	* lib/completion-support.exp (test_gdb_complete_tab_multiple):
	Split one gdb_test_multiple call in two to avoid a race.
---
 gdb/testsuite/lib/completion-support.exp | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/gdb/testsuite/lib/completion-support.exp b/gdb/testsuite/lib/completion-support.exp
index 97fed18b055..3199e85fd4d 100644
--- a/gdb/testsuite/lib/completion-support.exp
+++ b/gdb/testsuite/lib/completion-support.exp
@@ -151,8 +151,12 @@ proc test_gdb_complete_tab_multiple { input_line add_completed_line \
 		set maybe_bell ""
 	    }
 	    gdb_test_multiple "" "$test (second tab)" {
-		-re "^${maybe_bell}\r\n$expected_re\r\n$gdb_prompt $input_line_re$add_completed_line_re$" {
-		    pass "$test"
+		-re "^${maybe_bell}\r\n$expected_re\r\n$gdb_prompt " {
+		    gdb_test_multiple "" "$test (second tab)" {
+			-re "^$input_line_re$add_completed_line_re$" {
+			    pass "$test"
+			}
+		    }
 		}
 	    }
 	}
-- 
2.14.5

[PATCH 3/5] Teach gdb::option about string options

[Pedro Alves]

A following patch will make the "pipe" command use the gdb::option
framework for option processing.  However, "pipe"'s only option today
is a string option, "-d DELIM", and gdb::option does not support
string options yet.

This commit adds support for string options, mapped to var_string.
For now, a string is parsed up until the first whitespace.  I imagine
that we'll need to add support for quoting so that we could do:

 (gdb) cmd -option 'some -string'

without gdb confusing the "-string" for an option.

This doesn't seem important for pipe, so I'm leaving it for another
day.

One thing I'm not happy with, is that the string data is managed as a
raw malloc-allocated char *, which means that we need to xfree it
manually.  This is because var_string settings work that way too.
Although with var_string settings we're leaking the strings at gdb
exit, that was never really a problem.  For options though, leaking is
undesirable.

I think we should tackle that for both settings and options at the
same time, so for now I'm just managing the malloced data manually.
It's a bit ugly in option_def_and_value, but at least that's hidden
from view.

For testing, this adds a new "-string" option to "maint
test-settings", and then tweaks gdb.base/options.exp to exercise it.

gdb/ChangeLog:
yyyy-mm-dd  Pedro Alves  <palves@redhat.com>

	* cli/cli-option.c (union option_value) <string>: New field.
	(struct option_def_and_value): Add ctor, move ctor, dtor and
	disable copy ctor.
	(option_def_and_value::clear_value): New.
	(parse_option, save_option_value_in_ctx, get_val_type_str)
	(add_setshow_cmds_for_options): Handle var_string.
	* cli-option.h (union option_def::var_address) <string>: New
	field.
	(struct string_option_def): New.
	* maint-test-options.c (struct test_options_opts) <string_opt>:
	New field.
	(test_options_opts::~test_options_opts): New.
	(test_options_opts::dump): Also dump "-string".
	(test_options_option_defs): Install "string.

gdb/testsuite/ChangeLog:
yyyy-mm-dd  Pedro Alves  <palves@redhat.com>

	* gdb.base/options.exp (expect_none, expect_flag, expect_bool)
	(expect_integer): Adjust to expect "-string".
	(expect_string): New.
	(all_options): Expect "-string".
	(test-flag, test-boolean): Adjust to expect "-string".
	(test-string): New proc.
	(top level): Call it.
---
 gdb/cli/cli-option.c               | 87 ++++++++++++++++++++++++++++++++++++++
 gdb/cli/cli-option.h               | 21 +++++++++
 gdb/maint-test-options.c           | 23 ++++++++--
 gdb/testsuite/gdb.base/options.exp | 82 +++++++++++++++++++++++++++--------
 4 files changed, 193 insertions(+), 20 deletions(-)

diff --git a/gdb/cli/cli-option.c b/gdb/cli/cli-option.c
index 8f2844610b5..06f8b459e0e 100644
--- a/gdb/cli/cli-option.c
+++ b/gdb/cli/cli-option.c
@@ -43,6 +43,9 @@ union option_value
 
   /* For var_enum options.  */
   const char *enumeration;
+
+  /* For var_string options.  This is malloc-allocated.  */
+  char *string;
 };
 
 /* Holds an options definition and its value.  */
@@ -56,6 +59,55 @@ struct option_def_and_value
 
   /* The option's value, if any.  */
   gdb::optional<option_value> value;
+
+  /* Constructor.  */
+  option_def_and_value (const option_def &option_, void *ctx_,
+			gdb::optional<option_value> &&value_ = {})
+    : option (option_),
+      ctx (ctx_),
+      value (std::move (value_))
+  {
+    clear_value (option_, value_);
+  }
+
+  /* Move constructor.  Need this because for some types the values
+     are allocated on the heap.  */
+  option_def_and_value (option_def_and_value &&rval)
+    : option (rval.option),
+      ctx (rval.ctx),
+      value (std::move (rval.value))
+  {
+    clear_value (rval.option, rval.value);
+  }
+
+  /* Disable the copy constructor.  */
+  option_def_and_value (const option_def_and_value &rval) = delete;
+
+  ~option_def_and_value ()
+  {
+    if (value.has_value ())
+      {
+	if (option.type == var_string)
+	  xfree (value->string);
+      }
+  }
+
+private:
+
+  /* Clear the option_value, without releasing it.  This is used after
+     the value has been moved to some other option_def_and_value
+     instance.  This is needed because for some types the value is
+     allocated on the heap, so we must clear the pointer in the
+     source, to avoid a double free.  */
+  static void clear_value (const option_def &option,
+			   gdb::optional<option_value> &value)
+  {
+    if (value.has_value ())
+      {
+	if (option.type == var_string)
+	  value->string = nullptr;
+      }
+  }
 };
 
 static void save_option_value_in_ctx (gdb::optional<option_def_and_value> &ov);
@@ -373,6 +425,25 @@ parse_option (gdb::array_view<const option_def_group> options_group,
 	val.enumeration = parse_cli_var_enum (args, match->enums);
 	return option_def_and_value {*match, match_ctx, val};
       }
+    case var_string:
+      {
+	if (check_for_argument (args, "--"))
+	  {
+	    /* Treat e.g., "maint test-options -string --" as if there
+	       was no argument after "-string".  */
+	    error (_("-%s requires an argument"), match->name);
+	  }
+
+	const char *arg_start = *args;
+	*args = skip_to_space (*args);
+
+	if (*args == arg_start)
+	  error (_("-%s requires an argument"), match->name);
+
+	option_value val;
+	val.string = savestring (arg_start, *args - arg_start);
+	return option_def_and_value {*match, match_ctx, val};
+      }
 
     default:
       /* Not yet.  */
@@ -532,6 +603,11 @@ save_option_value_in_ctx (gdb::optional<option_def_and_value> &ov)
       *ov->option.var_address.enumeration (ov->option, ov->ctx)
 	= ov->value->enumeration;
       break;
+    case var_string:
+      *ov->option.var_address.string (ov->option, ov->ctx)
+	= ov->value->string;
+      ov->value->string = nullptr;
+      break;
     default:
       gdb_assert_not_reached ("unhandled option type");
     }
@@ -604,6 +680,8 @@ get_val_type_str (const option_def &opt, std::string &buffer)
 	  }
 	return buffer.c_str ();
       }
+    case var_string:
+      return "STRING";
     default:
       return nullptr;
     }
@@ -731,6 +809,15 @@ add_setshow_cmds_for_options (command_class cmd_class,
 				nullptr, option.show_cmd_cb,
 				set_list, show_list);
 	}
+      else if (option.type == var_string)
+	{
+	  add_setshow_string_cmd (option.name, cmd_class,
+				  option.var_address.string (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 1bfbfce1ce5..a26b52f7f29 100644
--- a/gdb/cli/cli-option.h
+++ b/gdb/cli/cli-option.h
@@ -86,6 +86,7 @@ public:
       unsigned int *(*uinteger) (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);
     }
   var_address;
 
@@ -261,6 +262,26 @@ struct enum_option_def : option_def
   }
 };
 
+/* A var_string command line option.  */
+
+template<typename Context>
+struct string_option_def : option_def
+{
+  string_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_string,
+		  (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/maint-test-options.c b/gdb/maint-test-options.c
index 7e7ef6e7992..ba36b2f89c4 100644
--- a/gdb/maint-test-options.c
+++ b/gdb/maint-test-options.c
@@ -58,10 +58,10 @@
    readline, for proper testing of TAB completion.
 
    These maintenance commands support options of all the different
-   available kinds of commands (boolean, enum, flag, uinteger):
+   available kinds of commands (boolean, enum, flag, string, uinteger):
 
     (gdb) maint test-options require-delimiter -[TAB]
-    -bool      -enum      -flag      -uinteger   -xx1       -xx2
+    -bool      -enum      -flag      -string     -uinteger   -xx1       -xx2
 
     (gdb) maint test-options require-delimiter -bool o[TAB]
     off  on
@@ -133,6 +133,12 @@ struct test_options_opts
   const char *enum_opt = test_options_enum_values_xxx;
   unsigned int uint_opt = 0;
   int zuint_unl_opt = 0;
+  char *string_opt = nullptr;
+
+  ~test_options_opts ()
+  {
+    xfree (string_opt);
+  }
 
   /* Dump the options to FILE.  ARGS is the remainder unprocessed
      arguments.  */
@@ -140,7 +146,7 @@ struct test_options_opts
   {
     fprintf_unfiltered (file,
 			_("-flag %d -xx1 %d -xx2 %d -bool %d "
-			  "-enum %s -uint %s -zuint-unl %s -- %s\n"),
+			  "-enum %s -uint %s -zuint-unl %s -string '%s' -- %s\n"),
 			flag_opt,
 			xx1_opt,
 			xx2_opt,
@@ -152,6 +158,9 @@ struct test_options_opts
 			(zuint_unl_opt == -1
 			 ? "unlimited"
 			 : plongest (zuint_unl_opt)),
+			(string_opt != nullptr
+			 ? string_opt
+			 : ""),
 			args);
   }
 };
@@ -216,6 +225,14 @@ static const gdb::option::option_def test_options_option_defs[] = {
     nullptr, /* show_doc */
     nullptr, /* help_doc */
   },
+
+  /* A string option.  */
+  gdb::option::string_option_def<test_options_opts> {
+    "string",
+    [] (test_options_opts *opts) { return &opts->string_opt; },
+    nullptr, /* show_cmd_cb */
+    N_("A string 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 1a652b3c9dc..e8f571d9ba9 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 -- $operand"
+    return "-flag 0 -xx1 0 -xx2 0 -bool 0 -enum xxx -uint 0 -zuint-unl 0 -string '' -- $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 -- $operand"
+    return "-flag 1 -xx1 0 -xx2 0 -bool 0 -enum xxx -uint 0 -zuint-unl 0 -string '' -- $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 -- $operand"
+    return "-flag 0 -xx1 0 -xx2 0 -bool 1 -enum xxx -uint 0 -zuint-unl 0 -string '' -- $operand"
 }
 
 # Return a string for the expected result of running "maint
@@ -116,18 +116,26 @@ 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 -- $operand"
+	return "-flag 0 -xx1 0 -xx2 0 -bool 0 -enum xxx -uint $val -zuint-unl 0 -string '' -- $operand"
     } elseif {$option == "zuinteger-unlimited"} {
-	return "-flag 0 -xx1 0 -xx2 0 -bool 0 -enum xxx -uint 0 -zuint-unl $val -- $operand"
+	return "-flag 0 -xx1 0 -xx2 0 -bool 0 -enum xxx -uint 0 -zuint-unl $val -string '' -- $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} {
+    return "-flag 0 -xx1 0 -xx2 0 -bool 0 -enum xxx -uint 0 -zuint-unl 0 -string '$str' -- $operand"
+}
+
 set all_options {
     "-bool"
     "-enum"
     "-flag"
+    "-string"
     "-uinteger"
     "-xx1"
     "-xx2"
@@ -577,7 +585,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 -- non flags args"
+	"-flag 0 -xx1 1 -xx2 1 -bool 0 -enum xxx -uint 0 -zuint-unl 0 -string '' -- non flags args"
 
     # Extract 2 known flags in front of unknown flags.
     gdb_test "$cmd -xx1 -xx2 -a -b -c -xx1 --" \
@@ -624,19 +632,11 @@ proc_with_prefix test-boolean {variant} {
     #   E.g., "frame apply all -past-main COMMAND".
 
     if {$variant == "require-delimiter"} {
+	set match_list $all_options
+	lappend match_list "off" "on"
 	res_test_gdb_complete_multiple \
 	    "1 [expect_none ""]" \
-	    "$cmd -bool " "" "" {
-	    "-bool"
-	    "-enum"
-	    "-flag"
-	    "-uinteger"
-	    "-xx1"
-	    "-xx2"
-	    "-zuinteger-unlimited"
-	    "off"
-	    "on"
-	}
+	    "$cmd -bool " "" "" $match_list
     } else {
 	res_test_gdb_complete_none "0 " "$cmd -bool "
     }
@@ -942,6 +942,53 @@ proc_with_prefix test-enum {variant} {
     gdb_test "$cmd -enum www --" "Undefined item: \"www\"."
 }
 
+# String option tests.
+proc_with_prefix test-string {variant} {
+    global all_options
+
+    set cmd [make_cmd $variant]
+
+    res_test_gdb_complete_none \
+	"1 [expect_none ""]" \
+	"$cmd -string "
+
+    # Check that "-" where a value is expected does not show the
+    # command's options.  I.e., a string's value is not optional.
+    # Check both completion and running the command.
+    res_test_gdb_complete_none \
+	"1 [expect_none ""]" \
+	"$cmd -string -"
+    gdb_test "$cmd -string --"\
+	"-string requires an argument"
+    if {$variant == "require-delimiter"} {
+	gdb_test "$cmd -string" [expect_none "-string"]
+    } else {
+	gdb_test "$cmd -string"\
+	    "-string requires an argument"
+    }
+
+    res_test_gdb_complete_none \
+	"1 [expect_none ""]" \
+	"$cmd -string STR"
+    gdb_test "$cmd -string STR --" [expect_string "STR" ""]
+
+    # Completing at "-" after parsing STR should list all options.
+    res_test_gdb_complete_multiple \
+	"1 [expect_string "STR" "-"]" \
+	"$cmd -string STR " "-" "" $all_options
+
+    # Check that only FOO is considered part of the string's value.
+    # I.e., that we stop parsing the string at the first whitespace.
+    if {$variant == "require-delimiter"} {
+	res_test_gdb_complete_none \
+	    "1 [expect_string "FOO" "BAR"]" \
+	    "$cmd -string FOO BAR"
+    } else {
+	res_test_gdb_complete_none "0 BAR" "$cmd -string FOO BAR"
+    }
+    gdb_test "$cmd -string FOO BAR --" "Unrecognized option at: BAR --"
+}
+
 # Run the options framework tests first.
 foreach_with_prefix cmd {
     "require-delimiter"
@@ -955,6 +1002,7 @@ foreach_with_prefix cmd {
 	test-uinteger $cmd $subcmd
     }
     test-enum $cmd
+    test-string $cmd
 }
 
 # Run the print integration tests, both as "standalone", and under
-- 
2.14.5

[PATCH 0/5] pipe command completer, and string options

[Pedro Alves]

Tromey noticed that the "pipe" command doesn't have a completer.
Initially I said I wasn't planning on looking at it, thinking that all
the building blocks were already in place.  But I quickly realized
that there's one piece missing -- string options...  So I took a stab
at implementing it.  Of course, lucky me, I ran into other issues and
latent bugs, now all addressed...

Pedro Alves (5):
  Fix test_gdb_complete_tab_multiple race
  Make gdb::option::complete_options save processed arguments too
  Teach gdb::option about string options
  Fix latent bug in test_gdb_complete_cmd_multiple
  pipe command completer

 gdb/cli/cli-cmds.c                       |  92 +++++++++++++--
 gdb/cli/cli-option.c                     | 157 ++++++++++++++++++++-----
 gdb/cli/cli-option.h                     |  21 ++++
 gdb/cli/cli-utils.c                      |   7 +-
 gdb/maint-test-options.c                 | 134 ++++++++++++++--------
 gdb/testsuite/gdb.base/options.exp       | 190 +++++++++++++++++++++++--------
 gdb/testsuite/gdb.base/shell.exp         |  47 +++++++-
 gdb/testsuite/lib/completion-support.exp |  11 +-
 8 files changed, 522 insertions(+), 137 deletions(-)

-- 
2.14.5

[PATCH 2/5] Make gdb::option::complete_options save processed arguments too

[Pedro Alves]

Currently, gdb::option::complete_options just discards any processed
option argument, because no completer needs that data.

When completing "pipe -d XXX gdbcmd XXX" however, the completer needs
to know about -d's argument (XXX), in order to know where input is
already past the gdb command and the delimiter.

In this commit, the fix for that is the factoring out of the
save_option_value_in_ctx function and calling it in complete_options.

For testing, this makes "maint show test-options-completion-result"
show the processed options too, like what the "maint test-options"
subcommands output when run.  Then, of course, gdb.base/options.exp is
adjusted.

Doing this exposed a couple latent bugs, which is what the other gdb
changes in the patch are for:

 - in the var_enum case, without the change, we'd end up with a null
   enum argument, and print:

     "-enum (null)"

 - The get_ulongest change is necessary to avoid advancing PP in a
   case where we end up throwing an error, e.g., when parsing "11x".
   Without the change the operand pointer shown by "maint show
   test-options-completion-result" would be left pointing at "x"
   instead of "11x".

gdb/ChangeLog:
yyyy-mm-dd  Pedro Alves  <palves@redhat.com>

	* cli/cli-option.c (parse_option) <var_enum>: Don't return an
	option_value with a null enumeration.
	(complete_options): Save the option values in the context.
	(save_option_value_in_ctx): New, factored out from ...
	(process_options): ... here.
	* cli/cli-utils.c (get_ulongest): Don't advance PP until the end
	of the function.
	* maint-test-options.c (test_options_opts::dump): New, factored
	out from ...
	(maintenance_test_options_command_mode): ... here.
	(maintenance_test_options_command_completion_result): Delete.
	(maintenance_test_options_command_completion_text): Update
	comment.
	(maintenance_show_test_options_completion_result): Change
	prototype.  Just print
	maintenance_test_options_command_completion_text.
	(save_completion_result): New.
	(maintenance_test_options_completer_mode): Pass options context to
	complete_options, and then save a dump.
	(_initialize_maint_test_options): Use add_cmd to install "maint
	show test-options-completion-result".

gdb/testsuite/ChangeLog:
yyyy-mm-dd  Pedro Alves  <palves@redhat.com>

	* gdb.base/options.exp (test-misc, test-flag, test-boolean)
	(test-uinteger, test-enum): Adjust res_test_gdb_... calls to pass
	the expected output in the success.
---
 gdb/cli/cli-option.c               |  70 ++++++++++++++---------
 gdb/cli/cli-utils.c                |   7 ++-
 gdb/maint-test-options.c           | 113 ++++++++++++++++++++++---------------
 gdb/testsuite/gdb.base/options.exp | 110 ++++++++++++++++++++++++++----------
 4 files changed, 195 insertions(+), 105 deletions(-)

diff --git a/gdb/cli/cli-option.c b/gdb/cli/cli-option.c
index 9a53ec0592d..8f2844610b5 100644
--- a/gdb/cli/cli-option.c
+++ b/gdb/cli/cli-option.c
@@ -58,6 +58,8 @@ struct option_def_and_value
   gdb::optional<option_value> value;
 };
 
+static void save_option_value_in_ctx (gdb::optional<option_def_and_value> &ov);
+
 /* Info passed around when handling completion.  */
 struct parse_option_completion_info
 {
@@ -349,11 +351,12 @@ parse_option (gdb::array_view<const option_def_group> options_group,
 	      {
 		complete_on_enum (completion->tracker,
 				  match->enums, *args, *args);
-		*args = after_arg;
+		if (completion->tracker.have_completions ())
+		  return {};
 
-		option_value val;
-		val.enumeration = nullptr;
-		return option_def_and_value {*match, match_ctx, val};
+		/* If we don't have completions, let the
+		   non-completion path throw on invalid enum value
+		   below, so that completion processing stops.  */
 	      }
 	  }
 
@@ -456,6 +459,11 @@ complete_options (completion_tracker &tracker,
 		    (*args - text);
 		  return true;
 		}
+
+	      /* If the caller passed in a context, then it is
+		 interested in the option argument values.  */
+	      if (ov && ov->ctx != nullptr)
+		save_option_value_in_ctx (ov);
 	    }
 	  else
 	    {
@@ -499,6 +507,36 @@ complete_options (completion_tracker &tracker,
   return false;
 }
 
+/* Save the parsed value in the option's context.  */
+
+static void
+save_option_value_in_ctx (gdb::optional<option_def_and_value> &ov)
+{
+  switch (ov->option.type)
+    {
+    case var_boolean:
+      {
+	bool value = ov->value.has_value () ? ov->value->boolean : true;
+	*ov->option.var_address.boolean (ov->option, ov->ctx) = value;
+      }
+      break;
+    case var_uinteger:
+      *ov->option.var_address.uinteger (ov->option, ov->ctx)
+	= ov->value->uinteger;
+      break;
+    case var_zuinteger_unlimited:
+      *ov->option.var_address.integer (ov->option, ov->ctx)
+	= ov->value->integer;
+      break;
+    case var_enum:
+      *ov->option.var_address.enumeration (ov->option, ov->ctx)
+	= ov->value->enumeration;
+      break;
+    default:
+      gdb_assert_not_reached ("unhandled option type");
+    }
+}
+
 /* See cli-option.h.  */
 
 bool
@@ -534,29 +572,7 @@ process_options (const char **args,
 
       processed_any = true;
 
-      switch (ov->option.type)
-	{
-	case var_boolean:
-	  {
-	    bool value = ov->value.has_value () ? ov->value->boolean : true;
-	    *ov->option.var_address.boolean (ov->option, ov->ctx) = value;
-	  }
-	  break;
-	case var_uinteger:
-	  *ov->option.var_address.uinteger (ov->option, ov->ctx)
-	    = ov->value->uinteger;
-	  break;
-	case var_zuinteger_unlimited:
-	  *ov->option.var_address.integer (ov->option, ov->ctx)
-	    = ov->value->integer;
-	  break;
-	case var_enum:
-	  *ov->option.var_address.enumeration (ov->option, ov->ctx)
-	    = ov->value->enumeration;
-	  break;
-	default:
-	  gdb_assert_not_reached ("unhandled option type");
-	}
+      save_option_value_in_ctx (ov);
     }
 }
 
diff --git a/gdb/cli/cli-utils.c b/gdb/cli/cli-utils.c
index f5d47aeffbc..333a86a81b9 100644
--- a/gdb/cli/cli-utils.c
+++ b/gdb/cli/cli-utils.c
@@ -60,13 +60,14 @@ get_ulongest (const char **pp, int trailer)
     }
   else
     {
-      retval = strtoulst (p, pp, 0);
-      if (p == *pp)
+      const char *end = p;
+      retval = strtoulst (p, &end, 0);
+      if (p == end)
 	{
 	  /* There is no number here.  (e.g. "cond a == b").  */
 	  error (_("Expected integer at: %s"), p);
 	}
-      p = *pp;
+      p = end;
     }
 
   if (!(isspace (*p) || *p == '\0' || *p == trailer))
diff --git a/gdb/maint-test-options.c b/gdb/maint-test-options.c
index 599155cbfe6..7e7ef6e7992 100644
--- a/gdb/maint-test-options.c
+++ b/gdb/maint-test-options.c
@@ -133,6 +133,27 @@ struct test_options_opts
   const char *enum_opt = test_options_enum_values_xxx;
   unsigned int uint_opt = 0;
   int zuint_unl_opt = 0;
+
+  /* Dump the options to FILE.  ARGS is the remainder unprocessed
+     arguments.  */
+  void dump (ui_file *file, const char *args) const
+  {
+    fprintf_unfiltered (file,
+			_("-flag %d -xx1 %d -xx2 %d -bool %d "
+			  "-enum %s -uint %s -zuint-unl %s -- %s\n"),
+			flag_opt,
+			xx1_opt,
+			xx2_opt,
+			boolean_opt,
+			enum_opt,
+			(uint_opt == UINT_MAX
+			 ? "unlimited"
+			 : pulongest (uint_opt)),
+			(zuint_unl_opt == -1
+			 ? "unlimited"
+			 : plongest (zuint_unl_opt)),
+			args);
+  }
 };
 
 /* Option definitions for the "maintenance test-options" commands.  */
@@ -226,46 +247,48 @@ maintenance_test_options_command_mode (const char *args,
   else
     args = skip_spaces (args);
 
-  printf_unfiltered (_("-flag %d -xx1 %d -xx2 %d -bool %d "
-		       "-enum %s -uint %s -zuint-unl %s -- %s\n"),
-		     opts.flag_opt,
-		     opts.xx1_opt,
-		     opts.xx2_opt,
-		     opts.boolean_opt,
-		     opts.enum_opt,
-		     (opts.uint_opt == UINT_MAX
-		      ? "unlimited"
-		      : pulongest (opts.uint_opt)),
-		     (opts.zuint_unl_opt == -1
-		      ? "unlimited"
-		      : plongest (opts.zuint_unl_opt)),
-		     args);
+  opts.dump (gdb_stdout, args);
 }
 
-/* Variables used by the "maintenance show
-   test-options-completion-result" command.  These variables are
-   stored by the completer of the "maint test-options"
-   subcommands.  */
+/* Variable used by the "maintenance show
+   test-options-completion-result" command.  This variable is stored
+   by the completer of the "maint test-options" subcommands.
 
-/* The result of gdb::option::complete_options.  */
-static int maintenance_test_options_command_completion_result;
-/* The text at the word point after gdb::option::complete_options
-   returns.  */
+   If the completer returned false, this includes the text at the word
+   point after gdb::option::complete_options returns.  If true, then
+   this includes a dump of the processed options.  */
 static std::string maintenance_test_options_command_completion_text;
 
 /* The "maintenance show test-options-completion-result" command.  */
 
 static void
-maintenance_show_test_options_completion_result
-  (struct ui_file *file, int from_tty,
-   struct cmd_list_element *c, const char *value)
+maintenance_show_test_options_completion_result (const char *args,
+						 int from_tty)
 {
-  if (maintenance_test_options_command_completion_result)
-    fprintf_filtered (file, "1\n");
+  puts_filtered (maintenance_test_options_command_completion_text.c_str ());
+}
+
+/* Save the completion result in the global variables read by the
+   "maintenance test-options require-delimiter" command.  */
+
+static void
+save_completion_result (const test_options_opts &opts, bool res,
+			const char *text)
+{
+  if (res)
+    {
+      string_file stream;
+
+      stream.puts ("1 ");
+      opts.dump (&stream, text);
+      maintenance_test_options_command_completion_text
+	= std::move (stream.string ());
+    }
   else
-    fprintf_filtered
-      (file, _("0 %s\n"),
-       maintenance_test_options_command_completion_text.c_str ());
+    {
+      maintenance_test_options_command_completion_text
+	= string_printf ("0 %s\n", text);
+    }
 }
 
 /* Implementation of completer for the "maintenance test-options
@@ -278,17 +301,19 @@ maintenance_test_options_completer_mode (completion_tracker &tracker,
 					 const char *text,
 					 gdb::option::process_options_mode mode)
 {
+  test_options_opts opts;
+
   try
     {
-      maintenance_test_options_command_completion_result
-	= gdb::option::complete_options
-	   (tracker, &text, mode,
-	    make_test_options_options_def_group (nullptr));
-      maintenance_test_options_command_completion_text = text;
+      bool res = (gdb::option::complete_options
+		  (tracker, &text, mode,
+		   make_test_options_options_def_group (&opts)));
+
+      save_completion_result (opts, res, text);
     }
   catch (const gdb_exception_error &ex)
     {
-      maintenance_test_options_command_completion_result = 1;
+      save_completion_result (opts, true, text);
       throw;
     }
 }
@@ -445,17 +470,13 @@ Options:\n\
   set_cmd_completer_handle_brkchars
     (cmd, maintenance_test_options_unknown_is_operand_command_completer);
 
-  add_setshow_zinteger_cmd ("test-options-completion-result", class_maintenance,
-			    &maintenance_test_options_command_completion_result,
-			    _("\
-Set maintenance test-options completion result."), _("\
-Show maintenance test-options completion result."), _("\
-Show the results of completing\n\
+  add_cmd ("test-options-completion-result", class_maintenance,
+	   maintenance_show_test_options_completion_result,
+	   _("\
+Show maintenance test-options completion result.\n\
+Shows the results of completing\n\
 \"maint test-options require-delimiter\",\n\
 \"maint test-options unknown-is-error\", or\n\
 \"maint test-options unknown-is-operand\"."),
-			    NULL,
-			    maintenance_show_test_options_completion_result,
-			    &maintenance_set_cmdlist,
-			    &maintenance_show_cmdlist);
+	   &maintenance_show_cmdlist);
 }
diff --git a/gdb/testsuite/gdb.base/options.exp b/gdb/testsuite/gdb.base/options.exp
index f88e6a87d3e..1a652b3c9dc 100644
--- a/gdb/testsuite/gdb.base/options.exp
+++ b/gdb/testsuite/gdb.base/options.exp
@@ -486,21 +486,27 @@ proc_with_prefix test-misc {variant} {
     }
 
     # Completing at "-" should list all options.
-    res_test_gdb_complete_multiple "1" "$cmd " "-" "" $all_options
+    res_test_gdb_complete_multiple \
+	"1 [expect_none "-"]" \
+	"$cmd " "-" "" $all_options
 
     # Now with a double dash.
     gdb_test "$cmd --" [expect_none ""]
 
     # "--" is recognized by options completer, gdb auto-appends a
     # space.
-    test_completer_recognizes 1 "$cmd --"
+    test_completer_recognizes \
+	"1 [expect_none "--"]" \
+	"$cmd --"
 
     # Now with a double dash, plus a dash as operand.
     gdb_test "$cmd -- -" [expect_none "-"]
     res_test_gdb_complete_none "0 -" "$cmd -- -"
 
     # Completing an unambiguous option just appends an empty space.
-    test_completer_recognizes 1 "$cmd -flag"
+    test_completer_recognizes \
+	"1 [expect_none "-flag"]" \
+	"$cmd -flag"
 
     # Try running an ambiguous option.
     if {$variant == "require-delimiter"} {
@@ -540,10 +546,14 @@ proc_with_prefix test-flag {variant} {
     set cmd [make_cmd $variant]
 
     # Completing a flag just appends a space.
-    test_completer_recognizes 1 "$cmd -flag"
+    test_completer_recognizes \
+	"1 [expect_none "-flag"]" \
+	"$cmd -flag"
 
     # Add a dash, and all options should be shown.
-    test_gdb_complete_multiple "$cmd  -flag " "-" "" $all_options
+    res_test_gdb_complete_multiple \
+	"1 [expect_flag "-"]" \
+	"$cmd  -flag " "-" "" $all_options
 
     # Basic smoke tests of accepted / not accepted values.
 
@@ -582,7 +592,9 @@ proc_with_prefix test-flag {variant} {
     # "on/off".
 
     if {$variant == "require-delimiter"} {
-	res_test_gdb_complete_none "1" "$cmd -flag o"
+	res_test_gdb_complete_none \
+	    "1 [expect_flag "o"]" \
+	    "$cmd -flag o"
 
 	gdb_test "$cmd -flag o" [expect_none "-flag o"]
     } else {
@@ -612,7 +624,9 @@ proc_with_prefix test-boolean {variant} {
     #   E.g., "frame apply all -past-main COMMAND".
 
     if {$variant == "require-delimiter"} {
-	res_test_gdb_complete_multiple 1 "$cmd -bool " "" "" {
+	res_test_gdb_complete_multiple \
+	    "1 [expect_none ""]" \
+	    "$cmd -bool " "" "" {
 	    "-bool"
 	    "-enum"
 	    "-flag"
@@ -628,7 +642,9 @@ proc_with_prefix test-boolean {variant} {
     }
 
     # Add another dash, and "on/off" are no longer offered:
-    res_test_gdb_complete_multiple 1 "$cmd -bool " "-" ""  $all_options
+    res_test_gdb_complete_multiple \
+	"1 [expect_bool "-"]" \
+	"$cmd -bool " "-" ""  $all_options
 
     # Basic smoke tests of accepted / not accepted values.
 
@@ -643,20 +659,25 @@ proc_with_prefix test-boolean {variant} {
     # However, the completer does recognize them if you start typing
     # the boolean value.
     foreach value {"0" "1"} {
-	test_completer_recognizes 1 "$cmd -bool $value"
+	test_completer_recognizes \
+	    "1 [expect_none ""]" \
+	    "$cmd -bool $value"
     }
     foreach value {"of" "off"} {
-	res_test_gdb_complete_unique 1 \
+	res_test_gdb_complete_unique \
+	    "1 [expect_none ""]" \
 	    "$cmd -bool $value" \
 	    "$cmd -bool off"
     }
     foreach value {"y" "ye" "yes"} {
-	res_test_gdb_complete_unique 1 \
+	res_test_gdb_complete_unique \
+	    "1 [expect_none ""]" \
 	    "$cmd -bool $value" \
 	    "$cmd -bool yes"
     }
     foreach value {"n" "no"} {
-	res_test_gdb_complete_unique 1 \
+	res_test_gdb_complete_unique \
+	    "1 [expect_none ""]" \
 	    "$cmd -bool $value" \
 	    "$cmd -bool no"
     }
@@ -668,7 +689,8 @@ proc_with_prefix test-boolean {variant} {
 	"enabl"
 	"enable"
     } {
-	res_test_gdb_complete_unique 1 \
+	res_test_gdb_complete_unique \
+	    "1 [expect_none ""]" \
 	    "$cmd -bool $value" \
 	    "$cmd -bool enable"
     }
@@ -681,13 +703,16 @@ proc_with_prefix test-boolean {variant} {
 	"disabl"
 	"disable"
     } {
-	res_test_gdb_complete_unique 1 \
+	res_test_gdb_complete_unique \
+	    "1 [expect_none ""]" \
 	    "$cmd -bool $value" \
 	    "$cmd -bool disable"
     }
 
     if {$variant == "require-delimiter"} {
-	res_test_gdb_complete_none "1" "$cmd -bool xxx"
+	res_test_gdb_complete_none \
+	    "1 [expect_none "xxx"]" \
+	    "$cmd -bool xxx"
     } else {
 	res_test_gdb_complete_none "0 xxx" "$cmd -bool xxx"
     }
@@ -763,7 +788,9 @@ proc_with_prefix test-boolean {variant} {
     # Completing after a boolean option + "o" does list "on/off",
     # though.
     if {$variant == "require-delimiter"} {
-	res_test_gdb_complete_multiple 1 "$cmd -bool " "o" "" {
+	res_test_gdb_complete_multiple \
+	    "1 [expect_none "o"]" \
+	    "$cmd -bool " "o" "" {
 	    "off"
 	    "on"
 	}
@@ -783,17 +810,22 @@ proc_with_prefix test-uinteger {variant option} {
     set cmd "[make_cmd $variant] -$option"
 
     # Test completing a uinteger option:
-    res_test_gdb_complete_multiple 1 "$cmd " "" "" {
+    res_test_gdb_complete_multiple \
+	"1 [expect_none ""]" \
+	"$cmd " "" "" {
 	"NUMBER"
 	"unlimited"
     }
 
     # NUMBER above is just a placeholder, make sure we don't complete
     # it as a valid option.
-    res_test_gdb_complete_none 1 "$cmd NU"
+    res_test_gdb_complete_none \
+	"1 [expect_none "NU"]" \
+	"$cmd NU"
 
     # "unlimited" is valid though.
-    res_test_gdb_complete_unique 1 \
+    res_test_gdb_complete_unique \
+	"1 [expect_none "u"]" \
 	"$cmd u" \
 	"$cmd unlimited"
 
@@ -815,24 +847,34 @@ proc_with_prefix test-uinteger {variant option} {
 
     # Don't offer completions until we're past the
     # -uinteger/-zuinteger-unlimited argument.
-    res_test_gdb_complete_none 1 "$cmd 1"
+    res_test_gdb_complete_none \
+	"1 [expect_none ""]" \
+	"$cmd 1"
 
     # A number of invalid values.
     foreach value {"x" "x " "1a" "1a " "1-" "1- " "unlimitedx"} {
-	res_test_gdb_complete_none 1 "$cmd $value"
+	res_test_gdb_complete_none \
+	    "1 [expect_none $value]" \
+	    "$cmd $value"
     }
 
     # Try "-1".
     if {$option == "uinteger"} {
 	# -1 is invalid uinteger.
 	foreach value {"-1" "-1 "} {
-	    res_test_gdb_complete_none 1 "$cmd $value"
+	    res_test_gdb_complete_none \
+		"1 [expect_none ""]" \
+		"$cmd $value"
 	}
     } else {
 	# -1 is valid for zuinteger-unlimited.
-	res_test_gdb_complete_none 1 "$cmd -1"
+	res_test_gdb_complete_none \
+	    "1 [expect_none ""]" \
+	    "$cmd -1"
 	if {$variant == "require-delimiter"} {
-	    res_test_gdb_complete_multiple 1 "$cmd -1 " "" "-" $all_options
+	    res_test_gdb_complete_multiple \
+		"1 [expect_integer $option "unlimited" ""]" \
+		"$cmd -1 " "" "-" $all_options
 	} else {
 	    res_test_gdb_complete_none "0 " "$cmd -1 "
 	}
@@ -846,7 +888,9 @@ proc_with_prefix test-uinteger {variant option} {
     #  - for !require-delimiter commands, completion offers nothing
     #    and returns false.
     if {$variant == "require-delimiter"} {
-	res_test_gdb_complete_multiple 1 "$cmd 1 " "" "-" $all_options
+	res_test_gdb_complete_multiple \
+	    "1 [expect_integer $option 1 ""]" \
+	    "$cmd 1 " "" "-" $all_options
     } else {
 	res_test_gdb_complete_none "0 " "$cmd 1 "
     }
@@ -854,7 +898,9 @@ proc_with_prefix test-uinteger {variant option} {
     # Test completing non-option arguments after "-uinteger 1 ".
     foreach operand {"x" "x " "1a" "1a " "1-" "1- "} {
 	if {$variant == "require-delimiter"} {
-	    res_test_gdb_complete_none 1 "$cmd 1 $operand"
+	    res_test_gdb_complete_none \
+		"1 [expect_integer $option 1 $operand]" \
+		"$cmd 1 $operand"
 	} else {
 	    res_test_gdb_complete_none "0 $operand" "$cmd 1 $operand"
 	}
@@ -864,7 +910,9 @@ proc_with_prefix test-uinteger {variant option} {
 	if {$variant == "unknown-is-operand"} {
 	    res_test_gdb_complete_none "0 $operand" "$cmd 1 $operand"
 	} else {
-	    res_test_gdb_complete_none 1 "$cmd 1 $operand"
+	    res_test_gdb_complete_none \
+		"1 [expect_integer $option 1 $operand]" \
+		"$cmd 1 $operand"
 	}
     }
 }
@@ -873,7 +921,9 @@ proc_with_prefix test-uinteger {variant option} {
 proc_with_prefix test-enum {variant} {
     set cmd [make_cmd $variant]
 
-    res_test_gdb_complete_multiple 1 "$cmd -enum " "" "" {
+    res_test_gdb_complete_multiple \
+	"1 [expect_none ""]" \
+	"$cmd -enum " "" "" {
 	"xxx"
 	"yyy"
 	"zzz"
@@ -882,7 +932,9 @@ proc_with_prefix test-enum {variant} {
     # Check that "-" where a value is expected does not show the
     # command's options.  I.e., an enum's value is not optional.
     # Check both completion and running the command.
-    res_test_gdb_complete_none 1 "$cmd -enum -"
+    res_test_gdb_complete_none \
+	"1 [expect_none "-"]" \
+	"$cmd -enum -"
     gdb_test "$cmd -enum --"\
 	"Requires an argument. Valid arguments are xxx, yyy, zzz\\."
 
-- 
2.14.5

[PATCH 5/5] pipe command completer

[Pedro Alves]

This commit adds a completer for the "pipe" command.  It can complete
"pipe"'s options, and the specified GDB command.

To make the completer aware of the "-d" option, this converts the
option processing to use gdb::option.

Tests included.

gdb/ChangeLog:
yyyy-mm-dd  Pedro Alves  <palves@redhat.com>

	* cli/cli-cmds.c (struct pipe_cmd_opts): New.
	(pipe_cmd_option_defs): New.
	(make_pipe_cmd_options_def_group): New.
	(pipe_command): Use gdb::option::process_options.
	(pipe_command_completer): New function.
	(_initialize_cli_cmds): Install completer for "pipe" command.

gdb/testsuite/ChangeLog:
yyyy-mm-dd  Pedro Alves  <palves@redhat.com>

	* gdb.base/shell.exp: Load completion-support.exp.
	Adjust expected error output.  Add completion tests.
---
 gdb/cli/cli-cmds.c               | 92 ++++++++++++++++++++++++++++++++++++----
 gdb/testsuite/gdb.base/shell.exp | 47 +++++++++++++++++++-
 2 files changed, 128 insertions(+), 11 deletions(-)

diff --git a/gdb/cli/cli-cmds.c b/gdb/cli/cli-cmds.c
index bc32fbbaf88..054d80b9b96 100644
--- a/gdb/cli/cli-cmds.c
+++ b/gdb/cli/cli-cmds.c
@@ -960,32 +960,68 @@ edit_command (const char *arg, int from_tty)
   xfree (p);
 }
 
+/* The options for the "pipe" command.  */
+
+struct pipe_cmd_opts
+{
+  /* For "-d".  */
+  char *delimiter = nullptr;
+
+  ~pipe_cmd_opts ()
+  {
+    xfree (delimiter);
+  }
+};
+
+static const gdb::option::option_def pipe_cmd_option_defs[] = {
+
+  gdb::option::string_option_def<pipe_cmd_opts> {
+    "d",
+    [] (pipe_cmd_opts *opts) { return &opts->delimiter; },
+    nullptr,
+    N_("Indicates to use the specified delimiter string to separate\n\
+COMMAND from SHELL_COMMAND, in alternative to |.  This is useful in\n\
+case COMMAND contains a | character."),
+  },
+
+};
+
+/* Create an option_def_group for the "pipe" command's options, with
+   OPTS as context.  */
+
+static inline gdb::option::option_def_group
+make_pipe_cmd_options_def_group (pipe_cmd_opts *opts)
+{
+  return {{pipe_cmd_option_defs}, opts};
+}
+
 /* Implementation of the "pipe" command.  */
 
 static void
 pipe_command (const char *arg, int from_tty)
 {
-  std::string delim ("|");
+  pipe_cmd_opts opts;
 
-  if (arg != nullptr && check_for_argument (&arg, "-d", 2))
-    {
-      delim = extract_arg (&arg);
-      if (delim.empty ())
-	error (_("Missing delimiter DELIM after -d"));
-    }
+  auto grp = make_pipe_cmd_options_def_group (&opts);
+  gdb::option::process_options
+    (&arg, gdb::option::PROCESS_OPTIONS_UNKNOWN_IS_OPERAND, grp);
+
+  const char *delim = "|";
+  if (opts.delimiter != nullptr)
+    delim = opts.delimiter;
 
   const char *command = arg;
   if (command == nullptr)
     error (_("Missing COMMAND"));
 
-  arg = strstr (arg, delim.c_str ());
+  arg = strstr (arg, delim);
 
   if (arg == nullptr)
     error (_("Missing delimiter before SHELL_COMMAND"));
 
   std::string gdb_cmd (command, arg - command);
 
-  arg += delim.length (); /* Skip the delimiter.  */
+  arg += strlen (delim); /* Skip the delimiter.  */
 
   if (gdb_cmd.empty ())
     gdb_cmd = repeat_previous ();
@@ -1019,6 +1055,43 @@ pipe_command (const char *arg, int from_tty)
   exit_status_set_internal_vars (exit_status);
 }
 
+/* Completer for the pipe command.  */
+
+static void
+pipe_command_completer (struct cmd_list_element *ignore,
+			completion_tracker &tracker,
+			const char *text, const char *word_ignored)
+{
+  pipe_cmd_opts opts;
+
+  const char *org_text = text;
+  auto grp = make_pipe_cmd_options_def_group (&opts);
+  if (gdb::option::complete_options
+      (tracker, &text, gdb::option::PROCESS_OPTIONS_UNKNOWN_IS_OPERAND, grp))
+    return;
+
+  const char *delimiter = "|";
+  if (opts.delimiter != nullptr)
+    delimiter = opts.delimiter;
+
+  /* Check if we're past option values already.  */
+  if (text > org_text && !isspace (text[-1]))
+    return;
+
+  const char *delim = strstr (text, delimiter);
+
+  /* If we're still not past the delimiter, complete the gdb
+     command.  */
+  if (delim == nullptr || delim == text)
+    {
+      complete_nested_command_line (tracker, text);
+      return;
+    }
+
+  /* We're past the delimiter.  What follows is a shell command, which
+     we don't know how to complete.  */
+}
+
 static void
 list_command (const char *arg, int from_tty)
 {
@@ -2044,6 +2117,7 @@ case COMMAND contains a | character.\n\
 \n\
 With no COMMAND, repeat the last executed command\n\
 and send its output to SHELL_COMMAND."));
+  set_cmd_completer_handle_brkchars (c, pipe_command_completer);
   add_com_alias ("|", "pipe", class_support, 0);
 
   add_com ("list", class_files, list_command, _("\
diff --git a/gdb/testsuite/gdb.base/shell.exp b/gdb/testsuite/gdb.base/shell.exp
index 2136d48919f..719435d4eb5 100644
--- a/gdb/testsuite/gdb.base/shell.exp
+++ b/gdb/testsuite/gdb.base/shell.exp
@@ -15,6 +15,8 @@
 
 # Test that the "shell", "!", "|" and "pipe" commands work.
 
+load_lib completion-support.exp
+
 gdb_exit
 gdb_start
 
@@ -92,8 +94,8 @@ gdb_test "p \$_shell_exitsignal" " = 2" "pipe interrupt exitsignal"
 
 # Error handling verifications.
 gdb_test "|" "Missing COMMAND" "all missing"
-gdb_test "|-d" "Missing delimiter DELIM after -d" "-d value missing"
-gdb_test "|-d    " "Missing delimiter DELIM after -d" "-d spaces value missing"
+gdb_test "|-d" "-d requires an argument" "-d value missing"
+gdb_test "|-d    " "-d requires an argument" "-d spaces value missing"
 gdb_test "| echo coucou" \
     "Missing delimiter before SHELL_COMMAND" \
     "| delimiter missing"
@@ -110,3 +112,44 @@ gdb_test "|-d! echo coucou ! wc" \
     "Missing delimiter before SHELL_COMMAND" \
     "delimiter missing due to missing space"
 
+# Completion tests.
+
+test_gdb_complete_unique \
+    "pipe" \
+    "pipe"
+
+# Note that unlike "pipe", "|" doesn't require a space.  This checks
+# that completion behaves that way too.
+foreach cmd {"pipe " "| " "|"} {
+    test_gdb_completion_offers_commands "$cmd"
+
+    # There's only one option.
+    test_gdb_complete_unique \
+	"${cmd}-" \
+	"${cmd}-d"
+
+    # Cannot complete "-d"'s argument.
+    test_gdb_complete_none "${cmd}-d "
+    test_gdb_complete_none "${cmd}-d main"
+
+    # Check completing a GDB command, with and without -d.
+    test_gdb_complete_unique \
+	"${cmd}maint set test-se" \
+	"${cmd}maint set test-settings"
+    test_gdb_complete_unique \
+	"${cmd}-d XXX maint set test-se" \
+	"${cmd}-d XXX maint set test-settings"
+
+    # Check that GDB doesn't try to complete the shell command.
+    test_gdb_complete_none \
+	"${cmd}print 1 | "
+
+    # Same, while making sure that the completer understands "-d".
+    test_gdb_complete_unique \
+	"${cmd}-d XXX maint set" \
+	"${cmd}-d XXX maint set"
+    test_gdb_complete_none \
+	"${cmd}-d set maint set"
+    test_gdb_complete_none \
+	"${cmd}-d set maint set "
+}
-- 
2.14.5

[PATCH 4/5] Fix latent bug in test_gdb_complete_cmd_multiple

[Pedro Alves]

A following patch will add the following to a testcase:

  test_gdb_completion_offers_commands "| "

And that tripped on a latent testsuite bug:

 (gdb) | PASS: gdb.base/shell.exp: tab complete "| "
 ^CQuit
 (gdb) complete |
 | !
 | +
 PASS: gdb.base/shell.exp: cmd complete "| "
 |  *** List may be truncated, max-completions reached. ***
 (gdb) FAIL: gdb.base/shell.exp: set max-completions 200
 set max-completions 200

The issue is that "|" ends up as part of a regexp, and "|" in regexps
has a special meaning...

Fix this with string_to_regexp.

gdb/testsuite/ChangeLog:
yyyy-mm-dd  Pedro Alves  <palves@redhat.com>

	* lib/completion-support.exp (test_gdb_complete_cmd_multiple): Use
	string_to_regexp.
---
 gdb/testsuite/lib/completion-support.exp | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/gdb/testsuite/lib/completion-support.exp b/gdb/testsuite/lib/completion-support.exp
index 3199e85fd4d..abe48b4a7f7 100644
--- a/gdb/testsuite/lib/completion-support.exp
+++ b/gdb/testsuite/lib/completion-support.exp
@@ -200,8 +200,9 @@ proc test_gdb_complete_cmd_multiple { cmd_prefix completion_word completion_list
     set expected_re [make_cmd_completion_list_re $cmd_prefix $completion_list $start_quote_char $end_quote_char]
 
     if {$max_completions} {
+	set cmd_prefix_re [string_to_regexp $cmd_prefix]
 	append expected_re \
-	    "$cmd_prefix \\*\\*\\* List may be truncated, max-completions reached\\. \\*\\*\\*.*\r\n"
+	    "$cmd_prefix_re \\*\\*\\* List may be truncated, max-completions reached\\. \\*\\*\\*.*\r\n"
     }
 
     set cmd_re [string_to_regexp "complete $cmd_prefix$completion_word"]
-- 
2.14.5

Re: [PATCH 4/5] Fix latent bug in test_gdb_complete_cmd_multiple

[Tom Tromey]

>>>>> "Pedro" == Pedro Alves <palves@redhat.com> writes:

Pedro>      if {$max_completions} {
Pedro> +	set cmd_prefix_re [string_to_regexp $cmd_prefix]
Pedro>  	append expected_re \
Pedro> -	    "$cmd_prefix \\*\\*\\* List may be truncated, max-completions reached\\. \\*\\*\\*.*\r\n"
Pedro> +	    "$cmd_prefix_re \\*\\*\\* List may be truncated, max-completions reached\\. \\*\\*\\*.*\r\n"
Pedro>      }
 
Pedro>      set cmd_re [string_to_regexp "complete $cmd_prefix$completion_word"]

It looks like this is going to end up running string_to_regexp twice --
once in the "if" and then again here, which is probably not desirable.

Tom

Re: [PATCH 4/5] Fix latent bug in test_gdb_complete_cmd_multiple

[Pedro Alves]

On 6/28/19 3:53 PM, Tom Tromey wrote:
>>>>>> "Pedro" == Pedro Alves <palves@redhat.com> writes:
> 
> Pedro>      if {$max_completions} {
> Pedro> +	set cmd_prefix_re [string_to_regexp $cmd_prefix]
> Pedro>  	append expected_re \
> Pedro> -	    "$cmd_prefix \\*\\*\\* List may be truncated, max-completions reached\\. \\*\\*\\*.*\r\n"
> Pedro> +	    "$cmd_prefix_re \\*\\*\\* List may be truncated, max-completions reached\\. \\*\\*\\*.*\r\n"
> Pedro>      }
>  
> Pedro>      set cmd_re [string_to_regexp "complete $cmd_prefix$completion_word"]
> 
> It looks like this is going to end up running string_to_regexp twice --
> once in the "if" and then again here, which is probably not desirable.

It won't because the second time is still referring to $cmd_prefix,
while the new code introduces a new $cmd_prefix_re variable.

Thanks,
Pedro Alves

Re: [PATCH 3/5] Teach gdb::option about string options

[Tom Tromey]

>>>>> "Pedro" == Pedro Alves <palves@redhat.com> writes:

Pedro> This commit adds support for string options, mapped to var_string.
Pedro> For now, a string is parsed up until the first whitespace.  I imagine
Pedro> that we'll need to add support for quoting so that we could do:

Pedro>  (gdb) cmd -option 'some -string'

Pedro> without gdb confusing the "-string" for an option.

Pedro> This doesn't seem important for pipe, so I'm leaving it for another
Pedro> day.

I wonder if we should file bugs for known holes like this.
On the one hand it seems nice to write down what we know.
On the other hand, maybe nobody will ever look at these.

Pedro> +  /* Disable the copy constructor.  */
Pedro> +  option_def_and_value (const option_def_and_value &rval) = delete;

I wonder if it makes sense to disable operator= as well.

Tom

Re: [PATCH 0/5] pipe command completer, and string options

[Tom Tromey]

>>>>> "Pedro" == Pedro Alves <palves@redhat.com> writes:

Pedro> Tromey noticed that the "pipe" command doesn't have a completer.
Pedro> Initially I said I wasn't planning on looking at it, thinking that all
Pedro> the building blocks were already in place.  But I quickly realized
Pedro> that there's one piece missing -- string options...  So I took a stab
Pedro> at implementing it.  Of course, lucky me, I ran into other issues and
Pedro> latent bugs, now all addressed...

I read through these and sent whatever comments I had.
Thanks again for doing this.

Tom

Re: [PATCH 5/5] pipe command completer

[Tom Tromey]

>>>>> "Pedro" == Pedro Alves <palves@redhat.com> writes:

Pedro> This commit adds a completer for the "pipe" command.  It can complete
Pedro> "pipe"'s options, and the specified GDB command.

Pedro> To make the completer aware of the "-d" option, this converts the
Pedro> option processing to use gdb::option.

Thank you for doing this.

Pedro> gdb/ChangeLog:
Pedro> yyyy-mm-dd  Pedro Alves  <palves@redhat.com>

Pedro> 	* cli/cli-cmds.c (struct pipe_cmd_opts): New.
Pedro> 	(pipe_cmd_option_defs): New.

BTW this is PR cli/24732.

Tom

Re: [PATCH 4/5] Fix latent bug in test_gdb_complete_cmd_multiple

[Tom Tromey]

>>>>> "Pedro" == Pedro Alves <palves@redhat.com> writes:

Pedro> On 6/28/19 3:53 PM, Tom Tromey wrote:
>>>>>>> "Pedro" == Pedro Alves <palves@redhat.com> writes:
>> 
Pedro> if {$max_completions} {
Pedro> +	set cmd_prefix_re [string_to_regexp $cmd_prefix]
Pedro> append expected_re \
Pedro> -	    "$cmd_prefix \\*\\*\\* List may be truncated, max-completions reached\\. \\*\\*\\*.*\r\n"
Pedro> +	    "$cmd_prefix_re \\*\\*\\* List may be truncated, max-completions reached\\. \\*\\*\\*.*\r\n"
Pedro> }
>> 
Pedro> set cmd_re [string_to_regexp "complete $cmd_prefix$completion_word"]
>> 
>> It looks like this is going to end up running string_to_regexp twice --
>> once in the "if" and then again here, which is probably not desirable.

Pedro> It won't because the second time is still referring to $cmd_prefix,
Pedro> while the new code introduces a new $cmd_prefix_re variable.

Indeed, I misread that.  Thanks.

Tom

Re: [PATCH 3/5] Teach gdb::option about string options

[Pedro Alves]

On 6/28/19 4:11 PM, Tom Tromey wrote:
>>>>>> "Pedro" == Pedro Alves <palves@redhat.com> writes:
> 
> Pedro> This commit adds support for string options, mapped to var_string.
> Pedro> For now, a string is parsed up until the first whitespace.  I imagine
> Pedro> that we'll need to add support for quoting so that we could do:
> 
> Pedro>  (gdb) cmd -option 'some -string'
> 
> Pedro> without gdb confusing the "-string" for an option.
> 
> Pedro> This doesn't seem important for pipe, so I'm leaving it for another
> Pedro> day.
> 
> I wonder if we should file bugs for known holes like this.
> On the one hand it seems nice to write down what we know.
> On the other hand, maybe nobody will ever look at these.

Yeah on the later.  I don't see anyone fixing that until
we have some option that requires it, and then, whoever
implements such an option will quickly run into it for sure.

> 
> Pedro> +  /* Disable the copy constructor.  */
> Pedro> +  option_def_and_value (const option_def_and_value &rval) = delete;
> 
> I wonder if it makes sense to disable operator= as well.
Indeed, I don't recall why I didn't use DISABLE_COPY_AND_ASSIGN.
I've done that now.

Thanks,
Pedro Alves

Re: [PATCH 5/5] pipe command completer

[Pedro Alves]

On 6/28/19 4:16 PM, Tom Tromey wrote:
>>>>>> "Pedro" == Pedro Alves <palves@redhat.com> writes:
> 
> Pedro> This commit adds a completer for the "pipe" command.  It can complete
> Pedro> "pipe"'s options, and the specified GDB command.
> 
> Pedro> To make the completer aware of the "-d" option, this converts the
> Pedro> option processing to use gdb::option.
> 
> Thank you for doing this.
> 
> Pedro> gdb/ChangeLog:
> Pedro> yyyy-mm-dd  Pedro Alves  <palves@redhat.com>
> 
> Pedro> 	* cli/cli-cmds.c (struct pipe_cmd_opts): New.
> Pedro> 	(pipe_cmd_option_defs): New.
> 
> BTW this is PR cli/24732.

Thanks, I added that to the ChangeLogs and pushed this in.

Thanks,
Pedro Alves