[4/4] gdb/mi: add a '-b' flag to the '-break-insert' cmd to force the condition
diff mbox series

Message ID 30001cddf6183155c7355df3c74848881f2d80c4.1617806599.git.tankut.baris.aktemur@intel.com
State New
Headers show
Series
  • Multi-context invalid breakpoint conditions and MI
Related show

Commit Message

Keith Seitz via Gdb-patches April 7, 2021, 2:55 p.m. 
Add a '-b' flag to the '-break-insert' command to be able to force
conditions.  The '-break-condition' command directly uses the CLI's
'cond' command; hence, it already recognizes the '-force' flag.

Because the '-dprintf-insert' command uses the same mechanism as the
'-break-insert' command, it obtains the '-b' flag, too.

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

	* mi/mi-cmd-break.c (mi_cmd_break_insert_1): Recognize the
	'-b' flag to force the condition in the '-break-insert' and
	'-dprintf-insert' commands.
	* NEWS: Mention the change.

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

	* gdb.mi/mi-break.exp (test_forced_conditions): New proc that
	is called by the test.

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

	* gdb.texinfo (GDB/MI Breakpoint Commands): Mention the '-b'
	flag of the '-break-insert' and '-dprintf-insert' commands,
	and the '--force' flag of the '-break-condition' command.
---
 gdb/NEWS                          |  7 +++++++
 gdb/doc/gdb.texinfo               | 16 ++++++++++++----
 gdb/mi/mi-cmd-break.c             | 10 ++++++++--
 gdb/testsuite/gdb.mi/mi-break.exp | 18 ++++++++++++++++++
 4 files changed, 45 insertions(+), 6 deletions(-)

-- 
2.17.1

Comments

Keith Seitz via Gdb-patches April 7, 2021, 3:18 p.m. | #1 
> Date: Wed,  7 Apr 2021 16:55:59 +0200

> From: Tankut Baris Aktemur via Gdb-patches <gdb-patches@sourceware.org>

> 

> diff --git a/gdb/NEWS b/gdb/NEWS

> index 6cf76a14317..0232ab92419 100644

> --- a/gdb/NEWS

> +++ b/gdb/NEWS

> @@ -31,6 +31,13 @@

>      equivalent of the CLI's "break -qualified" and "dprintf

>      -qualified".

>  

> + ** '-break-insert -b' and '-dprintf-insert -b'

> +

> +    The MI -break-insert and -dprintf-insert commands now support a

> +    '-b' flag to forcibly define a condition even when the condition

> +    is invalid at all locations of the breakpoint.  This is equivalent

> +    to the '--force-condition' flag of the CLI's "break" command.


By "invalid at all locations" did you mean "invalid for some of the
locations"?  IOW, does this flag handle the case when the condition is
valid for some locations and invalid for others, or is it specifically
for the case when the condition is invalid for _all_ the locations?
Keith Seitz via Gdb-patches April 7, 2021, 3:27 p.m. | #2 
On Wednesday, April 7, 2021 5:18 PM, Eli Zaretskii wrote:
> > Date: Wed,  7 Apr 2021 16:55:59 +0200

> > From: Tankut Baris Aktemur via Gdb-patches <gdb-patches@sourceware.org>

> >

> > diff --git a/gdb/NEWS b/gdb/NEWS

> > index 6cf76a14317..0232ab92419 100644

> > --- a/gdb/NEWS

> > +++ b/gdb/NEWS

> > @@ -31,6 +31,13 @@

> >      equivalent of the CLI's "break -qualified" and "dprintf

> >      -qualified".

> >

> > + ** '-break-insert -b' and '-dprintf-insert -b'

> > +

> > +    The MI -break-insert and -dprintf-insert commands now support a

> > +    '-b' flag to forcibly define a condition even when the condition

> > +    is invalid at all locations of the breakpoint.  This is equivalent

> > +    to the '--force-condition' flag of the CLI's "break" command.

> 

> By "invalid at all locations" did you mean "invalid for some of the

> locations"?  IOW, does this flag handle the case when the condition is

> valid for some locations and invalid for others, or is it specifically

> for the case when the condition is invalid for _all_ the locations?


It is specifically for all the locations.  If there are some locations at
which the condition is valid, the breakpoint is accepted.  There is no need
for the '-b' flag then.  The locations at which the condition is valid are
disabled.

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
Keith Seitz via Gdb-patches April 7, 2021, 3:53 p.m. | #3 
> From: "Aktemur, Tankut Baris" <tankut.baris.aktemur@intel.com>

