[PATCH 0/7] Add "Windows" OS ABI

[PATCH 0/7] Add "Windows" OS ABI

[Simon Marchi]

This patchset started out as a single patch to have the OS ABI Cygwin
applied to Windows x86-64 binaries, here:

    https://sourceware.org/legacy-ml/gdb-patches/2020-03/msg00195.html

with the follow-up here:

    https://sourceware.org/pipermail/gdb-patches/2020-March/000022.html

Eli pointed out that it doesn't make sense for binaries compilied with
MinGW to have the Cygwin OS ABI, that there should be separate OS ABIs
for Cygwin and non-Cygwin Windows binaries.  This already came up in the
following bug report:

    https://sourceware.org/bugzilla/show_bug.cgi?id=21500#add_comment

This patchset does a bit of refactor in that area.  Most importantly, it:

- adds a "Windows" OS ABI
- makes GDB recognize the proper OS ABI (Cygwin or Windows) when
  loading executables
- makes the builtin long type on Cygwin be 64 bits long

Simon Marchi (7):
  gdb: recognize 64 bits Windows executables as Cygwin osabi
  gdb: move enum gdb_osabi to osabi.h
  gdb: add Windows OS ABI
  gdb: rename i386-cygwin-tdep.c to i386-windows-tdep.c
  gdb: rename content of i386-windows-tdep.c, cygwin to windows
  gdb: select "Cygwin" OS ABI for Cygwin binaries
  gdb: define builtin long type to be 64 bits on amd64 Cygwin

 gdb/Makefile.in                               |   4 +-
 gdb/amd64-windows-tdep.c                      |  41 ++++++-
 gdb/configure.tgt                             |  10 +-
 gdb/defs.h                                    |  31 ------
 gdb/gdbarch.h                                 |   1 +
 gdb/gdbarch.sh                                |   1 +
 ...i386-cygwin-tdep.c => i386-windows-tdep.c} |  45 +++++---
 gdb/osabi.c                                   |   1 +
 gdb/osabi.h                                   |  32 ++++++
 gdb/windows-tdep.c                            | 101 ++++++++++++++++++
 gdb/windows-tdep.h                            |   6 ++
 11 files changed, 215 insertions(+), 58 deletions(-)
 rename gdb/{i386-cygwin-tdep.c => i386-windows-tdep.c} (83%)

-- 
2.25.1

[PATCH 1/7] gdb: recognize 64 bits Windows executables as Cygwin osabi

[Simon Marchi]

From: Simon Marchi <simon.marchi@efficios.com>

If I generate two Windows PE executables, one 32 bits and one 64 bits:

    $ x86_64-w64-mingw32-gcc test.c -g3 -O0 -o test_64
    $ i686-w64-mingw32-gcc test.c -g3 -O0 -o test_32
    $ file test_64
    test_64: PE32+ executable (console) x86-64, for MS Windows
    $ file test_32
    test_32: PE32 executable (console) Intel 80386, for MS Windows

When I load the 32 bits binary in my GNU/Linux-hosted GDB, the osabi is
correctly recognized as "Cygwin":

    $ ./gdb --data-directory=data-directory -nx test_32
    (gdb) show osabi
    The current OS ABI is "auto" (currently "Cygwin").

When I load the 64 bits binary in GDB, the osabi is incorrectly
recognized as "GNU/Linux":

    $ ./gdb --data-directory=data-directory -nx test_64
    (gdb) show osabi
    The current OS ABI is "auto" (currently "GNU/Linux").

The 32 bits one gets recognized by the i386_cygwin_osabi_sniffer
function, by its target name:

    if (strcmp (target_name, "pei-i386") == 0)
      return GDB_OSABI_CYGWIN;

The target name for the 64 bits binaries is "pei-x86-64".  It doesn't
get recognized by any osabi sniffer, so GDB falls back on its default
osabi, "GNU/Linux".

This patch adds an osabi sniffer function for the Windows 64 bits
executables in amd64-windows-tdep.c.  With it, the osabi is recognized
as "Cygwin", just like with the 32 bits binary.

Note that it may seems strange to have a binary generated by MinGW
(which has nothing to do with Cygwin) be recognized as a Cygwin binary.
This is indeed not accurate, but at the moment GDB uses the Cygwin for
everything Windows.  Subsequent patches will add a separate "Windows" OS
ABI for Windows binaries that are not Cygwin binaries.

gdb/ChangeLog:

	* amd64-windows-tdep.c (amd64_windows_osabi_sniffer): New
	function.
	(_initialize_amd64_windows_tdep): Register osabi sniffer.
---
 gdb/amd64-windows-tdep.c | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/gdb/amd64-windows-tdep.c b/gdb/amd64-windows-tdep.c
index d4d79682dd18..2ca979513cd7 100644
--- a/gdb/amd64-windows-tdep.c
+++ b/gdb/amd64-windows-tdep.c
@@ -1244,10 +1244,24 @@ amd64_windows_init_abi (struct gdbarch_info info, struct gdbarch *gdbarch)
   set_gdbarch_auto_wide_charset (gdbarch, amd64_windows_auto_wide_charset);
 }
 
+static gdb_osabi
+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_CYGWIN;
+
+  return GDB_OSABI_UNKNOWN;
+}
+
 void _initialize_amd64_windows_tdep ();
 void
 _initialize_amd64_windows_tdep ()
 {
   gdbarch_register_osabi (bfd_arch_i386, bfd_mach_x86_64, GDB_OSABI_CYGWIN,
                           amd64_windows_init_abi);
+
+  gdbarch_register_osabi_sniffer (bfd_arch_i386, bfd_target_coff_flavour,
+				  amd64_windows_osabi_sniffer);
 }
-- 
2.25.1

[PATCH 2/7] gdb: move enum gdb_osabi to osabi.h

[Simon Marchi]

From: Simon Marchi <simon.marchi@efficios.com>

I think it makes sense to have it there instead of in the catch-all
defs.h.

gdb/ChangeLog:

	* defs.h (enum gdb_osabi): Move to...
	* osabi.h (enum gdb_osabi): ... here.
	* gdbarch.sh: Include osabi.h in gdbarch.h.
	* gdbarch.h: Re-generate.
---
 gdb/defs.h     | 31 -------------------------------
 gdb/gdbarch.h  |  1 +
 gdb/gdbarch.sh |  1 +
 gdb/osabi.h    | 31 +++++++++++++++++++++++++++++++
 4 files changed, 33 insertions(+), 31 deletions(-)

diff --git a/gdb/defs.h b/gdb/defs.h
index 1ad52feb1f80..a75511158a40 100644
--- a/gdb/defs.h
+++ b/gdb/defs.h
@@ -478,37 +478,6 @@ enum val_prettyformat
 
 extern int longest_to_int (LONGEST);
 
-/* * List of known OS ABIs.  If you change this, make sure to update the
-   table in osabi.c.  */
-enum gdb_osabi
-{
-  GDB_OSABI_UNKNOWN = 0,	/* keep this zero */
-  GDB_OSABI_NONE,
-
-  GDB_OSABI_SVR4,
-  GDB_OSABI_HURD,
-  GDB_OSABI_SOLARIS,
-  GDB_OSABI_LINUX,
-  GDB_OSABI_FREEBSD,
-  GDB_OSABI_NETBSD,
-  GDB_OSABI_OPENBSD,
-  GDB_OSABI_WINCE,
-  GDB_OSABI_GO32,
-  GDB_OSABI_QNXNTO,
-  GDB_OSABI_CYGWIN,
-  GDB_OSABI_AIX,
-  GDB_OSABI_DICOS,
-  GDB_OSABI_DARWIN,
-  GDB_OSABI_SYMBIAN,
-  GDB_OSABI_OPENVMS,
-  GDB_OSABI_LYNXOS178,
-  GDB_OSABI_NEWLIB,
-  GDB_OSABI_SDE,
-  GDB_OSABI_PIKEOS,
-
-  GDB_OSABI_INVALID		/* keep this last */
-};
-
 /* Enumerate the requirements a symbol has in order to be evaluated.
    These are listed in order of "strength" -- a later entry subsumes
    earlier ones.  This fine-grained distinction is important because
diff --git a/gdb/gdbarch.h b/gdb/gdbarch.h
index 0259fcdbfd26..6dbb9d571ddb 100644
--- a/gdb/gdbarch.h
+++ b/gdb/gdbarch.h
@@ -40,6 +40,7 @@
 #include "dis-asm.h"
 #include "gdb_obstack.h"
 #include "infrun.h"
+#include "osabi.h"
 
 struct floatformat;
 struct ui_file;
diff --git a/gdb/gdbarch.sh b/gdb/gdbarch.sh
index 4a4b1bc66cfa..5a39dec83da2 100755
--- a/gdb/gdbarch.sh
+++ b/gdb/gdbarch.sh
@@ -1313,6 +1313,7 @@ cat <<EOF
 #include "dis-asm.h"
 #include "gdb_obstack.h"
 #include "infrun.h"
+#include "osabi.h"
 
 struct floatformat;
 struct ui_file;
diff --git a/gdb/osabi.h b/gdb/osabi.h
index bb0812e567f4..ff63db49affe 100644
--- a/gdb/osabi.h
+++ b/gdb/osabi.h
@@ -19,6 +19,37 @@
 #ifndef OSABI_H
 #define OSABI_H
 
+/* * List of known OS ABIs.  If you change this, make sure to update the
+   table in osabi.c.  */
+enum gdb_osabi
+{
+  GDB_OSABI_UNKNOWN = 0,	/* keep this zero */
+  GDB_OSABI_NONE,
+
+  GDB_OSABI_SVR4,
+  GDB_OSABI_HURD,
+  GDB_OSABI_SOLARIS,
+  GDB_OSABI_LINUX,
+  GDB_OSABI_FREEBSD,
+  GDB_OSABI_NETBSD,
+  GDB_OSABI_OPENBSD,
+  GDB_OSABI_WINCE,
+  GDB_OSABI_GO32,
+  GDB_OSABI_QNXNTO,
+  GDB_OSABI_CYGWIN,
+  GDB_OSABI_AIX,
+  GDB_OSABI_DICOS,
+  GDB_OSABI_DARWIN,
+  GDB_OSABI_SYMBIAN,
+  GDB_OSABI_OPENVMS,
+  GDB_OSABI_LYNXOS178,
+  GDB_OSABI_NEWLIB,
+  GDB_OSABI_SDE,
+  GDB_OSABI_PIKEOS,
+
+  GDB_OSABI_INVALID		/* keep this last */
+};
+
 /* Register an OS ABI sniffer.  Each arch/flavour may have more than
    one sniffer.  This is used to e.g. differentiate one OS's a.out from
    another.  The first sniffer to return something other than
-- 
2.25.1

[PATCH 3/7] gdb: add Windows OS ABI

[Simon Marchi]

From: Simon Marchi <simon.marchi@efficios.com>

GDB currently uses the "Cygwin" OS ABI (GDB_OSABI_CYGWIN) for everything
related to Windows.  If you build a GDB for a MinGW or Cygwin target, it
will have "Cygwin" as the default OS ABI in both cases (see
configure.tgt).  If you load either a MinGW or Cygwin binary, the
"Cygwin" OS ABI will be selected in both cases.

This is misleading, because Cygwin binaries are a subset of the binaries
running on Windows.  When building something with MinGW, the resulting
binary has nothing to do with Cygwin.  Cygwin binaries are only special
in that they are Windows binaries that link to the cygwin1.dll library
(if my understanding is correct).

Looking at i386-cygwin-tdep.c, we can see that GDB does nothing
different when dealing with Cygwin binaries versus non-Cygwin Windows
binaries.  However, there is at least one known bug which would require
us to make a distinction between the two OS ABIs, and that is the size
of the built-in "long" type on x86-64.  On native Windows, this is 4,
whereas on Cygwin it's 8.

So, this patch adds a new OS ABI, "Windows", and makes GDB use it for
i386 and x86-64 PE executables, instead of the "Cygwin" OS ABI.  A
subsequent patch will improve the OS ABI detection so that GDB
differentiates the non-Cygwin Windows binaries from the Cygwin Windows
binaries, and applies the "Cygwin" OS ABI for the latter.

The default OS ABI remains "Cygwin" for the GDBs built with a Cygwin
target.

I've decided to split the i386_cygwin_osabi_sniffer function in two,
I think it's cleaner to have a separate sniffer for Windows binaries and
Cygwin cores, each checking one specific thing.

gdb/ChangeLog:

	* osabi.h (enum gdb_osabi): Add GDB_OSABI_WINDOWS.
	* osabi.c (gdb_osabi_names): Add "Windows".
	* i386-cygwin-tdep.c (i386_cygwin_osabi_sniffer): Return
	GDB_OSABI_WINDOWS when the binary's target is "pei-i386".
	(i386_cygwin_core_osabi_sniffer): New function, extracted from
	i386_cygwin_osabi_sniffer.
	(_initialize_i386_cygwin_tdep): Register OS ABI
	GDB_OSABI_WINDOWS for i386.
	* amd64-windows-tdep.c (amd64_windows_osabi_sniffer): Return
	GDB_OSABI_WINDOWS when the binary's target is "pei-x86-64".
	(_initialize_amd64_windows_tdep): Register OS ABI GDB_OSABI_WINDOWS
	for x86-64.
	* configure.tgt: Use GDB_OSABI_WINDOWS as the default OS ABI
	when the target matches '*-*-mingw*'.
---
 gdb/amd64-windows-tdep.c |  5 ++++-
 gdb/configure.tgt        |  4 ++--
 gdb/i386-cygwin-tdep.c   | 17 ++++++++++++++---
 gdb/osabi.c              |  1 +
 gdb/osabi.h              |  1 +
 5 files changed, 22 insertions(+), 6 deletions(-)

diff --git a/gdb/amd64-windows-tdep.c b/gdb/amd64-windows-tdep.c
index 2ca979513cd7..88ff794abcb6 100644
--- a/gdb/amd64-windows-tdep.c
+++ b/gdb/amd64-windows-tdep.c
@@ -1250,7 +1250,7 @@ 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_CYGWIN;
+    return GDB_OSABI_WINDOWS;
 
   return GDB_OSABI_UNKNOWN;
 }
@@ -1259,6 +1259,9 @@ void _initialize_amd64_windows_tdep ();
 void
 _initialize_amd64_windows_tdep ()
 {
+  /* The Cygwin and Windows OS ABIs are currently equivalent.  */
+  gdbarch_register_osabi (bfd_arch_i386, bfd_mach_x86_64, GDB_OSABI_WINDOWS,
+                          amd64_windows_init_abi);
   gdbarch_register_osabi (bfd_arch_i386, bfd_mach_x86_64, GDB_OSABI_CYGWIN,
                           amd64_windows_init_abi);
 
