Re: [RFC stub-side break conditions 3/5] GDB-side changes

[Luis Machado <luis_gustavo at mentor dot com>]

On 01/06/2012 02:23 PM, Pedro Alves wrote:
> On 01/05/2012 02:56 PM, Luis Machado wrote:
>
> > Regarding always-inserted mode, if it is "on" we must send condition 
> > changes right away, or else the target will potentially miss 
> > breakpoint hits.
>
> Could you elaborate a bit more on how will it miss breakpoint hits?

That is a bit misleading. Not actually the target, but GDB may not be 
notified of breakpoint hits that it should've seen.

For example. Suppose we have a conditional breakpoint location inserted 
at address foo. The inferior is running and stopping at the breakpoint 
correctly when its condition is true.

Suppose we insert a new unconditional location at foo. If we don't synch 
the breakpoint conditions of both those locations with the target, the 
target will keep ignoring any breakpoint hits at foo for which the 
condition of the first location is false.  Of course we may miss a few 
breakpoint hits during the condition synchronization operation, but GDB 
will eventually synch the conditions with the target instead of waiting 
for the next appropriate time to do so.

Another example is a set of 2 duplicate conditional breakpoint locations 
inserted at address foo. If we make one of them unconditional while the 
process is running (always-inserted mode on), the conditions need to be 
synched with the target as well. If we don't, GDB may miss some 
breakpoint triggers (since the target won't report every trigger, just 
the conditional ones).

> > I'd like to hear about the change to "info break"
>
> As they say, a picture is worth a thousand words.  How does the new 
> output look like?  What about MI?

An example:

2       breakpoint      keep y   0x080483dd in main at loop.c:12
    stop only if i==5 (stub)

The change in the output is just a (stub) right after the conditional 
expression.

This is being printed together with the conditional expression, so 
frontends will possibly pick that output up without requiring any 
additional changes.

> > and also the way we should pass conditions down to the stub.  
> Currently i'm adding the agent expressions to a condition list in the 
> target info field of bp locations.
>
> Sounds good at first sight.
>
>
> I'm mostly wondering whether the assumption that we can re-insert
> a breakpoint at the same address is solid.

Z packets should, in theory, be implemented in an idempotent way 
according to the documentation. So adding the same breakpoint location 
multiple times shouldn't be a problem.

I thought about potential issues with single-stepping breakpoints, but 
they will be inserted with no conditions (making the target remove any 
conditions that might be active on its end), thus i expect them to work.

> > +/* Iterates through locations with address ADDRESS.  BP_LOCP_TMP 
> points to
> > +   each object.  BP_LOCP_START points to where the loop should 
> start from.
> > +   If BP_LOCP_START is a NULL pointer, the macro automatically 
> seeks the
> > +   appropriate location to start with.  */
> > +
> > +#define ALL_BP_LOCATIONS_AT_ADDR(BP_LOCP_TMP, BP_LOCP_START, 
> ADDRESS)    \
> > +    if (!BP_LOCP_START)                        \
> > +      BP_LOCP_START = get_first_locp_gte_addr (ADDRESS);        \
> > +    for (BP_LOCP_TMP = BP_LOCP_START;                \
> > +         (BP_LOCP_TMP<  bp_location + bp_location_count        \
> > + &&  (*BP_LOCP_TMP)->address == ADDRESS);            \
> > +         BP_LOCP_TMP++)
> > +
>
> The address alone is not enough when you consider multi-process.
> The users of this macro aren't considering that.  Several instances of
> this problem.

IIUIC, we may have situations with breakpoint locations at the same 
address but on different processes.

If a breakpoint spans multiple processes, we must resynch conditions for 
both processes. If not, then we should only re-synch target conditions 
for the process that contains such a breakpoint/location.

Does that make sense?

> > +/* Based on location BL, create a list of breakpoint conditions to be
> > +   passed on to the target.  If we have duplicated locations with 
> different
> > +   conditions, we will add such conditions to the list.  The idea 
> is that the
> > +   target will evaluate the list of conditions and will only notify 
> GDB when
> > +   one of them is true.  */
>
> What does the return code mean?
>
> > +
> > +static int
> > +build_target_condition_list (struct bp_location *bl)
> > +{

Stale, it does not need one. Fixed.

> > +  /* Even if the target evaluated the condition on its end and 
> notified GDB, we
> > +     need to do so again since GDB does not know if we stopped due 
> to a
> > +     breakpoint or a single step.
>
> > + This will only be done when we single step straight to a 
> conditional breakpoint.  */
>
> Where is the code that makes this true?

The second sentence is a bit stale. GDB re-evaluates conditions 
everytime now, so we don't need to worry about what kind of event is 
associated with the breakpoint trigger.

Fixed.
> > +
> >     if (frame_id_p (b->frame_id)
> > &&  !frame_id_eq (b->frame_id, get_stack_frame_id (get_current_frame 
> ())))
> >       bs->stop = 0;
> > @@ -4905,6 +5219,19 @@ print_one_breakpoint_location (struct br
> >         else
> >       ui_out_text (uiout, "\tstop only if ");
> >         ui_out_field_string (uiout, "cond", b->cond_string);
> > +
> > +      /* Print whether the remote stub is doing the breakpoint's 
> condition
> > +     evaluation.  If GDB is doing the evaluation, don't print 
> anything.  */
> > +      if (loc&&  is_breakpoint (b)&&  loc->cond_bytecode
> > + &&  breakpoint_condition_evaluation_mode ()
> > +      != condition_evaluation_gdb)
>
> What if you set a conditional breakpoint, have it evaluate the in target,
> and then, flip the setting to gdb-side, while the breakpoint's 
> condition is
> still being evaluated on the target side?  Shouldn't this always
> print "target"/"stub" when loc->cond_bytecode is non-NULL?

I addressed this for Eli's comments as well. Now we prevent the user 
from setting "target" evaluation mode if the target does not support it. 
A warning is displayed.

The scenario you mentioned is taken care of now. Assume the target 
supports condition evaluation, if the user switches from "gdb" to 
"target" evaluation mode, GDB will send the breakpoint conditions to the 
target.

Similarly, if the user switches from "target" to "gdb", those conditions 
will be cleared.

> > @@ -11387,7 +11810,10 @@ delete_breakpoint (struct breakpoint *bp
> >        itself, since remove_breakpoint looks at location's owner.  It
> >        might be better design to have location completely
> >        self-contained, but it's not the case now.  */
> > -  update_global_location_list (0);
> > +  if (is_breakpoint (bpt))
> > +    update_global_location_list (1);
>
> The whole point of the update_global_location_list parameter is being 
> able
> to say "don't insert locations new locations because I'm just deleting a
> breakpoint".  This was necessary for situations like following an 
> exec, IIRC.

I'll double check this since the testsuite didn't give me any visible 
errors.

> > +  else
> > +    update_global_location_list (0);
> >
>
>
> > +/* List of conditions used to pass conditions down to the target.  */
> > +struct condition_list
> > +{
> > +  /* Breakpoint condition.  */
> > +  struct agent_expr *expr;
> > +  /* Pointer to the next condition.  */
> > +  struct condition_list *next;
> > +};
>
> Can this be a VEC of `struct agent_expr *' instead?

I'm using VEC now.

Fixed all the other issues.

I'll send a new version of the patch soon.

Thanks for reviewing.

--
Luis
lgustavo@codesourcery.com