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

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

[Hui Zhu]

[-- Attachment #1: Type: text/plain, Size: 1334 bytes --]

We found that in powerpc arch board, it cannot pass some dprintf test:
FAIL: gdb.base/dprintf.exp: dprintf info 2 (pattern 6)
FAIL: gdb.mi/mi-dprintf.exp: mi expect stop (unknown output after running)
FAIL: gdb.mi/mi-dprintf.exp: mi 1st dprintf, agent (unknown output after running)

This because the gdbserver will always tell GDB that it support target-side breakpoint conditions and commands.  So "set dprintf-style agent" will always got success.
But target-side breakpoint conditions and commands are depend on 'Z' packet because GDB just can post target-side breakpoint conditions and commands with 'Z' packet.
The test will check if "set dprintf-style agent" success or not.  Because it will always succes.  So GDB change the commands to agent-printf that will make test get fail.

So I make a patch to check if the low-target support insert_point before return support target-side breakpoint conditions and commands.

Tested in powerpc-linux target.

2013-10-21  Hui Zhu  <yao@codesourcery.com>

	* linux-low.c (linux_supports_insert_point): New.
	(linux_target_op): Add linux_supports_insert_point.
	* server.c (handle_query): Check target_supports_insert_point
	before return support target-side breakpoint conditions and
	commands.
	* target.h (target_ops): Add supports_insert_point.
	(target_supports_insert_point): New.

[-- Attachment #2: fix-dprintf.txt --]
[-- Type: text/plain, Size: 2222 bytes --]

--- a/gdb/gdbserver/linux-low.c
+++ b/gdb/gdbserver/linux-low.c
@@ -4929,6 +4929,15 @@ linux_supports_range_stepping (void)
   return (*the_low_target.supports_range_stepping) ();
 }
 
+static int
+linux_supports_insert_point (void)
+{
+  if (the_low_target.insert_point != NULL)
+    return 1;
+
+  return 0;
+}
+
 /* Enumerate spufs IDs for process PID.  */
 static int
 spu_enumerate_spu_ids (long pid, unsigned char *buf, CORE_ADDR offset, int len)
@@ -5825,6 +5834,7 @@ static struct target_ops linux_target_op
   NULL,
 #endif
   linux_supports_range_stepping,
+  linux_supports_insert_point,
 };
 
 static void
--- a/gdb/gdbserver/server.c
+++ b/gdb/gdbserver/server.c
@@ -1806,9 +1806,16 @@ handle_query (char *own_buf, int packet_
 	  strcat (own_buf, ";tracenz+");
 	}
 
-      /* Support target-side breakpoint conditions and commands.  */
-      strcat (own_buf, ";ConditionalBreakpoints+");
-      strcat (own_buf, ";BreakpointCommands+");
+      /* GDB will send target-side breakpoint conditions and commands packets
+         with insert point packets.  So if target doesn't support insert
+         point, it should not support target-side breakpoint conditions
+         and commands packets.  */
+      if (target_supports_insert_point ())
+	{
+	  /* Support target-side breakpoint conditions and commands.  */
+	  strcat (own_buf, ";ConditionalBreakpoints+");
+	  strcat (own_buf, ";BreakpointCommands+");
+	}
 
       if (target_supports_agent ())
 	strcat (own_buf, ";QAgent+");
--- a/gdb/gdbserver/target.h
+++ b/gdb/gdbserver/target.h
@@ -365,6 +365,9 @@ struct target_ops
 
   /* Return true if target supports range stepping.  */
   int (*supports_range_stepping) (void);
+
+  /* Return true if target supports insert_point.  */
+  int (*supports_insert_point) (void);
 };
 
 extern struct target_ops *the_target;
@@ -504,6 +507,10 @@ int kill_inferior (int);
   (the_target->supports_range_stepping ? \
    (*the_target->supports_range_stepping) () : 0)
 
+#define target_supports_insert_point() \
+  (the_target->supports_insert_point ? \
+   (*the_target->supports_insert_point) () : 0)
+
 /* Start non-stop mode, returns 0 on success, -1 on failure.   */
 
 int start_non_stop (int nonstop);

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]

On 10/21/2013 11:30 AM, Hui Zhu wrote:
> We found that in powerpc arch board, it cannot pass some dprintf test:
> FAIL: gdb.base/dprintf.exp: dprintf info 2 (pattern 6)
> FAIL: gdb.mi/mi-dprintf.exp: mi expect stop (unknown output after running)
> FAIL: gdb.mi/mi-dprintf.exp: mi 1st dprintf, agent (unknown output after running)
> 
> This because the gdbserver will always tell GDB that it support target-side breakpoint conditions and commands.  So "set dprintf-style agent" will always got success.
> But target-side breakpoint conditions and commands are depend on 'Z' packet because GDB just can post target-side breakpoint conditions and commands with 'Z' packet.
> The test will check if "set dprintf-style agent" success or not.  Because it will always succes.  So GDB change the commands to agent-printf that will make test get fail.

There happens to be a single "the_low_target.insert_point" entry
point in gdbserver for all sorts of Z packets.  But Z0, Z1, Z2, etc.,
they're all different packets (software breakpoints, hardware breakpoints,
watchpoints, etc.).  A target might well support Z1 but not Z0.  Or it
may support Z2/Z3/Z4, but not Z0..Z1 -- that's the case of MIPS gdbserver.

So, having a "the_low_target.insert_point" hook installed does not
actually mean that dprintf will work.  (Consider what the patch
would need to do if instead of a single "the_low_target.insert_point"
entry point, we had one for each Zx packet.)

In the general case, gdbserver can't possibly know what packet GDB
will want to send with target conditions or target commands in.  GDB
could end up sending either a Z0, or a Z1.  The target might support
Z1, but not Z0, leaving gdb to handle memory breakpoints.
The concept of target-side conditions and commands is broader
than dprintf.

I think that first, GDB should be taught to handle this scenario
itself.  That is, if we try this against gdbserver:

 (gdb) set dprintf-style agent
 (gdb) set remote Z-packet off
 (gdb) dprint main,"foo"
 Dprintf 1 at 0x410776: file foo.c, line 10.
 (gdb) c

GDB should realize that the dprintf won't work, right
at insertion time, but, it currently does not.

-- 
Pedro Alves

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

[Hui Zhu]

On 10/21/13 23:37, Pedro Alves wrote:
> On 10/21/2013 11:30 AM, Hui Zhu wrote:
>> We found that in powerpc arch board, it cannot pass some dprintf test:
>> FAIL: gdb.base/dprintf.exp: dprintf info 2 (pattern 6)
>> FAIL: gdb.mi/mi-dprintf.exp: mi expect stop (unknown output after running)
>> FAIL: gdb.mi/mi-dprintf.exp: mi 1st dprintf, agent (unknown output after running)
>>
>> This because the gdbserver will always tell GDB that it support target-side breakpoint conditions and commands.  So "set dprintf-style agent" will always got success.
>> But target-side breakpoint conditions and commands are depend on 'Z' packet because GDB just can post target-side breakpoint conditions and commands with 'Z' packet.
>> The test will check if "set dprintf-style agent" success or not.  Because it will always succes.  So GDB change the commands to agent-printf that will make test get fail.
>
> There happens to be a single "the_low_target.insert_point" entry
> point in gdbserver for all sorts of Z packets.  But Z0, Z1, Z2, etc.,
> they're all different packets (software breakpoints, hardware breakpoints,
> watchpoints, etc.).  A target might well support Z1 but not Z0.  Or it
> may support Z2/Z3/Z4, but not Z0..Z1 -- that's the case of MIPS gdbserver.
>
> So, having a "the_low_target.insert_point" hook installed does not
> actually mean that dprintf will work.  (Consider what the patch
> would need to do if instead of a single "the_low_target.insert_point"
> entry point, we had one for each Zx packet.)
>
> In the general case, gdbserver can't possibly know what packet GDB
> will want to send with target conditions or target commands in.  GDB
> could end up sending either a Z0, or a Z1.  The target might support
> Z1, but not Z0, leaving gdb to handle memory breakpoints.
> The concept of target-side conditions and commands is broader
> than dprintf.
>
> I think that first, GDB should be taught to handle this scenario
> itself.  That is, if we try this against gdbserver:
>
>   (gdb) set dprintf-style agent
>   (gdb) set remote Z-packet off
>   (gdb) dprint main,"foo"
>   Dprintf 1 at 0x410776: file foo.c, line 10.
>   (gdb) c
>
> GDB should realize that the dprintf won't work, right
> at insertion time, but, it currently does not.
> 	