diff --git a/gdb/configure.tgt b/gdb/configure.tgt
index 755187dca657..6ebd32410e99 100644
--- a/gdb/configure.tgt
+++ b/gdb/configure.tgt
@@ -771,8 +771,8 @@ m68*-*-openbsd* | m88*-*-openbsd* | vax-*-openbsd*) ;;
 *-*-*-gnu*)	;; # prevent non-GNU kernels to match the Hurd rule below
 *-*-gnu*)	gdb_osabi=GDB_OSABI_HURD ;;
 *-*-mingw32ce*)	gdb_osabi=GDB_OSABI_WINCE ;;
-*-*-mingw* | *-*-cygwin*)
-		gdb_osabi=GDB_OSABI_CYGWIN ;;
+*-*-mingw*)	gdb_osabi=GDB_OSABI_WINDOWS ;;
+*-*-cygwin*)	gdb_osabi=GDB_OSABI_CYGWIN ;;
 *-*-dicos*)	gdb_osabi=GDB_OSABI_DICOS ;;
 *-*-symbianelf*)
 		gdb_osabi=GDB_OSABI_SYMBIAN ;;
diff --git a/gdb/i386-cygwin-tdep.c b/gdb/i386-cygwin-tdep.c
index cb66632648f7..b9a959d3c7d4 100644
--- a/gdb/i386-cygwin-tdep.c
+++ b/gdb/i386-cygwin-tdep.c
@@ -232,14 +232,22 @@ i386_cygwin_osabi_sniffer (bfd *abfd)
   const char *target_name = bfd_get_target (abfd);
 
   if (strcmp (target_name, "pei-i386") == 0)
-    return GDB_OSABI_CYGWIN;
+    return GDB_OSABI_WINDOWS;
+
+  return GDB_OSABI_UNKNOWN;
+}
+
+static enum gdb_osabi
+i386_cygwin_core_osabi_sniffer (bfd *abfd)
+{
+  const char *target_name = bfd_get_target (abfd);
 
   /* Cygwin uses elf core dumps.  Do not claim all ELF executables,
      check whether there is a .reg section of proper size.  */
   if (strcmp (target_name, "elf32-i386") == 0)
     {
       asection *section = bfd_get_section_by_name (abfd, ".reg");
-      if (section
+      if (section != nullptr
 	  && bfd_section_size (section) == I386_WINDOWS_SIZEOF_GREGSET)
 	return GDB_OSABI_CYGWIN;
     }
@@ -256,8 +264,11 @@ _initialize_i386_cygwin_tdep ()
 
   /* Cygwin uses elf core dumps.  */
   gdbarch_register_osabi_sniffer (bfd_arch_i386, bfd_target_elf_flavour,
-                                  i386_cygwin_osabi_sniffer);
+                                  i386_cygwin_core_osabi_sniffer);
 
+  /* The Cygwin and Windows OS ABIs are currently equivalent.  */
   gdbarch_register_osabi (bfd_arch_i386, 0, GDB_OSABI_CYGWIN,
                           i386_cygwin_init_abi);
+  gdbarch_register_osabi (bfd_arch_i386, 0, GDB_OSABI_WINDOWS,
+                          i386_cygwin_init_abi);
 }
diff --git a/gdb/osabi.c b/gdb/osabi.c
index b9a8687a7cc3..627b9d981515 100644
--- a/gdb/osabi.c
+++ b/gdb/osabi.c
@@ -72,6 +72,7 @@ static const struct osabi_names gdb_osabi_names[] =
   { "DJGPP", NULL },
   { "QNX-Neutrino", NULL },
   { "Cygwin", NULL },
+  { "Windows", NULL },
   { "AIX", NULL },
   { "DICOS", NULL },
   { "Darwin", NULL },
diff --git a/gdb/osabi.h b/gdb/osabi.h
index ff63db49affe..a7e6a10d0198 100644
--- a/gdb/osabi.h
+++ b/gdb/osabi.h
@@ -37,6 +37,7 @@ enum gdb_osabi
   GDB_OSABI_GO32,
   GDB_OSABI_QNXNTO,
   GDB_OSABI_CYGWIN,
+  GDB_OSABI_WINDOWS,
   GDB_OSABI_AIX,
   GDB_OSABI_DICOS,
   GDB_OSABI_DARWIN,
-- 
2.25.1

[PATCH 4/7] gdb: rename i386-cygwin-tdep.c to i386-windows-tdep.c

[Simon Marchi]

From: Simon Marchi <simon.marchi@efficios.com>

Since this file contains things that apply not only to Cygwin binaries,
but also to non-Cygwin Windows binaries, I think it would make more
sense for it to be called i386-windows-tdep.c.  It is analogous to
amd64-windows-tdep.c, which we already have.

gdb/ChangeLog:

	* i386-cygwin-tdep.c: Rename to...
	* i386-windows-tdep.c: ... this.
	* Makefile.in (ALL_TARGET_OBS): Rename i386-cygwin-tdep.c to
	i386-windows-tdep.c.
	* configure.tgt: Likewise.
---
 gdb/Makefile.in                                 | 4 ++--
 gdb/configure.tgt                               | 6 +++---
 gdb/{i386-cygwin-tdep.c => i386-windows-tdep.c} | 0
 3 files changed, 5 insertions(+), 5 deletions(-)
 rename gdb/{i386-cygwin-tdep.c => i386-windows-tdep.c} (100%)

diff --git a/gdb/Makefile.in b/gdb/Makefile.in
index 1db02c07ac28..d225b7d76679 100644
--- a/gdb/Makefile.in
+++ b/gdb/Makefile.in
@@ -733,7 +733,6 @@ ALL_TARGET_OBS = \
 	hppa-obsd-tdep.o \
 	hppa-tdep.o \
 	i386-bsd-tdep.o \
-	i386-cygwin-tdep.o \
 	i386-darwin-tdep.o \
 	i386-dicos-tdep.o \
 	i386-fbsd-tdep.o \
@@ -745,6 +744,7 @@ ALL_TARGET_OBS = \
 	i386-obsd-tdep.o \
 	i386-sol2-tdep.o \
 	i386-tdep.o \
+	i386-windows-tdep.o \
 	i387-tdep.o \
 	iq2000-tdep.o \
 	linux-record.o \
@@ -2161,7 +2161,6 @@ ALLDEPFILES = \
 	hppa-tdep.c \
 	i386-bsd-nat.c \
 	i386-bsd-tdep.c \
-	i386-cygwin-tdep.c \
 	i386-darwin-nat.c \
 	i386-darwin-tdep.c \
 	i386-dicos-tdep.c \
@@ -2178,6 +2177,7 @@ ALLDEPFILES = \
 	i386-sol2-nat.c \
 	i386-sol2-tdep.c \
 	i386-tdep.c \
+	i386-windows-tdep.c \
 	i387-tdep.c \
 	ia64-libunwind-tdep.c \
 	ia64-linux-nat.c \
diff --git a/gdb/configure.tgt b/gdb/configure.tgt
index 6ebd32410e99..34f703009eaa 100644
--- a/gdb/configure.tgt
+++ b/gdb/configure.tgt
@@ -304,11 +304,11 @@ i[34567]86-*-gnu*)
 	;;
 i[34567]86-*-cygwin*)
 	# Target: Intel 386 running win32
