Re: [PATCH 2/2] [gdb/build] Return const reference in thread_info_to_thread_handle

[Tom de Vries <tdevries@suse.de>]

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

On 8/23/23 17:43, Pedro Alves wrote:
> On 22/08/23 18:34, Tom de Vries wrote:
> 
>> I've given the gdb::array_view<const gdb_byte> a try, and that works fine, and greatly simplifies the patch, so thanks for that suggestion.
> 
> Great!
> 
>> The first patch is no longer necessary, but we could consider committing it nevertheless, since the work is done and may be needed in the future.
> 
> I agree.
> 
>>   
>> -gdb::byte_vector
>> +gdb::array_view<const gdb_byte>
>>   remote_target::thread_info_to_thread_handle (struct thread_info *tp)
>>   {
>>     remote_thread_info *priv = get_remote_thread_info (tp);
>> diff --git a/gdb/target-debug.h b/gdb/target-debug.h
>> index acb01d47e7c..31c25e9aefd 100644
>> --- a/gdb/target-debug.h
>> +++ b/gdb/target-debug.h
>> @@ -236,4 +236,18 @@ target_debug_print_gdb_byte_vector_r (gdb::byte_vector &vector)
>>   {
>>     target_debug_print_const_gdb_byte_vector_r (vector);
>>   }
>> +
>> +static void
>> +target_debug_print_gdb_array_view_const_gdb_byte (gdb::array_view<const gdb_byte> &vector)
> 
> array_views should be passed by value normally (because they're small).  Did you run into something
> that forced this to be a reference?  Or was it just copied from the vector case?  If it
> works, we should just drop the &.
> 

Done.

> Also, one of the points of array_view is that you can pass a vector as array_view
> argument, like you can pass std::string as string_view argument -- would it work to make
> target_debug_print_const_gdb_byte_vector_r call this new
> target_debug_print_gdb_array_view_const_gdb_byte instead of duplicating that code?
> 

Done, updated version attached.

Thanks,
- Tom


[-- Attachment #2: 0001-gdb-build-Return-gdb-array_view-in-thread_info_to_th.patch --]
[-- Type: text/x-patch, Size: 9355 bytes --]

From dfc97455f7eb28bdf071b63229e68df20d95d710 Mon Sep 17 00:00:00 2001
From: Tom de Vries <tdevries@suse.de>
Date: Tue, 22 Aug 2023 17:15:48 +0200
Subject: [PATCH] [gdb/build] Return gdb::array_view in
 thread_info_to_thread_handle

In remote_target::thread_info_to_thread_handle we return a copy:
...
gdb::byte_vector
remote_target::thread_info_to_thread_handle (struct thread_info *tp)
{
  remote_thread_info *priv = get_remote_thread_info (tp);
  return priv->thread_handle;
}
...

Fix this by returning a gdb::array_view instead:
...
gdb::array_view<const gdb_byte>
remote_target::thread_info_to_thread_handle (struct thread_info *tp)
...

Tested on x86_64-linux.

This fixes the build when building with -std=c++20.
---
 gdb/linux-thread-db.c     | 13 +++++++------
 gdb/python/py-infthread.c |  4 ++--
 gdb/remote.c              |  6 +++---
 gdb/target-debug.h        |  8 +++++++-
 gdb/target-delegates.c    | 16 ++++++++--------
 gdb/target.c              |  2 +-
 gdb/target.h              | 10 +++++++---
 7 files changed, 35 insertions(+), 24 deletions(-)

diff --git a/gdb/linux-thread-db.c b/gdb/linux-thread-db.c
index 7d9fd57da0c..16c250c104d 100644
--- a/gdb/linux-thread-db.c
+++ b/gdb/linux-thread-db.c
@@ -107,7 +107,7 @@ class thread_db_target final : public target_ops
   thread_info *thread_handle_to_thread_info (const gdb_byte *thread_handle,
 					     int handle_len,
 					     inferior *inf) override;
-  gdb::byte_vector thread_info_to_thread_handle (struct thread_info *) override;
+  gdb::array_view<const gdb_byte> thread_info_to_thread_handle (struct thread_info *) override;
 };
 
 static std::string libthread_db_search_path = LIBTHREAD_DB_SEARCH_PATH;
@@ -312,6 +312,7 @@ struct thread_db_thread_info : public private_thread_info
   /* Cached thread state.  */
   td_thrhandle_t th {};
   thread_t tid {};
+  gdb::optional<gdb::byte_vector> thread_handle;
 };
 
 static thread_db_thread_info *
@@ -1724,20 +1725,20 @@ thread_db_target::thread_handle_to_thread_info (const gdb_byte *thread_handle,
 
 /* Return the thread handle associated the thread_info pointer TP.  */
 
-gdb::byte_vector
+gdb::array_view<const gdb_byte>
 thread_db_target::thread_info_to_thread_handle (struct thread_info *tp)
 {
   thread_db_thread_info *priv = get_thread_db_thread_info (tp);
 
   if (priv == NULL)
-    return gdb::byte_vector ();
+    return {};
 
   int handle_size = sizeof (priv->tid);
-  gdb::byte_vector rv (handle_size);
+  priv->thread_handle.emplace (handle_size);
 
-  memcpy (rv.data (), &priv->tid, handle_size);
+  memcpy (priv->thread_handle->data (), &priv->tid, handle_size);
 
-  return rv;
+  return *priv->thread_handle;
 }
 
 /* Get the address of the thread local variable in load module LM which
diff --git a/gdb/python/py-infthread.c b/gdb/python/py-infthread.c
index 1bd25d01320..00d7171de64 100644
--- a/gdb/python/py-infthread.c
+++ b/gdb/python/py-infthread.c
@@ -291,8 +291,8 @@ thpy_thread_handle (PyObject *self, PyObject *args)
   thread_object *thread_obj = (thread_object *) self;
   THPY_REQUIRE_VALID (thread_obj);
 
-  gdb::byte_vector hv;
-  
+  gdb::array_view<const gdb_byte> hv;
+
   try
     {
       hv = target_thread_info_to_thread_handle (thread_obj->thread);
diff --git a/gdb/remote.c b/gdb/remote.c
index 7abe08439b5..5af40bd704c 100644
--- a/gdb/remote.c
+++ b/gdb/remote.c
@@ -784,8 +784,8 @@ class remote_target : public process_stratum_target
 					     int handle_len,
 					     inferior *inf) override;
 
-  gdb::byte_vector thread_info_to_thread_handle (struct thread_info *tp)
-						 override;
+  gdb::array_view<const gdb_byte> thread_info_to_thread_handle (struct thread_info *tp)
+    override;
 
   void stop (ptid_t) override;
 
@@ -14550,7 +14550,7 @@ remote_target::thread_handle_to_thread_info (const gdb_byte *thread_handle,
   return NULL;
 }
 
-gdb::byte_vector
+gdb::array_view<const gdb_byte>
 remote_target::thread_info_to_thread_handle (struct thread_info *tp)
 {
   remote_thread_info *priv = get_remote_thread_info (tp);
diff --git a/gdb/target-debug.h b/gdb/target-debug.h
index acb01d47e7c..082550df325 100644
--- a/gdb/target-debug.h
+++ b/gdb/target-debug.h
@@ -219,7 +219,7 @@ target_debug_print_size_t (size_t size)
 }
 
 static void
-target_debug_print_const_gdb_byte_vector_r (const gdb::byte_vector &vector)
+target_debug_print_gdb_array_view_const_gdb_byte (gdb::array_view<const gdb_byte> vector)
 {
   gdb_puts ("{", gdb_stdlog);
 
@@ -231,6 +231,12 @@ target_debug_print_const_gdb_byte_vector_r (const gdb::byte_vector &vector)
   gdb_puts (" }", gdb_stdlog);
 }
 
+static void
+target_debug_print_const_gdb_byte_vector_r (const gdb::byte_vector &vector)
+{
+  target_debug_print_gdb_array_view_const_gdb_byte (vector);
+}
+
 static void
 target_debug_print_gdb_byte_vector_r (gdb::byte_vector &vector)
 {
diff --git a/gdb/target-delegates.c b/gdb/target-delegates.c
index 06f22d57c27..715e50b8fb8 100644
--- a/gdb/target-delegates.c
+++ b/gdb/target-delegates.c
@@ -89,7 +89,7 @@ struct dummy_target : public target_ops
   const char *extra_thread_info (thread_info *arg0) override;
   const char *thread_name (thread_info *arg0) override;
   thread_info *thread_handle_to_thread_info (const gdb_byte *arg0, int arg1, inferior *arg2) override;
-  gdb::byte_vector thread_info_to_thread_handle (struct thread_info *arg0) override;
+  gdb::array_view<const_gdb_byte> thread_info_to_thread_handle (struct thread_info *arg0) override;
   void stop (ptid_t arg0) override;
   void interrupt () override;
   void pass_ctrlc () override;
@@ -263,7 +263,7 @@ struct debug_target : public target_ops
   const char *extra_thread_info (thread_info *arg0) override;
   const char *thread_name (thread_info *arg0) override;
   thread_info *thread_handle_to_thread_info (const gdb_byte *arg0, int arg1, inferior *arg2) override;
-  gdb::byte_vector thread_info_to_thread_handle (struct thread_info *arg0) override;
+  gdb::array_view<const_gdb_byte> thread_info_to_thread_handle (struct thread_info *arg0) override;
   void stop (ptid_t arg0) override;
   void interrupt () override;
   void pass_ctrlc () override;
@@ -1871,28 +1871,28 @@ debug_target::thread_handle_to_thread_info (const gdb_byte *arg0, int arg1, infe
   return result;
 }
 
-gdb::byte_vector
+gdb::array_view<const_gdb_byte>
 target_ops::thread_info_to_thread_handle (struct thread_info *arg0)
 {
   return this->beneath ()->thread_info_to_thread_handle (arg0);
 }
 
-gdb::byte_vector
+gdb::array_view<const_gdb_byte>
 dummy_target::thread_info_to_thread_handle (struct thread_info *arg0)
 {
-  return gdb::byte_vector ();
+  return gdb::array_view<const gdb_byte> ();
 }
 
-gdb::byte_vector
+gdb::array_view<const_gdb_byte>
 debug_target::thread_info_to_thread_handle (struct thread_info *arg0)
 {
   gdb_printf (gdb_stdlog, "-> %s->thread_info_to_thread_handle (...)\n", this->beneath ()->shortname ());
-  gdb::byte_vector result
+  gdb::array_view<const_gdb_byte> result
     = this->beneath ()->thread_info_to_thread_handle (arg0);
   gdb_printf (gdb_stdlog, "<- %s->thread_info_to_thread_handle (", this->beneath ()->shortname ());
   target_debug_print_struct_thread_info_p (arg0);
   gdb_puts (") = ", gdb_stdlog);
-  target_debug_print_gdb_byte_vector (result);
+  target_debug_print_gdb_array_view_const_gdb_byte (result);
   gdb_puts ("\n", gdb_stdlog);
   return result;
 }
diff --git a/gdb/target.c b/gdb/target.c
index 16f43d072cd..8b1d48d91d9 100644
--- a/gdb/target.c
+++ b/gdb/target.c
@@ -2640,7 +2640,7 @@ target_thread_handle_to_thread_info (const gdb_byte *thread_handle,
 
 /* See target.h.  */
 
-gdb::byte_vector
+gdb::array_view<const gdb_byte>
 target_thread_info_to_thread_handle (struct thread_info *tip)
 {
   target_ops *target = current_inferior ()->top_target ();
diff --git a/gdb/target.h b/gdb/target.h
index 6ae400e2cc2..0cea955cbd7 100644
--- a/gdb/target.h
+++ b/gdb/target.h
@@ -39,6 +39,10 @@ struct expression;
 struct dcache_struct;
 struct inferior;
 
+/* Define const gdb_byte using one identifier, to make it easy for
+   make-target-delegates.py to parse.  */
+typedef const gdb_byte const_gdb_byte;
+
 #include "infrun.h" /* For enum exec_direction_kind.  */
 #include "breakpoint.h" /* For enum bptype.  */
 #include "gdbsupport/scoped_restore.h"
@@ -679,8 +683,8 @@ struct target_ops
 						       inferior *inf)
       TARGET_DEFAULT_RETURN (NULL);
     /* See target_thread_info_to_thread_handle.  */
-    virtual gdb::byte_vector thread_info_to_thread_handle (struct thread_info *)
-      TARGET_DEFAULT_RETURN (gdb::byte_vector ());
+    virtual gdb::array_view<const_gdb_byte> thread_info_to_thread_handle (struct thread_info *)
+      TARGET_DEFAULT_RETURN (gdb::array_view<const gdb_byte> ());
     virtual void stop (ptid_t)
       TARGET_DEFAULT_IGNORE ();
     virtual void interrupt ()
@@ -1924,7 +1928,7 @@ extern struct thread_info *target_thread_handle_to_thread_info
 /* Given a thread, return the thread handle, a target-specific sequence of
    bytes which serves as a thread identifier within the program being
    debugged.  */
-extern gdb::byte_vector target_thread_info_to_thread_handle
+extern gdb::array_view<const gdb_byte> target_thread_info_to_thread_handle
   (struct thread_info *);
 
 /* Attempts to find the pathname of the executable file

base-commit: 21f8c9c1874f144bbe387874f586e61d4742e6eb
-- 
2.35.3