gdb: use bfd_get_section_contents to read section contents in, is_linked_with_cygwin_dll (was: Re: [PATCH 6/7] gdb: select "Cygwin" OS ABI for Cygwin binaries)

Message ID 6b6b7467-2db4-56c9-dd98-3082b7b68abe@polymtl.ca
State New
Headers show
Series
  • gdb: use bfd_get_section_contents to read section contents in, is_linked_with_cygwin_dll (was: Re: [PATCH 6/7] gdb: select "Cygwin" OS ABI for Cygwin binaries)
Related show

Commit Message

Pedro Alves via Gdb-patches April 2, 2020, 2:55 p.m.
On 2020-04-02 9:22 a.m., Tom Tromey wrote:
> There's no way to ever release this data.  Since it's only used once, it

> may be better to use bfd_get_full_section_contents, so the memory can be

> freed when done.


Good point, I hadn't thought of that.

Here's a patch.  I only tested it on GNU/Linux, making sure GDB is still able
to recognize a mingw binary and a cygwin binary.


From b7f4f941018c7b4a7431837ccf5eee5c992f4111 Mon Sep 17 00:00:00 2001
From: Simon Marchi <simon.marchi@polymtl.ca>

Date: Thu, 2 Apr 2020 10:34:39 -0400
Subject: [PATCH] gdb: use bfd_get_section_contents to read section contents in
 is_linked_with_cygwin_dll

The function is_linked_with_cygwin_dll currently uses
gdb_bfd_map_section to get some section contents.  This is not ideal
because that memory, which is only used in this function, can't be
released.  Instead, it was suggested to use bfd_get_section_contents,
and this is what this patch does.

I decided to make a small bfd_get_section_contents wrapper in gdb_bfd.c,
which returns the contents in a gdb::byte_vector.  The wrapper also
makes it easy to get the full section contents into a gdb::byte_vector.
Note that BFD provides the bfd_get_full_section_contents function, but
this one returns a newly-allocated buffer.  gdb_bfd_get_section_contents
offers a consistent interface, regardless of whether you need to read
part of a section or the full section.

gdb_bfd_get_section_contents could be used at many places that already
allocate a vector of the size of the section and then call
bfd_get_section_contents.  I think these call sites can be updated over
time.

gdb/ChangeLog:

	* gdb_bfd.h: Include gdbsupport/byte-vector.h and
	gdbsupport/gdb_optional.h.
	(BFD_SIZE_TYPE_MAX): New macro.
	(gdb_bfd_get_section_contents): New declaration.
	* gdb_bfd.c (gdb_bfd_get_section_contents): New function.
	* windows-tdep.c (is_linked_with_cygwin_dll): Use
	gdb_bfd_get_section_contents.
---
 gdb/gdb_bfd.c      | 25 ++++++++++++++++++++++++-
 gdb/gdb_bfd.h      | 15 +++++++++++++++
 gdb/windows-tdep.c | 13 ++++++-------
 3 files changed, 45 insertions(+), 8 deletions(-)

-- 
2.26.0

Comments

Simon Marchi April 2, 2020, 2:57 p.m. | #1
On 2020-04-02 10:55 a.m., Simon Marchi via Gdb-patches wrote:
> diff --git a/gdb/windows-tdep.c b/gdb/windows-tdep.c

> index 9c5dfd183bfa..9b7dc1d12b26 100644

> --- a/gdb/windows-tdep.c

> +++ b/gdb/windows-tdep.c

> @@ -1005,18 +1005,17 @@ is_linked_with_cygwin_dll (bfd *abfd)

>    bfd_vma idata_addr

>      = pe_data (abfd)->pe_opthdr.DataDirectory[PE_IMPORT_TABLE].VirtualAddress;

> 

> -  /* Map the section's data.  */

> -  bfd_size_type idata_size;

> -  const gdb_byte *const idata_contents

> -    = gdb_bfd_map_section (idata_section, &idata_size);

> -  if (idata_contents == nullptr)

> +  /* Get the section's data.  */

> +  gdb::byte_vector idata_contents;

> +  if (!gdb_bfd_get_section_contents(abfd, idata_section, &idata_contents))


Missing space here, consider it fixed.

Simon
Tom Tromey April 2, 2020, 7:01 p.m. | #2
>>>>> "Simon" == Simon Marchi <simon.marchi@polymtl.ca> writes:


Simon> +  return bfd_get_section_contents (abfd, section, contents->data (), offset, count);

