[PATCH 0/3] Tiny patches related to command lines

[PATCH 0/3] Tiny patches related to command lines

[Simon Marchi]

These patches address small things that bug me, which I found while reviewing
Tom's patch series about breakpoint commands.

Simon Marchi (3):
  Move command lines types/declarations to cli-script.h
  Error out immediatly when using if command without args in command
    list
  Add tests for loop_break and loop_continue commands

 gdb/breakpoint.h                    |  1 +
 gdb/cli/cli-script.c                |  3 +-
 gdb/cli/cli-script.h                | 76 ++++++++++++++++++++++++++++++++++++-
 gdb/defs.h                          | 74 ------------------------------------
 gdb/extension-priv.h                |  1 +
 gdb/gdbcmd.h                        |  1 +
 gdb/testsuite/gdb.base/commands.exp | 59 ++++++++++++++++++++++++++++
 7 files changed, 139 insertions(+), 76 deletions(-)

-- 
2.7.4

[PATCH 2/3] Error out immediatly when using if command without args in command list

[Simon Marchi]

When using "if" (or while) without args directly on gdb's command line,
you get this:

  (gdb) if
  if/while commands require arguments

When doing the same when entering a command list, you only get an error
when the command is executed, when parse_exp_in_context_1 fails to
evaluate the expression.

  (gdb) define foo
  Type commands for definition of "foo".
  End with a line saying just "end".
  >if
   >end
  >end
  (gdb) foo
  Argument required (expression to compute).

I think it would make more sense to error out when inputting the command
list directly:

  (gdb) define foo
  Type commands for definition of "foo".
  End with a line saying just "end".
  >if
  if/while commands require arguments.

The only required change is to check whether args is an empty string in
build_command_line.

gdb/ChangeLog:

	* cli/cli-script.c (build_command_line): For if/while commands,
	check whether args is empty.

gdb/testsuite/ChangeLog:

	* gdb.base/commands.exp: Call new procedure.
	(define_if_without_arg_test): New procedure.
---
 gdb/cli/cli-script.c                |  3 ++-
 gdb/testsuite/gdb.base/commands.exp | 16 ++++++++++++++++
 2 files changed, 18 insertions(+), 1 deletion(-)

