Notify about breakpoint modification when enabling/disabling single location

Message ID 20180529195020.12732-1-jan.vrany@fit.cvut.cz
State New
Headers show
Series
  • Notify about breakpoint modification when enabling/disabling single location
Related show

Commit Message

Jan Vrany May 29, 2018, 7:50 p.m.
When a single breakpoint location enableness was modified, observers were
not notified about it. This issue is now fixed.

gdb/ChangeLog:

        * breakpoint.c (enable_disable_bp_num_loc): Notify observers.

gdb/testsuite/ChangeLog:

        * gdb.mi/mi-breakpoint-changed-2.c: New file.
        * gdb.mi/mi-breakpoint-changed-2.exp: New file.
---
 gdb/ChangeLog                                 |  4 ++
 gdb/breakpoint.c                              |  2 +
 gdb/testsuite/ChangeLog                       |  5 ++
 .../gdb.mi/mi-breakpoint-changed-2.c          | 42 ++++++++++++++
 .../gdb.mi/mi-breakpoint-changed-2.exp        | 57 +++++++++++++++++++
 5 files changed, 110 insertions(+)
 create mode 100644 gdb/testsuite/gdb.mi/mi-breakpoint-changed-2.c
 create mode 100644 gdb/testsuite/gdb.mi/mi-breakpoint-changed-2.exp

-- 
2.17.0

Comments

Simon Marchi May 30, 2018, 7:22 p.m. | #1
Hi Jan,

Thanks for the update.

On 2018-05-29 03:50 PM, Jan Vrany wrote:
> When a single breakpoint location enableness was modified, observers were

> not notified about it. This issue is now fixed.

> 

> gdb/ChangeLog:

> 

>         * breakpoint.c (enable_disable_bp_num_loc): Notify observers.

> 

> gdb/testsuite/ChangeLog:

> 

>         * gdb.mi/mi-breakpoint-changed-2.c: New file.

>         * gdb.mi/mi-breakpoint-changed-2.exp: New file.

> ---

>  gdb/ChangeLog                                 |  4 ++

>  gdb/breakpoint.c                              |  2 +

>  gdb/testsuite/ChangeLog                       |  5 ++

>  .../gdb.mi/mi-breakpoint-changed-2.c          | 42 ++++++++++++++

>  .../gdb.mi/mi-breakpoint-changed-2.exp        | 57 +++++++++++++++++++

>  5 files changed, 110 insertions(+)

>  create mode 100644 gdb/testsuite/gdb.mi/mi-breakpoint-changed-2.c

>  create mode 100644 gdb/testsuite/gdb.mi/mi-breakpoint-changed-2.exp

> 

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

> index 98fb955126..0908a2aac4 100644

> --- a/gdb/ChangeLog

> +++ b/gdb/ChangeLog

> @@ -1,3 +1,7 @@

> +2018-05-29  Jan Vrany  <jan.vrany@fit.cvut.cz>

> +

> +	* breakpoint.c (enable_disable_bp_num_loc): Notify observers.

> +

>  2018-05-27  Tom Tromey  <tom@tromey.com>

>  

>  	* Makefile.in (DEPFILES): Don't reference REMOTE_OBS.

> diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c

> index 721afd2c04..3b380ee2a8 100644

> --- a/gdb/breakpoint.c

> +++ b/gdb/breakpoint.c

> @@ -14225,6 +14225,8 @@ enable_disable_bp_num_loc (int bp_num, int loc_num, bool enable)

>  	target_disable_tracepoint (loc);

>      }

>    update_global_location_list (UGLL_DONT_INSERT);

> +

> +  gdb::observers::breakpoint_modified.notify (loc->owner);

>  }

>  

