From patchwork Thu Jun 27 19:14:25 2019 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Pedro Alves X-Patchwork-Id: 33463 Received: (qmail 86040 invoked by alias); 27 Jun 2019 19:14:36 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Unsubscribe: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org Delivered-To: mailing list gdb-patches@sourceware.org Received: (qmail 85917 invoked by uid 89); 27 Jun 2019 19:14:35 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-25.9 required=5.0 tests=AWL, BAYES_00, GIT_PATCH_0, GIT_PATCH_1, GIT_PATCH_2, GIT_PATCH_3, SPF_HELO_PASS autolearn=ham version=3.3.1 spammy=tackle, quoting X-HELO: mx1.redhat.com Received: from mx1.redhat.com (HELO mx1.redhat.com) (209.132.183.28) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Thu, 27 Jun 2019 19:14:32 +0000 Received: from smtp.corp.redhat.com (int-mx01.intmail.prod.int.phx2.redhat.com [10.5.11.11]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 8B1C866995 for ; Thu, 27 Jun 2019 19:14:31 +0000 (UTC) Received: from localhost.localdomain (ovpn04.gateway.prod.ext.ams2.redhat.com [10.39.146.4]) by smtp.corp.redhat.com (Postfix) with ESMTP id 1BC8B600CC for ; Thu, 27 Jun 2019 19:14:30 +0000 (UTC) From: Pedro Alves To: gdb-patches@sourceware.org Subject: [PATCH 3/5] Teach gdb::option about string options Date: Thu, 27 Jun 2019 20:14:25 +0100 Message-Id: <20190627191427.20742-4-palves@redhat.com> In-Reply-To: <20190627191427.20742-1-palves@redhat.com> References: <20190627191427.20742-1-palves@redhat.com> 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 * cli/cli-option.c (union option_value) : 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) : New field. (struct string_option_def): New. * maint-test-options.c (struct test_options_opts) : 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 * 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 value; + + /* Constructor. */ + option_def_and_value (const option_def &option_, void *ctx_, + gdb::optional &&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 &value) + { + if (value.has_value ()) + { + if (option.type == var_string) + value->string = nullptr; + } + } }; static void save_option_value_in_ctx (gdb::optional &ov); @@ -373,6 +425,25 @@ parse_option (gdb::array_view 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 &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 +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; + } +}; + /* 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 { + "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