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

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

> ________________________________________
> 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