From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 19226 invoked by alias); 12 Dec 2013 12:55:27 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org Received: (qmail 19214 invoked by uid 89); 12 Dec 2013 12:55:26 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-1.5 required=5.0 tests=AWL,BAYES_00,RP_MATCHES_RCVD,SPF_HELO_PASS,SPF_PASS autolearn=ham version=3.3.2 X-HELO: mx1.redhat.com Received: from mx1.redhat.com (HELO mx1.redhat.com) (209.132.183.28) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Thu, 12 Dec 2013 12:55:25 +0000 Received: from int-mx10.intmail.prod.int.phx2.redhat.com (int-mx10.intmail.prod.int.phx2.redhat.com [10.5.11.23]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id rBCCtKXQ028523 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK); Thu, 12 Dec 2013 07:55:20 -0500 Received: from [127.0.0.1] (ovpn01.gateway.prod.ext.ams2.redhat.com [10.39.146.11]) by int-mx10.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id rBCCtIeg030573; Thu, 12 Dec 2013 07:55:19 -0500 Message-ID: <52A9B236.5060004@redhat.com> Date: Thu, 12 Dec 2013 12:55:00 -0000 From: Pedro Alves User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:17.0) Gecko/20130625 Thunderbird/17.0.7 MIME-Version: 1.0 To: Doug Evans CC: Hui Zhu , gdb-patches ml Subject: Re: [PATCH] Let gdbserver doesn't tell GDB it support target-side breakpoint conditions and commands if it doesn't support 'Z' packet References: <5265022F.8060203@mentor.com> <52654A2C.9010202@redhat.com> <529707C7.4040504@mentor.com> <5298AE7C.6020607@redhat.com> <529C80D2.2080608@mentor.com> <529C9B42.20600@redhat.com> <529D62F7.80701@mentor.com> <52A22582.8040509@redhat.com> <52A40015.207@mentor.com> <52A61E86.3020005@redhat.com> In-Reply-To: Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit X-SW-Source: 2013-12/txt/msg00492.txt.bz2 On 12/09/2013 09:07 PM, Doug Evans wrote: >> @@ -2554,13 +2577,18 @@ insert_bp_location (struct bp_location *bl, >> } >> } >> >> - if (val) >> + if (bp_err != GDB_NO_ERROR) >> { >> /* Can't set the breakpoint. */ >> - if (solib_name_from_address (bl->pspace, bl->address)) >> + >> + /* In some cases, we might not be able to insert a >> + breakpoint in a shared library that has already been >> + removed, but we have not yet processed the shlib unload >> + event. */ >> + if ((bp_err == GENERIC_ERROR || bp_err == MEMORY_ERROR) > > It's not readily clear that the code will DTRT if a GENERIC_ERROR > is thrown (instead of being assigned to bp_err manually). > [are we introducing a fragility akin to > source_python_script/UNSUPPORTED_ERROR - presumably not] > A comment affirming this is ok would be welcome. I've extended the comment. Updated patch below. ------------ Subject: [PATCH] Handle the case of a remote target supporting target side commands, but not on software breakpoints. Although we can tell upfront whether a remote target supports target side commands, we can only tell whether the target supports that in combination with a given breakpoint kind (software, hardware, watchpoints, etc.) when we go and try to insert such a breakpoint kind the first time. It's not desirable to make remote_insert_breakpoint simply return -1 in this case, because if the breakpoint was set in a shared library, insert_bp_location will assume that the breakpoint insertion failed because the library wasn't mapped in. insert_bp_location already handles errors/exceptions thrown from the target_insert_xxx methods, exactly so the backend can tell the user the detailed reason the insertion of hw breakpoints failed. But, in the case of software breakpoints, it discards the detailed error message. So the patch makes insert_bp_location use the error's message for SW breakpoints too, and, introduces a NOT_SUPPORTED_ERROR error code so that insert_bp_location doesn't confuse the error for failure due to a shared library disappearing. The result is: (gdb) c Warning: Cannot insert breakpoint 2: Target doesn't support breakpoints that have target side commands. 2013-12-12 Pedro Alves Hui Zhu PR gdb/16101 * breakpoint.c (insert_bp_location): Rename hw_bp_err_string to bp_err_string. Don't mark the location shlib_disabled if the error thrown wasn't a generic or memory error. Catch errors thrown while inserting breakpoints in overlayed code. Output error message of software breakpoints. * remote.c (remote_insert_breakpoint): If this breakpoint has target-side commands but this stub doesn't support Z0 packets, throw NOT_SUPPORTED_ERROR error. * exceptions.h (enum errors) : New error. * target.h (target_insert_breakpoint): Extend comment. (target_insert_hw_breakpoint): Add comment. --- gdb/breakpoint.c | 105 ++++++++++++++++++++++++++++++++++++++++--------------- gdb/exceptions.h | 3 ++ gdb/remote.c | 6 ++++ gdb/target.h | 11 ++++-- 4 files changed, 95 insertions(+), 30 deletions(-) diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c index 589aa19..169fafa 100644 --- a/gdb/breakpoint.c +++ b/gdb/breakpoint.c @@ -2395,8 +2395,8 @@ insert_bp_location (struct bp_location *bl, int *hw_breakpoint_error, int *hw_bp_error_explained_already) { - int val = 0; - const char *hw_bp_err_string = NULL; + enum errors bp_err = GDB_NO_ERROR; + const char *bp_err_message = NULL; struct gdb_exception e; if (!should_be_inserted (bl) || (bl->inserted && !bl->needs_update)) @@ -2496,12 +2496,16 @@ insert_bp_location (struct bp_location *bl, /* No overlay handling: just set the breakpoint. */ TRY_CATCH (e, RETURN_MASK_ALL) { + int val; + val = bl->owner->ops->insert_location (bl); + if (val) + bp_err = GENERIC_ERROR; } if (e.reason < 0) { - val = 1; - hw_bp_err_string = e.message; + bp_err = e.error; + bp_err_message = e.message; } } else @@ -2523,9 +2527,24 @@ insert_bp_location (struct bp_location *bl, /* Set a software (trap) breakpoint at the LMA. */ bl->overlay_target_info = bl->target_info; bl->overlay_target_info.placed_address = addr; - val = target_insert_breakpoint (bl->gdbarch, - &bl->overlay_target_info); - if (val != 0) + + /* No overlay handling: just set the breakpoint. */ + TRY_CATCH (e, RETURN_MASK_ALL) + { + int val; + + val = target_insert_breakpoint (bl->gdbarch, + &bl->overlay_target_info); + if (val) + bp_err = GENERIC_ERROR; + } + if (e.reason < 0) + { + bp_err = e.error; + bp_err_message = e.message; + } + + if (bp_err != GDB_NO_ERROR) fprintf_unfiltered (tmp_error_stream, "Overlay breakpoint %d " "failed: in ROM?\n", @@ -2538,12 +2557,16 @@ insert_bp_location (struct bp_location *bl, /* Yes. This overlay section is mapped into memory. */ TRY_CATCH (e, RETURN_MASK_ALL) { + int val; + val = bl->owner->ops->insert_location (bl); + if (val) + bp_err = GENERIC_ERROR; } if (e.reason < 0) { - val = 1; - hw_bp_err_string = e.message; + bp_err = e.error; + bp_err_message = e.message; } } else @@ -2554,13 +2577,23 @@ insert_bp_location (struct bp_location *bl, } } - if (val) + if (bp_err != GDB_NO_ERROR) { /* Can't set the breakpoint. */ - if (solib_name_from_address (bl->pspace, bl->address)) + + /* In some cases, we might not be able to insert a + breakpoint in a shared library that has already been + removed, but we have not yet processed the shlib unload + event. Unfortunately, some targets that implement + breakpoint insertion themselves (necessary if this is a + HW breakpoint, but SW breakpoints likewise) can't tell + why the breakpoint insertion failed (e.g., the remote + target doesn't define error codes), so we must treat + generic errors as memory errors. */ + if ((bp_err == GENERIC_ERROR || bp_err == MEMORY_ERROR) + && solib_name_from_address (bl->pspace, bl->address)) { /* See also: disable_breakpoints_in_shlibs. */ - val = 0; bl->shlib_disabled = 1; observer_notify_breakpoint_modified (bl->owner); if (!*disabled_breaks) @@ -2575,39 +2608,51 @@ insert_bp_location (struct bp_location *bl, *disabled_breaks = 1; fprintf_unfiltered (tmp_error_stream, "breakpoint #%d\n", bl->owner->number); + return 0; } else { if (bl->loc_type == bp_loc_hardware_breakpoint) { - *hw_breakpoint_error = 1; - *hw_bp_error_explained_already = hw_bp_err_string != NULL; + *hw_breakpoint_error = 1; + *hw_bp_error_explained_already = bp_err_message != NULL; fprintf_unfiltered (tmp_error_stream, "Cannot insert hardware breakpoint %d%s", - bl->owner->number, hw_bp_err_string ? ":" : ".\n"); - if (hw_bp_err_string) - fprintf_unfiltered (tmp_error_stream, "%s.\n", hw_bp_err_string); + bl->owner->number, bp_err_message ? ":" : ".\n"); + if (bp_err_message != NULL) + fprintf_unfiltered (tmp_error_stream, "%s.\n", bp_err_message); } else { - char *message = memory_error_message (TARGET_XFER_E_IO, - bl->gdbarch, bl->address); - struct cleanup *old_chain = make_cleanup (xfree, message); - - fprintf_unfiltered (tmp_error_stream, - "Cannot insert breakpoint %d.\n" - "%s\n", - bl->owner->number, message); - - do_cleanups (old_chain); + if (bp_err_message == NULL) + { + char *message + = memory_error_message (TARGET_XFER_E_IO, + bl->gdbarch, bl->address); + struct cleanup *old_chain = make_cleanup (xfree, message); + + fprintf_unfiltered (tmp_error_stream, + "Cannot insert breakpoint %d.\n" + "%s\n", + bl->owner->number, message); + do_cleanups (old_chain); + } + else + { + fprintf_unfiltered (tmp_error_stream, + "Cannot insert breakpoint %d: %s\n", + bl->owner->number, + bp_err_message); + } } + return 1; } } else bl->inserted = 1; - return val; + return 0; } else if (bl->loc_type == bp_loc_hardware_watchpoint @@ -2615,6 +2660,8 @@ insert_bp_location (struct bp_location *bl, watchpoints. It's not clear that it's necessary... */ && bl->owner->disposition != disp_del_at_next_stop) { + int val; + gdb_assert (bl->owner->ops != NULL && bl->owner->ops->insert_location != NULL); @@ -2658,6 +2705,8 @@ insert_bp_location (struct bp_location *bl, else if (bl->owner->type == bp_catchpoint) { + int val; + gdb_assert (bl->owner->ops != NULL && bl->owner->ops->insert_location != NULL); diff --git a/gdb/exceptions.h b/gdb/exceptions.h index 2cb8242..bd1cce4 100644 --- a/gdb/exceptions.h +++ b/gdb/exceptions.h @@ -97,6 +97,9 @@ enum errors { /* An undefined command was executed. */ UNDEFINED_COMMAND_ERROR, + /* Requested feature, method, mechanism, etc. is not supported. */ + NOT_SUPPORTED_ERROR, + /* Add more errors here. */ NR_ERRORS }; diff --git a/gdb/remote.c b/gdb/remote.c index 2ac8c36..dce5e05 100644 --- a/gdb/remote.c +++ b/gdb/remote.c @@ -8260,6 +8260,12 @@ remote_insert_breakpoint (struct gdbarch *gdbarch, } } + /* If this breakpoint has target-side commands but this stub doesn't + support Z0 packets, throw error. */ + if (!VEC_empty (agent_expr_p, bp_tgt->tcommands)) + throw_error (NOT_SUPPORTED_ERROR, _("\ +Target doesn't support breakpoints that have target side commands.")); + return memory_insert_breakpoint (gdbarch, bp_tgt); } diff --git a/gdb/target.h b/gdb/target.h index f22e5c6..d76f519 100644 --- a/gdb/target.h +++ b/gdb/target.h @@ -1123,8 +1123,10 @@ int target_write_memory_blocks (VEC(memory_write_request_s) *requests, #define target_files_info() \ (*current_target.to_files_info) (¤t_target) -/* Insert a breakpoint at address BP_TGT->placed_address in the target - machine. Result is 0 for success, non-zero for error. */ +/* Insert a hardware breakpoint at address BP_TGT->placed_address in + the target machine. Returns 0 for success, and returns non-zero or + throws an error (with a detailed failure reason error code and + message) otherwise. */ extern int target_insert_breakpoint (struct gdbarch *gdbarch, struct bp_target_info *bp_tgt); @@ -1553,6 +1555,11 @@ extern int target_insert_mask_watchpoint (CORE_ADDR, CORE_ADDR, int); extern int target_remove_mask_watchpoint (CORE_ADDR, CORE_ADDR, int); +/* Insert a hardware breakpoint at address BP_TGT->placed_address in + the target machine. Returns 0 for success, and returns non-zero or + throws an error (with a detailed failure reason error code and + message) otherwise. */ + #define target_insert_hw_breakpoint(gdbarch, bp_tgt) \ (*current_target.to_insert_hw_breakpoint) (gdbarch, bp_tgt) -- 1.7.11.7