From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-wr1-x444.google.com (mail-wr1-x444.google.com [IPv6:2a00:1450:4864:20::444]) by sourceware.org (Postfix) with ESMTPS id F30E13887008 for ; Tue, 14 Apr 2020 16:01:01 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.3.2 sourceware.org F30E13887008 Authentication-Results: sourceware.org; dmarc=none (p=none dis=none) header.from=embecosm.com Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=andrew.burgess@embecosm.com Received: by mail-wr1-x444.google.com with SMTP id t14so1730505wrw.12 for ; Tue, 14 Apr 2020 09:01:01 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=embecosm.com; s=google; h=date:from:to:cc:subject:message-id:references:mime-version :content-disposition:in-reply-to; bh=dsUR7RTtv8kE4r9xcl0WpxRpfdFpcgaqp0E1xEbjAGM=; b=DKOycYovfUBHv0Lt4h3s/Y20lOoaXUu0AL5eT8cpuWeegXP7KFMl0dy1KTyKz4K0eq m5LL3FkrNA7u0X0eNaF5t3D3qv4DUxKTyvN91OaynbbSKzcEi4/wsRorloHtSEbZIMV7 qp2B6iaY9xJ5d87gIgKmbAPToRNFgfN50gMKirQJgkaebcI9q0z0AImXwVt1/Vionoio pH30EbEz+tq7zaSycgUppRDYyhLk+bfF2FnoX+jGqCBicPWjpY2mpSTpADWZiOLlZpbz yjsNbob+9GDhS4SZZLFmhyt7Xf6CHQmh7CGMrntA39k5v7y+KQGPxiXtLKANaKh9Qv4v WUZw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:in-reply-to; bh=dsUR7RTtv8kE4r9xcl0WpxRpfdFpcgaqp0E1xEbjAGM=; b=SlyhgMyrMvQlr6mOuZH0hUEOBGxr5MpDO3wJ3kdN4wYvBZRmDJO0FsJRxQFDtE14BK v+5abbGFqFrARXf/15NucAH0SBQHBPqrEXFaa1BlezaVBvTPId/tLDopXhLiIIxDqCY5 8uc/7Sl/F+3lbVuxL8ChDrF8jtM+55/k0WaoQt4SlD3SVeeuM1qI5zaiVD9x7XLExT7R QPpiOTMWqRqK9nL2chA68pa9qG3JfIRvZe8lElB1fy34hdEvep4zAmJmh0YkWA+E7bK9 wTf2NKfadM9y0v8Os6tlDByMt9S0Ihzq0luw9EdG3Y7001oOVzwOz6Naljnecf5AUTMD QE6A== X-Gm-Message-State: AGi0PubzFx7TowwF1Hqjx1ug9ZBtmO06IMpaeatculfI1i6d+q7MUuKN gGbl0WXKsXdpilSIyS2YKTbBSQ== X-Google-Smtp-Source: APiQypKIkfgYCckFx5dW0CwmSbWRGeuQqD68OAKL/uC3oagrlroD26RP21ZALh1uUrwTbUd5XW6Org== X-Received: by 2002:adf:a406:: with SMTP id d6mr23899060wra.79.1586880060993; Tue, 14 Apr 2020 09:01:00 -0700 (PDT) Received: from localhost (host81-151-181-184.range81-151.btcentralplus.com. [81.151.181.184]) by smtp.gmail.com with ESMTPSA id o13sm20048349wrm.74.2020.04.14.09.01.00 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Tue, 14 Apr 2020 09:01:00 -0700 (PDT) Date: Tue, 14 Apr 2020 17:00:59 +0100 From: Andrew Burgess To: Simon Marchi Cc: Keno Fischer , gdb-patches@sourceware.org Subject: Re: [PATCH] breakpoint: Make sure location types match before swapping Message-ID: <20200414160059.GG2366@embecosm.com> References: <20200401013813.GA27550@juliacomputing.com> <372f95e1-4591-7e0d-90c7-168477e032ba@simark.ca> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <372f95e1-4591-7e0d-90c7-168477e032ba@simark.ca> X-Operating-System: Linux/5.5.13-200.fc31.x86_64 (x86_64) X-Uptime: 16:51:20 up 6 days, 7:06, X-Editor: GNU Emacs [ http://www.gnu.org/software/emacs ] X-Spam-Status: No, score=-24.5 required=5.0 tests=BAYES_00, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, GIT_PATCH_0, GIT_PATCH_1, GIT_PATCH_2, GIT_PATCH_3, RCVD_IN_DNSWL_NONE, SPF_HELO_NONE, SPF_PASS, TXREP autolearn=ham autolearn_force=no version=3.4.2 X-Spam-Checker-Version: SpamAssassin 3.4.2 (2018-09-13) 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: Tue, 14 Apr 2020 16:01:05 -0000 * Simon Marchi [2020-04-14 11:04:47 -0400]: > On 2020-04-14 3:01 a.m., Keno Fischer wrote: > > Bump. It would be great to get this fixed. > > > > On Tue, Mar 31, 2020 at 9:38 PM Keno Fischer wrote: > >> > >> This patch fixes PR gdb/25741 "GDB tries to set breakpoint using Z0, but remove it using z1". > >> In particular, what occurs in that case is that a hardware breakpoint is hit, > >> after which GDB removes it and establishes a single step breakpoint at the > >> same location. Afterwards, rather than simply removing this breakpoint and > >> re-enabling the hardware breakpoint, GDB simply swaps the activation, without > >> informing the server, leading to an inconsistency in GDB's view of the world > >> and the server's view of the world. To remidy this situation, this > >> patch adds a check that ensures two breakpoint locations have the > >> same type before they are considered equal and thus eligible for silent > >> swapping. > >> > >> gdb/ChangeLog: > >> * breakpoint.c (breakpoint_locations_match): Fix PR gdb/25741 > >> > >> Signed-off-by: Keno Fischer > >> --- > >> gdb/breakpoint.c | 2 +- > >> 1 file changed, 1 insertion(+), 1 deletion(-) > >> > >> diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c > >> index e49025461b..582dae1946 100644 > >> --- a/gdb/breakpoint.c > >> +++ b/gdb/breakpoint.c > >> @@ -6838,7 +6838,7 @@ breakpoint_locations_match (struct bp_location *loc1, > >> /* We compare bp_location.length in order to cover ranged breakpoints. */ > >> return (breakpoint_address_match (loc1->pspace->aspace, loc1->address, > >> loc2->pspace->aspace, loc2->address) > >> - && loc1->length == loc2->length); > >> + && loc1->length == loc2->length && loc1->loc_type == loc2->loc_type); > >> } > >> > >> static void > >> -- > >> 2.24.0 > >> > > I think the change makes sense, but this is not an area I know well, and it's one that > is a bit sensitive. I'll do a full test run and take a bit more time to look at it. > > In the mean time, if anybody else wants to take a look, go for it. I was able to reproduce this issue with a hacked up RISC-V target (forced software single step on for bare-metal) running on a development board I have access too. I agree that this fix feels like the right thing to do, it's inline with location checking done in watchpoint_locations_match. The only question I had when comparing to watchpoint_locations_match, is whether we should be similarly comparing the types of the owner breakpoint rather than the actual location. However, the b/p type is overloaded to contain more than just whether the breakpoint is h/w or s/w, so in this case we have the user h/w breakpoint, its type is bp_hardware_breakpoint, while the software single step breakpoint has type bp_single_step. The conclusion I came to is that it really is the type of the location we need to compare here. The other slight issue I had with the patch was that based on the original description I still had to go and figure out the exact conditions that would trigger this bug. I believe that to trigger this you need a h/w breakpoint placed on an instruction that loops to itself, I don't see how else this could happen. I took a crack at writing a more detailed break down of the conditions that trigger this bug. I'm still running this through testing here, but I'd be interested to hear if you think the fuller description is helpful. Thanks, Andrew --- >From d2b719b0f4857461064ed7b1da744a01b2ad2c6d Mon Sep 17 00:00:00 2001 From: Andrew Burgess Date: Tue, 14 Apr 2020 16:32:02 +0100 Subject: [PATCH] gdb: ensure location types match before swapping locations In the following conditions: - A target with hardware breakpoints available, and - A target that uses software single stepping, - An instruction at ADDRESS loops back to itself, Now consider the following steps: 1. The user places a hardware breakpoint at ADDRESS (an instruction that loops to itself), 2. The inferior runs and hits the breakpoint from #1, 3. The user tells GDB to 'continue'. In #3 when the user tells GDB to continue, GDB first disables the hardware breakpoint at ADDRESS, and then inserts a software single step breakpoint at ADDRESS. The original user created breakpoint was a hardware breakpoint, while the single step breakpoint will be a software breakpoint. GDB continues and immediately hits the software single step breakpoint. GDB then deletes the software single step breakpoint by calling delete_single_step_breakpoints, which eventually calls delete_breakpoint, which, once the breakpoint (and its locations) are deleted, calls update_global_location_list. During update_global_location_list GDB spots that we have an old location (the software single step breakpoint location) that is inserted, but being deleted, and a location at the same address which we are keeping, but which is not currently inserted (the original hardware breakpoint location), GDB then calls breakpoint_locations_match on these two locations. Currently the locations do match, and so GDB calls swap_insertion which swaps the "inserted" state of the two locations. GDB finally returns through the call stack and leaves delete_single_step_breakpoints. After this GDB continues with its normal "stopping" process, as part of this stopping process GDB removes all the breakpoints from the target. Due to the swap the original hardware breakpoint location is removed. The problem is that GDB inserted the software single step breakpoint as a software breakpoint, and then, thanks to the swap, tries to remove it as a hardware breakpoint. This will leave the target in an inconsistent state, and as in the original bug report (PR gdb/25741), could result in the target throwing an error. The solution for all this is to have two breakpoint locations of different types (hardware breakpoint vs software breakpoint) not match. The result of this is that GDB will no longer swap the inserted status of the two breakpoints. The original call to update_global_location_list (after deleting the software single step breakpoint) will at this point trigger a call to remove the breakpoint, something which will be done based on the type of that location. Later GDB will see that the original hardware breakpoint is already not inserted, and so will leave it alone. This patch was original proposed by Keno Fischer here: https://sourceware.org/pipermail/gdb-patches/2020-April/167202.html gdb/ChangeLog: PR gdb/25741 * breakpoint.c (breakpoint_locations_match): Compare location types. --- gdb/ChangeLog | 6 ++++++ gdb/breakpoint.c | 3 ++- 2 files changed, 8 insertions(+), 1 deletion(-) diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c index e49025461ba..2ab40a8e40a 100644 --- a/gdb/breakpoint.c +++ b/gdb/breakpoint.c @@ -6838,7 +6838,8 @@ breakpoint_locations_match (struct bp_location *loc1, /* We compare bp_location.length in order to cover ranged breakpoints. */ return (breakpoint_address_match (loc1->pspace->aspace, loc1->address, loc2->pspace->aspace, loc2->address) - && loc1->length == loc2->length); + && loc1->length == loc2->length + && loc1->loc_type == loc2->loc_type); } static void -- 2.25.2