> CC: "gdb-patches@sourceware.org" <gdb-patches@sourceware.org>

> Date: Wed, 7 Apr 2021 15:27:13 +0000

> 

> > > +    The MI -break-insert and -dprintf-insert commands now support a

> > > +    '-b' flag to forcibly define a condition even when the condition

> > > +    is invalid at all locations of the breakpoint.  This is equivalent

> > > +    to the '--force-condition' flag of the CLI's "break" command.

> > 

> > By "invalid at all locations" did you mean "invalid for some of the

> > locations"?  IOW, does this flag handle the case when the condition is

> > valid for some locations and invalid for others, or is it specifically

> > for the case when the condition is invalid for _all_ the locations?

> 

> It is specifically for all the locations.  If there are some locations at

> which the condition is valid, the breakpoint is accepted.  There is no need

> for the '-b' flag then.  The locations at which the condition is valid are

> disabled.


Then how about simply accepting the condition by default in that case,
without any need for a special flag?  IOW, it makes little sense to me
to accept the condition if it is _sometimes_ invalid, but not if it is
_always_ invalid.  It sounds like a gratuitous limitation.  Am I
missing something?
Keith Seitz via Gdb-patches April 7, 2021, 4:05 p.m. | #4 
On Wednesday, April 7, 2021 5:54 PM, Eli Zaretskii wrote:
> > From: "Aktemur, Tankut Baris" <tankut.baris.aktemur@intel.com>

> > CC: "gdb-patches@sourceware.org" <gdb-patches@sourceware.org>

> > Date: Wed, 7 Apr 2021 15:27:13 +0000

> >

> > > > +    The MI -break-insert and -dprintf-insert commands now support a

> > > > +    '-b' flag to forcibly define a condition even when the condition

> > > > +    is invalid at all locations of the breakpoint.  This is equivalent

> > > > +    to the '--force-condition' flag of the CLI's "break" command.

> > >

> > > By "invalid at all locations" did you mean "invalid for some of the

> > > locations"?  IOW, does this flag handle the case when the condition is

> > > valid for some locations and invalid for others, or is it specifically

> > > for the case when the condition is invalid for _all_ the locations?

> >

> > It is specifically for all the locations.  If there are some locations at

> > which the condition is valid, the breakpoint is accepted.  There is no need

> > for the '-b' flag then.  The locations at which the condition is valid are

> > disabled.

> 

> Then how about simply accepting the condition by default in that case,

> without any need for a special flag?  IOW, it makes little sense to me

> to accept the condition if it is _sometimes_ invalid, but not if it is

> _always_ invalid.  It sounds like a gratuitous limitation.  Am I

> missing something?


There are two problems with that approach:

1. Backward compatibility would be broken.

2. (The more important one, IMHO) If the user makes a typo in the condition,
the breakpoint would still be accepted by making it disabled at all the locations.
The force flag is the user telling GDB "I'm sure about this condition expression,
it will be meaningful in the future, just accept it." I think this scenario is less
common than the user entering really bad conditions.  Hence, it had made more sense
to reject the breakpoint in the default case.

-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
Keith Seitz via Gdb-patches April 7, 2021, 4:50 p.m. | #5 
> From: "Aktemur, Tankut Baris" <tankut.baris.aktemur@intel.com>

> CC: "gdb-patches@sourceware.org" <gdb-patches@sourceware.org>

> Date: Wed, 7 Apr 2021 16:05:11 +0000

> > Then how about simply accepting the condition by default in that case,

> > without any need for a special flag?  IOW, it makes little sense to me

> > to accept the condition if it is _sometimes_ invalid, but not if it is

> > _always_ invalid.  It sounds like a gratuitous limitation.  Am I

> > missing something?

> 

> There are two problems with that approach:

> 

> 1. Backward compatibility would be broken.


Would someone expect the current behavior?  Why would they?

> 2. (The more important one, IMHO) If the user makes a typo in the condition,

> the breakpoint would still be accepted by making it disabled at all the locations.


That's easy: issue a warning.

Anyway, if no one else thinks like me, I'm okay with doing it your
way.
Keith Seitz via Gdb-patches April 7, 2021, 10:26 p.m. | #6 
On 2021-04-07 10:55 a.m., Tankut Baris Aktemur via Gdb-patches wrote:
> @@ -203,6 +205,7 @@ mi_cmd_break_insert_1 (int dprintf, const char *command, char **argv, int argc)

