This is the mail archive of the gdb-patches@sourceware.org mailing list for the GDB project.
Index Nav: | [Date Index] [Subject Index] [Author Index] [Thread Index] | |
---|---|---|
Message Nav: | [Date Prev] [Date Next] | [Thread Prev] [Thread Next] |
Other format: | [Raw text] |
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:
dprintf-pending.txt
Description: Text document
Attachment:
dprintf-pending-test.txt
Description: Text document
Index Nav: | [Date Index] [Subject Index] [Author Index] [Thread Index] | |
---|---|---|
Message Nav: | [Date Prev] [Date Next] | [Thread Prev] [Thread Next] |