From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp.polymtl.ca (smtp.polymtl.ca [132.207.4.11]) by sourceware.org (Postfix) with ESMTPS id 50270385BF92 for ; Thu, 2 Apr 2020 19:42:57 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.3.2 sourceware.org 50270385BF92 Received: from simark.ca (simark.ca [158.69.221.121]) (authenticated bits=0) by smtp.polymtl.ca (8.14.7/8.14.7) with ESMTP id 032Jgghw026282 (version=TLSv1/SSLv3 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=NOT); Thu, 2 Apr 2020 15:42:47 -0400 DKIM-Filter: OpenDKIM Filter v2.11.0 smtp.polymtl.ca 032Jgghw026282 Received: from [10.0.0.11] (unknown [192.222.164.54]) (using TLSv1.3 with cipher TLS_AES_128_GCM_SHA256 (128/128 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by simark.ca (Postfix) with ESMTPSA id B15A41E581; Thu, 2 Apr 2020 15:42:42 -0400 (EDT) Subject: Re: [PATCH] gdb: use bfd_get_section_contents to read section contents in, is_linked_with_cygwin_dll To: Tom Tromey Cc: Pedro Alves via Gdb-patches , Pedro Alves , Simon Marchi , Jon Turney References: <20200316170845.184386-1-simon.marchi@polymtl.ca> <20200316170845.184386-7-simon.marchi@polymtl.ca> <0b76517a-f6dd-0aa8-17fb-ce5a9accbf42@redhat.com> <87ftdmvx85.fsf@tromey.com> <6b6b7467-2db4-56c9-dd98-3082b7b68abe@polymtl.ca> <878sjdvhi4.fsf@tromey.com> From: Simon Marchi Message-ID: <9b3e78c7-5df5-aa83-d04c-0060b4c75ce7@polymtl.ca> Date: Thu, 2 Apr 2020 15:42:41 -0400 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101 Thunderbird/68.6.0 MIME-Version: 1.0 In-Reply-To: <878sjdvhi4.fsf@tromey.com> Content-Type: text/plain; charset=utf-8 Content-Language: en-US-large Content-Transfer-Encoding: 7bit X-Poly-FromMTA: (simark.ca [158.69.221.121]) at Thu, 2 Apr 2020 19:42:43 +0000 X-Spam-Status: No, score=-25.7 required=5.0 tests=BAYES_00, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, GIT_PATCH_0, GIT_PATCH_1, GIT_PATCH_2, GIT_PATCH_3, RCVD_IN_MSPIKE_H3, RCVD_IN_MSPIKE_WL, SPF_HELO_PASS, SPF_PASS, TXREP autolearn=ham autolearn_force=no version=3.4.2 X-Spam-Checker-Version: SpamAssassin 3.4.2 (2018-09-13) on server2.sourceware.org X-BeenThere: gdb-patches@sourceware.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Gdb-patches mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 02 Apr 2020 19:43:00 -0000 On 2020-04-02 3:01 p.m., Tom Tromey wrote: >>>>>> "Simon" == Simon Marchi 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 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