How to get file descriptor from abfd?

How to get file descriptor from abfd?

[Paul Pluzhnikov]

Greetings,

Currently GDB allocates memory and does a bfd_read() on dwarf debug sections.

This causes GDB to be significantly slower on low-memory machines than
it has to be, and I have a patch to fix this by directly mmap()ing the
debug sections instead:
http://sourceware.org/ml/gdb-patches/2009-05/txt00023.txt

In that patch, I do:

+      caddr_t retbuf = mmap (0, map_length, PROT_READ, MAP_PRIVATE,
+			     fileno ((FILE *)abfd->iostream),
+			     pg_offset);

and this cast of abfd->iostream has been flagged as possibly un-kosher.

What is the proper way to get access to the underlying file descriptor
for a given abfd (if there is one)?
If there isn't a way, should there be?

I would have used bfd_get_file_window(), but it's only compiled in
when --use-mmap was given to bfd/configure.

Is there a way to turn that on for GDB?

The  bfd_get_file_window() performs above cast, after checking for:

      (abfd->flags & BFD_IN_MEMORY) == 0)

and later

      if (abfd->iostream == NULL
	  && (abfd->iovec == NULL
	      || abfd->iovec->bseek (abfd, offset, SEEK_SET) != 0))

The last 3 conditions can not be true in gdb/dwarf2read.c because we just
performed bfd_seek and bfd_bread on abfd.

Looks like BFD_IN_MEMORY could be set though.

Is it ok to do this:

  if ((abfd->flags & BFD_IN_MEMORY) == 0) ...

in gdb/dwarf2read.c?

Thanks,
-- 
Paul Pluzhnikov

Re: How to get file descriptor from abfd?

[Dave Korn]

Paul Pluzhnikov wrote:

> Currently GDB allocates memory and does a bfd_read() on dwarf debug sections.
> 
> This causes GDB to be significantly slower on low-memory machines than
> it has to be, and I have a patch to fix this by directly mmap()ing the
> debug sections instead:
> http://sourceware.org/ml/gdb-patches/2009-05/txt00023.txt
> 
> In that patch, I do:
> 
> +      caddr_t retbuf = mmap (0, map_length, PROT_READ, MAP_PRIVATE,
> +			     fileno ((FILE *)abfd->iostream),
> +			     pg_offset);
> 
> and this cast of abfd->iostream has been flagged as possibly un-kosher.
> 
> What is the proper way to get access to the underlying file descriptor
> for a given abfd (if there is one)?
> If there isn't a way, should there be?

  It's a bad idea, because anything you do to that FILE* might confuse the
underlying bfd_iovec stream.  The cleanest design would be to add an mmap
member function to bfd_iovec.

> I would have used bfd_get_file_window(), but it's only compiled in
> when --use-mmap was given to bfd/configure.
> 
> Is there a way to turn that on for GDB?

  Just configure with it.  It will be passed to the included bfd/ configure.

> Is it ok to do this:
> 
>   if ((abfd->flags & BFD_IN_MEMORY) == 0) ...
> 
> in gdb/dwarf2read.c?

  Won't be necessary if you do it the iovec way, there will just be a stub
implementation of 'bmmap()' for in-memory iovecs and a real one (conditioned
on HAVE_MMAP) for file-based iovecs.  Then gdb can call the bfd_mmap wrapper
function speculatively and just fall back to the old way of doing it if that
returns an error code.

    cheers,
      DaveK

Re: How to get file descriptor from abfd?

[Paul Pluzhnikov]

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

On Sun, May 31, 2009 at 11:24 AM, Dave Korn
<dave.korn.cygwin@googlemail.com> wrote:

> The cleanest design would be to add an mmap member function to bfd_iovec.

Thanks.

Would attached patch be acceptable?

Tested by building with and without HAVE_MMAP on Linux/x86_64, and by
building GDB against this and running its tests.


-- 
Paul Pluzhnikov

