From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 17311 invoked by alias); 9 Dec 2013 21:07:45 -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 17296 invoked by uid 89); 9 Dec 2013 21:07:44 -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,FREEMAIL_ENVFROM_END_DIGIT,FREEMAIL_FROM,RCVD_IN_DNSWL_LOW,SPF_PASS autolearn=ham version=3.3.2 X-HELO: mail-pb0-f45.google.com Received: from Unknown (HELO mail-pb0-f45.google.com) (209.85.160.45) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with (AES128-SHA encrypted) ESMTPS; Mon, 09 Dec 2013 21:07:43 +0000 Received: by mail-pb0-f45.google.com with SMTP id rp16so6134175pbb.32 for ; Mon, 09 Dec 2013 13:07:35 -0800 (PST) X-Received: by 10.68.6.99 with SMTP id z3mr23356976pbz.114.1386623255133; Mon, 09 Dec 2013 13:07:35 -0800 (PST) Received: from sspiff.sspiff.org.gmail.com (173-13-178-53-sfba.hfc.comcastbusiness.net. [173.13.178.53]) by mx.google.com with ESMTPSA id ug2sm28145259pac.21.2013.12.09.13.07.33 for (version=TLSv1.2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Mon, 09 Dec 2013 13:07:33 -0800 (PST) From: Doug Evans To: Pedro Alves 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> Date: Mon, 09 Dec 2013 21:07:00 -0000 In-Reply-To: <52A61E86.3020005@redhat.com> (Pedro Alves's message of "Mon, 09 Dec 2013 19:48:22 +0000") Message-ID: User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/24.3 (gnu/linux) MIME-Version: 1.0 Content-Type: text/plain X-IsSubscribed: yes X-SW-Source: 2013-12/txt/msg00352.txt.bz2 Pedro Alves writes: > 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? Hi. Various comments in line. > --- > 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; Presumably there's a sufficient reason to keep them, but the question must be asked. :-) Are the casts necessary? [does bp_err_message have to be a char *] > } > } > 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) 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. > + && 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)) If we must change things, I would prefer having a predicate and call that first. > { > /* 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, > + Left over from an editing pass? [It's not used in the patch, and would be confusing with UNSUPPORTED_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. */ This is a change not related to the patch in question (moving the comment to python.h). This community can get massively nitpicky about such things. [It's fine with me. I just want to point that out -- more clarity would be nice.] > > -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); > }