-	gdb_target_obs="i386-cygwin-tdep.o windows-tdep.o"
+	gdb_target_obs="i386-windows-tdep.o windows-tdep.o"
 	;;
 i[34567]86-*-mingw32*)
 	# Target: Intel 386 running win32
-	gdb_target_obs="i386-cygwin-tdep.o windows-tdep.o"
+	gdb_target_obs="i386-windows-tdep.o windows-tdep.o"
 	;;
 i[34567]86-*-go32* | i[34567]86-*-msdosdjgpp*)
 	# Target: i386 running DJGPP/go32.
@@ -730,7 +730,7 @@ x86_64-*-freebsd* | x86_64-*-kfreebsd*-gnu)
 x86_64-*-mingw* | x86_64-*-cygwin*)
         # Target: MingW/amd64
 	gdb_target_obs="amd64-windows-tdep.o \
-                        ${i386_tobjs} i386-cygwin-tdep.o \
+                        ${i386_tobjs} i386-windows-tdep.o \
                         windows-tdep.o"
         ;;
 x86_64-*-netbsd* | x86_64-*-knetbsd*-gnu)
diff --git a/gdb/i386-cygwin-tdep.c b/gdb/i386-windows-tdep.c
similarity index 100%
rename from gdb/i386-cygwin-tdep.c
rename to gdb/i386-windows-tdep.c
-- 
2.25.1

[PATCH 5/7] gdb: rename content of i386-windows-tdep.c, cygwin to windows

[Simon Marchi]

From: Simon Marchi <simon.marchi@efficios.com>

i386-cygwin-tdep.c has just been renamed to i386-windows-tdep.c, this
patch now renames everything in it that is not Cygwin-specific to
replace "cygwin" with "windows".

Note that I did not rename i386_cygwin_core_osabi_sniffer, since that
appears to be Cygwin-specific.

gdb/ChangeLog:

	* i386-windows-tdep.c: Mass-rename "cygwin" to "windows", except
	i386_cygwin_core_osabi_sniffer.
---
 gdb/i386-windows-tdep.c | 31 ++++++++++++++++---------------
 1 file changed, 16 insertions(+), 15 deletions(-)

diff --git a/gdb/i386-windows-tdep.c b/gdb/i386-windows-tdep.c
index b9a959d3c7d4..a71ceda781f8 100644
--- a/gdb/i386-windows-tdep.c
+++ b/gdb/i386-windows-tdep.c
@@ -1,4 +1,5 @@
-/* Target-dependent code for Cygwin running on i386's, for GDB.
+/* Target-dependent code for Windows (including Cygwin) running on i386's,
+   for GDB.
 
    Copyright (C) 2003-2020 Free Software Foundation, Inc.
 
@@ -188,25 +189,25 @@ i386_windows_core_pid_to_str (struct gdbarch *gdbarch, ptid_t ptid)
 }
 
 static CORE_ADDR
-i386_cygwin_skip_trampoline_code (struct frame_info *frame, CORE_ADDR pc)
+i386_windows_skip_trampoline_code (struct frame_info *frame, CORE_ADDR pc)
 {
   return i386_pe_skip_trampoline_code (frame, pc, NULL);
 }
 
 static const char *
-i386_cygwin_auto_wide_charset (void)
+i386_windows_auto_wide_charset (void)
 {
   return "UTF-16";
 }
 
 static void
-i386_cygwin_init_abi (struct gdbarch_info info, struct gdbarch *gdbarch)
+i386_windows_init_abi (struct gdbarch_info info, struct gdbarch *gdbarch)
 {
   struct gdbarch_tdep *tdep = gdbarch_tdep (gdbarch);
 
   windows_init_abi (info, gdbarch);
 
-  set_gdbarch_skip_trampoline_code (gdbarch, i386_cygwin_skip_trampoline_code);
+  set_gdbarch_skip_trampoline_code (gdbarch, i386_windows_skip_trampoline_code);
 
   set_gdbarch_skip_main_prologue (gdbarch, i386_skip_main_prologue);
 
@@ -223,11 +224,11 @@ i386_cygwin_init_abi (struct gdbarch_info info, struct gdbarch *gdbarch)
     (gdbarch, windows_core_xfer_shared_libraries);
   set_gdbarch_core_pid_to_str (gdbarch, i386_windows_core_pid_to_str);
 
-  set_gdbarch_auto_wide_charset (gdbarch, i386_cygwin_auto_wide_charset);
+  set_gdbarch_auto_wide_charset (gdbarch, i386_windows_auto_wide_charset);
 }
 
-static enum gdb_osabi
-i386_cygwin_osabi_sniffer (bfd *abfd)
+static gdb_osabi
+i386_windows_osabi_sniffer (bfd *abfd)
 {
   const char *target_name = bfd_get_target (abfd);
 
@@ -255,20 +256,20 @@ i386_cygwin_core_osabi_sniffer (bfd *abfd)
   return GDB_OSABI_UNKNOWN;
 }
 
-void _initialize_i386_cygwin_tdep ();
+void _initialize_i386_windows_tdep ();
 void
-_initialize_i386_cygwin_tdep ()
+_initialize_i386_windows_tdep ()
 {
   gdbarch_register_osabi_sniffer (bfd_arch_i386, bfd_target_coff_flavour,
-                                  i386_cygwin_osabi_sniffer);
+                                  i386_windows_osabi_sniffer);
 
   /* Cygwin uses elf core dumps.  */
   gdbarch_register_osabi_sniffer (bfd_arch_i386, bfd_target_elf_flavour,
                                   i386_cygwin_core_osabi_sniffer);
 
-  /* The Cygwin and Windows OS ABIs are currently equivalent.  */
-  gdbarch_register_osabi (bfd_arch_i386, 0, GDB_OSABI_CYGWIN,
-                          i386_cygwin_init_abi);
+  /* The Windows and Cygwin OS ABIs are currently equivalent.  */
   gdbarch_register_osabi (bfd_arch_i386, 0, GDB_OSABI_WINDOWS,
-                          i386_cygwin_init_abi);
+                          i386_windows_init_abi);
+  gdbarch_register_osabi (bfd_arch_i386, 0, GDB_OSABI_CYGWIN,
+                          i386_windows_init_abi);
 }
-- 
2.25.1

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

[Simon Marchi]

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;
+
+  /* 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 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)
+	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);
+    }
+
+    return false;
+}
+
 void _initialize_windows_tdep ();
 void
 _initialize_windows_tdep ()
diff --git a/gdb/windows-tdep.h b/gdb/windows-tdep.h
index 34474f259c2a..f2dc4260469d 100644
--- a/gdb/windows-tdep.h
+++ b/gdb/windows-tdep.h
@@ -33,4 +33,10 @@ extern void windows_xfer_shared_library (const char* so_name,
 
 extern void windows_init_abi (struct gdbarch_info info,
 			      struct gdbarch *gdbarch);
+
+/* Return true if the Portable Executable behind ABFD uses the Cygwin dll
+   (cygwin1.dll).  */
+
+extern bool is_linked_with_cygwin_dll (bfd *abfd);
+
 #endif
-- 
2.25.1

[PATCH 7/7] gdb: define builtin long type to be 64 bits on amd64 Cygwin

[Simon Marchi]

From: Simon Marchi <simon.marchi@efficios.com>

On Windows x86-64 (when building with MinGW), the size of the "long"
type is 32 bits.  amd64_windows_init_abi therefore does:

    set_gdbarch_long_bit (gdbarch, 32);

This is also used when the chosen OS ABI is Cygwin, where the "long"
type is 64 bits.  GDB therefore gets sizeof(long) wrong when using the
builtin long type:

    $ ./gdb -nx --data-directory=data-directory -batch -ex "set architecture i386:x86-64" -ex "set osabi Cygwin" -ex "print sizeof(long)"
    The target architecture is assumed to be i386:x86-64
    $1 = 4

This patch makes GDB avoid setting the size of the long type to 32 bits
when using the Cygwin OS ABI.  it will inherit the value set in
amd64_init_abi.

With this patch, I get:

    $ ./gdb -nx --data-directory=data-directory -batch -ex "set architecture i386:x86-64" -ex "set osabi Cygwin" -ex "print sizeof(long)"
    The target architecture is assumed to be i386:x86-64
    $1 = 8

gdb/ChangeLog:

	PR gdb/21500
	* amd64-windows-tdep.c (amd64_windows_init_abi): Rename
	to...
	(amd64_windows_init_abi_common): ... this.  Don't set size of
	long type.
	(amd64_windows_init_abi): New function.
	(amd64_cygwin_init_abi): New function.
	(_initialize_amd64_windows_tdep): Use amd64_cygwin_init_abi for
	the Cygwin OS ABI.
	* i386-windows-tdep.c (_initialize_i386_windows_tdep): Clarify
	comment.
---
 gdb/amd64-windows-tdep.c | 23 +++++++++++++++++------
 gdb/i386-windows-tdep.c  |  2 +-
 2 files changed, 18 insertions(+), 7 deletions(-)

diff --git a/gdb/amd64-windows-tdep.c b/gdb/amd64-windows-tdep.c
index e0346f8628fe..6d5076d1c437 100644
--- a/gdb/amd64-windows-tdep.c
+++ b/gdb/amd64-windows-tdep.c
@@ -1209,7 +1209,7 @@ amd64_windows_auto_wide_charset (void)
 }
 
 static void
-amd64_windows_init_abi (struct gdbarch_info info, struct gdbarch *gdbarch)
+amd64_windows_init_abi_common (gdbarch_info info, struct gdbarch *gdbarch)
 {
   /* The dwarf2 unwinder (appended very early by i386_gdbarch_init) is
      preferred over the SEH one.  The reasons are:
@@ -1229,9 +1229,6 @@ amd64_windows_init_abi (struct gdbarch_info info, struct gdbarch *gdbarch)
 
   windows_init_abi (info, gdbarch);
 
-  /* On Windows, "long"s are only 32bit.  */
-  set_gdbarch_long_bit (gdbarch, 32);
-
   /* Function calls.  */
   set_gdbarch_push_dummy_call (gdbarch, amd64_windows_push_dummy_call);
   set_gdbarch_return_value (gdbarch, amd64_windows_return_value);
@@ -1244,6 +1241,21 @@ amd64_windows_init_abi (struct gdbarch_info info, struct gdbarch *gdbarch)
   set_gdbarch_auto_wide_charset (gdbarch, amd64_windows_auto_wide_charset);
 }
 
