[PATCH] Fix dprintf work not right if it is pending

[PATCH] Fix dprintf work not right if it is pending

[Hui Zhu]

Hi.

I found that dprintf work not right if it is pending, for example:
testsuite$ gdb gdb.base/pending
(gdb) dprintf pendfunc1, "got\n"
Function "pendfunc1" not defined.
Make dprintf pending on future shared library load? (y or [n]) y
Dprintf 1 (pendfunc1, "got\n") pending.
(gdb) r
Starting program: /home/teawater/gdb/bgdbno/gdb/testsuite/gdb.base/pending
(gdb) c
Continuing.
in pendfunc1, x is 3
(gdb) c
Continuing.
in pendfunc1, x is 4
(gdb) c
Continuing.
in pendfunc1, x is 3
[Inferior 1 (process 2370) exited normally]

The dprintf's commands is setup in function init_breakpoint_sal:
       /* Dynamic printf requires and uses additional arguments on the
	 command line, otherwise it's an error.  */
       if (type == bp_dprintf)
	{
	  if (b->extra_string)
	    update_dprintf_command_list (b);
	  else
	    error (_("Format string required"));
	}
       else if (b->extra_string)
	error (_("Garbage '%s' at end of command"), b->extra_string);

But if the dprintf is pending.  When it reset by function bkpt_re_set, there is not code to code to update extra_string to commands.
So I add this code to function update_breakpoint_locations.  The issue is fixed.

Please help me review it.

Best.
Hui

2013-03-22  Hui Zhu  <hui@codesourcery.com>

	* breakpoint.c (update_breakpoint_locations): Add handler for dprintf.

--- a/gdb/breakpoint.c
+++ b/gdb/breakpoint.c
@@ -14075,6 +14075,18 @@ update_breakpoint_locations (struct brea
  
  	  new_loc->length = end - sals.sals[0].pc + 1;
  	}
+
+      /* Dynamic printf requires and uses additional arguments on the
+	 command line, otherwise it's an error.  */
+      if (b->type == bp_dprintf)
+	{
+	  if (b->extra_string)
+	    update_dprintf_command_list (b);
+	  else
+	    error (_("Format string required"));
+	}
+      else if (b->extra_string)
+	error (_("Garbage '%s' at end of command"), b->extra_string);
      }
  
    /* Update locations of permanent breakpoints.  */

Re: [PATCH] Fix dprintf work not right if it is pending

[Yao Qi]

On 03/22/2013 02:16 PM, Hui Zhu wrote:
> The dprintf's commands is setup in function init_breakpoint_sal:
>         /* Dynamic printf requires and uses additional arguments on the
> 	 command line, otherwise it's an error.  */
>         if (type == bp_dprintf)
> 	{
> 	  if (b->extra_string)

"b->extra_string != NULL" is encouraged nowadays.

> 	    update_dprintf_command_list (b);
> 	  else
> 	    error (_("Format string required"));
> 	}
>         else if (b->extra_string)
> 	error (_("Garbage '%s' at end of command"), b->extra_string);
>
> But if the dprintf is pending.  When it reset by function bkpt_re_set, there is not code to code to update extra_string to commands.
> So I add this code to function update_breakpoint_locations.  The issue is fixed.

The bug was reported in PR breakpoints/15292: Pending dprintf don't
work.

We need a test case here, I think, to show pending dprintf doesn't
work, and it works with your patch applied.

I am wondering whether we need a new breakpoint_ops field
"parse_extra_string", and use it like:

    b->ops->parse_extra_string (b, extra_string);

instead of duplicate the code.

-- 
Yao (齐尧)

Re: [PATCH] Fix dprintf work not right if it is pending

[Keith Seitz]

On 03/22/2013 04:12 AM, Yao Qi wrote:
>> But if the dprintf is pending.  When it reset by function bkpt_re_set,
>> there is not code to code to update extra_string to commands.
>> So I add this code to function update_breakpoint_locations.  The issue
>> is fixed.
>
> The bug was reported in PR breakpoints/15292: Pending dprintf don't
> work.
>
> We need a test case here, I think, to show pending dprintf doesn't
> work, and it works with your patch applied.
>
> I am wondering whether we need a new breakpoint_ops field
> "parse_extra_string", and use it like:
>
>     b->ops->parse_extra_string (b, extra_string);
>
> instead of duplicate the code.

I agree: special handling is necessary, but I don't care for either of 
these solutions. The original proposal clutters generic breakpoint code 
with dprintf-specific handling. As Yao correctly points out, this is 
what the breakpoint ops vector is for.

However, I don't like the idea of adding a new "parse_extra_string" 
method. It is far too vague. Parse extra_string when?

I think the better solution, and one which we already have the 
infrastructure for, is to define a dprintf_re_set method in the 
dprintf's breakpoint ops, updating the command list whenever a pending 
breakpoint is resolved.

This definitely needs a test.

Keith

Re: [PATCH] Fix dprintf work not right if it is pending

[Yao Qi]

On 03/24/2013 08:05 PM, Keith Seitz wrote:
> I think the better solution, and one which we already have the
> infrastructure for, is to define a dprintf_re_set method in the
> dprintf's breakpoint ops, updating the command list whenever a pending
> breakpoint is resolved.

Yeah, I agree.

-- 
Yao (齐尧)

Re: [PATCH] Fix dprintf work not right if it is pending

[Hui Zhu]

