From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from us-smtp-1.mimecast.com (us-smtp-2.mimecast.com [205.139.110.61]) by sourceware.org (Postfix) with ESMTP id 59288385BF83 for ; Tue, 14 Apr 2020 19:17:09 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.3.2 sourceware.org 59288385BF83 Received: from mail-ed1-f69.google.com (mail-ed1-f69.google.com [209.85.208.69]) (Using TLS) by relay.mimecast.com with ESMTP id us-mta-84-pfFaVPwnOxOo6k0YrwKhWQ-1; Tue, 14 Apr 2020 15:17:05 -0400 X-MC-Unique: pfFaVPwnOxOo6k0YrwKhWQ-1 Received: by mail-ed1-f69.google.com with SMTP id b12so700920edy.7 for ; Tue, 14 Apr 2020 12:17:04 -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=eVagm13Wua8oIeahhRYp9ZiGsNgT3jnx6Fq459KfR2c=; b=b6e2qtZ9Nll5EKex1vf91Gj7SUdCBy8tvW04Jrwp2tUEJyRCiBz/qc+tHMXuZccjIz /qlZHpsLjJ3zgbOBwUr2aB5zJp8PGMBsvgSZOFLxVDjaGo1wyqAfxYmf5eICILRMvsue dXic1yqfXMQYVO+5ZcsVAk0ynfQCoOPYLK65S4OQ4whb+B3HZ95upcGZkQt9/FTmlLln hXadxu4ISpdqOd7MF+WQO13ZBuUnbfmYpL4pUiqHprxNhsSwfnKZ7Pf6SdSnUYX2Lbp1 oT+Gj9IbxljSF3rEM1o8pjwB815Yu9fXHT738cp5La1LynhlOW7eehwbdmgwlDBYET30 b/+w== X-Gm-Message-State: AGi0PuaJDvS2+esqVwzjwe3IP8Wiq4+Mo9fFqpRbM6Crz7akuHZLr7J7 DxmS+p1hiYnlixyW1GYmOQvrz1ifsabkxTFUqtpadbMxux0txAyCXUuuJ17YZbfjdUE6obTXahZ xPxXCbSOnXCBYkHZXP4MSvg== X-Received: by 2002:a50:ee94:: with SMTP id f20mr21304031edr.95.1586891822764; Tue, 14 Apr 2020 12:17:02 -0700 (PDT) X-Google-Smtp-Source: APiQypKBOGOKcl5PgGkhmEw2K7UhcHXmE5M1xXJ1GV6G1xyo96YcXxi7OZOstDCaVQia2nznYuKWog== X-Received: by 2002:a50:ee94:: with SMTP id f20mr21304019edr.95.1586891822566; Tue, 14 Apr 2020 12:17:02 -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 j8sm2182708ejd.91.2020.04.14.12.17.01 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Tue, 14 Apr 2020 12:17:01 -0700 (PDT) Subject: Re: [PATCH] breakpoint: Make sure location types match before swapping To: Andrew Burgess , Simon Marchi References: <20200401013813.GA27550@juliacomputing.com> <372f95e1-4591-7e0d-90c7-168477e032ba@simark.ca> <20200414160059.GG2366@embecosm.com> Cc: Keno Fischer , gdb-patches@sourceware.org From: Pedro Alves Message-ID: <41e51f15-6729-ccbf-7833-3a621006cdbf@redhat.com> Date: Tue, 14 Apr 2020 20:17:00 +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: <20200414160059.GG2366@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=-12.3 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: Tue, 14 Apr 2020 19:17:11 -0000 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. Thanks, Pedro Alves