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

[Hui Zhu <teawater at gmail dot com>]

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: dprintf-pending.txt
Description: Text document

Attachment: dprintf-pending-test.txt
Description: Text document