This line looks too long.

Simon> +/* Wrapper around bfd_get_section_contents, returning the requested section
Simon> +   contents in *CONTENTS.  Return true on success, false otherwise.
Simon> +
Simon> +   If COUNT is not specified, read from OFFSET until the end of the
Simon> +   section.  */
Simon> +
Simon> +bool
Simon> +gdb_bfd_get_section_contents (bfd *abfd, asection *section,

Normally in .h files we don't put a newline after the type.

Simon> +  gdb::byte_vector idata_contents;
Simon> +  if (!gdb_bfd_get_section_contents(abfd, idata_section, &idata_contents))

Space before paren.

Otherwise this looks good.  Thank you for doing this.

Tom
Pedro Alves via Gdb-patches April 2, 2020, 7:42 p.m. | #3
On 2020-04-02 3:01 p.m., Tom Tromey wrote:
>>>>>> "Simon" == Simon Marchi <simon.marchi@polymtl.ca> writes:

> 

> Simon> +  return bfd_get_section_contents (abfd, section, contents->data (), offset, count);

> 

> This line looks too long.

> 

> Simon> +/* Wrapper around bfd_get_section_contents, returning the requested section

> Simon> +   contents in *CONTENTS.  Return true on success, false otherwise.

> Simon> +

> Simon> +   If COUNT is not specified, read from OFFSET until the end of the

> Simon> +   section.  */

> Simon> +

> Simon> +bool

> Simon> +gdb_bfd_get_section_contents (bfd *abfd, asection *section,

> 

> Normally in .h files we don't put a newline after the type.

> 

> Simon> +  gdb::byte_vector idata_contents;

> Simon> +  if (!gdb_bfd_get_section_contents(abfd, idata_section, &idata_contents))

> 

> Space before paren.

> 

> Otherwise this looks good.  Thank you for doing this.

> 

> Tom


Now that I re-read it, I don't think the function should take the extra OFFSET and
COUNT parameters, since they are not used anywhere.  I went around our calls to
bfd_get_section_contents, and I don't think they would be very useful.  And I would
prefer not to check in code if it's not going to be exercised.

So I'd go with this simpler version instead.  The extra complexity can be added later,
if needed.

From 791b9d9d068cdf00795fd0d002e4058534099d4f Mon Sep 17 00:00:00 2001
From: Simon Marchi <simon.marchi@polymtl.ca>

Date: Thu, 2 Apr 2020 10:34:39 -0400
Subject: [PATCH] gdb: use bfd_get_section_contents to read section contents in
 is_linked_with_cygwin_dll

The function is_linked_with_cygwin_dll currently uses
gdb_bfd_map_section to get some section contents.  This is not ideal
because that memory, which is only used in this function, can't be
released.  Instead, it was suggested to use
bfd_get_full_section_contents.

However, bfd_get_full_section_contents returns a newly allocated buffer,
which is not very practical to use with C++ automatic memory management
constructs.  I decided to make gdb_bfd_get_full_section_contents, a
small alternative to bfd_get_full_section_contents.  It is a small
wrapper around bfd_get_section_contents which returns the full contents
of the section in a gdb::byte_vector.

gdb_bfd_get_full_section_contents could be used at many places that
already allocate a vector of the size of the section and then call
bfd_get_section_contents.  I think these call sites can be updated over
time.

gdb/ChangeLog:

	* gdb_bfd.h: Include gdbsupport/byte-vector.h.
	(gdb_bfd_get_full_section_contents): New declaration.
	* gdb_bfd.c (gdb_bfd_get_full_section_contents): New function.
	* windows-tdep.c (is_linked_with_cygwin_dll): Use
	gdb_bfd_get_full_section_contents.
---
 gdb/gdb_bfd.c      | 13 ++++++++++++-
 gdb/gdb_bfd.h      |  9 +++++++++
 gdb/windows-tdep.c | 13 ++++++-------
 3 files changed, 27 insertions(+), 8 deletions(-)

diff --git a/gdb/gdb_bfd.c b/gdb/gdb_bfd.c
index 5a6dee2d51a8..a538c461cc88 100644
--- a/gdb/gdb_bfd.c
+++ b/gdb/gdb_bfd.c
@@ -926,7 +926,18 @@ gdb_bfd_requires_relocations (bfd *abfd)
   return gdata->needs_relocations;
 }

-
+/* See gdb_bfd.h.  */
+
+bool
+gdb_bfd_get_full_section_contents (bfd *abfd, asection *section,
+				   gdb::byte_vector *contents)
+{
+  bfd_size_type section_size = bfd_section_size (section);
+
+  contents->resize (section_size);
+
+  return bfd_get_section_contents (abfd, section, contents->data (), 0, section_size);
+}

 /* A callback for htab_traverse that prints a single BFD.  */

diff --git a/gdb/gdb_bfd.h b/gdb/gdb_bfd.h
index 9b1e292bf18f..ce72f78a23f3 100644
--- a/gdb/gdb_bfd.h
+++ b/gdb/gdb_bfd.h
@@ -21,6 +21,7 @@
 #define GDB_BFD_H

 #include "registry.h"
+#include "gdbsupport/byte-vector.h"
 #include "gdbsupport/gdb_ref_ptr.h"

 DECLARE_REGISTRY (bfd);
@@ -181,4 +182,12 @@ int gdb_bfd_count_sections (bfd *abfd);

 int gdb_bfd_requires_relocations (bfd *abfd);

+/* Alternative to bfd_get_full_section_contents that returns the section
+   contents in *CONTENTS, instead of an allocated buffer.
+
+   Return true on success, false otherwise.  */
+
+bool gdb_bfd_get_full_section_contents (bfd *abfd, asection *section,
+					gdb::byte_vector *contents);
+
 #endif /* GDB_BFD_H */
diff --git a/gdb/windows-tdep.c b/gdb/windows-tdep.c
index 0042ea350e80..662a46fe1d70 100644
--- a/gdb/windows-tdep.c
+++ b/gdb/windows-tdep.c
@@ -935,18 +935,17 @@ is_linked_with_cygwin_dll (bfd *abfd)
   bfd_vma idata_addr
     = pe_data (abfd)->pe_opthdr.DataDirectory[PE_IMPORT_TABLE].VirtualAddress;

-  /* Map the section's data.  */
-  bfd_size_type idata_size;
-  const gdb_byte *const idata_contents
-    = gdb_bfd_map_section (idata_section, &idata_size);
-  if (idata_contents == nullptr)
+  /* Get the section's data.  */
+  gdb::byte_vector idata_contents;
+  if (!gdb_bfd_get_full_section_contents (abfd, idata_section, &idata_contents))
     {
       warning (_("Failed to get content of .idata section."));
       return false;
     }

-  const gdb_byte *iter = idata_contents;
-  const gdb_byte *end = idata_contents + idata_size;
+  size_t idata_size = idata_contents.size ();
+  const gdb_byte *iter = idata_contents.data ();
+  const gdb_byte *end = idata_contents.data () + idata_size;
   const pe_import_directory_entry null_dir_entry = { 0 };

   /* Iterate through all directory entries.  */
-- 
2.26.0
Tom Tromey April 2, 2020, 7:45 p.m. | #4
>>>>> "Simon" == Simon Marchi via Gdb-patches <gdb-patches@sourceware.org> writes:


Simon> +  return bfd_get_section_contents (abfd, section, contents->data (), offset, count);
>> 

>> This line looks too long.


It's still too long in this version.

Simon> Now that I re-read it, I don't think the function should take the extra OFFSET and
Simon> COUNT parameters, since they are not used anywhere.  I went around our calls to
Simon> bfd_get_section_contents, and I don't think they would be very useful.  And I would
Simon> prefer not to check in code if it's not going to be exercised.

Simon> So I'd go with this simpler version instead.  The extra complexity can be added later,
Simon> if needed.

Sounds reasonable to me.

Tom
Pedro Alves via Gdb-patches April 2, 2020, 7:47 p.m. | #5
On 2020-04-02 3:45 p.m., Tom Tromey wrote:
>>>>>> "Simon" == Simon Marchi via Gdb-patches <gdb-patches@sourceware.org> writes:

> 

> Simon> +  return bfd_get_section_contents (abfd, section, contents->data (), offset, count);

>>>

>>> This line looks too long.

> 

> It's still too long in this version.


Sorry, I missed that comment earlier.  Fixed now.

> Simon> Now that I re-read it, I don't think the function should take the extra OFFSET and

> Simon> COUNT parameters, since they are not used anywhere.  I went around our calls to

> Simon> bfd_get_section_contents, and I don't think they would be very useful.  And I would

> Simon> prefer not to check in code if it's not going to be exercised.

> 

> Simon> So I'd go with this simpler version instead.  The extra complexity can be added later,

> Simon> if needed.

> 

> Sounds reasonable to me.


Thanks, I'm pushing it with the above issue fixed.

Simon

Patch

diff --git a/gdb/gdb_bfd.c b/gdb/gdb_bfd.c
index 5a6dee2d51a8..2b34e0f3e17c 100644
--- a/gdb/gdb_bfd.c
+++ b/gdb/gdb_bfd.c
@@ -926,7 +926,30 @@  gdb_bfd_requires_relocations (bfd *abfd)
   return gdata->needs_relocations;
 }

