Re: [PATCH] Fix MI dprintf-insert not printing when a location is pending.

[Keith Seitz <keiths@redhat.com>]

On 03/26/2015 09:47 AM, Antoine Tremblay wrote:
> gdb/ChangeLog:
>
> 	PR breakpoints/16465
> 	* breakpoint.c (create_breakpoint): Fix missing extra_string.

This is a real nit, so please don't go making any changes here unless a 
maintainer requests it, but this changelog entry doesn't really explain 
the change you've made. ["Save `extra_string' for pending breakpoints." 
is much more descriptive/helpful.]

> @@ -9783,10 +9786,16 @@ create_breakpoint (struct gdbarch *gdbarch,
>   	      cond_string = xstrdup (cond_string);
>   	      make_cleanup (xfree, cond_string);
>   	    }
> +	  /* Create a private copy of any extra string.  */
> +	  if (extra_string)

We explicitly test against NULL for pointers. [I know there are quite a 
few violations of this in this function. All are awaiting an 
easy/obvious separate cleanup. :-)]

> +	    {
> +	      extra_string = xstrdup (extra_string);
> +	      make_cleanup (xfree, extra_string);
> +	    }
>   	  b->cond_string = cond_string;
> +	  b->extra_string = extra_string;
>   	  b->thread = thread;
>   	}
> -      b->extra_string = NULL;
>         b->ignore_count = ignore_count;
>         b->disposition = tempflag ? disp_del : disp_donttouch;
>         b->condition_not_parsed = 1;

FWIW, I have pretty much an identical change in my locations API 
refactor, where I ran across this problem (and more) during testing.

> diff --git a/gdb/testsuite/gdb.mi/mi-dprintf-pending.exp b/gdb/testsuite/gdb.mi/mi-dprintf-pending.exp
> new file mode 100644
> index 0000000..6832f1d
> --- /dev/null
> +++ b/gdb/testsuite/gdb.mi/mi-dprintf-pending.exp
> @@ -0,0 +1,77 @@
> +if {[skip_shlib_tests]} {
> +if [get_compiler_info] {
> +if { [gdb_compile_shlib $libsrc1 $lib_sl1 $lib_opts] != "" } {
> +if { [gdb_compile $srcdir/$subdir/$srcfile $binfile executable $exec_opts] != ""} {

This is a /big/ nitpick, but it's something that consistently irritates 
me: compare the coding style of the four statements above. While there 
is no "rule" governing which is most correct/desired, I always use the 
first. I would ask you to choose one style and be consistent, but I am 
not asking you to make any changes right now. Just please keep this in 
mind in future patches.

> +# Set pending dprintf via MI.
> +mi_gdb_test "-dprintf-insert -f pendfunc1 \"hello\"" \
> +    ".*\\^done,bkpt={number=\"1\",type=\"dprintf\",disp=\"keep\",enabled=\"y\",addr=\"<PENDING>\",pending=\"pendfunc1\",times=\"0\",original-location=\"pendfunc1\"}" \
> +    "mi set dprintf"
> +
> +mi_gdb_test "-break-insert $bp_location1" ".*" "mi insert breakpoint bp_location1"

Is it possible to use mi_make_breakpoint for these tests?

> +
> +mi_run_cmd
> +
> +set msg "mi dprintf"
> +gdb_expect {
> +    -re ".*~\"hello\"" {
> +	pass $msg
> +    }
> +    -re ".*$mi_gdb_prompt$" {
> +	fail $msg
> +    }
> +    timeout {
> +	fail $msg
> +    }
> +}

This a pretty common test suite idiom, I think. Can mi_gdb_test be used 
instead of gdb_expect?

> +mi_expect_stop ".*" ".*" ".*" ".*" ".*" "" "$msg stop"
> diff --git a/gdb/testsuite/gdb.mi/mi-dprintf-pendshr.c b/gdb/testsuite/gdb.mi/mi-dprintf-pendshr.c
> new file mode 100644
> index 0000000..fe49a8d
> --- /dev/null
> +++ b/gdb/testsuite/gdb.mi/mi-dprintf-pendshr.c
> @@ -0,0 +1,27 @@
> +/* This testcase is part of GDB, the GNU debugger.
> +
> +   Copyright 2015 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/>.  */
> +
> +void
> +pendfunc1 ()
> +{
> +}
> +
> +void
> +pendfunc ()
> +{
> +  pendfunc1();
> +}
>

IIRC, we are now requiring test case conformance to the coding standard. 
[At least, that's what I've been told in the past.] So, "(void)" in the 
function decls and spaces between function names and '('. [I don't think 
we're requiring function comments for trivial stuff like this, though.]

Keith