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 4A8623858D33 for ; Sun, 19 Apr 2020 18:21:44 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.3.2 sourceware.org 4A8623858D33 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-60-zIjCTBJCM362iEuhMWqUGg-1; Sun, 19 Apr 2020 14:21:42 -0400 X-MC-Unique: zIjCTBJCM362iEuhMWqUGg-1 Received: by mail-ej1-f70.google.com with SMTP id gj7so4682387ejb.9 for ; Sun, 19 Apr 2020 11:21:42 -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=G2x8zad0t+O6FtVVfR90r6K5uYzHRCd+poRy8xMlMkU=; b=eB+b2OwBktJUgToT/70aCWlegSEBWoE7O5wYybOnYIdz+4c+FDoDcdpmMKzy5XHGux zFEoxGBhFGZMZBeCHmMolCUqPfUBscYWSax+V+6XvHgdTOuxDReuS1vJsPc9UZnj7OFm YgJCZsnGcAmnxSoXTrpZJ5t0m25e/NFL5NrW6YxWz0QHINHB9x4a/LlBKxcdGxERi/mc 5TJJDQ3EuJQssTgfZKSEQMUD5CxYlNmeLsyS9QsjzC8ym1skW9KVqzDucg/U5rBl3ICX kSNMSKLQUdQL+1DtZeDqQAfk50mHlkQwhFw49vM66T4Do7FvmszMem22uAM6cti2h+wp xXYA== X-Gm-Message-State: AGi0PuZxdQPSsuDnjQiSbY5RYfccRzGRpr2EVs+r5OPlpXI7P+MDsKfS rz3I4sR4Nu4g1cesUFCK7XrNoV2E5i1q1EO7vWT6sHH39l2VNQpC8IU71s5Ok7hDu1pqcLQycH5 BWM5Jyab49oEoBuLg1EW1GA== X-Received: by 2002:a17:907:2098:: with SMTP id pv24mr12857662ejb.22.1587320500681; Sun, 19 Apr 2020 11:21:40 -0700 (PDT) X-Google-Smtp-Source: APiQypJX66KajePu8M10J9+VheMBNGT//+wigjQKk9jT71U3QK98WzMUdbeImXHQP5W36NMK8uhiMw== X-Received: by 2002:a17:907:2098:: with SMTP id pv24mr12857642ejb.22.1587320500272; Sun, 19 Apr 2020 11:21:40 -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 g20sm4594693ejb.41.2020.04.19.11.21.38 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Sun, 19 Apr 2020 11:21:39 -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: <16bb0282-652d-c6c5-3e47-a81dd827eef5@redhat.com> Date: Sun, 19 Apr 2020 19:21:38 +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=-22.9 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:21:46 -0000 On 4/17/20 1:28 PM, Andrew Burgess wrote: > @@ -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; > + } Unfortunately this doesn't actually work correctly. For example: $ gdb ~/gdb/tests/threads -ex "set breakpoint always-inserted on" -ex "tar rem :9999" -ex "set debug remote 1" ... (gdb) hbreak main Sending packet: $m400700,40#28...Packet received: 89e58b... 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 Note no Z0 was sent after "b main". That's because the sw and hw breakpoints are still considered duplicates. But now, let's delete the hw breakpoint: (gdb) del 1 Sending packet: $z1,400736,1#68...Packet received: OK Note z1 (remove hw break) was sent, but Z0 (insert sw break) was not. At this point, the target side has no breakpoint installed! That is very wrong, because we have "set breakpoint always-inserted on". If the target was running at this point (e.g., non-stop mode), even with "set breakpoint always-inserted off", GDB should keep breakpoints installed, otherwise threads would miss the breakpoint. "set breakpoint always-inserted on" is just a handy way to emulate that. If we do any other operation that forces installing breakpoints, then finally the Z0 is sent: (gdb) b main Note: breakpoint 2 also set at pc 0x400736. Sending packet: $m400736,1#fe...Packet received: 48 Breakpoint 3 at 0x400736: file threads.c, line 57. Sending packet: $Z0,400736,1#47...Packet received: OK (gdb) I spent a good while yesterday trying to make this all work with GDB still considering hw breakpoints and sw breakpoints as duplicates. We also need to handle my concern about insert_bp_location changing the type of breakpoint locations from sw breakpoint to hw breakpoint, _after_ the update_global_location_list sorted out which breakpoints are duplicates of which. The result is that GDB behaves like this: (gdb) hbreak main Sending packet: $m400700,40#28...Packet received: 89e58b... 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 (gdb) del 1 Sending packet: $Z0,400736,1#47...Packet received: OK Sending packet: $z1,400736,1#68...Packet received: OK Sending packet: $Z0,400736,1#47...Packet received: OK Note how after deleting breakpoint 1 (the hw break), GDB first installs the sw breakpoint, and then deletes the hw breakpoint. This order is important to ensures that at no moment could a thread miss breakpoint 2. The final Z0, is an artifact of the target-side condition handling. We could get rid of that but I didn't try it. 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. >From e2fe5095cc2e136dadbec373ebaed2938e282145 Mon Sep 17 00:00:00 2001 From: Pedro Alves Date: Sat, 18 Apr 2020 23:28:46 +0100 Subject: [PATCH] Handle hw and sw breakpoints at the same address --- gdb/breakpoint.c | 314 +++++++++++++++++++++++++++++++++++++++++-------------- 1 file changed, 236 insertions(+), 78 deletions(-) diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c index e49025461ba..25ce3b9d217 100644 --- a/gdb/breakpoint.c +++ b/gdb/breakpoint.c @@ -2405,6 +2405,8 @@ breakpoint_kind (struct bp_location *bl, CORE_ADDR *addr) return gdbarch_breakpoint_kind_from_pc (bl->gdbarch, addr); } +static void handle_automatic_hardware_breakpoints (bp_location *bl); + /* Insert a low-level "breakpoint" of some type. BL is the breakpoint location. Any error messages are printed to TMP_ERROR_STREAM; and DISABLED_BREAKS, and HW_BREAKPOINT_ERROR are used to report problems. @@ -2454,69 +2456,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 +2769,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); } @@ -2905,6 +2848,22 @@ update_inserted_breakpoint_locations (void) } } +static void +breakpoint_error_stream (string_file *tmp_error_stream, + int *hw_breakpoint_error, + int *hw_bp_error_explained_already) +{ + /* If a hardware breakpoint or watchpoint was inserted, add a + message about possibly exhausted resources. */ + if (*hw_breakpoint_error && !*hw_bp_error_explained_already) + { + tmp_error_stream->printf ("Could not insert hardware breakpoints:\n\ +You may have requested too many hardware breakpoints/watchpoints.\n"); + } + target_terminal::ours_for_output (); + error_stream (*tmp_error_stream); +} + /* Used when starting or continuing the program. */ static void @@ -2991,17 +2950,9 @@ insert_breakpoint_locations (void) } if (error_flag) - { - /* If a hardware breakpoint or watchpoint was inserted, add a - message about possibly exhausted resources. */ - if (hw_breakpoint_error && !hw_bp_error_explained_already) - { - tmp_error_stream.printf ("Could not insert hardware breakpoints:\n\ -You may have requested too many hardware breakpoints/watchpoints.\n"); - } - target_terminal::ours_for_output (); - error_stream (tmp_error_stream); - } + breakpoint_error_stream (&tmp_error_stream, + &hw_breakpoint_error, + &hw_bp_error_explained_already); } /* Used when the program stops. @@ -8515,8 +8466,93 @@ mention (struct breakpoint *b) } +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); +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) + { + /* 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) + { + 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 +11487,29 @@ bp_location_is_less_than (const bp_location *a, const bp_location *b) if (a->permanent != b->permanent) return a->permanent > b->permanent; + /* Sort hardware breakpoint locations before software breakpoint + locations, in order to prefer inserting hardware breakpoint + locations. */ + auto type_order = [](bp_loc_type loc_type) + { + switch (loc_type) + { + case bp_loc_hardware_breakpoint: + return 0; + case bp_loc_software_breakpoint: + return 1; + case bp_loc_hardware_watchpoint: + return 2; + case bp_loc_other: + return 3; + } + + gdb_assert_not_reached ("bad switch"); + }; + + if (type_order (a->loc_type) < type_order (b->loc_type)) + 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 +11684,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. @@ -11654,6 +11714,17 @@ update_global_location_list (enum ugll_insert_mode insert_mode) struct bp_location *awp_loc_first; /* access watchpoint */ struct bp_location *rwp_loc_first; /* read watchpoint */ + /* Used for the case we need to insert a sw breakpoint to replace a + hw breakpoint, and vice versa. */ + bool error_flag = false; + int disabled_breaks = 0; + int hw_breakpoint_error = 0; + int hw_bp_error_explained_already = 0; + string_file tmp_error_stream; + /* Explicitly mark the warning -- this will only + be printed if there was an error. */ + tmp_error_stream.puts ("Warning:\n"); + /* Saved former bp_locations array which we compare against the newly built bp_locations from the current state of ALL_BREAKPOINTS. */ struct bp_location **old_locp; @@ -11673,6 +11744,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); @@ -11769,6 +11854,8 @@ update_global_location_list (enum ugll_insert_mode insert_mode) /* OLD_LOC comes from existing struct breakpoint. */ if (bl_address_is_meaningful (old_loc)) { + bp_location *replacement_loc = nullptr; + for (loc2p = locp; (loc2p < bp_locations + bp_locations_count && (*loc2p)->address == old_loc->address); @@ -11776,6 +11863,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 @@ -11786,12 +11876,25 @@ 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 ((old_loc->loc_type == bp_loc_hardware_breakpoint + || old_loc->loc_type == bp_loc_software_breakpoint) + && loc2->loc_type != old_loc->loc_type) + { + /* A hardware breakpoint is being + removed. Don't consider software + breakpoint locations as duplicates, + to avoid consuming hardware resources + unnecessarily without user + request. */ + if (unduplicated_should_be_inserted (loc2)) + replacement_loc = loc2; + continue; + } /* 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; @@ -11799,6 +11902,29 @@ update_global_location_list (enum ugll_insert_mode insert_mode) } } } + + /* If we're deleting a harware breakpoint, and we + have a replacement software breakpoint to install + (or vice versa), we must install the new one + before deleting the old one, so that the inferior + doesn't miss a breakpoint in case it is running + right now. */ + if (!keep_in_target && replacement_loc != nullptr) + { + scoped_restore_current_pspace_and_thread restore_pspace_thread; + + switch_to_program_space_and_thread (replacement_loc->pspace); + + replacement_loc->duplicate = false; + + int val + = insert_bp_location (replacement_loc, &tmp_error_stream, + &disabled_breaks, + &hw_breakpoint_error, + &hw_bp_error_explained_already); + if (val) + error_flag = val; + } } } @@ -11959,13 +12085,45 @@ update_global_location_list (enum ugll_insert_mode insert_mode) is not duplicated, and is the inserted one. All following are marked as duplicated, and are not inserted. */ if (loc->inserted) - swap_insertion (loc, *loc_first_p); + { + if (loc->loc_type == bp_loc_software_breakpoint + && (*loc_first_p)->loc_type == bp_loc_hardware_breakpoint) + { + /* Insert the replacement breakpoint first, so that + there's never a time the inferior could miss a + breakpoint, in case the inferior is running right + now. */ + scoped_restore_current_pspace_and_thread restore_pspace_thread; + + switch_to_program_space_and_thread ((*loc_first_p)->pspace); + + int val + = insert_bp_location ((*loc_first_p), &tmp_error_stream, + &disabled_breaks, + &hw_breakpoint_error, + &hw_bp_error_explained_already); + if (val) + error_flag = val; + + remove_breakpoint (loc); + } + else + swap_insertion (loc, *loc_first_p); + } loc->duplicate = 1; /* Clear the condition modification flag. */ loc->condition_changed = condition_unchanged; } + /* Should this be propagated down to insert_breakpoint_locations if + it's going to be called, so that we have a single place that + throws? */ + if (error_flag) + breakpoint_error_stream (&tmp_error_stream, + &hw_breakpoint_error, + &hw_bp_error_explained_already); + if (insert_mode == UGLL_INSERT || breakpoints_should_be_inserted_now ()) { if (insert_mode != UGLL_DONT_INSERT) base-commit: 8e4979ac1ea78147ecbcbf81af5e946873dda079 -- 2.14.5