diff mbox

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

Message ID 1504388179-579-3-git-send-email-simon.marchi@ericsson.com
State New, archived
Headers

Commit Message

Simon Marchi Sept. 2, 2017, 9:36 p.m. UTC 
  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(-)

Comments

Pedro Alves Sept. 4, 2017, 12:35 p.m. UTC | #1 
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
Simon Marchi Sept. 4, 2017, 5:15 p.m. UTC | #2 
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
diff mbox

Patch

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