>  /* Enable or disable a range of breakpoint locations.  BP_NUM is the

> diff --git a/gdb/testsuite/ChangeLog b/gdb/testsuite/ChangeLog

> index b2938b1bf1..c94b236b60 100644

> --- a/gdb/testsuite/ChangeLog

> +++ b/gdb/testsuite/ChangeLog

> @@ -1,3 +1,8 @@

> +2018-05-29  Jan Vrany  <jan.vrany@fit.cvut.cz>

> +

> +	* gdb.mi/mi-breakpoint-changed-2.c: New file.

> +	* gdb.mi/mi-breakpoint-changed-2.exp: New file.

> +

>  2018-05-24  Andrew Burgess  <andrew.burgess@embecosm.com>

>  

>  	PR gdb/23203

> diff --git a/gdb/testsuite/gdb.mi/mi-breakpoint-changed-2.c b/gdb/testsuite/gdb.mi/mi-breakpoint-changed-2.c

> new file mode 100644

> index 0000000000..c4a295aa21

> --- /dev/null

> +++ b/gdb/testsuite/gdb.mi/mi-breakpoint-changed-2.c

> @@ -0,0 +1,42 @@

> +/* This testcase is part of GDB, the GNU debugger.

> +

> +   Copyright 2018 Free Software Foundation, Inc.

> +

> +   This program is free software; you can redistribute it and/or modify

> +   it under the terms of the GNU General Public License as published by

> +   the Free Software Foundation; either version 3 of the License, or

> +   (at your option) any later version.

> +

> +   This program is distributed in the hope that it will be useful,

> +   but WITHOUT ANY WARRANTY; without even the implied warranty of

> +   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the

> +   GNU General Public License for more details.

> +

> +   You should have received a copy of the GNU General Public License

> +   along with this program.  If not, see <http://www.gnu.org/licenses/>.  */> +

> +#include <stdio.h>

> +#include <stdlib.h>

> +

> +static inline int 

> +add(int a, int b)  {

> +    return a + b;

> +}

> +

> +int 

> +my_rand_1(void) {

> +    return add(rand(), rand());

> +}

> +

> +int

> +my_rand_2(void) {

> +    int r = my_rand_1();

> +    return add(r, r);

> +}

> +

> +int

> +main (void)

> +{

> +  int i = my_rand_1();

> +  return 1; /* next-line */

> +}


Please try to make the C code follow the GNU formatting (spaces before parenthesis,
curly braces at column 0, etc).

Also, I got these warnings when applying your patch:

.git/rebase-apply/patch:75: trailing whitespace.
static inline int
.git/rebase-apply/patch:80: trailing whitespace.
int
.git/rebase-apply/patch:158: new blank line at EOF.

Make sure to remove trailing whitespaces and have a \n at end of file.


> \ No newline at end of file

> diff --git a/gdb/testsuite/gdb.mi/mi-breakpoint-changed-2.exp b/gdb/testsuite/gdb.mi/mi-breakpoint-changed-2.exp

> new file mode 100644

> index 0000000000..a67be67d32

> --- /dev/null

> +++ b/gdb/testsuite/gdb.mi/mi-breakpoint-changed-2.exp

> @@ -0,0 +1,57 @@

> +# Copyright 2018 Free Software Foundation, Inc.

> +

> +# This program is free software; you can redistribute it and/or modify

> +# it under the terms of the GNU General Public License as published by

> +# the Free Software Foundation; either version 3 of the License, or

> +# (at your option) any later version.

> +#

> +# This program is distributed in the hope that it will be useful,

> +# but WITHOUT ANY WARRANTY; without even the implied warranty of

> +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the

> +# GNU General Public License for more details.

> +#

> +# You should have received a copy of the GNU General Public License

> +# along with this program.  If not, see <http://www.gnu.org/licenses/>.



Could you add a one-line comment here that describes what this test tests?

Could you also give the test a more descriptive name?  Perhaps mi-breakpoint-location-ena-dis.exp?

> +

> +

> +load_lib mi-support.exp

> +set MIFLAGS "-i=mi"

> +

> +gdb_exit

> +if {[mi_gdb_start]} {

> +    continue

> +}

> +

> +#

> +# Start here

> +#

> +standard_testfile

> +

> +if {[gdb_compile "$srcdir/$subdir/$srcfile" $binfile executable {debug additional_flags=-O2}] != "" } {


I guess the -O2 is to make the compiler inline add?  Instead of relying of the
compiler inlining the function (which it might decide not to do), it might be
preferable to have two different functions with the same name.  Either by:

1. making your test c++, then you can have two overloads of "add".
2. making your test have two .c files, then you can have static "add" functions
   in each.

In both cases, GDB should create two locations for the breakpoint.

Simon

Patch

diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index 98fb955126..0908a2aac4 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,3 +1,7 @@ 
+2018-05-29  Jan Vrany  <jan.vrany@fit.cvut.cz>
+
+	* breakpoint.c (enable_disable_bp_num_loc): Notify observers.
+
 2018-05-27  Tom Tromey  <tom@tromey.com>
 
 	* Makefile.in (DEPFILES): Don't reference REMOTE_OBS.
diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
index 721afd2c04..3b380ee2a8 100644
--- a/gdb/breakpoint.c
+++ b/gdb/breakpoint.c
@@ -14225,6 +14225,8 @@  enable_disable_bp_num_loc (int bp_num, int loc_num, bool enable)
 	target_disable_tracepoint (loc);
     }
   update_global_location_list (UGLL_DONT_INSERT);
