Teach gdb::option about string options
authorPedro Alves <palves@redhat.com>
Wed, 3 Jul 2019 15:57:49 +0000 (16:57 +0100)
committerPedro Alves <palves@redhat.com>
Wed, 3 Jul 2019 15:59:24 +0000 (16:59 +0100)
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:
2019-07-03  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
use DISABLE_COPY_AND_ASSIGN.
(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): Add default
ctor and use DISABLE_COPY_AND_ASSIGN.
<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:
2019-07-03  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/ChangeLog
gdb/cli/cli-option.c
gdb/cli/cli-option.h
gdb/maint-test-options.c
gdb/testsuite/ChangeLog
gdb/testsuite/gdb.base/options.exp

index 43f0ef14cec43bd880f6ee6f9290757ad0a2742c..b08686beb2f6209fba010f2c8859374d4450b5e3 100644 (file)
@@ -1,3 +1,21 @@
+2019-07-03  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
+       use DISABLE_COPY_AND_ASSIGN.
+       (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): Add default
+       ctor and use DISABLE_COPY_AND_ASSIGN.
+       <string_opt>: New field.
+       (test_options_opts::~test_options_opts): New.
+       (test_options_opts::dump): Also dump "-string".
+       (test_options_option_defs): Install "string.
+
 2019-07-03  Pedro Alves  <palves@redhat.com>
 
        * cli/cli-option.c (parse_option) <var_enum>: Don't return an
index 8f2844610b56d55ae95be5d08beb2cd0910a59c2..07d552b7f5bbdaff7bc6f11a4e5c0ba999879ef6 100644 (file)
@@ -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,54 @@ 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_COPY_AND_ASSIGN (option_def_and_value);
+
+  ~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 +424,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 +602,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 +679,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 +808,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"));
     }
index 1bfbfce1ce512991f85f6afaa13274a99fa57df5..a26b52f7f29c92fb21bd7deab49442d99a2a5034 100644 (file)
@@ -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
index 7e7ef6e799261386af045e13b0ac20f76ba91d50..4caa865ae432f4edac1dea0a8fabf860f093ebc0 100644 (file)
    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,16 @@ 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 () = default;
+
+  DISABLE_COPY_AND_ASSIGN (test_options_opts);
+
+  ~test_options_opts ()
+  {
+    xfree (string_opt);
+  }
 
   /* Dump the options to FILE.  ARGS is the remainder unprocessed
      arguments.  */
@@ -140,7 +150,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 +162,9 @@ struct test_options_opts
                        (zuint_unl_opt == -1
                         ? "unlimited"
                         : plongest (zuint_unl_opt)),
+                       (string_opt != nullptr
+                        ? string_opt
+                        : ""),
                        args);
   }
 };
@@ -216,6 +229,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
index 209f15f9c352c17c9947d1476d31a85600cfc311..8a426ffc87714956d3e9c0130c6d2fce2431bed2 100644 (file)
@@ -1,3 +1,13 @@
+2019-07-03  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.
+
 2019-07-03  Pedro Alves  <palves@redhat.com>
 
        * gdb.base/options.exp (test-misc, test-flag, test-boolean)
index 1a652b3c9dc0ed358509264c6653713e80d638ad..e8f571d9ba9a712537d1d97123ca754cc731a986 100644 (file)
@@ -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