Hi Pedro,

According to your comments.  I make a new patch for it.
It add a check in remote_insert_breakpoint.  If this breakpoint have target breakpoint but it is not handled by "Z" packet, function will return error.

Please help me review it.

Because mi-dprintf.exp and dprintf.exp use "set dprintf-style agent" check if target support target-side breakpoint, they are still got fail after this patch.
If you think this patch's idea is OK, I will make patch for them.

Thanks,
Hui

2013-11-28  Hui Zhu  <hui@codesourcery.com>

	* remote.c (remote_insert_breakpoint): Add
	have_target_target_side_commands.

--- a/gdb/remote.c
+++ b/gdb/remote.c
@@ -8187,6 +8187,14 @@ static int
  remote_insert_breakpoint (struct gdbarch *gdbarch,
  			  struct bp_target_info *bp_tgt)
  {
+  int have_target_target_side_commands = 0;
+
+  /* Check bp_tgt->tcommands in this place because
+     remote_add_target_side_commands will release bp_tgt->tcommands.  */
+
+  if (!VEC_empty (agent_expr_p, bp_tgt->tcommands))
+    have_target_target_side_commands = 1;
+
    /* Try the "Z" s/w breakpoint packet if it is not already disabled.
       If it succeeds, then set the support to PACKET_ENABLE.  If it
       fails, and the user has explicitly requested the Z support then
@@ -8240,6 +8248,13 @@ remote_insert_breakpoint (struct gdbarch
  	}
      }
  
+  if (have_target_target_side_commands)
+    {
+      warning (_("\
+Target doesn't support breakpoints that have target side commands."));
+      return -1;
+    }
+
    return memory_insert_breakpoint (gdbarch, bp_tgt);
  }
  

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

[Maciej W. Rozycki]

On Thu, 28 Nov 2013, Hui Zhu wrote:

> According to your comments.  I make a new patch for it.
> It add a check in remote_insert_breakpoint.  If this breakpoint have target
> breakpoint but it is not handled by "Z" packet, function will return error.
> 
> Please help me review it.
> 
> Because mi-dprintf.exp and dprintf.exp use "set dprintf-style agent" check if
> target support target-side breakpoint, they are still got fail after this
> patch.
> If you think this patch's idea is OK, I will make patch for them.
> 
> Thanks,
> Hui
> 
> 2013-11-28  Hui Zhu  <hui@codesourcery.com>
> 
> 	* remote.c (remote_insert_breakpoint): Add
> 	have_target_target_side_commands.

 Please note that this is PR gdb/16101 now and refer to it in the 
ChangeLog entry.

  Maciej

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

[Hui Zhu]

On 11/29/13 00:18, Maciej W. Rozycki wrote:
> On Thu, 28 Nov 2013, Hui Zhu wrote:
>
>> According to your comments.  I make a new patch for it.
>> It add a check in remote_insert_breakpoint.  If this breakpoint have target
>> breakpoint but it is not handled by "Z" packet, function will return error.
>>
>> Please help me review it.
>>
>> Because mi-dprintf.exp and dprintf.exp use "set dprintf-style agent" check if
>> target support target-side breakpoint, they are still got fail after this
>> patch.
>> If you think this patch's idea is OK, I will make patch for them.
>>
>> Thanks,
>> Hui
>>
>> 2013-11-28  Hui Zhu  <hui@codesourcery.com>
>>
>> 	* remote.c (remote_insert_breakpoint): Add
>> 	have_target_target_side_commands.
>
>   Please note that this is PR gdb/16101 now and refer to it in the
> ChangeLog entry.
>
>    Maciej
>

Hi Maciej,

Thanks for your remind.
Update changelog to:
2013-11-28  Hui Zhu  <hui@codesourcery.com>

	PR gdb/16101
	* remote.c (remote_insert_breakpoint): Add
	have_target_target_side_commands.

Thanks,
Hui

[pushed] Plug target side conditions and commands leaks (was: 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]

On 11/28/2013 09:07 AM, Hui Zhu wrote:
> --- a/gdb/remote.c
> +++ b/gdb/remote.c
> @@ -8187,6 +8187,14 @@ static int
>   remote_insert_breakpoint (struct gdbarch *gdbarch,
>   			  struct bp_target_info *bp_tgt)
>   {
> +  int have_target_target_side_commands = 0;
> +
> +  /* Check bp_tgt->tcommands in this place because
> +     remote_add_target_side_commands will release bp_tgt->tcommands.  */

Well, that's quite fragile, isn't it.  If Z packets have been found
to not be supported, then nothing is releasing bp_tgt->tcommands
(and bp_tgt->conditions).

> +
> +  if (!VEC_empty (agent_expr_p, bp_tgt->tcommands))
> +    have_target_target_side_commands = 1;
> +

I've pushed this patch below.

----------
Plug target side conditions and commands leaks.

The memory management of bp_location->target_info.conditions|tcommands
is currently a little fragile.  If the target reports support for
target conditions or commands, and then target side breakpoint support
is disabled, or some error is thrown before remote_add_target_side_XXX
is called, we'll leak these lists.  This patch makes us free these
lists when the locations are deleted, and also, just before recreating
the commands|conditions lists.

Tested on x86_64 Fedora 17, native and gdbserver.

gdb/
2013-11-29  Pedro Alves  <palves@redhat.com>

	* breakpoint.c (build_target_condition_list): Release previous
	conditions.
	(build_target_command_list): Release previous commands.
	(bp_location_dtor): Release target conditions and commands.
	* remote.c (remote_add_target_side_condition): Don't release
	conditions.
	(remote_add_target_side_commands): Don't release commands.
---

 gdb/ChangeLog    |   10 ++++++++++
 gdb/breakpoint.c |    9 +++++++++
 gdb/remote.c     |    4 ----
 3 files changed, 19 insertions(+), 4 deletions(-)

diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index e86f5c3..5f0626c 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,3 +1,13 @@
+2013-11-29  Pedro Alves  <palves@redhat.com>
+
+	* breakpoint.c (build_target_condition_list): Release previous
+	conditions.
+	(build_target_command_list): Release previous commands.
+	(bp_location_dtor): Release target conditions and commands.
+	* remote.c (remote_add_target_side_condition): Don't release
+	conditions.
+	(remote_add_target_side_commands): Don't release commands.
+
 2013-11-29  Yao Qi  <yao@codesourcery.com>
 	    Pedro Alves  <palves@redhat.com>

diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
index 897b664..111660f 100644
--- a/gdb/breakpoint.c
+++ b/gdb/breakpoint.c
@@ -2096,6 +2096,9 @@ build_target_condition_list (struct bp_location *bl)
   int modified = bl->needs_update;
   struct bp_location *loc;

+  /* Release conditions left over from a previous insert.  */
+  VEC_free (agent_expr_p, bl->target_info.conditions);
+
   /* This is only meaningful if the target is
      evaluating conditions and if the user has
      opted for condition evaluation on the target's
@@ -2287,6 +2290,9 @@ build_target_command_list (struct bp_location *bl)
   int modified = bl->needs_update;
   struct bp_location *loc;

+  /* Release commands left over from a previous insert.  */
+  VEC_free (agent_expr_p, bl->target_info.tcommands);
+
   /* For now, limit to agent-style dprintf breakpoints.  */
   if (bl->owner->type != bp_dprintf
       || strcmp (dprintf_style, dprintf_style_agent) != 0)
@@ -12734,6 +12740,9 @@ bp_location_dtor (struct bp_location *self)
   if (self->cond_bytecode)
     free_agent_expr (self->cond_bytecode);
   xfree (self->function_name);
+
+  VEC_free (agent_expr_p, self->target_info.conditions);
+  VEC_free (agent_expr_p, self->target_info.tcommands);
 }

 static const struct bp_location_ops bp_location_ops =
