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

[Pedro Alves <alves dot ped at gmail dot com>]

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?