[-- Attachment #1: Type: text/plain, Size: 1884 bytes --]

Post a new version according to your comments.  And add test for it.

Thanks,
Hui

2013-03-26  Hui Zhu  <hui@codesourcery.com>

	* breakpoint.c (dprintf_re_set): New.
	(initialize_breakpoint_ops): Set dprintf_breakpoint_ops re_set
	to dprintf_re_set.

2013-03-26  Hui Zhu  <hui@codesourcery.com>

	* gdb.base/Makefile.in (EXECUTABLES): Add dprintf-pending.
	(MISCELLANEOUS): Add dprintf-pendshr.sl.
	* gdb.base/dprintf-pending.c, gdb.base/dprintf-pending.exp: New.

On Sun, Mar 24, 2013 at 8:05 PM, Keith Seitz <keiths@redhat.com> wrote:
> On 03/22/2013 04:12 AM, Yao Qi wrote:
>>>
>>> But if the dprintf is pending.  When it reset by function bkpt_re_set,
>>> there is not code to code to update extra_string to commands.
>>> So I add this code to function update_breakpoint_locations.  The issue
>>> is fixed.
>>
>>
>> The bug was reported in PR breakpoints/15292: Pending dprintf don't
>> work.
>>
>> We need a test case here, I think, to show pending dprintf doesn't
>> work, and it works with your patch applied.
>>
>> I am wondering whether we need a new breakpoint_ops field
>> "parse_extra_string", and use it like:
>>
>>     b->ops->parse_extra_string (b, extra_string);
>>
>> instead of duplicate the code.
>
>
> I agree: special handling is necessary, but I don't care for either of these
> solutions. The original proposal clutters generic breakpoint code with
> dprintf-specific handling. As Yao correctly points out, this is what the
> breakpoint ops vector is for.
>
> However, I don't like the idea of adding a new "parse_extra_string" method.
> It is far too vague. Parse extra_string when?
>
> I think the better solution, and one which we already have the
> infrastructure for, is to define a dprintf_re_set method in the dprintf's
> breakpoint ops, updating the command list whenever a pending breakpoint is
> resolved.
>
> This definitely needs a test.
>
> Keith

[-- Attachment #2: dprintf-pending.txt --]
[-- Type: text/plain, Size: 693 bytes --]

--- a/gdb/breakpoint.c
+++ b/gdb/breakpoint.c
@@ -12963,6 +12963,15 @@ bkpt_re_set (struct breakpoint *b)
   breakpoint_re_set_default (b);
 }
 
+static void
+dprintf_re_set (struct breakpoint *b)
+{
+  breakpoint_re_set_default (b);
+
+  if (b->extra_string != NULL)
+    update_dprintf_command_list (b);
+}
+
 static int
 bkpt_insert_location (struct bp_location *bl)
 {
@@ -16001,7 +16010,7 @@ initialize_breakpoint_ops (void)
 
   ops = &dprintf_breakpoint_ops;
   *ops = bkpt_base_breakpoint_ops;
-  ops->re_set = bkpt_re_set;
+  ops->re_set = dprintf_re_set;
   ops->resources_needed = bkpt_resources_needed;
   ops->print_it = bkpt_print_it;
   ops->print_mention = bkpt_print_mention;

[-- Attachment #3: dprintf-pending-test.txt --]
[-- Type: text/plain, Size: 5827 bytes --]

--- a/gdb/testsuite/gdb.base/Makefile.in
+++ b/gdb/testsuite/gdb.base/Makefile.in
@@ -10,7 +10,8 @@ EXECUTABLES = a2-run advance all-types a
 	call-strs callexit callfuncs callfwmall charset checkpoint \
 	chng-syms code_elim1 code_elim2 commands compiler complex \
 	condbreak consecutive constvars coremaker cursal cvexpr \
-	dbx-test del disasm-end-cu display dump dup-sect dup-sect.debug \
+	dbx-test del disasm-end-cu display dprintf-pending dump dup-sect \
+	dup-sect.debug \
 	dup-sect.stripped ending-run execd-prog expand-psymtabs exprs \
 	fileio find finish fixsection float foll-exec foll-fork foll-vfork \
 	frame-args freebpcmd fullname funcargs gcore \
@@ -44,7 +45,7 @@ EXECUTABLES = a2-run advance all-types a
 	wchar whatis whatis-exp catch-syscall \
 	pr10179 gnu_vector
 
-MISCELLANEOUS = coremmap.data ../foobar.baz fixsectshr.sl \
+MISCELLANEOUS = coremmap.data dprintf-pendshr.sl ../foobar.baz fixsectshr.sl \
 	pendshr.sl shreloc1.sl shreloc2.sl twice-tmp.c \
 	shr1.sl shr2.sl solib_sl.sl solib1.sl solib2.sl \
 	unloadshr.sl unloadshr2.sl watchpoint-solib-shr.sl \
--- /dev/null
+++ b/gdb/testsuite/gdb.base/dprintf-pending.c
@@ -0,0 +1,31 @@
+/* This testcase is part of GDB, the GNU debugger.
+
+   Copyright 2004-2013 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>
+
+int k = 0;
+
+extern void pendfunc (int x);
+
+int main()
+{
+  pendfunc (3); /* break main here */
+  pendfunc (4);
+  k = 1;
+  pendfunc (3);
+  return 0;
+}
--- /dev/null
+++ b/gdb/testsuite/gdb.base/dprintf-pending.exp
@@ -0,0 +1,91 @@
+#   Copyright 2013 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/>.
+
+
+if {[skip_shlib_tests]} {
+    return 0
+}
+
+set testfile "dprintf-pending"
+set libfile "dprintf-pendshr"
+set srcfile $testfile.c
+set libsrc  $srcdir/$subdir/$libfile.c
+set binfile $objdir/$subdir/$testfile
+set lib_sl  $objdir/$subdir/$libfile.sl
+
+set lib_opts  debug
+set exec_opts [list debug shlib=$lib_sl]
+
+if [get_compiler_info] {
+    return -1
+}
+
+if { [gdb_compile_shlib $libsrc $lib_sl $lib_opts] != ""
+     || [gdb_compile $srcdir/$subdir/$srcfile $binfile executable $exec_opts] != ""} {
+    untested "Could not compile either $libsrc or $srcdir/$subdir/$srcfile."
+    return -1
+}
+
+# Start with a fresh gdb.
+
+gdb_exit
+gdb_start
+gdb_reinitialize_dir $srcdir/$subdir
+
+gdb_test_multiple "dprintf pendfunc1, \"x=%d\\n\", x" "set pending dprintf" {
+     -re ".*Make dprintf pending.*y or \\\[n\\\]. $" {
+	    gdb_test "y" "Dprintf.*pendfunc1.*pending." "set pending dprintf (without symbols)"
+     }
+}
+
+gdb_test "info break" \
+    "Num     Type\[ \]+Disp Enb Address\[ \]+What.*
+\[0-9\]+\[\t \]+dprintf.*keep y.*PENDING.*pendfunc1.*" \
+"single pending dprintf info (without symbols)"
+
+gdb_load ${binfile}
+gdb_load_shlibs $lib_sl
+
+gdb_run_cmd
+
+gdb_test "" ".*x=3.*x=4.*x=3.*" "run to resolved dprintf (without symbols)"
+
+# Restart with a fresh gdb.
+
+gdb_exit
+gdb_start
+gdb_reinitialize_dir $srcdir/$subdir
+
+gdb_load ${binfile}
+gdb_load_shlibs $lib_sl
+
+#
+# Test setting, querying, and modifying pending breakpoints
+#
+
+gdb_test_multiple "dprintf pendfunc1, \"x=%d\\n\", x" "set pending dprintf" {
+     -re ".*Make dprintf pending.*y or \\\[n\\\]. $" {
+	    gdb_test "y" "Dprintf.*pendfunc1.*pending." "set pending dprintf"
+     }
+}
+
+gdb_test "info break" \
+    "Num     Type\[ \]+Disp Enb Address\[ \]+What.*
+\[0-9\]+\[\t \]+dprintf.*keep y.*PENDING.*pendfunc1.*" \
+"single pending dprintf info (without symbols)"
+
+gdb_run_cmd
+
+gdb_test "" ".*x=3.*x=4.*x=3.*" "run to resolved dprintf"
--- /dev/null
+++ b/gdb/testsuite/gdb.base/dprintf-pendshr.c
@@ -0,0 +1,28 @@
+/* This testcase is part of GDB, the GNU debugger.
+
+   Copyright 2013 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>
+
+void pendfunc1 (int x)
+{
+  int y = x + 4;
+}
+
+void pendfunc (int x)
+{
+  pendfunc1 (x);
+}

Re: [PATCH] Fix dprintf work not right if it is pending

[Keith Seitz]

On 03/25/2013 11:41 PM, Hui Zhu wrote:
> --- a/gdb/breakpoint.c
> +++ b/gdb/breakpoint.c
> @@ -12963,6 +12963,15 @@ bkpt_re_set (struct breakpoint *b)
>     breakpoint_re_set_default (b);
>   }
>
> +static void
> +dprintf_re_set (struct breakpoint *b)
> +{

This function needs a comment, even if it just mentions that this 
function is the re_set method for dprintf breakpoints.

> +  breakpoint_re_set_default (b);
> +
> +  if (b->extra_string != NULL)
> +    update_dprintf_command_list (b);
> +}
> +

This will update the command list every time breakpoints are reset and 
could be limited to only those needing updating. Is there perhaps a 
reason to always do this?

> --- /dev/null
> +++ b/gdb/testsuite/gdb.base/dprintf-pending.c
> @@ -0,0 +1,31 @@
> +/* This testcase is part of GDB, the GNU debugger.
> +
> +   Copyright 2004-2013 Free Software Foundation, Inc.
                 ^^^^^^^^^
I think you meant only 2013. :-)

> --- /dev/null
> +++ b/gdb/testsuite/gdb.base/dprintf-pending.exp
> +
> +if [get_compiler_info] {
> +    return -1
> +}

Can you please add braces around the expression here? [Yeah, that's a 
nit, but it is one of my Tcl pet peeves, and this is a new file. Time to 
stomp out the old crud.]

Keith

Re: [PATCH] Fix dprintf work not right if it is pending

[Hui Zhu]

[-- Attachment #1: Type: text/plain, Size: 1702 bytes --]

Hi Keith,

Thanks for your review.

On Thu, Mar 28, 2013 at 8:29 PM, Keith Seitz <keiths@redhat.com> wrote:
> On 03/25/2013 11:41 PM, Hui Zhu wrote:
>>
>> --- a/gdb/breakpoint.c
>> +++ b/gdb/breakpoint.c
>> @@ -12963,6 +12963,15 @@ bkpt_re_set (struct breakpoint *b)
>>     breakpoint_re_set_default (b);
>>   }
>>
>> +static void
>> +dprintf_re_set (struct breakpoint *b)
>> +{
>
>
> This function needs a comment, even if it just mentions that this function
> is the re_set method for dprintf breakpoints.

Fixed.

>
>> +  breakpoint_re_set_default (b);
>> +
>> +  if (b->extra_string != NULL)
>> +    update_dprintf_command_list (b);
>> +}
>> +
>
>
> This will update the command list every time breakpoints are reset and could
> be limited to only those needing updating. Is there perhaps a reason to
> always do this?

I think it need, because it need to generate different commands with
different status for example:
      if (target_can_run_breakpoint_commands ())
	printf_line = xstrprintf ("agent-printf %s", dprintf_args);

>
>> --- /dev/null
>> +++ b/gdb/testsuite/gdb.base/dprintf-pending.c
>> @@ -0,0 +1,31 @@
>> +/* This testcase is part of GDB, the GNU debugger.
>> +
>> +   Copyright 2004-2013 Free Software Foundation, Inc.
>
>                 ^^^^^^^^^
> I think you meant only 2013. :-)

Fixed.

>
>> --- /dev/null
>> +++ b/gdb/testsuite/gdb.base/dprintf-pending.exp
>> +
>> +if [get_compiler_info] {
>> +    return -1
>> +}
>
>
> Can you please add braces around the expression here? [Yeah, that's a nit,
> but it is one of my Tcl pet peeves, and this is a new file. Time to stomp
> out the old crud.]

Fixed.

Post the new version according to your comments.

Best,
Hui

>
> Keith

[-- Attachment #2: dprintf-pending.txt --]
[-- Type: text/plain, Size: 735 bytes --]

--- a/gdb/breakpoint.c
+++ b/gdb/breakpoint.c
@@ -12963,6 +12963,17 @@ bkpt_re_set (struct breakpoint *b)
   breakpoint_re_set_default (b);
 }
 
+/* Dprintf breakpoint_ops methods.  */
+
+static void
+dprintf_re_set (struct breakpoint *b)
+{
+  breakpoint_re_set_default (b);
+
+  if (b->extra_string != NULL)
+    update_dprintf_command_list (b);
+}
+
 static int
 bkpt_insert_location (struct bp_location *bl)
 {
@@ -16001,7 +16012,7 @@ initialize_breakpoint_ops (void)
 
   ops = &dprintf_breakpoint_ops;
   *ops = bkpt_base_breakpoint_ops;
-  ops->re_set = bkpt_re_set;
+  ops->re_set = dprintf_re_set;
   ops->resources_needed = bkpt_resources_needed;
   ops->print_it = bkpt_print_it;
   ops->print_mention = bkpt_print_mention;

[-- Attachment #3: dprintf-pending-test.txt --]
[-- Type: text/plain, Size: 5826 bytes --]

--- a/gdb/testsuite/gdb.base/Makefile.in
+++ b/gdb/testsuite/gdb.base/Makefile.in
@@ -10,7 +10,8 @@ EXECUTABLES = a2-run advance all-types a
 	call-strs callexit callfuncs callfwmall charset checkpoint \
 	chng-syms code_elim1 code_elim2 commands compiler complex \
 	condbreak consecutive constvars coremaker cursal cvexpr \
-	dbx-test del disasm-end-cu display dump dup-sect dup-sect.debug \
+	dbx-test del disasm-end-cu display dprintf-pending dump dup-sect \
+	dup-sect.debug \
 	dup-sect.stripped ending-run execd-prog expand-psymtabs exprs \
 	fileio find finish fixsection float foll-exec foll-fork foll-vfork \
 	frame-args freebpcmd fullname funcargs gcore \
@@ -44,7 +45,7 @@ EXECUTABLES = a2-run advance all-types a
 	wchar whatis whatis-exp catch-syscall \
 	pr10179 gnu_vector
 
-MISCELLANEOUS = coremmap.data ../foobar.baz fixsectshr.sl \
+MISCELLANEOUS = coremmap.data dprintf-pendshr.sl ../foobar.baz fixsectshr.sl \
 	pendshr.sl shreloc1.sl shreloc2.sl twice-tmp.c \
 	shr1.sl shr2.sl solib_sl.sl solib1.sl solib2.sl \
 	unloadshr.sl unloadshr2.sl watchpoint-solib-shr.sl \
--- /dev/null
+++ b/gdb/testsuite/gdb.base/dprintf-pending.c
@@ -0,0 +1,31 @@
+/* This testcase is part of GDB, the GNU debugger.
+
+   Copyright 2013 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>
+
+int k = 0;
+
+extern void pendfunc (int x);
+
+int main()
+{
+  pendfunc (3); /* break main here */
+  pendfunc (4);
+  k = 1;
+  pendfunc (3);
+  return 0;
+}
--- /dev/null
+++ b/gdb/testsuite/gdb.base/dprintf-pending.exp
@@ -0,0 +1,91 @@
+#   Copyright 2013 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/>.
+
+
+if {[skip_shlib_tests]} {
+    return 0
+}
+
+set testfile "dprintf-pending"
+set libfile "dprintf-pendshr"
+set srcfile $testfile.c
+set libsrc  $srcdir/$subdir/$libfile.c
+set binfile $objdir/$subdir/$testfile
+set lib_sl  $objdir/$subdir/$libfile.sl
+
+set lib_opts  debug
+set exec_opts [list debug shlib=$lib_sl]
+
+if { [get_compiler_info] } {
+    return -1
+}
+
+if { [gdb_compile_shlib $libsrc $lib_sl $lib_opts] != ""
+     || [gdb_compile $srcdir/$subdir/$srcfile $binfile executable $exec_opts] != ""} {
+    untested "Could not compile either $libsrc or $srcdir/$subdir/$srcfile."
+    return -1
+}
+
+# Start with a fresh gdb.
+
+gdb_exit
+gdb_start
+gdb_reinitialize_dir $srcdir/$subdir
+
+gdb_test_multiple "dprintf pendfunc1, \"x=%d\\n\", x" "set pending dprintf" {
+     -re ".*Make dprintf pending.*y or \\\[n\\\]. $" {
+	    gdb_test "y" "Dprintf.*pendfunc1.*pending." "set pending dprintf (without symbols)"
+     }
+}
+
+gdb_test "info break" \
+    "Num     Type\[ \]+Disp Enb Address\[ \]+What.*
+\[0-9\]+\[\t \]+dprintf.*keep y.*PENDING.*pendfunc1.*" \
+"single pending dprintf info (without symbols)"
+
+gdb_load ${binfile}
+gdb_load_shlibs $lib_sl
+
+gdb_run_cmd
+
+gdb_test "" ".*x=3.*x=4.*x=3.*" "run to resolved dprintf (without symbols)"
+
+# Restart with a fresh gdb.
+
+gdb_exit
+gdb_start
+gdb_reinitialize_dir $srcdir/$subdir
+
+gdb_load ${binfile}
+gdb_load_shlibs $lib_sl
+
+#
+# Test setting, querying, and modifying pending breakpoints
+#
+
+gdb_test_multiple "dprintf pendfunc1, \"x=%d\\n\", x" "set pending dprintf" {
+     -re ".*Make dprintf pending.*y or \\\[n\\\]. $" {
+	    gdb_test "y" "Dprintf.*pendfunc1.*pending." "set pending dprintf"
+     }
+}
+
+gdb_test "info break" \
+    "Num     Type\[ \]+Disp Enb Address\[ \]+What.*
+\[0-9\]+\[\t \]+dprintf.*keep y.*PENDING.*pendfunc1.*" \
+"single pending dprintf info (without symbols)"
+
+gdb_run_cmd
+
+gdb_test "" ".*x=3.*x=4.*x=3.*" "run to resolved dprintf"
--- /dev/null
+++ b/gdb/testsuite/gdb.base/dprintf-pendshr.c
@@ -0,0 +1,28 @@
+/* This testcase is part of GDB, the GNU debugger.
+
+   Copyright 2013 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>
+
+void pendfunc1 (int x)
+{
+  int y = x + 4;
+}
+
+void pendfunc (int x)
+{
+  pendfunc1 (x);
+}

Re: [PATCH] Fix dprintf work not right if it is pending

[Keith Seitz]

On 03/29/2013 12:42 AM, Hui Zhu wrote:

>>> +  breakpoint_re_set_default (b);
>>> +
>>> +  if (b->extra_string != NULL)
>>> +    update_dprintf_command_list (b);
>>> +}
>>> +
>>
>>
>> This will update the command list every time breakpoints are reset and could
>> be limited to only those needing updating. Is there perhaps a reason to
>> always do this?
>
> I think it need, because it need to generate different commands with
> different status for example:
>        if (target_can_run_breakpoint_commands ())
> 	printf_line = xstrprintf ("agent-printf %s", dprintf_args);
>

I'm not understanding this example. How is this likely to change 
whenever breakpoints are reset? Is there perhaps a way to add a test to 
demonstrate this requirement?

Otherwise, this patch looks good to me, and recommend that a maintainer 
give this a final review.

Keith

Re: [PATCH] Fix dprintf work not right if it is pending

[Hui Zhu]

[-- Attachment #1: Type: text/plain, Size: 1323 bytes --]

On Wed, Apr 3, 2013 at 2:05 AM, Keith Seitz <keiths@redhat.com> wrote:
> On 03/29/2013 12:42 AM, Hui Zhu wrote:
>
>>>> +  breakpoint_re_set_default (b);
>>>> +
>>>> +  if (b->extra_string != NULL)
>>>> +    update_dprintf_command_list (b);
>>>> +}
>>>> +
>>>
>>>
>>>
>>> This will update the command list every time breakpoints are reset and
>>> could
>>> be limited to only those needing updating. Is there perhaps a reason to
>>> always do this?
>>
>>
>> I think it need, because it need to generate different commands with
>> different status for example:
>>        if (target_can_run_breakpoint_commands ())
>>         printf_line = xstrprintf ("agent-printf %s", dprintf_args);
>>
>
> I'm not understanding this example. How is this likely to change whenever
> breakpoints are reset? Is there perhaps a way to add a test to demonstrate
> this requirement?

If the target that GDB connect to is changed  (for example, we set
dprintf and used it with local host target.  Then we changed to use a
remote target), GDB need set different commands of dprintfs.
The code that I post is from function update_dprintf_command_list.

I updated dprintf-pending.exp for this test.

>
> Otherwise, this patch looks good to me, and recommend that a maintainer give
> this a final review.
>

Thanks for your help.

> Keith

Best,
Hui

[-- Attachment #2: dprintf-pending.txt --]
[-- Type: text/plain, Size: 735 bytes --]

--- a/gdb/breakpoint.c
+++ b/gdb/breakpoint.c
@@ -12963,6 +12963,17 @@ bkpt_re_set (struct breakpoint *b)
   breakpoint_re_set_default (b);
 }
 
+/* Dprintf breakpoint_ops methods.  */
+
+static void
+dprintf_re_set (struct breakpoint *b)
+{
+  breakpoint_re_set_default (b);
+
+  if (b->extra_string != NULL)
+    update_dprintf_command_list (b);
+}
+
 static int
 bkpt_insert_location (struct bp_location *bl)
 {
@@ -16001,7 +16012,7 @@ initialize_breakpoint_ops (void)
 
   ops = &dprintf_breakpoint_ops;
   *ops = bkpt_base_breakpoint_ops;
-  ops->re_set = bkpt_re_set;
+  ops->re_set = dprintf_re_set;
   ops->resources_needed = bkpt_resources_needed;
   ops->print_it = bkpt_print_it;
   ops->print_mention = bkpt_print_mention;

[-- Attachment #3: dprintf-pending-test.txt --]
[-- Type: text/plain, Size: 6130 bytes --]

--- a/gdb/testsuite/gdb.base/Makefile.in
+++ b/gdb/testsuite/gdb.base/Makefile.in
@@ -10,7 +10,8 @@ EXECUTABLES = a2-run advance all-types a
 	call-strs callexit callfuncs callfwmall charset checkpoint \
 	chng-syms code_elim1 code_elim2 commands compiler complex \
 	condbreak consecutive constvars coremaker cursal cvexpr \
-	dbx-test del disasm-end-cu display dump dup-sect dup-sect.debug \
+	dbx-test del disasm-end-cu display dprintf-pending dump dup-sect \
+	dup-sect.debug \
 	dup-sect.stripped ending-run execd-prog expand-psymtabs exprs \
 	fileio find finish fixsection float foll-exec foll-fork foll-vfork \
 	frame-args freebpcmd fullname funcargs gcore \
@@ -44,7 +45,7 @@ EXECUTABLES = a2-run advance all-types a
 	wchar whatis whatis-exp catch-syscall \
 	pr10179 gnu_vector
 
-MISCELLANEOUS = coremmap.data ../foobar.baz fixsectshr.sl \
+MISCELLANEOUS = coremmap.data dprintf-pendshr.sl ../foobar.baz fixsectshr.sl \
 	pendshr.sl shreloc1.sl shreloc2.sl twice-tmp.c \
 	shr1.sl shr2.sl solib_sl.sl solib1.sl solib2.sl \
 	unloadshr.sl unloadshr2.sl watchpoint-solib-shr.sl \
--- /dev/null
+++ b/gdb/testsuite/gdb.base/dprintf-pending.c
@@ -0,0 +1,31 @@
+/* This testcase is part of GDB, the GNU debugger.
+
+   Copyright 2013 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>
+
+int k = 0;
+
+extern void pendfunc (int x);
+
+int main()
+{
+  pendfunc (3); /* break main here */
+  pendfunc (4);
+  k = 1;
+  pendfunc (3);
+  return 0;
+}
--- /dev/null
+++ b/gdb/testsuite/gdb.base/dprintf-pending.exp
@@ -0,0 +1,104 @@
+#   Copyright 2013 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 gdbserver-support.exp
+
+if {[skip_shlib_tests]} {
+    return 0
+}
+
+set testfile "dprintf-pending"
+set libfile "dprintf-pendshr"
+set srcfile $testfile.c
+set libsrc  $srcdir/$subdir/$libfile.c
+set binfile $objdir/$subdir/$testfile
+set lib_sl  $objdir/$subdir/$libfile.sl
+
+set lib_opts  debug
+set exec_opts [list debug shlib=$lib_sl]
+
+if { [get_compiler_info] } {
+    return -1
+}
+
+if { [gdb_compile_shlib $libsrc $lib_sl $lib_opts] != ""
+     || [gdb_compile $srcdir/$subdir/$srcfile $binfile executable $exec_opts] != ""} {
+    untested "Could not compile either $libsrc or $srcdir/$subdir/$srcfile."
+    return -1
+}
+
+# Start with a fresh gdb.
+
+gdb_exit
+gdb_start
+gdb_reinitialize_dir $srcdir/$subdir
+
+gdb_test_multiple "dprintf pendfunc1, \"x=%d\\n\", x" "set pending dprintf" {
+     -re ".*Make dprintf pending.*y or \\\[n\\\]. $" {
+	    gdb_test "y" "Dprintf.*pendfunc1.*pending." "set pending dprintf (without symbols)"
+     }
+}
+
+gdb_test "info break" \
+    "Num     Type\[ \]+Disp Enb Address\[ \]+What.*
+\[0-9\]+\[\t \]+dprintf.*keep y.*PENDING.*pendfunc1.*" \
+"single pending dprintf info (without symbols)"
+
+gdb_load ${binfile}
+gdb_load_shlibs $lib_sl
+
+gdb_run_cmd
+
+gdb_test "" ".*x=3.*x=4.*x=3.*" "run to resolved dprintf (without symbols)"
+
+# Restart with a fresh gdb.
+
+gdb_exit
+gdb_start
+gdb_reinitialize_dir $srcdir/$subdir
+
+gdb_load ${binfile}
+gdb_load_shlibs $lib_sl
+
+#
+# Test setting, querying, and modifying pending breakpoints
+#
+
+gdb_test_multiple "dprintf pendfunc1, \"x=%d\\n\", x" "set pending dprintf" {
+     -re ".*Make dprintf pending.*y or \\\[n\\\]. $" {
+	    gdb_test "y" "Dprintf.*pendfunc1.*pending." "set pending dprintf"
+     }
+}
+
+gdb_test "info break" \
+    "Num     Type\[ \]+Disp Enb Address\[ \]+What.*
+\[0-9\]+\[\t \]+dprintf.*keep y.*PENDING.*pendfunc1.*" \
+"single pending dprintf info"
+
+gdb_run_cmd
+
+gdb_test "" ".*x=3.*x=4.*x=3.*" "run to resolved dprintf"
+
+if { [skip_gdbserver_tests] } {
+    return 0
+}
+
+# Get warning or no output is OK.
+gdb_test "set dprintf-style agent" ".*" "Set dprintf style to agent"
+
+gdbserver_run ""
+
+gdb_test "info break" ".*agent-printf \"x=%d\\\\n\", x" \
+"single pending dprintf info with agent-printf"
--- /dev/null
+++ b/gdb/testsuite/gdb.base/dprintf-pendshr.c
@@ -0,0 +1,28 @@
+/* This testcase is part of GDB, the GNU debugger.
+
+   Copyright 2013 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>
+
+void pendfunc1 (int x)
+{
+  int y = x + 4;
+}
+
+void pendfunc (int x)
+{
+  pendfunc1 (x);
+}

Re: [PATCH] Fix dprintf work not right if it is pending

[Pedro Alves]

Hello,

Thanks for the help Keith.  Much appreciated.

On 04/02/2013 07:05 PM, Keith Seitz wrote:
> On 03/29/2013 12:42 AM, Hui Zhu wrote:
> 
>>>> +  breakpoint_re_set_default (b);
>>>> +
>>>> +  if (b->extra_string != NULL)
>>>> +    update_dprintf_command_list (b);

You shouldn't be able to create a dprintf without an
extra string, right?  But, we can't get to the extra string
until the breakpoint's location is pending, so we couldn't
check when the breakpoint was created.

$ ./gdb -q -nx
(gdb) dprintf pendfunc
No symbol table is loaded.  Use the "file" command.
Make dprintf pending on future shared library load? (y or [n]) y
Dprintf 1 (pendfunc) pending.
(gdb) info breakpoints
Num     Type           Disp Enb Address    What
1       dprintf        keep y   <PENDING>  pendfunc
(gdb)

Ok, now let's load symbols.

(gdb) file ./testsuite/gdb.base/dprintf-pending
Reading symbols from /home/pedro/gdb/mygit/build/gdb/testsuite/gdb.base/dprintf-pending...done.

and:

(gdb) info breakpoints
Num     Type           Disp Enb Address            What
1       dprintf        keep y   0x0000000000400560 <pendfunc@plt>

the location resolved.  But, notice no commands attached...

(gdb) start
Temporary breakpoint 2 at 0x400690: file ../../../src/gdb/testsuite/gdb.base/dprintf-pending.c, line 26.
Starting program: /home/pedro/gdb/mygit/build/gdb/testsuite/gdb.base/dprintf-pending
Temporary breakpoint 2, main () at ../../../src/gdb/testsuite/gdb.base/dprintf-pending.c:26
26        pendfunc (3); /* break main here */
(gdb) n
(gdb) info breakpoints
Num     Type           Disp Enb Address            What
1       dprintf        keep y   0x00007ffff7dfc69d in pendfunc at ../../../src/gdb/testsuite/gdb.base/dprintf-pendshr.c:27
        breakpoint already hit 1 time
(gdb)

I think we want this:

static void
dprintf_re_set (struct breakpoint *b)
{
  breakpoint_re_set_default (b);

  /* This breakpoint could have been pending, and be resolved now, and
     if so, we should now have the extra string.  If we don't, the
     dprintf was malformed when created, but we couldn't tell because
     we can't extract the extra string until the location is
     resolved.  */
  if (b->loc != NULL && b->extra_string == NULL)
    error (_("Format string required"));

  if (b->extra_string != NULL)
    update_dprintf_command_list (b);
}

Please add a test for this.

>>>> +}
>>>> +
>>>
>>>
>>> This will update the command list every time breakpoints are reset and could
>>> be limited to only those needing updating. Is there perhaps a reason to
>>> always do this?

You mean, only update the command list if there isn't one before
(because the breakpoint was pending before) ?

>>
>> I think it need, because it need to generate different commands with
>> different status for example:
>>        if (target_can_run_breakpoint_commands ())
>>     printf_line = xstrprintf ("agent-printf %s", dprintf_args);
>>
> 
> I'm not understanding this example. How is this likely to change whenever breakpoints are reset? Is there perhaps a way to add a test to demonstrate this requirement?

I think what he's saying is, even independently of issues with
pending dprintf breakpoints, if you, in the same gdb run:

1 - connect to target 1, that can run breakpoint commands.
2 - create a dprintf, which resolves fine.
3 - disconnect from target 2
4 - connect to target 2, that can NOT run breakpoint commands.

After steps #3/#4, you'll want the dprintf command list to
be updated, because target 1 and 2 may well return different
answers for target_can_run_breakpoint_commands().
Given absence of finer grained resetting, we get to do
it all the time.

On 04/04/2013 02:29 PM, Hui Zhu wrote:
> +# Restart with a fresh gdb.
> +
> +gdb_exit
> +gdb_start
> +gdb_reinitialize_dir $srcdir/$subdir
> +
> +gdb_load ${binfile}

Use clean_restart here.

> +gdb_test_multiple "dprintf pendfunc1, \"x=%d\\n\", x" "set pending dprintf" {
> +     -re ".*Make dprintf pending.*y or \\\[n\\\]. $" {
> +	    gdb_test "y" "Dprintf.*pendfunc1.*pending." "set pending dprintf"
> +     }
> +}
> +

gdb_test has built-in support for questions.  Write these sorts
of things as:

gdb_test \
    "dprintf pendfunc1, \"x=%d\\n\", x" \
    "Dprintf.*pendfunc1.*pending." \
    "set pending dprintf (without symbols)" \
    ".*Make dprintf pending.*y or \\\[n\\\]. $" \
    "y"

There's at least one more instance.

> +if { [skip_gdbserver_tests] } {
> +    return 0
> +}
> +
> +# Get warning or no output is OK.
> +gdb_test "set dprintf-style agent" ".*" "Set dprintf style to agent"
> +
> +gdbserver_run ""

I'd much prefer remove this skip_gdbserver_tests check, and this
gdbserver_run.  IOW, keep running the test against the target
the current board is set up with.  There are remote servers other
than GDBserver out there.

> +# Get warning or no output is OK.
> +gdb_test "set dprintf-style agent" ".*" "Set dprintf style to agent"

What warning would that be?  This here?:

  else if (strcmp (dprintf_style, dprintf_style_agent) == 0)
    {
      if (target_can_run_breakpoint_commands ())
	printf_line = xstrprintf ("agent-printf %s", dprintf_args);
      else
	{
	  warning (_("Target cannot run dprintf commands, falling back to GDB printf"));
	  printf_line = xstrprintf ("printf %s", dprintf_args);
	}
    }

> +
> +gdbserver_run ""
> +
> +gdb_test "info break" ".*agent-printf \"x=%d\\\\n\", x" \

If that warning triggers, then this will fail...  In fact,
you should see that when you remove the gdbserver bits.

Please make the "set" test check explicitly either no output, or the
warning, and then the "info break" test check the corresponding expected
output.  Then please make sure the test passes with native, and
also the native-gdbserver and native-extended-gdbserver boards.
It fails with the native-gdbserver board, because the program
exists and gdbserver exits before the "set dprintf-style agent".
You'll need to add something to prevent that.

-- 
Pedro Alves

Re: [PATCH] Fix dprintf work not right if it is pending

[Keith Seitz]

On 04/05/2013 09:06 AM, Pedro Alves wrote:
> Thanks for the help Keith.  Much appreciated.

I'm not exactly sure I would call this "helping." I think I created more 
work than I actually saved. My apologies for that. That certainly wasn't 
my intent.

> I think we want this:
>
> static void
> dprintf_re_set (struct breakpoint *b)
> {
>    breakpoint_re_set_default (b);
>
>    /* This breakpoint could have been pending, and be resolved now, and
>       if so, we should now have the extra string.  If we don't, the
>       dprintf was malformed when created, but we couldn't tell because
>       we can't extract the extra string until the location is
>       resolved.  */
>    if (b->loc != NULL && b->extra_string == NULL)
>      error (_("Format string required"));
>
>    if (b->extra_string != NULL)
>      update_dprintf_command_list (b);
> }

Yeah, that's good. Me likey. That was more like what I have in my sandbox.

> You mean, only update the command list if there isn't one before
> (because the breakpoint was pending before) ?

Yeah, I did mean that. Thank you to both you and Hui for clarifying why 
this is necessary.

Thank you for your review of my review, too!

Keith

Re: [PATCH] Fix dprintf work not right if it is pending

[Pedro Alves]

On 04/06/2013 10:45 PM, Keith Seitz wrote:
> On 04/05/2013 09:06 AM, Pedro Alves wrote:
>> Thanks for the help Keith.  Much appreciated.
> 
> I'm not exactly sure I would call this "helping." I think I created more work than I actually saved. My apologies for that. That certainly wasn't my intent.

You're _really_ too hard on yourself.  :-)  Pushing for moving this
to the breakpoint_ops->re_set hook was very helpful.

-- 
Pedro Alves

Re: [PATCH] Fix dprintf work not right if it is pending

[Hui Zhu]

[-- Attachment #1: Type: text/plain, Size: 7236 bytes --]

Hi Pedro,

Thanks for your help.

On Sat, Apr 6, 2013 at 12:06 AM, Pedro Alves <palves@redhat.com> wrote:
> Hello,
>
> Thanks for the help Keith.  Much appreciated.
>
> On 04/02/2013 07:05 PM, Keith Seitz wrote:
>> On 03/29/2013 12:42 AM, Hui Zhu wrote:
>>
>>>>> +  breakpoint_re_set_default (b);
>>>>> +
>>>>> +  if (b->extra_string != NULL)
>>>>> +    update_dprintf_command_list (b);
>
> You shouldn't be able to create a dprintf without an
> extra string, right?  But, we can't get to the extra string
> until the breakpoint's location is pending, so we couldn't
> check when the breakpoint was created.
>
> $ ./gdb -q -nx
> (gdb) dprintf pendfunc
> No symbol table is loaded.  Use the "file" command.
> Make dprintf pending on future shared library load? (y or [n]) y
> Dprintf 1 (pendfunc) pending.
> (gdb) info breakpoints
> Num     Type           Disp Enb Address    What
> 1       dprintf        keep y   <PENDING>  pendfunc
> (gdb)
>
> Ok, now let's load symbols.
>
> (gdb) file ./testsuite/gdb.base/dprintf-pending
> Reading symbols from /home/pedro/gdb/mygit/build/gdb/testsuite/gdb.base/dprintf-pending...done.
>
> and:
>
> (gdb) info breakpoints
> Num     Type           Disp Enb Address            What
> 1       dprintf        keep y   0x0000000000400560 <pendfunc@plt>
>
> the location resolved.  But, notice no commands attached...
>
> (gdb) start
> Temporary breakpoint 2 at 0x400690: file ../../../src/gdb/testsuite/gdb.base/dprintf-pending.c, line 26.
> Starting program: /home/pedro/gdb/mygit/build/gdb/testsuite/gdb.base/dprintf-pending
> Temporary breakpoint 2, main () at ../../../src/gdb/testsuite/gdb.base/dprintf-pending.c:26
> 26        pendfunc (3); /* break main here */
> (gdb) n
> (gdb) info breakpoints
> Num     Type           Disp Enb Address            What
> 1       dprintf        keep y   0x00007ffff7dfc69d in pendfunc at ../../../src/gdb/testsuite/gdb.base/dprintf-pendshr.c:27
>         breakpoint already hit 1 time
> (gdb)
>
> I think we want this:
>
> static void
> dprintf_re_set (struct breakpoint *b)
> {
>   breakpoint_re_set_default (b);
>
>   /* This breakpoint could have been pending, and be resolved now, and
>      if so, we should now have the extra string.  If we don't, the
>      dprintf was malformed when created, but we couldn't tell because
>      we can't extract the extra string until the location is
>      resolved.  */
>   if (b->loc != NULL && b->extra_string == NULL)
>     error (_("Format string required"));
>
>   if (b->extra_string != NULL)
>     update_dprintf_command_list (b);
> }
>
> Please add a test for this.

Fixed and updated test for it.

>
>>>>> +}
>>>>> +
>>>>
>>>>
>>>> This will update the command list every time breakpoints are reset and could
>>>> be limited to only those needing updating. Is there perhaps a reason to
>>>> always do this?
>
> You mean, only update the command list if there isn't one before
> (because the breakpoint was pending before) ?
>
>>>
>>> I think it need, because it need to generate different commands with
>>> different status for example:
>>>        if (target_can_run_breakpoint_commands ())
>>>     printf_line = xstrprintf ("agent-printf %s", dprintf_args);
>>>
>>
>> I'm not understanding this example. How is this likely to change whenever breakpoints are reset? Is there perhaps a way to add a test to demonstrate this requirement?
>
> I think what he's saying is, even independently of issues with
> pending dprintf breakpoints, if you, in the same gdb run:
>
> 1 - connect to target 1, that can run breakpoint commands.
> 2 - create a dprintf, which resolves fine.
> 3 - disconnect from target 2
> 4 - connect to target 2, that can NOT run breakpoint commands.
>
> After steps #3/#4, you'll want the dprintf command list to
> be updated, because target 1 and 2 may well return different
> answers for target_can_run_breakpoint_commands().
> Given absence of finer grained resetting, we get to do
> it all the time.

Thanks.  This part is so clear that I added it as comments of this part of code.

>
> On 04/04/2013 02:29 PM, Hui Zhu wrote:
>> +# Restart with a fresh gdb.
>> +
>> +gdb_exit
>> +gdb_start
>> +gdb_reinitialize_dir $srcdir/$subdir
>> +
>> +gdb_load ${binfile}
>
> Use clean_restart here.
>
>> +gdb_test_multiple "dprintf pendfunc1, \"x=%d\\n\", x" "set pending dprintf" {
>> +     -re ".*Make dprintf pending.*y or \\\[n\\\]. $" {
>> +         gdb_test "y" "Dprintf.*pendfunc1.*pending." "set pending dprintf"
>> +     }
>> +}
>> +
>
> gdb_test has built-in support for questions.  Write these sorts
> of things as:
>
> gdb_test \
>     "dprintf pendfunc1, \"x=%d\\n\", x" \
>     "Dprintf.*pendfunc1.*pending." \
>     "set pending dprintf (without symbols)" \
>     ".*Make dprintf pending.*y or \\\[n\\\]. $" \
>     "y"
>
> There's at least one more instance.
>
>> +if { [skip_gdbserver_tests] } {
>> +    return 0
>> +}
>> +
>> +# Get warning or no output is OK.
>> +gdb_test "set dprintf-style agent" ".*" "Set dprintf style to agent"
>> +
>> +gdbserver_run ""
>
> I'd much prefer remove this skip_gdbserver_tests check, and this
> gdbserver_run.  IOW, keep running the test against the target
> the current board is set up with.  There are remote servers other
> than GDBserver out there.

The code after skip_gdbserver_tests check is to test:
  if (b->extra_string != NULL)
    update_dprintf_command_list (b);
My thought is change the target to show "printf" is changed to "agent-printf".
Now I removed all this part of code.


>
>> +# Get warning or no output is OK.
>> +gdb_test "set dprintf-style agent" ".*" "Set dprintf style to agent"
>
> What warning would that be?  This here?:
>
>   else if (strcmp (dprintf_style, dprintf_style_agent) == 0)
>     {
>       if (target_can_run_breakpoint_commands ())
>         printf_line = xstrprintf ("agent-printf %s", dprintf_args);
>       else
>         {
>           warning (_("Target cannot run dprintf commands, falling back to GDB printf"));
>           printf_line = xstrprintf ("printf %s", dprintf_args);
>         }
>     }
>
>> +
>> +gdbserver_run ""
>> +
>> +gdb_test "info break" ".*agent-printf \"x=%d\\\\n\", x" \
>
> If that warning triggers, then this will fail...  In fact,
> you should see that when you remove the gdbserver bits.
>
> Please make the "set" test check explicitly either no output, or the
> warning, and then the "info break" test check the corresponding expected
> output.  Then please make sure the test passes with native, and
> also the native-gdbserver and native-extended-gdbserver boards.
> It fails with the native-gdbserver board, because the program
> exists and gdbserver exits before the "set dprintf-style agent".
> You'll need to add something to prevent that.
>
> --
> Pedro Alves
>

I post new patches accord to your commnets.  Please help me review them.

Best,
Hui

2013-04-07  Pedro Alves  <palves@redhat.com>
   Hui Zhu  <hui@codesourcery.com>

* breakpoint.c (dprintf_re_set): New.
(initialize_breakpoint_ops): Set dprintf_breakpoint_ops re_set
to dprintf_re_set.

2013-04-07  Hui Zhu  <hui@codesourcery.com>

* gdb.base/Makefile.in (EXECUTABLES): Add dprintf-pending.
(MISCELLANEOUS): Add dprintf-pendshr.sl.
* gdb.base/dprintf-pending.c, gdb.base/dprintf-pending.exp: New.

[-- Attachment #2: dprintf-pending.txt --]
[-- Type: text/plain, Size: 1618 bytes --]

--- a/gdb/breakpoint.c
+++ b/gdb/breakpoint.c
@@ -12963,6 +12963,35 @@ bkpt_re_set (struct breakpoint *b)
   breakpoint_re_set_default (b);
 }
 
+/* Dprintf breakpoint_ops methods.  */
+
+static void
+dprintf_re_set (struct breakpoint *b)
+{
+  breakpoint_re_set_default (b);
+
+  /* This breakpoint could have been pending, and be resolved now, and
+     if so, we should now have the extra string.  If we don't, the
+     dprintf was malformed when created, but we couldn't tell because
+     we can't extract the extra string until the location is
+     resolved.  */
+  if (b->loc != NULL && b->extra_string == NULL)
+    error (_("Format string required"));
+
+  /* 1 - connect to target 1, that can run breakpoint commands.
+     2 - create a dprintf, which resolves fine.
+     3 - disconnect from target 2
+     4 - connect to target 2, that can NOT run breakpoint commands.
+
+     After steps #3/#4, you'll want the dprintf command list to
+     be updated, because target 1 and 2 may well return different
+     answers for target_can_run_breakpoint_commands().
+     Given absence of finer grained resetting, we get to do
+     it all the time.  */
+  if (b->extra_string != NULL)
+    update_dprintf_command_list (b);
+}
+
 static int
 bkpt_insert_location (struct bp_location *bl)
 {
@@ -16001,7 +16030,7 @@ initialize_breakpoint_ops (void)
 
   ops = &dprintf_breakpoint_ops;
   *ops = bkpt_base_breakpoint_ops;
-  ops->re_set = bkpt_re_set;
+  ops->re_set = dprintf_re_set;
   ops->resources_needed = bkpt_resources_needed;
   ops->print_it = bkpt_print_it;
   ops->print_mention = bkpt_print_mention;

[-- Attachment #3: dprintf-pending-test.txt --]
[-- Type: text/plain, Size: 6003 bytes --]

--- a/gdb/testsuite/gdb.base/Makefile.in
+++ b/gdb/testsuite/gdb.base/Makefile.in
@@ -10,7 +10,8 @@ EXECUTABLES = a2-run advance all-types a
 	call-strs callexit callfuncs callfwmall charset checkpoint \
 	chng-syms code_elim1 code_elim2 commands compiler complex \
 	condbreak consecutive constvars coremaker cursal cvexpr \
-	dbx-test del disasm-end-cu display dump dup-sect dup-sect.debug \
+	dbx-test del disasm-end-cu display dprintf-pending dump dup-sect \
+	dup-sect.debug \
 	dup-sect.stripped ending-run execd-prog expand-psymtabs exprs \
 	fileio find finish fixsection float foll-exec foll-fork foll-vfork \
 	frame-args freebpcmd fullname funcargs gcore \
@@ -44,7 +45,7 @@ EXECUTABLES = a2-run advance all-types a
 	wchar whatis whatis-exp catch-syscall \
 	pr10179 gnu_vector
 
-MISCELLANEOUS = coremmap.data ../foobar.baz fixsectshr.sl \
+MISCELLANEOUS = coremmap.data dprintf-pendshr.sl ../foobar.baz fixsectshr.sl \
 	pendshr.sl shreloc1.sl shreloc2.sl twice-tmp.c \
 	shr1.sl shr2.sl solib_sl.sl solib1.sl solib2.sl \
 	unloadshr.sl unloadshr2.sl watchpoint-solib-shr.sl \
--- /dev/null
+++ b/gdb/testsuite/gdb.base/dprintf-pending.c
@@ -0,0 +1,31 @@
+/* This testcase is part of GDB, the GNU debugger.
+
+   Copyright 2013 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>
+
+int k = 0;
+
+extern void pendfunc (int x);
+
+int main()
+{
+  pendfunc (3); /* break main here */
+  pendfunc (4);
+  k = 1;
+  pendfunc (3);
+  return 0;
+}
--- /dev/null
+++ b/gdb/testsuite/gdb.base/dprintf-pending.exp
@@ -0,0 +1,99 @@
+#   Copyright 2013 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 gdbserver-support.exp
+
+if {[skip_shlib_tests]} {
+    return 0
+}
+
+set testfile "dprintf-pending"
+set libfile "dprintf-pendshr"
+set srcfile $testfile.c
+set libsrc  $srcdir/$subdir/$libfile.c
+set binfile $objdir/$subdir/$testfile
+set lib_sl  $objdir/$subdir/$libfile.sl
+
+set lib_opts  debug
+set exec_opts [list debug shlib=$lib_sl]
+
+if { [get_compiler_info] } {
+    return -1
+}
+
+if { [gdb_compile_shlib $libsrc $lib_sl $lib_opts] != ""
+     || [gdb_compile $srcdir/$subdir/$srcfile $binfile executable $exec_opts] != ""} {
+    untested "Could not compile either $libsrc or $srcdir/$subdir/$srcfile."
+    return -1
+}
+
+gdb_exit
+gdb_start
+gdb_reinitialize_dir $srcdir/$subdir
+
+gdb_test \
+    "dprintf pendfunc" \
+    "Dprintf.*pendfunc.*pending." \
+    "set pending dprintf (without format)" \
+    ".*Make dprintf pending.*y or \\\[n\\\]. $" \
+    "y"
+
+gdb_test "file ${binfile}" ".*Error in re-setting breakpoint.*" "load symbols get without format error"
+
+gdb_exit
+gdb_start
+gdb_reinitialize_dir $srcdir/$subdir
+
+gdb_test \
+    "dprintf pendfunc1, \"x=%d\\n\", x" \
+    "Dprintf.*pendfunc1.*pending." \
+    "set pending dprintf (without symbols)" \
+    ".*Make dprintf pending.*y or \\\[n\\\]. $" \
+    "y"
+
+gdb_test "info break" \
+    "Num     Type\[ \]+Disp Enb Address\[ \]+What.*
+\[0-9\]+\[\t \]+dprintf.*keep y.*PENDING.*pendfunc1.*" \
+"single pending dprintf info (without symbols)"
+
+gdb_load ${binfile}
+gdb_load_shlibs $lib_sl
+
+gdb_run_cmd
+
+gdb_test "" ".*x=3.*x=4.*x=3.*" "run to resolved dprintf (without symbols)"
+
+clean_restart ${binfile}
+gdb_load_shlibs $lib_sl
+
+#
+# Test setting, querying, and modifying pending breakpoints
+#
+
+gdb_test \
+    "dprintf pendfunc1, \"x=%d\\n\", x" \
+    "Dprintf.*pendfunc1.*pending." \
+    "set pending dprintf" \
+    ".*Make dprintf pending.*y or \\\[n\\\]. $" \
+    "y"
+
+gdb_test "info break" \
+    "Num     Type\[ \]+Disp Enb Address\[ \]+What.*
+\[0-9\]+\[\t \]+dprintf.*keep y.*PENDING.*pendfunc1.*" \
+"single pending dprintf info"
+
+gdb_run_cmd
+
+gdb_test "" ".*x=3.*x=4.*x=3.*" "run to resolved dprintf"
--- /dev/null
+++ b/gdb/testsuite/gdb.base/dprintf-pendshr.c
@@ -0,0 +1,28 @@
+/* This testcase is part of GDB, the GNU debugger.
+
+   Copyright 2013 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>
+
+void pendfunc1 (int x)
+{
+  int y = x + 4;
+}
+
+void pendfunc (int x)
+{
+  pendfunc1 (x);
+}

Re: [PATCH] Fix dprintf work not right if it is pending

[Hui Zhu]

[-- Attachment #1: Type: text/plain, Size: 7733 bytes --]

Hi,

I removed "#include <stdio.h>" from test code dprintf-pendshr.c and
dprintf-pending.c according to the remind from Yao in IRC.

The attachments is the new version.

Thanks,
Hui

On Sun, Apr 7, 2013 at 10:27 PM, Hui Zhu <teawater@gmail.com> wrote:
> Hi Pedro,
>
> Thanks for your help.
>
> On Sat, Apr 6, 2013 at 12:06 AM, Pedro Alves <palves@redhat.com> wrote:
>> Hello,
>>
>> Thanks for the help Keith.  Much appreciated.
>>
>> On 04/02/2013 07:05 PM, Keith Seitz wrote:
>>> On 03/29/2013 12:42 AM, Hui Zhu wrote:
>>>
>>>>>> +  breakpoint_re_set_default (b);
>>>>>> +
>>>>>> +  if (b->extra_string != NULL)
>>>>>> +    update_dprintf_command_list (b);
>>
>> You shouldn't be able to create a dprintf without an
>> extra string, right?  But, we can't get to the extra string
>> until the breakpoint's location is pending, so we couldn't
>> check when the breakpoint was created.
>>
>> $ ./gdb -q -nx
>> (gdb) dprintf pendfunc
>> No symbol table is loaded.  Use the "file" command.
>> Make dprintf pending on future shared library load? (y or [n]) y
>> Dprintf 1 (pendfunc) pending.
>> (gdb) info breakpoints
>> Num     Type           Disp Enb Address    What
>> 1       dprintf        keep y   <PENDING>  pendfunc
>> (gdb)
>>
>> Ok, now let's load symbols.
>>
>> (gdb) file ./testsuite/gdb.base/dprintf-pending
>> Reading symbols from /home/pedro/gdb/mygit/build/gdb/testsuite/gdb.base/dprintf-pending...done.
>>
>> and:
>>
>> (gdb) info breakpoints
>> Num     Type           Disp Enb Address            What
>> 1       dprintf        keep y   0x0000000000400560 <pendfunc@plt>
>>
>> the location resolved.  But, notice no commands attached...
>>
>> (gdb) start
>> Temporary breakpoint 2 at 0x400690: file ../../../src/gdb/testsuite/gdb.base/dprintf-pending.c, line 26.
>> Starting program: /home/pedro/gdb/mygit/build/gdb/testsuite/gdb.base/dprintf-pending
>> Temporary breakpoint 2, main () at ../../../src/gdb/testsuite/gdb.base/dprintf-pending.c:26
>> 26        pendfunc (3); /* break main here */
>> (gdb) n
>> (gdb) info breakpoints
>> Num     Type           Disp Enb Address            What
>> 1       dprintf        keep y   0x00007ffff7dfc69d in pendfunc at ../../../src/gdb/testsuite/gdb.base/dprintf-pendshr.c:27
>>         breakpoint already hit 1 time
>> (gdb)
>>
>> I think we want this:
>>
>> static void
>> dprintf_re_set (struct breakpoint *b)
>> {
>>   breakpoint_re_set_default (b);
>>
>>   /* This breakpoint could have been pending, and be resolved now, and
>>      if so, we should now have the extra string.  If we don't, the
>>      dprintf was malformed when created, but we couldn't tell because
>>      we can't extract the extra string until the location is
>>      resolved.  */
>>   if (b->loc != NULL && b->extra_string == NULL)
>>     error (_("Format string required"));
>>
>>   if (b->extra_string != NULL)
>>     update_dprintf_command_list (b);
>> }
>>
>> Please add a test for this.
>
> Fixed and updated test for it.
>
>>
>>>>>> +}
>>>>>> +
>>>>>
>>>>>
>>>>> This will update the command list every time breakpoints are reset and could
>>>>> be limited to only those needing updating. Is there perhaps a reason to
>>>>> always do this?
>>
>> You mean, only update the command list if there isn't one before
>> (because the breakpoint was pending before) ?
>>
>>>>
>>>> I think it need, because it need to generate different commands with
>>>> different status for example:
>>>>        if (target_can_run_breakpoint_commands ())
>>>>     printf_line = xstrprintf ("agent-printf %s", dprintf_args);
>>>>
>>>
>>> I'm not understanding this example. How is this likely to change whenever breakpoints are reset? Is there perhaps a way to add a test to demonstrate this requirement?
>>
>> I think what he's saying is, even independently of issues with
>> pending dprintf breakpoints, if you, in the same gdb run:
>>
>> 1 - connect to target 1, that can run breakpoint commands.
>> 2 - create a dprintf, which resolves fine.
>> 3 - disconnect from target 2
>> 4 - connect to target 2, that can NOT run breakpoint commands.
>>
>> After steps #3/#4, you'll want the dprintf command list to
>> be updated, because target 1 and 2 may well return different
>> answers for target_can_run_breakpoint_commands().
>> Given absence of finer grained resetting, we get to do
>> it all the time.
>
> Thanks.  This part is so clear that I added it as comments of this part of code.
>
>>
>> On 04/04/2013 02:29 PM, Hui Zhu wrote:
>>> +# Restart with a fresh gdb.
>>> +
>>> +gdb_exit
>>> +gdb_start
>>> +gdb_reinitialize_dir $srcdir/$subdir
>>> +
>>> +gdb_load ${binfile}
>>
>> Use clean_restart here.
>>
>>> +gdb_test_multiple "dprintf pendfunc1, \"x=%d\\n\", x" "set pending dprintf" {
>>> +     -re ".*Make dprintf pending.*y or \\\[n\\\]. $" {
>>> +         gdb_test "y" "Dprintf.*pendfunc1.*pending." "set pending dprintf"
>>> +     }
>>> +}
>>> +
>>
>> gdb_test has built-in support for questions.  Write these sorts
>> of things as:
>>
>> gdb_test \
>>     "dprintf pendfunc1, \"x=%d\\n\", x" \
>>     "Dprintf.*pendfunc1.*pending." \
>>     "set pending dprintf (without symbols)" \
>>     ".*Make dprintf pending.*y or \\\[n\\\]. $" \
>>     "y"
>>
>> There's at least one more instance.
>>
>>> +if { [skip_gdbserver_tests] } {
>>> +    return 0
>>> +}
>>> +
>>> +# Get warning or no output is OK.
>>> +gdb_test "set dprintf-style agent" ".*" "Set dprintf style to agent"
>>> +
>>> +gdbserver_run ""
>>
>> I'd much prefer remove this skip_gdbserver_tests check, and this
>> gdbserver_run.  IOW, keep running the test against the target
>> the current board is set up with.  There are remote servers other
>> than GDBserver out there.
>
> The code after skip_gdbserver_tests check is to test:
>   if (b->extra_string != NULL)
>     update_dprintf_command_list (b);
> My thought is change the target to show "printf" is changed to "agent-printf".
> Now I removed all this part of code.
>
>
>>
>>> +# Get warning or no output is OK.
>>> +gdb_test "set dprintf-style agent" ".*" "Set dprintf style to agent"
>>
>> What warning would that be?  This here?:
>>
>>   else if (strcmp (dprintf_style, dprintf_style_agent) == 0)
>>     {
>>       if (target_can_run_breakpoint_commands ())
>>         printf_line = xstrprintf ("agent-printf %s", dprintf_args);
>>       else
>>         {
>>           warning (_("Target cannot run dprintf commands, falling back to GDB printf"));
>>           printf_line = xstrprintf ("printf %s", dprintf_args);
>>         }
>>     }
>>
>>> +
>>> +gdbserver_run ""
>>> +
>>> +gdb_test "info break" ".*agent-printf \"x=%d\\\\n\", x" \
>>
>> If that warning triggers, then this will fail...  In fact,
>> you should see that when you remove the gdbserver bits.
>>
>> Please make the "set" test check explicitly either no output, or the
>> warning, and then the "info break" test check the corresponding expected
>> output.  Then please make sure the test passes with native, and
>> also the native-gdbserver and native-extended-gdbserver boards.
>> It fails with the native-gdbserver board, because the program
>> exists and gdbserver exits before the "set dprintf-style agent".
>> You'll need to add something to prevent that.
>>
>> --
>> Pedro Alves
>>
>
> I post new patches accord to your commnets.  Please help me review them.
>
> Best,
> Hui
>
> 2013-04-07  Pedro Alves  <palves@redhat.com>
>    Hui Zhu  <hui@codesourcery.com>
>
> * breakpoint.c (dprintf_re_set): New.
> (initialize_breakpoint_ops): Set dprintf_breakpoint_ops re_set
> to dprintf_re_set.
>
> 2013-04-07  Hui Zhu  <hui@codesourcery.com>
>
> * gdb.base/Makefile.in (EXECUTABLES): Add dprintf-pending.
> (MISCELLANEOUS): Add dprintf-pendshr.sl.
> * gdb.base/dprintf-pending.c, gdb.base/dprintf-pending.exp: New.

[-- Attachment #2: dprintf-pending.txt --]
[-- Type: text/plain, Size: 1618 bytes --]

--- a/gdb/breakpoint.c
+++ b/gdb/breakpoint.c
@@ -12963,6 +12963,35 @@ bkpt_re_set (struct breakpoint *b)
   breakpoint_re_set_default (b);
 }
 
+/* Dprintf breakpoint_ops methods.  */
+
+static void
+dprintf_re_set (struct breakpoint *b)
+{
+  breakpoint_re_set_default (b);
+
+  /* This breakpoint could have been pending, and be resolved now, and
+     if so, we should now have the extra string.  If we don't, the
+     dprintf was malformed when created, but we couldn't tell because
+     we can't extract the extra string until the location is
+     resolved.  */
+  if (b->loc != NULL && b->extra_string == NULL)
+    error (_("Format string required"));
+
+  /* 1 - connect to target 1, that can run breakpoint commands.
+     2 - create a dprintf, which resolves fine.
+     3 - disconnect from target 2
+     4 - connect to target 2, that can NOT run breakpoint commands.
+
+     After steps #3/#4, you'll want the dprintf command list to
+     be updated, because target 1 and 2 may well return different
+     answers for target_can_run_breakpoint_commands().
+     Given absence of finer grained resetting, we get to do
+     it all the time.  */
+  if (b->extra_string != NULL)
+    update_dprintf_command_list (b);
+}
+
 static int
 bkpt_insert_location (struct bp_location *bl)
 {
@@ -16001,7 +16030,7 @@ initialize_breakpoint_ops (void)
 
   ops = &dprintf_breakpoint_ops;
   *ops = bkpt_base_breakpoint_ops;
-  ops->re_set = bkpt_re_set;
+  ops->re_set = dprintf_re_set;
   ops->resources_needed = bkpt_resources_needed;
   ops->print_it = bkpt_print_it;
   ops->print_mention = bkpt_print_mention;

[-- Attachment #3: dprintf-pending-test.txt --]
[-- Type: text/plain, Size: 5959 bytes --]

--- a/gdb/testsuite/gdb.base/Makefile.in
+++ b/gdb/testsuite/gdb.base/Makefile.in
@@ -10,7 +10,8 @@ EXECUTABLES = a2-run advance all-types a
 	call-strs callexit callfuncs callfwmall charset checkpoint \
 	chng-syms code_elim1 code_elim2 commands compiler complex \
 	condbreak consecutive constvars coremaker cursal cvexpr \
-	dbx-test del disasm-end-cu display dump dup-sect dup-sect.debug \
+	dbx-test del disasm-end-cu display dprintf-pending dump dup-sect \
+	dup-sect.debug \
 	dup-sect.stripped ending-run execd-prog expand-psymtabs exprs \
 	fileio find finish fixsection float foll-exec foll-fork foll-vfork \
 	frame-args freebpcmd fullname funcargs gcore \
@@ -44,7 +45,7 @@ EXECUTABLES = a2-run advance all-types a
 	wchar whatis whatis-exp catch-syscall \
 	pr10179 gnu_vector
 
-MISCELLANEOUS = coremmap.data ../foobar.baz fixsectshr.sl \
+MISCELLANEOUS = coremmap.data dprintf-pendshr.sl ../foobar.baz fixsectshr.sl \
 	pendshr.sl shreloc1.sl shreloc2.sl twice-tmp.c \
 	shr1.sl shr2.sl solib_sl.sl solib1.sl solib2.sl \
 	unloadshr.sl unloadshr2.sl watchpoint-solib-shr.sl \
--- /dev/null
+++ b/gdb/testsuite/gdb.base/dprintf-pending.c
@@ -0,0 +1,29 @@
+/* This testcase is part of GDB, the GNU debugger.
+
+   Copyright 2013 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/>.  */
+
+int k = 0;
+
+extern void pendfunc (int x);
+
+int main()
+{
+  pendfunc (3); /* break main here */
+  pendfunc (4);
+  k = 1;
+  pendfunc (3);
+  return 0;
+}
--- /dev/null
+++ b/gdb/testsuite/gdb.base/dprintf-pending.exp
@@ -0,0 +1,99 @@
+#   Copyright 2013 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 gdbserver-support.exp
+
+if {[skip_shlib_tests]} {
+    return 0
+}
+
+set testfile "dprintf-pending"
+set libfile "dprintf-pendshr"
+set srcfile $testfile.c
+set libsrc  $srcdir/$subdir/$libfile.c
+set binfile $objdir/$subdir/$testfile
+set lib_sl  $objdir/$subdir/$libfile.sl
+
+set lib_opts  debug
+set exec_opts [list debug shlib=$lib_sl]
+
+if { [get_compiler_info] } {
+    return -1
+}
+
+if { [gdb_compile_shlib $libsrc $lib_sl $lib_opts] != ""
+     || [gdb_compile $srcdir/$subdir/$srcfile $binfile executable $exec_opts] != ""} {
+    untested "Could not compile either $libsrc or $srcdir/$subdir/$srcfile."
+    return -1
+}
+
+gdb_exit
+gdb_start
+gdb_reinitialize_dir $srcdir/$subdir
+
+gdb_test \
+    "dprintf pendfunc" \
+    "Dprintf.*pendfunc.*pending." \
+    "set pending dprintf (without format)" \
+    ".*Make dprintf pending.*y or \\\[n\\\]. $" \
+    "y"
+
+gdb_test "file ${binfile}" ".*Error in re-setting breakpoint.*" "load symbols get without format error"
+
+gdb_exit
+gdb_start
+gdb_reinitialize_dir $srcdir/$subdir
+
+gdb_test \
+    "dprintf pendfunc1, \"x=%d\\n\", x" \
+    "Dprintf.*pendfunc1.*pending." \
+    "set pending dprintf (without symbols)" \
+    ".*Make dprintf pending.*y or \\\[n\\\]. $" \
+    "y"
+
+gdb_test "info break" \
+    "Num     Type\[ \]+Disp Enb Address\[ \]+What.*
+\[0-9\]+\[\t \]+dprintf.*keep y.*PENDING.*pendfunc1.*" \
+"single pending dprintf info (without symbols)"
+
+gdb_load ${binfile}
+gdb_load_shlibs $lib_sl
+
+gdb_run_cmd
+
+gdb_test "" ".*x=3.*x=4.*x=3.*" "run to resolved dprintf (without symbols)"
+
+clean_restart ${binfile}
+gdb_load_shlibs $lib_sl
+
+#
+# Test setting, querying, and modifying pending breakpoints
+#
+
+gdb_test \
+    "dprintf pendfunc1, \"x=%d\\n\", x" \
+    "Dprintf.*pendfunc1.*pending." \
+    "set pending dprintf" \
+    ".*Make dprintf pending.*y or \\\[n\\\]. $" \
+    "y"
+
+gdb_test "info break" \
+    "Num     Type\[ \]+Disp Enb Address\[ \]+What.*
+\[0-9\]+\[\t \]+dprintf.*keep y.*PENDING.*pendfunc1.*" \
+"single pending dprintf info"
+
+gdb_run_cmd
+
+gdb_test "" ".*x=3.*x=4.*x=3.*" "run to resolved dprintf"
--- /dev/null
+++ b/gdb/testsuite/gdb.base/dprintf-pendshr.c
@@ -0,0 +1,26 @@
+/* This testcase is part of GDB, the GNU debugger.
+
+   Copyright 2013 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 (int x)
+{
+  int y = x + 4;
+}
+
+void pendfunc (int x)
+{
+  pendfunc1 (x);
+}

Re: [PATCH] Fix dprintf work not right if it is pending

[Pedro Alves]

Hi Hui,

Getting there.

On 04/08/2013 08:19 AM, Hui Zhu wrote:
> +  /* 1 - connect to target 1, that can run breakpoint commands.
> +     2 - create a dprintf, which resolves fine.
> +     3 - disconnect from target 2

This should have been "from target 1".

> +     4 - connect to target 2, that can NOT run breakpoint commands.



> +load_lib gdbserver-support.exp
> +

You're not calling gdbserver directly anymore, so this is no
longer necessary.


> +set testfile "dprintf-pending"
> +set libfile "dprintf-pendshr"
> +set srcfile $testfile.c
> +set libsrc  $srcdir/$subdir/$libfile.c
> +set binfile $objdir/$subdir/$testfile
> +set lib_sl  $objdir/$subdir/$libfile.sl

Please use standard_testfile/standard_output_file.


Instead of these explicit "(without ...)":

> +    "set pending dprintf (without format)" \
...
> +    "set pending dprintf (without symbols)" \
...
> +"single pending dprintf info (without symbols)"
...
> +gdb_test "" ".*x=3.*x=4.*x=3.*" "run to resolved dprintf (without symbols)"

Please use with_test_prefix, like

with_test_prefix "without format" {
  ... group all "without format" bits here, including test setup ...
}

with_test_prefix "without symbols" {
  ... group all "without symbols" bits here ...
}


> +gdb_test "file ${binfile}" ".*Error in re-setting breakpoint.*" "load symbols get without format error"

"load symbols get without format error" is not very correct English
and I find it hard to grok.  I suggest instead "resolved dprintf fails to
be re-set".  No need to mention "without format" here, as that will be
covered by with_test_prefix.

-- 
Pedro Alves

Re: [PATCH] Fix dprintf work not right if it is pending

[Hui Zhu]

[-- Attachment #1: Type: text/plain, Size: 2323 bytes --]

Hi Pedro,

Thanks for your review.

On Mon, Apr 8, 2013 at 10:39 PM, Pedro Alves <palves@redhat.com> wrote:
> Hi Hui,
>
> Getting there.
>
> On 04/08/2013 08:19 AM, Hui Zhu wrote:
>> +  /* 1 - connect to target 1, that can run breakpoint commands.
>> +     2 - create a dprintf, which resolves fine.
>> +     3 - disconnect from target 2
>
> This should have been "from target 1".

Fixed.

>
>> +     4 - connect to target 2, that can NOT run breakpoint commands.
>
>
>
>> +load_lib gdbserver-support.exp
>> +
>
> You're not calling gdbserver directly anymore, so this is no
> longer necessary.

Fixed.

>
>
>> +set testfile "dprintf-pending"
>> +set libfile "dprintf-pendshr"
>> +set srcfile $testfile.c
>> +set libsrc  $srcdir/$subdir/$libfile.c
>> +set binfile $objdir/$subdir/$testfile
>> +set lib_sl  $objdir/$subdir/$libfile.sl
>
> Please use standard_testfile/standard_output_file.

Fixed.

>
>
> Instead of these explicit "(without ...)":
>
>> +    "set pending dprintf (without format)" \
> ...
>> +    "set pending dprintf (without symbols)" \
> ...
>> +"single pending dprintf info (without symbols)"
> ...
>> +gdb_test "" ".*x=3.*x=4.*x=3.*" "run to resolved dprintf (without symbols)"
>
> Please use with_test_prefix, like
>
> with_test_prefix "without format" {
>   ... group all "without format" bits here, including test setup ...
> }
>
> with_test_prefix "without symbols" {
>   ... group all "without symbols" bits here ...
> }
>

Fixed.

>
>> +gdb_test "file ${binfile}" ".*Error in re-setting breakpoint.*" "load symbols get without format error"
>
> "load symbols get without format error" is not very correct English
> and I find it hard to grok.  I suggest instead "resolved dprintf fails to
> be re-set".  No need to mention "without format" here, as that will be
> covered by with_test_prefix.
>

Fixed.

> --
> Pedro Alves
>

Post a new version according to your comments.

Best,
Hui

2013-04-07  Pedro Alves  <palves@redhat.com>
   Hui Zhu  <hui@codesourcery.com>

* breakpoint.c (dprintf_re_set): New.
(initialize_breakpoint_ops): Set dprintf_breakpoint_ops re_set
to dprintf_re_set.

2013-04-07  Hui Zhu  <hui@codesourcery.com>

* gdb.base/Makefile.in (EXECUTABLES): Add dprintf-pending.
(MISCELLANEOUS): Add dprintf-pendshr.sl.
* gdb.base/dprintf-pending.c, gdb.base/dprintf-pending.exp: New.

[-- Attachment #2: dprintf-pending.txt --]
[-- Type: text/plain, Size: 1618 bytes --]

--- a/gdb/breakpoint.c
+++ b/gdb/breakpoint.c
@@ -12963,6 +12963,35 @@ bkpt_re_set (struct breakpoint *b)
   breakpoint_re_set_default (b);
 }
 
+/* Dprintf breakpoint_ops methods.  */
+
+static void
+dprintf_re_set (struct breakpoint *b)
+{
+  breakpoint_re_set_default (b);
+
+  /* This breakpoint could have been pending, and be resolved now, and
+     if so, we should now have the extra string.  If we don't, the
+     dprintf was malformed when created, but we couldn't tell because
+     we can't extract the extra string until the location is
+     resolved.  */
+  if (b->loc != NULL && b->extra_string == NULL)
+    error (_("Format string required"));
+
+  /* 1 - connect to target 1, that can run breakpoint commands.
+     2 - create a dprintf, which resolves fine.
+     3 - disconnect from target 1
+     4 - connect to target 2, that can NOT run breakpoint commands.
+
+     After steps #3/#4, you'll want the dprintf command list to
+     be updated, because target 1 and 2 may well return different
+     answers for target_can_run_breakpoint_commands().
+     Given absence of finer grained resetting, we get to do
+     it all the time.  */
+  if (b->extra_string != NULL)
+    update_dprintf_command_list (b);
+}
+
 static int
 bkpt_insert_location (struct bp_location *bl)
 {
@@ -16001,7 +16030,7 @@ initialize_breakpoint_ops (void)
 
   ops = &dprintf_breakpoint_ops;
   *ops = bkpt_base_breakpoint_ops;
-  ops->re_set = bkpt_re_set;
+  ops->re_set = dprintf_re_set;
   ops->resources_needed = bkpt_resources_needed;
   ops->print_it = bkpt_print_it;
   ops->print_mention = bkpt_print_mention;

[-- Attachment #3: dprintf-pending-test.txt --]
[-- Type: text/plain, Size: 5893 bytes --]

--- a/gdb/testsuite/gdb.base/Makefile.in
+++ b/gdb/testsuite/gdb.base/Makefile.in
@@ -10,7 +10,8 @@ EXECUTABLES = a2-run advance all-types a
 	call-strs callexit callfuncs callfwmall charset checkpoint \
 	chng-syms code_elim1 code_elim2 commands compiler complex \
 	condbreak consecutive constvars coremaker cursal cvexpr \
-	dbx-test del disasm-end-cu display dump dup-sect dup-sect.debug \
+	dbx-test del disasm-end-cu display dprintf-pending dump dup-sect \
+	dup-sect.debug \
 	dup-sect.stripped ending-run execd-prog expand-psymtabs exprs \
 	fileio find finish fixsection float foll-exec foll-fork foll-vfork \
 	frame-args freebpcmd fullname funcargs gcore \
@@ -44,7 +45,7 @@ EXECUTABLES = a2-run advance all-types a
 	wchar whatis whatis-exp catch-syscall \
 	pr10179 gnu_vector
 
-MISCELLANEOUS = coremmap.data ../foobar.baz fixsectshr.sl \
+MISCELLANEOUS = coremmap.data dprintf-pendshr.sl ../foobar.baz fixsectshr.sl \
 	pendshr.sl shreloc1.sl shreloc2.sl twice-tmp.c \
 	shr1.sl shr2.sl solib_sl.sl solib1.sl solib2.sl \
 	unloadshr.sl unloadshr2.sl watchpoint-solib-shr.sl \
--- /dev/null
+++ b/gdb/testsuite/gdb.base/dprintf-pending.c
@@ -0,0 +1,29 @@
+/* This testcase is part of GDB, the GNU debugger.
+
+   Copyright 2013 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/>.  */
+
+int k = 0;
+
+extern void pendfunc (int x);
+
+int main()
+{
+  pendfunc (3); /* break main here */
+  pendfunc (4);
+  k = 1;
+  pendfunc (3);
+  return 0;
+}
--- /dev/null
+++ b/gdb/testsuite/gdb.base/dprintf-pending.exp
@@ -0,0 +1,100 @@
+#   Copyright 2013 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/>.
+
+
+if {[skip_shlib_tests]} {
+    return 0
+}
+
+standard_testfile .c
+set libfile "dprintf-pendshr"
+set libsrc  $srcdir/$subdir/$libfile.c
+set lib_sl  $objdir/$subdir/$libfile.sl
+
+set lib_opts  debug
+set exec_opts [list debug shlib=$lib_sl]
+
+if { [get_compiler_info] } {
+    return -1
+}
+
+if { [gdb_compile_shlib $libsrc $lib_sl $lib_opts] != ""
+     || [gdb_compile $srcdir/$subdir/$srcfile $binfile executable $exec_opts] != ""} {
+    untested "Could not compile either $libsrc or $srcdir/$subdir/$srcfile."
+    return -1
+}
+
+with_test_prefix "without format" {
+    gdb_exit
+    gdb_start
+    gdb_reinitialize_dir $srcdir/$subdir
+
+    gdb_test \
+	"dprintf pendfunc" \
+	"Dprintf.*pendfunc.*pending." \
+	"set pending dprintf" \
+	".*Make dprintf pending.*y or \\\[n\\\]. $" \
+	"y"
+
+    gdb_test "file ${binfile}" ".*Error in re-setting breakpoint.*" "resolved dprintf fails to be re-set"
+}
+
+with_test_prefix "without symbols" {
+    gdb_exit
+    gdb_start
+    gdb_reinitialize_dir $srcdir/$subdir
+
+    gdb_test \
+	"dprintf pendfunc1, \"x=%d\\n\", x" \
+	"Dprintf.*pendfunc1.*pending." \
+	"set pending dprintf" \
+	".*Make dprintf pending.*y or \\\[n\\\]. $" \
+	"y"
+
+    gdb_test "info break" \
+        "Num     Type\[ \]+Disp Enb Address\[ \]+What.*
+\[0-9\]+\[\t \]+dprintf.*keep y.*PENDING.*pendfunc1.*" \
+	"single pending dprintf info"
+
+    gdb_load ${binfile}
+    gdb_load_shlibs $lib_sl
+
+    gdb_run_cmd
+
+    gdb_test "" ".*x=3.*x=4.*x=3.*" "run to resolved dprintf"
+}
+
+clean_restart ${binfile}
+gdb_load_shlibs $lib_sl
+
+#
+# Test setting, querying, and modifying pending breakpoints
+#
+
+gdb_test \
+    "dprintf pendfunc1, \"x=%d\\n\", x" \
+    "Dprintf.*pendfunc1.*pending." \
+    "set pending dprintf" \
+    ".*Make dprintf pending.*y or \\\[n\\\]. $" \
+    "y"
+
+gdb_test "info break" \
+    "Num     Type\[ \]+Disp Enb Address\[ \]+What.*
+\[0-9\]+\[\t \]+dprintf.*keep y.*PENDING.*pendfunc1.*" \
+"single pending dprintf info"
+
+gdb_run_cmd
+
+gdb_test "" ".*x=3.*x=4.*x=3.*" "run to resolved dprintf"
--- /dev/null
+++ b/gdb/testsuite/gdb.base/dprintf-pendshr.c
@@ -0,0 +1,26 @@
+/* This testcase is part of GDB, the GNU debugger.
+
+   Copyright 2013 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 (int x)
+{
+  int y = x + 4;
+}
+
+void pendfunc (int x)
+{
+  pendfunc1 (x);
+}

Re: [PATCH] Fix dprintf work not right if it is pending

[Pedro Alves]

Hi Hui,

On 04/09/2013 10:00 AM, Hui Zhu wrote:
> On Mon, Apr 8, 2013 at 10:39 PM, Pedro Alves <palves@redhat.com> wrote:
>>
>>> +set testfile "dprintf-pending"
>>> +set libfile "dprintf-pendshr"
>>> +set srcfile $testfile.c
>>> +set libsrc  $srcdir/$subdir/$libfile.c
>>> +set binfile $objdir/$subdir/$testfile
>>> +set lib_sl  $objdir/$subdir/$libfile.sl
>>
>> Please use standard_testfile/standard_output_file.
> 
> Fixed.

Close, but:

> +standard_testfile .c
> +set libfile "dprintf-pendshr"
> +set libsrc  $srcdir/$subdir/$libfile.c
> +set lib_sl  $objdir/$subdir/$libfile.sl

".c" is not necessary.  Do use standard_output_file please.

Like so:

standard_testfile

set libfile "dprintf-pendshr"
set libsrc  $srcdir/$subdir/$libfile.c
set lib_sl [standard_output_file $libfile.sl]


Oh,

> +++ b/gdb/breakpoint.c
> @@ -12963,6 +12963,35 @@ bkpt_re_set (struct breakpoint *b)
>    breakpoint_re_set_default (b);
>  }
>
> +/* Dprintf breakpoint_ops methods.  */
> +
> +static void
> +dprintf_re_set (struct breakpoint *b)
> +{
...
> +}
> +
>  static int
>  bkpt_insert_location (struct bp_location *bl)
>  {

I only now noticed you're putting the new dprintf_re_set
function right in the middle of the

/* Default breakpoint_ops methods.  */

bkpt_re_set (struct breakpoint *b)
...
bkpt_insert_location (struct bp_location *bl)
...
bkpt_remove_location (struct bp_location *bl)
...
bkpt_breakpoint_hit (const struct bp_location *bl,
...
bkpt_resources_needed (const struct bp_location *bl)
...
bkpt_print_it (bpstat bs)
...
bkpt_print_mention (struct breakpoint *b)
...
bkpt_decode_linespec (struct breakpoint *b, char **s,
...

section.  If you keep scrolling down, you'll notice that
we have a section/block/group of functions for each
breakpoint type.

/* Virtual table for internal breakpoints.  */
/* Virtual table for momentary breakpoints  */
/* Specific methods for probe breakpoints.  */
/* The breakpoint_ops structure to be used in tracepoints.  */

etc. for all other types.  Please don't break this layout.

Otherwise OK.

Thanks,
-- 
Pedro Alves

Re: [PATCH] Fix dprintf work not right if it is pending

[Hui Zhu]

Hi Pedro,

Thanks for your review.

On Tue, Apr 9, 2013 at 8:03 PM, Pedro Alves <palves@redhat.com> wrote:
> Hi Hui,
>
> On 04/09/2013 10:00 AM, Hui Zhu wrote:
>> On Mon, Apr 8, 2013 at 10:39 PM, Pedro Alves <palves@redhat.com> wrote:
>>>
>>>> +set testfile "dprintf-pending"
>>>> +set libfile "dprintf-pendshr"
>>>> +set srcfile $testfile.c
>>>> +set libsrc  $srcdir/$subdir/$libfile.c
>>>> +set binfile $objdir/$subdir/$testfile
>>>> +set lib_sl  $objdir/$subdir/$libfile.sl
>>>
>>> Please use standard_testfile/standard_output_file.
>>
>> Fixed.
>
> Close, but:
>
>> +standard_testfile .c
>> +set libfile "dprintf-pendshr"
>> +set libsrc  $srcdir/$subdir/$libfile.c
>> +set lib_sl  $objdir/$subdir/$libfile.sl
>
> ".c" is not necessary.  Do use standard_output_file please.
>
> Like so:
>
> standard_testfile
>
> set libfile "dprintf-pendshr"
> set libsrc  $srcdir/$subdir/$libfile.c
> set lib_sl [standard_output_file $libfile.sl]

Fixed.

>
>
> Oh,
>
>> +++ b/gdb/breakpoint.c
>> @@ -12963,6 +12963,35 @@ bkpt_re_set (struct breakpoint *b)
>>    breakpoint_re_set_default (b);
>>  }
>>
>> +/* Dprintf breakpoint_ops methods.  */
>> +
>> +static void
>> +dprintf_re_set (struct breakpoint *b)
>> +{
> ...
>> +}
>> +
>>  static int
>>  bkpt_insert_location (struct bp_location *bl)
>>  {
>
> I only now noticed you're putting the new dprintf_re_set
> function right in the middle of the
>
> /* Default breakpoint_ops methods.  */
>
> bkpt_re_set (struct breakpoint *b)
> ...
> bkpt_insert_location (struct bp_location *bl)
> ...
> bkpt_remove_location (struct bp_location *bl)
> ...
> bkpt_breakpoint_hit (const struct bp_location *bl,
> ...
> bkpt_resources_needed (const struct bp_location *bl)
> ...
> bkpt_print_it (bpstat bs)
> ...
> bkpt_print_mention (struct breakpoint *b)
> ...
> bkpt_decode_linespec (struct breakpoint *b, char **s,
> ...
>
> section.  If you keep scrolling down, you'll notice that
> we have a section/block/group of functions for each
> breakpoint type.
>
> /* Virtual table for internal breakpoints.  */
> /* Virtual table for momentary breakpoints  */
> /* Specific methods for probe breakpoints.  */
> /* The breakpoint_ops structure to be used in tracepoints.  */
>
> etc. for all other types.  Please don't break this layout.

Fixed.

>
> Otherwise OK.

Commited in http://sourceware.org/ml/gdb-cvs/2013-04/msg00097.html

Thanks,
Hui

>
> Thanks,
> --
> Pedro Alves
>

Re: [PATCH] Fix dprintf work not right if it is pending

[Hui Zhu]

Hi Joel,

http://sourceware.org/ml/gdb-cvs/2013-04/msg00097.html

This is a bug fix.  Can I check it in to 7.6 branch?

Thanks,
Hui

On Wed, Apr 10, 2013 at 12:40 PM, Hui Zhu <teawater@gmail.com> wrote:
> Hi Pedro,
>
> Thanks for your review.
>
> On Tue, Apr 9, 2013 at 8:03 PM, Pedro Alves <palves@redhat.com> wrote:
>> Hi Hui,
>>
>> On 04/09/2013 10:00 AM, Hui Zhu wrote:
>>> On Mon, Apr 8, 2013 at 10:39 PM, Pedro Alves <palves@redhat.com> wrote:
>>>>
>>>>> +set testfile "dprintf-pending"
>>>>> +set libfile "dprintf-pendshr"
>>>>> +set srcfile $testfile.c
>>>>> +set libsrc  $srcdir/$subdir/$libfile.c
>>>>> +set binfile $objdir/$subdir/$testfile
>>>>> +set lib_sl  $objdir/$subdir/$libfile.sl
>>>>
>>>> Please use standard_testfile/standard_output_file.
>>>
>>> Fixed.
>>
>> Close, but:
>>
>>> +standard_testfile .c
>>> +set libfile "dprintf-pendshr"
>>> +set libsrc  $srcdir/$subdir/$libfile.c
>>> +set lib_sl  $objdir/$subdir/$libfile.sl
>>
>> ".c" is not necessary.  Do use standard_output_file please.
>>
>> Like so:
>>
>> standard_testfile
>>
>> set libfile "dprintf-pendshr"
>> set libsrc  $srcdir/$subdir/$libfile.c
>> set lib_sl [standard_output_file $libfile.sl]
>
> Fixed.
>
>>
>>
>> Oh,
>>
>>> +++ b/gdb/breakpoint.c
>>> @@ -12963,6 +12963,35 @@ bkpt_re_set (struct breakpoint *b)
>>>    breakpoint_re_set_default (b);
>>>  }
>>>
>>> +/* Dprintf breakpoint_ops methods.  */
>>> +
>>> +static void
>>> +dprintf_re_set (struct breakpoint *b)
>>> +{
>> ...
>>> +}
>>> +
>>>  static int
>>>  bkpt_insert_location (struct bp_location *bl)
>>>  {
>>
>> I only now noticed you're putting the new dprintf_re_set
>> function right in the middle of the
>>
>> /* Default breakpoint_ops methods.  */
>>
>> bkpt_re_set (struct breakpoint *b)
>> ...
>> bkpt_insert_location (struct bp_location *bl)
>> ...
>> bkpt_remove_location (struct bp_location *bl)
>> ...
>> bkpt_breakpoint_hit (const struct bp_location *bl,
>> ...
>> bkpt_resources_needed (const struct bp_location *bl)
>> ...
>> bkpt_print_it (bpstat bs)
>> ...
>> bkpt_print_mention (struct breakpoint *b)
>> ...
>> bkpt_decode_linespec (struct breakpoint *b, char **s,
>> ...
>>
>> section.  If you keep scrolling down, you'll notice that
>> we have a section/block/group of functions for each
>> breakpoint type.
>>
>> /* Virtual table for internal breakpoints.  */
>> /* Virtual table for momentary breakpoints  */
>> /* Specific methods for probe breakpoints.  */
>> /* The breakpoint_ops structure to be used in tracepoints.  */
>>
>> etc. for all other types.  Please don't break this layout.
>
> Fixed.
>
>>
>> Otherwise OK.
>
> Commited in http://sourceware.org/ml/gdb-cvs/2013-04/msg00097.html
>
> Thanks,
> Hui
>
>>
>> Thanks,
>> --
>> Pedro Alves
>>

Re: [PATCH] Fix dprintf work not right if it is pending

[Joel Brobecker]

> http://sourceware.org/ml/gdb-cvs/2013-04/msg00097.html
> This is a bug fix.  Can I check it in to 7.6 branch?

Not sure if the patch is considered safe enough, but it should only
affect dprintf, so no objection. Pedro, would it be OK with you?

-- 
Joel

Re: [PATCH] Fix dprintf work not right if it is pending

[Pedro Alves]

On 04/11/2013 02:26 AM, Joel Brobecker wrote:
>> http://sourceware.org/ml/gdb-cvs/2013-04/msg00097.html
>> This is a bug fix.  Can I check it in to 7.6 branch?
> 
> Not sure if the patch is considered safe enough, but it should only
> affect dprintf, so no objection. Pedro, would it be OK with you?

Yes, I think it's safe enough to put it in 7.6.

Thanks,
-- 
Pedro Alves

Re: [PATCH] Fix dprintf work not right if it is pending

[Hui Zhu]

Checked in to http://sourceware.org/ml/gdb-cvs/2013-04/msg00113.html

Best,
Hui

On Thu, Apr 11, 2013 at 4:37 PM, Pedro Alves <palves@redhat.com> wrote:
> On 04/11/2013 02:26 AM, Joel Brobecker wrote:
>>> http://sourceware.org/ml/gdb-cvs/2013-04/msg00097.html
>>> This is a bug fix.  Can I check it in to 7.6 branch?
>>
>> Not sure if the patch is considered safe enough, but it should only
>> affect dprintf, so no objection. Pedro, would it be OK with you?
>
> Yes, I think it's safe enough to put it in 7.6.
>
> Thanks,
> --
> Pedro Alves
>

Re: [PATCH] Fix dprintf work not right if it is pending

[Hui Zhu]

On Fri, Mar 22, 2013 at 7:12 PM, Yao Qi <yao@codesourcery.com> wrote:
> On 03/22/2013 02:16 PM, Hui Zhu wrote:
>>
>> The dprintf's commands is setup in function init_breakpoint_sal:
>>         /* Dynamic printf requires and uses additional arguments on the
>>          command line, otherwise it's an error.  */
>>         if (type == bp_dprintf)
>>         {
>>           if (b->extra_string)
>
>
> "b->extra_string != NULL" is encouraged nowadays.

Thanks for your remind.  I get a lot of review like this, do we have
some doc for that.  I checked the "gnu code stand" but I didn't find
it.

>
>
>>             update_dprintf_command_list (b);
>>           else
>>             error (_("Format string required"));
>>         }
>>         else if (b->extra_string)
>>         error (_("Garbage '%s' at end of command"), b->extra_string);
>>
>> But if the dprintf is pending.  When it reset by function bkpt_re_set,
>> there is not code to code to update extra_string to commands.
>> So I add this code to function update_breakpoint_locations.  The issue is
>> fixed.
>
>
> The bug was reported in PR breakpoints/15292: Pending dprintf don't
> work.
>
> We need a test case here, I think, to show pending dprintf doesn't
> work, and it works with your patch applied.
>
> I am wondering whether we need a new breakpoint_ops field
> "parse_extra_string", and use it like:
>
>    b->ops->parse_extra_string (b, extra_string);
>
> instead of duplicate the code.
>
> --
> Yao (齐尧)

Re: [PATCH] Fix dprintf work not right if it is pending

[Yao Qi]

On 03/25/2013 12:07 PM, Hui Zhu wrote:
> Thanks for your remind.  I get a lot of review like this, do we have
> some doc for that.  I checked the "gnu code stand" but I didn't find
> it.

See "16.1.4 C Usage" in

http://sourceware.org/gdb/current/onlinedocs/gdbint/Coding-Standards.html

-- 
Yao (齐尧)