diff --git a/gdb/remote.c b/gdb/remote.c
index 186c058..be54450 100644
--- a/gdb/remote.c
+++ b/gdb/remote.c
@@ -8151,8 +8151,6 @@ remote_add_target_side_condition (struct gdbarch *gdbarch,
 	buf = pack_hex_byte (buf, aexpr->buf[i]);
       *buf = '\0';
     }
-
-  VEC_free (agent_expr_p, bp_tgt->conditions);
   return 0;
 }

@@ -8183,8 +8181,6 @@ remote_add_target_side_commands (struct gdbarch *gdbarch,
 	buf = pack_hex_byte (buf, aexpr->buf[i]);
       *buf = '\0';
     }
-
-  VEC_free (agent_expr_p, bp_tgt->tcommands);
 }

 /* Insert a breakpoint.  On targets that have software breakpoint

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]

On 11/28/2013 09:07 AM, Hui Zhu wrote:

> +  if (have_target_target_side_commands)

This can now just be:

  if (!VEC_empty (agent_expr_p, bp_tgt->tcommands))

OK with that change.

> +    {
> +      warning (_("\
> +Target doesn't support breakpoints that have target side commands."));

I was doing to suggest making this an error instead, that
insert_bp_location would print the error string, but that's
only true for hw breakpoints... insert_bp_location's error
handling is quite messy.  For instance, if this breakpoint
is in a a shared library, this will disable the breakpoint,
even though the cause of the error is clearly not that the
shared library disappeared (i.e., not a memory error).

> +      return -1;
> +    }
-- 
Pedro Alves

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

[Hui Zhu]

[-- Attachment #1: Type: text/plain, Size: 1571 bytes --]

On 11/29/13 23:10, Pedro Alves wrote:
> On 11/28/2013 09:07 AM, Hui Zhu wrote:
>
>> +  if (have_target_target_side_commands)
>
> This can now just be:
>
>    if (!VEC_empty (agent_expr_p, bp_tgt->tcommands))
>
> OK with that change.
>
>> +    {
>> +      warning (_("\
>> +Target doesn't support breakpoints that have target side commands."));
>
> I was doing to suggest making this an error instead, that
> insert_bp_location would print the error string, but that's
> only true for hw breakpoints... insert_bp_location's error
> handling is quite messy.  For instance, if this breakpoint
> is in a a shared library, this will disable the breakpoint,
> even though the cause of the error is clearly not that the
> shared library disappeared (i.e., not a memory error).
>
>> +      return -1;
>> +    }

Updated the patch according to your comments.

And I make a patch for dprintf.exp and mi-dprintf.exp to make test OK on the target that doesn't support "Zx" packets.

The patches were tested and pass regression test on X86_64 and PPC.

Please help me review it.

Thanks,
Hui

2013-12-02  Hui Zhu  <hui@codesourcery.com>

	PR gdb/16101
	* remote.c (remote_insert_breakpoint): If this breakpoint has
	target-side commands but this stub doesn't support Z0 packets,
	throw error.

2013-12-02  Hui Zhu  <hui@codesourcery.com>

	PR gdb/16101
	* gdb.base/dprintf.exp: Add check for the the gdbserver of some
	architecture doesn't support some "Zx" doesn't support some "Zx"
	packets.
	* gdb.mi/mi-dprintf.exp: Ditto.
	* lib/mi-support.exp: Add check for continue get error.

[-- Attachment #2: fix-dprintf-test-v2.txt --]
[-- Type: text/plain, Size: 433 bytes --]

--- a/gdb/remote.c
+++ b/gdb/remote.c
@@ -8244,6 +8244,12 @@ remote_insert_breakpoint (struct 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))
+    error (_("\
+Target doesn't support breakpoints that have target side commands."));
+
   return memory_insert_breakpoint (gdbarch, bp_tgt);
 }
 

[-- Attachment #3: dprintf-test.txt --]
[-- Type: text/plain, Size: 2033 bytes --]

--- a/gdb/testsuite/gdb.base/dprintf.exp
+++ b/gdb/testsuite/gdb.base/dprintf.exp
@@ -108,6 +108,26 @@ gdb_test_multiple "set dprintf-style age
     }
 }
 
+# Continue with target-side breakpoint commands will get error if GDB
+# work with the gdbserver of some architecture doesn't support some "Zx"
+# packets.
+if $target_can_dprintf {
+    gdb_run_cmd
+
+    gdb_test "" "Breakpoint"
+
+    set msg "continue with target-side breakpoint commands"
+    gdb_test_multiple "continue" $msg {
+	-re "Cannot insert breakpoint $decimal\.\r\n.*\r\n$gdb_prompt $" {
+	    set target_can_dprintf 0
+	    pass "$msg - cannot do"
+	}
+	-re ".*$gdb_prompt $" {
+	    pass "$msg - can do"
+	}
+    }
+}
+
 if $target_can_dprintf {
     gdb_run_cmd
 
@@ -120,7 +140,7 @@ if $target_can_dprintf {
     gdb_test_sequence "info breakpoints" "dprintf info 2" {
 	"\[\r\n\]Num     Type           Disp Enb Address +What"
 	"\[\r\n\]2       breakpoint"
-	"\[\r\n\]\tbreakpoint already hit 2 times"
+	"\[\r\n\]\tbreakpoint already hit 3 times"
 	"\[\r\n\]3       dprintf"
 	"\[\r\n\]\tbreakpoint already hit 2 times"
 	"\[\r\n\]        agent-printf \"At foo entry\\\\n\""
--- a/gdb/testsuite/gdb.mi/mi-dprintf.exp
+++ b/gdb/testsuite/gdb.mi/mi-dprintf.exp
@@ -139,6 +139,16 @@ gdb_expect {
     }
 }
 
+# Continue with target-side breakpoint commands will get error if GDB
+# work with The gdbserver of some architecture doesn't support some "Zx"
+# packets.
+if $target_can_dprintf {
+    if {[mi_run_cmd] == -1} {
+	pass "continue with target-side breakpoint commands - cannot do"
+	set target_can_dprintf 0
+    }
+}
+
 if $target_can_dprintf {
     mi_run_cmd
 
--- a/gdb/testsuite/lib/mi-support.exp
+++ b/gdb/testsuite/lib/mi-support.exp
@@ -843,6 +843,7 @@ proc mi_run_cmd_full {use_mi_command arg
 	    send_gdb "${run_prefix}continue\n"
 	    gdb_expect 60 {
 		-re "${run_match}\\^running\[\r\n\]+\\*running,thread-id=\"\[^\"\]+\"\r\n$mi_gdb_prompt" {}
+		-re "\\^error.*\r\n$mi_gdb_prompt" { return -1 }
 		default {}
 	    }
 	    return 0

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]

On 12/02/2013 12:45 PM, Hui Zhu wrote:
> On 11/29/13 23:10, Pedro Alves wrote:
>> > On 11/28/2013 09:07 AM, Hui Zhu wrote:
>> >
>>> >> +  if (have_target_target_side_commands)
>> >
>> > This can now just be:
>> >
>> >    if (!VEC_empty (agent_expr_p, bp_tgt->tcommands))
>> >
>> > OK with that change.
>> >
>>> >> +    {
>>> >> +      warning (_("\
>>> >> +Target doesn't support breakpoints that have target side commands."));
>> >
>> > I was doing to suggest making this an error instead, that
>> > insert_bp_location would print the error string, but that's
>> > only true for hw breakpoints... insert_bp_location's error
>> > handling is quite messy.  For instance, if this breakpoint
>> > is in a a shared library, this will disable the breakpoint,
>> > even though the cause of the error is clearly not that the
>> > shared library disappeared (i.e., not a memory error).
>> >
>>> >> +      return -1;
>>> >> +    }
> Updated the patch according to your comments.

But you switched to "error" anyway?  Above I was saying that I
was going that suggest it, but then explained why I didn't think
it would work.  Was I wrong?

> 
> And I make a patch for dprintf.exp and mi-dprintf.exp to make test OK on the target that doesn't support "Zx" packets.
> 
> The patches were tested and pass regression test on X86_64 and PPC.

(It's best to stick to one patch per email, otherwise things
end up confusing.  I suggest looking into git send-email.)

-- 
Pedro Alves

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

[Hui Zhu]

On 12/02/13 22:37, Pedro Alves wrote:
> On 12/02/2013 12:45 PM, Hui Zhu wrote:
>> On 11/29/13 23:10, Pedro Alves wrote:
>>>> On 11/28/2013 09:07 AM, Hui Zhu wrote:
>>>>
>>>>>> +  if (have_target_target_side_commands)
>>>>
>>>> This can now just be:
>>>>
>>>>     if (!VEC_empty (agent_expr_p, bp_tgt->tcommands))
>>>>
>>>> OK with that change.
>>>>
>>>>>> +    {
>>>>>> +      warning (_("\
>>>>>> +Target doesn't support breakpoints that have target side commands."));
>>>>
>>>> I was doing to suggest making this an error instead, that
>>>> insert_bp_location would print the error string, but that's
>>>> only true for hw breakpoints... insert_bp_location's error
>>>> handling is quite messy.  For instance, if this breakpoint
>>>> is in a a shared library, this will disable the breakpoint,
>>>> even though the cause of the error is clearly not that the
>>>> shared library disappeared (i.e., not a memory error).
>>>>
>>>>>> +      return -1;
>>>>>> +    }
>> Updated the patch according to your comments.
>
> But you switched to "error" anyway?  Above I was saying that I
> was going that suggest it, but then explained why I didn't think
> it would work.  Was I wrong?

The reason is insert_bp_location doesn't have code to handle this error.  I make a new patch that include this code.

Please help me review it.

>
>>
>> And I make a patch for dprintf.exp and mi-dprintf.exp to make test OK on the target that doesn't support "Zx" packets.
>>
>> The patches were tested and pass regression test on X86_64 and PPC.
>
> (It's best to stick to one patch per email, otherwise things
> end up confusing.  I suggest looking into git send-email.)
>

OK.  I will post new version of this patch in another mail.

Thanks,
Hui

2013-12-03  Hui Zhu  <hui@codesourcery.com>

	PR gdb/16101
	* breakpoint.c (insert_bp_location): output error message of
	software breakpoints.
	* remote.c (remote_insert_breakpoint): If this breakpoint has
	target-side commands but this stub doesn't support Z0 packets,
	throw error.

--- a/gdb/breakpoint.c
+++ b/gdb/breakpoint.c
@@ -2588,6 +2588,12 @@ insert_bp_location (struct bp_location *
                    if (hw_bp_err_string)
                      fprintf_unfiltered (tmp_error_stream, "%s.\n", hw_bp_err_string);
  		}
+	      else if (hw_bp_err_string != NULL)
+	        {
+		  fprintf_unfiltered (tmp_error_stream,
+				      "Cannot insert breakpoint %d:%s\n",
+				      bl->owner->number, hw_bp_err_string);
+		}
  	      else
  		{
  		  char *message = memory_error_message (TARGET_XFER_E_IO,
--- a/gdb/remote.c
+++ b/gdb/remote.c
@@ -8244,6 +8244,12 @@ remote_insert_breakpoint (struct 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))
+    error (_("\
+Target doesn't support breakpoints that have target side commands."));
+
    return memory_insert_breakpoint (gdbarch, bp_tgt);
  }
  

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

[Hui Zhu]

On 12/03/13 12:49, Hui Zhu wrote:
> On 12/02/13 22:37, Pedro Alves wrote:
>> On 12/02/2013 12:45 PM, Hui Zhu wrote:
>>> On 11/29/13 23:10, Pedro Alves wrote:
>>>>> On 11/28/2013 09:07 AM, Hui Zhu wrote:
>>>>>
>>>>>>> +  if (have_target_target_side_commands)
>>>>>
>>>>> This can now just be:
>>>>>
>>>>>     if (!VEC_empty (agent_expr_p, bp_tgt->tcommands))
>>>>>
>>>>> OK with that change.
>>>>>
>>>>>>> +    {
>>>>>>> +      warning (_("\
>>>>>>> +Target doesn't support breakpoints that have target side commands."));
>>>>>
>>>>> I was doing to suggest making this an error instead, that
>>>>> insert_bp_location would print the error string, but that's
>>>>> only true for hw breakpoints... insert_bp_location's error
>>>>> handling is quite messy.  For instance, if this breakpoint
>>>>> is in a a shared library, this will disable the breakpoint,
>>>>> even though the cause of the error is clearly not that the
>>>>> shared library disappeared (i.e., not a memory error).
>>>>>
>>>>>>> +      return -1;
>>>>>>> +    }
>>> Updated the patch according to your comments.
>>
>> But you switched to "error" anyway?  Above I was saying that I
>> was going that suggest it, but then explained why I didn't think
>> it would work.  Was I wrong?
>
> The reason is insert_bp_location doesn't have code to handle this error.  I make a new patch that include this code.
>
> Please help me review it.
>
>>
>>>
>>> And I make a patch for dprintf.exp and mi-dprintf.exp to make test OK on the target that doesn't support "Zx" packets.
>>>
>>> The patches were tested and pass regression test on X86_64 and PPC.
>>
>> (It's best to stick to one patch per email, otherwise things
>> end up confusing.  I suggest looking into git send-email.)
>>
>
> OK.  I will post new version of this patch in another mail.
>
> Thanks,
> Hui
>

This patch is for dprintf.exp and mi-dprintf.exp to make test OK on the target that doesn't support "Zx" packets.

Thanks,
Hui

2013-12-03  Hui Zhu  <hui@codesourcery.com>

	PR gdb/16101
	* gdb.base/dprintf.exp: Add check for the the gdbserver of some
	architecture doesn't support some "Zx" doesn't support some "Zx"
	packets.
	* gdb.mi/mi-dprintf.exp: Ditto.
	* lib/mi-support.exp: Add check for continue get error.

--- a/gdb/testsuite/gdb.base/dprintf.exp
+++ b/gdb/testsuite/gdb.base/dprintf.exp
@@ -108,6 +108,26 @@ gdb_test_multiple "set dprintf-style age
      }
  }
  
+# Continue with target-side breakpoint commands will get error if GDB
+# work with The gdbserver of some architecture doesn't support some "Zx"
+# packets.
+if $target_can_dprintf {
+    gdb_run_cmd
+
+    gdb_test "" "Breakpoint"
+
+    set msg "continue with target-side breakpoint commands"
+    gdb_test_multiple "continue" $msg {
+	-re "Cannot insert breakpoint $decimal:.*\r\n.*\r\n$gdb_prompt $" {
+	    set target_can_dprintf 0
+	    pass "$msg - cannot do"
+	}
+	-re ".*$gdb_prompt $" {
+	    pass "$msg - can do"
+	}
+    }
+}
+
  if $target_can_dprintf {
      gdb_run_cmd
  
@@ -120,7 +140,7 @@ if $target_can_dprintf {
      gdb_test_sequence "info breakpoints" "dprintf info 2" {
  	"\[\r\n\]Num     Type           Disp Enb Address +What"
  	"\[\r\n\]2       breakpoint"
-	"\[\r\n\]\tbreakpoint already hit 2 times"
+	"\[\r\n\]\tbreakpoint already hit 3 times"
  	"\[\r\n\]3       dprintf"
  	"\[\r\n\]\tbreakpoint already hit 2 times"
  	"\[\r\n\]        agent-printf \"At foo entry\\\\n\""
--- a/gdb/testsuite/gdb.mi/mi-dprintf.exp
+++ b/gdb/testsuite/gdb.mi/mi-dprintf.exp
@@ -139,6 +139,16 @@ gdb_expect {
      }
  }
  
+# Continue with target-side breakpoint commands will get error if GDB
+# work with The gdbserver of some architecture doesn't support some "Zx"
+# packets.
+if $target_can_dprintf {
+    if {[mi_run_cmd] == -1} {
+	pass "continue with target-side breakpoint commands - cannot do"
+	set target_can_dprintf 0
+    }
+}
+
  if $target_can_dprintf {
      mi_run_cmd
  
--- a/gdb/testsuite/lib/mi-support.exp
+++ b/gdb/testsuite/lib/mi-support.exp
@@ -843,6 +843,7 @@ proc mi_run_cmd_full {use_mi_command arg
  	    send_gdb "${run_prefix}continue\n"
  	    gdb_expect 60 {
  		-re "${run_match}\\^running\[\r\n\]+\\*running,thread-id=\"\[^\"\]+\"\r\n$mi_gdb_prompt" {}
+		-re "\\^error.*\r\n$mi_gdb_prompt" { return -1 }
  		default {}
  	    }
  	    return 0

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]

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.
- 'hw_bp_err_string' ends up misnamed after this patch.

-- 
Pedro Alves

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

[Hui Zhu]

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.

> - 'hw_bp_err_string' ends up misnamed after this patch.

Change it to "bp_err_string".

>

Post a new patch according to your comments.
This patch is tested and pass the regression test in x86_64 linux.

Please help me review it.

Thanks,
Hui

2013-12-08  Hui Zhu  <hui@codesourcery.com>

	PR gdb/16101
	* breakpoint.c (insert_bp_location): Change hw_bp_err_string to
	bp_err_string.
	Output error message of software breakpoints.
	Make solib error message output use same code with hardware
	breakpoints and software breakpoints.
	* remote.c (remote_insert_breakpoint): If this breakpoint has
	target-side commands but this stub doesn't support Z0 packets,
	throw error.

--- a/gdb/breakpoint.c
+++ b/gdb/breakpoint.c
@@ -2396,7 +2396,7 @@ insert_bp_location (struct bp_location *
  		    int *hw_bp_error_explained_already)
  {
    int val = 0;
-  char *hw_bp_err_string = NULL;
+  char *bp_err_string = NULL;
    struct gdb_exception e;
  
    if (!should_be_inserted (bl) || (bl->inserted && !bl->needs_update))
@@ -2501,7 +2501,7 @@ insert_bp_location (struct bp_location *
  	  if (e.reason < 0)
  	    {
  	      val = 1;
-	      hw_bp_err_string = (char *) e.message;
+	      bp_err_string = (char *) e.message;
  	    }
  	}
        else
@@ -2543,7 +2543,7 @@ insert_bp_location (struct bp_location *
  	      if (e.reason < 0)
  	        {
  	          val = 1;
-	          hw_bp_err_string = (char *) e.message;
+	          bp_err_string = (char *) e.message;
  	        }
  	    }
  	  else
@@ -2557,6 +2557,36 @@ insert_bp_location (struct bp_location *
        if (val)
  	{
  	  /* Can't set the breakpoint.  */
