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

[Pedro Alves <palves@redhat.com>]

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