Re: [PATCH 5/5] gdbserver: pass osabi to GDB in target description

[Luis Machado <luis.machado@arm.com>]

On 10/8/24 18:11, Andrew Burgess wrote:
> On a Windows machine I built gdbserver, configured for the target
> 'x86_64-w64-mingw32', then on a GNU/Linux machine I built GDB with
> support for all target (--enable-targets=all).
> 
> On the Windows machine I start gdbserver with a small test binary:
> 
>   $ gdbserver 192.168.129.25:54321 C:\some\directory\executable.exe
> 
> On the GNU/Linux machine I start GDB without the test binary, and
> connect to gdbserver.
> 
> As I have not given GDB the test binary, my expectation is that GDB
> would connect to gdbserver and then download the file over the remote
> protocol, but instead I was presented with this message:
> 
>   (gdb) target remote 192.168.129.25:54321
>   Remote debugging using 192.168.129.25:54321
>   warning: C:\some\directory\executable.exe: No such file or directory.
>   0x00007ffa3e1e1741 in ?? ()
>   (gdb)
> 
> What I found is that if I told GDB where to find the binary, like
> this:
> 
>   (gdb) file target:C:/some/directory/executable.exe
>   A program is being debugged already.
>   Are you sure you want to change the file? (y or n) y
>   Reading C:/some/directory/executable.exe from remote target...
>   warning: File transfers from remote targets can be slow. Use "set sysroot" to access files locally instead.
>   Reading C:/some/directory/executable.exe from remote target...
>   Reading symbols from target:C:/some/directory/executable.exe...
>   (gdb)
> 
> then GDB would download the executable.
> 
> I eventually tracked the problem down to exec_file_find (solib.c).
> The remote target was passing an absolute Windows filename (beginning
> with "C:/" in this case), but in exec_file_find GDB was failing the
> IS_TARGET_ABSOLUTE_PATH call, and so was treating the filename as
> relative.
> 
> The IS_TARGET_ABSOLUTE_PATH call was failing because GDB thought that
> the file system kind was "unix", and as the filename didn't start with
> a "/" it assumed the filename was not absolute.
> 
> But I'm connecting to a Windows target, my 'target-file-system-kind'
> was set to "auto", so should be figuring out that my file-system is
> "dos-based".
> 
> Looking in effective_target_file_system_kind (filesystem.c), we find
> that the logic of "auto" is delegated to the current gdbarch.  However
> in windows-tdep.c we see:
> 
>   set_gdbarch_has_dos_based_file_system (gdbarch, 1);
> 
> So if we are using a Windows gdbarch we should have "dos-based"
> filesystems.  What this means is that after connecting to the remote
> target GDB has selected the wrong gdbarch.
> 
> What's happening is that the target description sent back by the
> remote target only includes the x86-64 registers.  There's no
> information about which OS we're on.  As a consequence, GDB picks the
> first x86-64 gdbarch which can handle the provided register set, which
> happens to be a GNU/Linux gdbarch.
> 
> And indeed, there doesn't appear to be anywhere in gdbserver that sets
> the osabi on the target descriptions, though some target descriptions
> do have their osabi set when the description is created, e.g. in:
> 
>   gdb/arch/amd64.c	- Sets GNU/Linux osabi when appropriate.
>   gdb/arch/i386.c	- Likewise.
>   gdb/arch/tic6x.c	- Always set GNU/Linux osabi.
> 
> Most target descriptions are created without an osabi, gdbserver does
> nothing to fix this, and the description is returned to GDB without an
> osabi included.
> 
> I propose that we always set the osabi name on the target descriptions
> returned from gdbserver.  We could try to do this when the description
> is first created, but that would mean passing extra flags into the
> tdesc creation code (or just passing the osabi string in), and I don't
> think that's really necessary.  If we consider the tdesc creation as
> being about figuring out which registers are on the target, then it
> makes sense that the osabi information is injected later.
> 
> So what I've done is require the osabi name to be passed to the
> init_target_desc function.  This is called, I believe, for all
> targets, in the gdbserver code.
> 
> Now when I connect to the Windows remote the target description
> returned includes the osabi name.  With this extra information GDB
> selects the correct gdbarch object, which means that GDB understands
> the target has a "dos-based" file-system.  With that correct GDB
> understands that the filename it was given is absolute, and so fetches
> the file from the remote as we'd like.
> ---
>  gdbserver/linux-aarch32-tdesc.cc |  2 +-
>  gdbserver/linux-aarch64-tdesc.cc |  3 ++-
>  gdbserver/linux-arc-low.cc       |  2 +-
>  gdbserver/linux-arm-tdesc.cc     |  2 +-
>  gdbserver/linux-csky-low.cc      |  2 +-
>  gdbserver/linux-loongarch-low.cc |  2 +-
>  gdbserver/linux-riscv-low.cc     |  2 +-
>  gdbserver/linux-tic6x-low.cc     |  2 +-
>  gdbserver/linux-x86-tdesc.cc     | 15 +++++++++++++--
>  gdbserver/netbsd-aarch64-low.cc  |  2 +-
>  gdbserver/netbsd-amd64-low.cc    |  2 +-
>  gdbserver/netbsd-i386-low.cc     |  2 +-
>  gdbserver/tdesc.cc               |  5 ++++-
>  gdbserver/tdesc.h                |  5 +++--
>  gdbserver/win32-i386-low.cc      |  4 ++--
>  gdbserver/win32-low.h            |  7 +++++++
>  16 files changed, 41 insertions(+), 18 deletions(-)
> 
> diff --git a/gdbserver/linux-aarch32-tdesc.cc b/gdbserver/linux-aarch32-tdesc.cc
> index b8987752b9f..441fe668e6a 100644
> --- a/gdbserver/linux-aarch32-tdesc.cc
> +++ b/gdbserver/linux-aarch32-tdesc.cc
> @@ -34,7 +34,7 @@ aarch32_linux_read_description ()
>        tdesc_aarch32 = aarch32_create_target_description (false);
>  
>        static const char *expedite_regs[] = { "r11", "sp", "pc", 0 };
> -      init_target_desc (tdesc_aarch32, expedite_regs);
> +      init_target_desc (tdesc_aarch32, expedite_regs, GDB_OSABI_LINUX);
>      }
>    return tdesc_aarch32;
>  }
> diff --git a/gdbserver/linux-aarch64-tdesc.cc b/gdbserver/linux-aarch64-tdesc.cc
> index 31ec7854cc0..39d5bccdce1 100644
> --- a/gdbserver/linux-aarch64-tdesc.cc
> +++ b/gdbserver/linux-aarch64-tdesc.cc
> @@ -67,7 +67,8 @@ aarch64_linux_read_description (const aarch64_features &features)
>  
>        expedited_registers.push_back (nullptr);
>  
> -      init_target_desc (tdesc, (const char **) expedited_registers.data ());
> +      init_target_desc (tdesc, (const char **) expedited_registers.data (),
> +			GDB_OSABI_LINUX);
>  
>        tdesc_aarch64_map[features] = tdesc;
>      }
> diff --git a/gdbserver/linux-arc-low.cc b/gdbserver/linux-arc-low.cc
> index dda224f0a74..798c535aee8 100644
> --- a/gdbserver/linux-arc-low.cc
> +++ b/gdbserver/linux-arc-low.cc
> @@ -114,7 +114,7 @@ arc_linux_read_description (void)
>    target_desc_up tdesc = arc_create_target_description (features);
>  
>    static const char *expedite_regs[] = { "sp", "status32", nullptr };
> -  init_target_desc (tdesc.get (), expedite_regs);
> +  init_target_desc (tdesc.get (), expedite_regs, GDB_OSABI_LINUX);
>  
>    return tdesc.release ();
>  }
> diff --git a/gdbserver/linux-arm-tdesc.cc b/gdbserver/linux-arm-tdesc.cc
> index 559f9b0f3dc..fff2e948f81 100644
> --- a/gdbserver/linux-arm-tdesc.cc
> +++ b/gdbserver/linux-arm-tdesc.cc
> @@ -37,7 +37,7 @@ arm_linux_read_description (arm_fp_type fp_type)
>        tdesc = arm_create_target_description (fp_type, false);
>  
>        static const char *expedite_regs[] = { "r11", "sp", "pc", 0 };
> -      init_target_desc (tdesc, expedite_regs);
> +      init_target_desc (tdesc, expedite_regs, GDB_OSABI_LINUX);
>  
>        tdesc_arm_list[fp_type] = tdesc;
>      }
> diff --git a/gdbserver/linux-csky-low.cc b/gdbserver/linux-csky-low.cc
> index 2eb5a2df17b..18a0d152b5a 100644
> --- a/gdbserver/linux-csky-low.cc
> +++ b/gdbserver/linux-csky-low.cc
> @@ -133,7 +133,7 @@ csky_target::low_arch_setup ()
>  
>    if (tdesc->expedite_regs.empty ())
>      {
> -      init_target_desc (tdesc.get (), expedite_regs);
> +      init_target_desc (tdesc.get (), expedite_regs, GDB_OSABI_LINUX);
>        gdb_assert (!tdesc->expedite_regs.empty ());
>      }
>  
> diff --git a/gdbserver/linux-loongarch-low.cc b/gdbserver/linux-loongarch-low.cc
> index 584ea64a7d9..cf7d6c0743c 100644
> --- a/gdbserver/linux-loongarch-low.cc
> +++ b/gdbserver/linux-loongarch-low.cc
> @@ -85,7 +85,7 @@ loongarch_target::low_arch_setup ()
>  
>    if (tdesc->expedite_regs.empty ())
>      {
> -      init_target_desc (tdesc.get (), expedite_regs);
> +      init_target_desc (tdesc.get (), expedite_regs, GDB_OSABI_LINUX);
>        gdb_assert (!tdesc->expedite_regs.empty ());
>      }
>    current_process ()->tdesc = tdesc.release ();
> diff --git a/gdbserver/linux-riscv-low.cc b/gdbserver/linux-riscv-low.cc
> index c4554c507a8..7170ad9922e 100644
> --- a/gdbserver/linux-riscv-low.cc
> +++ b/gdbserver/linux-riscv-low.cc
> @@ -91,7 +91,7 @@ riscv_target::low_arch_setup ()
>  
>    if (tdesc->expedite_regs.empty ())
>      {
> -      init_target_desc (tdesc.get (), expedite_regs);
> +      init_target_desc (tdesc.get (), expedite_regs, GDB_OSABI_LINUX);
>        gdb_assert (!tdesc->expedite_regs.empty ());
>      }
>  
> diff --git a/gdbserver/linux-tic6x-low.cc b/gdbserver/linux-tic6x-low.cc
> index 707be2e7b0f..754dd00590c 100644
> --- a/gdbserver/linux-tic6x-low.cc
> +++ b/gdbserver/linux-tic6x-low.cc
> @@ -228,7 +228,7 @@ tic6x_read_description (enum c6x_feature feature)
>      {
>        *tdesc = tic6x_create_target_description (feature);
>        static const char *expedite_regs[] = { "A15", "PC", NULL };
> -      init_target_desc (*tdesc, expedite_regs);
> +      init_target_desc (*tdesc, expedite_regs, GDB_OSABI_LINUX);
>      }
>  
>    return *tdesc;
> diff --git a/gdbserver/linux-x86-tdesc.cc b/gdbserver/linux-x86-tdesc.cc
> index 13c80762605..6aa5c4ab970 100644
> --- a/gdbserver/linux-x86-tdesc.cc
> +++ b/gdbserver/linux-x86-tdesc.cc
> @@ -26,10 +26,21 @@
>  void
>  x86_linux_post_init_tdesc (target_desc *tdesc, bool is_64bit)
>  {
> +  enum gdb_osabi osabi = GDB_OSABI_LINUX;
> +
> +#ifndef IN_PROCESS_AGENT
> +  /* x86 target descriptions are created with the osabi already set.
> +     However, init_target_desc requires us to override the already set
> +     value.  That's fine, out new string should match the old one.  */
> +  gdb_assert (tdesc_osabi_name (tdesc) != nullptr);
> +  gdb_assert (strcmp (tdesc_osabi_name (tdesc),
> +		      gdbarch_osabi_name (osabi)) == 0);
> +#endif /* ! IN_PROCESS_AGENT */
> +
>  #ifdef __x86_64__
>    if (is_64bit)
> -    init_target_desc (tdesc, amd64_expedite_regs);
> +    init_target_desc (tdesc, amd64_expedite_regs, osabi);
>    else
>  #endif
> -    init_target_desc (tdesc, i386_expedite_regs);
> +    init_target_desc (tdesc, i386_expedite_regs, osabi);
>  }
> diff --git a/gdbserver/netbsd-aarch64-low.cc b/gdbserver/netbsd-aarch64-low.cc
> index f20a1a71773..8834e0ad894 100644
> --- a/gdbserver/netbsd-aarch64-low.cc
> +++ b/gdbserver/netbsd-aarch64-low.cc
> @@ -98,7 +98,7 @@ netbsd_aarch64_target::low_arch_setup ()
>      = aarch64_create_target_description ({});
>  
>    static const char *expedite_regs_aarch64[] = { "x29", "sp", "pc", NULL };
> -  init_target_desc (tdesc, expedite_regs_aarch64);
> +  init_target_desc (tdesc, expedite_regs_aarch64, GDB_OSABI_NETBSD);
>  
>    current_process ()->tdesc = tdesc;
>  }
> diff --git a/gdbserver/netbsd-amd64-low.cc b/gdbserver/netbsd-amd64-low.cc
> index b3f3aab5ec3..ad7cb430b92 100644
> --- a/gdbserver/netbsd-amd64-low.cc
> +++ b/gdbserver/netbsd-amd64-low.cc
> @@ -193,7 +193,7 @@ netbsd_amd64_target::low_arch_setup ()
>    target_desc *tdesc
>      = amd64_create_target_description (X86_XSTATE_SSE_MASK, false, false, false);
>  
> -  init_target_desc (tdesc, amd64_expedite_regs);
> +  init_target_desc (tdesc, amd64_expedite_regs, GDB_OSABI_NETBSD);
>  
>    current_process ()->tdesc = tdesc;
>  }
> diff --git a/gdbserver/netbsd-i386-low.cc b/gdbserver/netbsd-i386-low.cc
> index 247a39797c4..ea6fce4c6f9 100644
> --- a/gdbserver/netbsd-i386-low.cc
> +++ b/gdbserver/netbsd-i386-low.cc
> @@ -142,7 +142,7 @@ netbsd_i386_target::low_arch_setup ()
>    target_desc *tdesc
>      = i386_create_target_description (X86_XSTATE_SSE_MASK, false, false);
>  
> -  init_target_desc (tdesc, i386_expedite_regs);
> +  init_target_desc (tdesc, i386_expedite_regs, GDB_OSABI_NETBSD);
>  
>    current_process ()->tdesc = tdesc;
>  }
> diff --git a/gdbserver/tdesc.cc b/gdbserver/tdesc.cc
> index d052f43c76e..da1287abbbe 100644
> --- a/gdbserver/tdesc.cc
> +++ b/gdbserver/tdesc.cc
> @@ -53,7 +53,8 @@ void target_desc::accept (tdesc_element_visitor &v) const
>  
>  void
>  init_target_desc (struct target_desc *tdesc,
> -		  const char **expedite_regs)
> +		  const char **expedite_regs,
> +		  enum gdb_osabi osabi)
>  {
>    int offset = 0;
>  
> @@ -88,6 +89,8 @@ init_target_desc (struct target_desc *tdesc,
>    int expedite_count = 0;
>    while (expedite_regs[expedite_count] != nullptr)
>      tdesc->expedite_regs.push_back (expedite_regs[expedite_count++]);
> +
> +  set_tdesc_osabi (tdesc, osabi);
>  #endif
>  }
>  
> diff --git a/gdbserver/tdesc.h b/gdbserver/tdesc.h
> index 6fc37d038bc..80d9a9bebcb 100644
> --- a/gdbserver/tdesc.h
> +++ b/gdbserver/tdesc.h
> @@ -81,10 +81,11 @@ void copy_target_description (struct target_desc *dest,
>  			      const struct target_desc *src);
>  
>  /* Initialize TDESC, and then set its expedite_regs field to
> -   EXPEDITE_REGS.  */
> +   EXPEDITE_REGS.  The osabi of TDESC is set to OSABI.  */
>  
>  void init_target_desc (struct target_desc *tdesc,
> -		       const char **expedite_regs);
> +		       const char **expedite_regs,
> +		       enum gdb_osabi osabi);
>  
>  /* Return the current inferior's target description.  Never returns
>     NULL.  */
> diff --git a/gdbserver/win32-i386-low.cc b/gdbserver/win32-i386-low.cc
> index 0a761ca58ef..13f9aca99b1 100644
> --- a/gdbserver/win32-i386-low.cc
> +++ b/gdbserver/win32-i386-low.cc
> @@ -596,12 +596,12 @@ i386_arch_setup (void)
>  #ifdef __x86_64__
>    tdesc = amd64_create_target_description (X86_XSTATE_SSE_MASK, false,
>  					   false, false);
> -  init_target_desc (tdesc, amd64_expedite_regs);
> +  init_target_desc (tdesc, amd64_expedite_regs, WINDOWS_OSABI);
>    win32_tdesc = tdesc;
>  #endif
>  
>    tdesc = i386_create_target_description (X86_XSTATE_SSE_MASK, false, false);
> -  init_target_desc (tdesc, i386_expedite_regs);
> +  init_target_desc (tdesc, i386_expedite_regs, WINDOWS_OSABI);
>  #ifdef __x86_64__
>    wow64_win32_tdesc = tdesc;
>  #else
> diff --git a/gdbserver/win32-low.h b/gdbserver/win32-low.h
> index ff997df0a66..e0a09674518 100644
> --- a/gdbserver/win32-low.h
> +++ b/gdbserver/win32-low.h
> @@ -21,6 +21,7 @@
>  
>  #include <windows.h>
>  #include "nat/windows-nat.h"
> +#include "gdbsupport/osabi-common.h"
>  
>  struct target_desc;
>  
> @@ -31,6 +32,12 @@ extern const struct target_desc *win32_tdesc;
>  extern const struct target_desc *wow64_win32_tdesc;
>  #endif
>  
> +#ifdef __CYGWIN__
> +constexpr enum gdb_osabi WINDOWS_OSABI = GDB_OSABI_CYGWIN;
> +#else
> +constexpr enum gdb_osabi WINDOWS_OSABI = GDB_OSABI_WINDOWS;
> +#endif
> +
>  struct win32_target_ops
>  {
>    /* Architecture-specific setup.  */

Looks good to me.

Approved-By: Luis Machado <luis.machado@arm.com>