+	  if (bl->loc_type == bp_loc_hardware_breakpoint)
+	    {
+	      *hw_breakpoint_error = 1;
+	      *hw_bp_error_explained_already = bp_err_string != NULL;
+	      fprintf_unfiltered (tmp_error_stream,
+				  "Cannot insert hardware breakpoint %d%s",
+				  bl->owner->number, bp_err_string ? ":" : ".\n");
+	      if (bp_err_string)
+		fprintf_unfiltered (tmp_error_stream, "%s.\n", bp_err_string);
+	    }
+	  else if (bp_err_string != NULL)
+	    {
+	      fprintf_unfiltered (tmp_error_stream,
+				  "Cannot insert breakpoint %d:%s\n",
+				  bl->owner->number, bp_err_string);
+	    }
+	  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 (solib_name_from_address (bl->pspace, bl->address))
  	    {
  	      /* See also: disable_breakpoints_in_shlibs.  */
@@ -2564,45 +2594,13 @@ insert_bp_location (struct bp_location *
  	      bl->shlib_disabled = 1;
  	      observer_notify_breakpoint_modified (bl->owner);
  	      if (!*disabled_breaks)
-		{
-		  fprintf_unfiltered (tmp_error_stream,
-				      "Cannot insert breakpoint %d.\n",
-				      bl->owner->number);
-		  fprintf_unfiltered (tmp_error_stream,
-				      "Temporarily disabling shared "
-				      "library breakpoints:\n");
-		}
+		fprintf_unfiltered (tmp_error_stream,
+				    "Temporarily disabling shared "
+				    "library breakpoints:\n");
  	      *disabled_breaks = 1;
  	      fprintf_unfiltered (tmp_error_stream,
  				  "breakpoint #%d\n", bl->owner->number);
  	    }
-	  else
-	    {
-	      if (bl->loc_type == bp_loc_hardware_breakpoint)
-		{
-                  *hw_breakpoint_error = 1;
-                  *hw_bp_error_explained_already = hw_bp_err_string != 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);
-		}
-	      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);
-		}
-
-	    }
  	}
        else
  	bl->inserted = 1;
