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

[Pedro Alves <palves@redhat.com>]

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