Re: [PATCH] Notify about breakpoint modification when enabling/disabling single location

[Simon Marchi <simon dot marchi at ericsson dot com>]

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