--- a/gdb/remote.c
+++ b/gdb/remote.c
@@ -8260,6 +8260,12 @@ remote_insert_breakpoint (struct 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))
+    error (_("\
+Target doesn't support breakpoints that have target side commands."));
+
    return memory_insert_breakpoint (gdbarch, bp_tgt);
  }
  

  

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]

On Sat, Dec 7, 2013 at 9:13 PM, Hui Zhu <hui_zhu@mentor.com> wrote:
> 2013-12-08  Hui Zhu  <hui@codesourcery.com>
>
>         PR gdb/16101
>         * breakpoint.c (insert_bp_location): Change hw_bp_err_string to
>         bp_err_string.
>
>         Output error message of software breakpoints.
>         Make solib error message output use same code with hardware
>         breakpoints and software breakpoints.
>
>         * remote.c (remote_insert_breakpoint): If this breakpoint has
>         target-side commands but this stub doesn't support Z0 packets,
>         throw error.

Nit.  This changelog is badly formatted.
Please fix.

I suspect, though this is only based on perusal of the patch,
and not close analysis,  that you meant to write:

        PR gdb/16101
        * breakpoint.c (insert_bp_location): Change hw_bp_err_string to
        bp_err_string.  Output error message of software breakpoints.
        Make solib error message output use same code with hardware
        breakpoints and software breakpoints.

         * remote.c (remote_insert_breakpoint): If this breakpoint has
         target-side commands but this stub doesn't support Z0 packets,
         throw error.

[I may have gotten the indentation wrong, cut-n-paste in gmail, blech.
The point is the blank line before "Output error ..." is wrong.  ISTM.]

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

[Hui Zhu]

On 12/08/13 16:34, Doug Evans wrote:
> On Sat, Dec 7, 2013 at 9:13 PM, Hui Zhu <hui_zhu@mentor.com> wrote:
>> 2013-12-08  Hui Zhu  <hui@codesourcery.com>
>>
>>          PR gdb/16101
>>          * breakpoint.c (insert_bp_location): Change hw_bp_err_string to
>>          bp_err_string.
>>
>>          Output error message of software breakpoints.
>>          Make solib error message output use same code with hardware
>>          breakpoints and software breakpoints.
>>
>>          * remote.c (remote_insert_breakpoint): If this breakpoint has
>>          target-side commands but this stub doesn't support Z0 packets,
>>          throw error.
>
> Nit.  This changelog is badly formatted.
> Please fix.
>
> I suspect, though this is only based on perusal of the patch,
> and not close analysis,  that you meant to write:
>
>          PR gdb/16101
>          * breakpoint.c (insert_bp_location): Change hw_bp_err_string to
>          bp_err_string.  Output error message of software breakpoints.
>          Make solib error message output use same code with hardware
>          breakpoints and software breakpoints.
>
>           * remote.c (remote_insert_breakpoint): If this breakpoint has
>           target-side commands but this stub doesn't support Z0 packets,
>           throw error.
>

Thanks for your remind.

