[v2,0/4] Introduce the "with" command

Message ID 20190618003902.19805-1-palves@redhat.com
Headers show
Series
  • Introduce the "with" command
Related show

Message

Pedro Alves June 18, 2019, 12:38 a.m.
( See original discussion and prototype here:
   https://sourceware.org/ml/gdb-patches/2019-05/msg00570.html )

 (gdb) help with
 Temporarily set SETTING to VALUE, run COMMAND, and restore SETTING.
 Usage: with SETTING [VALUE] [-- COMMAND]
 Usage: w SETTING [VALUE] [-- COMMAND]
 With no COMMAND, repeats the last executed command.
 SETTING is any setting settable with the "set" command.
 E.g.:
   with language pascal -- print obj
   with print elements unlimited -- print obj

More details in patch #4.

New in v2:

 - Now a series of 4 patches instead of a single patch.  The main
   patch is patch #4.  This patch includes documentation bits.

 - The "with" command's implementation moved from printcmd.c to
   cli/cli-cmds.c, near "show".

 - Philippe pointed out a bug in v1.  The issue was related to how on
   var_uinteger commands, "unlimited" is user-visible as "0", but
   stored internally in the command's control variable as "-1".
   Without proper internal/user-visible translation, restoring a
   setting's original value failed, if the setting was originally set
   to "unlimited".  In order to fix that, in v2 I'm reusing code from
   do_show_command to convert the set/show command's control variable
   to a string representation.

 - In order to thoroughly test the point above, I thought I'd reuse
   the recently introduced "maint test-settings set/show
   uinteger/zuinteger-unlimited/..." commands, in order to test "with"
   against all command type variants (all enum var_types).  But,
   instead of adding a new "maint test-settings with" command just for
   that, I thought that it was better if we added a more broadly
   usable "maint with" command that worked with all maintenance
   settings.  So the series includes a patch to rename "maint
   test-settings set/show" to "maint set/show test-settings".  That's
   patch #3.  That patch includes documentation bits, though
   of borderline-obvious kind.

- Making the new gdb.base/with.exp testcase exercise "maint with
  test-settings" uncovered bugs in the "maint set/show test-settings"
  settings.  Those are fixed by patch #1.

Pedro Alves (4):
  Fix defaults of some "maint test-settings" subcommands
  Fix a few comments in maint-test-settings.c
  "maint test-settings set/show" -> "maint set/show test-settings"
  Introduce the "with" command

 gdb/doc/gdb.texinfo                 | 101 +++++++++++++-
 gdb/NEWS                            |  18 ++-
 gdb/cli/cli-cmds.c                  | 129 +++++++++++++++++-
 gdb/cli/cli-cmds.h                  |  13 ++
 gdb/cli/cli-setshow.c               |  74 +++++-----
 gdb/cli/cli-setshow.h               |   5 +
 gdb/command.h                       |  19 +--
 gdb/maint-test-settings.c           | 167 +++++++++++------------
 gdb/maint.c                         |  27 ++++
 gdb/testsuite/gdb.base/settings.exp |  35 ++---
 gdb/testsuite/gdb.base/with.c       |  41 ++++++
 gdb/testsuite/gdb.base/with.exp     | 261 ++++++++++++++++++++++++++++++++++++
 gdb/top.c                           |   7 +-
 13 files changed, 738 insertions(+), 159 deletions(-)
 create mode 100644 gdb/testsuite/gdb.base/with.c
 create mode 100644 gdb/testsuite/gdb.base/with.exp

-- 
2.14.5

Comments

Philippe Waroquiers June 19, 2019, 12:34 a.m. | #1
On Tue, 2019-06-18 at 01:38 +0100, Pedro Alves wrote:
> ( See original discussion and prototype here:

>    https://sourceware.org/ml/gdb-patches/2019-05/msg00570.html )

> 

>  (gdb) help with

>  Temporarily set SETTING to VALUE, run COMMAND, and restore SETTING.

>  Usage: with SETTING [VALUE] [-- COMMAND]

>  Usage: w SETTING [VALUE] [-- COMMAND]

>  With no COMMAND, repeats the last executed command.

>  SETTING is any setting settable with the "set" command.

>  E.g.:

>    with language pascal -- print obj

>    with print elements unlimited -- print obj

> 

> More details in patch #4.

> 

> New in v2:

I played a little bit with this version, no bug encountered.

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 ...)

Philippe
Pedro Alves June 19, 2019, 1:05 p.m. | #2
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 <palves@redhat.com>

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"
 
-- 
2.14.5
Philippe Waroquiers June 19, 2019, 1:40 p.m. | #3
On Wed, 2019-06-19 at 14:05 +0100, Pedro Alves wrote:
> 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.

Yes, I think this is a good approach, and the error messages
are very clear.

Thanks

Philippe
Pedro Alves July 3, 2019, 12:49 p.m. | #4
On 6/18/19 1:38 AM, Pedro Alves wrote:
> 

>  (gdb) help with

>  Temporarily set SETTING to VALUE, run COMMAND, and restore SETTING.

>  Usage: with SETTING [VALUE] [-- COMMAND]

>  Usage: w SETTING [VALUE] [-- COMMAND]

>  With no COMMAND, repeats the last executed command.

>  SETTING is any setting settable with the "set" command.

>  E.g.:

>    with language pascal -- print obj

>    with print elements unlimited -- print obj

> 


I've merged this in now.

Thanks,
Pedro Alves
Sergio Durigan Junior Aug. 2, 2019, 11:24 p.m. | #5
On Wednesday, July 03 2019, Pedro Alves wrote:

> On 6/18/19 1:38 AM, Pedro Alves wrote:

>> 

>>  (gdb) help with

>>  Temporarily set SETTING to VALUE, run COMMAND, and restore SETTING.

>>  Usage: with SETTING [VALUE] [-- COMMAND]

>>  Usage: w SETTING [VALUE] [-- COMMAND]

>>  With no COMMAND, repeats the last executed command.

>>  SETTING is any setting settable with the "set" command.

>>  E.g.:

>>    with language pascal -- print obj

>>    with print elements unlimited -- print obj

>> 

>

> I've merged this in now.


Hi Pedro,

First of all, thanks for the nice feature.

BuildBot has unfortunately caught a new failure on gdb.base/with.exp
when run agains native-extended-gdbserver:

  https://sourceware.org/ml/gdb-testers/2019-q3/msg00213.html

  new FAIL: gdb.base/with.exp: repeat: reinvoke with no previous command to relaunch

I've also found it here when preparing a new Fedora GDB release.

Thanks,

-- 
Sergio
GPG key ID: 237A 54B1 0287 28BF 00EF  31F4 D0EB 7628 65FC 5E36
Please send encrypted e-mail if possible
http://sergiodj.net/