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

Pedro Alves
2020-04-01T21:37:02Z

On 3/16/20 5:08 PM, Simon Marchi via Gdb-patches wrote:
> From: Simon Marchi <simon.marchi@efficios.com>
>
> Before this patch, the "Windows" OS ABI is selected for all Windows
> executables, including Cygwin ones. This patch makes GDB differentiate
> Cygwin binaries from non-Cygwin ones, and selects the "Cygwin" OS ABI
> for the Cygwin ones.
>
> To check whether a Windows PE executable is a Cygwin one, we check the
> library list in the .idata section, see if it contains "cygwin1.dll".
>
> I had to add code to parse the .idata section, because BFD doesn't seem
> to expose this information. BFD does parse this information, but only
> to print it in textual form (function pe_print_idata):
>
> https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;a=blob;f=bfd/peXXigen.c;h=e42d646552a0ca1e856e082256cd3d943b54ddf0;hb=HEAD#l1261
>
> Here's the relevant portion of the PE format documentation:
>
> https://docs.microsoft.com/en-us/windows/win32/debug/pe-format#the-idata-section
>
> This page was also useful:
>
> https://blog.kowalczyk.info/articles/pefileformat.html#9ccef823-67e7-4372-9172-045d7b1fb006
>
> With this patch applied, this is what I get:
>
> (gdb) file some_mingw_x86_64_binary.exe
> Reading symbols from some_mingw_x86_64_binary.exe...
> (gdb) show osabi
> The current OS ABI is "auto" (currently "Windows").
> The default OS ABI is "GNU/Linux".
>
> (gdb) file some_mingw_i386_binary.exe
> Reading symbols from some_mingw_i386_binary.exe...
> (gdb) show osabi
> The current OS ABI is "auto" (currently "Windows").
> The default OS ABI is "GNU/Linux".
>
> (gdb) file some_cygwin_x86_64_binary.exe
> Reading symbols from some_cygwin_x86_64_binary.exe...
> (gdb) show osabi
> The current OS ABI is "auto" (currently "Cygwin").
> The default OS ABI is "GNU/Linux".
>
> gdb/ChangeLog:
>
> * windows-tdep.h (is_linked_with_cygwin_dll): New declaration.
> * windows-tdep.c (CYGWIN_DLL_NAME): New.
> (pe_import_directory_entry): New struct type.
> (is_linked_with_cygwin_dll): New function.
> * amd64-windows-tdep.c (amd64_windows_osabi_sniffer): Select
> GDB_OSABI_CYGWIN if the BFD is linked with the Cygwin DLL.
> * i386-windows-tdep.c (i386_windows_osabi_sniffer): Likewise.
> ---
> gdb/amd64-windows-tdep.c | 9 ++--
> gdb/i386-windows-tdep.c | 9 ++--
> gdb/windows-tdep.c | 101 +++++++++++++++++++++++++++++++++++++++
> gdb/windows-tdep.h | 6 +++
> 4 files changed, 119 insertions(+), 6 deletions(-)
>
> diff --git a/gdb/amd64-windows-tdep.c b/gdb/amd64-windows-tdep.c
> index 88ff794abcb6..e0346f8628fe 100644
> --- a/gdb/amd64-windows-tdep.c
> +++ b/gdb/amd64-windows-tdep.c
> @@ -1249,10 +1249,13 @@ amd64_windows_osabi_sniffer (bfd *abfd)
> {
> const char *target_name = bfd_get_target (abfd);
>
> - if (strcmp (target_name, "pei-x86-64") == 0)
> - return GDB_OSABI_WINDOWS;
> + if (!streq (target_name, "pei-x86-64"))
> + return GDB_OSABI_UNKNOWN;
>
> - return GDB_OSABI_UNKNOWN;
> + if (is_linked_with_cygwin_dll (abfd))
> + return GDB_OSABI_CYGWIN;
> +
> + return GDB_OSABI_WINDOWS;
> }
>
> void _initialize_amd64_windows_tdep ();
> diff --git a/gdb/i386-windows-tdep.c b/gdb/i386-windows-tdep.c
> index a71ceda781f8..bd6107b02f1f 100644
> --- a/gdb/i386-windows-tdep.c
> +++ b/gdb/i386-windows-tdep.c
> @@ -232,10 +232,13 @@ i386_windows_osabi_sniffer (bfd *abfd)
> {
> const char *target_name = bfd_get_target (abfd);
>
> - if (strcmp (target_name, "pei-i386") == 0)
> - return GDB_OSABI_WINDOWS;
> + if (!streq (target_name, "pei-i386"))
> + return GDB_OSABI_UNKNOWN;
>
> - return GDB_OSABI_UNKNOWN;
> + if (is_linked_with_cygwin_dll (abfd))
> + return GDB_OSABI_CYGWIN;
> +
> + return GDB_OSABI_WINDOWS;
> }
>
> static enum gdb_osabi
> diff --git a/gdb/windows-tdep.c b/gdb/windows-tdep.c
> index e02b1ceed387..32e551bcb175 100644
> --- a/gdb/windows-tdep.c
> +++ b/gdb/windows-tdep.c
> @@ -38,6 +38,8 @@
> #include "libcoff.h"
> #include "solist.h"
>
> +#define CYGWIN_DLL_NAME "cygwin1.dll"
> +
> /* Windows signal numbers differ between MinGW flavors and between
> those and Cygwin. The below enumeration was gleaned from the
> respective headers; the ones marked with MinGW64/Cygwin are defined
> @@ -898,6 +900,105 @@ static const struct internalvar_funcs tlb_funcs =
> NULL
> };
>
> +/* Layout of an element of a PE's Import Directory Table. Based on:
> +
> + https://docs.microsoft.com/en-us/windows/win32/debug/pe-format#import-directory-table
> + */
> +
> +struct pe_import_directory_entry
> +{
> + uint32_t import_lookup_table_rva;
> + uint32_t timestamp;
> + uint32_t forwarder_chain;
> + uint32_t name_rva;
> + uint32_t import_address_table_rva;
> +};
> +
> +gdb_static_assert (sizeof (pe_import_directory_entry) == 20);
> +
> +/* Return true if the Portable Executable behind ABFD uses the Cygwin dll
> + (cygwin1.dll). */
> +/* See windows-tdep.h. */
> +
> +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.

> +
> + /* 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.

> +
> + /* 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.

Thanks,
Pedro Alves