[PATCH 0/7] Rewrite gdb_bfd_openr_iovec to be type-safe

[PATCH 0/7] Rewrite gdb_bfd_openr_iovec to be type-safe

[Tom Tromey]

This series rewrites gdb_bfd_openr_iovec to be type-safe.

Note that I can't really test the solib-rocm.c changes, this was done
as a best effort.

Regression tested on x86-64 Fedora 36.

---
Tom Tromey (7):
      Introduce type-safe variant of gdb_bfd_openr_iovec
      Small constructor change to target_buffer
      Convert mem_bfd_iovec to new type-safe gdb_bfd_openr_iovec
      Convert target fileio to new type-safe gdb_bfd_openr_iovec
      Convert minidebug to new type-safe gdb_bfd_openr_iovec
      Convert solib-rocm to new type-safe gdb_bfd_openr_iovec
      Remove old gdb_bfd_openr_iovec

 gdb/gdb_bfd.c    | 211 +++++++++++++++++++++++++++----------------------------
 gdb/gdb_bfd.h    |  43 ++++++++----
 gdb/minidebug.c  |  98 +++++++++++---------------
 gdb/solib-rocm.c |  39 +++-------
 4 files changed, 182 insertions(+), 209 deletions(-)
---
base-commit: 1f08d324601ef7fd4cce3cff8f8db4a774791828
change-id: 20230824-gdb-bfd-vec-ba7f6c1139bb

Best regards,
-- 
Tom Tromey <tromey@adacore.com>

[PATCH 1/7] Introduce type-safe variant of gdb_bfd_openr_iovec

[Tom Tromey]

This patch adds a new, type-safe variant of gdb_bfd_openr_iovec.  In
this approach, the underlying user data is simply an object, the
callbacks are methods, and the "open" function is a function view.
Nothing uses this new code yet.
---
 gdb/gdb_bfd.c | 42 ++++++++++++++++++++++++++++++++++++++++++
 gdb/gdb_bfd.h | 31 +++++++++++++++++++++++++++++++
 2 files changed, 73 insertions(+)

diff --git a/gdb/gdb_bfd.c b/gdb/gdb_bfd.c
index 3765561cbe9..0bb97460898 100644
--- a/gdb/gdb_bfd.c
+++ b/gdb/gdb_bfd.c
@@ -907,6 +907,48 @@ gdb_bfd_openw (const char *filename, const char *target)
 
 /* See gdb_bfd.h.  */
 
