Re: [PATCH 1/3] gdb, gdbserver, zpoint: report z_point support

[Guinevere Larsen <blarsen@redhat.com>]

On 28/03/2023 13:54, Mohamed Bouhaouel via Gdb-patches wrote:
> Report if the target supports Z-points when exchanging features
> between GDB and gdbserver.
>
> Signed-off-by: Mohamed Bouhaouel <mohamed.bouhaouel@intel.com>

Hi!

Thanks for working on this, and sorry for the long time to get a review.

The code changes look good, but the commit message could use some work. 
Calling them "Z-points" threw me for a loop that google couldn't help. 
Maybe rewording to something like:

Make GDB and gdbserver report which types of breakpoints and watchpoints 
are supported when exchanging features. This is done by replying with 
Z<type>+ (Supported) or Z<type>- (unsupported)

Also, I think this would warrant a NEWS entry and an update to the 
remote protocol documentation.

-- 
Cheers,
Guinevere Larsen
She/Her/Hers

> ---
>   gdb/remote.c           | 15 +++++++++++++++
>   gdbserver/mem-break.cc |  2 +-
>   gdbserver/mem-break.h  |  4 ++++
>   gdbserver/server.cc    |  7 +++++++
>   4 files changed, 27 insertions(+), 1 deletion(-)
>
> diff --git a/gdb/remote.c b/gdb/remote.c
> index 526df313ea7..1139ce3ddf7 100644
> --- a/gdb/remote.c
> +++ b/gdb/remote.c
> @@ -755,6 +755,9 @@ class remote_target : public process_stratum_target
>     int insert_watchpoint (CORE_ADDR, int, enum target_hw_bp_type,
>   			 struct expression *) override;
>   
> +  /* Returns TRUE if GDB Z breakpoint type is supported, FALSE otherwise.  */
> +  bool supports_z_point_type (int);
> +
>     int remove_watchpoint (CORE_ADDR, int, enum target_hw_bp_type,
>   			 struct expression *) override;
>   
> @@ -5604,6 +5607,11 @@ static const struct protocol_feature remote_protocol_features[] = {
>     { "no-resumed", PACKET_DISABLE, remote_supported_packet, PACKET_no_resumed },
>     { "memory-tagging", PACKET_DISABLE, remote_supported_packet,
>       PACKET_memory_tagging_feature },
> +  { "Z0", PACKET_SUPPORT_UNKNOWN, remote_supported_packet, PACKET_Z0 },
> +  { "Z1", PACKET_SUPPORT_UNKNOWN, remote_supported_packet, PACKET_Z1 },
> +  { "Z2", PACKET_SUPPORT_UNKNOWN, remote_supported_packet, PACKET_Z2 },
> +  { "Z3", PACKET_SUPPORT_UNKNOWN, remote_supported_packet, PACKET_Z3 },
> +  { "Z4", PACKET_SUPPORT_UNKNOWN, remote_supported_packet, PACKET_Z4 },
>   };
>   
>   static char *remote_support_xml;
> @@ -10843,6 +10851,13 @@ watchpoint_to_Z_packet (int type)
>       }
>   }
>   
> +bool
> +remote_target::supports_z_point_type (int type)
> +{
> +  Z_packet_type packet = watchpoint_to_Z_packet (type);
> +  return (m_features.packet_support (PACKET_Z0 + packet) != PACKET_DISABLE);
> +}
> +
>   int
>   remote_target::insert_watchpoint (CORE_ADDR addr, int len,
>   				  enum target_hw_bp_type type, struct expression *cond)
> diff --git a/gdbserver/mem-break.cc b/gdbserver/mem-break.cc
> index c669842228d..20434f7c9b4 100644
> --- a/gdbserver/mem-break.cc
> +++ b/gdbserver/mem-break.cc
> @@ -987,7 +987,7 @@ find_gdb_breakpoint (char z_type, CORE_ADDR addr, int kind)
>     return NULL;
>   }
>   
> -static int
> +int
>   z_type_supported (char z_type)
>   {
>     return (z_type >= '0' && z_type <= '4'
> diff --git a/gdbserver/mem-break.h b/gdbserver/mem-break.h
> index 9bf7aa84932..458ba0e6be2 100644
> --- a/gdbserver/mem-break.h
> +++ b/gdbserver/mem-break.h
> @@ -276,4 +276,8 @@ int remove_memory_breakpoint (struct raw_breakpoint *bp);
>   void clone_all_breakpoints (struct thread_info *child_thread,
>   			    const struct thread_info *parent_thread);
>   
> +
> +/* Returns TRUE if Z_TYPE is supported by the target.  */
> +
> +int z_type_supported (char z_type);
>   #endif /* GDBSERVER_MEM_BREAK_H */
> diff --git a/gdbserver/server.cc b/gdbserver/server.cc
> index 949849b63a2..e758cd0d509 100644
> --- a/gdbserver/server.cc
> +++ b/gdbserver/server.cc
> @@ -2480,6 +2480,13 @@ handle_query (char *own_buf, int packet_len, int *new_packet_len_p)
>         if (target_supports_memory_tagging ())
>   	strcat (own_buf, ";memory-tagging+");
>   
> +      /* Z points support.  */
> +      strcat (own_buf, z_type_supported ('0') ? ";Z0+" : ";Z0-");
> +      strcat (own_buf, z_type_supported ('1') ? ";Z1+" : ";Z1-");
> +      strcat (own_buf, z_type_supported ('2') ? ";Z2+" : ";Z2-");
> +      strcat (own_buf, z_type_supported ('3') ? ";Z3+" : ";Z3-");
> +      strcat (own_buf, z_type_supported ('4') ? ";Z4+" : ";Z4-");
> +
>         /* Reinitialize components as needed for the new connection.  */
>         hostio_handle_new_gdb_connection ();
>         target_handle_new_gdb_connection ();