From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 25482 invoked by alias); 9 Dec 2013 19:48:35 -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 25470 invoked by uid 89); 9 Dec 2013 19:48:34 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-1.4 required=5.0 tests=AWL,BAYES_00,SPF_HELO_PASS,SPF_PASS autolearn=ham version=3.3.2 X-HELO: mx1.redhat.com Received: from Unknown (HELO mx1.redhat.com) (209.132.183.28) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Mon, 09 Dec 2013 19:48:32 +0000 Received: from int-mx02.intmail.prod.int.phx2.redhat.com (int-mx02.intmail.prod.int.phx2.redhat.com [10.5.11.12]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id rB9JmODQ007576 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK); Mon, 9 Dec 2013 14:48:24 -0500 Received: from [127.0.0.1] (ovpn01.gateway.prod.ext.ams2.redhat.com [10.39.146.11]) by int-mx02.intmail.prod.int.phx2.redhat.com (8.13.8/8.13.8) with ESMTP id rB9JmMCQ029561; Mon, 9 Dec 2013 14:48:23 -0500 Message-ID: <52A61E86.3020005@redhat.com> Date: Mon, 09 Dec 2013 19:48: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: Hui Zhu CC: 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> In-Reply-To: <52A40015.207@mentor.com> Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit X-SW-Source: 2013-12/txt/msg00350.txt.bz2 On 12/08/2013 05:13 AM, Hui Zhu wrote: > On 12/07/13 03:29, Pedro Alves wrote: >> > Please analyze the insert_bp_location's error handling carefully: >> > >> > - if solib_name_from_address is true (the dprintf is set in a >> > shared library), we won't see this new error message, while >> > we should. > I change all this part to: > First, output error message. > Second, if it is solib breakpoint, disable breakpoint. But, the current code makes sure that we only see an error for a shared library breakpoint once (see disabled_breaks). After the patch, we'll see an error each time we'll try to insert such a breakpoint. We suppress errors when inserting/removing breakpoints in shared libraries because: /* In some cases, we might not be able to remove a breakpoint in a shared library that has already been removed, but we have not yet processed the shlib unload event. */ and it'd be annoying to see a bunch of errors whenever that happens. I think breakpoint.c needs to be able to distinguish what happened. We already need to handle exceptions thrown from with ops->insert_location / target_insert_foo, so we might as well go the exception way, and add a new error code. There's already something like means "error, unsupported": /* Feature is not supported in this copy of GDB. */ UNSUPPORTED_ERROR, but looking at what it's used for -- if sourcing a python script fails because Python was not configured into the gdb build -- it doesn't look like a good idea to reuse that error code as is: /* Load script FILE, which has already been opened as STREAM. */ static void source_script_from_stream (FILE *stream, const char *file) { if (script_ext_mode != script_ext_off && strlen (file) > 3 && !strcmp (&file[strlen (file) - 3], ".py")) { volatile struct gdb_exception e; TRY_CATCH (e, RETURN_MASK_ERROR) { source_python_script (stream, file); } if (e.reason < 0) { /* Should we fallback to ye olde GDB script mode? */ if (script_ext_mode == script_ext_soft && e.reason == RETURN_ERROR && e.error == UNSUPPORTED_ERROR) { fseek (stream, 0, SEEK_SET); script_from_file (stream, (char*) file); } else { /* Nope, just punt. */ throw_exception (e); } } } else script_from_file (stream, file); } If GDB does support python, and the sourced script happens to do something that triggers that error for some other reason, the above mistakes the error for Python not being supported. Actually, this looks fragile to me. We really can't reuse UNSUPPORTED_ERROR for anything else but "source_python_script is not implemented in this build". (Even in a multi-extension language world, if say, the Python script happens to run something in Scheme, and that raises a UNSUPPORTED_ERROR, we still wouldn't want the fallback code to trigger above.) So I though about renaming UNSUPPORTED_ERROR to something less generic, and then add a new error code (or repurpose the UNSUPPORTED_ERROR name for the new error). But, I don't see why we need to implement this source_python_script case with an exception/error code at all. We can just as well have source_python_script return a boolean indication. Then we're again free to repurpose UNSUPPORTED_ERROR. I'm testing the below. WDYT? --- gdb/breakpoint.c | 100 +++++++++++++++++++++++++++++++++++++--------------- gdb/cli/cli-cmds.c | 14 ++------ gdb/exceptions.h | 5 ++- gdb/python/python.c | 14 ++++---- gdb/python/python.h | 10 +++++- gdb/remote.c | 6 ++++ 6 files changed, 100 insertions(+), 49 deletions(-) diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c index 111660f..c99d0ee 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; - char *hw_bp_err_string = NULL; + enum errors bp_err = GDB_NO_ERROR; + 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 = (char *) e.message; + bp_err = e.error; + bp_err_message = (char *) 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 = (char *) 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 = (char *) e.message; + bp_err = e.error; + bp_err_message = (char *) e.message; } } else @@ -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) + && 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 +2603,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 +2655,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 +2700,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/cli/cli-cmds.c b/gdb/cli/cli-cmds.c index 52a6bc9..ff988d2 100644 --- a/gdb/cli/cli-cmds.c +++ b/gdb/cli/cli-cmds.c @@ -527,24 +527,16 @@ source_script_from_stream (FILE *stream, const char *file) { volatile struct gdb_exception e; - TRY_CATCH (e, RETURN_MASK_ERROR) - { - source_python_script (stream, file); - } - if (e.reason < 0) + if (!source_python_script (stream, file)) { /* Should we fallback to ye olde GDB script mode? */ - if (script_ext_mode == script_ext_soft - && e.reason == RETURN_ERROR && e.error == UNSUPPORTED_ERROR) + if (script_ext_mode == script_ext_soft) { fseek (stream, 0, SEEK_SET); script_from_file (stream, (char*) file); } else - { - /* Nope, just punt. */ - throw_exception (e); - } + error (_("Python scripting is not supported in this copy of GDB.")); } } else diff --git a/gdb/exceptions.h b/gdb/exceptions.h index 705f1a1..fd772b6 100644 --- a/gdb/exceptions.h +++ b/gdb/exceptions.h @@ -79,7 +79,7 @@ enum errors { /* Error accessing memory. */ MEMORY_ERROR, - /* Feature is not supported in this copy of GDB. */ + /* Requested feature, method, mechanism, etc. is not supported. */ UNSUPPORTED_ERROR, /* Value not available. E.g., a register was not collected in a @@ -100,6 +100,9 @@ enum errors { /* An undefined command was executed. */ UNDEFINED_COMMAND_ERROR, + /* Feature is not supported in this copy of GDB. */ + NOT_SUPPORTED_ERROR, + /* Add more errors here. */ NR_ERRORS }; diff --git a/gdb/python/python.c b/gdb/python/python.c index 1873936..6e8cbfa 100644 --- a/gdb/python/python.c +++ b/gdb/python/python.c @@ -764,12 +764,9 @@ gdbpy_find_pc_line (PyObject *self, PyObject *args) return result; } -/* Read a file as Python code. - FILE is the file to run. FILENAME is name of the file FILE. - This does not throw any errors. If an exception occurs python will print - the traceback and clear the error indicator. */ +/* See python.h. */ -void +int source_python_script (FILE *file, const char *filename) { struct cleanup *cleanup; @@ -777,6 +774,8 @@ source_python_script (FILE *file, const char *filename) cleanup = ensure_python_env (get_current_arch (), current_language); python_run_simple_file (file, filename); do_cleanups (cleanup); + + return 1; } @@ -1387,11 +1386,10 @@ eval_python_from_control_command (struct command_line *cmd) error (_("Python scripting is not supported in this copy of GDB.")); } -void +int source_python_script (FILE *file, const char *filename) { - throw_error (UNSUPPORTED_ERROR, - _("Python scripting is not supported in this copy of GDB.")); + return 0; } int diff --git a/gdb/python/python.h b/gdb/python/python.h index c07b2aa..2d37d2d 100644 --- a/gdb/python/python.h +++ b/gdb/python/python.h @@ -93,7 +93,15 @@ extern void finish_python_initialization (void); void eval_python_from_control_command (struct command_line *); -void source_python_script (FILE *file, const char *filename); +/* Read a file as Python code. + FILE is the file to run. FILENAME is name of the file FILE. + This does not throw any errors. If an exception occurs python will print + the traceback and clear the error indicator. + + Returns false if GDB was configured without Python support, + otherwise returns true. */ + +int source_python_script (FILE *file, const char *filename); int apply_val_pretty_printer (struct type *type, const gdb_byte *valaddr, int embedded_offset, CORE_ADDR address, diff --git a/gdb/remote.c b/gdb/remote.c index 2ac8c36..453d06c 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 (UNSUPPORTED_ERROR, _("\ +Target doesn't support breakpoints that have target side commands.")); + return memory_insert_breakpoint (gdbarch, bp_tgt); } -- 1.7.11.7