2009-06-01  Paul Pluzhnikov  <ppluzhnikov@google.com>

        * bfd-in2.h: bfd_mmap prototype
        * bfdio.c (bfd_mmap): New function.
        * libbfd.h (bfd_iovec): Add bmmap.
        * cache.c (cache_bmap): New function.
        (cache_iovec): Initialize bmmap member.
        * opencls.c (opncls_bmmap): New function.
        (opncls_iovec): Initialize bmmap member.

[-- Attachment #2: bfd-mmap-patch-20090601.txt --]
[-- Type: text/plain, Size: 4912 bytes --]

Index: bfd-in2.h
===================================================================
RCS file: /cvs/src/src/bfd/bfd-in2.h,v
retrieving revision 1.482
diff -u -p -u -r1.482 bfd-in2.h
--- bfd-in2.h	1 Jun 2009 13:11:51 -0000	1.482
+++ bfd-in2.h	2 Jun 2009 05:30:15 -0000
@@ -463,6 +463,7 @@ extern int bfd_seek (bfd *, file_ptr, in
 extern file_ptr bfd_tell (bfd *);
 extern int bfd_flush (bfd *);
 extern int bfd_stat (bfd *, struct stat *);
+extern void *bfd_mmap (bfd *, void *, bfd_size_type, int, int, file_ptr);
 
 /* Deprecated old routines.  */
 #if __GNUC__
Index: bfdio.c
===================================================================
RCS file: /cvs/src/src/bfd/bfdio.c,v
retrieving revision 1.21
diff -u -p -u -r1.21 bfdio.c
--- bfdio.c	24 May 2009 11:47:27 -0000	1.21
+++ bfdio.c	2 Jun 2009 05:30:15 -0000
@@ -158,6 +158,8 @@ DESCRIPTION
 .  int (*bclose) (struct bfd *abfd);
 .  int (*bflush) (struct bfd *abfd);
 .  int (*bstat) (struct bfd *abfd, struct stat *sb);
+.  void (*bmmap) (struct bfd *abfd, void *addr, bfd_size_type len,
+.                 int prot, int flags, file_ptr offset);
 .};
 
 */
@@ -511,3 +513,31 @@ bfd_get_size (bfd *abfd)
 
   return buf.st_size;
 }
+
+
+/*
+FUNCTION
+	bfd_mmap
+
+SYNOPSIS
+	void *bfd_mmap (bfd *abfd, void *addr, bfd_size_type len,
+	                int prot, int flags, file_ptr offset);
+
+DESCRIPTION
+	Return mmap()ed region of the file, if possible and implemented.
+
+*/
+
+void *
+bfd_mmap (bfd *abfd, void *addr, bfd_size_type len,
+	  int prot, int flags, file_ptr offset)
+{
+  void *ret = (void *)-1;
+  if ((abfd->flags & BFD_IN_MEMORY) != 0)
+    return ret;
+
+  if (abfd->iovec == NULL)
+    return ret;
+
+  return abfd->iovec->bmmap (abfd, addr, len, prot, flags, offset);
+}
Index: cache.c
===================================================================
RCS file: /cvs/src/src/bfd/cache.c,v
retrieving revision 1.34
diff -u -p -u -r1.34 cache.c
--- cache.c	30 Oct 2008 09:05:32 -0000	1.34
+++ cache.c	2 Jun 2009 05:30:15 -0000
@@ -46,6 +46,10 @@ SUBSECTION
 #include "libbfd.h"
 #include "libiberty.h"
 
