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

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

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