[master,+,11] gdb/mi: handle no condition argument case for -break-condition

Message ID 99f6d855251dbae1ae00a533e9afb5eeec298118.1626077925.git.tankut.baris.aktemur@intel.com
State New
Headers show
Series
  • [master,+,11] gdb/mi: handle no condition argument case for -break-condition
Related show

Commit Message

Simon Marchi via Gdb-patches July 12, 2021, 8:20 a.m.
As reported in PR gdb/28076 [1], passing no condition argument to the
-break-condition command (e.g.: "-break-condition 2") should clear the
condition for breakpoint 2, just like CLI's "condition 2", but instead
an error message is returned:

  ^error,msg="-break-condition: Missing the <number> and/or <expr> argument"

The current implementation of the -break-condition command's argument
handling (79aabb7308c "gdb/mi: add a '--force' flag to the
'-break-condition' command") was done according to the documentation,
where the condition argument seemed mandatory.  However, the
-break-condition command originally (i.e. before the 79aabb7308c
patch) used the CLI's "cond" command, and back then not passing a
condition argument was clearing out the condition.  So, this is a
regression in terms of the behavior.

Fix the argument handling of the -break-condition command to allow not
having a condition argument, and also update the document to make the
behavior clear.  Also add test cases to test the scenarios which were
previously not covered.

[1] https://sourceware.org/bugzilla/show_bug.cgi?id=28076

gdb/ChangeLog:
2021-07-12  Tankut Baris Aktemur  <tankut.baris.aktemur@intel.com>

	PR gdb/28076
	* mi/mi-cmd-break.c (mi_cmd_break_condition): Handle the case
	of having no condition argument.

gdb/doc/ChangeLog:
2021-07-12  Tankut Baris Aktemur  <tankut.baris.aktemur@intel.com>

	PR gdb/28076
	* gdb.texinfo (GDB/MI Breakpoint Commands): Mention clearing
	the condition in the -break-condition command.

gdb/testsuite/ChangeLog:
2021-07-12  Tankut Baris Aktemur  <tankut.baris.aktemur@intel.com>

	PR gdb/28076
	* gdb.mi/mi-break.exp: Add more tests to check clearing the
	breakpoint condition.
---
 gdb/doc/gdb.texinfo               |  5 +++--
 gdb/mi/mi-cmd-break.c             | 15 +++++++--------
 gdb/testsuite/gdb.mi/mi-break.exp | 22 ++++++++++++++++++++++
 3 files changed, 32 insertions(+), 10 deletions(-)

-- 
2.17.1

Comments

Simon Marchi via Gdb-patches July 12, 2021, 8:39 a.m. | #1
On Monday, July 12, 2021 10:20 AM, Aktemur, Tankut Baris wrote:
> As reported in PR gdb/28076 [1], passing no condition argument to the

> -break-condition command (e.g.: "-break-condition 2") should clear the

> condition for breakpoint 2, just like CLI's "condition 2", but instead

> an error message is returned:

> 

>   ^error,msg="-break-condition: Missing the <number> and/or <expr> argument"

> 

> The current implementation of the -break-condition command's argument

> handling (79aabb7308c "gdb/mi: add a '--force' flag to the

> '-break-condition' command") was done according to the documentation,

> where the condition argument seemed mandatory.  However, the

> -break-condition command originally (i.e. before the 79aabb7308c

> patch) used the CLI's "cond" command, and back then not passing a

> condition argument was clearing out the condition.  So, this is a

> regression in terms of the behavior.

> 

> Fix the argument handling of the -break-condition command to allow not

> having a condition argument, and also update the document to make the

> behavior clear.  Also add test cases to test the scenarios which were

> previously not covered.

> 

> [1] https://sourceware.org/bugzilla/show_bug.cgi?id=28076

> 

> gdb/ChangeLog:

> 2021-07-12  Tankut Baris Aktemur  <tankut.baris.aktemur@intel.com>

> 

> 	PR gdb/28076

> 	* mi/mi-cmd-break.c (mi_cmd_break_condition): Handle the case

> 	of having no condition argument.

> 

> gdb/doc/ChangeLog:

> 2021-07-12  Tankut Baris Aktemur  <tankut.baris.aktemur@intel.com>

> 

> 	PR gdb/28076

> 	* gdb.texinfo (GDB/MI Breakpoint Commands): Mention clearing

> 	the condition in the -break-condition command.

> 

> gdb/testsuite/ChangeLog:

> 2021-07-12  Tankut Baris Aktemur  <tankut.baris.aktemur@intel.com>

> 

> 	PR gdb/28076

> 	* gdb.mi/mi-break.exp: Add more tests to check clearing the

> 	breakpoint condition.

> ---

>  gdb/doc/gdb.texinfo               |  5 +++--

>  gdb/mi/mi-cmd-break.c             | 15 +++++++--------

>  gdb/testsuite/gdb.mi/mi-break.exp | 22 ++++++++++++++++++++++

>  3 files changed, 32 insertions(+), 10 deletions(-)

> 

> diff --git a/gdb/doc/gdb.texinfo b/gdb/doc/gdb.texinfo

> index a16a3825ebb..a486bbe3607 100644

> --- a/gdb/doc/gdb.texinfo

> +++ b/gdb/doc/gdb.texinfo

> @@ -30565,7 +30565,7 @@ times="0"@}

>  @subsubheading Synopsis

> 

>  @smallexample

> - -break-condition [ --force ] @var{number} @var{expr}

> + -break-condition [ --force ] @var{number} [ @var{expr} ]

>  @end smallexample

> 

>  Breakpoint @var{number} will stop the program only if the condition in

> @@ -30573,7 +30573,8 @@ Breakpoint @var{number} will stop the program only if the condition

> in

>  @samp{-break-list} output (see the description of the @samp{-break-list}

>  command below).  If the @samp{--force} flag is passed, the condition

>  is forcibly defined even when it is invalid for all locations of

> -breakpoint @var{number}.

> +breakpoint @var{number}.  If the @var{expr} argument is omitted,

> +breakpoint @var{number} becomes unconditional.

> 

>  @subsubheading @value{GDBN} Command

> 

> diff --git a/gdb/mi/mi-cmd-break.c b/gdb/mi/mi-cmd-break.c

> index 5439937f66b..c2d642d75b4 100644

> --- a/gdb/mi/mi-cmd-break.c

> +++ b/gdb/mi/mi-cmd-break.c

> @@ -423,20 +423,19 @@ mi_cmd_break_condition (const char *command, char **argv, int argc)

>  	}