+#ifdef HAVE_MMAP
+#include <sys/mman.h>
+#endif
+
 /* In some cases we can optimize cache operation when reopening files.
    For instance, a flush is entirely unnecessary if the file is already
    closed, so a flush would use CACHE_NO_OPEN.  Similarly, a seek using
@@ -388,10 +392,38 @@ cache_bstat (struct bfd *abfd, struct st
   return sts;
 }
 
+static void *
+cache_bmmap (struct bfd *abfd ATTRIBUTE_UNUSED,
+	     void *addr ATTRIBUTE_UNUSED,
+	     bfd_size_type len ATTRIBUTE_UNUSED,
+	     int prot ATTRIBUTE_UNUSED,
+	     int flags ATTRIBUTE_UNUSED,
+	     file_ptr offset ATTRIBUTE_UNUSED)
+{
+  void *ret = (void *)-1;
+
+  if ((abfd->flags & BFD_IN_MEMORY) != 0)
+    abort ();
+#ifdef HAVE_MMAP
+  else
+    {
+      FILE *f = bfd_cache_lookup (abfd, CACHE_NO_SEEK_ERROR);
+      if (f == NULL)
+	return ret;
+
+      ret = mmap (addr, len, prot, flags, fileno (f), offset);
+      if (ret == (void *)-1)
+	bfd_set_error (bfd_error_system_call);
+    }
+#endif
+
+  return ret;
+}
+
 static const struct bfd_iovec cache_iovec =
 {
   &cache_bread, &cache_bwrite, &cache_btell, &cache_bseek,
-  &cache_bclose, &cache_bflush, &cache_bstat
+  &cache_bclose, &cache_bflush, &cache_bstat, &cache_bmmap
 };
 
 /*
Index: libbfd.h
===================================================================
RCS file: /cvs/src/src/bfd/libbfd.h,v
retrieving revision 1.216
diff -u -p -u -r1.216 libbfd.h
--- libbfd.h	1 Jun 2009 13:11:52 -0000	1.216
+++ libbfd.h	2 Jun 2009 05:30:15 -0000
@@ -773,6 +773,9 @@ struct bfd_iovec
   int (*bclose) (struct bfd *abfd);
   int (*bflush) (struct bfd *abfd);
   int (*bstat) (struct bfd *abfd, struct stat *sb);
+  /* Just like mmap: (void*)-1 on failure, mmapped address on success.  */
+  void *(*bmmap) (struct bfd *abfd, void *addr, bfd_size_type len,
+                  int prot, int flags, file_ptr offset);
 };
 /* Extracted from bfdwin.c.  */
 struct _bfd_window_internal {
Index: opncls.c
===================================================================
RCS file: /cvs/src/src/bfd/opncls.c,v
retrieving revision 1.53
diff -u -p -u -r1.53 opncls.c
--- opncls.c	23 Aug 2008 08:08:58 -0000	1.53
+++ opncls.c	2 Jun 2009 05:30:15 -0000
@@ -505,9 +505,20 @@ opncls_bstat (struct bfd *abfd, struct s
   return (vec->stat) (abfd, vec->stream, sb);
 }
 
+static void *
+opncls_bmmap (struct bfd *abfd ATTRIBUTE_UNUSED,
+	      void *addr ATTRIBUTE_UNUSED,
+	      bfd_size_type len ATTRIBUTE_UNUSED,
+	      int prot ATTRIBUTE_UNUSED,
+	      int flags ATTRIBUTE_UNUSED,
+	      file_ptr offset ATTRIBUTE_UNUSED)
+{
+  return (void *)-1;
+}
+
 static const struct bfd_iovec opncls_iovec = {
   &opncls_bread, &opncls_bwrite, &opncls_btell, &opncls_bseek,
-  &opncls_bclose, &opncls_bflush, &opncls_bstat
+  &opncls_bclose, &opncls_bflush, &opncls_bstat, &opncls_bmmap
 };
 
 bfd *

Re: How to get file descriptor from abfd?

[Paul Pluzhnikov]

On Mon, Jun 1, 2009 at 10:54 PM, Paul Pluzhnikov<ppluzhnikov@google.com> wrote:

> Would attached patch be acceptable?

Ping?

-- 
Paul Pluzhnikov

Re: How to get file descriptor from abfd?

[Dave Korn]

Paul Pluzhnikov wrote:
> On Mon, Jun 1, 2009 at 10:54 PM, Paul Pluzhnikov<ppluzhnikov@google.com> wrote:
> 
>> Would attached patch be acceptable?
> 
> Ping?

  I can't approve this as it's outside my area.  I have verified that it
applies cleanly and builds without failures on i686-pc-cygwin (although that
doesn't support --enable-mmap currently for reasons I have yet to discover)
and have just a couple of minor formatting comments on the patch:

>  Index: bfdio.c
> ===================================================================
> RCS file: /cvs/src/src/bfd/bfdio.c,v
> retrieving revision 1.21
> diff -u -p -u -r1.21 bfdio.c
> --- bfdio.c	24 May 2009 11:47:27 -0000	1.21
> +++ bfdio.c	2 Jun 2009 05:30:15 -0000
> @@ -158,6 +158,8 @@ DESCRIPTION
>  .  int (*bclose) (struct bfd *abfd);
>  .  int (*bflush) (struct bfd *abfd);
>  .  int (*bstat) (struct bfd *abfd, struct stat *sb);
> +.  void (*bmmap) (struct bfd *abfd, void *addr, bfd_size_type len,
> +.                 int prot, int flags, file_ptr offset);

  The return type here should be void*, not plain void.

> +static void *
> +cache_bmmap (struct bfd *abfd ATTRIBUTE_UNUSED,
> +	     void *addr ATTRIBUTE_UNUSED,
> +	     bfd_size_type len ATTRIBUTE_UNUSED,
> +	     int prot ATTRIBUTE_UNUSED,
> +	     int flags ATTRIBUTE_UNUSED,
> +	     file_ptr offset ATTRIBUTE_UNUSED)
> +{
> +  void *ret = (void *)-1;

  Gnu coding standards request a space between the cast and the -1.  This also
applies to several other casts throughout the patch.

> +
> +  if ((abfd->flags & BFD_IN_MEMORY) != 0)
> +    abort ();
> +#ifdef HAVE_MMAP
> +  else
> +    {
> +      FILE *f = bfd_cache_lookup (abfd, CACHE_NO_SEEK_ERROR);
> +      if (f == NULL)
> +	return ret;
> +
> +      ret = mmap (addr, len, prot, flags, fileno (f), offset);
> +      if (ret == (void *)-1)
> +	bfd_set_error (bfd_error_system_call);
> +    }
> +#endif
> +
> +  return ret;
> +

  Since abort() doesn't return, the else clause and extra level of nested
braces is superfluous here.  Straight-line code would be cleaner IMO.

  Anyway, that's minor stuff, as I said; the patch looks fundamentally sound
to me, but I can't OK it for you.

    cheers,
      DaveK

Re: How to get file descriptor from abfd?

[Paul Pluzhnikov]

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

On Wed, Jun 10, 2009 at 8:26 AM, Dave
Korn<dave.korn.cygwin@googlemail.com> wrote:

>> +. void (*bmmap) (struct bfd *abfd, void *addr, bfd_size_type len,
>> +.                int prot, int flags, file_ptr offset);
>
> The return type here should be void*, not plain void.

Thanks, fixed.

>> + void *ret = (void *)-1;
>
> Gnu coding standards request a space between the cast and the -1.

Fixed in all the places I could find.

>> + if ((abfd->flags & BFD_IN_MEMORY) != 0)
>> +   abort ();
>> +#ifdef HAVE_MMAP
>> +  else
>> +    {
>> +      FILE *f = bfd_cache_lookup (abfd, CACHE_NO_SEEK_ERROR);
>
> Since abort() doesn't return, the else clause and extra level of nested
> braces is superfluous here.

The braces introduce a new scope for 'FILE *f'.
If I move the declaration to the top of function, then it fails to build
when !HAVE_MMAP:

  cc1: warnings being treated as errors
  ../../src/bfd/cache.c: In function 'cache_bmmap':
  ../../src/bfd/cache.c:404: warning: unused variable 'f'

I left this "as is".

Corrected patch attached.

Thanks,
-- 
Paul Pluzhnikov

[-- Attachment #2: bfd-mmap-patch-20090610.txt --]
[-- Type: text/plain, Size: 4922 bytes --]

Index: bfd-in2.h
===================================================================
RCS file: /cvs/src/src/bfd/bfd-in2.h,v
retrieving revision 1.482
diff -u -p -u -r1.482 bfd-in2.h
--- bfd-in2.h	1 Jun 2009 13:11:51 -0000	1.482
+++ bfd-in2.h	10 Jun 2009 17:01:28 -0000
@@ -463,6 +463,7 @@ extern int bfd_seek (bfd *, file_ptr, in
 extern file_ptr bfd_tell (bfd *);
 extern int bfd_flush (bfd *);
 extern int bfd_stat (bfd *, struct stat *);
+extern void *bfd_mmap (bfd *, void *, bfd_size_type, int, int, file_ptr);
 
 /* Deprecated old routines.  */
 #if __GNUC__
Index: bfdio.c
===================================================================
RCS file: /cvs/src/src/bfd/bfdio.c,v
retrieving revision 1.21
diff -u -p -u -r1.21 bfdio.c
--- bfdio.c	24 May 2009 11:47:27 -0000	1.21
+++ bfdio.c	10 Jun 2009 17:01:28 -0000
@@ -158,6 +158,8 @@ DESCRIPTION
 .  int (*bclose) (struct bfd *abfd);
 .  int (*bflush) (struct bfd *abfd);
 .  int (*bstat) (struct bfd *abfd, struct stat *sb);
+.  void* (*bmmap) (struct bfd *abfd, void *addr, bfd_size_type len,
+.                  int prot, int flags, file_ptr offset);
 .};
 
 */
@@ -511,3 +513,31 @@ bfd_get_size (bfd *abfd)
 
   return buf.st_size;
 }
+
+
+/*
+FUNCTION
+	bfd_mmap
+
+SYNOPSIS
+	void *bfd_mmap (bfd *abfd, void *addr, bfd_size_type len,
+	                int prot, int flags, file_ptr offset);
+
+DESCRIPTION
+	Return mmap()ed region of the file, if possible and implemented.
+
+*/
+
+void *
+bfd_mmap (bfd *abfd, void *addr, bfd_size_type len,
+	  int prot, int flags, file_ptr offset)
+{
+  void *ret = (void *)-1;
+  if ((abfd->flags & BFD_IN_MEMORY) != 0)
+    return ret;
+
+  if (abfd->iovec == NULL)
+    return ret;
+
+  return abfd->iovec->bmmap (abfd, addr, len, prot, flags, offset);
+}
Index: cache.c
===================================================================
RCS file: /cvs/src/src/bfd/cache.c,v
retrieving revision 1.34
diff -u -p -u -r1.34 cache.c
--- cache.c	30 Oct 2008 09:05:32 -0000	1.34
+++ cache.c	10 Jun 2009 17:01:28 -0000
@@ -46,6 +46,10 @@ SUBSECTION
 #include "libbfd.h"
 #include "libiberty.h"
 
+#ifdef HAVE_MMAP
+#include <sys/mman.h>
+#endif
+
 /* In some cases we can optimize cache operation when reopening files.
    For instance, a flush is entirely unnecessary if the file is already
    closed, so a flush would use CACHE_NO_OPEN.  Similarly, a seek using
@@ -388,10 +392,38 @@ cache_bstat (struct bfd *abfd, struct st
   return sts;
 }
 
+static void *
+cache_bmmap (struct bfd *abfd ATTRIBUTE_UNUSED,
+	     void *addr ATTRIBUTE_UNUSED,
+	     bfd_size_type len ATTRIBUTE_UNUSED,
+	     int prot ATTRIBUTE_UNUSED,
+	     int flags ATTRIBUTE_UNUSED,
+	     file_ptr offset ATTRIBUTE_UNUSED)
+{
+  void *ret = (void *) -1;
+
+  if ((abfd->flags & BFD_IN_MEMORY) != 0)
+    abort ();
+#ifdef HAVE_MMAP
+  else
+    {
+      FILE *f = bfd_cache_lookup (abfd, CACHE_NO_SEEK_ERROR);
+      if (f == NULL)
+	return ret;
+
+      ret = mmap (addr, len, prot, flags, fileno (f), offset);
+      if (ret == (void *) -1)
+	bfd_set_error (bfd_error_system_call);
+    }
+#endif
+
+  return ret;
+}
+
 static const struct bfd_iovec cache_iovec =
 {
   &cache_bread, &cache_bwrite, &cache_btell, &cache_bseek,
-  &cache_bclose, &cache_bflush, &cache_bstat
+  &cache_bclose, &cache_bflush, &cache_bstat, &cache_bmmap
 };
 
 /*
Index: libbfd.h
===================================================================
RCS file: /cvs/src/src/bfd/libbfd.h,v
retrieving revision 1.216
diff -u -p -u -r1.216 libbfd.h
--- libbfd.h	1 Jun 2009 13:11:52 -0000	1.216
+++ libbfd.h	10 Jun 2009 17:01:28 -0000
@@ -773,6 +773,9 @@ struct bfd_iovec
   int (*bclose) (struct bfd *abfd);
   int (*bflush) (struct bfd *abfd);
   int (*bstat) (struct bfd *abfd, struct stat *sb);
+  /* Just like mmap: (void*)-1 on failure, mmapped address on success.  */
+  void *(*bmmap) (struct bfd *abfd, void *addr, bfd_size_type len,
+                  int prot, int flags, file_ptr offset);
 };
 /* Extracted from bfdwin.c.  */
 struct _bfd_window_internal {
Index: opncls.c
===================================================================
RCS file: /cvs/src/src/bfd/opncls.c,v
retrieving revision 1.53
diff -u -p -u -r1.53 opncls.c
--- opncls.c	23 Aug 2008 08:08:58 -0000	1.53
+++ opncls.c	10 Jun 2009 17:01:28 -0000
@@ -505,9 +505,20 @@ opncls_bstat (struct bfd *abfd, struct s
   return (vec->stat) (abfd, vec->stream, sb);
 }
 
+static void *
+opncls_bmmap (struct bfd *abfd ATTRIBUTE_UNUSED,
+	      void *addr ATTRIBUTE_UNUSED,
+	      bfd_size_type len ATTRIBUTE_UNUSED,
+	      int prot ATTRIBUTE_UNUSED,
+	      int flags ATTRIBUTE_UNUSED,
+	      file_ptr offset ATTRIBUTE_UNUSED)
+{
+  return (void *) -1;
+}
+
 static const struct bfd_iovec opncls_iovec = {
   &opncls_bread, &opncls_bwrite, &opncls_btell, &opncls_bseek,
-  &opncls_bclose, &opncls_bflush, &opncls_bstat
+  &opncls_bclose, &opncls_bflush, &opncls_bstat, &opncls_bmmap
 };
 
 bfd *

Re: How to get file descriptor from abfd?

[Alan Modra]

On Wed, Jun 10, 2009 at 10:09:00AM -0700, Paul Pluzhnikov wrote:
> Corrected patch attached.

OK.

-- 
Alan Modra
Australia Development Lab, IBM

Re: How to get file descriptor from abfd?

[Paul Pluzhnikov]

On Wed, Jun 10, 2009 at 4:59 PM, Alan Modra<amodra@bigpond.net.au> wrote:

> On Wed, Jun 10, 2009 at 10:09:00AM -0700, Paul Pluzhnikov wrote:
>> Corrected patch attached.
>
> OK.

Thanks. So committed.
-- 
Paul Pluzhnikov

Re: How to get file descriptor from abfd?

[Dave Korn]

Paul Pluzhnikov wrote:

> The braces introduce a new scope for 'FILE *f'.
> If I move the declaration to the top of function, then it fails to build
> when !HAVE_MMAP:
> 
>   cc1: warnings being treated as errors
>   ../../src/bfd/cache.c: In function 'cache_bmmap':
>   ../../src/bfd/cache.c:404: warning: unused variable 'f'
> 
> I left this "as is".

  Yes, of course you are correct here; I forgot the complication of the #if
relating to whether the variable was needed or not.

    cheers,
      DaveK

Re: How to get file descriptor from abfd?

[Dave Korn]

Paul Pluzhnikov wrote:

> Index: bfdio.c
> ===================================================================

> +.  void* (*bmmap) (struct bfd *abfd, void *addr, bfd_size_type len,
> +.                  int prot, int flags, file_ptr offset);

  That should probably be void-space-star-bracket, to match:

> +  /* Just like mmap: (void*)-1 on failure, mmapped address on success.  */
> +  void *(*bmmap) (struct bfd *abfd, void *addr, bfd_size_type len,

... but it's not important enough to fix now and can be left for whoever's
working in that area next.

    cheers,
      DaveK