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

[Keith Seitz <keiths@redhat.com>]

On 5/30/22 12:08, Philippe Waroquiers via Gdb-patches wrote:
> Ping^5

Hi, I'm sorry it has taken so long for someone to look at this
because I think this is a really useful UI improvement. Thanks!

I've really only got some pretty minor questions to ask and a nit or
two to mention. Nothing serious.

>> Below is the detailed feedback, summary is that everybody prefers
>> to see the breakpoint and location number, and does not see a (real)
>> need to have this configurable.

I looked up the history of this patch a bit to understand the motivation
for querying co-workers... I see Eli commented in the original April
discussion:

> I'm not sure everyone will want to see the likes of
>
>  Thread 1 "foobar" hit breakpoint 10.42, some_func () at ...
>
> But that's MO; I'd be interested in opinions of others.

Well, I'm not a global maintainer, but I'll chime in with my opinion,
if everyone will permit me. [Ha! You're already reading it!]

I don't think it is a big deal to see ".42" in the output. Far more
irritating is not knowing exactly which of the many locations it hit.
I've always felt this was akin to ambiguous output by GDB. "You hit
breakpoint X." Oh, wait, that's one of fifty different real breakpoints.

I could make the argument for "2" vs "2.1" when there is only one location,
but you've addressed that case.

I admit, though, I am less critical of UI changes than many. I'd like
to reiterate Eli's suggestion and also encourage others to weigh in.

>>> Also, when a breakpoint is reached, the convenience variables
>>> $bkptno and $locno are set to the encountered breakpoint number
>>> and location number.

We have convenience variables for when we set breakpoints ($bpnum)
and tracepoints ($tpnum). Should we re-use those or would that lead
to user confusion? AFAICT, we only use those variables when a
{break,trace}point is created or modified. This seems like it could
be a natural extension of the utility of those variables.

I am not asking for any changes, I'm genuinely curious if reusing
these existing convenience vars would be desirable in your opinion.

>>> diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
>>> index 1a227e5d120..a7d74d598db 100644
>>> --- a/gdb/breakpoint.c
>>> +++ b/gdb/breakpoint.c
>>> @@ -687,6 +687,19 @@ get_breakpoint (int num)
>>>     return nullptr;
>>>   }
>>>   
>>>
>>> +/* Return TRUE if NUM refer to an existing breakpoint that has
>>> +   multiple locations.  */
>>> +
>>> +static bool
>>> +has_multiple_locations (int num)

At one time, we used to append "_p" to these types of predicate-
style functions. Are we abandoning this practice? [Mind you,
this is simply a meta-question. Again, I am not asking for
any changes.]

>>> +{
>>> +  for (breakpoint *b : all_breakpoints ())
>>> +    if (b->number == num)
>>> +      return b->loc != nullptr && b->loc->next != nullptr;
>>> +
>>> +  return false;
>>> +}
>>> +
>>>   \f
>>>   
>>>
>>>   /* Mark locations as "conditions have changed" in case the target supports
>>> @@ -4364,6 +4369,57 @@ bpstat_num (bpstat **bsp, int *num)
>>>     return 1;
>>>   }
>>>   
>>>
>>> +/* See breakpoint.h  */
>>> +
>>> +int
>>> +bpstat_locno (bpstat *bs)
>>> +{
>>> +  const struct breakpoint *b = bs->breakpoint_at;
>>> +  const struct bp_location *bl = bs->bp_location_at.get ();
>>> +
>>> +  int locno = 0;
>>> +
>>> +  if (b != nullptr && b->loc->next != nullptr)
>>> +    {
>>> +      const bp_location *bl_i;
>>> +
>>> +      for (bl_i = b->loc;
>>> +	   bl_i != bl && bl_i->next != nullptr;
>>> +	   bl_i = bl_i->next)
>>> +	locno++;
>>> +
>>> +      if (bl_i == bl)
>>> +	locno++;
>>> +      else
>>> +	{
>>> +	  warning (_("location number not found for breakpoint %d address %p."),
>>> +		   b->number, paddress (bl->gdbarch, bl->address));

Doesn't paddress require %s (or even nicer: %ps with a styled_string ())?

>>> +	  locno = 0;
>>> +	}
>>> +    }
>>> +
>>> +  return locno;
>>> +}
>>> +
>>> +/* See breakpoint.h.  */
>>> +
>>> +void
>>> +print_num_locno (bpstat *bs, struct ui_out *uiout)
>>> +{
>>> +  struct breakpoint *b = bs->breakpoint_at;
>>> +
>>> +  if (b == nullptr)
>>> +    uiout->text (_("deleted breakpoint"));
>>> +  else
>>> +    {
>>> +      uiout->field_signed ("bkptno", b->number);
>>> +
>>> +      int locno = bpstat_locno (bs);
>>> +      if (locno != 0)
>>> +	uiout->message (".%pF", signed_field ("locno", locno));
>>> +    }
>>> +}
>>> +
>>>   /* See breakpoint.h.  */
>>>   
>>>
>>>   void
>>> diff --git a/gdb/testsuite/lib/gdb.exp b/gdb/testsuite/lib/gdb.exp
>>> index 7c35fbb18c4..a53d929be02 100644
>>> --- a/gdb/testsuite/lib/gdb.exp
>>> +++ b/gdb/testsuite/lib/gdb.exp
>>> @@ -227,6 +227,14 @@ set inferior_exited_re "(?:\\\[Inferior \[0-9\]+ \\(\[^\n\r\]*\\) exited)"
>>>   # E.g., $1, $2, etc.
>>>   set valnum_re "\\\$$decimal"
>>>   
>>>
>>> +# A regular expression that matches a breakpoint hit with a breakpoint
>>> +# having several locations
>>> +set bkptno_num_re "$decimal\\.$decimal"
>>> +
>>> +# A regular expression that matches a breakpoint hit
>>> +# with one or several locations.
>>> +set bkptno_numopt_re "($decimal\\.$decimal|$decimal)"
>>> +

Nice! I've always thought this would be needed "one day." I guess that
day finally arrived.

>>>   ### Only procedures should come after this point.
>>>   
>>>
>>>   #
>>> @@ -699,7 +708,13 @@ proc runto { linespec args } {
>>>   	    }
>>>   	    return 1
>>>   	}
>>> -	-re "Breakpoint \[0-9\]*, \[0-9xa-f\]* in .*$gdb_prompt $" {
>>> +	-re "Breakpoint \[0-9\]*, \[0-9xa-f\]* in .*$gdb_prompt $" {
>>> +	    if { $print_pass } {
>>> +		pass $test_name
>>> +	    }
>>> +	    return 1
>>> +	}
>>> +	-re "Breakpoint \[0-9\]*\.\[0-9\]*, \[0-9xa-f\]* in .*$gdb_prompt $" {
>>>   	    if { $print_pass } {
>>>   		pass $test_name
>>>   	    }

It doesn't seem that we make a distinction here between the "Breakpoint N" vs "Breakpoint N.M"
cases. Is it possible to merge these two branches into one using bkptno_numopt_re?
[super super minor nit]

Thank you so much for your patience. I hope a global maintainer with approval
authority will follow-up.

Keith