From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from us-smtp-1.mimecast.com (us-smtp-delivery-1.mimecast.com [205.139.110.120]) by sourceware.org (Postfix) with ESMTP id D888C385B835 for ; Fri, 17 Apr 2020 17:22:29 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.3.2 sourceware.org D888C385B835 Received: from mail-ej1-f71.google.com (mail-ej1-f71.google.com [209.85.218.71]) (Using TLS) by relay.mimecast.com with ESMTP id us-mta-278-eGszgc_bNxGlbeK9gEauSA-1; Fri, 17 Apr 2020 13:22:25 -0400 X-MC-Unique: eGszgc_bNxGlbeK9gEauSA-1 Received: by mail-ej1-f71.google.com with SMTP id v3so1355866ejx.8 for ; Fri, 17 Apr 2020 10:22:25 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:subject:to:references:cc:from:message-id:date :user-agent:mime-version:in-reply-to:content-language :content-transfer-encoding; bh=tWR9oMxeepV+NcBzJSzKoqO1I3Q+ndSwLBldczUR4Cs=; b=ptli6ajZpeSugJxP7QUet9KpB2R3nk75dcDV58OPXwBfWtVzOUB9Nel3QcopA8tp0q rnliJb0cgKVwRVH9TWVWMhu5NT2i5GMQRfMF3VmHW7MqxBFe2TvZwt9MHGbNQ+Xhu5+g sOTuFcTPuakikBL9b/sPJ/+ARilisKQNqGEMfP7lCy40LKDFZ8AdrnUscx1kvmBj8sNV 1lsXvwp5ZLi7Lxp1/wf/o60da7OnNDyiAH4jFM3DBmf5Lp9lOYfhopwjqbio85pF+rrx /q4EpjLLNyX4pb8R4F2KBXS7sa5XRsLvX6cC3gLZL6gALAce12jzE0/IRFyR0sK6ueN1 guEw== X-Gm-Message-State: AGi0PuYxjq00QXDXt5GUaMgLn6HKBz9o+bomVyArLnKKnQ73VfLT4zMo CqHaRsCUsMYSmlCwPW2gHafX82R87K3odHTgfveask8xCQYEimbsqRuUrqxtayqjaMyWmmdgtXK 9XlySlg9XG1ASIeRNHH84/A== X-Received: by 2002:aa7:c781:: with SMTP id n1mr4215135eds.136.1587144143436; Fri, 17 Apr 2020 10:22:23 -0700 (PDT) X-Google-Smtp-Source: APiQypKsYkYWpQC4yF6tzHjK/yNHdNCLlsc9nQSsct0FDnjNe7RLwLYYpcVBfz6Q2G+iy3TJ7rg/xw== X-Received: by 2002:aa7:c781:: with SMTP id n1mr4215109eds.136.1587144143182; Fri, 17 Apr 2020 10:22:23 -0700 (PDT) Received: from ?IPv6:2001:8a0:f909:7b00:56ee:75ff:fe8d:232b? ([2001:8a0:f909:7b00:56ee:75ff:fe8d:232b]) by smtp.gmail.com with ESMTPSA id dd7sm3427798ejb.5.2020.04.17.10.22.22 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Fri, 17 Apr 2020 10:22:22 -0700 (PDT) Subject: Re: [PATCH] breakpoint: Make sure location types match before swapping To: Andrew Burgess References: <20200401013813.GA27550@juliacomputing.com> <372f95e1-4591-7e0d-90c7-168477e032ba@simark.ca> <20200414160059.GG2366@embecosm.com> <41e51f15-6729-ccbf-7833-3a621006cdbf@redhat.com> <20200417122839.GK2366@embecosm.com> Cc: Simon Marchi , Keno Fischer , gdb-patches@sourceware.org From: Pedro Alves Message-ID: Date: Fri, 17 Apr 2020 18:22:21 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101 Thunderbird/60.2.1 MIME-Version: 1.0 In-Reply-To: <20200417122839.GK2366@embecosm.com> Content-Language: en-US X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit X-Spam-Status: No, score=-10.0 required=5.0 tests=BAYES_00, DKIMWL_WL_HIGH, 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: Fri, 17 Apr 2020 17:22:31 -0000 On 4/17/20 1:28 PM, Andrew Burgess wrote: > * 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. That happens because when a software breakpoint location is created, it is created with condition_changed == condition_modified. Then, in update_global_location_list, we end up in force_breakpoint_reinsertion: /* Check if this is a new/duplicated location or a duplicated location that had its condition modified. If so, we want to send its condition to the target if evaluation of conditions is taking place there. */ if ((*loc2p)->condition_changed == condition_modified && (last_addr != old_loc->address || last_pspace_num != old_loc->pspace->num)) { force_breakpoint_reinsertion (*loc2p); last_pspace_num = old_loc->pspace->num; } force_breakpoint_reinsertion forces reinsertion of all breakpoint locations at the same address. > > I don't see this behaviour, even on an unmodified GDB, I was debugging against pristine master x86-64 gdbserver. And: (gdb) show breakpoint condition-evaluation Breakpoint condition evaluation mode is auto (currently target). Maybe you tested against a different remote server which doesn't support target-side condition evaluation? > and I know some remove targets, for example OpenOCD, will sanity check against such > double insertions and throw an error. If they do that, they should be fixed. z/Z packets are supposed to be idempotent. Double insertions are to be expected and should not cause an error. @emph{Implementation notes: A remote target shall return an empty string for an unrecognized breakpoint or watchpoint packet @var{type}. A remote target shall support either both or neither of a given @samp{Z@var{type}@dots{}} and @samp{z@var{type}@dots{}} packet pair. To avoid potential problems with duplicate packets, the operations should ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ be implemented in an idempotent way.} ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ We took advantage of that when we added support for target-side breakpoint conditions. > > 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? Let me try to find a bit of time to give this a think. Thanks, Pedro Alves