diff mbox

Fix PR gdb/17035: "show user" doesn't list user-defined commands that have empty bodies.

Message ID 87sikc1p8z.fsf@anubis.Home
State New
Headers show

Commit Message

Gabriel Krisman Bertazi Aug. 31, 2014, 6:35 p.m. UTC
Hello Pedro,

I appreciate your comments,

>> I don't see that as a problem, because python/scheme are also user
>> commands (or, at least, are part of class_user class).  Considering that
>> "help user-defined" also shows python/scheme commands, I believe it
>> would be more practical to just list them on "show user" (should we
>> update the doc string, as well).
>
> I think we'll need to track down the original discussion, and
> ask whoever was involved.  Otherwise, we'd just be going in
> circles.  :-)
>
> E.g., that commit changed the online help and the manual to
> try to make that clear.
>
> (gdb) help show user
> Show definitions of non-python/scheme user defined commands.
> Argument is the name of the user defined command.
> With no argument, show definitions of all user defined commands.
>

Reviewing the original discussion, (for the record,
<https://sourceware.org/ml/gdb-patches/2012-02/msg00246.html>), Doug has
made a pretty good point on not showing python/scheme commands here.

Since this is a simple bug fix, I'd rather not modify the behavior
here, so I rewrote the patch to not show python/scheme commands.  It got
a little simpler, since I don't have to touch a lot of testcases
anymore.

Now we use the fact that the c->function union is NULL when we have a
python/scheme command (they both use c->func directly).

I also added two testcases to make sure we are not printing scheme or
python commands, as you suggested.

>> Using the verification for cfunc/sfunc above, also avoids
>> printing the "user-defined" line.
>
> Hmm, but what is that "user-defined" command ?

user-defined is a null command.  It does nothing.  It is defined as
follows:

  add_cmd ("user-defined", class_user, NULL, _("\
User-defined commands.\n\
The commands in this class are those defined by the user.\n\
Use the \"define\" command to define a command."), &cmdlist);

I guess it's only purpose is at help user-defined.

Do you think it is acceptable now?

gdb/
2014-08-20  Gabriel Krisman Bertazi  <gabriel@krisman.be>

	* cli/cli-cmds.c (show_user): Verify if c->function is NULL to
	  decide whether c is a python/scheme command.
	* cli/cli-script.c (show_user_1): Only verify cmdlines after
	  printing command name.

gdb/testsuite/
2014-08-20  Gabriel Krisman Bertazi  <gabriel@krisman.be>

	* gdb.base/commands.exp: Include tests to verify user-defined
	  commands with empty bodies.
	* gdb.python/py-cmd.exp: Don't show user-defined python commands
	  in `show user command`
	* gdb.python/scm-cmd.exp: Don't show user-defined scheme commands
	  in `show user command`

Comments

Pedro Alves Sept. 2, 2014, 12:51 p.m. UTC | #1
On 08/31/2014 07:35 PM, Gabriel Krisman Bertazi wrote:
> Hello Pedro,
> 
> I appreciate your comments,
> 
>>> >> I don't see that as a problem, because python/scheme are also user
>>> >> commands (or, at least, are part of class_user class).  Considering that
>>> >> "help user-defined" also shows python/scheme commands, I believe it
>>> >> would be more practical to just list them on "show user" (should we
>>> >> update the doc string, as well).
>> >
>> > I think we'll need to track down the original discussion, and
>> > ask whoever was involved.  Otherwise, we'd just be going in
>> > circles.  :-)
>> >
>> > E.g., that commit changed the online help and the manual to
>> > try to make that clear.
>> >
>> > (gdb) help show user
>> > Show definitions of non-python/scheme user defined commands.
>> > Argument is the name of the user defined command.
>> > With no argument, show definitions of all user defined commands.
>> >
> Reviewing the original discussion, (for the record,
> <https://sourceware.org/ml/gdb-patches/2012-02/msg00246.html>), Doug has
> made a pretty good point on not showing python/scheme commands here.
> 
> Since this is a simple bug fix, I'd rather not modify the behavior
> here, so I rewrote the patch to not show python/scheme commands.  It got
> a little simpler, since I don't have to touch a lot of testcases
> anymore.
> 
> Now we use the fact that the c->function union is NULL when we have a
> python/scheme command (they both use c->func directly).
> 
> I also added two testcases to make sure we are not printing scheme or
> python commands, as you suggested.

Thanks!

> 
>>> >> Using the verification for cfunc/sfunc above, also avoids
>>> >> printing the "user-defined" line.
>> >
>> > Hmm, but what is that "user-defined" command ?
> user-defined is a null command.  It does nothing.  It is defined as
> follows:
> 
>   add_cmd ("user-defined", class_user, NULL, _("\
> User-defined commands.\n\
> The commands in this class are those defined by the user.\n\
> Use the \"define\" command to define a command."), &cmdlist);
> 
> I guess it's only purpose is at help user-defined.

Yeah.

> 
> Do you think it is acceptable now?

Yeah.  Close now.  Only nits on the details.

> 
> gdb/
> 2014-08-20  Gabriel Krisman Bertazi  <gabriel@krisman.be>
> 
> 	* cli/cli-cmds.c (show_user): Verify if c->function is NULL to
> 	  decide whether c is a python/scheme command.
> 	* cli/cli-script.c (show_user_1): Only verify cmdlines after
> 	  printing command name.
> 
> gdb/testsuite/
> 2014-08-20  Gabriel Krisman Bertazi  <gabriel@krisman.be>
> 
> 	* gdb.base/commands.exp: Include tests to verify user-defined
> 	  commands with empty bodies.

> 	* gdb.python/py-cmd.exp: Don't show user-defined python commands
> 	  in `show user command`

Prefix that with "Test that we" or some such.  Add missing period
at the end.


> 	* gdb.python/scm-cmd.exp: Don't show user-defined scheme commands
> 	  in `show user command`

Likewise.

> 
> 
> show_user_v2.patch
> 
> 
> diff --git a/gdb/cli/cli-cmds.c b/gdb/cli/cli-cmds.c
> index b415267..4f624f2 100644
> --- a/gdb/cli/cli-cmds.c
> +++ b/gdb/cli/cli-cmds.c
> @@ -1245,8 +1245,9 @@ show_user (char *args, int from_tty)
>        const char *comname = args;
>  
>        c = lookup_cmd (&comname, cmdlist, "", 0, 1);
> -      /* c->user_commands would be NULL if it's a python/scheme command.  */
> -      if (c->class != class_user || !c->user_commands)
> +      /* c->function.cfunc would be NULL if it's a python/scheme
> +	 command.  */
> +      if (c->class != class_user || c->function.cfunc == NULL)
>  	error (_("Not a user command."));
>        show_user_1 (c, "", args, gdb_stdout);
>      }
> @@ -1254,7 +1255,8 @@ show_user (char *args, int from_tty)
>      {
>        for (c = cmdlist; c; c = c->next)
>  	{
> -	  if (c->class == class_user || c->prefixlist != NULL)
> +	  if ((c->class == class_user && c->function.cfunc != NULL)


> -      /* c->user_commands would be NULL if it's a python/scheme command.  */
> -      if (c->class != class_user || !c->user_commands)

> +	  if ((c->class == class_user && c->function.cfunc != NULL)
> +	      || c->prefixlist != NULL)

I think it'd be great if we had a small helper function for this
predicate.  So that we could write something like:

	  if (cli_user_command_p (c))

and:

	  if (cli_user_command_p (c) || c->prefixlist != NULL)





Also, best not assume the compiler allows accessing c->function.cfunc
when it's c->function.sfunc that is actually active in the union, as
in:

> +	  if ((c->class == class_user && c->function.cfunc != NULL)



int
cli_user_command_p (struct cmd_list_element *cmd)
{
  return (cmd->func == do_cfunc && cmd->function.cfunc != NULL)
          || cmd->func == do_sfunc && cmd->function.sfunc != NULL);
}

or just (don't know if it works):

int
cli_user_command_p (struct cmd_list_element *cmd)
{
  return (cmd->func == do_cfunc || cmd->func == do_sfunc);
}


> +	      || c->prefixlist != NULL)
>  	    show_user_1 (c, "", c->name, gdb_stdout);
>  	}
>      }


> diff --git a/gdb/cli/cli-script.c b/gdb/cli/cli-script.c
> index 0f0a97e..37cb82a 100644
> --- a/gdb/cli/cli-script.c
> +++ b/gdb/cli/cli-script.c
> @@ -1717,10 +1717,10 @@ show_user_1 (struct cmd_list_element *c, const char *prefix, const char *name,
>      }
>  
>    cmdlines = c->user_commands;
> -  if (!cmdlines)
> -    return;
>    fprintf_filtered (stream, "User command \"%s%s\":\n", prefix, name);
>  
> +  if (!cmdlines)
> +    return;
>    print_command_lines (current_uiout, cmdlines, 1);
>    fputs_filtered ("\n", stream);
>  }
> diff --git a/gdb/testsuite/gdb.base/commands.exp b/gdb/testsuite/gdb.base/commands.exp
> index 7363420..b2d1c76 100644
> --- a/gdb/testsuite/gdb.base/commands.exp
> +++ b/gdb/testsuite/gdb.base/commands.exp
> @@ -243,6 +243,28 @@ proc user_defined_command_test {} {
>     gdb_test "show user mycommand" \
>  	"  while \\\$arg0.*set.*    if \\\(\\\$arg0.*p/x.*    else\[^\n\].*p/x.*    end\[^\n\].*  end\[^\n\].*" \
>  	   "display user command in user_defined_command_test"
> +
> +    #  Create and test an user-defined command with an empty body.

s/an user/a user/

> +    gdb_test_multiple "define myemptycommand" \
> +	"define myemptycommand in user_defined_command_test" {
> +	    -re "End with"  {
> +		pass "define myemptycommand in user_defined_command_test"
> +	    }
> +	}
> +    gdb_test "end" \
> +	"" \
> +	"End definition of user-defined command with empty body."

lowercase "end".  No period at the end.

> +
> +    gdb_test_no_output "myemptycommand" \
> +	"execute user-defined empty command in user_defined_command_test"
> +
> +    gdb_test "show user" \
> +	"User command \"myemptycommand.*" \
> +	"display empty command in command list in user_defined_command_test"
> +
> +    gdb_test "show user myemptycommand" \
> +	"User command \"myemptycommand.*" \
> +	"display user-defined emtpy command in user_defined_command_test"

Typo: "empty"

>  proc watchpoint_command_test {} {
> diff --git a/gdb/testsuite/gdb.guile/scm-cmd.exp b/gdb/testsuite/gdb.guile/scm-cmd.exp
> index a407f63..36ef03d 100644
> --- a/gdb/testsuite/gdb.guile/scm-cmd.exp
> +++ b/gdb/testsuite/gdb.guile/scm-cmd.exp
> @@ -134,6 +134,10 @@ gdb_test "help user-defined" \
>      "User-defined commands.\[\r\n\]+The commands in this class are those defined by the user.\[\r\n\]+Use the \"define\" command to define a command.\[\r\n\]+List of commands:\[\r\n\]+test-help -- Docstring\[\r\n\]+Type \"help\" followed by command name for full documentation.\[\r\n\]+Type \"apropos word\" to search for commands related to \"word\".\[\r\n\]+Command name abbreviations are allowed if unambiguous.\[\r\n\]+" \
>      "see user-defined command in `help user-defined`"
>  
> +# Make sure the command does not show up in `show user`.
> +gdb_test "show user test-help" "Not a user command\." \
> +    "Don't show user-defined scheme command in `show user command`"
> +

lowercase "don't".

>  # Test expression completion on fields.
>  
>  gdb_test_multiline "expression completion command" \
> diff --git a/gdb/testsuite/gdb.python/py-cmd.exp b/gdb/testsuite/gdb.python/py-cmd.exp
> index a87aecb..3ae4053 100644
> --- a/gdb/testsuite/gdb.python/py-cmd.exp
> +++ b/gdb/testsuite/gdb.python/py-cmd.exp
> @@ -161,6 +161,10 @@ gdb_test "test_help ugh" "test_cmd output, arg = ugh" "call simple user-defined
>  # Make sure the command shows up in `help user-defined`.
>  gdb_test "help user-defined" "User-defined commands.\[\r\n\]+The commands in this class are those defined by the user.\[\r\n\]+Use the \"define\" command to define a command.\[\r\n\]+\[\r\n\]+List of commands:\[\r\n\]+\[\r\n\]+test_help -- Docstring\[\r\n\]+\[\r\n\]+Type \"help\" followed by command name for full documentation.\[\r\n\]+Type \"apropos word\" to search for commands related to \"word\".\[\r\n\]+Command name abbreviations are allowed if unambiguous.\[\r\n\]+" "see user-defined command in `help user-defined`"
>  
> +# Make sure the command does not show up in `show user`.
> +gdb_test "show user test_help" "Not a user command\." \
> +    "Don't show user-defined python command in `show user command`"

lowercase "don't".

Thanks,
Pedro Alves
diff mbox

Patch

diff --git a/gdb/cli/cli-cmds.c b/gdb/cli/cli-cmds.c
index b415267..4f624f2 100644
--- a/gdb/cli/cli-cmds.c
+++ b/gdb/cli/cli-cmds.c
@@ -1245,8 +1245,9 @@  show_user (char *args, int from_tty)
       const char *comname = args;
 
       c = lookup_cmd (&comname, cmdlist, "", 0, 1);
-      /* c->user_commands would be NULL if it's a python/scheme command.  */
-      if (c->class != class_user || !c->user_commands)
+      /* c->function.cfunc would be NULL if it's a python/scheme
+	 command.  */
+      if (c->class != class_user || c->function.cfunc == NULL)
 	error (_("Not a user command."));
       show_user_1 (c, "", args, gdb_stdout);
     }
@@ -1254,7 +1255,8 @@  show_user (char *args, int from_tty)
     {
       for (c = cmdlist; c; c = c->next)
 	{
-	  if (c->class == class_user || c->prefixlist != NULL)
+	  if ((c->class == class_user && c->function.cfunc != NULL)
+	      || c->prefixlist != NULL)
 	    show_user_1 (c, "", c->name, gdb_stdout);
 	}
     }
diff --git a/gdb/cli/cli-script.c b/gdb/cli/cli-script.c
index 0f0a97e..37cb82a 100644
--- a/gdb/cli/cli-script.c
+++ b/gdb/cli/cli-script.c
@@ -1717,10 +1717,10 @@  show_user_1 (struct cmd_list_element *c, const char *prefix, const char *name,
     }
 
   cmdlines = c->user_commands;
-  if (!cmdlines)
-    return;
   fprintf_filtered (stream, "User command \"%s%s\":\n", prefix, name);
 
+  if (!cmdlines)
+    return;
   print_command_lines (current_uiout, cmdlines, 1);
   fputs_filtered ("\n", stream);
 }
