RE: [PATCH 1/4] Fix dprintf bugs

[Marc Khouzam <marc.khouzam@ericsson.com>]

> -----Original Message-----
> From: gdb-patches-owner@sourceware.org 
> [mailto:gdb-patches-owner@sourceware.org] On Behalf Of Yao Qi
> Sent: Thursday, February 28, 2013 8:16 AM
> To: gdb-patches@sourceware.org
> Subject: [PATCH 1/4] Fix dprintf bugs
> 
> Hi,
> This patch fixes PR15075.  Besides this bug, this patch also fixes
> some other problems:
> 
>   - Two dprintf are set on the same address.  Only one printf is
>     executed.
>   - dprintf and regular breakpoint are set on the same address,
>     inferior doesn't stop.
> 
> In order to fix these problems, I don't append "continue" command to
> dprintf breakpoint, and execute commands of dprintf after
> bpstat_check_breakpoint_conditions if condition is true.  The reason I
> choose this place (after bpstat_check_breakpoint_conditions instead of
> in bpstat_check_breakpoint_conditions) to handle dprintf is that we
> have to set BS->stop to zero, but have to update hit count if
> condition is true, so we have to do two things together.
> 
> With this patch, GDB executes dprintf commands after
> bpstat_check_breakpoint_conditions, the different place where
> breakpoint commands are executed.  I propose to disable setting
> command for dprintf, or disallow user setting commands for dprintf.
> If we believe it is important to allow user setting commands for
> dprintf (a good use case?), we need a new design like creating struct
> dprintf as a 'sub-class' of breakpoint and adding its own field for
> printf commands.  The code on sending commands to the remote should be
> adjusted as well.

I've played around with this patch and I like the solution a lot!

I like the fact that a user cannot modify the list of commands for dprintf.
If they want to use commands, they should use a breakpoint, not
a dprintf.  It is no longer possible to update the dprintf string
without having to create a new dprintf though; but if this is to be
supported, I think it should be a specific dprintf operation.

I no longer see *stopped/*running events in MI, which is great.

I'm still hesitant about the -break-modified event in that case
though.  I believe the event is triggered because the hit count 
has changed.  For a normal bp, it makes sense to have this event
in this case, since execution has stopped and only a single 
event will be seen (not exactly true for non-stop, but still
makes sense, I think).  However, for dprintf which is meant to 
let the inferior continue to run, there could be quite many 
hit events very quickly.  Since we already have some feeback
that the dprintf has hit through the actual printf string, I'm
leaning towards not having that event for dprintf hits.
Furthermore, this event is not being sent when using dprintf-style
"agent" anyway.

I also saw that conditions are now properly respected for dprintf-style
"gdb" and "call".  That is great.  Conditions are still not respected for 
style "agent" but that is a separate issue I believe (PR 15180).

I did notice that although commands cannot be set for dprintf from
the CLI they are not blocked for MI:

(gdb) interpreter-exec mi "-break-commands 1 hello"
^done
(gdb) info b
Num     Type           Disp Enb Address            What
1       dprintf        keep y   0x0000000000400570 in main() at loopfirst.cc:8
        hello

Finally, it would be nice to have MI commands to deal with dprintf :)
Re-using the breakpoint MI commands sound best, although I wonder about
two cases:
a) Would we be able to specify the printf string in the -break-insert command?
b) How could we change the printf command after a dprintf is created?  Will we
   need to delete it and create a new one, or would we be able to update it?

All that being said, I'm impressed at how many improvements this
relatively small patch provides.  Nice work.

Marc