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 <hui_zhu@mentor.com>]

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