Re: [RFAv3] Show locno for 'multi location' breakpoint hit msg+conv var $bkptno $locno.

[Philippe Waroquiers <philippe.waroquiers@skynet.be>]

On Thu, 2022-11-10 at 08:57 -0700, Tom Tromey wrote:
> > > > > > "Philippe" == Philippe Waroquiers via Gdb-patches <gdb-patches@sourceware.org>
> > > > > > writes:


> Philippe> Also, when a breakpoint is reached, the convenience variables
> Philippe> $bkptno and $locno are set to the encountered breakpoint number
> Philippe> and location number.
> 
> I believe we have a convention that new internally-created convenience
> variables will start with "_".
> 
> I wondered a little whether we wanted to introduce $_bkptno or just
> repurpose the already-existing $bpnum, but I see we already do this kind
> of thing for tracepoints, and I suppose having different variables for
> different semantics is good... it's just a bit unfortunate that the
> names are similar.
Re-using $bpnum was discarded as this would break existing scripts assuming
$bpnum value is kept unchanged when a break is hit.

Thinking again about the names, having $bpnum and $bkptno (or $_bkptno)
is confusing.
So, $_hit_bpnum and $_hit_locno for the 2 new variables seem more clear
and more consistent.

> 
> Note that the breakpoint number convenience variable is
> https://sourceware.org/bugzilla/show_bug.cgi?id=12464, so this should
> be in a Bug: trailer and also mentioned in the form "PR breakpoints/12464"
> somewhere in the commit message.
> 
> Philippe> +Inside a command list, you can use the command
> Philippe> +@kbd{disable $bkptno} to disable the encountered breakpoint.
> Philippe> +
> Philippe> +If your breakpoint has several code locations, the command
> Philippe> +@kbd{disable $bkptno.$locno} will disable the specific breakpoint code
> Philippe> +location encountered.
> 
> It may be worth mentioning that the $_bkptno.$_locno form will work even
> when your breakpoint only has a single location, and also the ".1"
> special case for single-location breakpoints.
Will do.

> 
> Philippe> @@ -8494,7 +8494,21 @@ print_stop_location (const target_waitstatus &ws)
> Philippe>       LOCATION: Print only location
> Philippe>       SRC_AND_LOC: Print location and source line.  */
> Philippe>    if (do_frame_printing)
> Philippe> -    print_stack_frame (get_selected_frame (NULL), 0, source_flag, 1);
> Philippe> +    {
> Philippe> +      if (tp->control.stop_bpstat != nullptr)
> Philippe> +	{
> Philippe> +	  const struct breakpoint *b = tp->control.stop_bpstat->breakpoint_at;
> Philippe> +
> Philippe> +	  if (b != nullptr)
> Philippe> +	    {
> Philippe> +	      int locno = bpstat_locno (tp->control.stop_bpstat);
> Philippe> +	      set_internalvar_integer (lookup_internalvar ("bkptno"), b-
> >number);
> Philippe> +	      set_internalvar_integer (lookup_internalvar ("locno"),
> Philippe> +				       (locno > 0 ? locno : 1));
> 
> This is in print_stop_location - but what if the location is not
> printed?
> 
> I am curious if these variables can be, are intended to be, or should be
> usable from breakpoint conditions or silent 'commands'.

I think the semantic we want for these variables is to give
the breakpoint number of the breakpoint that was just (or last) hit.

I did not know about the "silent" breakpoint feature.
IMO, it looks better to have these variables set for silent breakpoints
as they should be available in the silent breakpoint command list.

A condition evaluating to false means the breakpoint is not considered as hit
(e.g. the nr of times a breakpoint is hit is only incremented
and its commands are only run when the condition is true).

Also, for some targets, the breakpoint condition are evaluated
at the target side.

So, it looks reasonable to me to only set these variables when a breakpoint is (really)
hit, and not to set them when evaluating a breakpoint condition.

> 
> I suspect maybe bpstat_do_actions_1 is a better spot for this, though
> I'm not really sure.
Effectively not very clear, in particular, when several breakpoints are set
at the same place and when some or all of these breakpoints are silent.
When several breakpoints are not silent, GDB prints only one of these.
In this case, the variables should be set to the printed breakpoint.
When all breakpoints are silent, I guess it is not important to choose
a particular value.

Waiting for a possible reply to this mail, I will start a new version
of the patch based on the above


Thanks
Philippe