From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-wr1-x441.google.com (mail-wr1-x441.google.com [IPv6:2a00:1450:4864:20::441]) by sourceware.org (Postfix) with ESMTPS id D03CD385C426 for ; Fri, 17 Apr 2020 12:28:43 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.3.2 sourceware.org D03CD385C426 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-x441.google.com with SMTP id j2so2813730wrs.9 for ; Fri, 17 Apr 2020 05:28:43 -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=8j3AHdLEKQUuffN2a3h6bZ301CQOgpvvlJo0ADEg8BA=; b=awYZQxTOiEfUUgvNSM073NQQcTEpf6VKGSBTrlNoIN9x+rKAinQE90r10XDhH5Ixlg SE5E8BQ6lRhWwI7lU3FI+/a0jJPjcyBStJwXhoi+A7j9xz9vjkFMepKm06EqvpKf0HKU WViVf5Bmlba8CN0tvBlCZcjudpeM8750YSBUsWxE7OlaLHzPJsUIOZ6KxlWMBaANDKDe hilKhki90BS/4ADjgs5Au2cpbb5aKb6lbPA6wXadNHm+nvv7mcSB1tmv8H9kpOp/L6V4 7Mz9TtHX10i0S9YpO2KE3r0OGF97D5ALJCg1nhwG3oq3En9Sc6+A5Zv4G2z6ClCrrg5v zVXw== 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=8j3AHdLEKQUuffN2a3h6bZ301CQOgpvvlJo0ADEg8BA=; b=LJdbKMSDC5SHKKQssW8hLYd5JyNbrQfH0vqcvFa9AYb5Zm59gtE9BZ0i2B2JSRsjyW CgzfbZhJf0NZdTQEwDTlYvEy9VLmDkxlyBWyUWUszx5Pa9HD2PJlnyP8iHjT7ACheEZJ /iAXFb+CWBpuex5yl9t67j2kso3TM0mpU1pbOZ1RWZhkTrcCjcUjGMjMh5Dno3LT81cl GWx/YyUOzwEzaDYL6PHlpaM5G4sJmT/we1hfYPzFWigDiNzl77KsNcWzhNEUEm1Ow71C rike1lYDMoWJgdt8A90iS12eOvCWRqhQHQRrlGbZxiTcSLQ7BBG9hc9L0Hirbo5DuRcY QKuA== X-Gm-Message-State: AGi0PuZCFeS1V9vF0kcFZkNGJAcu4Y9x3jNQwF6ueD9R0BbrdjC2Pc7T 8cdjm4LbiajXDEojgLz/WcreVA== X-Google-Smtp-Source: APiQypIZPxUNS73sEj5++ucNOYZdRoXLQP+I420LcaF370QhCYJNXl+jzptpm4U1qM7Hj0pr7EUnBQ== X-Received: by 2002:adf:edc6:: with SMTP id v6mr3616887wro.8.1587126522822; Fri, 17 Apr 2020 05:28:42 -0700 (PDT) Received: from localhost (host81-151-181-184.range81-151.btcentralplus.com. [81.151.181.184]) by smtp.gmail.com with ESMTPSA id r5sm7536555wmr.15.2020.04.17.05.28.41 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Fri, 17 Apr 2020 05:28:42 -0700 (PDT) Date: Fri, 17 Apr 2020 13:28:39 +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: <20200417122839.GK2366@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: 13:23:32 up 9 days, 3:38, X-Editor: GNU Emacs [ http://www.gnu.org/software/emacs ] X-Spam-Status: No, score=-23.7 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: Fri, 17 Apr 2020 12:28:46 -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 While working on a revised patch I tried to reproduce this, and it's most odd. Notice that both before and after you have two Z1 packets, you're inserting the hardware breakpoint twice with no intermediate removal. I don't see this behaviour, even on an unmodified GDB, and I know some remove targets, for example OpenOCD, will sanity check against such double insertions and throw an error. Just though this was worth mentioning. Anyway, here's a revised patch. I've moved the location of the type check so it is only done now for the swapping case. This should resolve all the concerns you raised, while still addressing the original issue. I updated the commit message a bit too. What do you think? Thanks, Andrew --- commit 10d62976088600ee39b9b828bae93f82a44a2974 Author: Andrew Burgess Date: Tue Apr 14 16:32:02 2020 +0100 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 at ADDRESS, 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 (the original hardware breakpoint) at the same address which we are keeping, but which is not currently inserted, 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. The user created hardware breakpoint is marked as inserted, while the GDB internal software single step breakpoint is now marked as not inserted. After this GDB 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 it is now the user created hardware breakpoint that is marked as inserted, so it is this breakpoint GDB tries to remove. The problem is that GDB inserted the software single step breakpoint as a software breakpoint, but is now trying to remove the hardware breakpoint. The problem is removing a software breakpoint is very different to removing a hardware breakpoint, this could result is some undetected undefined behaviour, or as in the original bug report (PR gdb/25741), could result in the target throwing an error. This original patch proposed by Keno Fischer is here: https://sourceware.org/pipermail/gdb-patches/2020-April/167202.html This patch modified breakpoint_locations_match so that two breakpoints of different types would no longer match. This resolves the immediate problem described above, but introduces some other issues. The breakpoint_locations_match function is also used to control whether two breakpoints places at the same address should both be inserted or not. Consider this (slightly contrived) use case: (gdb) set debug remote 1 (gdb) set breakpoint always-inserted on (gdb) hbreak MyFunc0 Sending packet: $me0000202,2#84...Packet received: 0000 Hardware assisted breakpoint 3 at 0xe0000202: file test.c, line 288. Sending packet: $m80000188,1#63...Packet received: 00 Sending packet: $Z1,e0000202,2#ce...Packet received: OK (gdb) break MyFunc0 Note: breakpoint 3 also set at pc 0xe0000202. Sending packet: $me0000202,2#84...Packet received: 0000 Breakpoint 4 at 0xe0000202: file test.c, line 288. Sending packet: $m80000188,1#63...Packet received: 00 Notice after setting the hardware breakpoint we see the Z1 packet sent, but after setting the software breakpoint there's no Z0 packet, GDB sees that the locations match and doesn't make the second insertion. If we modify breakpoint_locations_match then we would insert both a hardware _and_ software breakpoint. This probably doesn't matter, but is not ideal, so a solution that doesn't do this would be better. The proposal here is to focus specifically on the case where we are considering swapping the inserted status of two breakpoint locations, and moves the location type check out of breakpoint_locations_match, and into the caller. This resolves the original issue, while avoiding the double insertion problem. gdb/ChangeLog: PR gdb/25741 * breakpoint.c (breakpoint_locations_match): Compare location types. diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c index e49025461ba..afd6459a634 100644 --- a/gdb/breakpoint.c +++ b/gdb/breakpoint.c @@ -11786,6 +11786,17 @@ update_global_location_list (enum ugll_insert_mode insert_mode) gdb_assert (is_hardware_watchpoint (loc2->owner)); loc2->watchpoint_type = old_loc->watchpoint_type; } + else if (is_breakpoint (old_loc->owner)) + { + /* If we swap the inserted status of a + hardware and software breakpoint then GDB + will insert the breakpoint as one type, + and later try to remove the breakpoint as + the other type. This is not good. */ + gdb_assert (is_breakpoint (loc2->owner)); + if (old_loc->loc_type != loc2->loc_type) + continue; + } /* loc2 is a duplicated location. We need to check if it should be inserted in case it will be