>      {"f", PENDING_OPT, 0},

>      {"d", DISABLE_OPT, 0},

>      {"a", TRACEPOINT_OPT, 0},

> +    {"b", FORCE_CONDITION_OPT, 0},

>      {"-qualified", QUALIFIED_OPT, 0},

>      {"-source" , EXPLICIT_SOURCE_OPT, 1},

>      {"-function", EXPLICIT_FUNC_OPT, 1},


Hi, it's me nitpicking names again: instead of choosing a meaningless
single letter, could we use --force-condition?  It would just be more
obvious to any human reading MI logs, and machines don't really care
about the extra typing.

> +proc test_forced_conditions {} {

> +    # Test forcing an invalid condition.

> +    mi_gdb_test "info break"

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

> +        ".*warning: failed to validate condition at location 15.1, disabling:.*" \

> +        "invalid condition is forced"

> +

> +    mi_gdb_test "-break-insert -c bad -b callme" \

> +        ".*warning: failed to validate condition at location 1, disabling:.*" \

> +        "breakpoint with bad condition is forced"

> +

> +    mi_gdb_test "-dprintf-insert -c bad -b callme 123" \

> +        ".*warning: failed to validate condition at location 1, disabling:.*" \

> +        "dprintf with bad condition is forced"



It would be good to validate at least that the commands succeed (output ^done).

At least for the -break-insert case, it might be a good idea to improve
(to be able to pass the -b/--force-condition flag) and use
mi_create_breakpoint_multi.  You could then validate what the command
outputs (one location with enabled="N").

Simon

Patch
diff mbox series 

diff --git a/gdb/NEWS b/gdb/NEWS
index 6cf76a14317..0232ab92419 100644
--- a/gdb/NEWS
+++ b/gdb/NEWS
@@ -31,6 +31,13 @@ 
     equivalent of the CLI's "break -qualified" and "dprintf
     -qualified".
 
+ ** '-break-insert -b' and '-dprintf-insert -b'
+
+    The MI -break-insert and -dprintf-insert commands now support a
+    '-b' flag to forcibly define a condition even when the condition
+    is invalid at all locations of the breakpoint.  This is equivalent
+    to the '--force-condition' flag of the CLI's "break" command.
+
 * GDB now supports core file debugging for x86_64 Cygwin programs.
 
 * GDB will now look for the .gdbinit file in a config directory before
diff --git a/gdb/doc/gdb.texinfo b/gdb/doc/gdb.texinfo
index bfac2b6d245..79839bdfbd5 100644
--- a/gdb/doc/gdb.texinfo
+++ b/gdb/doc/gdb.texinfo
@@ -30396,13 +30396,15 @@  times="0"@}
 @subsubheading Synopsis
 
 @smallexample
- -break-condition @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
 @var{expr} is true.  The condition becomes part of the
 @samp{-break-list} output (see the description of the @samp{-break-list}
-command below).
+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}.
 
 @subsubheading @value{GDBN} Command
 
@@ -30567,7 +30569,7 @@  N.A.
 @subsubheading Synopsis
 
 @smallexample
- -break-insert [ -t ] [ -h ] [ -f ] [ -d ] [ -a ] [ --qualified ]
+ -break-insert [ -t ] [ -h ] [ -f ] [ -d ] [ -a ] [ -b ] [ --qualified ]
     [ -c @var{condition} ] [ -i @var{ignore-count} ]
     [ -p @var{thread-id} ] [ @var{location} ]
 @end smallexample
@@ -30624,6 +30626,9 @@  Create a tracepoint.  @xref{Tracepoints}.  When this parameter
 is used together with @samp{-h}, a fast tracepoint is created.
 @item -c @var{condition}
 Make the breakpoint conditional on @var{condition}.
+@item -b
+Forcibly define the breakpoint even if the condition is invalid at
+all of the breakpoint locations.
 @item -i @var{ignore-count}
 Initialize the @var{ignore-count}.
 @item -p @var{thread-id}
@@ -30692,7 +30697,7 @@  times="0"@}]@}
 @subsubheading Synopsis
 
 @smallexample
- -dprintf-insert [ -t ] [ -f ] [ -d ] [ --qualified ]
+ -dprintf-insert [ -t ] [ -f ] [ -d ] [ -b ] [ --qualified ]
     [ -c @var{condition} ] [ -i @var{ignore-count} ]
     [ -p @var{thread-id} ] [ @var{location} ] [ @var{format} ]
     [ @var{argument} ]