+static void
+amd64_windows_init_abi (struct gdbarch_info info, struct gdbarch *gdbarch)
+{
+  amd64_windows_init_abi_common (info, gdbarch);
+
+  /* On Windows, "long"s are only 32bit.  */
+  set_gdbarch_long_bit (gdbarch, 32);
+}
+
+static void
+amd64_cygwin_init_abi (struct gdbarch_info info, struct gdbarch *gdbarch)
+{
+  amd64_windows_init_abi_common (info, gdbarch);
+}
+
 static gdb_osabi
 amd64_windows_osabi_sniffer (bfd *abfd)
 {
@@ -1262,11 +1274,10 @@ void _initialize_amd64_windows_tdep ();
 void
 _initialize_amd64_windows_tdep ()
 {
-  /* The Cygwin and Windows OS ABIs are currently equivalent.  */
   gdbarch_register_osabi (bfd_arch_i386, bfd_mach_x86_64, GDB_OSABI_WINDOWS,
                           amd64_windows_init_abi);
   gdbarch_register_osabi (bfd_arch_i386, bfd_mach_x86_64, GDB_OSABI_CYGWIN,
-                          amd64_windows_init_abi);
+                          amd64_cygwin_init_abi);
 
   gdbarch_register_osabi_sniffer (bfd_arch_i386, bfd_target_coff_flavour,
 				  amd64_windows_osabi_sniffer);
diff --git a/gdb/i386-windows-tdep.c b/gdb/i386-windows-tdep.c
index bd6107b02f1f..b26731c6bf28 100644
--- a/gdb/i386-windows-tdep.c
+++ b/gdb/i386-windows-tdep.c
@@ -270,7 +270,7 @@ _initialize_i386_windows_tdep ()
   gdbarch_register_osabi_sniffer (bfd_arch_i386, bfd_target_elf_flavour,
                                   i386_cygwin_core_osabi_sniffer);
 
-  /* The Windows and Cygwin OS ABIs are currently equivalent.  */
+  /* The Windows and Cygwin OS ABIs are currently equivalent on i386.  */
   gdbarch_register_osabi (bfd_arch_i386, 0, GDB_OSABI_WINDOWS,
                           i386_windows_init_abi);
   gdbarch_register_osabi (bfd_arch_i386, 0, GDB_OSABI_CYGWIN,
-- 
2.25.1

Re: [PATCH 0/7] Add "Windows" OS ABI

[Eli Zaretskii]

> From: Simon Marchi <simon.marchi@polymtl.ca>
> Cc: Eli Zaretskii <eliz@gnu.org>,
> 	Jon Turney <jon.turney@dronecode.org.uk>,
> 	Simon Marchi <simon.marchi@polymtl.ca>
> Date: Mon, 16 Mar 2020 13:08:38 -0400
> 
> This patchset started out as a single patch to have the OS ABI Cygwin
> applied to Windows x86-64 binaries, here:
> 
>     https://sourceware.org/legacy-ml/gdb-patches/2020-03/msg00195.html
> 
> with the follow-up here:
> 
>     https://sourceware.org/pipermail/gdb-patches/2020-March/000022.html
> 
> Eli pointed out that it doesn't make sense for binaries compilied with
> MinGW to have the Cygwin OS ABI, that there should be separate OS ABIs
> for Cygwin and non-Cygwin Windows binaries.  This already came up in the
> following bug report:
> 
>     https://sourceware.org/bugzilla/show_bug.cgi?id=21500#add_comment
> 
> This patchset does a bit of refactor in that area.  Most importantly, it:
> 
> - adds a "Windows" OS ABI
> - makes GDB recognize the proper OS ABI (Cygwin or Windows) when
>   loading executables
> - makes the builtin long type on Cygwin be 64 bits long

Thanks for working on this, the changes LGTM.

Re: [PATCH 0/7] Add "Windows" OS ABI

[Simon Marchi]

On 2020-03-16 1:46 p.m., Eli Zaretskii wrote:
>> From: Simon Marchi <simon.marchi@polymtl.ca>
>> Cc: Eli Zaretskii <eliz@gnu.org>,
>> 	Jon Turney <jon.turney@dronecode.org.uk>,
>> 	Simon Marchi <simon.marchi@polymtl.ca>
>> Date: Mon, 16 Mar 2020 13:08:38 -0400
>>
>> This patchset started out as a single patch to have the OS ABI Cygwin
>> applied to Windows x86-64 binaries, here:
>>
>>     https://sourceware.org/legacy-ml/gdb-patches/2020-03/msg00195.html
>>
>> with the follow-up here:
>>
>>     https://sourceware.org/pipermail/gdb-patches/2020-March/000022.html
>>
>> Eli pointed out that it doesn't make sense for binaries compilied with
>> MinGW to have the Cygwin OS ABI, that there should be separate OS ABIs
>> for Cygwin and non-Cygwin Windows binaries.  This already came up in the
>> following bug report:
>>
>>     https://sourceware.org/bugzilla/show_bug.cgi?id=21500#add_comment
>>
>> This patchset does a bit of refactor in that area.  Most importantly, it:
>>
>> - adds a "Windows" OS ABI
>> - makes GDB recognize the proper OS ABI (Cygwin or Windows) when
>>   loading executables
>> - makes the builtin long type on Cygwin be 64 bits long
> 
> Thanks for working on this, the changes LGTM.
> 

Thanks for taking a look, I'll give a bit more time for others (Jon might be
interested) to take a look if and comment.

Simon

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

[Christian Biesinger]

On Mon, Mar 16, 2020 at 12:09 PM Simon Marchi via Gdb-patches
<gdb-patches@sourceware.org> 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.  */

Remove the first of these two comments?

> +
> +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;
> +
> +  /* 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 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)
> +       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);
> +    }
> +
> +    return false;
> +}
> +
>  void _initialize_windows_tdep ();
>  void
>  _initialize_windows_tdep ()
> diff --git a/gdb/windows-tdep.h b/gdb/windows-tdep.h
> index 34474f259c2a..f2dc4260469d 100644
> --- a/gdb/windows-tdep.h
> +++ b/gdb/windows-tdep.h
> @@ -33,4 +33,10 @@ extern void windows_xfer_shared_library (const char* so_name,
>
>  extern void windows_init_abi (struct gdbarch_info info,
>                               struct gdbarch *gdbarch);
> +
> +/* Return true if the Portable Executable behind ABFD uses the Cygwin dll
> +   (cygwin1.dll).  */
> +
> +extern bool is_linked_with_cygwin_dll (bfd *abfd);
> +
>  #endif
> --
> 2.25.1
>

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

[Simon Marchi]

On 2020-03-16 2:16 p.m., Christian Biesinger wrote:
>> @@ -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.  */
> 
> Remove the first of these two comments?

Indeed, thanks.

Simon

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

[Jon Turney]

On 16/03/2020 17:08, Simon Marchi 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):
> 
[...]
> +
> +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;

I'm fine with this as-is, but FTR I think this only happens to work 
because binutils ld (which is probably the only way to currently build a 
cygwin executable) puts the import table in the .idata section.

The strictly correct way to locate the import table is to use the data 
directory (as pe_print_idata() does)

(See 
https://docs.microsoft.com/en-us/windows/win32/debug/pe-format#optional-header-data-directories-image-only)

(Notwithstanding the MS documentation you linked, I believe MS tools can 
put the import table in .rdata)

Re: [PATCH 0/7] Add "Windows" OS ABI

[Jon Turney]

On 16/03/2020 17:48, Simon Marchi wrote:
> On 2020-03-16 1:46 p.m., Eli Zaretskii wrote:
>>> From: Simon Marchi <simon.marchi@polymtl.ca>
>>> Cc: Eli Zaretskii <eliz@gnu.org>,
>>> 	Jon Turney <jon.turney@dronecode.org.uk>,
>>> 	Simon Marchi <simon.marchi@polymtl.ca>
>>> Date: Mon, 16 Mar 2020 13:08:38 -0400
>>>
>>> This patchset started out as a single patch to have the OS ABI Cygwin
>>> applied to Windows x86-64 binaries, here:
>>>
>>>      https://sourceware.org/legacy-ml/gdb-patches/2020-03/msg00195.html
>>>
>>> with the follow-up here:
>>>
>>>      https://sourceware.org/pipermail/gdb-patches/2020-March/000022.html
>>>
>>> Eli pointed out that it doesn't make sense for binaries compilied with
>>> MinGW to have the Cygwin OS ABI, that there should be separate OS ABIs
>>> for Cygwin and non-Cygwin Windows binaries.  This already came up in the
>>> following bug report:
>>>
>>>      https://sourceware.org/bugzilla/show_bug.cgi?id=21500#add_comment
>>>
>>> This patchset does a bit of refactor in that area.  Most importantly, it:
>>>
>>> - adds a "Windows" OS ABI
>>> - makes GDB recognize the proper OS ABI (Cygwin or Windows) when
>>>    loading executables
>>> - makes the builtin long type on Cygwin be 64 bits long
>>
>> Thanks for working on this, the changes LGTM.
>>
> 
> Thanks for taking a look, I'll give a bit more time for others (Jon might be
> interested) to take a look if and comment.

Patches look good, and appear work correctly in some brief testing.

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

[Simon Marchi]

