Re: [PATCH 6/7] gdb: select "Cygwin" OS ABI for Cygwin binaries

Simon Marchi
2020-04-01T21:54:15Z

On 2020-04-01 5:36 p.m., Pedro Alves wrote:
> On 3/16/20 5:08 PM, Simon Marchi via Gdb-patches wrote:
>> +bool
>> +is_linked_with_cygwin_dll (bfd *abfd)
>> +{
>> + /* The list of DLLs a PE is linked to is in the .idata section. See:
>> +
>> + https://docs.microsoft.com/en-us/windows/win32/debug/pe-format#the-idata-section
>> + */
>> + asection *idata_section = bfd_get_section_by_name (abfd, ".idata");
>> + if (idata_section == nullptr)
>> + return false;
>> +
>> + /* Find the virtual address of the .idata section. We must subtract this
>> + from the RVAs (relative virtual addresses) to obtain an offset in the
>> + section. */
>> + bfd_vma idata_addr =
>> + pe_data (abfd)->pe_opthdr.DataDirectory[PE_IMPORT_TABLE].VirtualAddress;
>
> = on next line. Unless it doesn't fit, then let's ignore.

Yes it fits.
>> +
>> + /* 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)
>> + {
>> + warning (_("Failed to get content of .idata section."));
>> + return false;
>> + }
>> +
>> + const gdb_byte *iter = idata_contents;
>> + const gdb_byte *end = idata_contents + idata_size;
>> + const pe_import_directory_entry null_dir_entry = { 0 };
>> +
>> + /* Iterate through all directory entries. */
>> + while (true)
>> + {
>> + /* Is there enough space left in the section for another entry? */
>> + if (iter + sizeof (pe_import_directory_entry) > end)
>> + {
>> + warning (_("Failed to parse .idata section: unexpected end of "
>> + ".idata section."));
>> + break;
>> + }
>> +
>> + pe_import_directory_entry *dir_entry = (pe_import_directory_entry *) iter;
>
> Is 'iter' guaranteed to be sufficiently aligned? Recall that this is
> host-independent code.

I admit I haven't thought about that. Within the PE file, the data of sections is
aligned on at least 512 bytes. See "FileAlignment" here:

https://docs.microsoft.com/en-us/windows/win32/debug/pe-format#optional-header-windows-specific-fields-image-only

However, when BFD maps the file/section data in memory for GDB to read, is that mapping
guaranteed to be sufficiently aligned as well?

>> +
>> + /* Is it the end of list marker? */
>> + if (memcmp (dir_entry, &null_dir_entry,
>> + sizeof (pe_import_directory_entry)) == 0)
>> + break;
>> +
>> + bfd_vma name_addr = dir_entry->name_rva;
>> +
>> + /* If the name's virtual address is smaller than the section's virtual
>> + address, there's a problem. */
>> + if (name_addr < idata_addr
>> + || name_addr >= (idata_addr + idata_size))
>> + {
>> + warning (_("\
>> +Failed to parse .idata section: name's virtual address (0x%" BFD_VMA_FMT "x) \
>> +is outside .idata section's range [0x%" BFD_VMA_FMT "x, 0x%" BFD_VMA_FMT "x[."),
>> + name_addr, idata_addr, idata_addr + idata_size);
>> + break;
>> + }
>> +
>> + const gdb_byte *name = &idata_contents[name_addr - idata_addr];
>> +
>> + /* Make sure we don't overshoot the end of the section with the streq. */
>> + if (name + sizeof(CYGWIN_DLL_NAME) > end)
>
> Missing space before parens.

This patch is already pushed so I've pushed the following to fix the style issues.

Thanks for the review,

Simon

From 2836752f8ff55ea1fc7f6b1e7f8ff778775646f8 Mon Sep 17 00:00:00 2001
From: Simon Marchi <simon.marchi@polymtl.ca>
Date: Wed, 1 Apr 2020 17:41:31 -0400
Subject: [PATCH] gdb: fix style issues in is_linked_with_cygwin_dll

gdb/ChangeLog:

* windows-tdep.c (is_linked_with_cygwin_dll): Fix style.
---
gdb/ChangeLog | 4 ++++
gdb/windows-tdep.c | 8 ++++----
2 files changed, 8 insertions(+), 4 deletions(-)

diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index d5715a8fa005..4775ff858aab 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,3 +1,7 @@
+2020-04-01 Simon Marchi <simon.marchi@polymtl.ca>
+
+ * windows-tdep.c (is_linked_with_cygwin_dll): Fix style.
+
2020-04-01 Bernd Edlinger <bernd.edlinger@hotmail.de>

* buildsym.c (record_line): Fix undefined behavior and preserve
diff --git a/gdb/windows-tdep.c b/gdb/windows-tdep.c
index 31b7b57005df..0042ea350e80 100644
--- a/gdb/windows-tdep.c
+++ b/gdb/windows-tdep.c
@@ -932,8 +932,8 @@ is_linked_with_cygwin_dll (bfd *abfd)
/* Find the virtual address of the .idata section. We must subtract this
from the RVAs (relative virtual addresses) to obtain an offset in the
section. */
- bfd_vma idata_addr =
- pe_data (abfd)->pe_opthdr.DataDirectory[PE_IMPORT_TABLE].VirtualAddress;
+ bfd_vma idata_addr
+ = pe_data (abfd)->pe_opthdr.DataDirectory[PE_IMPORT_TABLE].VirtualAddress;

/* Map the section's data. */
bfd_size_type idata_size;
@@ -984,14 +984,14 @@ is outside .idata section's range [0x%" BFD_VMA_FMT "x, 0x%" BFD_VMA_FMT "x[."),
const gdb_byte *name = &idata_contents[name_addr - idata_addr];

/* Make sure we don't overshoot the end of the section with the streq. */
- if (name + sizeof(CYGWIN_DLL_NAME) > end)
+ if (name + sizeof (CYGWIN_DLL_NAME) > end)
continue;

/* Finally, check if this is the dll name we are looking for. */
if (streq ((const char *) name, CYGWIN_DLL_NAME))
return true;

- iter += sizeof(pe_import_directory_entry);
+ iter += sizeof (pe_import_directory_entry);
}

return false;
--
2.26.0