>      }

> 

> -  /* There must be at least two more args: a bpnum and a condition

> -     expression.  */

> -  if (oind + 1 >= argc)

> -    error (_("-break-condition: Missing the <number> and/or <expr> "

> -	     "argument"));

> +  /* There must be at least one more arg: a bpnum.  */

> +  if (oind >= argc)

> +    error (_("-break-condition: Missing the <number> argument"));

> 

>    int bpnum = atoi (argv[oind]);

> 

>    /* The rest form the condition expr.  */

> -  std::string expr (argv[oind + 1]);

> -  for (int i = oind + 2; i < argc; ++i)

> +  std::string expr = "";

> +  for (int i = oind + 1; i < argc; ++i)

>      {

> -      expr += " ";

>        expr += argv[i];

> +      if (i + 1 < argc)

> +	expr += " ";

>      }

> 

>    set_breakpoint_condition (bpnum, expr.c_str (), 0 /* from_tty */,

> diff --git a/gdb/testsuite/gdb.mi/mi-break.exp b/gdb/testsuite/gdb.mi/mi-break.exp

> index b6ef3483004..f8ff9beb7d2 100644

> --- a/gdb/testsuite/gdb.mi/mi-break.exp

> +++ b/gdb/testsuite/gdb.mi/mi-break.exp

> @@ -435,6 +435,28 @@ proc_with_prefix test_forced_conditions {} {

>      mi_gdb_test "-break-info 16" \

>  	"\\^done,[mi_make_breakpoint_table [list $bp]]" \

>          "invalid condition is defined"

> +

> +    # No cond argument should clear the condition.

> +    mi_gdb_test "-break-condition 16" \

> +        "~\"Breakpoint 16's condition is now valid at location 1, enabling.*\\^done" \

> +        "clear the condition"

> +    set bp [mi_make_breakpoint -number 16]

> +    mi_gdb_test "-break-info 16" \

> +	"\\^done,[mi_make_breakpoint_table [list $bp]]" \

> +        "condition is cleared"

> +

> +    # Zero-argument is an error.

> +    mi_gdb_test "-break-condition" \

> +	"\\^error,msg=\"-break-condition: Missing the <number> argument\"" \

> +        "no arguments to -break-condition"

> +

> +    # Passing --force with no condition should not crash or raise an error.

> +    mi_gdb_test "-break-condition --force 16" \

> +	"\\^done" \

> +        "clear the condition with --force"

> +    mi_gdb_test "-break-condition --force" \

> +	"\\^error,msg=\"-break-condition: Missing the <number> argument\"" \

> +        "no arguments with --force"


There are space vs. tab problems above.  I've fixed them on my local branch.

-Baris


Intel Deutschland GmbH
Registered Address: Am Campeon 10, 85579 Neubiberg, Germany
Tel: +49 89 99 8853-0, www.intel.de <http://www.intel.de>
Managing Directors: Christin Eisenschmid, Sharon Heck, Tiffany Doon Silva  
Chairperson of the Supervisory Board: Nicole Lau
Registered Office: Munich
Commercial Register: Amtsgericht Muenchen HRB 186928
Jonah Graham July 12, 2021, 1:36 p.m. | #2
On Mon, 12 Jul 2021 at 04:20, Tankut Baris Aktemur <
tankut.baris.aktemur@intel.com> wrote:

> As reported in PR gdb/28076 [1], passing no condition argument to the

>


Thanks Baris. I have applied the patch and it works well. Thanks for fixing
the docs too!


> +    error (_("-break-condition: Missing the <number> argument"));

>


The error message has changed compared to previous GDB releases, not sure
if that was intentional or if it matters, the old versions said:

^error,msg="Argument required (breakpoint number)."

the new message renders as:

^error,msg="-break-condition: Missing the <number> argument"

Eclipse CDT does not use the contents of error message (because we never
send a command with such an error :-) so from my perspective it is fine to
change the error message.

Jonah




>

>
Simon Marchi via Gdb-patches July 12, 2021, 2:01 p.m. | #3
From: Jonah Graham <jonah@kichwacoders.com>

Sent: Monday, July 12, 2021 3:37 PM
To: Aktemur, Tankut Baris <tankut.baris.aktemur@intel.com>
Cc: gdb-patches@sourceware.org; Simon Marchi <simon.marchi@polymtl.ca>
Subject: Re: [PATCH master + 11] gdb/mi: handle no condition argument case for -break-condition

The error message has changed compared to previous GDB releases, not sure if that was intentional or if it matters, the old versions said:

^error,msg="Argument required (breakpoint number)."

the new message renders as:

^error,msg="-break-condition: Missing the <number> argument"

Eclipse CDT does not use the contents of error message (because we never send a command with such an error :-) so from my perspective it is fine to change the error message.

Jonah

The former error message was coming from the CLI’s “cond” command:

  (gdb) cond
  Argument required (breakpoint number).

In my opinion this message is equivalent to the new message in terms of descriptiveness,
except for the “-break-condition” prefix.  Let’s see what Simon or another maintainer
thinks about preserving the exact same error message.

Thanks
-Baris


Intel Deutschland GmbH
Registered Address: Am Campeon 10, 85579 Neubiberg, Germany
Tel: +49 89 99 8853-0, www.intel.de <http://www.intel.de>
Managing Directors: Christin Eisenschmid, Sharon Heck, Tiffany Doon Silva  
Chairperson of the Supervisory Board: Nicole Lau
Registered Office: Munich
Commercial Register: Amtsgericht Muenchen HRB 186928
Simon Marchi via Gdb-patches July 12, 2021, 8:53 p.m. | #4
> The error message has changed compared to previous GDB releases, not sure if that was intentional or if it matters, the old versions said:

> 

>  

> 

> ^error,msg="Argument required (breakpoint number)."

> 

>  

> 

> the new message renders as:

> 

>  

> 

> ^error,msg="-break-condition: Missing the <number> argument"

> 

>  

> 

> Eclipse CDT does not use the contents of error message (because we never send a command with such an error :-) so from my perspective it is fine to change the error message.

> 

>  

> 

> Jonah

> 

>      

> 

> The former error message was coming from the CLI’s “cond” command:

> 

>  

> 

>   (gdb) cond

> 

>   Argument required (breakpoint number).

> 

>  

> 

> In my opinion this message is equivalent to the new message in terms of descriptiveness,

> 

> except for the “-break-condition” prefix.  Let’s see what Simon or another maintainer

> 

> thinks about preserving the exact same error message.


I really don't mind.  The code part of the patch LGTM.  Eli, any
comments on the doc?

Simon
Joel Brobecker July 24, 2021, 9:15 p.m. | #5
Hi Eli,

The code change for this patch has been approved by Simon.
I was hoping you could take a look at the documentation changes
for Tankut?

thank you!

On Mon, Jul 12, 2021 at 10:20:01AM +0200, Tankut Baris Aktemur via Gdb-patches wrote:
> As reported in PR gdb/28076 [1], passing no condition argument to the

> -break-condition command (e.g.: "-break-condition 2") should clear the

> condition for breakpoint 2, just like CLI's "condition 2", but instead

> an error message is returned:

> 

>   ^error,msg="-break-condition: Missing the <number> and/or <expr> argument"

> 

> The current implementation of the -break-condition command's argument

> handling (79aabb7308c "gdb/mi: add a '--force' flag to the

> '-break-condition' command") was done according to the documentation,

> where the condition argument seemed mandatory.  However, the

> -break-condition command originally (i.e. before the 79aabb7308c

> patch) used the CLI's "cond" command, and back then not passing a

> condition argument was clearing out the condition.  So, this is a

> regression in terms of the behavior.

> 

> Fix the argument handling of the -break-condition command to allow not

> having a condition argument, and also update the document to make the

> behavior clear.  Also add test cases to test the scenarios which were

> previously not covered.

> 

> [1] https://sourceware.org/bugzilla/show_bug.cgi?id=28076

> 

> gdb/ChangeLog:

> 2021-07-12  Tankut Baris Aktemur  <tankut.baris.aktemur@intel.com>

> 

> 	PR gdb/28076

> 	* mi/mi-cmd-break.c (mi_cmd_break_condition): Handle the case

> 	of having no condition argument.

> 

> gdb/doc/ChangeLog:

> 2021-07-12  Tankut Baris Aktemur  <tankut.baris.aktemur@intel.com>

> 

> 	PR gdb/28076

> 	* gdb.texinfo (GDB/MI Breakpoint Commands): Mention clearing

> 	the condition in the -break-condition command.

> 

> gdb/testsuite/ChangeLog:

> 2021-07-12  Tankut Baris Aktemur  <tankut.baris.aktemur@intel.com>

> 

> 	PR gdb/28076

> 	* gdb.mi/mi-break.exp: Add more tests to check clearing the

> 	breakpoint condition.

> ---

>  gdb/doc/gdb.texinfo               |  5 +++--

>  gdb/mi/mi-cmd-break.c             | 15 +++++++--------

>  gdb/testsuite/gdb.mi/mi-break.exp | 22 ++++++++++++++++++++++

>  3 files changed, 32 insertions(+), 10 deletions(-)

> 

> diff --git a/gdb/doc/gdb.texinfo b/gdb/doc/gdb.texinfo

> index a16a3825ebb..a486bbe3607 100644

> --- a/gdb/doc/gdb.texinfo

> +++ b/gdb/doc/gdb.texinfo

> @@ -30565,7 +30565,7 @@ times="0"@}

>  @subsubheading Synopsis

>  

>  @smallexample

> - -break-condition [ --force ] @var{number} @var{expr}

> + -break-condition [ --force ] @var{number} [ @var{expr} ]

>  @end smallexample

>  

>  Breakpoint @var{number} will stop the program only if the condition in

> @@ -30573,7 +30573,8 @@ Breakpoint @var{number} will stop the program only if the condition in

>  @samp{-break-list} output (see the description of the @samp{-break-list}

>  command below).  If the @samp{--force} flag is passed, the condition

>  is forcibly defined even when it is invalid for all locations of

> -breakpoint @var{number}.

> +breakpoint @var{number}.  If the @var{expr} argument is omitted,

> +breakpoint @var{number} becomes unconditional.

>  

>  @subsubheading @value{GDBN} Command

>  

> diff --git a/gdb/mi/mi-cmd-break.c b/gdb/mi/mi-cmd-break.c

> index 5439937f66b..c2d642d75b4 100644

> --- a/gdb/mi/mi-cmd-break.c

> +++ b/gdb/mi/mi-cmd-break.c

> @@ -423,20 +423,19 @@ mi_cmd_break_condition (const char *command, char **argv, int argc)

>  	}

>      }

