From patchwork Wed Jun 19 13:05:26 2019 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Patchwork-Submitter: Pedro Alves X-Patchwork-Id: 33209 Received: (qmail 93528 invoked by alias); 19 Jun 2019 13:05:32 -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 93460 invoked by uid 89); 19 Jun 2019 13:05:32 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-26.9 required=5.0 tests=BAYES_00, GIT_PATCH_0, GIT_PATCH_1, GIT_PATCH_2, GIT_PATCH_3, RCVD_IN_DNSWL_NONE autolearn=ham version=3.3.1 spammy=with_command, sk:with_co X-HELO: mail-wr1-f66.google.com Received: from mail-wr1-f66.google.com (HELO mail-wr1-f66.google.com) (209.85.221.66) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Wed, 19 Jun 2019 13:05:30 +0000 Received: by mail-wr1-f66.google.com with SMTP id c2so3315002wrm.8 for ; Wed, 19 Jun 2019 06:05:29 -0700 (PDT) Return-Path: Received: from ?IPv6:2001:8a0:f913:f700:56ee:75ff:fe8d:232b? ([2001:8a0:f913:f700:56ee:75ff:fe8d:232b]) by smtp.gmail.com with ESMTPSA id x3sm21912382wrp.78.2019.06.19.06.05.27 (version=TLS1_3 cipher=AEAD-AES128-GCM-SHA256 bits=128/128); Wed, 19 Jun 2019 06:05:27 -0700 (PDT) Subject: Re: [PATCH v2 0/4] Introduce the "with" command To: Philippe Waroquiers , gdb-patches@sourceware.org References: <20190618003902.19805-1-palves@redhat.com> <1560904492.8865.12.camel@skynet.be> From: Pedro Alves Message-ID: <594eaa7a-3e28-74e7-c5a1-2f9297a8d5d9@redhat.com> Date: Wed, 19 Jun 2019 14:05:26 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101 Thunderbird/60.2.1 MIME-Version: 1.0 In-Reply-To: <1560904492.8865.12.camel@skynet.be> On 6/19/19 1:34 AM, Philippe Waroquiers wrote: >> New in v2: > I played a little bit with this version, no bug encountered. > Thanks much, again. > 2 small nits in the error message for unknown 'with settings': > (gdb) with xxxx yyyy -- echo coucou > Undefined withcommand: "xxxx".  Try "help wit". > (gdb)  > > (this message is produced by lookup_cmd, that is not too > much 'with' aware it seems ...) Eh, somehow I missed adding a testcase for this scenario. Thanks for noticing this. I had code in with_command_1 to throw on unknown setting: if (set_cmd == nullptr) error (_("Unknown setting %s"), cmd); but it's not reachable, because I'm passing "0" as allow_unknown argument to lookup_cmd... I tried passing "1" instead, and, of course that error becomes reachable. However, if the setting is a prefixed setting, like "with print elements", then lookup_cmd still throws an error, near the bottom, from the else branch: if (c->prefixlist && **line && !c->allow_unknown) undef_cmd_error (c->prefixname, *line); regardless of the allow_unknown parameter. This made me reconsider, and I'm thinking that indeed, passing allow_unknown as false is the right thing to do, so that we get consistent error messages: (gdb) with foo Undefined set command: "foo". Try "help set". (gdb) with print foo Undefined set print command: "foo". Try "help set print". (gdb) maint with foo Undefined maintenance set command: "foo". Try "help maintenance set". (gdb) maint with test-settings foo Undefined maintenance set test-settings command: "foo". Try "help maintenance set test-settings". I'm thinking that the errors talking about "set" instead of "with" can be seen as a feature. If you consider the prefixed case, like "with print foo", the error is telling you where to look at the available settings for that prefix. Looking at the lookup_cmd code, I realized that I was missing a test for ambiguous settings too. Now added. The funny missing space and 'h' were because we're supposed to include a space in the CMDTYPE argument passed to lookup_cmd, and I was passing "with", with no space. I added a new parameter to with_command_1, so that we can pass down "maintenance with " too. The completer function already had the same parameter. WDYT? From 4db38e12c0b393231a7af955a1bd4e9573bfe19e Mon Sep 17 00:00:00 2001 From: Pedro Alves Date: Wed, 19 Jun 2019 13:29:08 +0100 Subject: [PATCH] fix --- gdb/cli/cli-cmds.c | 14 +++++++------- gdb/cli/cli-cmds.h | 8 +++++--- gdb/maint.c | 2 +- gdb/testsuite/gdb.base/with.exp | 18 ++++++++++++++++++ 4 files changed, 31 insertions(+), 11 deletions(-) diff --git a/gdb/cli/cli-cmds.c b/gdb/cli/cli-cmds.c index 0f5da72e128..283bc446026 100644 --- a/gdb/cli/cli-cmds.c +++ b/gdb/cli/cli-cmds.c @@ -214,7 +214,8 @@ show_command (const char *arg, int from_tty) /* See cli/cli-cmds.h. */ void -with_command_1 (cmd_list_element *setlist, const char *args, int from_tty) +with_command_1 (const char *set_cmd_prefix, + cmd_list_element *setlist, const char *args, int from_tty) { const char *delim = strstr (args, "--"); const char *nested_cmd = nullptr; @@ -225,11 +226,10 @@ with_command_1 (cmd_list_element *setlist, const char *args, int from_tty) if (delim == nullptr || *skip_spaces (&delim[2]) == '\0') nested_cmd = repeat_previous (); - const char *cmd = args; - cmd_list_element *set_cmd = lookup_cmd (&args, setlist, "with", 0, 1); - - if (set_cmd == nullptr) - error (_("Unknown setting %s"), cmd); + cmd_list_element *set_cmd = lookup_cmd (&args, setlist, set_cmd_prefix, + /*allow_unknown=*/ 0, + /*ignore_help_classes=*/ 1); + gdb_assert (set_cmd != nullptr); if (set_cmd->var == nullptr) error (_("Cannot use this setting with the \"with\" command")); @@ -308,7 +308,7 @@ with_command_completer_1 (const char *set_cmd_prefix, static void with_command (const char *args, int from_tty) { - with_command_1 (setlist, args, from_tty); + with_command_1 ("set ", setlist, args, from_tty); } /* "with" command completer. */ diff --git a/gdb/cli/cli-cmds.h b/gdb/cli/cli-cmds.h index 2c8e9a676c1..94e210a84eb 100644 --- a/gdb/cli/cli-cmds.h +++ b/gdb/cli/cli-cmds.h @@ -143,9 +143,11 @@ extern int source_verbose; extern int trace_commands; /* Common code for the "with" and "maintenance with" commands. - SETLIST is the command list for corresponding "set" command: i.e., - "set" or "maintenance set". */ -extern void with_command_1 (cmd_list_element *setlist, + SET_CMD_PREFIX is the spelling of the corresponding "set" command + prefix: i.e., "set " or "maintenance set ". SETLIST is the command + element for the same "set" command prefix. */ +extern void with_command_1 (const char *set_cmd_prefix, + cmd_list_element *setlist, const char *args, int from_tty); /* Common code for the completers of the "with" and "maintenance with" diff --git a/gdb/maint.c b/gdb/maint.c index 10bb4fe7e9a..2c10903539b 100644 --- a/gdb/maint.c +++ b/gdb/maint.c @@ -640,7 +640,7 @@ maintenance_show_cmd (const char *args, int from_tty) static void maintenance_with_cmd (const char *args, int from_tty) { - with_command_1 (maintenance_set_cmdlist, args, from_tty); + with_command_1 ("maintenance set ", maintenance_set_cmdlist, args, from_tty); } /* "maintenance with" command completer. */ diff --git a/gdb/testsuite/gdb.base/with.exp b/gdb/testsuite/gdb.base/with.exp index b1d3adb3fda..9ea768563a3 100644 --- a/gdb/testsuite/gdb.base/with.exp +++ b/gdb/testsuite/gdb.base/with.exp @@ -220,6 +220,24 @@ with_test_prefix "run control" { # Check errors. with_test_prefix "errors" { + # Try both an unknown root setting and an unknown prefixed + # setting. The errors come from different locations in the + # sources. + gdb_test "with xxxx yyyy" \ + "Undefined set command: \"xxxx\". Try \"help set\"\\." + gdb_test "with print xxxx yyyy" \ + "Undefined set print command: \"xxxx yyyy\". Try \"help set print\"\\." + # Try one error case for "maint with", to make sure the right + # "maintenance with" prefix is shown. + gdb_test "maint with xxxx yyyy" \ + "Undefined maintenance set command: \"xxxx\". Try \"help maintenance set\"\\." + + # Try ambiguous settings. + gdb_test "with w" \ + "Ambiguous set command \"w\": watchdog, width, write\\." + gdb_test "with print m" \ + "Ambiguous set print command \"m\": max-depth, max-symbolic-offset\\." + gdb_test "with variable xxx=1" \ "Cannot use this setting with the \"with\" command"