From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-pl1-x62a.google.com (mail-pl1-x62a.google.com [IPv6:2607:f8b0:4864:20::62a]) by sourceware.org (Postfix) with ESMTPS id 307373858D3C for ; Sun, 26 Feb 2023 06:16:41 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 307373858D3C 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-pl1-x62a.google.com with SMTP id n6so2257651plf.5 for ; Sat, 25 Feb 2023 22:16:41 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20210112; h=content-transfer-encoding:in-reply-to:content-language:references :to:from:subject:user-agent:mime-version:date:message-id:from:to:cc :subject:date:message-id:reply-to; bh=VAvocpLuZjEUlqdQ9z7oXJMsJVE1E/OjfwoqKhwlhj8=; b=ffqsE0BuCHrCSijR9Lcfqams4z03ZAQEYFwmLyY4tH3iGk6m1PgLcoHapmQl4Xgulp au3MoVQqZswgneci9mQyVEW1Xf8uWDRSHh9wdOi8qfs4FyYo4uLc9f/0zCLkPBop3jVj j1qDEA6wgZTVYgffSfcKuFkBQTTKoJu4j/rJYOVN8cYjA04Tc87V6gmwYwuHGoKHQKNo ooyLc5NuXCHue/zd4LenfxWBEa/rZ52FJUlt12FPE9b/E99IVwqTVa7o6mbtNowNYJNE qLpIb2i27Y9o89i8SfqQ/EHkNyGarE03j/funvhaJW1gbzG0x6zKUmCkzSPxUxHMBiJK IEJg== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=content-transfer-encoding:in-reply-to:content-language:references :to:from:subject:user-agent:mime-version:date:message-id :x-gm-message-state:from:to:cc:subject:date:message-id:reply-to; bh=VAvocpLuZjEUlqdQ9z7oXJMsJVE1E/OjfwoqKhwlhj8=; b=EQG9cjniUnVqDt/CHkQlJodC5qFNVQ9Q8M6ip5C7bBW9+EnaKvUdE3+kGER+vmAeRM CnbP/pC0+i86/C5N1CFb89lnK0ljRzOb34QI68815uf2vbRSFYgaWYV8pupJzdBZgFfl uyfdY8Pg1VF0P1lxTrn4M7MunCGt16x440vi8icJBho0VpfH7Wcl4alcg3DqjTQxKWMV 2Ea43pn8LXcWMzpPV3DwgqRcySEJlcEGvICB3f4coiGYDyQqv72rX9pOKJkgYZaI+2zE Auxad5ocxphPFaW9OMaw23O2IW5jUvdBnGckgc+YHAdUckLC1Wx/w/Vq8eV0TeiUPyP5 fDAw== X-Gm-Message-State: AO0yUKUBXevbACpe1bjjdhBafFlubTUOZgovESxVuOK6jtFZxhbn1MSR KfcfmS5l5dnq65SmH85BIqYGUivagqVplw== X-Google-Smtp-Source: AK7set/1xdbo2MOmfSy6cE2Gw3K3aps8Tj286i4BlEDR9VFzQkflkidWEzzf+2cvRPyRw4+a3WQk9w== X-Received: by 2002:a05:6a20:4da4:b0:cb:6f9c:166d with SMTP id gj36-20020a056a204da400b000cb6f9c166dmr18567411pzb.43.1677392199968; Sat, 25 Feb 2023 22:16:39 -0800 (PST) Received: from [192.168.2.102] (61-220-36-70.hinet-ip.hinet.net. [61.220.36.70]) by smtp.gmail.com with ESMTPSA id bt1-20020a632901000000b004fbd021bad6sm1798750pgb.38.2023.02.25.22.16.38 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Sat, 25 Feb 2023 22:16:39 -0800 (PST) Message-ID: <6f2d7693-7f29-c125-1b7a-44a8a4fbd76e@gmail.com> Date: Sun, 26 Feb 2023 14:16:37 +0800 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:102.0) Gecko/20100101 Thunderbird/102.8.0 Subject: [RFC] [PATCH v3] [PR python/29603] Disable out-of-scope watchpoints From: Johnson Sun 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> <915dd1be-feb5-967d-2d32-af61c3fee9a7@gmail.com> Content-Language: en-US In-Reply-To: <915dd1be-feb5-967d-2d32-af61c3fee9a7@gmail.com> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 8bit X-Spam-Status: No, score=-10.1 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,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: Request for comments: . Johnson On 2/19/2023 12:26 AM, JohnsonSun wrote: > Ping for: > . > > I would like to ask for some feedback before submitting the v4 patch. > > Johnson > > On 1/23/2023 6:15 PM, Johnson Sun wrote: >> 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 >>> >