On 2020-03-16 3:03 p.m., Jon Turney 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;
> 
> I'm fine with this as-is, but FTR I think this only happens to work 
> because binutils ld (which is probably the only way to currently build a 
> cygwin executable) puts the import table in the .idata section.
> 
> The strictly correct way to locate the import table is to use the data 
> directory (as pe_print_idata() does)
> 
> (See 
> https://docs.microsoft.com/en-us/windows/win32/debug/pe-format#optional-header-data-directories-image-only)
> 
> (Notwithstanding the MS documentation you linked, I believe MS tools can 
> put the import table in .rdata)
> 

After clarification with Jon on IRC, the current code is sufficient for the
moment.

I've pushed the series, thanks.

Simon

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

[Tom Tromey]

>>>>> "Simon" == Simon Marchi via Gdb-patches <gdb-patches@sourceware.org> writes:

Simon> +	  warning (_("\
Simon> +Failed to parse .idata section: name's virtual address (0x%" BFD_VMA_FMT "x) \
Simon> +is outside .idata section's range [0x%" BFD_VMA_FMT "x, 0x%" BFD_VMA_FMT "x[."),
Simon> +		   name_addr, idata_addr, idata_addr + idata_size);

Our internal test suite has started failing a test due to this output:

     warning: Failed to parse .idata section: name's virtual address (0x1) is outside .idata section's range [0x120500, 0x12bd64[.
     warning: Failed to parse .idata section: name's virtual address (0x1) is outside .idata section's range [0xea210, 0xefbb0[.
     warning: Failed to parse .idata section: name's virtual address (0x1) is outside .idata section's range [0x9b578, 0x9d042[.

I assume this is a bug in this code?  I don't really know.

Are there known problems here?

I can probably get you an executable if that would help.
("Probably" because I haven't tried to reproduce by hand, I've only seen
it in automation, and I'd have to do it by hand to get the exe.)

Tom

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

[Simon Marchi]

On 2020-04-01 3:05 p.m., Tom Tromey wrote:
>>>>>> "Simon" == Simon Marchi via Gdb-patches <gdb-patches@sourceware.org> writes:
> 
> Simon> +	  warning (_("\
> Simon> +Failed to parse .idata section: name's virtual address (0x%" BFD_VMA_FMT "x) \
> Simon> +is outside .idata section's range [0x%" BFD_VMA_FMT "x, 0x%" BFD_VMA_FMT "x[."),
> Simon> +		   name_addr, idata_addr, idata_addr + idata_size);
> 
> Our internal test suite has started failing a test due to this output:
> 
>      warning: Failed to parse .idata section: name's virtual address (0x1) is outside .idata section's range [0x120500, 0x12bd64[.
>      warning: Failed to parse .idata section: name's virtual address (0x1) is outside .idata section's range [0xea210, 0xefbb0[.
>      warning: Failed to parse .idata section: name's virtual address (0x1) is outside .idata section's range [0x9b578, 0x9d042[.
> 
> I assume this is a bug in this code?  I don't really know.
> 
> Are there known problems here?
> 
> I can probably get you an executable if that would help.
> ("Probably" because I haven't tried to reproduce by hand, I've only seen
> it in automation, and I'd have to do it by hand to get the exe.)
> 
> Tom
> 

Yes, I would appreciate if you could get me an executable, or steps to produce one.

Thanks,

Simon

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

[Pedro Alves]

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

Re: [PATCH 0/7] Add "Windows" OS ABI

[Pedro Alves]

On 3/16/20 5:08 PM, Simon Marchi via Gdb-patches wrote:
> This patchset started out as a single patch to have the OS ABI Cygwin
> applied to Windows x86-64 binaries, here:
> 
>     https://sourceware.org/legacy-ml/gdb-patches/2020-03/msg00195.html
> 
> with the follow-up here:
> 
>     https://sourceware.org/pipermail/gdb-patches/2020-March/000022.html
> 
> Eli pointed out that it doesn't make sense for binaries compilied with
> MinGW to have the Cygwin OS ABI, that there should be separate OS ABIs
> for Cygwin and non-Cygwin Windows binaries.  This already came up in the
> following bug report:
> 
>     https://sourceware.org/bugzilla/show_bug.cgi?id=21500#add_comment

Hurray.  Thanks for doing this.  Recently, another spot that could use
this was added in windows-tdep.c.  See:

  https://sourceware.org/legacy-ml/gdb-patches/2020-01/msg00048.html

Look for "bummer".

Thanks,
Pedro Alves

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

[Simon Marchi]

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

Re: [PATCH 0/7] Add "Windows" OS ABI

[Simon Marchi]

On 2020-04-01 5:42 p.m., Pedro Alves wrote:
> On 3/16/20 5:08 PM, Simon Marchi via Gdb-patches wrote:
>> This patchset started out as a single patch to have the OS ABI Cygwin
>> applied to Windows x86-64 binaries, here:
>>
>>     https://sourceware.org/legacy-ml/gdb-patches/2020-03/msg00195.html
>>
>> with the follow-up here:
>>
>>     https://sourceware.org/pipermail/gdb-patches/2020-March/000022.html
>>
>> Eli pointed out that it doesn't make sense for binaries compilied with
>> MinGW to have the Cygwin OS ABI, that there should be separate OS ABIs
>> for Cygwin and non-Cygwin Windows binaries.  This already came up in the
>> following bug report:
>>
>>     https://sourceware.org/bugzilla/show_bug.cgi?id=21500#add_comment
> 
> Hurray.  Thanks for doing this.  Recently, another spot that could use
> this was added in windows-tdep.c.  See:
> 
>   https://sourceware.org/legacy-ml/gdb-patches/2020-01/msg00048.html
> 
> Look for "bummer".

Ok, I'll see what I can do.

Simon

[PATCH] gdb: stop using host-dependent signal numbers in, windows-tdep.c (was: Re: [PATCH 0/7] Add "Windows" OS ABI)

[Simon Marchi]

On 2020-04-01 5:56 p.m., Simon Marchi via Gdb-patches wrote:
> On 2020-04-01 5:42 p.m., Pedro Alves wrote:
>> On 3/16/20 5:08 PM, Simon Marchi via Gdb-patches wrote:
>>> This patchset started out as a single patch to have the OS ABI Cygwin
>>> applied to Windows x86-64 binaries, here:
>>>
>>>     https://sourceware.org/legacy-ml/gdb-patches/2020-03/msg00195.html
>>>
>>> with the follow-up here:
>>>
>>>     https://sourceware.org/pipermail/gdb-patches/2020-March/000022.html
>>>
>>> Eli pointed out that it doesn't make sense for binaries compilied with
>>> MinGW to have the Cygwin OS ABI, that there should be separate OS ABIs
>>> for Cygwin and non-Cygwin Windows binaries.  This already came up in the
>>> following bug report:
>>>
>>>     https://sourceware.org/bugzilla/show_bug.cgi?id=21500#add_comment
>>
>> Hurray.  Thanks for doing this.  Recently, another spot that could use
>> this was added in windows-tdep.c.  See:
>>
>>   https://sourceware.org/legacy-ml/gdb-patches/2020-01/msg00048.html
>>
>> Look for "bummer".
> 
> Ok, I'll see what I can do.
> 
> Simon

Here's a patch that does that.  Note that I have only built-tested it.  I'm so
inefficient with Windows that if I wait until I have the time to actually try
it, I'll never get it done.  So I'm hoping that the Windows gurus can give it
a look.

Simon


From b08ef225a3fb1030e03ae53ed91fdc6f79550603 Mon Sep 17 00:00:00 2001
From: Simon Marchi <simon.marchi@polymtl.ca>
Date: Wed, 1 Apr 2020 22:01:54 -0400
Subject: [PATCH] gdb: stop using host-dependent signal numbers in
 windows-tdep.c

The signal enumeration in windows-tdep.c is defined differently whether
it is compiled on Cygwin or not.  This is problematic, since the code in
tdep files is not supposed to be influenced by the host platform (the
platform GDB itself runs on).

This makes a difference in windows_gdb_signal_to_target.  An obvious
example of clash is SIGABRT.  Let's pretend we are cross-debugging a
Cygwin process from a MinGW (non-Cygwin Windows) GDB.  If GDB needs to
translate the gdb signal number GDB_SIGNAL_ABRT into a target
equivalent, it would obtain the MinGW number (22), despite the target
being a Cygwin process.  Conversely, if debugging a MinGW process from a
Cygwin-hosted GDB, GDB_SIGNAL_ABRT would be converted to a Cygwin signal
number (6) despite the target being a MinGW process.  This is wrong,
since we want the result to depend on the target's platform, not GDB's
platform.

This known flaw was accepted because at the time we had a single OS ABI
(called Cygwin) for all Windows binaries (Cygwin ones and non-Cygwin
ones).  This limitation is now lifted, as we now have separate Windows
and Cygwin OS ABIs.  This means we are able to detect at runtime whether
the binary we are debugging is a Cygwin one or non-Cygwin one.

This patch splits the signal enum in two, one for the MinGW flavors and
one for Cygwin, removing all the ifdefs that made it depend on the host
platform.  It then makes two separate gdb_signal_to_target gdbarch
methods, that are used according to the OS ABI selected at runtime.

There is a bit of re-shuffling needed in how the gdbarch'es are
initialized, but nothing major.

gdb/ChangeLog:

	* windows-tdep.h (windows_init_abi): Add comment.
	(cygwin_init_abi): New declaration.
	* windows-tdep.c: Split signal enumeration in two, one for
	Windows and one for Cygwin.
	(windows_gdb_signal_to_target): Only deal with signal of the
	Windows OS ABI.
	(cygwin_gdb_signal_to_target): New function.
	(windows_init_abi): Rename to windows_init_abi_common, don't set
	gdb_signal_to_target gdbarch method.  Add new new function with
	this name.
	(cygwin_init_abi): New function.
	* amd64-windows-tdep.c (amd64_windows_init_abi_common): Add
	comment.  Don't call windows_init_abi.
	(amd64_windows_init_abi): Add comment, call windows_init_abi.
	(amd64_cygwin_init_abi): Add comment, call cygwin_init_abi.
	* i386-windows-tdep.c (i386_windows_init_abi): Rename to
	i386_windows_init_abi_common, don't call windows_init_abi.  Add
	a new function of this name.
	(i386_cygwin_init_abi): New function.
	(_initialize_i386_windows_tdep): Bind i386_cygwin_init_abi to
	OS ABI Cygwin.
---
 gdb/amd64-windows-tdep.c |  10 +-
 gdb/i386-windows-tdep.c  |  27 ++++-
 gdb/windows-tdep.c       | 214 ++++++++++++++++++++++++++-------------
 gdb/windows-tdep.h       |   9 ++
 4 files changed, 181 insertions(+), 79 deletions(-)

diff --git a/gdb/amd64-windows-tdep.c b/gdb/amd64-windows-tdep.c
index 6d5076d1c437..740152b7de8e 100644
--- a/gdb/amd64-windows-tdep.c
+++ b/gdb/amd64-windows-tdep.c
@@ -1208,6 +1208,8 @@ amd64_windows_auto_wide_charset (void)
   return "UTF-16";
 }

+/* Common parts for gdbarch initialization for Windows and Cygwin on AMD64.  */
+
 static void
 amd64_windows_init_abi_common (gdbarch_info info, struct gdbarch *gdbarch)
 {
@@ -1227,8 +1229,6 @@ amd64_windows_init_abi_common (gdbarch_info info, struct gdbarch *gdbarch)
   amd64_init_abi (info, gdbarch,
 		  amd64_target_description (X86_XSTATE_SSE_MASK, false));

-  windows_init_abi (info, gdbarch);
-
   /* Function calls.  */
   set_gdbarch_push_dummy_call (gdbarch, amd64_windows_push_dummy_call);
   set_gdbarch_return_value (gdbarch, amd64_windows_return_value);
@@ -1241,19 +1241,25 @@ amd64_windows_init_abi_common (gdbarch_info info, struct gdbarch *gdbarch)
   set_gdbarch_auto_wide_charset (gdbarch, amd64_windows_auto_wide_charset);
 }

+/* gdbarch initialization for Windows on AMD64.  */
+
 static void
 amd64_windows_init_abi (struct gdbarch_info info, struct gdbarch *gdbarch)
 {
   amd64_windows_init_abi_common (info, gdbarch);
+  windows_init_abi (info, gdbarch);

   /* On Windows, "long"s are only 32bit.  */
   set_gdbarch_long_bit (gdbarch, 32);
 }

+/* gdbarch initialization for Cygwin on AMD64.  */
+
 static void
 amd64_cygwin_init_abi (struct gdbarch_info info, struct gdbarch *gdbarch)
 {
   amd64_windows_init_abi_common (info, gdbarch);
+  cygwin_init_abi (info, gdbarch);
 }

 static gdb_osabi
diff --git a/gdb/i386-windows-tdep.c b/gdb/i386-windows-tdep.c
index b26731c6bf28..3a07c862f233 100644
--- a/gdb/i386-windows-tdep.c
+++ b/gdb/i386-windows-tdep.c
@@ -200,13 +200,13 @@ i386_windows_auto_wide_charset (void)
   return "UTF-16";
 }

+/* Common parts for gdbarch initialization for Windows and Cygwin on i386.  */
+
 static void
-i386_windows_init_abi (struct gdbarch_info info, struct gdbarch *gdbarch)
+i386_windows_init_abi_common (struct gdbarch_info info, struct gdbarch *gdbarch)
 {
   struct gdbarch_tdep *tdep = gdbarch_tdep (gdbarch);

-  windows_init_abi (info, gdbarch);
-
   set_gdbarch_skip_trampoline_code (gdbarch, i386_windows_skip_trampoline_code);

   set_gdbarch_skip_main_prologue (gdbarch, i386_skip_main_prologue);
@@ -227,6 +227,24 @@ i386_windows_init_abi (struct gdbarch_info info, struct gdbarch *gdbarch)
   set_gdbarch_auto_wide_charset (gdbarch, i386_windows_auto_wide_charset);
 }

+/* gdbarch initialization for Windows on i386.  */
+
+static void
+i386_windows_init_abi (struct gdbarch_info info, struct gdbarch *gdbarch)
+{
+  i386_windows_init_abi_common (info, gdbarch);
+  windows_init_abi (info, gdbarch);
+}
+
+/* gdbarch initialization for Cygwin on i386.  */
+
+static void
+i386_cygwin_init_abi (struct gdbarch_info info, struct gdbarch *gdbarch)
+{
+  i386_windows_init_abi_common (info, gdbarch);
+  cygwin_init_abi (info, gdbarch);
+}
+
 static gdb_osabi
 i386_windows_osabi_sniffer (bfd *abfd)
 {
@@ -270,9 +288,8 @@ _initialize_i386_windows_tdep ()
   gdbarch_register_osabi_sniffer (bfd_arch_i386, bfd_target_elf_flavour,
                                   i386_cygwin_core_osabi_sniffer);

-  /* The Windows and Cygwin OS ABIs are currently equivalent on i386.  */
   gdbarch_register_osabi (bfd_arch_i386, 0, GDB_OSABI_WINDOWS,
                           i386_windows_init_abi);
   gdbarch_register_osabi (bfd_arch_i386, 0, GDB_OSABI_CYGWIN,
-                          i386_windows_init_abi);
+			  i386_cygwin_init_abi);
 }
diff --git a/gdb/windows-tdep.c b/gdb/windows-tdep.c
index 0042ea350e80..9c5dfd183bfa 100644
--- a/gdb/windows-tdep.c
+++ b/gdb/windows-tdep.c
@@ -41,55 +41,69 @@
 #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
-   only by MinGW64 and Cygwin, not by mingw.org's MinGW.  FIXME: We
-   should really have distinct MinGW vs Cygwin OSABIs, and two
-   separate enums, selected at runtime.  */
+   those and Cygwin.  The below enumerations were gleaned from the
+   respective headers.  */
+
+/* Signal numbers for the various MinGW flavors.  The ones marked with
+   MinGW-w64 are defined by MinGW-w64, not by mingw.org's MinGW.  */

 enum
-  {
-   WINDOWS_SIGHUP = 1,	/* MinGW64/Cygwin */
-   WINDOWS_SIGINT = 2,
-   WINDOWS_SIGQUIT = 3,	/* MinGW64/Cygwin */
-   WINDOWS_SIGILL = 4,
-   WINDOWS_SIGTRAP = 5,	/* MinGW64/Cygwin */
-#ifdef __CYGWIN__
-   WINDOWS_SIGABRT = 6,
-#else
-   WINDOWS_SIGIOT = 6,	/* MinGW64 */
-#endif
-   WINDOWS_SIGEMT = 7,	/* MinGW64/Cygwin */
-   WINDOWS_SIGFPE = 8,
-   WINDOWS_SIGKILL = 9,	/* MinGW64/Cygwin */
-   WINDOWS_SIGBUS = 10,	/* MinGW64/Cygwin */
-   WINDOWS_SIGSEGV = 11,
-   WINDOWS_SIGSYS = 12,	/* MinGW64/Cygwin */
-   WINDOWS_SIGPIPE = 13,/* MinGW64/Cygwin */
-   WINDOWS_SIGALRM = 14,/* MinGW64/Cygwin */
-   WINDOWS_SIGTERM = 15,
-#ifdef __CYGWIN__
-   WINDOWS_SIGURG = 16,
-   WINDOWS_SIGSTOP = 17,
-   WINDOWS_SIGTSTP = 18,
-   WINDOWS_SIGCONT = 19,
-   WINDOWS_SIGCHLD = 20,
-   WINDOWS_SIGTTIN = 21,
-   WINDOWS_SIGTTOU = 22,
-   WINDOWS_SIGIO = 23,
-   WINDOWS_SIGXCPU = 24,
-   WINDOWS_SIGXFSZ = 25,
-   WINDOWS_SIGVTALRM = 26,
-   WINDOWS_SIGPROF = 27,
-   WINDOWS_SIGWINCH = 28,
-   WINDOWS_SIGLOST = 29,
-   WINDOWS_SIGUSR1 = 30,
-   WINDOWS_SIGUSR2 = 31
-#else
-   WINDOWS_SIGBREAK = 21,
-   WINDOWS_SIGABRT = 22
-#endif
-  };
+{
+  WINDOWS_SIGHUP = 1,	/* MinGW-w64 */
+  WINDOWS_SIGINT = 2,
+  WINDOWS_SIGQUIT = 3,	/* MinGW-w64 */
+  WINDOWS_SIGILL = 4,
+  WINDOWS_SIGTRAP = 5,	/* MinGW-w64 */
+  WINDOWS_SIGIOT = 6,	/* MinGW-w64 */
+  WINDOWS_SIGEMT = 7,	/* MinGW-w64 */
+  WINDOWS_SIGFPE = 8,
+  WINDOWS_SIGKILL = 9,	/* MinGW-w64 */
+  WINDOWS_SIGBUS = 10,	/* MinGW-w64 */
+  WINDOWS_SIGSEGV = 11,
+  WINDOWS_SIGSYS = 12,	/* MinGW-w64 */
+  WINDOWS_SIGPIPE = 13,	/* MinGW-w64 */
+  WINDOWS_SIGALRM = 14,	/* MinGW-w64 */
+  WINDOWS_SIGTERM = 15,
+  WINDOWS_SIGBREAK = 21,
+  WINDOWS_SIGABRT = 22,
+};
+
+/* Signal numbers for Cygwin.  */
+
+enum
+{
+  CYGWIN_SIGHUP = 1,
+  CYGWIN_SIGINT = 2,
+  CYGWIN_SIGQUIT = 3,
+  CYGWIN_SIGILL = 4,
+  CYGWIN_SIGTRAP = 5,
+  CYGWIN_SIGABRT = 6,
+  CYGWIN_SIGEMT = 7,
+  CYGWIN_SIGFPE = 8,
+  CYGWIN_SIGKILL = 9,
+  CYGWIN_SIGBUS = 10,
+  CYGWIN_SIGSEGV = 11,
+  CYGWIN_SIGSYS = 12,
+  CYGWIN_SIGPIPE = 13,
+  CYGWIN_SIGALRM = 14,
+  CYGWIN_SIGTERM = 15,
+  CYGWIN_SIGURG = 16,
+  CYGWIN_SIGSTOP = 17,
+  CYGWIN_SIGTSTP = 18,
+  CYGWIN_SIGCONT = 19,
+  CYGWIN_SIGCHLD = 20,
+  CYGWIN_SIGTTIN = 21,
+  CYGWIN_SIGTTOU = 22,
+  CYGWIN_SIGIO = 23,
+  CYGWIN_SIGXCPU = 24,
+  CYGWIN_SIGXFSZ = 25,
+  CYGWIN_SIGVTALRM = 26,
+  CYGWIN_SIGPROF = 27,
+  CYGWIN_SIGWINCH = 28,
+  CYGWIN_SIGLOST = 29,
+  CYGWIN_SIGUSR1 = 30,
+  CYGWIN_SIGUSR2 = 31,
+};

 struct cmd_list_element *info_w32_cmdlist;

@@ -607,7 +621,7 @@ init_w32_command_list (void)
     }
 }

-/* Implementation of `gdbarch_gdb_signal_to_target'.  */
+/* Implementation of `gdbarch_gdb_signal_to_target' for Windows.  */

 static int
 windows_gdb_signal_to_target (struct gdbarch *gdbarch, enum gdb_signal signal)
@@ -646,40 +660,81 @@ windows_gdb_signal_to_target (struct gdbarch *gdbarch, enum gdb_signal signal)
       return WINDOWS_SIGALRM;
     case GDB_SIGNAL_TERM:
       return WINDOWS_SIGTERM;
-#ifdef __CYGWIN__
+    }
+  return -1;
+}
+
+/* Implementation of `gdbarch_gdb_signal_to_target' for Cygwin.  */
+
+static int
+cygwin_gdb_signal_to_target (struct gdbarch *gdbarch, enum gdb_signal signal)
+{
+  switch (signal)
+    {
+    case GDB_SIGNAL_0:
+      return 0;
+    case GDB_SIGNAL_HUP:
+      return CYGWIN_SIGHUP;
+    case GDB_SIGNAL_INT:
+      return CYGWIN_SIGINT;
+    case GDB_SIGNAL_QUIT:
+      return CYGWIN_SIGQUIT;
+    case GDB_SIGNAL_ILL:
+      return CYGWIN_SIGILL;
+    case GDB_SIGNAL_TRAP:
+      return CYGWIN_SIGTRAP;
+    case GDB_SIGNAL_ABRT:
+      return CYGWIN_SIGABRT;
+    case GDB_SIGNAL_EMT:
+      return CYGWIN_SIGEMT;
+    case GDB_SIGNAL_FPE:
+      return CYGWIN_SIGFPE;
+    case GDB_SIGNAL_KILL:
+      return CYGWIN_SIGKILL;
+    case GDB_SIGNAL_BUS:
+      return CYGWIN_SIGBUS;
+    case GDB_SIGNAL_SEGV:
+      return CYGWIN_SIGSEGV;
+    case GDB_SIGNAL_SYS:
+      return CYGWIN_SIGSYS;
+    case GDB_SIGNAL_PIPE:
+      return CYGWIN_SIGPIPE;
+    case GDB_SIGNAL_ALRM:
+      return CYGWIN_SIGALRM;
+    case GDB_SIGNAL_TERM:
+      return CYGWIN_SIGTERM;
     case GDB_SIGNAL_URG:
-      return WINDOWS_SIGURG;
+      return CYGWIN_SIGURG;
     case GDB_SIGNAL_STOP:
-      return WINDOWS_SIGSTOP;
+      return CYGWIN_SIGSTOP;
     case GDB_SIGNAL_TSTP:
-      return WINDOWS_SIGTSTP;
+      return CYGWIN_SIGTSTP;
     case GDB_SIGNAL_CONT:
-      return WINDOWS_SIGCONT;
+      return CYGWIN_SIGCONT;
     case GDB_SIGNAL_CHLD:
-      return WINDOWS_SIGCHLD;
+      return CYGWIN_SIGCHLD;
     case GDB_SIGNAL_TTIN:
-      return WINDOWS_SIGTTIN;
+      return CYGWIN_SIGTTIN;
     case GDB_SIGNAL_TTOU:
-      return WINDOWS_SIGTTOU;
+      return CYGWIN_SIGTTOU;
     case GDB_SIGNAL_IO:
-      return WINDOWS_SIGIO;
+      return CYGWIN_SIGIO;
     case GDB_SIGNAL_XCPU:
-      return WINDOWS_SIGXCPU;
+      return CYGWIN_SIGXCPU;
     case GDB_SIGNAL_XFSZ:
-      return WINDOWS_SIGXFSZ;
+      return CYGWIN_SIGXFSZ;
     case GDB_SIGNAL_VTALRM:
-      return WINDOWS_SIGVTALRM;
+      return CYGWIN_SIGVTALRM;
     case GDB_SIGNAL_PROF:
-      return WINDOWS_SIGPROF;
+      return CYGWIN_SIGPROF;
     case GDB_SIGNAL_WINCH:
-      return WINDOWS_SIGWINCH;
+      return CYGWIN_SIGWINCH;
     case GDB_SIGNAL_PWR:
-      return WINDOWS_SIGLOST;
+      return CYGWIN_SIGLOST;
     case GDB_SIGNAL_USR1:
-      return WINDOWS_SIGUSR1;
+      return CYGWIN_SIGUSR1;
     case GDB_SIGNAL_USR2:
-      return WINDOWS_SIGUSR2;
-#endif	/* __CYGWIN__ */
+      return CYGWIN_SIGUSR2;
     }
   return -1;
 }
@@ -865,11 +920,11 @@ windows_solib_create_inferior_hook (int from_tty)

 static struct target_so_ops windows_so_ops;

-/* To be called from the various GDB_OSABI_CYGWIN handlers for the
-   various Windows architectures and machine types.  */
+/* Common parts for gdbarch initialization for the Windows and Cygwin OS
+   ABIs.  */

-void
-windows_init_abi (struct gdbarch_info info, struct gdbarch *gdbarch)
+static void
+windows_init_abi_common (struct gdbarch_info info, struct gdbarch *gdbarch)
 {
   set_gdbarch_wchar_bit (gdbarch, 16);
   set_gdbarch_wchar_signed (gdbarch, 0);
@@ -881,8 +936,6 @@ windows_init_abi (struct gdbarch_info info, struct gdbarch *gdbarch)
   set_gdbarch_iterate_over_objfiles_in_search_order
     (gdbarch, windows_iterate_over_objfiles_in_search_order);

-  set_gdbarch_gdb_signal_to_target (gdbarch, windows_gdb_signal_to_target);
-
   windows_so_ops = solib_target_so_ops;
   windows_so_ops.solib_create_inferior_hook
     = windows_solib_create_inferior_hook;
@@ -891,6 +944,23 @@ windows_init_abi (struct gdbarch_info info, struct gdbarch *gdbarch)
   set_gdbarch_get_siginfo_type (gdbarch, windows_get_siginfo_type);
 }

+/* See windows-tdep.h.  */
+void
+windows_init_abi (struct gdbarch_info info, struct gdbarch *gdbarch)
+{
+  windows_init_abi_common (info, gdbarch);
+  set_gdbarch_gdb_signal_to_target (gdbarch, windows_gdb_signal_to_target);
+}
+
+/* See windows-tdep.h.  */
+
+void
+cygwin_init_abi (struct gdbarch_info info, struct gdbarch *gdbarch)
+{
+  windows_init_abi_common (info, gdbarch);
+  set_gdbarch_gdb_signal_to_target (gdbarch, cygwin_gdb_signal_to_target);
+}
+
 /* Implementation of `tlb' variable.  */

 static const struct internalvar_funcs tlb_funcs =
diff --git a/gdb/windows-tdep.h b/gdb/windows-tdep.h
index f2dc4260469d..cd7717bd9174 100644
--- a/gdb/windows-tdep.h
+++ b/gdb/windows-tdep.h
@@ -31,9 +31,18 @@ extern void windows_xfer_shared_library (const char* so_name,
 					 struct gdbarch *gdbarch,
 					 struct obstack *obstack);

+/* To be called from the various GDB_OSABI_WINDOWS handlers for the
+   various Windows architectures and machine types.  */
+
 extern void windows_init_abi (struct gdbarch_info info,
 			      struct gdbarch *gdbarch);

+/* To be called from the various GDB_OSABI_CYGWIN handlers for the
+   various Windows architectures and machine types.  */
+
+extern void cygwin_init_abi (struct gdbarch_info info,
+			     struct gdbarch *gdbarch);
+
 /* Return true if the Portable Executable behind ABFD uses the Cygwin dll
    (cygwin1.dll).  */

-- 
2.26.0

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

[Tom Tromey]

While reading Pedro's note I noticed:

>> +  const gdb_byte *const idata_contents
>> +    = gdb_bfd_map_section (idata_section, &idata_size);

There's no way to ever release this data.  Since it's only used once, it
may be better to use bfd_get_full_section_contents, so the memory can be
freed when done.

Tom

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

[Pedro Alves]

On 4/1/20 10:53 PM, Simon Marchi via Gdb-patches wrote:
> 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

Ah, good.  Sounds like the data structures were designed for
memory mapping.  Which isn't that surprising.

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

I think so.  If bfd heap allocates, then memory will be aligned to the word size at
least.  If bfd mmaps, then memory will be aligned to page size.  And then if the
data structures are aligned, we should be good.

Thanks,
Pedro Alves

Re: [PATCH] gdb: stop using host-dependent signal numbers in, windows-tdep.c

[Pedro Alves]

On 4/2/20 4:06 AM, Simon Marchi wrote:
> On 2020-04-01 5:56 p.m., Simon Marchi via Gdb-patches wrote:

>> Ok, I'll see what I can do.
> 
> Here's a patch that does that.  Note that I have only built-tested it.  I'm so
> inefficient with Windows that if I wait until I have the time to actually try
> it, I'll never get it done.  So I'm hoping that the Windows gurus can give it
> a look.

Wow, thanks for doing this.  Most excellent.

It looks good to me.

Thanks,
Pedro Alves

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

[Simon Marchi]

On 2020-04-02 9:56 a.m., Pedro Alves wrote:
> On 4/1/20 10:53 PM, Simon Marchi via Gdb-patches wrote:
>> However, when BFD maps the file/section data in memory for GDB to read, is that mapping
>> guaranteed to be sufficiently aligned as well?
> 
> I think so.  If bfd heap allocates, then memory will be aligned to the word size at
> least.  If bfd mmaps, then memory will be aligned to page size.  And then if the
> data structures are aligned, we should be good.

Following this message from Tom [1], I'm going to change the code to use
bfd_get_full_section_contents and store the data into a pre-allocated gdb::byte_vector.
Does that change anything?

[1] https://marc.info/?l=gdb-patches&m=158583388704648&w=2

Simon

Re: [PATCH] gdb: stop using host-dependent signal numbers in, windows-tdep.c

[Simon Marchi]

On 2020-04-02 10:00 a.m., Pedro Alves wrote:
> On 4/2/20 4:06 AM, Simon Marchi wrote:
>> On 2020-04-01 5:56 p.m., Simon Marchi via Gdb-patches wrote:
> 
>>> Ok, I'll see what I can do.
>>
>> Here's a patch that does that.  Note that I have only built-tested it.  I'm so
>> inefficient with Windows that if I wait until I have the time to actually try
>> it, I'll never get it done.  So I'm hoping that the Windows gurus can give it
>> a look.
> 
> Wow, thanks for doing this.  Most excellent.
> 
> It looks good to me.
> 
> Thanks,
> Pedro Alves

Thanks.  I'll give some time to Jon and/or Eli to take a look before pushing.

Simon

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

[Pedro Alves]

On 4/2/20 3:01 PM, Simon Marchi wrote:
> On 2020-04-02 9:56 a.m., Pedro Alves wrote:
>> On 4/1/20 10:53 PM, Simon Marchi via Gdb-patches wrote:
>>> However, when BFD maps the file/section data in memory for GDB to read, is that mapping
>>> guaranteed to be sufficiently aligned as well?
>>
>> I think so.  If bfd heap allocates, then memory will be aligned to the word size at
>> least.  If bfd mmaps, then memory will be aligned to page size.  And then if the
>> data structures are aligned, we should be good.
> 
> Following this message from Tom [1], I'm going to change the code to use
> bfd_get_full_section_contents and store the data into a pre-allocated gdb::byte_vector.
> Does that change anything?

I don't think it does.

Thanks,
Pedro Alves

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

[Simon Marchi]

On 2020-04-02 10:03 a.m., Pedro Alves wrote:
> On 4/2/20 3:01 PM, Simon Marchi wrote:
>> On 2020-04-02 9:56 a.m., Pedro Alves wrote:
>>> On 4/1/20 10:53 PM, Simon Marchi via Gdb-patches wrote:
>>>> However, when BFD maps the file/section data in memory for GDB to read, is that mapping
>>>> guaranteed to be sufficiently aligned as well?
>>>
>>> I think so.  If bfd heap allocates, then memory will be aligned to the word size at
>>> least.  If bfd mmaps, then memory will be aligned to page size.  And then if the
>>> data structures are aligned, we should be good.
>>
>> Following this message from Tom [1], I'm going to change the code to use
>> bfd_get_full_section_contents and store the data into a pre-allocated gdb::byte_vector.
>> Does that change anything?
> 
> I don't think it does.

Err, just to make a correction: bfd_get_full_section_contents allocates the memory itself
and returns a pointer, so I won't use gdb::byte_vector.  Since BFD does the allocation
itself, then we can say it's responsible for returning a properly aligned buffer.

Simon

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

[Simon Marchi]

On 2020-04-02 10:08 a.m., Simon Marchi wrote:
> On 2020-04-02 10:03 a.m., Pedro Alves wrote:
>> On 4/2/20 3:01 PM, Simon Marchi wrote:
>>> On 2020-04-02 9:56 a.m., Pedro Alves wrote:
>>>> On 4/1/20 10:53 PM, Simon Marchi via Gdb-patches wrote:
>>>>> However, when BFD maps the file/section data in memory for GDB to read, is that mapping
>>>>> guaranteed to be sufficiently aligned as well?
>>>>
>>>> I think so.  If bfd heap allocates, then memory will be aligned to the word size at
>>>> least.  If bfd mmaps, then memory will be aligned to page size.  And then if the
>>>> data structures are aligned, we should be good.
>>>
>>> Following this message from Tom [1], I'm going to change the code to use
>>> bfd_get_full_section_contents and store the data into a pre-allocated gdb::byte_vector.
>>> Does that change anything?
>>
>> I don't think it does.
> 
> Err, just to make a correction: bfd_get_full_section_contents allocates the memory itself
> and returns a pointer, so I won't use gdb::byte_vector.  Since BFD does the allocation
> itself, then we can say it's responsible for returning a properly aligned buffer.

Scratch that again.  I now realized that bfd_get_section_contents uses a user-provided buffer,
unlike bfd_get_full_section_contents.  I am not sure yet what I'm going to use.

Simon

[PATCH] gdb: use bfd_get_section_contents to read section contents in, is_linked_with_cygwin_dll (was: Re: [PATCH 6/7] gdb: select "Cygwin" OS ABI for Cygwin binaries)

[Simon Marchi]

On 2020-04-02 9:22 a.m., Tom Tromey wrote:
> There's no way to ever release this data.  Since it's only used once, it
> may be better to use bfd_get_full_section_contents, so the memory can be
> freed when done.

Good point, I hadn't thought of that.

Here's a patch.  I only tested it on GNU/Linux, making sure GDB is still able
to recognize a mingw binary and a cygwin binary.


From b7f4f941018c7b4a7431837ccf5eee5c992f4111 Mon Sep 17 00:00:00 2001
From: Simon Marchi <simon.marchi@polymtl.ca>
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_section_contents,
and this is what this patch does.

I decided to make a small bfd_get_section_contents wrapper in gdb_bfd.c,
which returns the contents in a gdb::byte_vector.  The wrapper also
makes it easy to get the full section contents into a gdb::byte_vector.
Note that BFD provides the bfd_get_full_section_contents function, but
this one returns a newly-allocated buffer.  gdb_bfd_get_section_contents
offers a consistent interface, regardless of whether you need to read
part of a section or the full section.

gdb_bfd_get_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 and
	gdbsupport/gdb_optional.h.
	(BFD_SIZE_TYPE_MAX): New macro.
	(gdb_bfd_get_section_contents): New declaration.
	* gdb_bfd.c (gdb_bfd_get_section_contents): New function.
	* windows-tdep.c (is_linked_with_cygwin_dll): Use
	gdb_bfd_get_section_contents.
---
 gdb/gdb_bfd.c      | 25 ++++++++++++++++++++++++-
 gdb/gdb_bfd.h      | 15 +++++++++++++++
 gdb/windows-tdep.c | 13 ++++++-------
 3 files changed, 45 insertions(+), 8 deletions(-)

diff --git a/gdb/gdb_bfd.c b/gdb/gdb_bfd.c
index 5a6dee2d51a8..2b34e0f3e17c 100644
--- a/gdb/gdb_bfd.c
+++ b/gdb/gdb_bfd.c
@@ -926,7 +926,30 @@ gdb_bfd_requires_relocations (bfd *abfd)
   return gdata->needs_relocations;
 }

-\f
+/* See gdb_bfd.h.  */
+
+bool
+gdb_bfd_get_section_contents (bfd *abfd, asection *section,
+			      gdb::byte_vector *contents, file_ptr offset,
+			      bfd_size_type count)
+{
+  bfd_size_type section_size = bfd_section_size (section);
+
+  /* Allow `offset == section_size`, to allow the corner case of
+     `offset == section_size`, `count = 0`.  */
+  gdb_assert (offset <= section_size);
+
+  /* If COUNT is unspecified, get the contents from OFFSET until the end of the
+     section.  */
+  if (count == BFD_SIZE_TYPE_MAX)
+    count = section_size - offset;
+
+  gdb_assert ((offset + count) <= section_size);
+
+  contents->resize (count);
+
+  return bfd_get_section_contents (abfd, section, contents->data (), offset, count);
+}

 /* A callback for htab_traverse that prints a single BFD.  */

diff --git a/gdb/gdb_bfd.h b/gdb/gdb_bfd.h
index 9b1e292bf18f..ffea4f6b715e 100644
--- a/gdb/gdb_bfd.h
+++ b/gdb/gdb_bfd.h
@@ -21,6 +21,8 @@
 #define GDB_BFD_H

 #include "registry.h"
+#include "gdbsupport/byte-vector.h"
+#include "gdbsupport/gdb_optional.h"
 #include "gdbsupport/gdb_ref_ptr.h"

 DECLARE_REGISTRY (bfd);
@@ -30,6 +32,8 @@ DECLARE_REGISTRY (bfd);

 #define TARGET_SYSROOT_PREFIX "target:"

+#define BFD_SIZE_TYPE_MAX ((bfd_size_type) -1)
+
 /* Returns nonzero if NAME starts with TARGET_SYSROOT_PREFIX, zero
    otherwise.  */

@@ -181,4 +185,15 @@ int gdb_bfd_count_sections (bfd *abfd);

 int gdb_bfd_requires_relocations (bfd *abfd);

+/* Wrapper around bfd_get_section_contents, returning the requested section
+   contents in *CONTENTS.  Return true on success, false otherwise.
+
+   If COUNT is not specified, read from OFFSET until the end of the
+   section.  */
+
+bool
+gdb_bfd_get_section_contents (bfd *abfd, asection *section,
+			      gdb::byte_vector *contents, file_ptr offset = 0,
+			      bfd_size_type count = BFD_SIZE_TYPE_MAX);
+
 #endif /* GDB_BFD_H */
diff --git a/gdb/windows-tdep.c b/gdb/windows-tdep.c
index 9c5dfd183bfa..9b7dc1d12b26 100644
--- a/gdb/windows-tdep.c
+++ b/gdb/windows-tdep.c
@@ -1005,18 +1005,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_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

Re: [PATCH] gdb: use bfd_get_section_contents to read section contents in, is_linked_with_cygwin_dll

[Simon Marchi]

On 2020-04-02 10:55 a.m., Simon Marchi via Gdb-patches wrote:
> diff --git a/gdb/windows-tdep.c b/gdb/windows-tdep.c
> index 9c5dfd183bfa..9b7dc1d12b26 100644
> --- a/gdb/windows-tdep.c
> +++ b/gdb/windows-tdep.c
> @@ -1005,18 +1005,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_section_contents(abfd, idata_section, &idata_contents))

Missing space here, consider it fixed.

Simon

Re: [PATCH] gdb: stop using host-dependent signal numbers in, windows-tdep.c

[Eli Zaretskii]

> From: Simon Marchi <simark@simark.ca>
> Date: Thu, 2 Apr 2020 10:02:48 -0400
> Cc: Jon Turney <jon.turney@dronecode.org.uk>
> 
> > Wow, thanks for doing this.  Most excellent.
> > 
> > It looks good to me.
> > 
> > Thanks,
> > Pedro Alves
> 
> Thanks.  I'll give some time to Jon and/or Eli to take a look before pushing.

LGTM, thanks.

Re: [PATCH] gdb: use bfd_get_section_contents to read section contents in, is_linked_with_cygwin_dll

[Tom Tromey]

>>>>> "Simon" == Simon Marchi <simon.marchi@polymtl.ca> 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

Re: [PATCH] gdb: use bfd_get_section_contents to read section contents in, is_linked_with_cygwin_dll

[Simon Marchi]

On 2020-04-02 3:01 p.m., Tom Tromey wrote:
>>>>>> "Simon" == Simon Marchi <simon.marchi@polymtl.ca> 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 <simon.marchi@polymtl.ca>
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;
 }

-\f
+/* 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

Re: [PATCH] gdb: use bfd_get_section_contents to read section contents in, is_linked_with_cygwin_dll

[Tom Tromey]

>>>>> "Simon" == Simon Marchi via Gdb-patches <gdb-patches@sourceware.org> writes:

Simon> +  return bfd_get_section_contents (abfd, section, contents->data (), offset, count);
>> 
>> This line looks too long.

It's still too long in this version.

Simon> Now that I re-read it, I don't think the function should take the extra OFFSET and
Simon> COUNT parameters, since they are not used anywhere.  I went around our calls to
Simon> bfd_get_section_contents, and I don't think they would be very useful.  And I would
Simon> prefer not to check in code if it's not going to be exercised.

Simon> So I'd go with this simpler version instead.  The extra complexity can be added later,
Simon> if needed.

Sounds reasonable to me.

Tom

Re: [PATCH] gdb: use bfd_get_section_contents to read section contents in, is_linked_with_cygwin_dll

[Simon Marchi]

On 2020-04-02 3:45 p.m., Tom Tromey wrote:
>>>>>> "Simon" == Simon Marchi via Gdb-patches <gdb-patches@sourceware.org> writes:
> 
> Simon> +  return bfd_get_section_contents (abfd, section, contents->data (), offset, count);
>>>
>>> This line looks too long.
> 
> It's still too long in this version.

Sorry, I missed that comment earlier.  Fixed now.

> Simon> Now that I re-read it, I don't think the function should take the extra OFFSET and
> Simon> COUNT parameters, since they are not used anywhere.  I went around our calls to
> Simon> bfd_get_section_contents, and I don't think they would be very useful.  And I would
> Simon> prefer not to check in code if it's not going to be exercised.
> 
> Simon> So I'd go with this simpler version instead.  The extra complexity can be added later,
> Simon> if needed.
> 
> Sounds reasonable to me.

Thanks, I'm pushing it with the above issue fixed.

Simon

Re: [PATCH] gdb: stop using host-dependent signal numbers in, windows-tdep.c

[Jon Turney]

On 02/04/2020 15:02, Simon Marchi wrote:
> On 2020-04-02 10:00 a.m., Pedro Alves wrote:
>> On 4/2/20 4:06 AM, Simon Marchi wrote:
>>> On 2020-04-01 5:56 p.m., Simon Marchi via Gdb-patches wrote:
>>
>>>> Ok, I'll see what I can do.
>>>
>>> Here's a patch that does that.  Note that I have only built-tested it.  I'm so
>>> inefficient with Windows that if I wait until I have the time to actually try
>>> it, I'll never get it done.  So I'm hoping that the Windows gurus can give it
>>> a look.
>>
>> Wow, thanks for doing this.  Most excellent.
>>
>> It looks good to me.
>>
>> Thanks,
>> Pedro Alves
> 
> Thanks.  I'll give some time to Jon and/or Eli to take a look before pushing.

I don't see any problems with this.

Re: [PATCH] gdb: stop using host-dependent signal numbers in, windows-tdep.c

[Simon Marchi]

On 2020-04-08 8:45 a.m., Jon Turney wrote:
> On 02/04/2020 15:02, Simon Marchi wrote:
>> On 2020-04-02 10:00 a.m., Pedro Alves wrote:
>>> On 4/2/20 4:06 AM, Simon Marchi wrote:
>>>> On 2020-04-01 5:56 p.m., Simon Marchi via Gdb-patches wrote:
>>>
>>>>> Ok, I'll see what I can do.
>>>>
>>>> Here's a patch that does that.  Note that I have only built-tested it.  I'm so
>>>> inefficient with Windows that if I wait until I have the time to actually try
>>>> it, I'll never get it done.  So I'm hoping that the Windows gurus can give it
>>>> a look.
>>>
>>> Wow, thanks for doing this.  Most excellent.
>>>
>>> It looks good to me.
>>>
>>> Thanks,
>>> Pedro Alves
>>
>> Thanks.  I'll give some time to Jon and/or Eli to take a look before pushing.
> 
> I don't see any problems with this.
> 

Thanks, I pushed it.

Simon