+
+  gdb::observers::breakpoint_modified.notify (loc->owner);
 }
 
 /* Enable or disable a range of breakpoint locations.  BP_NUM is the
diff --git a/gdb/testsuite/ChangeLog b/gdb/testsuite/ChangeLog
index b2938b1bf1..c94b236b60 100644
--- a/gdb/testsuite/ChangeLog
+++ b/gdb/testsuite/ChangeLog
@@ -1,3 +1,8 @@ 
+2018-05-29  Jan Vrany  <jan.vrany@fit.cvut.cz>
+
+	* gdb.mi/mi-breakpoint-changed-2.c: New file.
+	* gdb.mi/mi-breakpoint-changed-2.exp: New file.
+
 2018-05-24  Andrew Burgess  <andrew.burgess@embecosm.com>
 
 	PR gdb/23203
diff --git a/gdb/testsuite/gdb.mi/mi-breakpoint-changed-2.c b/gdb/testsuite/gdb.mi/mi-breakpoint-changed-2.c
new file mode 100644
index 0000000000..c4a295aa21
--- /dev/null
+++ b/gdb/testsuite/gdb.mi/mi-breakpoint-changed-2.c
@@ -0,0 +1,42 @@ 
+/* This testcase is part of GDB, the GNU debugger.
+
+   Copyright 2018 Free Software Foundation, Inc.
+
+   This program is free software; you can redistribute it and/or modify
+   it under the terms of the GNU General Public License as published by
+   the Free Software Foundation; either version 3 of the License, or
+   (at your option) any later version.
+
+   This program is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+   GNU General Public License for more details.
+
+   You should have received a copy of the GNU General Public License
+   along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
+
+#include <stdio.h>
+#include <stdlib.h>
+
+static inline int 
+add(int a, int b)  {
+    return a + b;
+}
+
+int 
+my_rand_1(void) {
+    return add(rand(), rand());
+}
+
+int
+my_rand_2(void) {
+    int r = my_rand_1();
+    return add(r, r);
+}
+
+int
+main (void)
+{
+  int i = my_rand_1();
+  return 1; /* next-line */
+}
\ No newline at end of file
diff --git a/gdb/testsuite/gdb.mi/mi-breakpoint-changed-2.exp b/gdb/testsuite/gdb.mi/mi-breakpoint-changed-2.exp
new file mode 100644
index 0000000000..a67be67d32
--- /dev/null
+++ b/gdb/testsuite/gdb.mi/mi-breakpoint-changed-2.exp
@@ -0,0 +1,57 @@ 
+# Copyright 2018 Free Software Foundation, Inc.
+
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 3 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.
+
+
+load_lib mi-support.exp
+set MIFLAGS "-i=mi"
+
+gdb_exit
+if {[mi_gdb_start]} {
+    continue
+}
+
+#
+# Start here
+#
+standard_testfile
+
+if {[gdb_compile "$srcdir/$subdir/$srcfile" $binfile executable {debug additional_flags=-O2}] != "" } {
+    return -1
+}
+
+mi_run_to_main
+
+mi_gdb_test "break add" \
+	{(&.*)*.*~"Breakpoint 2 at.*\\n".*=breakpoint-created,bkpt=\{number="2",type="breakpoint".*\},\{number="2.1",enabled="y".*\}.*\n\^done} \
+	"break add"
+
+# Modify enableness through MI commands shouldn't trigger MI
+# notification.
+mi_gdb_test "-break-disable 2.2" "\\^done" "-break-disable 2.2"
+mi_gdb_test "-break-enable 2.2"  "\\^done" "-break-enable 2.2"	
+
+# Modify enableness through CLI commands should trigger MI
+# notification.
+mi_gdb_test "dis 2.2" \
+	{.*=breakpoint-modified,bkpt=\{number="2",type="breakpoint".*\},\{number="2.1",enabled="y".*\},\{number="2.2",enabled="n".*\}.*\n\^done} \
+	"dis 2.2"
+mi_gdb_test "en 2.2" \
+	{.*=breakpoint-modified,bkpt=\{number="2",type="breakpoint".*\},\{number="2.1",enabled="y".*\},\{number="2.2",enabled="y".*\}.*\n\^done} \
+	"en 2.2"
+
+
+mi_gdb_exit
+
+