diff --git a/gdb/testsuite/gdb.base/commands.exp b/gdb/testsuite/gdb.base/commands.exp
index 7363420..b2d1c76 100644
--- a/gdb/testsuite/gdb.base/commands.exp
+++ b/gdb/testsuite/gdb.base/commands.exp
@@ -243,6 +243,28 @@  proc user_defined_command_test {} {
    gdb_test "show user mycommand" \
 	"  while \\\$arg0.*set.*    if \\\(\\\$arg0.*p/x.*    else\[^\n\].*p/x.*    end\[^\n\].*  end\[^\n\].*" \
 	   "display user command in user_defined_command_test"
+
+    #  Create and test an user-defined command with an empty body.
+    gdb_test_multiple "define myemptycommand" \
+	"define myemptycommand in user_defined_command_test" {
+	    -re "End with"  {
+		pass "define myemptycommand in user_defined_command_test"
+	    }
+	}
+    gdb_test "end" \
+	"" \
+	"End definition of user-defined command with empty body."
+
+    gdb_test_no_output "myemptycommand" \
+	"execute user-defined empty command in user_defined_command_test"
+
+    gdb_test "show user" \
+	"User command \"myemptycommand.*" \
+	"display empty command in command list in user_defined_command_test"
+
+    gdb_test "show user myemptycommand" \
+	"User command \"myemptycommand.*" \
+	"display user-defined emtpy command in user_defined_command_test"
 }
 
 proc watchpoint_command_test {} {
diff --git a/gdb/testsuite/gdb.guile/scm-cmd.exp b/gdb/testsuite/gdb.guile/scm-cmd.exp
index a407f63..36ef03d 100644
--- a/gdb/testsuite/gdb.guile/scm-cmd.exp
+++ b/gdb/testsuite/gdb.guile/scm-cmd.exp
@@ -134,6 +134,10 @@  gdb_test "help user-defined" \
     "User-defined commands.\[\r\n\]+The commands in this class are those defined by the user.\[\r\n\]+Use the \"define\" command to define a command.\[\r\n\]+List of commands:\[\r\n\]+test-help -- Docstring\[\r\n\]+Type \"help\" followed by command name for full documentation.\[\r\n\]+Type \"apropos word\" to search for commands related to \"word\".\[\r\n\]+Command name abbreviations are allowed if unambiguous.\[\r\n\]+" \
     "see user-defined command in `help user-defined`"
 
+# Make sure the command does not show up in `show user`.
+gdb_test "show user test-help" "Not a user command\." \
+    "Don't show user-defined scheme command in `show user command`"
+
 # Test expression completion on fields.
 
 gdb_test_multiline "expression completion command" \
diff --git a/gdb/testsuite/gdb.python/py-cmd.exp b/gdb/testsuite/gdb.python/py-cmd.exp
index a87aecb..3ae4053 100644
--- a/gdb/testsuite/gdb.python/py-cmd.exp
+++ b/gdb/testsuite/gdb.python/py-cmd.exp
@@ -161,6 +161,10 @@  gdb_test "test_help ugh" "test_cmd output, arg = ugh" "call simple user-defined
 # Make sure the command shows up in `help user-defined`.
 gdb_test "help user-defined" "User-defined commands.\[\r\n\]+The commands in this class are those defined by the user.\[\r\n\]+Use the \"define\" command to define a command.\[\r\n\]+\[\r\n\]+List of commands:\[\r\n\]+\[\r\n\]+test_help -- Docstring\[\r\n\]+\[\r\n\]+Type \"help\" followed by command name for full documentation.\[\r\n\]+Type \"apropos word\" to search for commands related to \"word\".\[\r\n\]+Command name abbreviations are allowed if unambiguous.\[\r\n\]+" "see user-defined command in `help user-defined`"
 
+# Make sure the command does not show up in `show user`.
+gdb_test "show user test_help" "Not a user command\." \
+    "Don't show user-defined python command in `show user command`"
+
 # Test expression completion on fields
 gdb_py_test_multiple "expression completion command" \
   "python" "" \