From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-pf1-x436.google.com (mail-pf1-x436.google.com [IPv6:2607:f8b0:4864:20::436]) by sourceware.org (Postfix) with ESMTPS id D403F3858D32 for ; Mon, 23 Jan 2023 10:15:07 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org D403F3858D32 Authentication-Results: sourceware.org; dmarc=pass (p=none dis=none) header.from=gmail.com Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=gmail.com Received: by mail-pf1-x436.google.com with SMTP id x4so8384483pfj.1 for ; Mon, 23 Jan 2023 02:15:07 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20210112; h=content-transfer-encoding:in-reply-to:from:references:to :content-language:subject:user-agent:mime-version:date:message-id :from:to:cc:subject:date:message-id:reply-to; bh=WKbGKAGCB9nitn/jGJPiPdnc1w1nkBkd9hCG2DRN4MY=; b=icUGiz5NtfpzJUoytZcFDYmmB2SFgtZjOB+CW/7eyhh3IXFFuA0YeOaPZkM4M+mIAH rQ0Vi0uCMpb9za5vmDJ30OAjbCgqNfpDeV+TXhVyJqf8qUH3+5ZZB46En6AFLtwRH20P ZzIzgUZMh6KgPeqz+5Ntf0jGlf+Vk7V8dVWz7ub8CY8gbnSo5ZigD9OKJzAzSWBSl2Nx 12wOG+h/FufSJKywW2USUVwuLLtdZ2p0U4SPcAkkt39vclX0xGmo1KIRcQR3uKjySz1K k60RmqKvIBxWXdXdOdafi7nLRA1BZkqHJIn7MK08DXD/vuIfnMfRDyR8d8b6iIM62Y/4 G3tQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=content-transfer-encoding:in-reply-to:from:references:to :content-language:subject:user-agent:mime-version:date:message-id :x-gm-message-state:from:to:cc:subject:date:message-id:reply-to; bh=WKbGKAGCB9nitn/jGJPiPdnc1w1nkBkd9hCG2DRN4MY=; b=flHKZotWd6h2MhF6y3/QetYPz+Tsk9dlSN7jwjPUcuPiQGSy4Jo0yxPCk/MasAWMZG 9ErblbR8050ZukhVmkhMeDultZNMeDJaXqHIRRTN2XyUSm4qWnYThKvAF/4PBbCqS0jO j+pWbmq3hCBWhy/6X6h8H91rhTJzrGpd+yl8LUL2JuLEMLituyR5is8jA/qUeQl/QPnR +999vC+wycdwbDpK0m5ouft3VqEN1IBMTYOXXFFVx/0m9hxX/xUiHAldNTa3U0A8H6eV RNYDdR9TfDK6najcHZrkGWdt04VlcL3SOF8f/DuxUDM2hAtGN2fzr2EfXpNq/UHACanP E5Mg== X-Gm-Message-State: AFqh2kpgr4oNcTGQPrgbWY4VAm1UEHbpVNoqJplsOWIiO1IXE+kUVKIn lAtZa0CZgFD9sv0mQHa6+d2W9t1ACAuPsQ== X-Google-Smtp-Source: AMrXdXsCYohryLDBrIxIpHro5oY+EcYHC/goJjWPg55mWdMQNEnGtra2alOW6rpRx4nxL0as7q8utA== X-Received: by 2002:a62:ea0e:0:b0:577:d10d:6eab with SMTP id t14-20020a62ea0e000000b00577d10d6eabmr25260864pfh.21.1674468906681; Mon, 23 Jan 2023 02:15:06 -0800 (PST) Received: from [192.168.2.101] (61-220-36-70.hinet-ip.hinet.net. [61.220.36.70]) by smtp.gmail.com with ESMTPSA id b75-20020a621b4e000000b0058ba53aaa75sm20935459pfb.99.2023.01.23.02.15.05 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Mon, 23 Jan 2023 02:15:06 -0800 (PST) Message-ID: Date: Mon, 23 Jan 2023 18:15:03 +0800 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:102.0) Gecko/20100101 Thunderbird/102.6.1 Subject: Re: [PATCH v3] [PR python/29603] Disable out-of-scope watchpoints Content-Language: en-US To: SimonMarchi , LancelotSIX , gdb-patches@sourceware.org References: <20221020175707.193041-1-j3.soon777@gmail.com> <20221020181101.193226-1-j3.soon777@gmail.com> <9e6aa582-efd6-b5bf-d4b9-aebeb2e9e845@simark.ca> From: Johnson Sun In-Reply-To: <9e6aa582-efd6-b5bf-d4b9-aebeb2e9e845@simark.ca> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 8bit X-Spam-Status: No, score=-10.7 required=5.0 tests=BAYES_00,BODY_8BITS,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,FREEMAIL_ENVFROM_END_DIGIT,FREEMAIL_FROM,GIT_PATCH_0,NICE_REPLY_A,RCVD_IN_DNSWL_NONE,SPF_HELO_NONE,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 List-Id: Hi, Thanks for reviewing this! > the supplied test does not pass for me The current test doesn't seem to produce consistent results across different machines, I'll try to fix it in the next version. > Is there a reason to do it here in particular, and not, let's say as > soon as we change the disposition to disp_del_at_next_stop? I have implemented this in the v1 patch, I called `disable_breakpoint' as soon as we change the disposition to `disp_del_at_next_stop' (in `watchpoint_del_at_next_stop'). However, LancelotSIX mentioned that the fix is in a non-extension-related code path, and suggested disabling it in `bpstat_check_breakpoint_conditions' instead (the v3 patch). See: https://sourceware.org/pipermail/gdb-patches/2022-September/192120.html Both the v1 and v3 patch fixes the issue. I personally prefer the v1 patch. (i.e., modifying `watchpoint_del_at_next_stop') > shouldn't marking the watchpoint as > disp_del_at_next_stop make it so it won't be inserted next time we > resume?  After all should_be_inserted returns false for breakpoint > locations that are disp_del_at_next_stop.  Perhaps it's because since we > don't do a "normal" stop, breakpoint locations stay inserted, and > there's nothing that pulls this location out of the target? Therefore, > maybe a solution, to keep things coherent, would be to pull the > breakpoint locations out when marking the breakpoint as > disp_del_at_next_stop?  This way, we would respect the invariant that a > breakpoint disp_del_at_next_stop can't be inserted (I don't know if this > is really what is happening though, it's just speculation). For software watchpoints, they cannot be pulled out of the target, since they may not be inserted into the target in the first place:      /* Locations of type bp_loc_other and     bp_loc_software_watchpoint are only maintained at GDB side,     so there is no need to remove them.  */     -- gdb/breakpoint.c:3840 Their expressions are re-evaluated by single-stepping through the program:     Software watchpoints are very slow, since GDB needs to single-step     the program being debugged and test the value of the watched     expression(s) after each instruction.     -- https://sourceware.org/gdb/wiki/Internals%20Watchpoints Therefore, we must either disable or delete the software watchpoint. We cannot pull it out of the target since it's only maintained on the GDB side. > And, in a broader scope, why wait until the next normal stop to delete > the watchpoint?  A "normal" stop being a stop that we present to the > user (the normal_stop function).  We currently call > breakpoint_auto_delete in normal_stop, which is why we don't reach it > when your Breakpoint.stop method returns False.  At the end of, let's > say, handle_signal_stop, could we go over the bpstat chain and delete > any breakpoint we've just hit that is marked for deletion? I believe this choice is due to performance issues. In an early attempt, I tried to call `breakpoint_auto_delete' on all kinds of stops. However, this implementation is very slow when we have a large number of breakpoints, since we need to go over the entire bpstat chain on all stops. I recall that this implementation fails on certain testcases with a large number of breakpoints with many breakpoint stops. Furthermore, the reason for using the `disp_del_at_next_stop' state remains unclear, as mentioned in the comments:     /* NOTE drow/2003-09-08: This state only exists for removing        watchpoints.  It's not clear that it's necessary...  */     -- gdb/breakpoint.c:2914 By tracing the git history, the `disp_del_at_next_stop' state is introduced in commit c906108c21474dfb4ed285bcc0ac6fe02cd400cc, which doesn't provide any proper explanation of this state. I think the best way to deal with this is to avoid going over the entire bpstat chain when deleting breakpoints. Potentially by keeping track of a chain of breakpoints that should be deleted, and changing the bpstat chain to a doubly linked list for the ease of deletion. Such changes deserve a dedicated patch, though. To sum up, I prefer modifying `watchpoint_del_at_next_stop' (i.e., the v1 patch). If you also think it's appropriate, I'll fix the failing test and prepare the v4 patch accordingly. Johnson On 1/13/2023 11:40 PM, SimonMarchi wrote: > > On 10/20/22 14:11, Johnson Sun via Gdb-patches wrote: >> Currently, when a local software watchpoint goes out of scope, GDB sets the >> watchpoint's disposition to `delete at next stop' and then normal stops >> (i.e., stop and wait for the next GDB command). When GDB normal stops, it >> automatically deletes the breakpoints with their disposition set to >> `delete at next stop'. >> >> Suppose a Python script decides not to normal stop when a local software >> watchpoint goes out of scope, the watchpoint will not be automatically >> deleted even when its disposition is set to `delete at next stop'. >> >> Since GDB single-steps the program and tests the watched expression after each >> instruction, not deleting the watchpoint causes the watchpoint to be hit many >> more times than it should, as reported in PR python/29603. >> >> This was happening because the watchpoint is not deleted or disabled when >> going out of scope. >> >> This commit fixes this issue by disabling the watchpoint when going out of >> scope. It also adds a test to ensure this feature isn't regressed in the >> future. >> >> Two other solutions seem to solve this issue, but are in fact inappropriate: >> 1. Automatically delete breakpoints on all kinds of stops >> (in `fetch_inferior_event'). This solution is very slow since the deletion >> requires O(N) time for N breakpoints. >> 2. Disable the watchpoint after the watchpoint's disposition is set to >> `delete at next stop' (in `watchpoint_del_at_next_stop'). This solution >> modifies a non-extension-related code path, and isn't preferred since this >> issue cannot occur without extension languages. (gdb scripts always normal >> stop before continuing execution) > I have a bit of trouble reviewing this, because I'm not too familiar > with the lifecycle of watchpoints (I know the principle, but not the > specifically where things happen in GDB). So it's difficult to tell > whether this is right or if there's a better way to handle it. > > However, the supplied test does not pass for me: > > source /home/simark/build/binutils-gdb/gdb/testsuite/outputs/gdb.python/py-watchpoint/py-watchpoint.py > Watchpoint 2: i > Python script imported > (gdb) PASS: gdb.python/py-watchpoint.exp: import python scripts > python print(len(gdb.breakpoints())) > 2 > (gdb) PASS: gdb.python/py-watchpoint.exp: check modified BP count > gdb_expect_list pattern: /Watchpoint Hit: ./ > continue > Continuing. > Watchpoint Hit: 1 > gdb_expect_list pattern: /[ > ]+Watchpoint . deleted because the program has left the block in/ > FAIL: gdb.python/py-watchpoint.exp: run until program stops (pattern 2) (timeout) > gdb_expect_list pattern: /[ > ]+which its expression is valid./ > gdb_expect_list pattern: /Watchpoint Hit: ./ > gdb_expect_list pattern: /[ > ]+012\[Inferior 1 \(process .*\) exited normally\]/ > gdb_expect_list pattern: // > python print(len(gdb.breakpoints())) > Watchpoint Hit: 2 > Watchpoint Hit: 3 > Watchpoint Hit: 4 > > Watchpoint 2 deleted because the program has left the block in > which its expression is valid. > Watchpoint Hit: 5 > 012[Inferior 1 (process 2381681) exited normally] > (gdb) FAIL: gdb.python/py-watchpoint.exp: check BP count (the program exited) > >> Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=29603 >> --- >> gdb/breakpoint.c | 2 + >> gdb/testsuite/gdb.python/py-watchpoint.c | 27 ++++++++++++ >> gdb/testsuite/gdb.python/py-watchpoint.exp | 48 ++++++++++++++++++++++ >> gdb/testsuite/gdb.python/py-watchpoint.py | 30 ++++++++++++++ >> 4 files changed, 107 insertions(+) >> create mode 100644 gdb/testsuite/gdb.python/py-watchpoint.c >> create mode 100644 gdb/testsuite/gdb.python/py-watchpoint.exp >> create mode 100644 gdb/testsuite/gdb.python/py-watchpoint.py >> >> diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c >> index bff3bac7d1a..15f4ae2131c 100644 >> --- a/gdb/breakpoint.c >> +++ b/gdb/breakpoint.c >> @@ -5340,6 +5340,8 @@ bpstat_check_breakpoint_conditions (bpstat *bs, thread_info *thread) >> /* Evaluate extension language breakpoints that have a "stop" method >> implemented. */ >> bs->stop = breakpoint_ext_lang_cond_says_stop (b); >> + if (b->disposition == disp_del_at_next_stop) >> + disable_breakpoint(b); > Is there a reason to do it here in particular, and not, let's say as > soon as we change the disposition to disp_del_at_next_stop? > > Other question: shouldn't marking the watchpoint as > disp_del_at_next_stop make it so it won't be inserted next time we > resume? After all should_be_inserted returns false for breakpoint > locations that are disp_del_at_next_stop. Perhaps it's because since we > don't do a "normal" stop, breakpoint locations stay inserted, and > there's nothing that pulls this location out of the target? Therefore, > maybe a solution, to keep things coherent, would be to pull the > breakpoint locations out when marking the breakpoint as > disp_del_at_next_stop? This way, we would respect the invariant that a > breakpoint disp_del_at_next_stop can't be inserted (I don't know if this > is really what is happening though, it's just speculation). > > And, in a broader scope, why wait until the next normal stop to delete > the watchpoint? A "normal" stop being a stop that we present to the > user (the normal_stop function). We currently call > breakpoint_auto_delete in normal_stop, which is why we don't reach it > when your Breakpoint.stop method returns False. At the end of, let's > say, handle_signal_stop, could we go over the bpstat chain and delete > any breakpoint we've just hit that is marked for deletion? > > Simon >