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

[Hui Zhu <teawater@gmail.com>]

[-- 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);
+}