Re: [PATCH] Let gdbserver doesn't tell GDB it support target-side breakpoint conditions and commands if it doesn't support 'Z' packet

[Pedro Alves <palves@redhat.com>]

On 12/09/2013 09:07 PM, Doug Evans wrote:
> 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.

I can try that.  Does your script API series already have
something like that?  I'd guess it's probably touching this
code too.

> 
>> >  	{
>> >  	  /* 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.]

Yeah.  I wrote that before noticing UNSUPPORTED_ERROR, then
forgot to remove.

>> > 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).

Yeah, the previous post was just an RFC, I didn't mean to apply
all of it as a single commit.  In this case, there are two
implementations of that function (the real one, and then
the dummy one for when Python isn't configured in), but
only of them is documented, and I needed to document the
return code, which affects the dummy version too.  Moving
to the header sorted that out.  BTW, I realize this is
probably conflicting with your scripts API series.  ISTR
that removes the dummy functions anyway, right?  In any
case, I'll try the predicate way.

-- 
Pedro Alves