[PATCH] Let gdbserver doesn't tell GDB it support target-side breakpoint conditions and commands if it doesn't support 'Z' packet
Doug Evans
xdje42@gmail.com
Mon Dec 9 21:07:00 GMT 2013
Pedro Alves <palves@redhat.com> 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);
> }
More information about the Gdb-patches
mailing list