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 A9487384D1BE for ; Thu, 6 Oct 2022 08:45:15 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org A9487384D1BE 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-593-hNJHBYWBOM-UuM3ibdry8A-1; Thu, 06 Oct 2022 04:45:14 -0400 X-MC-Unique: hNJHBYWBOM-UuM3ibdry8A-1 Received: by mail-wm1-f71.google.com with SMTP id h129-20020a1c2187000000b003be5419a7e4so67940wmh.1 for ; Thu, 06 Oct 2022 01:45:14 -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=egYruHnGuGrEtzOtbQBDUVWhRX7AaxAO7BmD8yQhV/s=; b=WJt9N615ahWCUWx9mzi122F1SxctGYdjBIXQrIO4DtIl+9hxuHhqdGn7erLPA6Jw4D F8tycbB4xuOrIIb5e89sIk+8rkXx5LwLBpqe5E5lcUG0oR8V7czwjOajrRq0HIOgPep+ qm0dVHCHBwMEg1Asfb5B95/7qr9VhjqPx7Kj/ZbTTW5IzzxzgmEoKAnqQxEWrkTXzt/2 9mRhyOMFaUlHsZXIKWJKbjrlXU7G2fGnkQfEJ3XU4sUYUEgkE+obLJns0bAysRI+d83d 8WAnHPh9sNfgDYJjfK+LygjdarcH1qY1VWZZ2E8IElg1OBzH1D1ZQT/Zlz3+IAZ+zEGi hXCw== X-Gm-Message-State: ACrzQf3G+ynpkV82ex3Dxbd/VUZbege5uwFL5oYyecrOt2EX12DkwWpr VR4N+ryj3cvSt9wH6uR0Qfdl8tYC23QRdo+9OSR+F9PdDGrK0V3VRkQALz4JR77Kk83FR+l2JUx ovNBTyUSyPwBrK+UMFTvGQA== X-Received: by 2002:a05:600c:1987:b0:3b4:9b03:c440 with SMTP id t7-20020a05600c198700b003b49b03c440mr2235996wmq.14.1665045913051; Thu, 06 Oct 2022 01:45:13 -0700 (PDT) X-Google-Smtp-Source: AMsMyM7x3SzbGGB0G8diRnPgjN6V9hy1byyIbBgQ/9QU0mczUqxfoMBVCKOk4+SwrQN5hHUa8zvzdQ== X-Received: by 2002:a05:600c:1987:b0:3b4:9b03:c440 with SMTP id t7-20020a05600c198700b003b49b03c440mr2235982wmq.14.1665045912751; Thu, 06 Oct 2022 01:45:12 -0700 (PDT) Received: from localhost (52.72.115.87.dyn.plus.net. [87.115.72.52]) by smtp.gmail.com with ESMTPSA id n3-20020a1ca403000000b003b49bd61b19sm4181719wme.15.2022.10.06.01.45.12 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Thu, 06 Oct 2022 01:45:12 -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: <2f051e15-d1df-25a4-e20e-bad58f8e3fa0@simark.ca> References: <129b33db9ac98dc806807d6595fbeec3e18edeb1.1664962429.git.aburgess@redhat.com> <2f051e15-d1df-25a4-e20e-bad58f8e3fa0@simark.ca> Date: Thu, 06 Oct 2022 09:45:11 +0100 Message-ID: <87edvlfii0.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 08:45:17 -0000 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. Anyway. I've tweaked my original patches slightly and will push them, I'll follow up to this mail with what I'm actually pushing. Thanks, Andrew > > 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