diff --git a/gdb/cli/cli-script.c b/gdb/cli/cli-script.c
index 64b4c2b..594a071 100644
--- a/gdb/cli/cli-script.c
+++ b/gdb/cli/cli-script.c
@@ -147,7 +147,8 @@ build_command_line (enum command_control_type type, const char *args)
 {
   struct command_line *cmd;
 
-  if (args == NULL && (type == if_control || type == while_control))
+  if ((args == NULL || strlen (args) == 0)
+      && (type == if_control || type == while_control))
     error (_("if/while commands require arguments."));
   gdb_assert (args != NULL);
 
diff --git a/gdb/testsuite/gdb.base/commands.exp b/gdb/testsuite/gdb.base/commands.exp
index c934052..677361a 100644
--- a/gdb/testsuite/gdb.base/commands.exp
+++ b/gdb/testsuite/gdb.base/commands.exp
@@ -1011,6 +1011,21 @@ proc_with_prefix redefine_backtrace_test {} {
     gdb_test "bt" "hibob" "execute bt command"
 }
 
+# Test using "if" and "while" without args when building a command list.
+
+proc define_if_without_arg_test {} {
+    foreach cmd {if while} {
+	set test "define some_command_$cmd"
+	gdb_test_multiple $test $test {
+	    -re "End with"  {
+		pass $test
+	    }
+	}
+
+	gdb_test "$cmd" "if/while commands require arguments." "type $cmd without args"
+    }
+}
+
 # Test an input line split with a continuation character (backslash)
 # while entering a multi-line command (in a secondary prompt).
 
@@ -1076,5 +1091,6 @@ if_commands_test
 error_clears_commands_left
 redefine_hook_test
 backslash_in_multi_line_command_test
+define_if_without_arg_test
 # This one should come last, as it redefines "backtrace".
 redefine_backtrace_test
-- 
2.7.4

Re: [PATCH 2/3] Error out immediatly when using if command without args in command list

[Pedro Alves]

On 09/02/2017 10:36 PM, Simon Marchi wrote:
> When using "if" (or while) without args directly on gdb's command line,
> you get this:
> 
>   (gdb) if
>   if/while commands require arguments
> 
> When doing the same when entering a command list, you only get an error
> when the command is executed, when parse_exp_in_context_1 fails to
> evaluate the expression.
> 
>   (gdb) define foo
>   Type commands for definition of "foo".
>   End with a line saying just "end".
>   >if
>    >end
>   >end
>   (gdb) foo
>   Argument required (expression to compute).
> 
> I think it would make more sense to error out when inputting the command
> list directly:
> 
>   (gdb) define foo
>   Type commands for definition of "foo".
>   End with a line saying just "end".
>   >if
>   if/while commands require arguments.
> 
> The only required change is to check whether args is an empty string in
> build_command_line.
> 

LGTM.  Tiny nit further below.

BTW, as a potential improvement, we could consider also not
canceling the whole command definition, but instead go back to
expecting  another line.  It's a bit annoying to have to type
everything from scratch.  I've run into that occasionally with
tracepoints, like:

 (gdb) trace foo
 (gdb) actions
 Enter actions for tracepoint 1, one per line.
 End with a line saying just "end".
 >collect ...
 >collect ...
 > #... several lines later:
 >endd  # whoops, a typo.
 `endd' is not a tracepoint action, or is ambiguous.
 (gdb) # bah, have to start over.

Instead of:

 (gdb) trace foo
 (gdb) actions
 Enter actions for tracepoint 1, one per line.
 End with a line saying just "end".
 >collect ...
 >collect ...
 > #... several lines later:
 >endd
 `endd' is not a tracepoint action, or is ambiguous.
 >end
(gdb)

The same safety net applied to if/while typos might be useful.
Just an idea.

> --- a/gdb/cli/cli-script.c
> +++ b/gdb/cli/cli-script.c
> @@ -147,7 +147,8 @@ build_command_line (enum command_control_type type, const char *args)
>  {
>    struct command_line *cmd;
>  
> -  if (args == NULL && (type == if_control || type == while_control))
> +  if ((args == NULL || strlen (args) == 0)
> +      && (type == if_control || type == while_control))
>      error (_("if/while commands require arguments."));
>    gdb_assert (args != NULL);

Nit: might not make a difference with modern compilers, though
the canonical way to check for entry string would be:

  *args == '\0'

Thanks,
Pedro Alves

Re: [PATCH 2/3] Error out immediatly when using if command without args in command list

[Simon Marchi]

On 2017-09-04 14:35, Pedro Alves wrote:
> On 09/02/2017 10:36 PM, Simon Marchi wrote:
>> When using "if" (or while) without args directly on gdb's command 
>> line,
>> you get this:
>> 
>>   (gdb) if
>>   if/while commands require arguments
>> 
>> When doing the same when entering a command list, you only get an 
>> error
>> when the command is executed, when parse_exp_in_context_1 fails to
>> evaluate the expression.
>> 
>>   (gdb) define foo
>>   Type commands for definition of "foo".
>>   End with a line saying just "end".
>>   >if
>>    >end
>>   >end
>>   (gdb) foo
>>   Argument required (expression to compute).
>> 
>> I think it would make more sense to error out when inputting the 
>> command
>> list directly:
>> 
>>   (gdb) define foo
>>   Type commands for definition of "foo".
>>   End with a line saying just "end".
>>   >if
>>   if/while commands require arguments.
>> 
>> The only required change is to check whether args is an empty string 
>> in
>> build_command_line.
>> 
> 
> LGTM.  Tiny nit further below.
> 
> BTW, as a potential improvement, we could consider also not
> canceling the whole command definition, but instead go back to
> expecting  another line.  It's a bit annoying to have to type
> everything from scratch.  I've run into that occasionally with
> tracepoints, like:
> 
>  (gdb) trace foo
>  (gdb) actions
>  Enter actions for tracepoint 1, one per line.
>  End with a line saying just "end".
>  >collect ...
>  >collect ...
>  > #... several lines later:
>  >endd  # whoops, a typo.
>  `endd' is not a tracepoint action, or is ambiguous.
>  (gdb) # bah, have to start over.
> 
> Instead of:
> 
>  (gdb) trace foo
>  (gdb) actions
>  Enter actions for tracepoint 1, one per line.
>  End with a line saying just "end".
>  >collect ...
>  >collect ...
>  > #... several lines later:
>  >endd
>  `endd' is not a tracepoint action, or is ambiguous.
>  >end
> (gdb)
> 
> The same safety net applied to if/while typos might be useful.
> Just an idea.

I thought about the same thing.  We just need to make it clear that the 
erroneous command didn't make it in the command list.

>> --- a/gdb/cli/cli-script.c
>> +++ b/gdb/cli/cli-script.c
>> @@ -147,7 +147,8 @@ build_command_line (enum command_control_type 
>> type, const char *args)
>>  {
>>    struct command_line *cmd;
>> 
>> -  if (args == NULL && (type == if_control || type == while_control))
>> +  if ((args == NULL || strlen (args) == 0)
>> +      && (type == if_control || type == while_control))
>>      error (_("if/while commands require arguments."));
>>    gdb_assert (args != NULL);
> 
> Nit: might not make a difference with modern compilers, though
> the canonical way to check for entry string would be:
> 
>   *args == '\0'

Ok, I'm pushing now with that changed.

Thanks,
Simon

[PATCH 1/3] Move command lines types/declarations to cli-script.h

[Simon Marchi]

I think it would make more sense if the types and function declarations
related to command lines were in cli-script.h rather than defs.h, since
the related function definitions are in cli-script.c.

I had to add a few includes here and there.  I also had to rename the
"lines" parameter of command_lines_deleter::operator(), because ncurses
has a "#define lines ..." that was interfering when cli-script.h is
included by some TUI source files that also include ncurses header files.

gdb/ChangeLog:

	* cli/cli-script.h (enum misc_command_type): Move from defs.h.
	(enum command_control_type): Likewise.
	(struct command_line): Likewise.
	(free_command_lines): Likewise.
	(struct command_lines_deleter): Likewise.
	(command_line_up): Likewise.
	(read_command_lines): Likewise.
	(read_command_lines_1): Likewise.
	* defs.h (enum misc_command_type): Move to cli/cli-script.h.
	(enum command_control_type): Likewise.
	(struct command_line): Likewise.
	(free_command_lines): Likewise.
	(struct command_lines_deleter): Likewise.
	(command_line_up): Likewise.
	(read_command_lines): Likewise.
	(read_command_lines_1): Likewise.
	* breakpoint.h: Include cli/cli-script.h.
	* extension-priv.h: Likewise.
	* gdbcmd.h: Likewise.
---
 gdb/breakpoint.h     |  1 +
 gdb/cli/cli-script.h | 76 +++++++++++++++++++++++++++++++++++++++++++++++++++-
 gdb/defs.h           | 74 --------------------------------------------------
 gdb/extension-priv.h |  1 +
 gdb/gdbcmd.h         |  1 +
 5 files changed, 78 insertions(+), 75 deletions(-)

diff --git a/gdb/breakpoint.h b/gdb/breakpoint.h
index dc2b098..4bd407e 100644
--- a/gdb/breakpoint.h
+++ b/gdb/breakpoint.h
@@ -27,6 +27,7 @@
 #include "break-common.h"
 #include "probe.h"
 #include "location.h"
+#include "cli/cli-script.h"
 #include <vector>
 
 struct value;
diff --git a/gdb/cli/cli-script.h b/gdb/cli/cli-script.h
index 9514938..234faf7 100644
--- a/gdb/cli/cli-script.h
+++ b/gdb/cli/cli-script.h
@@ -18,9 +18,83 @@
 #define CLI_SCRIPT_H 1
 
 struct ui_file;
-struct command_line;
 struct cmd_list_element;
 
+/* * Control types for commands.  */
+
+enum misc_command_type
+{
+  ok_command,
+  end_command,
+  else_command,
+  nop_command
+};
+
+enum command_control_type
+{
+  simple_control,
+  break_control,
+  continue_control,
+  while_control,
+  if_control,
+  commands_control,
+  python_control,
+  compile_control,
+  guile_control,
+  while_stepping_control,
+  invalid_control
+};
+
+/* * Structure for saved commands lines (for breakpoints, defined
+   commands, etc).  */
+
+struct command_line
+{
+  struct command_line *next;
+  char *line;
+  enum command_control_type control_type;
+  union
+    {
+      struct
+	{
+	  enum compile_i_scope_types scope;
+	  void *scope_data;
+	}
+      compile;
+    }
+  control_u;
+  /* * The number of elements in body_list.  */
+  int body_count;
+  /* * For composite commands, the nested lists of commands.  For
+     example, for "if" command this will contain the then branch and
+     the else branch, if that is available.  */
+  struct command_line **body_list;
+};
+
+extern void free_command_lines (struct command_line **);
+
+/* A deleter for command_line that calls free_command_lines.  */
+
+struct command_lines_deleter
+{
+  void operator() (command_line *cmd_lines) const
+  {
+    free_command_lines (&cmd_lines);
+  }
+};
+
+/* A unique pointer to a command_line.  */
+
+typedef std::unique_ptr<command_line, command_lines_deleter> command_line_up;
+
+extern command_line_up read_command_lines (char *, int, int,
+					   void (*)(char *, void *),
+					   void *);
+extern command_line_up read_command_lines_1 (char * (*) (void), int,
+					     void (*)(char *, void *),
+					     void *);
+
+
 /* Exported to cli/cli-cmds.c */
 
 extern void script_from_file (FILE *stream, const char *file);
diff --git a/gdb/defs.h b/gdb/defs.h
index e180523..af9e32e 100644
--- a/gdb/defs.h
+++ b/gdb/defs.h
@@ -400,80 +400,6 @@ enum lval_type
     lval_computed
   };
 
-/* * Control types for commands.  */
-
-enum misc_command_type
-  {
-    ok_command,
-    end_command,
-    else_command,
-    nop_command
-  };
-
-enum command_control_type
-  {
-    simple_control,
-    break_control,
-    continue_control,
-    while_control,
-    if_control,
-    commands_control,
-    python_control,
-    compile_control,
-    guile_control,
-    while_stepping_control,
-    invalid_control
-  };
-
-/* * Structure for saved commands lines (for breakpoints, defined
-   commands, etc).  */
-
-struct command_line
-  {
-    struct command_line *next;
-    char *line;
-    enum command_control_type control_type;
-    union
-      {
-	struct
-	  {
-	    enum compile_i_scope_types scope;
-	    void *scope_data;
-	  }
-	compile;
-      }
-    control_u;
-    /* * The number of elements in body_list.  */
-    int body_count;
-    /* * For composite commands, the nested lists of commands.  For
-       example, for "if" command this will contain the then branch and
-       the else branch, if that is available.  */
-    struct command_line **body_list;
-  };
-
-extern void free_command_lines (struct command_line **);
-
-/* A deleter for command_line that calls free_command_lines.  */
-
-struct command_lines_deleter
-{
-  void operator() (command_line *lines) const
-  {
-    free_command_lines (&lines);
-  }
-};
-
-/* A unique pointer to a command_line.  */
-
-typedef std::unique_ptr<command_line, command_lines_deleter> command_line_up;
-
-extern command_line_up read_command_lines (char *, int, int,
-					   void (*)(char *, void *),
-					   void *);
-extern command_line_up read_command_lines_1 (char * (*) (void), int,
-					     void (*)(char *, void *),
-					     void *);
-
 /* * Parameters of the "info proc" command.  */
 
 enum info_proc_what
diff --git a/gdb/extension-priv.h b/gdb/extension-priv.h
index f77f088..4d16ac5 100644
--- a/gdb/extension-priv.h
+++ b/gdb/extension-priv.h
@@ -23,6 +23,7 @@
 
 #include "extension.h"
 #include <signal.h>
+#include "cli/cli-script.h"
 
 /* The return code for some API calls.  */
 
diff --git a/gdb/gdbcmd.h b/gdb/gdbcmd.h
index 14c7f4e..3191ecd 100644
--- a/gdb/gdbcmd.h
+++ b/gdb/gdbcmd.h
@@ -26,6 +26,7 @@
 
 #include "command.h"
 #include "ui-out.h"
+#include "cli/cli-script.h"
 
 /* Chain containing all defined commands.  */
 
-- 
2.7.4

Re: [PATCH 1/3] Move command lines types/declarations to cli-script.h

[Pedro Alves]

On 09/02/2017 10:36 PM, Simon Marchi wrote:
> I think it would make more sense if the types and function declarations
> related to command lines were in cli-script.h rather than defs.h, since
> the related function definitions are in cli-script.c.
> 
> I had to add a few includes here and there.  I also had to rename the
> "lines" parameter of command_lines_deleter::operator(), because ncurses
> has a "#define lines ..." that was interfering when cli-script.h is
> included by some TUI source files that also include ncurses header files.

LGTM.

Thanks,
Pedro Alves

[PATCH 3/3] Add tests for loop_break and loop_continue commands

[Simon Marchi]

I grepped the testsuite for loop_break and loop_continue and didn't find
anything, so I wrote some simple tests for those.

gdb/testsuite/ChangeLog:

	* gdb.base/commands.exp: Call the new procedures.
	(loop_break_test, loop_continue_test): New procedures.
---
 gdb/testsuite/gdb.base/commands.exp | 43 +++++++++++++++++++++++++++++++++++++
 1 file changed, 43 insertions(+)

diff --git a/gdb/testsuite/gdb.base/commands.exp b/gdb/testsuite/gdb.base/commands.exp
index 677361a..49a9242 100644
--- a/gdb/testsuite/gdb.base/commands.exp
+++ b/gdb/testsuite/gdb.base/commands.exp
@@ -1026,6 +1026,47 @@ proc define_if_without_arg_test {} {
     }
 }
 
+# Test the loop_break command.
+
+proc_with_prefix loop_break_test {} {
+    gdb_test_no_output "set \$a = 0" "initialize \$a"
+
+    gdb_test \
+    [multi_line_input \
+	 "while \$a < 5" \
+	 "  if \$a == 3" \
+	 "    loop_break" \
+	 "  end" \
+	 "  set \$a = \$a + 1" \
+	 "end"] \
+    "" \
+    "run while loop"
+
+    gdb_test "print \$a" " = 3" "validate \$a"
+}
+
+# Test the loop_continue command.
+
+proc_with_prefix loop_continue_test {} {
+    gdb_test_no_output "set \$a = 0" "initialize \$a"
+    gdb_test_no_output "set \$b = 0" "initialize \$b"
+
+    gdb_test \
+    [multi_line_input \
+	 "while \$a < 5" \
+	 "  set \$a = \$a + 1" \
+	 "  if \$a % 2 == 0" \
+	 "    loop_continue" \
+	 "  end" \
+	 "  set \$b = \$b + 1" \
+	 "end"] \
+    "" \
+    "run while loop"
+
+    gdb_test "print \$a" " = 5" "validate \$a"
+    gdb_test "print \$b" " = 3" "validate \$b"
+}
+
 # Test an input line split with a continuation character (backslash)
 # while entering a multi-line command (in a secondary prompt).
 
@@ -1092,5 +1133,7 @@ error_clears_commands_left
 redefine_hook_test
 backslash_in_multi_line_command_test
 define_if_without_arg_test
+loop_break_test
+loop_continue_test
 # This one should come last, as it redefines "backtrace".
 redefine_backtrace_test
-- 
2.7.4

Re: [PATCH 3/3] Add tests for loop_break and loop_continue commands

[Pedro Alves]

On 09/02/2017 10:36 PM, Simon Marchi wrote:
> I grepped the testsuite for loop_break and loop_continue and didn't find
> anything, so I wrote some simple tests for those.
> 
> gdb/testsuite/ChangeLog:
> 
> 	* gdb.base/commands.exp: Call the new procedures.
> 	(loop_break_test, loop_continue_test): New procedures.

LGTM, please push, and thanks for doing this.

I do wonder whether we handle loop_break/loop_continue in nested
loops correctly though, since the tests don't seem to
exercise that.

Thanks,
Pedro Alves

Re: [PATCH 3/3] Add tests for loop_break and loop_continue commands

[Simon Marchi]

On 2017-09-04 14:36, Pedro Alves wrote:
> On 09/02/2017 10:36 PM, Simon Marchi wrote:
>> I grepped the testsuite for loop_break and loop_continue and didn't 
>> find
>> anything, so I wrote some simple tests for those.
>> 
>> gdb/testsuite/ChangeLog:
>> 
>> 	* gdb.base/commands.exp: Call the new procedures.
>> 	(loop_break_test, loop_continue_test): New procedures.
> 
> LGTM, please push, and thanks for doing this.
> 
> I do wonder whether we handle loop_break/loop_continue in nested
> loops correctly though, since the tests don't seem to
> exercise that.
> 
> Thanks,
> Pedro Alves

Ah good point.  I am pushing this one, and I'll work on another patch to 
test this.

Thanks,

Simon