>  

> -  /* There must be at least two more args: a bpnum and a condition

> -     expression.  */

> -  if (oind + 1 >= argc)

> -    error (_("-break-condition: Missing the <number> and/or <expr> "

> -	     "argument"));

> +  /* There must be at least one more arg: a bpnum.  */

> +  if (oind >= argc)

> +    error (_("-break-condition: Missing the <number> argument"));

>  

>    int bpnum = atoi (argv[oind]);

>  

>    /* The rest form the condition expr.  */

> -  std::string expr (argv[oind + 1]);

> -  for (int i = oind + 2; i < argc; ++i)

> +  std::string expr = "";

> +  for (int i = oind + 1; i < argc; ++i)

>      {

> -      expr += " ";

>        expr += argv[i];

> +      if (i + 1 < argc)

> +	expr += " ";

>      }

>  

>    set_breakpoint_condition (bpnum, expr.c_str (), 0 /* from_tty */,

> diff --git a/gdb/testsuite/gdb.mi/mi-break.exp b/gdb/testsuite/gdb.mi/mi-break.exp

> index b6ef3483004..f8ff9beb7d2 100644

> --- a/gdb/testsuite/gdb.mi/mi-break.exp

> +++ b/gdb/testsuite/gdb.mi/mi-break.exp

> @@ -435,6 +435,28 @@ proc_with_prefix test_forced_conditions {} {

>      mi_gdb_test "-break-info 16" \

>  	"\\^done,[mi_make_breakpoint_table [list $bp]]" \

>          "invalid condition is defined"

> +

> +    # No cond argument should clear the condition.

> +    mi_gdb_test "-break-condition 16" \

> +        "~\"Breakpoint 16's condition is now valid at location 1, enabling.*\\^done" \

> +        "clear the condition"

> +    set bp [mi_make_breakpoint -number 16]

> +    mi_gdb_test "-break-info 16" \

> +	"\\^done,[mi_make_breakpoint_table [list $bp]]" \

> +        "condition is cleared"

> +

> +    # Zero-argument is an error.

> +    mi_gdb_test "-break-condition" \

> +	"\\^error,msg=\"-break-condition: Missing the <number> argument\"" \

> +        "no arguments to -break-condition"

> +

> +    # Passing --force with no condition should not crash or raise an error.

> +    mi_gdb_test "-break-condition --force 16" \

> +	"\\^done" \

> +        "clear the condition with --force"

> +    mi_gdb_test "-break-condition --force" \

> +	"\\^error,msg=\"-break-condition: Missing the <number> argument\"" \

> +        "no arguments with --force"

>  }

>  

>  proc test_break {mi_mode} {

> -- 

> 2.17.1

> 


-- 
Joel
Simon Marchi via Gdb-patches July 25, 2021, 6:06 a.m. | #6
> Date: Sat, 24 Jul 2021 14:15:24 -0700

> From: Joel Brobecker <brobecker@adacore.com>

> Cc: Joel Brobecker <brobecker@adacore.com>

> 

> Hi Eli,

> 

> The code change for this patch has been approved by Simon.

> I was hoping you could take a look at the documentation changes

> for Tankut?


Oops, sorry for missing it.  Yes, the documentation parts are okay.

Patch

diff --git a/gdb/doc/gdb.texinfo b/gdb/doc/gdb.texinfo
index a16a3825ebb..a486bbe3607 100644
--- a/gdb/doc/gdb.texinfo
+++ b/gdb/doc/gdb.texinfo
@@ -30565,7 +30565,7 @@  times="0"@}
 @subsubheading Synopsis
 
 @smallexample
- -break-condition [ --force ] @var{number} @var{expr}
+ -break-condition [ --force ] @var{number} [ @var{expr} ]
 @end smallexample
 
 Breakpoint @var{number} will stop the program only if the condition in
@@ -30573,7 +30573,8 @@  Breakpoint @var{number} will stop the program only if the condition in
 @samp{-break-list} output (see the description of the @samp{-break-list}
 command below).  If the @samp{--force} flag is passed, the condition
 is forcibly defined even when it is invalid for all locations of
-breakpoint @var{number}.
+breakpoint @var{number}.  If the @var{expr} argument is omitted,
+breakpoint @var{number} becomes unconditional.
 
 @subsubheading @value{GDBN} Command
 
diff --git a/gdb/mi/mi-cmd-break.c b/gdb/mi/mi-cmd-break.c
index 5439937f66b..c2d642d75b4 100644
--- a/gdb/mi/mi-cmd-break.c
+++ b/gdb/mi/mi-cmd-break.c
@@ -423,20 +423,19 @@  mi_cmd_break_condition (const char *command, char **argv, int argc)
 	}
     }
 
