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 [207.211.31.120]) by sourceware.org (Postfix) with ESMTP id BA5153858D31 for ; Sun, 19 Apr 2020 18:49:18 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.3.2 sourceware.org BA5153858D31 Received: from mail-ej1-f70.google.com (mail-ej1-f70.google.com [209.85.218.70]) (Using TLS) by relay.mimecast.com with ESMTP id us-mta-237-_Sx6RRqbMw291dCxlmLvxQ-1; Sun, 19 Apr 2020 14:49:16 -0400 X-MC-Unique: _Sx6RRqbMw291dCxlmLvxQ-1 Received: by mail-ej1-f70.google.com with SMTP id gj7so4798918ejb.9 for ; Sun, 19 Apr 2020 11:49:16 -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=3e4EJNePoW3R1yUV/CmAsJ5nT8mSRkjOKMVHvSQW+5c=; b=WvR4QjZA1E+pPUni67JN17gbchhhqENYG7Q0VF+j0brwqJ8pg1SHb09h2ip4nrHMHJ /Gs0jqJkzQFbQdojhZLayfwBRzyH7fpLGKEL/AKqV5aRs2t64g3/TyhG8J08vKwRrGUq eKwsCY6Cw2ymq3yOhOg+pdlJuRH5oZyRz8S4vJlbB8BVm7ySnzGTNM5rRRsSQOP4yiD0 NjvfHzNF8k/gVW1CFzYcFySQEUn9Tg+qMpAWSZC43IkO8YryzIVf9v4rSQ4Jtu348XAn 6rC54aOeHQjm/BiC1stKEcixQH5zf6sUKHm6HSWs5zQKtFAE88V8vM2/Wnc3vDgGJqtp w6zg== X-Gm-Message-State: AGi0PuaIri2A2+zL2EQFA2fLnp+ook63ImGwMPDl+RChSslQkwfkwTXp cEhEgvN4CC7TvkucyLfOJGvqhYcy1MoJAu4zaxL+riySlk9GeUmebwsFf9cmUmcKr1Yg008mfsN 925ZIiNtiPr0OkvSFmB3feA== X-Received: by 2002:a05:6402:22cc:: with SMTP id dm12mr11567205edb.159.1587322154826; Sun, 19 Apr 2020 11:49:14 -0700 (PDT) X-Google-Smtp-Source: APiQypLTTAsRnsw8DNzh516Ci248feIYf61fSBAOGDRXsLadWjBVwEd5lyrtd81NGgxg+m3oxRHyVA== X-Received: by 2002:a05:6402:22cc:: with SMTP id dm12mr11567195edb.159.1587322154408; Sun, 19 Apr 2020 11:49:14 -0700 (PDT) Received: from ?IPv6:2001:8a0:f909:7b00:2327:23ca:3e56:ef5f? ([2001:8a0:f909:7b00:2327:23ca:3e56:ef5f]) by smtp.gmail.com with ESMTPSA id x25sm2896349ejf.49.2020.04.19.11.49.12 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Sun, 19 Apr 2020 11:49:13 -0700 (PDT) Subject: [PATCH] Stop considering hw and sw breakpoints duplicates (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> <16bb0282-652d-c6c5-3e47-a81dd827eef5@redhat.com> Cc: Simon Marchi , Keno Fischer , gdb-patches@sourceware.org From: Pedro Alves Message-ID: Date: Sun, 19 Apr 2020 19:49:12 +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: <16bb0282-652d-c6c5-3e47-a81dd827eef5@redhat.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=-23.3 required=5.0 tests=BAYES_00, DKIM_INVALID, DKIM_SIGNED, GIT_PATCH_0, GIT_PATCH_1, GIT_PATCH_2, GIT_PATCH_3, KAM_DMARC_STATUS, 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: Sun, 19 Apr 2020 18:49:21 -0000 On 4/19/20 7:21 PM, Pedro Alves via Gdb-patches wrote: > It was only after all the effort that I realized that it's > pointless to try to make hw and sw breakpoint locations > match _if we always need to install the new sw break before > deleting the hw break_. I mean, we go through all that > effort, but then there's always going to be a small window > where the remote target needs to be able to handle > a sw breakpoint and a hw breakpoint at the same address > anyway (e.g., handle target-side breakpoint conditions > correctly, handle target-side commands correctly, etc.) > > So the patch below is the one I wrote yesterday. It > wasn't finished, but was well advanced. I'm posting > it for the archives and for discussion. At this point, > I don't believe we should follow this patch's approach. > I'll send another reply with today's new patch. So here's my proposed patch. This makes breakpoint_locations_match consider the location's type too, like Keno's original patch. But then does a bunch more stuff to actually make that work correctly. - We need to handle the duplicates detection better. Take a look at the loop at the tail end of update_global_location_list. Currently, because breakpoint locations aren't sorted by type, we can end up with, at the same address, a sw break, then a hw break, then a sw break, etc. The result is that that second sw break won't be considered a duplicate of the first sw break. Seems like we already handle that incorrectly for range breakpoints. - The "set breakpoint auto-hw" handling is moved out of insert_bp_location to update_global_location_list, before the duplicates determination. This change comes with one visible behavior change with "set breakpoint auto-hw off", however. Here: Before: (gdb) break *0x400487 Breakpoint 2 at 0x400487: file .../src/gdb/testsuite/gdb.base/breakpoint-in-ro-region.c, line 22. (gdb) c Continuing. Warning: Cannot insert breakpoint 2. Cannot set software breakpoint at read-only address 0x400487 Command aborted. (gdb) After: (gdb) break *0x400487 Breakpoint 2 at 0x400487: file .../src/gdb/testsuite/gdb.base/breakpoint-in-ro-region.c, line 22. warning: Breakpoint 2.1 disabled. Cannot set software breakpoint at read-only address 0x400487 I.e., we now warn earlier, instead of waiting until resume time to error out. - In update_breakpoint_locations, the logic of matching old locations with new locations, in the have_ambiguous_names case, is updated to still consider sw vs hw locations the same. - I went through all ALL_BP_LOCATIONS_AT_ADDR uses, to see if any needed updating. Note that that macro walks all locations at a given address, and doesn't call breakpoint_locations_match. The result against GDBserver is this: (gdb) hbreak main Sending packet: $m400700,40#28...Packet received: 89e58b0.... 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: $Z0,400736,1#47...Packet received: OK Sending packet: $Z1,400736,1#48...Packet received: OK (gdb) del 1 Sending packet: $z1,400736,1#68...Packet received: OK Sending packet: $Z0,400736,1#47...Packet received: OK (gdb) Or, with "set breakpoint condition-evaluation host", (gdb) hbreak main Sending packet: $m400736,1#fe...Packet received: 48 Hardware assisted breakpoint 3 at 0x400736: file threads.c, line 57. Sending packet: $Z1,400736,1#48...Packet received: OK (gdb) b main Note: breakpoint 3 also set at pc 0x400736. Sending packet: $m400736,1#fe...Packet received: 48 Breakpoint 4 at 0x400736: file threads.c, line 57. Sending packet: $Z0,400736,1#47...Packet received: OK (gdb) del 3 Sending packet: $z1,400736,1#68...Packet received: OK >From 10d0944768e6ce59861c1522ed48449422a76736 Mon Sep 17 00:00:00 2001 From: Pedro Alves Date: Sun, 19 Apr 2020 18:25:55 +0100 Subject: [PATCH] Stop considering hw and sw breakpoints duplicates --- gdb/breakpoint.c | 269 ++++++++++++++------- gdb/testsuite/gdb.base/breakpoint-in-ro-region.exp | 2 +- 2 files changed, 183 insertions(+), 88 deletions(-) diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c index e49025461ba..27799e89807 100644 --- a/gdb/breakpoint.c +++ b/gdb/breakpoint.c @@ -144,6 +144,10 @@ static void describe_other_breakpoints (struct gdbarch *, static int watchpoint_locations_match (struct bp_location *loc1, struct bp_location *loc2); +static int breakpoint_locations_match (struct bp_location *loc1, + struct bp_location *loc2, + bool sw_hw_bps_match = false); + static int breakpoint_location_address_match (struct bp_location *bl, const struct address_space *aspace, CORE_ADDR addr); @@ -2125,10 +2129,14 @@ build_target_condition_list (struct bp_location *bl) return; /* Do a first pass to check for locations with no assigned - conditions or conditions that fail to parse to a valid agent expression - bytecode. If any of these happen, then it's no use to send conditions - to the target since this location will always trigger and generate a - response back to GDB. */ + conditions or conditions that fail to parse to a valid agent + expression bytecode. If any of these happen, then it's no use to + send conditions to the target since this location will always + trigger and generate a response back to GDB. Note we consider + all locations at the same address irrespective of type, i.e., + even if the locations aren't considered duplicates (e.g., + software breakpoint and hardware breakpoint at the same + address). */ ALL_BP_LOCATIONS_AT_ADDR (loc2p, locp, bl->address) { loc = (*loc2p); @@ -2177,8 +2185,12 @@ build_target_condition_list (struct bp_location *bl) } } - /* No NULL conditions or failed bytecode generation. Build a condition list - for this location's address. */ + /* No NULL conditions or failed bytecode generation. Build a + condition list for this location's address. If we have software + and hardware locations at the same address, they aren't + considered duplicates, but we still marge all the conditions + anyway, as it's simpler, and doesn't really make a practical + difference. */ ALL_BP_LOCATIONS_AT_ADDR (loc2p, locp, bl->address) { loc = (*loc2p); @@ -2296,9 +2308,9 @@ build_target_command_list (struct bp_location *bl) if (dprintf_style != dprintf_style_agent) return; - /* For now, if we have any duplicate location that isn't a dprintf, - don't install the target-side commands, as that would make the - breakpoint not be reported to the core, and we'd lose + /* For now, if we have any location at the same address that isn't a + dprintf, don't install the target-side commands, as that would + make the breakpoint not be reported to the core, and we'd lose control. */ ALL_BP_LOCATIONS_AT_ADDR (loc2p, locp, bl->address) { @@ -2360,12 +2372,19 @@ build_target_command_list (struct bp_location *bl) } } - /* No NULL commands or failed bytecode generation. Build a command list - for this location's address. */ + /* No NULL commands or failed bytecode generation. Build a command + list for all duplicate locations at this location's address. + Note that here we must care for whether the breakpoint location + types are considered duplicates, otherwise, say, if we have a + software and hardware location at the same address, the target + could end up running the commands twice. For the moment, we only + support targets-side commands with dprintf, but it doesn't hurt + to be pedantically correct in case that changes. */ ALL_BP_LOCATIONS_AT_ADDR (loc2p, locp, bl->address) { loc = (*loc2p); - if (loc->owner->extra_string + if (breakpoint_locations_match (bl, loc) + && loc->owner->extra_string && is_breakpoint (loc->owner) && loc->pspace->num == bl->pspace->num && loc->owner->enable_state == bp_enabled @@ -2454,69 +2473,6 @@ insert_bp_location (struct bp_location *bl, if (bl->loc_type == bp_loc_software_breakpoint || bl->loc_type == bp_loc_hardware_breakpoint) { - if (bl->owner->type != bp_hardware_breakpoint) - { - /* If the explicitly specified breakpoint type - is not hardware breakpoint, check the memory map to see - if the breakpoint address is in read only memory or not. - - Two important cases are: - - location type is not hardware breakpoint, memory - is readonly. We change the type of the location to - hardware breakpoint. - - location type is hardware breakpoint, memory is - read-write. This means we've previously made the - location hardware one, but then the memory map changed, - so we undo. - - When breakpoints are removed, remove_breakpoints will use - location types we've just set here, the only possible - problem is that memory map has changed during running - program, but it's not going to work anyway with current - gdb. */ - struct mem_region *mr - = lookup_mem_region (bl->target_info.reqstd_address); - - if (mr) - { - if (automatic_hardware_breakpoints) - { - enum bp_loc_type new_type; - - if (mr->attrib.mode != MEM_RW) - new_type = bp_loc_hardware_breakpoint; - else - new_type = bp_loc_software_breakpoint; - - if (new_type != bl->loc_type) - { - static int said = 0; - - bl->loc_type = new_type; - if (!said) - { - fprintf_filtered (gdb_stdout, - _("Note: automatically using " - "hardware breakpoints for " - "read-only addresses.\n")); - said = 1; - } - } - } - else if (bl->loc_type == bp_loc_software_breakpoint - && mr->attrib.mode != MEM_RW) - { - fprintf_unfiltered (tmp_error_stream, - _("Cannot insert breakpoint %d.\n" - "Cannot set software breakpoint " - "at read-only address %s\n"), - bl->owner->number, - paddress (bl->gdbarch, bl->address)); - return 1; - } - } - } - /* First check to see if we have to handle an overlay. */ if (overlay_debugging == ovly_off || bl->section == NULL @@ -2830,7 +2786,11 @@ insert_breakpoints (void) /* Updating watchpoints creates new locations, so update the global location list. Explicitly tell ugll to insert locations and - ignore breakpoints_always_inserted_mode. */ + ignore breakpoints_always_inserted_mode. Also, + update_global_location_list tries to "upgrade" software + breakpoints to hardware breakpoints to handle "set breakpoint + auto-hw", so we need to call it even if we don't have new + locations. */ update_global_location_list (UGLL_INSERT); } @@ -6813,11 +6773,14 @@ tracepoint_locations_match (struct bp_location *loc1, /* Assuming LOC1 and LOC2's types' have meaningful target addresses (bl_address_is_meaningful), returns true if LOC1 and LOC2 represent - the same location. */ + the same location. If SW_HW_BPS_MATCH is true, then software + breakpoint locations and hardware breakpoint locations match, + otherwise they don't. */ static int -breakpoint_locations_match (struct bp_location *loc1, - struct bp_location *loc2) +breakpoint_locations_match (struct bp_location *loc1, + struct bp_location *loc2, + bool sw_hw_bps_match) { int hw_point1, hw_point2; @@ -6835,9 +6798,12 @@ breakpoint_locations_match (struct bp_location *loc1, else if (is_tracepoint (loc1->owner) || is_tracepoint (loc2->owner)) return tracepoint_locations_match (loc1, loc2); else - /* We compare bp_location.length in order to cover ranged breakpoints. */ + /* We compare bp_location.length in order to cover ranged + breakpoints. Keep this in sync with + bp_location_is_less_than. */ return (breakpoint_address_match (loc1->pspace->aspace, loc1->address, loc2->pspace->aspace, loc2->address) + && (loc1->loc_type == loc2->loc_type || sw_hw_bps_match) && loc1->length == loc2->length); } @@ -8515,8 +8481,99 @@ mention (struct breakpoint *b) } +/* Return the location number of LOC. */ + +static int +bp_location_number (bp_location *loc) +{ + int n = 1; + for (bp_location *l = loc->owner->loc; l != nullptr; l = l->next) + { + if (l == loc) + return n; + ++n; + } + + gdb_assert_not_reached ("breakpoint location not found in owner breakpoint"); +} + static bool bp_loc_is_permanent (struct bp_location *loc); +/* Handle "set breakpoint auto-hw". + + If the explicitly specified breakpoint type is not hardware + breakpoint, check the memory map to see whether the breakpoint + address is in read-only memory. + + And then, if "set breakpoint auto-hw" is enabled: + + - location type is not hardware breakpoint, memory is read-only. + We change the type of the location to hardware breakpoint. + + - location type is hardware breakpoint, memory is read-write. This + means we've previously made the location hardware one, but then the + memory map changed, so we undo. + + However, if "set breakpoint auto-hw" is disabled, and memory is + read-only, we warn and disable the breakpoint location. */ + +static void +handle_automatic_hardware_breakpoints (bp_location *bl) +{ + if (bl->loc_type == bp_loc_software_breakpoint + || bl->loc_type == bp_loc_hardware_breakpoint) + { + if (bl->owner->type != bp_hardware_breakpoint) + { + /* When breakpoints are removed, remove_breakpoints will use + location types we've just set here, the only possible + problem is that memory map has changed during running + program, but it's not going to work anyway with current + gdb. */ + mem_region *mr = lookup_mem_region (bl->address); + + if (mr != nullptr) + { + if (automatic_hardware_breakpoints) + { + enum bp_loc_type new_type; + + if (mr->attrib.mode != MEM_RW) + new_type = bp_loc_hardware_breakpoint; + else + new_type = bp_loc_software_breakpoint; + + if (new_type != bl->loc_type) + { + static bool said = false; + + bl->loc_type = new_type; + if (!said) + { + fprintf_filtered (gdb_stdout, + _("Note: automatically using " + "hardware breakpoints for " + "read-only addresses.\n")); + said = true; + } + } + } + else if (bl->loc_type == bp_loc_software_breakpoint + && mr->attrib.mode != MEM_RW) + { + bl->enabled = false; + + warning (_("Breakpoint %d.%d disabled.\n" + "Cannot set software breakpoint " + "at read-only address %s\n"), + bl->owner->number, bp_location_number (bl), + paddress (bl->gdbarch, bl->address)); + } + } + } + } +} + static struct bp_location * add_location_to_breakpoint (struct breakpoint *b, const struct symtab_and_line *sal) @@ -11451,6 +11508,18 @@ bp_location_is_less_than (const bp_location *a, const bp_location *b) if (a->permanent != b->permanent) return a->permanent > b->permanent; + /* Sort by type in order to make duplicate determination easier. + See update_global_location_list. This is kept in sync with + breakpoint_locations_match. */ + if (a->loc_type < b->loc_type) + return true; + + /* Likewise, for range-breakpoints, sort by length. */ + if (a->loc_type == bp_loc_hardware_breakpoint + && b->loc_type == bp_loc_hardware_breakpoint + && a->length < b->length) + return true; + /* Make the internal GDB representation stable across GDB runs where A and B memory inside GDB can differ. Breakpoint locations of the same type at the same address can be sorted in arbitrary order. */ @@ -11625,6 +11694,7 @@ force_breakpoint_reinsertion (struct bp_location *bl) loc->cond_bytecode.reset (); } } + /* Called whether new breakpoints are created, or existing breakpoints deleted, to update the global location list and recompute which locations are duplicate of which. @@ -11673,6 +11743,20 @@ update_global_location_list (enum ugll_insert_mode insert_mode) ALL_BREAKPOINTS (b) for (loc = b->loc; loc; loc = loc->next) *locp++ = loc; + + /* See if we need to "upgrade" a software breakpoint to a hardware + breakpoint. Do this before deciding whether locations are + duplicates. Also do this before sorting because sorting order + depends on location type. */ + for (locp = bp_locations; + locp < bp_locations + bp_locations_count; + locp++) + { + loc = *locp; + if (!loc->inserted) + handle_automatic_hardware_breakpoints (loc); + } + std::sort (bp_locations, bp_locations + bp_locations_count, bp_location_is_less_than); @@ -11776,6 +11860,9 @@ update_global_location_list (enum ugll_insert_mode insert_mode) { struct bp_location *loc2 = *loc2p; + if (loc2 == old_loc) + continue; + if (breakpoint_locations_match (loc2, old_loc)) { /* Read watchpoint locations are switched to @@ -11790,8 +11877,7 @@ update_global_location_list (enum ugll_insert_mode insert_mode) /* loc2 is a duplicated location. We need to check if it should be inserted in case it will be unduplicated. */ - if (loc2 != old_loc - && unduplicated_should_be_inserted (loc2)) + if (unduplicated_should_be_inserted (loc2)) { swap_insertion (old_loc, loc2); keep_in_target = 1; @@ -13508,11 +13594,20 @@ update_breakpoint_locations (struct breakpoint *b, if (have_ambiguous_names) { for (; l; l = l->next) - if (breakpoint_locations_match (e, l)) - { - l->enabled = 0; - break; - } + { + /* Ignore software vs hardware location type at + this point, because with "set breakpoint + auto-hw", after a re-set, locations that were + hardware can end up as software, or vice versa. + As mentioned above, this is an heuristic and in + practice should give the correct answer often + enough. */ + if (breakpoint_locations_match (e, l, true)) + { + l->enabled = 0; + break; + } + } } else { diff --git a/gdb/testsuite/gdb.base/breakpoint-in-ro-region.exp b/gdb/testsuite/gdb.base/breakpoint-in-ro-region.exp index 65f7baae9f5..b950e433ed8 100644 --- a/gdb/testsuite/gdb.base/breakpoint-in-ro-region.exp +++ b/gdb/testsuite/gdb.base/breakpoint-in-ro-region.exp @@ -171,7 +171,7 @@ gdb_test "p /x *(char *) $main_lo = 1" \ # Ensure that inserting a software breakpoint in a known-read-only # region fails. gdb_test "break *$main_lo" \ - "Cannot insert breakpoint .*Cannot set software breakpoint at read-only address $main_lo.*" \ + "warning: Breakpoint .* disabled.*Cannot set software breakpoint at read-only address $main_lo.*" \ "inserting software breakpoint in read-only memory fails" delete_breakpoints base-commit: 8e4979ac1ea78147ecbcbf81af5e946873dda079 -- 2.14.5