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 817783854164 for ; Thu, 6 Oct 2022 11:06:33 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 817783854164 Received: from mail-wm1-f71.google.com (mail-wm1-f71.google.com [209.85.128.71]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_128_GCM_SHA256) id us-mta-537-I72I2qmwOjaqxrLiHCy_9w-1; Thu, 06 Oct 2022 07:06:27 -0400 X-MC-Unique: I72I2qmwOjaqxrLiHCy_9w-1 Received: by mail-wm1-f71.google.com with SMTP id 18-20020a05600c229200b003c1cb5820bbso445003wmf.2 for ; Thu, 06 Oct 2022 04:06:26 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=mime-version:message-id:date:references:in-reply-to:subject:to:from :x-gm-message-state:from:to:cc:subject:date:message-id:reply-to; bh=HqzjbKWNH8unhoehL1voaZcN6x+JdOIHBvtRrGKzS08=; b=5riR9gJY8YvlleA8mjvJGi1pVSIlcTaILsIffiApezxdQykGjGP/V76+B/2T8kkxW+ E4CM5XbaDU+qq1wFVF//4i21HTFMRyr4AuhJZcU2kOeLpMWBDIuxXDdb+8DwrjbY7EzN OkjeBrPEwWFo+/4CFh8o0un3RZ4XzTYjv/T8oEk+sTWJekjpCSRNDHfiUWTjbzkRQOyr nQbMMP6OEDmHIVn7VjaFD58BibSeNz8rG8jKW4rmLWQ13GyFOAvRM+Ezl04B2y6rFI1Z d/qcOaIDlZKS7uh/XeV8jRf/Jg9Gv4nyd8vAeqJnnItexAZQc2OthrQV+pJg+KJ+eqJK RVyQ== X-Gm-Message-State: ACrzQf10/MDtuDspT/dZ87UlOhusz7dfm49qesQk786zRaqAj9YjcPJi oC61lCSoFPWl2YTXxTtqOSPihCK7j7I1w1UD3249K4bSX81WYX/YhS6G7b77irHiNILWOeCbkPm VjNQNpcEVlCxmxNC6RmiqZQ== X-Received: by 2002:a05:600c:1f18:b0:3b4:c4ae:f666 with SMTP id bd24-20020a05600c1f1800b003b4c4aef666mr6446655wmb.88.1665054384348; Thu, 06 Oct 2022 04:06:24 -0700 (PDT) X-Google-Smtp-Source: AMsMyM7A7VWO8tM/cnTF7ufuLTkNnCFl6Rowpa+u/X+FulAS8hj8Ui3Meem7bvesOboAeu/xO/x8PA== X-Received: by 2002:a05:600c:1f18:b0:3b4:c4ae:f666 with SMTP id bd24-20020a05600c1f1800b003b4c4aef666mr6446643wmb.88.1665054384110; Thu, 06 Oct 2022 04:06:24 -0700 (PDT) Received: from localhost (52.72.115.87.dyn.plus.net. [87.115.72.52]) by smtp.gmail.com with ESMTPSA id bh9-20020a05600005c900b0022ccbc7efb5sm18085358wrb.73.2022.10.06.04.06.23 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Thu, 06 Oct 2022 04:06:23 -0700 (PDT) From: Andrew Burgess To: Simon Marchi , gdb-patches@sourceware.org Subject: Re: [PATCH 2/2] gdb: more infrun debug from breakpoint.c In-Reply-To: <87edvlfii0.fsf@redhat.com> References: <129b33db9ac98dc806807d6595fbeec3e18edeb1.1664962429.git.aburgess@redhat.com> <2f051e15-d1df-25a4-e20e-bad58f8e3fa0@simark.ca> <87edvlfii0.fsf@redhat.com> Date: Thu, 06 Oct 2022 12:06:22 +0100 Message-ID: <87bkqpfbyp.fsf@redhat.com> MIME-Version: 1.0 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: text/plain X-Spam-Status: No, score=-10.3 required=5.0 tests=BAYES_00, DKIMWL_WL_HIGH, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, GIT_PATCH_0, RCVD_IN_BARRACUDACENTRAL, RCVD_IN_DNSWL_NONE, SPF_HELO_NONE, SPF_NONE, 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: Thu, 06 Oct 2022 11:06:50 -0000 Andrew Burgess writes: > Simon Marchi writes: > >> 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"); > > That does look better, but unfortunately, is not correct. The problem > is this line, about midway through the function: > > bs->stop = breakpoint_ext_lang_cond_says_stop (b); > > which invokes e.g. the Python Breakpoint.stop methods. It is possible > that this sets the stop field back to 0. > > Having looked at this a little, I now have a bunch of questions about > this code, like, prior to the quoted line we perform the thread and > frame checks for the breakpoint, but after the quoted line we perform > the condition and ignore count checks for the breakpoint. > > This means that for a breakpoint in the wrong thread/frame, the Python > stop method is never run. > > But, for a breakpoint with a condition or ignore count, the stop method > is run, and might choose to return True, indicating GDB should stop. > We'll then check the condition, or check the ignore count, and decide > that we should continue. > > This doesn't seem right. > > I'm tempted to suggest that either we should run the extension language > hooks first, before any other checks, so the hook is _always_ run, but > then update the docs to say that GDB might still choose to override a > stop request for a breakpoint. Or, we should run the extension language > hooks after all other checks, so the hook is _only_ run if GDB could > stop at this breakpoint. So, after a little more digging, it's not as bad as I thought. GDB will mostly stop a user trying to implement an extension language stop method, and add a condition to a breakpoint. So we don't need to worry about that case. The ignore count case is still open though. I think over time the ignore count implementation has drifted a little from the docs. The manual says: If a breakpoint has a positive ignore count and a condition, the condition is not checked. Once the ignore count reaches zero, @value{GDBN} resumes checking the condition. But I don't think that is true any longer. We always check the condition, and the ignore count will override the condition, but only when the condition would otherwise stop the breakpoint. Which I think makes sense? The ignore count is now ignoring the next N times the breakpoint _would_ have stopped, not just ignoring the breakpoint the next N times we hit it. Given this has been this way for a long time now, it's probably not a good idea to mess with it, and it probably makes sense for the extension language to match this behaviour. I think there's probably some documentation improvements that I could make in this area, but I don't think GDB itself needs to change. Thanks, Andrew