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 at redhat dot com>]

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