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

[Pedro Alves]

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?

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

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



 > +static int
 > +gdb_evaluates_breakpoint_condition_p (void)
 > +{
 > +  const char *mode = breakpoint_condition_evaluation_mode ();
 > +
 > +  return (mode == condition_evaluation_gdb? 1 : 0);

Missing space before `?'.  Or just write

   return (mode == condition_evaluation_gdb);

Same thing.

 > +}


 > +/* 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.


 > +/* 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)
 > +{



On 01/05/2012 02:56 PM, Luis Machado wrote:
 > +	}
 > +      /* We will never reach this point.  */
 > +    }

Then by all means gdb_assert.  :-)




 > +  /* 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?

 > +
 >     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?

 > +	{
 > +	  ui_out_text (uiout, " (");
 > +	  ui_out_field_string (uiout, "condeval",
 > +			       breakpoint_condition_evaluation_mode ());
 > +	  ui_out_text (uiout, ")");
 > +	}
 > +
 >         ui_out_text (uiout, "\n");
 >       }




 > @@ -10366,12 +10696,56 @@ swap_insertion (struct bp_location *left
 >
 >     left->inserted = right->inserted;
 >     left->duplicate = right->duplicate;
 > +  left->needs_update = right->needs_update;
 >     left->target_info = right->target_info;
 >     right->inserted = left_inserted;
 >     right->duplicate = left_duplicate;
 > +  left->needs_update = right->needs_update;

Doesn't look right.  Should probably be

   const int left_needs_update = left->needs_update;
   ...
   left->needs_update = right->needs_update;
   ...
   right->needs_update = left_needs_update;



 > +	  modified = modified? 1 : (*loc2p)->condition_changed;

Space before `?'.  Perhaps clearer alternatives are:

   modified = modified || (*loc2p)->condition_changed;

or even the common:

   if (!modified)
     modified = (*loc2p)->condition_changed;




 > +      /* Check if this is a new/deleted duplicated location or a duplicated
 > +	 location that had its condition modified.  If so,  we want to send its

Spurious space.


 > @@ -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.

 > +  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?