+gdb_bfd_ref_ptr
+gdb_bfd_openr_iovec (const char *filename, const char *target,
+		     gdb_iovec_opener_ftype open_fn)
+{
+  auto do_open = [] (bfd *nbfd, void *closure) -> void *
+  {
+    auto real_opener = (gdb_iovec_opener_ftype *) closure;
+    return (*real_opener) (nbfd);
+  };
+
+  auto read_trampoline = [] (bfd *nbfd, void *stream, void *buf,
+			     file_ptr nbytes, file_ptr offset) -> file_ptr
+  {
+    gdb_bfd_iovec_base *obj = (gdb_bfd_iovec_base *) stream;
+    return obj->read (nbfd, buf, nbytes, offset);
+  };
+
+  auto stat_trampoline = [] (struct bfd *abfd, void *stream,
+			     struct stat *sb) -> int
+  {
+    gdb_bfd_iovec_base *obj = (gdb_bfd_iovec_base *) stream;
+    return obj->stat (abfd, sb);
+  };
+
+  auto close_trampoline = [] (struct bfd *nbfd, void *stream) -> int
+  {
+    gdb_bfd_iovec_base *obj = (gdb_bfd_iovec_base *) stream;
+    delete obj;
+    /* Success.  */
+    return 0;
+  };
+
+  bfd *result = bfd_openr_iovec (filename, target,
+				 do_open, &open_fn,
+				 read_trampoline, close_trampoline,
+				 stat_trampoline);
+
+  return gdb_bfd_ref_ptr::new_reference (result);
+}
+
+/* See gdb_bfd.h.  */
+
 gdb_bfd_ref_ptr
 gdb_bfd_openr_iovec (const char *filename, const char *target,
 		     void *(*open_func) (struct bfd *nbfd,
diff --git a/gdb/gdb_bfd.h b/gdb/gdb_bfd.h
index d15b1106d9a..ae374f5d7ae 100644
--- a/gdb/gdb_bfd.h
+++ b/gdb/gdb_bfd.h
@@ -22,6 +22,7 @@
 
 #include "registry.h"
 #include "gdbsupport/byte-vector.h"
+#include "gdbsupport/function-view.h"
 #include "gdbsupport/gdb_ref_ptr.h"
 #include "gdbsupport/iterator-range.h"
 #include "gdbsupport/next-iterator.h"
@@ -150,6 +151,36 @@ gdb_bfd_ref_ptr gdb_bfd_openr (const char *, const char *);
 
 gdb_bfd_ref_ptr gdb_bfd_openw (const char *, const char *);
 
+/* The base class for BFD "iovec" implementations.  This is used by
+   gdb_bfd_openr_iovec and enables better type safety.  */
+
+class gdb_bfd_iovec_base
+{
+protected:
+
+  gdb_bfd_iovec_base () = default;
+
+public:
+
+  virtual ~gdb_bfd_iovec_base () = default;
+
+  /* The "read" callback.  */
+  virtual file_ptr read (bfd *abfd, void *buffer, file_ptr nbytes,
+			 file_ptr offset) = 0;
+
+  /* The "stat" callback.  */
+  virtual int stat (struct bfd *abfd, struct stat *sb) = 0;
+};
+
+/* The type of the function used to open a new iovec-based BFD.  */
+using gdb_iovec_opener_ftype
+     = gdb::function_view<gdb_bfd_iovec_base * (bfd *)>;
+
+/* A type-safe wrapper for bfd_openr_iovec.  */
+
+gdb_bfd_ref_ptr gdb_bfd_openr_iovec (const char *filename, const char *target,
+				     gdb_iovec_opener_ftype open_fn);
+
 /* A wrapper for bfd_openr_iovec that initializes the gdb-specific
    reference count.  */
 

-- 
2.40.1

Re: [PATCH 1/7] Introduce type-safe variant of gdb_bfd_openr_iovec

[Lancelot SIX]

Hi Tom,

On Thu, Aug 24, 2023 at 11:12:18AM -0600, Tom Tromey via Gdb-patches wrote:
> This patch adds a new, type-safe variant of gdb_bfd_openr_iovec.  In
> this approach, the underlying user data is simply an object, the
> callbacks are methods, and the "open" function is a function view.
> Nothing uses this new code yet.
> ---
>  gdb/gdb_bfd.c | 42 ++++++++++++++++++++++++++++++++++++++++++
>  gdb/gdb_bfd.h | 31 +++++++++++++++++++++++++++++++
>  2 files changed, 73 insertions(+)
> 
> diff --git a/gdb/gdb_bfd.c b/gdb/gdb_bfd.c
> index 3765561cbe9..0bb97460898 100644
> --- a/gdb/gdb_bfd.c
> +++ b/gdb/gdb_bfd.c
> @@ -907,6 +907,48 @@ gdb_bfd_openw (const char *filename, const char *target)
>  
>  /* See gdb_bfd.h.  */
>  
> +gdb_bfd_ref_ptr
> +gdb_bfd_openr_iovec (const char *filename, const char *target,
> +		     gdb_iovec_opener_ftype open_fn)
> +{
> +  auto do_open = [] (bfd *nbfd, void *closure) -> void *
> +  {
> +    auto real_opener = (gdb_iovec_opener_ftype *) closure;

I think I would have a personal preference toward using static_cast
here, but in this case the C-style cast and static cast are equivalent.

Other than this remark, this looks good to me.
Reviewed-By: Lancelot Six <lancelot.six@amd.com>

Best,
Lancelot.

> +    return (*real_opener) (nbfd);
> +  };
> +
> +  auto read_trampoline = [] (bfd *nbfd, void *stream, void *buf,
> +			     file_ptr nbytes, file_ptr offset) -> file_ptr
> +  {
> +    gdb_bfd_iovec_base *obj = (gdb_bfd_iovec_base *) stream;
> +    return obj->read (nbfd, buf, nbytes, offset);
> +  };
> +
> +  auto stat_trampoline = [] (struct bfd *abfd, void *stream,
> +			     struct stat *sb) -> int
> +  {
> +    gdb_bfd_iovec_base *obj = (gdb_bfd_iovec_base *) stream;
> +    return obj->stat (abfd, sb);
> +  };
> +
> +  auto close_trampoline = [] (struct bfd *nbfd, void *stream) -> int
> +  {
> +    gdb_bfd_iovec_base *obj = (gdb_bfd_iovec_base *) stream;
> +    delete obj;
> +    /* Success.  */
> +    return 0;
> +  };
> +
> +  bfd *result = bfd_openr_iovec (filename, target,
> +				 do_open, &open_fn,
> +				 read_trampoline, close_trampoline,
> +				 stat_trampoline);
> +
> +  return gdb_bfd_ref_ptr::new_reference (result);
> +}
> +
> +/* See gdb_bfd.h.  */
> +
>  gdb_bfd_ref_ptr
>  gdb_bfd_openr_iovec (const char *filename, const char *target,
>  		     void *(*open_func) (struct bfd *nbfd,
> diff --git a/gdb/gdb_bfd.h b/gdb/gdb_bfd.h
> index d15b1106d9a..ae374f5d7ae 100644
> --- a/gdb/gdb_bfd.h
> +++ b/gdb/gdb_bfd.h
> @@ -22,6 +22,7 @@
>  
>  #include "registry.h"
>  #include "gdbsupport/byte-vector.h"
> +#include "gdbsupport/function-view.h"
>  #include "gdbsupport/gdb_ref_ptr.h"
>  #include "gdbsupport/iterator-range.h"
>  #include "gdbsupport/next-iterator.h"
> @@ -150,6 +151,36 @@ gdb_bfd_ref_ptr gdb_bfd_openr (const char *, const char *);
>  
>  gdb_bfd_ref_ptr gdb_bfd_openw (const char *, const char *);
>  
> +/* The base class for BFD "iovec" implementations.  This is used by
> +   gdb_bfd_openr_iovec and enables better type safety.  */
> +
> +class gdb_bfd_iovec_base
> +{
> +protected:
> +
> +  gdb_bfd_iovec_base () = default;
> +
> +public:
> +
> +  virtual ~gdb_bfd_iovec_base () = default;
> +
> +  /* The "read" callback.  */
> +  virtual file_ptr read (bfd *abfd, void *buffer, file_ptr nbytes,
> +			 file_ptr offset) = 0;
> +
> +  /* The "stat" callback.  */
> +  virtual int stat (struct bfd *abfd, struct stat *sb) = 0;
> +};
> +
> +/* The type of the function used to open a new iovec-based BFD.  */
> +using gdb_iovec_opener_ftype
> +     = gdb::function_view<gdb_bfd_iovec_base * (bfd *)>;
> +
> +/* A type-safe wrapper for bfd_openr_iovec.  */
> +
> +gdb_bfd_ref_ptr gdb_bfd_openr_iovec (const char *filename, const char *target,
> +				     gdb_iovec_opener_ftype open_fn);
> +
>  /* A wrapper for bfd_openr_iovec that initializes the gdb-specific
>     reference count.  */
>  
> 
> -- 
> 2.40.1
> 

Re: [PATCH 1/7] Introduce type-safe variant of gdb_bfd_openr_iovec

[Tom Tromey]

>>>>> "Lancelot" == Lancelot SIX <lsix@lancelotsix.com> writes:

>> +    auto real_opener = (gdb_iovec_opener_ftype *) closure;

Lancelot> I think I would have a personal preference toward using static_cast
Lancelot> here, but in this case the C-style cast and static cast are equivalent.

I made this change.

Tom

[PATCH 2/7] Small constructor change to target_buffer

[Tom Tromey]

This changes the target_buffer constructor to initialize m_filename
rather than assign to it.
---
 gdb/gdb_bfd.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/gdb/gdb_bfd.c b/gdb/gdb_bfd.c
index 0bb97460898..733d5e109bb 100644
--- a/gdb/gdb_bfd.c
+++ b/gdb/gdb_bfd.c
@@ -226,10 +226,10 @@ struct target_buffer
      target memory.  */
   target_buffer (CORE_ADDR base, ULONGEST size)
     : m_base (base),
-      m_size (size)
+      m_size (size),
+      m_filename (xstrprintf ("<in-memory@%s>",
+			      core_addr_to_string_nz (m_base)))
   {
-    m_filename
-      = xstrprintf ("<in-memory@%s>", core_addr_to_string_nz (m_base));
   }
 
   /* Return the size of the in-memory BFD file.  */

-- 
2.40.1

[PATCH 3/7] Convert mem_bfd_iovec to new type-safe gdb_bfd_openr_iovec

[Tom Tromey]

This converts the mem_bfd_iovec / target_buffer code to use the new
type-safe gdb_bfd_openr_iovec.
---
 gdb/gdb_bfd.c | 61 ++++++++++++++++++++---------------------------------------
 1 file changed, 20 insertions(+), 41 deletions(-)

diff --git a/gdb/gdb_bfd.c b/gdb/gdb_bfd.c
index 733d5e109bb..2f8bb33997b 100644
--- a/gdb/gdb_bfd.c
+++ b/gdb/gdb_bfd.c
@@ -220,7 +220,7 @@ gdb_bfd_has_target_filename (struct bfd *abfd)
 /* For `gdb_bfd_open_from_target_memory`.  An object that manages the
    details of a BFD in target memory.  */
 
-struct target_buffer
+struct target_buffer : public gdb_bfd_iovec_base
 {
   /* Constructor.  BASE and SIZE define where the BFD can be found in
      target memory.  */
@@ -245,6 +245,11 @@ struct target_buffer
   const char *filename () const
   { return m_filename.get (); }
 
+  file_ptr read (bfd *abfd, void *buffer, file_ptr nbytes,
+		 file_ptr offset) override;
+
+  int stat (struct bfd *abfd, struct stat *sb) override;
+
 private:
   /* The base address of the in-memory BFD file.  */
   CORE_ADDR m_base;
@@ -256,47 +261,23 @@ struct target_buffer
   gdb::unique_xmalloc_ptr<char> m_filename;
 };
 
-/* For `gdb_bfd_open_from_target_memory`.  Opening the file is a no-op.  */
-
-static void *
-mem_bfd_iovec_open (struct bfd *abfd, void *open_closure)
-{
-  return open_closure;
-}
-
-/* For `gdb_bfd_open_from_target_memory`.  Closing the file is just freeing the
-   base/size pair on our side.  */
-
-static int
-mem_bfd_iovec_close (struct bfd *abfd, void *stream)
-{
-  struct target_buffer *buffer = (target_buffer *) stream;
-  delete buffer;
-
-  /* Zero means success.  */
-  return 0;
-}
-
 /* For `gdb_bfd_open_from_target_memory`.  For reading the file, we just need to
    pass through to target_read_memory and fix up the arguments and return
    values.  */
 
-static file_ptr
-mem_bfd_iovec_pread (struct bfd *abfd, void *stream, void *buf,
+file_ptr
+target_buffer::read (struct bfd *abfd, void *buf,
 		     file_ptr nbytes, file_ptr offset)
 {
-  struct target_buffer *buffer = (struct target_buffer *) stream;
-
   /* If this read will read all of the file, limit it to just the rest.  */
-  if (offset + nbytes > buffer->size ())
-    nbytes = buffer->size () - offset;
+  if (offset + nbytes > size ())
+    nbytes = size () - offset;
 
   /* If there are no more bytes left, we've reached EOF.  */
   if (nbytes == 0)
     return 0;
 
-  int err
-    = target_read_memory (buffer->base () + offset, (gdb_byte *) buf, nbytes);
+  int err = target_read_memory (base () + offset, (gdb_byte *) buf, nbytes);
   if (err)
     return -1;
 
@@ -306,13 +287,11 @@ mem_bfd_iovec_pread (struct bfd *abfd, void *stream, void *buf,
 /* For `gdb_bfd_open_from_target_memory`.  For statting the file, we only
    support the st_size attribute.  */
 
-static int
-mem_bfd_iovec_stat (struct bfd *abfd, void *stream, struct stat *sb)
+int
+target_buffer::stat (struct bfd *abfd, struct stat *sb)
 {
-  struct target_buffer *buffer = (struct target_buffer*) stream;
-
   memset (sb, 0, sizeof (struct stat));
-  sb->st_size = buffer->size ();
+  sb->st_size = size ();
   return 0;
 }
 
@@ -322,14 +301,14 @@ gdb_bfd_ref_ptr
 gdb_bfd_open_from_target_memory (CORE_ADDR addr, ULONGEST size,
 				 const char *target)
 {
-  struct target_buffer *buffer = new target_buffer (addr, size);
+  std::unique_ptr<target_buffer> buffer
+    = gdb::make_unique<target_buffer> (addr, size);
 
   return gdb_bfd_openr_iovec (buffer->filename (), target,
-			      mem_bfd_iovec_open,
-			      buffer,
-			      mem_bfd_iovec_pread,
-			      mem_bfd_iovec_close,
-			      mem_bfd_iovec_stat);
+			      [&] (bfd *nbfd)
+			      {
+				return buffer.release ();
+			      });
 }
 
 /* bfd_openr_iovec OPEN_CLOSURE data for gdb_bfd_open.  */

-- 
2.40.1

[PATCH 4/7] Convert target fileio to new type-safe gdb_bfd_openr_iovec

[Tom Tromey]

This converts the target fileio BFD iovec implementation to use the
new type-safe gdb_bfd_openr_iovec.
---
 gdb/gdb_bfd.c | 97 ++++++++++++++++++++++++++++++-----------------------------
 1 file changed, 50 insertions(+), 47 deletions(-)

diff --git a/gdb/gdb_bfd.c b/gdb/gdb_bfd.c
index 2f8bb33997b..c635ba007d0 100644
--- a/gdb/gdb_bfd.c
+++ b/gdb/gdb_bfd.c
@@ -311,30 +311,48 @@ gdb_bfd_open_from_target_memory (CORE_ADDR addr, ULONGEST size,
 			      });
 }
 
-/* bfd_openr_iovec OPEN_CLOSURE data for gdb_bfd_open.  */
-struct gdb_bfd_open_closure
+/* An object that manages the underlying stream for a BFD, using
+   target file I/O.  */
+
+struct target_fileio_stream : public gdb_bfd_iovec_base
 {
-  inferior *inf;
-  bool warn_if_slow;
+  target_fileio_stream (bfd *nbfd, int fd)
+    : m_bfd (nbfd),
+      m_fd (fd)
+  {
+  }
+
+  ~target_fileio_stream ();
+
+  file_ptr read (bfd *abfd, void *buffer, file_ptr nbytes,
+		 file_ptr offset) override;
+
+  int stat (struct bfd *abfd, struct stat *sb) override;
+
+private:
+
+  /* The BFD.  Saved for the destructor.  */
+  bfd *m_bfd;
+
+  /* The file descriptor.  */
+  int m_fd;
 };
 
-/* Wrapper for target_fileio_open suitable for passing as the
-   OPEN_FUNC argument to gdb_bfd_openr_iovec.  */
+/* Wrapper for target_fileio_open suitable for use as a helper
+   function for gdb_bfd_openr_iovec.  */
 
-static void *
-gdb_bfd_iovec_fileio_open (struct bfd *abfd, void *open_closure)
+static target_fileio_stream *
+gdb_bfd_iovec_fileio_open (struct bfd *abfd, inferior *inf, bool warn_if_slow)
 {
   const char *filename = bfd_get_filename (abfd);
   int fd;
   fileio_error target_errno;
-  int *stream;
-  gdb_bfd_open_closure *oclosure = (gdb_bfd_open_closure *) open_closure;
 
   gdb_assert (is_target_filename (filename));
 
-  fd = target_fileio_open (oclosure->inf,
+  fd = target_fileio_open (inf,
 			   filename + strlen (TARGET_SYSROOT_PREFIX),
-			   FILEIO_O_RDONLY, 0, oclosure->warn_if_slow,
+			   FILEIO_O_RDONLY, 0, warn_if_slow,
 			   &target_errno);
   if (fd == -1)
     {
@@ -343,19 +361,15 @@ gdb_bfd_iovec_fileio_open (struct bfd *abfd, void *open_closure)
       return NULL;
     }
 
-  stream = XCNEW (int);
-  *stream = fd;
-  return stream;
+  return new target_fileio_stream (abfd, fd);
 }
 
-/* Wrapper for target_fileio_pread suitable for passing as the
-   PREAD_FUNC argument to gdb_bfd_openr_iovec.  */
+/* Wrapper for target_fileio_pread.  */
 
-static file_ptr
-gdb_bfd_iovec_fileio_pread (struct bfd *abfd, void *stream, void *buf,
+file_ptr
+target_fileio_stream::read (struct bfd *abfd, void *buf,
 			    file_ptr nbytes, file_ptr offset)
 {
-  int fd = *(int *) stream;
   fileio_error target_errno;
   file_ptr pos, bytes;
 
@@ -364,7 +378,7 @@ gdb_bfd_iovec_fileio_pread (struct bfd *abfd, void *stream, void *buf,
     {
       QUIT;
 
-      bytes = target_fileio_pread (fd, (gdb_byte *) buf + pos,
+      bytes = target_fileio_pread (m_fd, (gdb_byte *) buf + pos,
 				   nbytes - pos, offset + pos,
 				   &target_errno);
       if (bytes == 0)
@@ -392,46 +406,35 @@ gdb_bfd_close_warning (const char *name, const char *reason)
   warning (_("cannot close \"%s\": %s"), name, reason);
 }
 
-/* Wrapper for target_fileio_close suitable for passing as the
-   CLOSE_FUNC argument to gdb_bfd_openr_iovec.  */
+/* Wrapper for target_fileio_close.  */
 
-static int
-gdb_bfd_iovec_fileio_close (struct bfd *abfd, void *stream)
+target_fileio_stream::~target_fileio_stream ()
 {
-  int fd = *(int *) stream;
   fileio_error target_errno;
 
-  xfree (stream);
-
   /* Ignore errors on close.  These may happen with remote
      targets if the connection has already been torn down.  */
   try
     {
-      target_fileio_close (fd, &target_errno);
+      target_fileio_close (m_fd, &target_errno);
     }
   catch (const gdb_exception &ex)
     {
       /* Also avoid crossing exceptions over bfd.  */
-      gdb_bfd_close_warning (bfd_get_filename (abfd),
+      gdb_bfd_close_warning (bfd_get_filename (m_bfd),
 			     ex.message->c_str ());
     }
-
-  /* Zero means success.  */
-  return 0;
 }
 
-/* Wrapper for target_fileio_fstat suitable for passing as the
-   STAT_FUNC argument to gdb_bfd_openr_iovec.  */
+/* Wrapper for target_fileio_fstat.  */
 
-static int
-gdb_bfd_iovec_fileio_fstat (struct bfd *abfd, void *stream,
-			    struct stat *sb)
+int
+target_fileio_stream::stat (struct bfd *abfd, struct stat *sb)
 {
-  int fd = *(int *) stream;
   fileio_error target_errno;
   int result;
 
-  result = target_fileio_fstat (fd, sb, &target_errno);
+  result = target_fileio_fstat (m_fd, sb, &target_errno);
   if (result == -1)
     {
       errno = fileio_error_to_host (target_errno);
@@ -482,13 +485,13 @@ gdb_bfd_open (const char *name, const char *target, int fd,
 	{
 	  gdb_assert (fd == -1);
 
-	  gdb_bfd_open_closure open_closure { current_inferior (), warn_if_slow };
-	  return gdb_bfd_openr_iovec (name, target,
-				      gdb_bfd_iovec_fileio_open,
-				      &open_closure,
-				      gdb_bfd_iovec_fileio_pread,
-				      gdb_bfd_iovec_fileio_close,
-				      gdb_bfd_iovec_fileio_fstat);
+	  auto open = [&] (bfd *nbfd) -> gdb_bfd_iovec_base *
+	  {
+	    return gdb_bfd_iovec_fileio_open (nbfd, current_inferior (),
+					      warn_if_slow);
+	  };
+
+	  return gdb_bfd_openr_iovec (name, target, open);
 	}
 
       name += strlen (TARGET_SYSROOT_PREFIX);

-- 
2.40.1

[PATCH 5/7] Convert minidebug to new type-safe gdb_bfd_openr_iovec

[Tom Tromey]

This converts the minidebug BFD iovec implementation to the new
type-safe gdb_bfd_openr_iovec.
---
 gdb/minidebug.c | 98 +++++++++++++++++++++++----------------------------------
 1 file changed, 40 insertions(+), 58 deletions(-)

diff --git a/gdb/minidebug.c b/gdb/minidebug.c
index 979e569e639..e2b60fb959c 100644
--- a/gdb/minidebug.c
+++ b/gdb/minidebug.c
@@ -57,7 +57,7 @@ static lzma_allocator gdb_lzma_allocator = { alloc_lzma, free_lzma, NULL };
    a section.  This keeps only the last decompressed block in memory
    to allow larger data without using to much memory.  */
 
-struct gdb_lzma_stream
+struct gdb_lzma_stream : public gdb_bfd_iovec_base
 {
   /* Section of input BFD from which we are decoding data.  */
   asection *section = nullptr;
@@ -69,19 +69,25 @@ struct gdb_lzma_stream
   bfd_size_type data_start = 0;
   bfd_size_type data_end = 0;
   gdb::byte_vector data;
-};
 
-/* bfd_openr_iovec OPEN_P implementation for
-   find_separate_debug_file_in_section.  OPEN_CLOSURE is 'asection *'
-   of the section to decompress.
 
-   Return 'struct gdb_lzma_stream *' must be freed by caller by delete,
-   together with its INDEX lzma data.  */
+  ~gdb_lzma_stream ()
+  {
+    lzma_index_end (index, &gdb_lzma_allocator);
+  }
 
-static void *
-lzma_open (struct bfd *nbfd, void *open_closure)
+  file_ptr read (bfd *abfd, void *buffer, file_ptr nbytes,
+		 file_ptr offset) override;
+
+  int stat (struct bfd *abfd, struct stat *sb) override;
+};
+
+/* bfd_openr_iovec implementation helper for
+   find_separate_debug_file_in_section.  */
+
+static gdb_lzma_stream *
+lzma_open (struct bfd *nbfd, asection *section)
 {
-  asection *section = (asection *) open_closure;
   bfd_size_type size, offset;
   lzma_stream_flags options;
   gdb_byte footer[LZMA_STREAM_HEADER_SIZE];
@@ -127,15 +133,13 @@ lzma_open (struct bfd *nbfd, void *open_closure)
   return lstream;
 }
 
-/* bfd_openr_iovec PREAD_P implementation for
-   find_separate_debug_file_in_section.  Passed STREAM
-   is 'struct gdb_lzma_stream *'.  */
+/* bfd_openr_iovec read implementation for
+   find_separate_debug_file_in_section.  */
 
-static file_ptr
-lzma_pread (struct bfd *nbfd, void *stream, void *buf, file_ptr nbytes,
-	    file_ptr offset)
+file_ptr
+gdb_lzma_stream::read (struct bfd *nbfd, void *buf, file_ptr nbytes,
+		       file_ptr offset)
 {
-  struct gdb_lzma_stream *lstream = (struct gdb_lzma_stream *) stream;
   bfd_size_type chunk_size;
   lzma_index_iter iter;
   file_ptr block_offset;
@@ -147,12 +151,9 @@ lzma_pread (struct bfd *nbfd, void *stream, void *buf, file_ptr nbytes,
   res = 0;
   while (nbytes > 0)
     {
-      if (lstream->data.empty ()
-	  || lstream->data_start > offset || offset >= lstream->data_end)
+      if (data.empty () || data_start > offset || offset >= data_end)
 	{
-	  asection *section = lstream->section;
-
-	  lzma_index_iter_init (&iter, lstream->index);
+	  lzma_index_iter_init (&iter, index);
 	  if (lzma_index_iter_locate (&iter, offset))
 	    break;
 
@@ -184,15 +185,14 @@ lzma_pread (struct bfd *nbfd, void *stream, void *buf, file_ptr nbytes,
 	      != LZMA_OK)
 	    break;
 
-	  lstream->data = std::move (uncompressed);
-	  lstream->data_start = iter.block.uncompressed_file_offset;
-	  lstream->data_end = (iter.block.uncompressed_file_offset
-			       + iter.block.uncompressed_size);
+	  data = std::move (uncompressed);
+	  data_start = iter.block.uncompressed_file_offset;
+	  data_end = (iter.block.uncompressed_file_offset
+		      + iter.block.uncompressed_size);
 	}
 
-      chunk_size = std::min (nbytes, (file_ptr) lstream->data_end - offset);
-      memcpy (buf, lstream->data.data () + offset - lstream->data_start,
-	      chunk_size);
+      chunk_size = std::min (nbytes, (file_ptr) data_end - offset);
+      memcpy (buf, data.data () + offset - data_start, chunk_size);
       buf = (gdb_byte *) buf + chunk_size;
       offset += chunk_size;
       nbytes -= chunk_size;
@@ -202,36 +202,14 @@ lzma_pread (struct bfd *nbfd, void *stream, void *buf, file_ptr nbytes,
   return res;
 }
 
-/* bfd_openr_iovec CLOSE_P implementation for
-   find_separate_debug_file_in_section.  Passed STREAM
-   is 'struct gdb_lzma_stream *'.  */
-
-static int
-lzma_close (struct bfd *nbfd,
-	    void *stream)
-{
-  struct gdb_lzma_stream *lstream = (struct gdb_lzma_stream *) stream;
-
-  lzma_index_end (lstream->index, &gdb_lzma_allocator);
-  delete lstream;
-
-  /* Zero means success.  */
-  return 0;
-}
-
-/* bfd_openr_iovec STAT_P implementation for
-   find_separate_debug_file_in_section.  Passed STREAM
-   is 'struct gdb_lzma_stream *'.  */
+/* bfd_openr_iovec stat implementation for
+   find_separate_debug_file_in_section.  */
 
-static int
-lzma_stat (struct bfd *abfd,
-	   void *stream,
-	   struct stat *sb)
+int
+gdb_lzma_stream::stat (struct bfd *abfd, struct stat *sb)
 {
-  struct gdb_lzma_stream *lstream = (struct gdb_lzma_stream *) stream;
-
   memset (sb, 0, sizeof (struct stat));
-  sb->st_size = lzma_index_uncompressed_size (lstream->index);
+  sb->st_size = lzma_index_uncompressed_size (index);
   return 0;
 }
 
@@ -265,8 +243,12 @@ find_separate_debug_file_in_section (struct objfile *objfile)
   std::string filename = string_printf (_(".gnu_debugdata for %s"),
 					objfile_name (objfile));
 
-  abfd = gdb_bfd_openr_iovec (filename.c_str (), gnutarget, lzma_open,
-			      section, lzma_pread, lzma_close, lzma_stat);
+  auto open = [&] (bfd *nbfd) -> gdb_lzma_stream *
+  {
+    return lzma_open (nbfd, section);
+  };
+
+  abfd = gdb_bfd_openr_iovec (filename.c_str (), gnutarget, open);
   if (abfd == NULL)
     return NULL;
 

-- 
2.40.1

[PATCH 6/7] Convert solib-rocm to new type-safe gdb_bfd_openr_iovec

[Tom Tromey]

This converts the solib-rocm BFD iovec implementations to the new
type-safe gdb_bfd_openr_iovec.  They were already essentially using
this approach, just without the type-safe wrapper.

I can't build or test this patch.
---
 gdb/solib-rocm.c | 39 +++++++++------------------------------
 1 file changed, 9 insertions(+), 30 deletions(-)

diff --git a/gdb/solib-rocm.c b/gdb/solib-rocm.c
index 882920a3711..145624b888a 100644
--- a/gdb/solib-rocm.c
+++ b/gdb/solib-rocm.c
@@ -268,7 +268,7 @@ namespace {
 
 /* Interface to interact with a ROCm code object stream.  */
 
-struct rocm_code_object_stream
+struct rocm_code_object_stream : public gdb_bfd_iovec_base
 {
   DISABLE_COPY_AND_ASSIGN (rocm_code_object_stream);
 
@@ -457,8 +457,8 @@ rocm_code_object_stream_memory::read (void *buf, file_ptr size,
 
 } /* anonymous namespace */
 
-static void *
-rocm_bfd_iovec_open (bfd *abfd, void *inferior_void)
+static gdb_bfd_iovec_base *
+rocm_bfd_iovec_open (bfd *abfd, inferior *inferior)
 {
   gdb::string_view uri (bfd_get_filename (abfd));
   gdb::string_view protocol_delim = "://";
@@ -522,7 +522,6 @@ rocm_bfd_iovec_open (bfd *abfd, void *inferior_void)
     {
       ULONGEST offset = 0;
       ULONGEST size = 0;
-      inferior *inferior = static_cast<struct inferior *> (inferior_void);
 
       auto try_strtoulst = [] (gdb::string_view v)
 	{
@@ -607,28 +606,6 @@ rocm_bfd_iovec_open (bfd *abfd, void *inferior_void)
     }
 }
 
-static int
-rocm_bfd_iovec_close (bfd *nbfd, void *data)
-{
-  delete static_cast<rocm_code_object_stream *> (data);
-
-  return 0;
-}
-
-static file_ptr
-rocm_bfd_iovec_pread (bfd *abfd, void *data, void *buf, file_ptr size,
-		      file_ptr offset)
-{
-  return static_cast<rocm_code_object_stream *> (data)->read (buf, size,
-							      offset);
-}
-
-static int
-rocm_bfd_iovec_stat (bfd *abfd, void *data, struct stat *sb)
-{
-  return static_cast<rocm_code_object_stream *> (data)->stat (sb);
-}
-
 static gdb_bfd_ref_ptr
 rocm_solib_bfd_open (const char *pathname)
 {
@@ -636,10 +613,12 @@ rocm_solib_bfd_open (const char *pathname)
   if (strstr (pathname, "://") == nullptr)
     return svr4_so_ops.bfd_open (pathname);
 
-  gdb_bfd_ref_ptr abfd
-    = gdb_bfd_openr_iovec (pathname, "elf64-amdgcn", rocm_bfd_iovec_open,
-			   current_inferior (), rocm_bfd_iovec_pread,
-			   rocm_bfd_iovec_close, rocm_bfd_iovec_stat);
+  auto open = [&] (bfd *nbfd) -> gdb_bfd_iovec_base *
+  {
+    return rocm_bfd_iovec_open (nbfd, current_inferior ());
+  };
+
+  gdb_bfd_ref_ptr abfd = gdb_bfd_openr_iovec (pathname, "elf64-amdgcn", open);
 
   if (abfd == nullptr)
     error (_("Could not open `%s' as an executable file: %s"), pathname,

-- 
2.40.1

Re: [PATCH 6/7] Convert solib-rocm to new type-safe gdb_bfd_openr_iovec

[Lancelot SIX]

Hi Tom,

I have tested this patch.  As it is, it fails to compile.

Here is a diff fixing the build.  Changes are minor:
- Change rocm_code_object_*::read to accept the " bfd *" first param
  to properly override gdb_bfd_iovec_base::read.
- The "open" lambda passed to gdb_bfd_openr_iovec does not need to
  capture anything.

Best,
Lancelot.

---
 gdb/solib-rocm.c | 29 ++++++++++-------------------
 1 file changed, 10 insertions(+), 19 deletions(-)

diff --git a/gdb/solib-rocm.c b/gdb/solib-rocm.c
index 009a5c1b774..6b84f09e88b 100644
--- a/gdb/solib-rocm.c
+++ b/gdb/solib-rocm.c
@@ -272,20 +272,9 @@ struct rocm_code_object_stream : public gdb_bfd_iovec_base
 {
   DISABLE_COPY_AND_ASSIGN (rocm_code_object_stream);
 
-  /* Copy SIZE bytes from the underlying objfile storage starting at OFFSET
-     into the user provided buffer BUF.
+  int stat (bfd *abfd, struct stat *sb) final override;
 
-     Return the number of bytes actually copied (might be inferior to SIZE if
-     the end of the stream is reached).  */
-  virtual file_ptr read (void *buf, file_ptr size, file_ptr offset) = 0;
-
-  /* Retrieve file information in SB.
-
-     Return 0 on success.  On failure, set the appropriate bfd error number
-     (using bfd_set_error) and return -1.  */
-  int stat (struct stat *sb);
-
-  virtual ~rocm_code_object_stream () = default;
+  ~rocm_code_object_stream () override = default;
 
 protected:
   rocm_code_object_stream () = default;
@@ -298,7 +287,7 @@ struct rocm_code_object_stream : public gdb_bfd_iovec_base
 };
 
 int
-rocm_code_object_stream::stat (struct stat *sb)
+rocm_code_object_stream::stat (bfd *, struct stat *sb)
 {
   const LONGEST size = this->size ();
   if (size == -1)
@@ -319,7 +308,8 @@ struct rocm_code_object_stream_file final : rocm_code_object_stream
   rocm_code_object_stream_file (inferior *inf, int fd, ULONGEST offset,
 				ULONGEST size);
 
-  file_ptr read (void *buf, file_ptr size, file_ptr offset) override;
+  file_ptr read (bfd *abfd, void *buf, file_ptr size,
+		 file_ptr offset) override;
 
   LONGEST size () override;
 
@@ -348,7 +338,7 @@ rocm_code_object_stream_file::rocm_code_object_stream_file
 }
 
 file_ptr
-rocm_code_object_stream_file::read (void *buf, file_ptr size,
+rocm_code_object_stream_file::read (bfd *, void *buf, file_ptr size,
 				    file_ptr offset)
 {
   fileio_error target_errno;
@@ -423,7 +413,8 @@ struct rocm_code_object_stream_memory final : public rocm_code_object_stream
 
   rocm_code_object_stream_memory (gdb::byte_vector buffer);
 
-  file_ptr read (void *buf, file_ptr size, file_ptr offset) override;
+  file_ptr read (bfd *abfd, void *buf, file_ptr size,
+		 file_ptr offset) override;
 
 protected:
 
@@ -445,7 +436,7 @@ rocm_code_object_stream_memory::rocm_code_object_stream_memory
 }
 
 file_ptr
-rocm_code_object_stream_memory::read (void *buf, file_ptr size,
+rocm_code_object_stream_memory::read (bfd *, void *buf, file_ptr size,
 				      file_ptr offset)
 {
   if (size > m_objfile_image.size () - offset)
@@ -613,7 +604,7 @@ rocm_solib_bfd_open (const char *pathname)
   if (strstr (pathname, "://") == nullptr)
     return svr4_so_ops.bfd_open (pathname);
 
-  auto open = [&] (bfd *nbfd) -> gdb_bfd_iovec_base *
+  auto open = [] (bfd *nbfd) -> gdb_bfd_iovec_base *
   {
     return rocm_bfd_iovec_open (nbfd, current_inferior ());
   };

Re: [PATCH 6/7] Convert solib-rocm to new type-safe gdb_bfd_openr_iovec

[Tom Tromey]

>>>>> "Lancelot" == Lancelot SIX <lsix@lancelotsix.com> writes:

Lancelot> Hi Tom,
Lancelot> I have tested this patch.  As it is, it fails to compile.

Thank you.

Lancelot> Here is a diff fixing the build.  Changes are minor:
Lancelot> - Change rocm_code_object_*::read to accept the " bfd *" first param
Lancelot>   to properly override gdb_bfd_iovec_base::read.
Lancelot> - The "open" lambda passed to gdb_bfd_openr_iovec does not need to
Lancelot>   capture anything.

I've applied it for v2.

Tom

[PATCH 7/7] Remove old gdb_bfd_openr_iovec

[Tom Tromey]

This removes the old gdb_bfd_openr_iovec entirely.  I think any new
code should use the type-safe approach.
---
 gdb/gdb_bfd.c | 25 -------------------------
 gdb/gdb_bfd.h | 18 ------------------
 2 files changed, 43 deletions(-)

diff --git a/gdb/gdb_bfd.c b/gdb/gdb_bfd.c
index c635ba007d0..7741fc98e97 100644
--- a/gdb/gdb_bfd.c
+++ b/gdb/gdb_bfd.c
@@ -931,31 +931,6 @@ gdb_bfd_openr_iovec (const char *filename, const char *target,
 
 /* See gdb_bfd.h.  */
 
-gdb_bfd_ref_ptr
-gdb_bfd_openr_iovec (const char *filename, const char *target,
-		     void *(*open_func) (struct bfd *nbfd,
-					 void *open_closure),
-		     void *open_closure,
-		     file_ptr (*pread_func) (struct bfd *nbfd,
-					     void *stream,
-					     void *buf,
-					     file_ptr nbytes,
-					     file_ptr offset),
-		     int (*close_func) (struct bfd *nbfd,
-					void *stream),
-		     int (*stat_func) (struct bfd *abfd,
-				       void *stream,
-				       struct stat *sb))
-{
-  bfd *result = bfd_openr_iovec (filename, target,
-				 open_func, open_closure,
-				 pread_func, close_func, stat_func);
-
-  return gdb_bfd_ref_ptr::new_reference (result);
-}
-
-/* See gdb_bfd.h.  */
-
 void
 gdb_bfd_mark_parent (bfd *child, bfd *parent)
 {
diff --git a/gdb/gdb_bfd.h b/gdb/gdb_bfd.h
index ae374f5d7ae..604365b61b1 100644
--- a/gdb/gdb_bfd.h
+++ b/gdb/gdb_bfd.h
@@ -181,24 +181,6 @@ using gdb_iovec_opener_ftype
 gdb_bfd_ref_ptr gdb_bfd_openr_iovec (const char *filename, const char *target,
 				     gdb_iovec_opener_ftype open_fn);
 
-/* A wrapper for bfd_openr_iovec that initializes the gdb-specific
-   reference count.  */
-
-gdb_bfd_ref_ptr gdb_bfd_openr_iovec (const char *filename, const char *target,
-				     void *(*open_func) (struct bfd *nbfd,
-							 void *open_closure),
-				     void *open_closure,
-				     file_ptr (*pread_func) (struct bfd *nbfd,
-							     void *stream,
-							     void *buf,
-							     file_ptr nbytes,
-							     file_ptr offset),
-				     int (*close_func) (struct bfd *nbfd,
-							void *stream),
-				     int (*stat_func) (struct bfd *abfd,
-						       void *stream,
-						       struct stat *sb));
-
 /* A wrapper for bfd_openr_next_archived_file that initializes the
    gdb-specific reference count.  */
 

-- 
2.40.1

Re: [PATCH 0/7] Rewrite gdb_bfd_openr_iovec to be type-safe

[Lancelot SIX]

Hi Tom,

I have sent a couple of remarks on patch 1 & patch 6.  I quickly went
through the other patches of the series and did not notice any issue.

Best,
Lancelot.

On Thu, Aug 24, 2023 at 11:12:17AM -0600, Tom Tromey via Gdb-patches wrote:
> This series rewrites gdb_bfd_openr_iovec to be type-safe.
> 
> Note that I can't really test the solib-rocm.c changes, this was done
> as a best effort.
> 
> Regression tested on x86-64 Fedora 36.
> 
> ---
> Tom Tromey (7):
>       Introduce type-safe variant of gdb_bfd_openr_iovec
>       Small constructor change to target_buffer
>       Convert mem_bfd_iovec to new type-safe gdb_bfd_openr_iovec
>       Convert target fileio to new type-safe gdb_bfd_openr_iovec
>       Convert minidebug to new type-safe gdb_bfd_openr_iovec
>       Convert solib-rocm to new type-safe gdb_bfd_openr_iovec
>       Remove old gdb_bfd_openr_iovec
> 
>  gdb/gdb_bfd.c    | 211 +++++++++++++++++++++++++++----------------------------
>  gdb/gdb_bfd.h    |  43 ++++++++----
>  gdb/minidebug.c  |  98 +++++++++++---------------
>  gdb/solib-rocm.c |  39 +++-------
>  4 files changed, 182 insertions(+), 209 deletions(-)
> ---
> base-commit: 1f08d324601ef7fd4cce3cff8f8db4a774791828
> change-id: 20230824-gdb-bfd-vec-ba7f6c1139bb
> 
> Best regards,
> -- 
> Tom Tromey <tromey@adacore.com>
>