From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from simark.ca (simark.ca [158.69.221.121]) by sourceware.org (Postfix) with ESMTPS id 1BEFB385828D for ; Wed, 5 Oct 2022 12:27:45 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 1BEFB385828D Received: from [10.0.0.11] (unknown [217.28.27.60]) (using TLSv1.3 with cipher TLS_AES_128_GCM_SHA256 (128/128 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by simark.ca (Postfix) with ESMTPSA id 4F3C01E0D5; Wed, 5 Oct 2022 08:27:44 -0400 (EDT) Message-ID: <2f051e15-d1df-25a4-e20e-bad58f8e3fa0@simark.ca> Date: Wed, 5 Oct 2022 08:27:43 -0400 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101 Thunderbird/102.3.1 Subject: Re: [PATCH 2/2] gdb: more infrun debug from breakpoint.c Content-Language: en-US To: Andrew Burgess , gdb-patches@sourceware.org References: <129b33db9ac98dc806807d6595fbeec3e18edeb1.1664962429.git.aburgess@redhat.com> From: Simon Marchi In-Reply-To: <129b33db9ac98dc806807d6595fbeec3e18edeb1.1664962429.git.aburgess@redhat.com> Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit X-Spam-Status: No, score=-12.1 required=5.0 tests=BAYES_00, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, GIT_PATCH_0, NICE_REPLY_A, SPF_HELO_PASS, SPF_PASS, TXREP 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: Wed, 05 Oct 2022 12:27:46 -0000 On 2022-10-05 05:34, Andrew Burgess via Gdb-patches wrote: > This commit adds additional infrun debug from the breakpoint.c file. > The new debug output all relates to breakpoint condition evaluation. > > There is already some infrun debug emitted from the breakpoint.c file, > so hopefully, adding more will not be contentious. I think the > functions being instrumented make sense as part of the infrun process, > the inferior stops, evaluates the condition, and then either stops or > continues. This new debug gives more insight into that process. > > I had to make the bp_location* argument to find_loc_num_by_location > const, and add a declaration for find_loc_num_by_location. > > There should be no user visible changes unless they turn on debug > output. > --- > gdb/breakpoint.c | 22 ++++++++++++++++++++-- > 1 file changed, 20 insertions(+), 2 deletions(-) > > diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c > index 002f4a935b1..38546ea44ba 100644 > --- a/gdb/breakpoint.c > +++ b/gdb/breakpoint.c > @@ -165,6 +165,8 @@ static std::vector bkpt_probe_decode_location_spec > > static bool bl_address_is_meaningful (bp_location *loc); > > +static int find_loc_num_by_location (const bp_location *loc); > + > /* update_global_location_list's modes of operation wrt to whether to > insert locations now. */ > enum ugll_insert_mode > @@ -5302,6 +5304,8 @@ bpstat_check_watchpoint (bpstat *bs) > static void > bpstat_check_breakpoint_conditions (bpstat *bs, thread_info *thread) > { > + INFRUN_SCOPED_DEBUG_ENTER_EXIT; > + > const struct bp_location *bl; > struct breakpoint *b; > /* Assume stop. */ > @@ -5316,6 +5320,10 @@ bpstat_check_breakpoint_conditions (bpstat *bs, thread_info *thread) > b = bs->breakpoint_at; > gdb_assert (b != NULL); > > + infrun_debug_printf ("thread = %s, breakpoint %d.%d", > + thread->ptid.to_string ().c_str (), > + b->number, find_loc_num_by_location (bl)); > + > /* 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 breakpoint. */ > @@ -5323,6 +5331,9 @@ bpstat_check_breakpoint_conditions (bpstat *bs, thread_info *thread) > if (frame_id_p (b->frame_id) > && !frame_id_eq (b->frame_id, get_stack_frame_id (get_current_frame ()))) > { > + infrun_debug_printf ("incorrect frame %s not %s, not stopping", > + get_stack_frame_id (get_current_frame ()).to_string ().c_str (), > + b->frame_id.to_string ().c_str ()); > bs->stop = 0; > return; > } > @@ -5333,6 +5344,7 @@ bpstat_check_breakpoint_conditions (bpstat *bs, thread_info *thread) > if ((b->thread != -1 && b->thread != thread->global_num) > || (b->task != 0 && b->task != ada_get_task_number (thread))) > { > + infrun_debug_printf ("incorrect thread or task, not stopping"); > bs->stop = 0; > return; > } > @@ -5424,16 +5436,22 @@ bpstat_check_breakpoint_conditions (bpstat *bs, thread_info *thread) > > if (cond && !condition_result) > { > + infrun_debug_printf ("condition_result = false, not stopping"); > bs->stop = 0; > } > else if (b->ignore_count > 0) > { > + infrun_debug_printf ("ignore count %d, not stopping", > + b->ignore_count); > b->ignore_count--; > bs->stop = 0; > /* Increase the hit count even though we don't stop. */ > ++(b->hit_count); > gdb::observers::breakpoint_modified.notify (b); > - } > + } > + > + if (bs->stop) > + infrun_debug_printf ("stopping at this breakpoint"); > } I'd suggest a little change just for readability: if (cond != nullptr && !condition_result) { ... return; } if (b->ignore_count > 0) { ... return; } infrun_debug_printf ("stopping at this breakpoint"); This way, all cases where we decided not to stop are handled as early returns (there are more above in the function). And if we reach the very end of the function, it means we decided to stop. Other than that, this patch and the previous one LGTM. Simon