RE: [PATCH] add -s option to make -break-insert support dprintf

Marc Khouzam
2013-04-10T10:31:04Z

> ________________________________________
> From: gdb-patches-owner@sourceware.org [gdb-patches-owner@sourceware.org] on behalf of Pedro Alves [palves@redhat.com]
> Sent: April 9, 2013 1:50 PM
> To: Hui Zhu
> Cc: Eli Zaretskii; Hui Zhu; gdb-patches@sourceware.org; Marc Khouzam
> Subject: Re: [PATCH] add -s option to make -break-insert support dprintf
>
> Hi Hui,
>
> Thanks for the patch.
>
> New MI features need a NEWS entry.
>
> On 03/29/2013 08:01 AM, Hui Zhu wrote:
>
> > + if (hardware && dprintf)
> > + error (_("-break-insert: -h and -s cannot be use together"));
>
> "cannot be used"
>
> > @@ -180,11 +189,14 @@ mi_cmd_break_insert (char *command, char
> > regular non-jump based tracepoints. */
> > type_wanted = (tracepoint
> > ? (hardware ? bp_fast_tracepoint : bp_tracepoint)
> > - : (hardware ? bp_hardware_breakpoint : bp_breakpoint));
> > - ops = tracepoint ? &tracepoint_breakpoint_ops : &bkpt_breakpoint_ops;
> > + : (hardware ? bp_hardware_breakpoint
> > + : (dprintf ? bp_dprintf : bp_breakpoint)));
> > + ops = tracepoint ? &tracepoint_breakpoint_ops
> > + : (dprintf ? &dprintf_breakpoint_ops
> > + : &bkpt_breakpoint_ops);
>
> This is getting unnecessarily hard for humans to grok. Write
> instead as (untested):
>
> if (tracepoint)
> {
> /* move existing comment on fast tracepoints here */
> type_wanted = hardware ? bp_fast_tracepoint : bp_tracepoint;
> ops = &tracepoint_breakpoint_ops;
> }
> else if (dprintf)
> {
> type_wanted = bp_dprintf;
> ops = &dprintf_breakpoint_ops;
> }
> else
> {
> type_wanted = hardware ? bp_hardware_breakpoint : bp_breakpoint;
> ops = &bkpt_breakpoint_ops;
> }

In the orginal patch, having both 'hardware' and 'dprintf' true would
create a hardware breakpoint (not dprintf), but would still set 'ops'
to &dprintf_breakpoint_ops. This didn't look right to me.

A side-effect of Pedro's change is that the hardware dprintf case
will be handled properly. I think that is a good thing. However,
I wanted to mention it, as I don't know if there are other changes
needed to handle a hardware dprintf (or if it really should be allowed).
I am allowed to create a hardware breakpoint with a printf condition,
so I guess a hardware dprintf would make sense, but I'm not sure.

Marc