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: 1676 bytes --]

On 8/22/23 16:43, Pedro Alves wrote:
> On 2023-08-21 11:53, Tom de Vries via Gdb-patches wrote:
>> 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 const reference instead:
>> ...
>> const gdb::optional<gdb::byte_vector> &
>> remote_target::thread_info_to_thread_handle (struct thread_info *tp)
>> ...
>>
>> Returning a gdb::optional allows us to return a nullptr, or std::nullopt in
>> std::optional terms, something that is required by
>> thread_db_target::thread_info_to_thread_handle.
>>
>> In gdb we use gdb::optional instead std::optional, because std::optional is
>> availabe starting c++17 and we support c++11 and c++14, but gdb::nullopt is
>> currently not available, though a submission is available [1].
>>
>> So we use a kludge gdb_optional_byte_vector_nullopt.
>>
> 
> IMHO making the function return gdb::array_view<const gdb_byte>
> would be even better.  Then the byte_vector is completely an implementation
> detail, and, you wouldn't need to wrap with optional, as you could just
> return an empty array_view, like currently we return an empty byte_vector.
> 

Hi Pedro,

thanks for the review.

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.

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.

Thanks,
- Tom

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

From 5efef08023db7f5cf63a028ddf091db591272df3 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        | 14 ++++++++++++++
 gdb/target-delegates.c    | 16 ++++++++--------
 gdb/target.c              |  2 +-
 gdb/target.h              | 10 +++++++---
 7 files changed, 42 insertions(+), 23 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..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)
+{
+  gdb_puts ("{", gdb_stdlog);
+
+  for (size_t i = 0; i < vector.size (); i++)
+    {
+      gdb_printf (gdb_stdlog, " %s",
+		  phex_nz (vector[i], 1));
+    }
+  gdb_puts (" }", gdb_stdlog);
+}
+
 #endif /* TARGET_DEBUG_H */
diff --git a/gdb/target-delegates.c b/gdb/target-delegates.c
index e4e14380181..81261b13211 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::byte_vector result;
+  gdb::array_view<const_gdb_byte> result;
   gdb_printf (gdb_stdlog, "-> %s->thread_info_to_thread_handle (...)\n", this->beneath ()->shortname ());
   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: a4822788d7c41926941b1c6c405c82aeffb72ad7
-- 
2.35.3