2013-12-08  Hui Zhu  <hui@codesourcery.com>

	PR gdb/16101
	* breakpoint.c (insert_bp_location): Change hw_bp_err_string to
	bp_err_string.  Output error message of software breakpoints.
	Make solib error message output use same code with hardware
	breakpoints and software breakpoints.
	* remote.c (remote_insert_breakpoint): If this breakpoint has
	target-side commands but this stub doesn't support Z0 packets,
	throw error.

> [I may have gotten the indentation wrong, cut-n-paste in gmail, blech.
> The point is the blank line before "Output error ..." is wrong.  ISTM.]
>

I got some indentation issue with web gmail in a few weeks ago.
But this time it looks OK in my part.  I think they has fixed the issue.

Thanks,
Hui

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]

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;
 }
 
 \f
@@ -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

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]

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;
>  }
>  
>  \f
> @@ -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);
>  }

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]

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

[PATCH] Eliminate UNSUPPORTED_ERROR.

[Pedro Alves]

On 12/10/2013 05:34 PM, Pedro Alves wrote:
> On 12/09/2013 09:07 PM, Doug Evans wrote:

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

OK?

----------
Subject: [PATCH] Eliminate UNSUPPORTED_ERROR.

I have a case that could use an exception for "unsupported feature".
I found UNSUPPORTED_ERROR, but looking deeper, I think as is, reusing
it for other things would be fragile.  E.g., if the Python script
sourced by source_script_from_stream triggers any other missing
functionality that would result in UNSUPPORTED_ERROR being propagated
out to source_script_from_stream, that would confuse the error for
Python not being built into GDB.

This patch thus redoes things a little.  Instead of using an exception
for the "No Python" scenario, check whether Python is configured in
before actually trying to source the file.  It adds a new function
instead of using #ifdef HAVE_PYTHON directly, as that is better at
avoiding bitrot, as both Python and !Python paths are visible to the
compiler this way.

Tested on Fedora 17, with and without Python.

gdb/
2013-12-10  Pedro Alves  <palves@redhat.com>

	* cli/cli-cmds.c (source_script_from_stream) Use have_python
	instead of catching UNSUPPORTED_ERROR.
	* exceptions.h (UNSUPPORTED_ERROR): Delete.
	* python/python.c (source_python_script) [!HAVE_PYTHON]: Internal
	error if called.
	* python/python.h (have_python): New static inline function.
---
 gdb/cli/cli-cmds.c  | 27 ++++++++-------------------
 gdb/exceptions.h    |  3 ---
 gdb/python/python.c |  5 +++--
 gdb/python/python.h | 13 +++++++++++++
 4 files changed, 24 insertions(+), 24 deletions(-)

diff --git a/gdb/cli/cli-cmds.c b/gdb/cli/cli-cmds.c
index 52a6bc9..a0586ff 100644
--- a/gdb/cli/cli-cmds.c
+++ b/gdb/cli/cli-cmds.c
@@ -525,27 +525,16 @@ 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)
+      if (have_python ())
+	source_python_script (stream, file);
+      else if (script_ext_mode == script_ext_soft)
 	{
-	  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);
-	    }
+	  /* Fallback to GDB script mode.  */
+	  fseek (stream, 0, SEEK_SET);
+	  script_from_file (stream, (char*) file);
 	}
+      else
+	error (_("Python scripting is not supported in this copy of GDB."));
     }
   else
     script_from_file (stream, file);
diff --git a/gdb/exceptions.h b/gdb/exceptions.h
index 705f1a1..2cb8242 100644
--- a/gdb/exceptions.h
+++ b/gdb/exceptions.h
@@ -79,9 +79,6 @@ enum errors {
   /* Error accessing memory.  */
   MEMORY_ERROR,
 
-  /* Feature is not supported in this copy of GDB.  */
-  UNSUPPORTED_ERROR,
-
   /* Value not available.  E.g., a register was not collected in a
      traceframe.  */
   NOT_AVAILABLE_ERROR,
diff --git a/gdb/python/python.c b/gdb/python/python.c
index 1873936..55bb6cf 100644
--- a/gdb/python/python.c
+++ b/gdb/python/python.c
@@ -1390,8 +1390,9 @@ eval_python_from_control_command (struct command_line *cmd)
 void
 source_python_script (FILE *file, const char *filename)
 {
-  throw_error (UNSUPPORTED_ERROR,
-	       _("Python scripting is not supported in this copy of GDB."));
+  internal_error (__FILE__, __LINE__,
+		  _("source_python_script called when Python scripting is "
+		    "not supported."));
 }
 
 int
diff --git a/gdb/python/python.h b/gdb/python/python.h
index c07b2aa..abbb581 100644
--- a/gdb/python/python.h
+++ b/gdb/python/python.h
@@ -89,6 +89,19 @@ typedef enum py_frame_args
   CLI_ALL_VALUES
 } py_frame_args;
 
+/* Returns true if Python support is built into GDB, false
+   otherwise.  */
+
+static inline int
+have_python (void)
+{
+#ifdef HAVE_PYTHON
+  return 1;
+#else
+  return 0;
+#endif
+}
+
 extern void finish_python_initialization (void);
 
 void eval_python_from_control_command (struct command_line *);
-- 
1.7.11.7

Re: [PATCH] Eliminate UNSUPPORTED_ERROR.

[Doug Evans]

On Tue, Dec 10, 2013 at 10:14 AM, Pedro Alves <palves@redhat.com> wrote:
> On 12/10/2013 05:34 PM, Pedro Alves wrote:
>> On 12/09/2013 09:07 PM, Doug Evans wrote:
>
>>>>> --- 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.
>
> OK?

A few nits, but yep.

>
> gdb/
> 2013-12-10  Pedro Alves  <palves@redhat.com>
>
>         * cli/cli-cmds.c (source_script_from_stream) Use have_python
>         instead of catching UNSUPPORTED_ERROR.
>         * exceptions.h (UNSUPPORTED_ERROR): Delete.
>         * python/python.c (source_python_script) [!HAVE_PYTHON]: Internal
>         error if called.
>         * python/python.h (have_python): New static inline function.
> ---
>  gdb/cli/cli-cmds.c  | 27 ++++++++-------------------
>  gdb/exceptions.h    |  3 ---
>  gdb/python/python.c |  5 +++--
>  gdb/python/python.h | 13 +++++++++++++
>  4 files changed, 24 insertions(+), 24 deletions(-)
>
> diff --git a/gdb/cli/cli-cmds.c b/gdb/cli/cli-cmds.c
> index 52a6bc9..a0586ff 100644
> --- a/gdb/cli/cli-cmds.c
> +++ b/gdb/cli/cli-cmds.c
> @@ -525,27 +525,16 @@ 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)
> +      if (have_python ())
> +       source_python_script (stream, file);
> +      else if (script_ext_mode == script_ext_soft)
>         {
> -         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);
> -           }
> +         /* Fallback to GDB script mode.  */
> +         fseek (stream, 0, SEEK_SET);
> +         script_from_file (stream, (char*) file);

Remove the fseek and cast.

[that'll probably require removing the { } too, sigh]

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]

On Tue, Dec 10, 2013 at 9:34 AM, Pedro Alves <palves@redhat.com> wrote:
>> Doug wrote:
>> 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.

Hi.  I read this one after the actual patch.  No worries.

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

Fortunately I anticipated the patch and included a change in my updated series.
It'll need some tweaks if UNSUPPORTED_ERROR is removed but easy enough.

Re: [PATCH] Eliminate UNSUPPORTED_ERROR.

[Pedro Alves]

On 12/11/2013 04:33 PM, Doug Evans wrote:
>> > -         /* 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);
>> > -           }
>> > +         /* Fallback to GDB script mode.  */
>> > +         fseek (stream, 0, SEEK_SET);
>> > +         script_from_file (stream, (char*) file);
> Remove the fseek and cast.

Hmm, indeed the fseek doesn't look necessary.  Will do.  However,
that makes me wonder whether I'm missing something, as it doesn't
look necessary before my patch either.  Was it ever really
needed?  I see you added it (and the unnecessary cast too ;-) ) in:
  https://www.sourceware.org/ml/gdb-patches/2010-04/msg00110.html
but I can't tell why.

-- 
Pedro Alves

