Re: [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 at gmail dot com>]

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);
>  }