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

Message ID 20190627191427.20742-4-palves@redhat.com
State New
Headers show
Series
  • pipe command completer, and string options
Related show

Commit Message

Pedro Alves June 27, 2019, 7:14 p.m.
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(-)

-- 
2.14.5

Comments

Tom Tromey June 28, 2019, 3:11 p.m. | #1
>>>>> "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
Pedro Alves July 3, 2019, 4:25 p.m. | #2
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

Patch

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