From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.129.124]) by sourceware.org (Postfix) with ESMTPS id 7405938418B1 for ; Fri, 3 Jun 2022 20:40:05 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 7405938418B1 Received: from mimecast-mx02.redhat.com (mimecast-mx02.redhat.com [66.187.233.88]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id us-mta-621-LAc4_4uSPqWYxdTEJ0RqFA-1; Fri, 03 Jun 2022 16:40:02 -0400 X-MC-Unique: LAc4_4uSPqWYxdTEJ0RqFA-1 Received: from smtp.corp.redhat.com (int-mx04.intmail.prod.int.rdu2.redhat.com [10.11.54.4]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx02.redhat.com (Postfix) with ESMTPS id B5DB3811E75; Fri, 3 Jun 2022 20:40:01 +0000 (UTC) Received: from [10.2.17.30] (unknown [10.2.17.30]) by smtp.corp.redhat.com (Postfix) with ESMTPS id 545372026D64; Fri, 3 Jun 2022 20:40:01 +0000 (UTC) Message-ID: <24da89de-d36f-53c7-fe69-a8c7a1c38caf@redhat.com> Date: Fri, 3 Jun 2022 13:40:00 -0700 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101 Thunderbird/91.9.1 Subject: Re: [RFA] Show locno for 'multi location' breakpoint hit msg+conv var $bkptno $locno. To: Philippe Waroquiers , gdb-patches@sourceware.org References: <20220417155311.3487509-1-philippe.waroquiers@skynet.be> <874988bef430ed6e239e74a4a94eb2ad13175f4a.camel@skynet.be> <1001e780cc1d9f7a85923d6707bf17f063b8da90.camel@skynet.be> From: Keith Seitz In-Reply-To: <1001e780cc1d9f7a85923d6707bf17f063b8da90.camel@skynet.be> X-Scanned-By: MIMEDefang 2.78 on 10.11.54.4 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Language: en-US Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 8bit X-Spam-Status: No, score=-20.2 required=5.0 tests=BAYES_00, DKIM_INVALID, DKIM_SIGNED, GIT_PATCH_0, KAM_DMARC_NONE, KAM_DMARC_STATUS, NICE_REPLY_A, RCVD_IN_DNSWL_NONE, SPF_HELO_NONE, SPF_NONE, TXREP, T_SCC_BODY_TEXT_LINE autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on server2.sourceware.org X-BeenThere: gdb-patches@sourceware.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Gdb-patches mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 03 Jun 2022 20:40:11 -0000 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; >>> +} >>> + >>>   >>> >>> >>>  /* 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