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 0DF9F38582B2 for ; Fri, 23 Sep 2022 08:32:39 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 0DF9F38582B2 Received: from mail-ed1-f72.google.com (mail-ed1-f72.google.com [209.85.208.72]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_128_GCM_SHA256) id us-mta-625-INXZsN5AMmWWBmMtR8SL-w-1; Fri, 23 Sep 2022 04:32:37 -0400 X-MC-Unique: INXZsN5AMmWWBmMtR8SL-w-1 Received: by mail-ed1-f72.google.com with SMTP id v11-20020a056402348b00b004516e0b7eedso8329801edc.8 for ; Fri, 23 Sep 2022 01:32:37 -0700 (PDT) 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; bh=2JxSN5kX9wdxv6k9DvKR1KbBXGzMypxg6WKpRjtFZjo=; b=4Rnja4jiL7Y/tA3Ngjd6pUeSLyjXojS5XVay9TR+MDVcuDq3QbSZc83IcXe7Mh5Arn jWzcc6pLIT1nz3EgXxz/oJ5mcwNPxCFqE/hCNO5M9icQ0vAxHz9TFKIAz9IDhf2iAbjj M2hwkEh3FRQV/Dp9wrEr82BuMjdu7ZhHVE3cglkImX6ea//vUfc6MCGY8O2PKvwr2cBg ytXJ+Y535pgYPz0zLeRGQW7LOX2ob1Gd+QeLuFVxkd1oPw5AyUcLjiBc8KLPCLTNmjKw GX4iZXDmwQbWsHiSCaKYkO43FfcdIwiAZxLRsS1G5zFqAMgAnUBPuCx8s831i6slzV/6 7Caw== X-Gm-Message-State: ACrzQf3i+MQu/1+8JXi1X6oyii62RFF0rbHGtAQDzauxRaBRB2oEiq5B j0/GLo/viM0E0bwifI3Ltwv2p02meU5NbntTRSRbHzX7fNW6urQ3ZAI1WyOy2SDJ511a+OuYRRV icZ9TIvt627VvvqGNiBXTTA== X-Received: by 2002:a17:907:3e12:b0:780:523b:d9cf with SMTP id hp18-20020a1709073e1200b00780523bd9cfmr6179654ejc.326.1663921956527; Fri, 23 Sep 2022 01:32:36 -0700 (PDT) X-Google-Smtp-Source: AMsMyM5XUzec/TeF1CBb/mh3VrdhB1Ul8Jhw3M+b123fmADIxyVWN1SGQHcHZ5CdzwD1a+FVrNiWBQ== X-Received: by 2002:a17:907:3e12:b0:780:523b:d9cf with SMTP id hp18-20020a1709073e1200b00780523bd9cfmr6179635ejc.326.1663921956191; Fri, 23 Sep 2022 01:32:36 -0700 (PDT) Received: from [192.168.0.45] (ip-213-220-232-121.bb.vodafone.cz. [213.220.232.121]) by smtp.gmail.com with ESMTPSA id h17-20020a170906399100b0072f4f4dc038sm3692322eje.42.2022.09.23.01.32.35 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Fri, 23 Sep 2022 01:32:35 -0700 (PDT) Message-ID: <77260123-d1c5-c22f-1040-7401f22fb144@redhat.com> Date: Fri, 23 Sep 2022 10:32:34 +0200 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101 Thunderbird/102.2.1 Subject: Re: [PATCH v2] [PR python/18655] Fix deletion of FinishBreakpoints To: Johnson Sun , gdb-patches@sourceware.org References: <20220920172426.90659-1-j3.soon777@gmail.com> <20220923054123.78393-1-j3.soon777@gmail.com> From: Bruno Larsen In-Reply-To: <20220923054123.78393-1-j3.soon777@gmail.com> X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Language: en-US Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit X-Spam-Status: No, score=-12.7 required=5.0 tests=BAYES_00, DKIMWL_WL_HIGH, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, GIT_PATCH_0, KAM_SHORT, NICE_REPLY_A, RCVD_IN_DNSWL_LOW, 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: Fri, 23 Sep 2022 08:32:48 -0000 On 23/09/2022 07:41, Johnson Sun wrote: > This commit fixes the deletion of FinishBreakpoints by setting the disposition > to disp_del_at_next_stop instead, which gets cleaned up in > breakpoint_auto_delete. > > The original code fails since it does not correctly delete the temporary > FinishBreakpoints in `gdb/python/py-finishbreakpoint.c': > > /* Can't delete it here, but it will be removed at the next stop. */ > disable_breakpoint (bp_obj->bp); > gdb_assert (bp_obj->bp->disposition == disp_del); > > The code above aims to delete the breakpoint on the next hit by setting the > disposition to `disp_del'. However, the FinishBreakpoint is disabled, so it > will never be hit (thus never be deleted before the program stops). > > The fix only consists of a single line, setting the disposition to > `disp_del_at_next_stop': > > bp_obj->bp->disposition = disp_del_at_next_stop; > > The code above allows the breakpoint to be deleted in `breakpoint_auto_delete': > > for (breakpoint *b : all_breakpoints_safe ()) > if (b->disposition == disp_del_at_next_stop) > delete_breakpoint (b); > > A minimal test is added to verify this issue. > > Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=18655 Hi Johnson! Thanks for the new version of the patch! I think I wasn't quite clear in my first review, but we tend to try to explain the problem and solution as separate from the code as possible, because some times code can change drastically, but knowing the logic behind a previous solution might help with current problems. With this in mind, I would usually write a commit message like this: Currently, GDB creates FinishBreakpoints to execute the command finish, and these are meant to be temporary breakpoints. However, they are not being cleaned up after use, as reported in PR python/18655. This was happening because the disposition of the breakpoint was not being set correctly. This commit fixes this issue by correctly setting the disposition in the post stop hook of the breakpoint. It also adds a test to ensure this feature isn't regressed in the future. Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=18655 Feel free to change the wording to your preference, or to match better what GDB actually does (this is my first time even hearing about FinishBreakpoints), but I do think that this style of explanation would be more beneficial when referring back to this commit in the future. I also have 2 minor comments below. > --- > gdb/python/py-finishbreakpoint.c | 1 + > .../py-finish-breakpoint-deletion.c | 31 +++++++++++++ > .../py-finish-breakpoint-deletion.exp | 43 +++++++++++++++++++ > .../py-finish-breakpoint-deletion.py | 33 ++++++++++++++ > 4 files changed, 108 insertions(+) > create mode 100644 gdb/testsuite/gdb.python/py-finish-breakpoint-deletion.c > create mode 100644 gdb/testsuite/gdb.python/py-finish-breakpoint-deletion.exp > create mode 100644 gdb/testsuite/gdb.python/py-finish-breakpoint-deletion.py > > diff --git a/gdb/python/py-finishbreakpoint.c b/gdb/python/py-finishbreakpoint.c > index c80096f6810..a219bc82f15 100644 > --- a/gdb/python/py-finishbreakpoint.c > +++ b/gdb/python/py-finishbreakpoint.c > @@ -146,6 +146,7 @@ bpfinishpy_post_stop_hook (struct gdbpy_breakpoint_object *bp_obj) > /* Can't delete it here, but it will be removed at the next stop. */ > disable_breakpoint (bp_obj->bp); > gdb_assert (bp_obj->bp->disposition == disp_del); > + bp_obj->bp->disposition = disp_del_at_next_stop; I think you can remove the assert. There is no point checking the previous disposition if you'll rewrite it in the next line IMHO. > } > catch (const gdb_exception &except) > { > diff --git a/gdb/testsuite/gdb.python/py-finish-breakpoint-deletion.c b/gdb/testsuite/gdb.python/py-finish-breakpoint-deletion.c > new file mode 100644 > index 00000000000..258c8dc3861 > --- /dev/null > +++ b/gdb/testsuite/gdb.python/py-finish-breakpoint-deletion.c > @@ -0,0 +1,31 @@ > +/* This testcase is part of GDB, the GNU debugger. > + > + Copyright 2022 Free Software Foundation, Inc. > + > + This program is free software; you can redistribute it and/or modify > + it under the terms of the GNU General Public License as published by > + the Free Software Foundation; either version 3 of the License, or > + (at your option) any later version. > + > + This program is distributed in the hope that it will be useful, > + but WITHOUT ANY WARRANTY; without even the implied warranty of > + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the > + GNU General Public License for more details. > + > + You should have received a copy of the GNU General Public License > + along with this program. If not, see . */ > + > +int > +subroutine (int a) > +{ > + return a; > +} > + > +int > +main () > +{ > + int i; > + for (i = 0; i < 5; i++) > + subroutine(i); space before the ( Cheers, Bruno