From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-pg1-x534.google.com (mail-pg1-x534.google.com [IPv6:2607:f8b0:4864:20::534]) by sourceware.org (Postfix) with ESMTPS id E75183858D32 for ; Sat, 18 Feb 2023 16:26:36 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org E75183858D32 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-pg1-x534.google.com with SMTP id k15so550269pgg.5 for ; Sat, 18 Feb 2023 08:26:36 -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=/c5yFeRh+R2k/jSNxXoQRu1S8pULN3AAGdL5I4sAjPQ=; b=Mfj/JmJvaO+0NBTl+9iZPKNwuL5dVHO+5D84DWRXwC3tOOPyBN8vkRsBuR+xQ84XUi 3iKUpHRJlg5IVWU2rz0f12LYfNT7rGqi/CfSkgYJZ30KqA2APuQRdpt6od7rU0M4dY7d 82I+YkCysHJ6OoBK7UfvYRSvDsFyNWybMwIalsBbeE0pCTCCI8bs6N0Y8NM5QRtBDI3N Ehj3zFwoCX3pNzVjP+bf14mu+smKqhLYy2DohVDlESdtLZAfxyIecyftJ0nxRG3lOeEB 8Foxwj7eHa3xaVtAzGOpp/Ms9XHe9XGyon9SiWIrFLv9YFtij+dU1YgeZze2nxoSFayu 128g== 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=/c5yFeRh+R2k/jSNxXoQRu1S8pULN3AAGdL5I4sAjPQ=; b=1K/GMwrhE/iEYkjN/KQFOB5W1I235MTVQdI0KQdGv9bsxtfuaz+S5erPR6jA5+1u/4 BJLxpsdldyQ+PvjW3oT2jQ0eyvDeYPIWdY9/54PnACNq0U73ZsiajT0v6ddJeurnMz8Z sZCA20aZ5vJMe/O2CFJS3tNayl/sJ8tLml3SR1bJXpWhN3/29Wehs1BZVetOUoundken 2cf0Ncwq6EuyaXrvn3+t+EMSA14Z62x/vgobzbGs0P4ZxjJxuZ1/o5t8ynC6DmUcRAsI FJjM8kByL/i7Ce3jnLrln8H5BaweTmXWukixpq83ziJAy5FOY1T6mwj8kwAd2nT1dJj/ JxQw== X-Gm-Message-State: AO0yUKUTPNBep9/wzqOo39rCI/1Qs2QGacJHFt9+zcYXjc9RBEXirrvv nONY7vCgfYlEVXUGLOjq0Qs= X-Google-Smtp-Source: AK7set8ojn/IQnc5ZPN0eZZ0KJw06lGul+68/9mMuNbJ91WvNWdIQWhAK91QEVO9vfYHuH1rx8zMpQ== X-Received: by 2002:aa7:96d0:0:b0:5aa:2216:5c59 with SMTP id h16-20020aa796d0000000b005aa22165c59mr5942475pfq.34.1676737595771; Sat, 18 Feb 2023 08:26:35 -0800 (PST) Received: from [192.168.117.117] (sd237023.ching-de.ab.nthu.edu.tw. [140.114.237.23]) by smtp.gmail.com with ESMTPSA id e25-20020aa78c59000000b005a87d636c70sm4836331pfd.130.2023.02.18.08.26.33 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Sat, 18 Feb 2023 08:26:34 -0800 (PST) Message-ID: <915dd1be-feb5-967d-2d32-af61c3fee9a7@gmail.com> Date: Sun, 19 Feb 2023 00:26:32 +0800 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:102.0) Gecko/20100101 Thunderbird/102.7.2 Subject: [PING] [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> Content-Language: en-US In-Reply-To: Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 8bit X-Spam-Status: No, score=-10.3 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: 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 >>