-
+/* See gdb_bfd.h.  */
+
+bool
+gdb_bfd_get_section_contents (bfd *abfd, asection *section,
+			      gdb::byte_vector *contents, file_ptr offset,
+			      bfd_size_type count)
+{
+  bfd_size_type section_size = bfd_section_size (section);
+
+  /* Allow `offset == section_size`, to allow the corner case of
+     `offset == section_size`, `count = 0`.  */
+  gdb_assert (offset <= section_size);
+
+  /* If COUNT is unspecified, get the contents from OFFSET until the end of the
+     section.  */
+  if (count == BFD_SIZE_TYPE_MAX)
+    count = section_size - offset;
+
+  gdb_assert ((offset + count) <= section_size);
+
+  contents->resize (count);
+
+  return bfd_get_section_contents (abfd, section, contents->data (), offset, count);
+}

 /* A callback for htab_traverse that prints a single BFD.  */

diff --git a/gdb/gdb_bfd.h b/gdb/gdb_bfd.h
index 9b1e292bf18f..ffea4f6b715e 100644
--- a/gdb/gdb_bfd.h
+++ b/gdb/gdb_bfd.h
@@ -21,6 +21,8 @@ 
 #define GDB_BFD_H

 #include "registry.h"
+#include "gdbsupport/byte-vector.h"
+#include "gdbsupport/gdb_optional.h"
 #include "gdbsupport/gdb_ref_ptr.h"

 DECLARE_REGISTRY (bfd);