@@ -30718,6 +30723,9 @@  cannot be parsed.
 Create a disabled breakpoint.
 @item -c @var{condition}
 Make the breakpoint conditional on @var{condition}.
+@item -b
+Forcibly define the breakpoint even if the condition is invalid at
+all of the breakpoint locations.
 @item -i @var{ignore-count}
 Set the ignore count of the breakpoint (@pxref{Conditions, ignore count})
 to @var{ignore-count}.
diff --git a/gdb/mi/mi-cmd-break.c b/gdb/mi/mi-cmd-break.c
index 5a4a62ce8c3..09517d67927 100644
--- a/gdb/mi/mi-cmd-break.c
+++ b/gdb/mi/mi-cmd-break.c
@@ -183,6 +183,7 @@  mi_cmd_break_insert_1 (int dprintf, const char *command, char **argv, int argc)
   int is_explicit = 0;
   struct explicit_location explicit_loc;
   std::string extra_string;
+  bool force_condition = false;
 
   enum opt
     {
@@ -191,7 +192,8 @@  mi_cmd_break_insert_1 (int dprintf, const char *command, char **argv, int argc)
       TRACEPOINT_OPT,
       QUALIFIED_OPT,
       EXPLICIT_SOURCE_OPT, EXPLICIT_FUNC_OPT,
-      EXPLICIT_LABEL_OPT, EXPLICIT_LINE_OPT
+      EXPLICIT_LABEL_OPT, EXPLICIT_LINE_OPT,
+      FORCE_CONDITION_OPT
     };
   static const struct mi_opt opts[] =
   {
@@ -203,6 +205,7 @@  mi_cmd_break_insert_1 (int dprintf, const char *command, char **argv, int argc)
     {"f", PENDING_OPT, 0},
     {"d", DISABLE_OPT, 0},
     {"a", TRACEPOINT_OPT, 0},
+    {"b", FORCE_CONDITION_OPT, 0},
     {"-qualified", QUALIFIED_OPT, 0},
     {"-source" , EXPLICIT_SOURCE_OPT, 1},
     {"-function", EXPLICIT_FUNC_OPT, 1},
@@ -269,6 +272,9 @@  mi_cmd_break_insert_1 (int dprintf, const char *command, char **argv, int argc)
 	  is_explicit = 1;
 	  explicit_loc.line_offset = linespec_parse_line_offset (oarg);
 	  break;
+	case FORCE_CONDITION_OPT:
+	  force_condition = true;
+	  break;
 	}
     }
 
@@ -353,7 +359,7 @@  mi_cmd_break_insert_1 (int dprintf, const char *command, char **argv, int argc)
 
   create_breakpoint (get_current_arch (), location.get (), condition, thread,
 		     extra_string.c_str (),
-		     false,
+		     force_condition,
 		     0 /* condition and thread are valid.  */,
 		     temp_p, type_wanted,
 		     ignore_count,
diff --git a/gdb/testsuite/gdb.mi/mi-break.exp b/gdb/testsuite/gdb.mi/mi-break.exp
index ac13e4d1e09..87724dea185 100644
--- a/gdb/testsuite/gdb.mi/mi-break.exp
+++ b/gdb/testsuite/gdb.mi/mi-break.exp
@@ -408,6 +408,22 @@  proc test_explicit_breakpoints {} {
 	".*Source filename requires function, label, or line offset.*"
 }
 
+proc test_forced_conditions {} {
+    # Test forcing an invalid condition.
+    mi_gdb_test "info break"
+    mi_gdb_test "-break-condition -force 15 bad" \
+        ".*warning: failed to validate condition at location 15.1, disabling:.*" \
+        "invalid condition is forced"
+
+    mi_gdb_test "-break-insert -c bad -b callme" \
+        ".*warning: failed to validate condition at location 1, disabling:.*" \
+        "breakpoint with bad condition is forced"
+
+    mi_gdb_test "-dprintf-insert -c bad -b callme 123" \
+        ".*warning: failed to validate condition at location 1, disabling:.*" \
+        "dprintf with bad condition is forced"
+}
+
 proc test_break {mi_mode} {
     global srcdir subdir binfile
 
@@ -440,6 +456,8 @@  proc test_break {mi_mode} {
     test_abreak_creation
 
     test_explicit_breakpoints
+
+    test_forced_conditions
 }
 
 if [gdb_debug_enabled] {