-  /* There must be at least two more args: a bpnum and a condition
-     expression.  */
-  if (oind + 1 >= argc)
-    error (_("-break-condition: Missing the <number> and/or <expr> "
-	     "argument"));
+  /* There must be at least one more arg: a bpnum.  */
+  if (oind >= argc)
+    error (_("-break-condition: Missing the <number> argument"));
 
   int bpnum = atoi (argv[oind]);
 
   /* The rest form the condition expr.  */
-  std::string expr (argv[oind + 1]);
-  for (int i = oind + 2; i < argc; ++i)
+  std::string expr = "";
+  for (int i = oind + 1; i < argc; ++i)
     {
-      expr += " ";
       expr += argv[i];
+      if (i + 1 < argc)
+	expr += " ";
     }
 
   set_breakpoint_condition (bpnum, expr.c_str (), 0 /* from_tty */,
diff --git a/gdb/testsuite/gdb.mi/mi-break.exp b/gdb/testsuite/gdb.mi/mi-break.exp
index b6ef3483004..f8ff9beb7d2 100644
--- a/gdb/testsuite/gdb.mi/mi-break.exp
+++ b/gdb/testsuite/gdb.mi/mi-break.exp
@@ -435,6 +435,28 @@  proc_with_prefix test_forced_conditions {} {
     mi_gdb_test "-break-info 16" \
 	"\\^done,[mi_make_breakpoint_table [list $bp]]" \
         "invalid condition is defined"
+
+    # No cond argument should clear the condition.
+    mi_gdb_test "-break-condition 16" \
+        "~\"Breakpoint 16's condition is now valid at location 1, enabling.*\\^done" \
+        "clear the condition"
+    set bp [mi_make_breakpoint -number 16]
+    mi_gdb_test "-break-info 16" \
+	"\\^done,[mi_make_breakpoint_table [list $bp]]" \
+        "condition is cleared"
+
+    # Zero-argument is an error.
+    mi_gdb_test "-break-condition" \
+	"\\^error,msg=\"-break-condition: Missing the <number> argument\"" \
+        "no arguments to -break-condition"
+
+    # Passing --force with no condition should not crash or raise an error.
+    mi_gdb_test "-break-condition --force 16" \
+	"\\^done" \
+        "clear the condition with --force"
+    mi_gdb_test "-break-condition --force" \
+	"\\^error,msg=\"-break-condition: Missing the <number> argument\"" \
+        "no arguments with --force"
 }
 
 proc test_break {mi_mode} {