@@ -30,6 +32,8 @@  DECLARE_REGISTRY (bfd);

 #define TARGET_SYSROOT_PREFIX "target:"

+#define BFD_SIZE_TYPE_MAX ((bfd_size_type) -1)
+
 /* Returns nonzero if NAME starts with TARGET_SYSROOT_PREFIX, zero
    otherwise.  */

@@ -181,4 +185,15 @@  int gdb_bfd_count_sections (bfd *abfd);

 int gdb_bfd_requires_relocations (bfd *abfd);

+/* Wrapper around bfd_get_section_contents, returning the requested section
+   contents in *CONTENTS.  Return true on success, false otherwise.
+
+   If COUNT is not specified, read from OFFSET until the end of the
+   section.  */
+
+bool
+gdb_bfd_get_section_contents (bfd *abfd, asection *section,
+			      gdb::byte_vector *contents, file_ptr offset = 0,
+			      bfd_size_type count = BFD_SIZE_TYPE_MAX);
+
 #endif /* GDB_BFD_H */
diff --git a/gdb/windows-tdep.c b/gdb/windows-tdep.c
index 9c5dfd183bfa..9b7dc1d12b26 100644
--- a/gdb/windows-tdep.c
+++ b/gdb/windows-tdep.c
@@ -1005,18 +1005,17 @@  is_linked_with_cygwin_dll (bfd *abfd)
   bfd_vma idata_addr
     = pe_data (abfd)->pe_opthdr.DataDirectory[PE_IMPORT_TABLE].VirtualAddress;

-  /* Map the section's data.  */
-  bfd_size_type idata_size;
-  const gdb_byte *const idata_contents
-    = gdb_bfd_map_section (idata_section, &idata_size);
-  if (idata_contents == nullptr)
+  /* Get the section's data.  */
+  gdb::byte_vector idata_contents;
+  if (!gdb_bfd_get_section_contents(abfd, idata_section, &idata_contents))
     {
       warning (_("Failed to get content of .idata section."));
       return false;
     }

-  const gdb_byte *iter = idata_contents;
-  const gdb_byte *end = idata_contents + idata_size;
+  size_t idata_size = idata_contents.size ();
+  const gdb_byte *iter = idata_contents.data ();
+  const gdb_byte *end = idata_contents.data () + idata_size;
   const pe_import_directory_entry null_dir_entry = { 0 };

   /* Iterate through all directory entries.  */