Re: [PATCH] Eliminate UNSUPPORTED_ERROR.

[Doug Evans]

On Wed, Dec 11, 2013 at 11:17 AM, Pedro Alves <palves@redhat.com> wrote:
> On 12/11/2013 04:33 PM, Doug Evans wrote:
>>> > -         /* 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);
>>> > -           }
>>> > +         /* Fallback to GDB script mode.  */
>>> > +         fseek (stream, 0, SEEK_SET);
>>> > +         script_from_file (stream, (char*) file);
>> Remove the fseek and cast.
>
> Hmm, indeed the fseek doesn't look necessary.  Will do.  However,
> that makes me wonder whether I'm missing something, as it doesn't
> look necessary before my patch either.  Was it ever really
> needed?

I think I added it to be extra conservative in case
source_python_script happened to read from the file.
[I know it normally wouldn't read and then throw an UNSUPPORTED_ERROR,
but I'm guessing I didn't want this code to assume that.]

Now that we're not even calling source_python_script the need is gone.

 I see you added it (and the unnecessary cast too ;-) ) in:
>   https://www.sourceware.org/ml/gdb-patches/2010-04/msg00110.html
> but I can't tell why.

script_from_file was changed to take a const char *arg the prior week.
Presumably I just didn't update that part of the patch.

Re: [PATCH] Eliminate UNSUPPORTED_ERROR.

[Pedro Alves]

On 12/12/2013 04:22 AM, Doug Evans wrote:
> On Wed, Dec 11, 2013 at 11:17 AM, Pedro Alves <palves@redhat.com> wrote:
>> On 12/11/2013 04:33 PM, Doug Evans wrote:
>>>>> -         /* 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);
>>>>> -           }
>>>>> +         /* Fallback to GDB script mode.  */
>>>>> +         fseek (stream, 0, SEEK_SET);
>>>>> +         script_from_file (stream, (char*) file);
>>> Remove the fseek and cast.
>>
>> Hmm, indeed the fseek doesn't look necessary.  Will do.  However,
>> that makes me wonder whether I'm missing something, as it doesn't
>> look necessary before my patch either.  Was it ever really
>> needed?
> 
> I think I added it to be extra conservative in case
> source_python_script happened to read from the file.
> [I know it normally wouldn't read and then throw an UNSUPPORTED_ERROR,
> but I'm guessing I didn't want this code to assume that.]
> 
> Now that we're not even calling source_python_script the need is gone.
> 
>  I see you added it (and the unnecessary cast too ;-) ) in:
>>   https://www.sourceware.org/ml/gdb-patches/2010-04/msg00110.html
>> but I can't tell why.
> 
> script_from_file was changed to take a const char *arg the prior week.
> Presumably I just didn't update that part of the patch.
> 

Alright, pushed, as below.
--------
Eliminate UNSUPPORTED_ERROR.

I have a case that could use an exception for "unsupported feature".
I found UNSUPPORTED_ERROR, but looking deeper, I think as is, reusing
it for other things would be fragile.  E.g., if the Python script
sourced by source_script_from_stream triggers any other missing
functionality that would result in UNSUPPORTED_ERROR being propagated
out to source_script_from_stream, that would confuse the error for
Python not being built into GDB.

This patch thus redoes things a little.  Instead of using an exception
for the "No Python" scenario, check whether Python is configured in
before actually trying to source the file.  It adds a new function
instead of using #ifdef HAVE_PYTHON directly, as that is better at
avoiding bitrot, as both Python and !Python paths are visible to the
compiler this way.

Tested on Fedora 17, with and without Python.

gdb/
2013-12-12  Pedro Alves  <palves@redhat.com>

	* cli/cli-cmds.c (source_script_from_stream) Use have_python
	instead of catching UNSUPPORTED_ERROR.
	* exceptions.h (UNSUPPORTED_ERROR): Delete.
	* python/python.c (source_python_script) [!HAVE_PYTHON]: Internal
	error if called.
	* python/python.h (have_python): New static inline function.
---
 gdb/ChangeLog       |  9 +++++++++
 gdb/cli/cli-cmds.c  | 26 +++++++-------------------
 gdb/exceptions.h    |  3 ---
 gdb/python/python.c |  5 +++--
 gdb/python/python.h | 13 +++++++++++++
 5 files changed, 32 insertions(+), 24 deletions(-)

diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index 40368b5..637a462 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,3 +1,12 @@
+2013-12-12  Pedro Alves  <palves@redhat.com>
+
+	* cli/cli-cmds.c (source_script_from_stream) Use have_python
+	instead of catching UNSUPPORTED_ERROR.
+	* exceptions.h (UNSUPPORTED_ERROR): Delete.
+	* python/python.c (source_python_script) [!HAVE_PYTHON]: Internal
+	error if called.
+	* python/python.h (have_python): New static inline function.
+
 2013-12-11  Doug Evans  <dje@google.com>
 
 	* dwarf2read.c (lookup_dwo_cutu): Include name of dwp file in
diff --git a/gdb/cli/cli-cmds.c b/gdb/cli/cli-cmds.c
index 52a6bc9..73e03cf 100644
--- a/gdb/cli/cli-cmds.c
+++ b/gdb/cli/cli-cmds.c
@@ -525,27 +525,15 @@ 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)
+      if (have_python ())
+	source_python_script (stream, file);
+      else if (script_ext_mode == script_ext_soft)
 	{
-	  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);
-	    }
+	  /* Fallback to GDB script mode.  */
+	  script_from_file (stream, file);
 	}
+      else
+	error (_("Python scripting is not supported in this copy of GDB."));
     }
   else
     script_from_file (stream, file);
diff --git a/gdb/exceptions.h b/gdb/exceptions.h
index 705f1a1..2cb8242 100644
--- a/gdb/exceptions.h
+++ b/gdb/exceptions.h
@@ -79,9 +79,6 @@ enum errors {
   /* Error accessing memory.  */
   MEMORY_ERROR,
 
-  /* Feature is not supported in this copy of GDB.  */
-  UNSUPPORTED_ERROR,
-
   /* Value not available.  E.g., a register was not collected in a
      traceframe.  */
   NOT_AVAILABLE_ERROR,
diff --git a/gdb/python/python.c b/gdb/python/python.c
index 1873936..55bb6cf 100644
--- a/gdb/python/python.c
+++ b/gdb/python/python.c
@@ -1390,8 +1390,9 @@ eval_python_from_control_command (struct command_line *cmd)
 void
 source_python_script (FILE *file, const char *filename)
 {
-  throw_error (UNSUPPORTED_ERROR,
-	       _("Python scripting is not supported in this copy of GDB."));
+  internal_error (__FILE__, __LINE__,
+		  _("source_python_script called when Python scripting is "
+		    "not supported."));
 }
 
 int
diff --git a/gdb/python/python.h b/gdb/python/python.h
index c07b2aa..abbb581 100644
--- a/gdb/python/python.h
+++ b/gdb/python/python.h
@@ -89,6 +89,19 @@ typedef enum py_frame_args
   CLI_ALL_VALUES
 } py_frame_args;
 
+/* Returns true if Python support is built into GDB, false
+   otherwise.  */
+
+static inline int
+have_python (void)
+{
+#ifdef HAVE_PYTHON
+  return 1;
+#else
+  return 0;
+#endif
+}
+
 extern void finish_python_initialization (void);
 
 void eval_python_from_control_command (struct command_line *);
-- 
1.7.11.7

breakpoint.c:insert_bp_location: Constify local. (was: 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]

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

A bit of an unrelated change, but OK, I'll bite.  ;-)

Pushed.
----------
breakpoint.c:insert_bp_location: Constify local.

gdb/
2013-12-12  Pedro Alves  <palves@redhat.com>

	* breakpoint.c (insert_bp_location): Make 'hw_bp_err_string' local
	const, and remove casts.
---
 gdb/ChangeLog    | 5 +++++
 gdb/breakpoint.c | 6 +++---
 2 files changed, 8 insertions(+), 3 deletions(-)

diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index 637a462..c49895b 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,5 +1,10 @@
 2013-12-12  Pedro Alves  <palves@redhat.com>
 
