Re: [PATCH] gdb: use bfd_get_section_contents to read section contents in, is_linked_with_cygwin_dll

[Simon Marchi <simon.marchi@polymtl.ca>]

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;
 }

-\f
+/* 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