From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-wr1-x443.google.com (mail-wr1-x443.google.com [IPv6:2a00:1450:4864:20::443]) by sourceware.org (Postfix) with ESMTPS id 0DBFF385DC1E for ; Wed, 15 Apr 2020 20:46:47 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.3.2 sourceware.org 0DBFF385DC1E 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-x443.google.com with SMTP id j2so1794516wrs.9 for ; Wed, 15 Apr 2020 13:46:47 -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=5+2snhRnBWdxn01tIEPH8uuHil3Y6ICtg1PPEyB59nc=; b=Q4MwSghq9isu31d8kiVHLSrtGWXsFuRYSjpk3nHMF6DrFCCfWAAHaGcCS8BbyBo4ig L3wMZlf3CpAAOKikqiWvudTKqHrjWHOl3TRVZhvzKvw4+E+yZ6GEfxZak8AhRYFp0WsS BQ/A19IPmFnnNJ9tY3mcUjYEcgscnhnLFwSNzL71O+xrovwA7k3ScXDtM9MD9a9kjK3I 7dzFICXCrRzoEI8vwPlbBEpkp1iP6/Ub5gOsr4UHroPLyEqepQa8wA8jkUws3nYXBzQt a+8p1JYYl8W7bs9VV7k6NaA3kK+T2AFD5osPzLtXC1dCZKEdFvxwrSLUSeO58gge/G7V uw4A== 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=5+2snhRnBWdxn01tIEPH8uuHil3Y6ICtg1PPEyB59nc=; b=FrIMwzDCIQzylaFfkE/zCzC2A6YPyr6hAQBXGY4Ij/JMPeUx9jvfHiD2c/Y3AlDy/w vBDrczAndgbrbXa6DDzlTc5Amqkn0ntYlbmOgz799MhgWFQwrLa8BEL1hEXgM9BtoV4i y0kBXZqhVhNhPhpa8imKVp17F1HDzvJjP4LNneTqPpoYk87ZCtxJ4bq3hAq7H67m4+4G 1OqqPAmkRSE5RxqH6f5cQ3XvGqjhCHaQcoZl8pbIeSIMKHmx+LqNPxmik4CEllCiKA7C gUP15UvuSQhQadZl0lwpswJBBnpj+01s0woKYORwPz7y0OKXkVxldwg+SFsjdz1Zyi1p i/hA== X-Gm-Message-State: AGi0Pua1JHq2T5p7LKDRnELrvI0xDAoUkoUL+LzLE3pp8cO9PMJZ+m1a pW/tWTzLcbkhKtCgZmv2+EvBug== X-Google-Smtp-Source: APiQypLm5lTD45DVSZYBIWESW545baXL0PwS/BsF/ZLgER71s/5Sxl3PL2jdOyPqFRCkrKMQxnsrww== X-Received: by 2002:adf:82a6:: with SMTP id 35mr27767515wrc.378.1586983606107; Wed, 15 Apr 2020 13:46:46 -0700 (PDT) Received: from localhost (host81-151-181-184.range81-151.btcentralplus.com. [81.151.181.184]) by smtp.gmail.com with ESMTPSA id p16sm18289491wro.21.2020.04.15.13.46.44 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Wed, 15 Apr 2020 13:46:44 -0700 (PDT) Date: Wed, 15 Apr 2020 21:46:42 +0100 From: Andrew Burgess To: Pedro Alves Cc: Simon Marchi , Keno Fischer , gdb-patches@sourceware.org Subject: Re: [PATCH] breakpoint: Make sure location types match before swapping Message-ID: <20200415204642.GH2222@embecosm.com> References: <20200401013813.GA27550@juliacomputing.com> <372f95e1-4591-7e0d-90c7-168477e032ba@simark.ca> <20200414160059.GG2366@embecosm.com> <41e51f15-6729-ccbf-7833-3a621006cdbf@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <41e51f15-6729-ccbf-7833-3a621006cdbf@redhat.com> X-Operating-System: Linux/5.5.13-200.fc31.x86_64 (x86_64) X-Uptime: 21:41:14 up 7 days, 11:55, X-Editor: GNU Emacs [ http://www.gnu.org/software/emacs ] X-Spam-Status: No, score=-11.3 required=5.0 tests=BAYES_00, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, 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: Wed, 15 Apr 2020 20:46:49 -0000 * Pedro Alves [2020-04-14 20:17:00 +0100]: > On 4/14/20 5:00 PM, Andrew Burgess wrote: > > > 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. > > It is. > > > 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. > > This seems right at first blush, though there are some things > that we need to look at. Here are 3 cases that found while > looking for breakpoint_locations_match callers: > > #1 > > E.g., with this, GDB will now install both a hw breakpoint location > and a software location at the same address. E.g., a contrived case > to see it happen would be, with: > > (gdb) set breakpoint always-inserted on > (gdb) set debug remote 1 > > before: > > (gdb) hbreak main > Sending packet: $m400736,1#fe...Packet received: 48 > Hardware assisted breakpoint 1 at 0x400736: file threads.c, line 57. > Sending packet: $Z1,400736,1#48...Packet received: OK > (gdb) b main > Note: breakpoint 1 also set at pc 0x400736. > Sending packet: $m400736,1#fe...Packet received: 48 > Breakpoint 2 at 0x400736: file threads.c, line 57. > Sending packet: $Z1,400736,1#48...Packet received: OK > > after: > > (gdb) hbreak main > Sending packet: $m400736,1#fe...Packet received: 48 > Hardware assisted breakpoint 1 at 0x400736: file threads.c, line 57. > Sending packet: $Z1,400736,1#48...Packet received: OK > (gdb) break main > Note: breakpoint 1 also set at pc 0x400736. > Sending packet: $m400736,1#fe...Packet received: 48 > Breakpoint 2 at 0x400736: file threads.c, line 57. > Sending packet: $Z1,400736,1#48...Packet received: OK > Sending packet: $Z0,400736,1#47...Packet received: OK > > This is likely not to cause a problem, but it's worth it to consider. > > #2 > > Another thing to consider is the automatic hardware breakpoints > support. See insert_bp_location. That means that breakpoint > locations can start as software breakpoints but still end up > inserted as hw breakpoints. Similarly to #1 above, without the patch, > gdb is considering those locations as duplicates, and after the > patch it no longer will. So we may end up with multiple insertions > for the same location. Again, similar to #1. > > #3 > > I suspect this the (automatic hardware breakpoints) can interfere with > e.g., update_breakpoint_locations 's logic of matching old locations > with new locations, in the have_ambiguous_names case. I.e., after > insertion the locations are hw breakpoint locations, but after > a breakpoint re-set, the new locations will be software breakpoint > locations until they try to be inserted. Before the patch, the > disabled state will still be carried over. After the patch, they won't. > I think. > > Maybe we need to explicitly consider the case in > update_breakpoint_locations. I'll take another look at this code, but it sounds almost like we should split breakpoint_locations_match in two, we have: 1. match (v1) - two breakpoints at the same address match regardless of type, this would be used when deciding if we should insert bp_location 1, 2, or both. 2. match (v2) - when one breakpoint is already inserted, can another breakpoint "take over" its inserted status. It's almost as though we want: bool breakpoint_locations_swapable ( .... ) { return (breakpoint_locations_match ( .... ) && loc1->type == loc2->type); } Maybe? I'll take another look at this when I can. Thanks, Andrew