+	* breakpoint.c (insert_bp_location): Make 'hw_bp_err_string' local
+	const, and remove casts.
+
+2013-12-12  Pedro Alves  <palves@redhat.com>
+
 	* cli/cli-cmds.c (source_script_from_stream) Use have_python
 	instead of catching UNSUPPORTED_ERROR.
 	* exceptions.h (UNSUPPORTED_ERROR): Delete.
diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
index 111660f..589aa19 100644
--- a/gdb/breakpoint.c
+++ b/gdb/breakpoint.c
@@ -2396,7 +2396,7 @@ insert_bp_location (struct bp_location *bl,
 		    int *hw_bp_error_explained_already)
 {
   int val = 0;
-  char *hw_bp_err_string = NULL;
+  const char *hw_bp_err_string = NULL;
   struct gdb_exception e;
 
   if (!should_be_inserted (bl) || (bl->inserted && !bl->needs_update))
@@ -2501,7 +2501,7 @@ insert_bp_location (struct bp_location *bl,
 	  if (e.reason < 0)
 	    {
 	      val = 1;
-	      hw_bp_err_string = (char *) e.message;
+	      hw_bp_err_string = e.message;
 	    }
 	}
       else
@@ -2543,7 +2543,7 @@ insert_bp_location (struct bp_location *bl,
 	      if (e.reason < 0)
 	        {
 	          val = 1;
-	          hw_bp_err_string = (char *) e.message;
+	          hw_bp_err_string = e.message;
 	        }
 	    }
 	  else
-- 
1.7.11.7

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]

On 12/09/2013 09:07 PM, Doug Evans wrote:

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

I've extended the comment.  Updated patch below.
------------

Subject: [PATCH] Handle the case of a remote target supporting target side
 commands, but not on software breakpoints.

Although we can tell upfront whether a remote target supports target
side commands, we can only tell whether the target supports that in
combination with a given breakpoint kind (software, hardware,
watchpoints, etc.) when we go and try to insert such a breakpoint kind
the first time.  It's not desirable to make remote_insert_breakpoint
simply return -1 in this case, because if the breakpoint was set in a
shared library, insert_bp_location will assume that the breakpoint
insertion failed because the library wasn't mapped in.

insert_bp_location already handles errors/exceptions thrown from the
target_insert_xxx methods, exactly so the backend can tell the user
the detailed reason the insertion of hw breakpoints failed.  But, in
the case of software breakpoints, it discards the detailed error
message.

So the patch makes insert_bp_location use the error's message
for SW breakpoints too, and, introduces a NOT_SUPPORTED_ERROR error
code so that insert_bp_location doesn't confuse the error for failure
due to a shared library disappearing.

The result is:

(gdb) c
Warning:
Cannot insert breakpoint 2: Target doesn't support breakpoints that have target side commands.

2013-12-12  Pedro Alves  <palves@redhat.com>
	    Hui Zhu  <hui@codesourcery.com>

	PR gdb/16101
	* breakpoint.c (insert_bp_location): Rename hw_bp_err_string to
	bp_err_string.  Don't mark the location shlib_disabled if the
	error thrown wasn't a generic or memory error.  Catch errors
	thrown while inserting breakpoints in overlayed code.  Output
	error message of software breakpoints.
	* remote.c (remote_insert_breakpoint): If this breakpoint has
	target-side commands but this stub doesn't support Z0 packets,
	throw NOT_SUPPORTED_ERROR error.
	* exceptions.h (enum errors) <NOT_SUPPORTED_ERROR>: New error.
	* target.h (target_insert_breakpoint): Extend comment.
	(target_insert_hw_breakpoint): Add comment.
---
 gdb/breakpoint.c | 105 ++++++++++++++++++++++++++++++++++++++++---------------
 gdb/exceptions.h |   3 ++
 gdb/remote.c     |   6 ++++
 gdb/target.h     |  11 ++++--
 4 files changed, 95 insertions(+), 30 deletions(-)

diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
index 589aa19..169fafa 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;
-  const char *hw_bp_err_string = NULL;
+  enum errors bp_err = GDB_NO_ERROR;
+  const 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 = e.message;
+	      bp_err = e.error;
+	      bp_err_message = 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 = 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 = e.message;
+		  bp_err = e.error;
+		  bp_err_message = e.message;
 	        }
 	    }
 	  else
@@ -2554,13 +2577,23 @@ 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.  Unfortunately, some targets that implement
+	     breakpoint insertion themselves (necessary if this is a
+	     HW breakpoint, but SW breakpoints likewise) can't tell
+	     why the breakpoint insertion failed (e.g., the remote
+	     target doesn't define error codes), so we must treat
+	     generic errors as memory errors.  */
+	  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 +2608,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 +2660,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 +2705,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/exceptions.h b/gdb/exceptions.h
index 2cb8242..bd1cce4 100644
--- a/gdb/exceptions.h
+++ b/gdb/exceptions.h
@@ -97,6 +97,9 @@ enum errors {
   /* An undefined command was executed.  */
   UNDEFINED_COMMAND_ERROR,
 
+  /* Requested feature, method, mechanism, etc. is not supported.  */
+  NOT_SUPPORTED_ERROR,
+
   /* Add more errors here.  */
   NR_ERRORS
 };
diff --git a/gdb/remote.c b/gdb/remote.c
index 2ac8c36..dce5e05 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 (NOT_SUPPORTED_ERROR, _("\
+Target doesn't support breakpoints that have target side commands."));
+
   return memory_insert_breakpoint (gdbarch, bp_tgt);
 }
 
diff --git a/gdb/target.h b/gdb/target.h
index f22e5c6..d76f519 100644
--- a/gdb/target.h
+++ b/gdb/target.h
@@ -1123,8 +1123,10 @@ int target_write_memory_blocks (VEC(memory_write_request_s) *requests,
 #define	target_files_info()	\
      (*current_target.to_files_info) (&current_target)
 
-/* Insert a breakpoint at address BP_TGT->placed_address in the target
-   machine.  Result is 0 for success, non-zero for error.  */
+/* Insert a hardware breakpoint at address BP_TGT->placed_address in
+   the target machine.  Returns 0 for success, and returns non-zero or
+   throws an error (with a detailed failure reason error code and
+   message) otherwise.  */
 
 extern int target_insert_breakpoint (struct gdbarch *gdbarch,
 				     struct bp_target_info *bp_tgt);
@@ -1553,6 +1555,11 @@ extern int target_insert_mask_watchpoint (CORE_ADDR, CORE_ADDR, int);
 
 extern int target_remove_mask_watchpoint (CORE_ADDR, CORE_ADDR, int);
 
+/* Insert a hardware breakpoint at address BP_TGT->placed_address in
+   the target machine.  Returns 0 for success, and returns non-zero or
+   throws an error (with a detailed failure reason error code and
+   message) otherwise.  */
+
 #define target_insert_hw_breakpoint(gdbarch, bp_tgt) \
      (*current_target.to_insert_hw_breakpoint) (gdbarch, bp_tgt)
 
-- 
1.7.11.7

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]

On 12/12/2013 12:55 PM, Pedro Alves wrote:
> 2013-12-12  Pedro Alves  <palves@redhat.com>
> 	    Hui Zhu  <hui@codesourcery.com>
> 
> 	PR gdb/16101
> 	* breakpoint.c (insert_bp_location): Rename hw_bp_err_string to
> 	bp_err_string.  Don't mark the location shlib_disabled if the
> 	error thrown wasn't a generic or memory error.  Catch errors
> 	thrown while inserting breakpoints in overlayed code.  Output
> 	error message of software breakpoints.
> 	* remote.c (remote_insert_breakpoint): If this breakpoint has
> 	target-side commands but this stub doesn't support Z0 packets,
> 	throw NOT_SUPPORTED_ERROR error.
> 	* exceptions.h (enum errors) <NOT_SUPPORTED_ERROR>: New error.
> 	* target.h (target_insert_breakpoint): Extend comment.
> 	(target_insert_hw_breakpoint): Add comment.

I've pushed this one in.

-- 
Pedro Alves