[PATCH 0/3] Set osabi in remote target descriptions

[PATCH 0/3] Set osabi in remote target descriptions

[Andrew Burgess]

I tried doing some remote debugging of a Window machine from a Linux
machine and ran into some problems caused by gdbserver failing to set
the osabi in the target description that is sent to GDB.  This series
fixes this issue.

Thanks,
Andrew

---

Andrew Burgess (3):
  gdbserver: make arch and osabi names gdb::unique_xmalloc_ptr<char>
  gdb: make use of set_tdesc_osabi overload in features/ files
  gdbserver: pass osabi to GDB in target description

 gdb/features/mips-dsp-linux.c        |  2 +-
 gdb/features/mips-linux.c            |  2 +-
 gdb/features/or1k-linux.c            |  2 +-
 gdb/features/sparc/sparc32-solaris.c |  2 +-
 gdb/features/sparc/sparc64-solaris.c |  2 +-
 gdb/target-descriptions.c            |  2 +-
 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         | 14 ++++++++++++--
 gdbserver/netbsd-aarch64-low.cc      |  2 +-
 gdbserver/netbsd-amd64-low.cc        |  2 +-
 gdbserver/netbsd-i386-low.cc         |  2 +-
 gdbserver/tdesc.cc                   | 23 +++++++++++------------
 gdbserver/tdesc.h                    | 11 +++++------
 gdbserver/win32-i386-low.cc          |  4 ++--
 gdbserver/win32-low.h                |  7 +++++++
 22 files changed, 55 insertions(+), 39 deletions(-)


base-commit: bcb92f7ba7b22ac882c000cabfd7ca8bea47c184
-- 
2.25.4

[PATCH 1/3] gdbserver: make arch and osabi names gdb::unique_xmalloc_ptr<char>

[Andrew Burgess]

Convert target_desc::arch and target_desc::osabi from 'const char*' to
gdb::unique_xmalloc_ptr<char>.  This also allows us to remove the user
defined ~target_desc destructor.

I doubt it ever actually occurred, but in theory at least, there was a
memory leak in set_tdesc_architecture and set_tdesc_osabi where the
member variables were assigned without freeing any previous
value... but I suspect that usually these fields are only set once.

There should be no user visible changes after this commit.
---
 gdbserver/tdesc.cc | 16 +++++-----------
 gdbserver/tdesc.h  |  6 ++----
 2 files changed, 7 insertions(+), 15 deletions(-)

diff --git a/gdbserver/tdesc.cc b/gdbserver/tdesc.cc
index 3265793da96..7f11120c318 100644
--- a/gdbserver/tdesc.cc
+++ b/gdbserver/tdesc.cc
@@ -20,12 +20,6 @@
 
 #ifndef IN_PROCESS_AGENT
 
-target_desc::~target_desc ()
-{
-  xfree ((char *) arch);
-  xfree ((char *) osabi);
-}
-
 bool target_desc::operator== (const target_desc &other) const
 {
   if (reg_defs != other.reg_defs)
@@ -162,7 +156,7 @@ tdesc_compatible_info_arch_name (const tdesc_compatible_info_up &c_info)
 const char *
 tdesc_architecture_name (const struct target_desc *target_desc)
 {
-  return target_desc->arch;
+  return target_desc->arch.get ();
 }
 
 /* See gdbsupport/tdesc.h.  */
@@ -171,7 +165,7 @@ void
 set_tdesc_architecture (struct target_desc *target_desc,
 			const char *name)
 {
-  target_desc->arch = xstrdup (name);
+  target_desc->arch = make_unique_xstrdup (name);
 }
 
 /* See gdbsupport/tdesc.h.  */
@@ -179,7 +173,7 @@ set_tdesc_architecture (struct target_desc *target_desc,
 const char *
 tdesc_osabi_name (const struct target_desc *target_desc)
 {
-  return target_desc->osabi;
+  return target_desc->osabi.get ();
 }
 
 /* See gdbsupport/tdesc.h.  */
@@ -187,7 +181,7 @@ tdesc_osabi_name (const struct target_desc *target_desc)
 void
 set_tdesc_osabi (struct target_desc *target_desc, const char *name)
 {
-  target_desc->osabi = xstrdup (name);
+  target_desc->osabi = make_unique_xstrdup (name);
 }
 
 /* See gdbsupport/tdesc.h.  */
@@ -198,7 +192,7 @@ tdesc_get_features_xml (const target_desc *tdesc)
   /* Either .xmltarget or .features is not NULL.  */
   gdb_assert (tdesc->xmltarget != NULL
 	      || (!tdesc->features.empty ()
-		  && tdesc->arch != NULL));
+		  && tdesc_architecture_name (tdesc) != nullptr));
 
   if (tdesc->xmltarget == NULL)
     {
diff --git a/gdbserver/tdesc.h b/gdbserver/tdesc.h
index 534b8b000f5..6fc37d038bc 100644
--- a/gdbserver/tdesc.h
+++ b/gdbserver/tdesc.h
@@ -54,18 +54,16 @@ struct target_desc final : tdesc_element
   mutable const char *xmltarget = NULL;
 
   /* The value of <architecture> element in the XML, replying GDB.  */
-  const char *arch = NULL;
+  gdb::unique_xmalloc_ptr<char> arch = nullptr;
 
   /* The value of <osabi> element in the XML, replying GDB.  */
-  const char *osabi = NULL;
+  gdb::unique_xmalloc_ptr<char> osabi = nullptr;
 
 public:
   target_desc ()
     : registers_size (0)
   {}
 
-  ~target_desc ();
-
   bool operator== (const target_desc &other) const;
 
   bool operator!= (const target_desc &other) const
-- 
2.25.4

Re: [PATCH 1/3] gdbserver: make arch and osabi names gdb::unique_xmalloc_ptr<char>

[Tom Tromey]

>>>>> "Andrew" == Andrew Burgess <aburgess@redhat.com> writes:

Andrew> Convert target_desc::arch and target_desc::osabi from 'const char*' to
Andrew> gdb::unique_xmalloc_ptr<char>.  This also allows us to remove the user
Andrew> defined ~target_desc destructor.

Andrew> I doubt it ever actually occurred, but in theory at least, there was a
Andrew> memory leak in set_tdesc_architecture and set_tdesc_osabi where the
Andrew> member variables were assigned without freeing any previous
Andrew> value... but I suspect that usually these fields are only set once.

Andrew> There should be no user visible changes after this commit.

Looks good to me, thank you.
Approved-By: Tom Tromey <tom@tromey.com>

Tom

Re: [PATCH 1/3] gdbserver: make arch and osabi names gdb::unique_xmalloc_ptr<char>

[Simon Marchi]

On 2024-10-06 14:37, Andrew Burgess wrote:
> Convert target_desc::arch and target_desc::osabi from 'const char*' to
> gdb::unique_xmalloc_ptr<char>.  This also allows us to remove the user
> defined ~target_desc destructor.
> 
> I doubt it ever actually occurred, but in theory at least, there was a
> memory leak in set_tdesc_architecture and set_tdesc_osabi where the
> member variables were assigned without freeing any previous
> value... but I suspect that usually these fields are only set once.
> 
> There should be no user visible changes after this commit.

Could they be std::string instead?

Simon

Re: [PATCH 1/3] gdbserver: make arch and osabi names gdb::unique_xmalloc_ptr<char>

[Andrew Burgess]

Simon Marchi <simark@simark.ca> writes:

> On 2024-10-06 14:37, Andrew Burgess wrote:
>> Convert target_desc::arch and target_desc::osabi from 'const char*' to
>> gdb::unique_xmalloc_ptr<char>.  This also allows us to remove the user
>> defined ~target_desc destructor.
>> 
>> I doubt it ever actually occurred, but in theory at least, there was a
>> memory leak in set_tdesc_architecture and set_tdesc_osabi where the
>> member variables were assigned without freeing any previous
>> value... but I suspect that usually these fields are only set once.
>> 
>> There should be no user visible changes after this commit.
>
> Could they be std::string instead?

I considered this, but that would, I think, require wider reaching
changes as the API to add the osabi to the returned tdesc passes the
information via 'const char *'.  This approach added the memory safety
without requiring any further changes.

If you'd prefer std::string be used then let me know and I'll update
things.

Thanks,
Andrew

Re: [PATCH 1/3] gdbserver: make arch and osabi names gdb::unique_xmalloc_ptr<char>

[Simon Marchi]

On 2024-10-09 07:07, Andrew Burgess wrote:
> Simon Marchi <simark@simark.ca> writes:
> 
>> On 2024-10-06 14:37, Andrew Burgess wrote:
>>> Convert target_desc::arch and target_desc::osabi from 'const char*' to
>>> gdb::unique_xmalloc_ptr<char>.  This also allows us to remove the user
>>> defined ~target_desc destructor.
>>>
>>> I doubt it ever actually occurred, but in theory at least, there was a
>>> memory leak in set_tdesc_architecture and set_tdesc_osabi where the
>>> member variables were assigned without freeing any previous
>>> value... but I suspect that usually these fields are only set once.
>>>
>>> There should be no user visible changes after this commit.
>>
>> Could they be std::string instead?
> 
> I considered this, but that would, I think, require wider reaching
> changes as the API to add the osabi to the returned tdesc passes the
> information via 'const char *'.  This approach added the memory safety
> without requiring any further changes.
> 
> If you'd prefer std::string be used then let me know and I'll update
> things.

Up to you, I don't mind, it can be done later.

Simon

Re: [PATCH 1/3] gdbserver: make arch and osabi names gdb::unique_xmalloc_ptr<char>

[Andrew Burgess]

Simon Marchi <simark@simark.ca> writes:

> On 2024-10-09 07:07, Andrew Burgess wrote:
>> Simon Marchi <simark@simark.ca> writes:
>> 
>>> On 2024-10-06 14:37, Andrew Burgess wrote:
>>>> Convert target_desc::arch and target_desc::osabi from 'const char*' to
>>>> gdb::unique_xmalloc_ptr<char>.  This also allows us to remove the user
>>>> defined ~target_desc destructor.
>>>>
>>>> I doubt it ever actually occurred, but in theory at least, there was a
>>>> memory leak in set_tdesc_architecture and set_tdesc_osabi where the
>>>> member variables were assigned without freeing any previous
>>>> value... but I suspect that usually these fields are only set once.
>>>>
>>>> There should be no user visible changes after this commit.
>>>
>>> Could they be std::string instead?
>> 
>> I considered this, but that would, I think, require wider reaching
>> changes as the API to add the osabi to the returned tdesc passes the
>> information via 'const char *'.  This approach added the memory safety
>> without requiring any further changes.
>> 
>> If you'd prefer std::string be used then let me know and I'll update
>> things.
>
> Up to you, I don't mind, it can be done later.

I'll leave it then if that's OK.

My reason is that, if the osbi member variable changes to std::string
then it makes sense that tdesc_osabi_name return 'const std::string &'.
Which means that tdesc_osabi_name in GDB will also need to return 'const
std::string &', but on the GDB side we store 'enum gdb_osabi' rather
than the actual string ... so now that code needs updating too.

I only wanted to fix this so that I could call set_tdesc_osbi without
leaking memory, and gdb::unique_xmalloc_ptr<char>, which we use in loads
of places, achieves that, and leaves the API untouched.

But I'm not against changing to std::string if someone's keen :)

Thanks,
Andrew

Re: [PATCH 1/3] gdbserver: make arch and osabi names gdb::unique_xmalloc_ptr<char>

[Simon Marchi]

On 2024-10-09 08:17, Andrew Burgess wrote:
> Simon Marchi <simark@simark.ca> writes:
> 
>> On 2024-10-09 07:07, Andrew Burgess wrote:
>>> Simon Marchi <simark@simark.ca> writes:
>>>
>>>> On 2024-10-06 14:37, Andrew Burgess wrote:
>>>>> Convert target_desc::arch and target_desc::osabi from 'const char*' to
>>>>> gdb::unique_xmalloc_ptr<char>.  This also allows us to remove the user
>>>>> defined ~target_desc destructor.
>>>>>
>>>>> I doubt it ever actually occurred, but in theory at least, there was a
>>>>> memory leak in set_tdesc_architecture and set_tdesc_osabi where the
>>>>> member variables were assigned without freeing any previous
>>>>> value... but I suspect that usually these fields are only set once.
>>>>>
>>>>> There should be no user visible changes after this commit.
>>>>
>>>> Could they be std::string instead?
>>>
>>> I considered this, but that would, I think, require wider reaching
>>> changes as the API to add the osabi to the returned tdesc passes the
>>> information via 'const char *'.  This approach added the memory safety
>>> without requiring any further changes.
>>>
>>> If you'd prefer std::string be used then let me know and I'll update
>>> things.
>>
>> Up to you, I don't mind, it can be done later.
> 
> I'll leave it then if that's OK.
> 
> My reason is that, if the osbi member variable changes to std::string
> then it makes sense that tdesc_osabi_name return 'const std::string &'.
> Which means that tdesc_osabi_name in GDB will also need to return 'const
> std::string &', but on the GDB side we store 'enum gdb_osabi' rather
> than the actual string ... so now that code needs updating too.
> 
> I only wanted to fix this so that I could call set_tdesc_osbi without
> leaking memory, and gdb::unique_xmalloc_ptr<char>, which we use in loads
> of places, achieves that, and leaves the API untouched.
> 
> But I'm not against changing to std::string if someone's keen :)

Yeah, it makes sense to take smaller, simpler steps.  I might do it once
your series is merged, to scratch my itch :).

Simon

Re: [PATCH 1/3] gdbserver: make arch and osabi names gdb::unique_xmalloc_ptr<char>

[Andrew Burgess]

Simon Marchi <simark@simark.ca> writes:

> On 2024-10-09 08:17, Andrew Burgess wrote:
>> Simon Marchi <simark@simark.ca> writes:
>> 
>>> On 2024-10-09 07:07, Andrew Burgess wrote:
>>>> Simon Marchi <simark@simark.ca> writes:
>>>>
>>>>> On 2024-10-06 14:37, Andrew Burgess wrote:
>>>>>> Convert target_desc::arch and target_desc::osabi from 'const char*' to
>>>>>> gdb::unique_xmalloc_ptr<char>.  This also allows us to remove the user
>>>>>> defined ~target_desc destructor.
>>>>>>
>>>>>> I doubt it ever actually occurred, but in theory at least, there was a
>>>>>> memory leak in set_tdesc_architecture and set_tdesc_osabi where the
>>>>>> member variables were assigned without freeing any previous
>>>>>> value... but I suspect that usually these fields are only set once.
>>>>>>
>>>>>> There should be no user visible changes after this commit.
>>>>>
>>>>> Could they be std::string instead?
>>>>
>>>> I considered this, but that would, I think, require wider reaching
>>>> changes as the API to add the osabi to the returned tdesc passes the
>>>> information via 'const char *'.  This approach added the memory safety
>>>> without requiring any further changes.
>>>>
>>>> If you'd prefer std::string be used then let me know and I'll update
>>>> things.
>>>
>>> Up to you, I don't mind, it can be done later.
>> 
>> I'll leave it then if that's OK.
>> 
>> My reason is that, if the osbi member variable changes to std::string
>> then it makes sense that tdesc_osabi_name return 'const std::string &'.
>> Which means that tdesc_osabi_name in GDB will also need to return 'const
>> std::string &', but on the GDB side we store 'enum gdb_osabi' rather
>> than the actual string ... so now that code needs updating too.
>> 
>> I only wanted to fix this so that I could call set_tdesc_osbi without
>> leaking memory, and gdb::unique_xmalloc_ptr<char>, which we use in loads
>> of places, achieves that, and leaves the API untouched.
>> 
>> But I'm not against changing to std::string if someone's keen :)
>
> Yeah, it makes sense to take smaller, simpler steps.  I might do it once
> your series is merged, to scratch my itch :).

Sounds good.

I'll hold off merging until you've had a chance to check out v2 w.r.t
the switch to use the 'enum gdb_osabi' instead of passing strings.

Thanks,
Andrew

[PATCH 2/3] gdb: make use of set_tdesc_osabi overload in features/ files

[Andrew Burgess]

There are two versions of the set_tdesc_osabi function in GDB:

  void
  set_tdesc_osabi (struct target_desc *target_desc, const char *name)
  {
    set_tdesc_osabi (target_desc, osabi_from_tdesc_string (name));
  }

  void
  set_tdesc_osabi (struct target_desc *target_desc, enum gdb_osabi osabi)
  {
    target_desc->osabi = osabi;
  }

In the gdb/features/ files we call the second of these functions, like
this:

  set_tdesc_osabi (result.get (), osabi_from_tdesc_string ("GNU/Linux"));

This can be replaced with a call to the first set_tdesc_osabi
function, so lets do that.  I think that this makes the features/ code
slightly simpler and easier to understand.

There should be no user visible changes after this commit.
---
 gdb/features/mips-dsp-linux.c        | 2 +-
 gdb/features/mips-linux.c            | 2 +-
 gdb/features/or1k-linux.c            | 2 +-
 gdb/features/sparc/sparc32-solaris.c | 2 +-
 gdb/features/sparc/sparc64-solaris.c | 2 +-
 gdb/target-descriptions.c            | 2 +-
 6 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/gdb/features/mips-dsp-linux.c b/gdb/features/mips-dsp-linux.c
index d8e40283caf..17d30aac68f 100644
--- a/gdb/features/mips-dsp-linux.c
+++ b/gdb/features/mips-dsp-linux.c
@@ -11,7 +11,7 @@ initialize_tdesc_mips_dsp_linux (void)
   target_desc_up result = allocate_target_description ();
   set_tdesc_architecture (result.get (), bfd_scan_arch ("mips"));
 
-  set_tdesc_osabi (result.get (), osabi_from_tdesc_string ("GNU/Linux"));
+  set_tdesc_osabi (result.get (), "GNU/Linux");
 
   struct tdesc_feature *feature;
 
diff --git a/gdb/features/mips-linux.c b/gdb/features/mips-linux.c
index f93eef5e41d..a43526db24e 100644
--- a/gdb/features/mips-linux.c
+++ b/gdb/features/mips-linux.c
@@ -11,7 +11,7 @@ initialize_tdesc_mips_linux (void)
   target_desc_up result = allocate_target_description ();
   set_tdesc_architecture (result.get (), bfd_scan_arch ("mips"));
 
-  set_tdesc_osabi (result.get (), osabi_from_tdesc_string ("GNU/Linux"));
+  set_tdesc_osabi (result.get (), "GNU/Linux");
 
   struct tdesc_feature *feature;
 
diff --git a/gdb/features/or1k-linux.c b/gdb/features/or1k-linux.c
index 24731458de2..551b38f59c0 100644
--- a/gdb/features/or1k-linux.c
+++ b/gdb/features/or1k-linux.c
@@ -11,7 +11,7 @@ initialize_tdesc_or1k_linux (void)
   target_desc_up result = allocate_target_description ();
   set_tdesc_architecture (result.get (), bfd_scan_arch ("or1k"));
 
-  set_tdesc_osabi (result.get (), osabi_from_tdesc_string ("GNU/Linux"));
+  set_tdesc_osabi (result.get (), "GNU/Linux");
 
   struct tdesc_feature *feature;
 
diff --git a/gdb/features/sparc/sparc32-solaris.c b/gdb/features/sparc/sparc32-solaris.c
index dce96851f96..ce57abaaaf2 100644
--- a/gdb/features/sparc/sparc32-solaris.c
+++ b/gdb/features/sparc/sparc32-solaris.c
@@ -11,7 +11,7 @@ initialize_tdesc_sparc32_solaris (void)
   target_desc_up result = allocate_target_description ();
   set_tdesc_architecture (result.get (), bfd_scan_arch ("sparc"));
 
-  set_tdesc_osabi (result.get (), osabi_from_tdesc_string ("Solaris"));
+  set_tdesc_osabi (result.get (), "Solaris");
 
   struct tdesc_feature *feature;
 
diff --git a/gdb/features/sparc/sparc64-solaris.c b/gdb/features/sparc/sparc64-solaris.c
index d030df63c6c..92cc88cd5cf 100644
--- a/gdb/features/sparc/sparc64-solaris.c
+++ b/gdb/features/sparc/sparc64-solaris.c
@@ -11,7 +11,7 @@ initialize_tdesc_sparc64_solaris (void)
   target_desc_up result = allocate_target_description ();
   set_tdesc_architecture (result.get (), bfd_scan_arch ("sparc:v9"));
 
-  set_tdesc_osabi (result.get (), osabi_from_tdesc_string ("Solaris"));
+  set_tdesc_osabi (result.get (), "Solaris");
 
   struct tdesc_feature *feature;
 
diff --git a/gdb/target-descriptions.c b/gdb/target-descriptions.c
index d78da14918f..c165750c6c5 100644
--- a/gdb/target-descriptions.c
+++ b/gdb/target-descriptions.c
@@ -1318,7 +1318,7 @@ class print_c_tdesc : public tdesc_element_visitor
 	&& tdesc_osabi (e) < GDB_OSABI_INVALID)
       {
 	gdb_printf
-	  ("  set_tdesc_osabi (result.get (), osabi_from_tdesc_string (\"%s\"));\n",
+	  ("  set_tdesc_osabi (result.get (), \"%s\");\n",
 	   gdbarch_osabi_name (tdesc_osabi (e)));
 	gdb_printf ("\n");
       }
-- 
2.25.4

Re: [PATCH 2/3] gdb: make use of set_tdesc_osabi overload in features/ files

[Tom Tromey]

>>>>> "Andrew" == Andrew Burgess <aburgess@redhat.com> writes:

Andrew> In the gdb/features/ files we call the second of these functions, like
Andrew> this:

Andrew>   set_tdesc_osabi (result.get (), osabi_from_tdesc_string ("GNU/Linux"));

Andrew> This can be replaced with a call to the first set_tdesc_osabi
Andrew> function, so lets do that.  I think that this makes the features/ code
Andrew> slightly simpler and easier to understand.

Andrew> There should be no user visible changes after this commit.

Looks good.
Approved-By: Tom Tromey <tom@tromey.com>

Tom

Re: [PATCH 2/3] gdb: make use of set_tdesc_osabi overload in features/ files

[Simon Marchi]

On 2024-10-06 14:37, Andrew Burgess wrote:
> There are two versions of the set_tdesc_osabi function in GDB:
> 
>   void
>   set_tdesc_osabi (struct target_desc *target_desc, const char *name)
>   {
>     set_tdesc_osabi (target_desc, osabi_from_tdesc_string (name));
>   }
> 
>   void
>   set_tdesc_osabi (struct target_desc *target_desc, enum gdb_osabi osabi)
>   {
>     target_desc->osabi = osabi;
>   }
> 
> In the gdb/features/ files we call the second of these functions, like
> this:
> 
>   set_tdesc_osabi (result.get (), osabi_from_tdesc_string ("GNU/Linux"));
> 
> This can be replaced with a call to the first set_tdesc_osabi
> function, so lets do that.  I think that this makes the features/ code
> slightly simpler and easier to understand.
> 
> There should be no user visible changes after this commit.

Just a nit but... while at it, couldn't we change them to use the
enumerator directly, to save lookups at runtime?

   set_tdesc_osabi (result.get (), GDB_OSABI_LINUX);

I guess we would need a enum -> string (GDB_OSABI_LINUX ->
"GDB_OSABI_LINUX") function, which we have for other enums.  We just
don't have it for this enum yet.  Another case for adding a "magic enum"
library :).

Simon

Re: [PATCH 2/3] gdb: make use of set_tdesc_osabi overload in features/ files

[Andrew Burgess]

Simon Marchi <simark@simark.ca> writes:

> On 2024-10-06 14:37, Andrew Burgess wrote:
>> There are two versions of the set_tdesc_osabi function in GDB:
>> 
>>   void
>>   set_tdesc_osabi (struct target_desc *target_desc, const char *name)
>>   {
>>     set_tdesc_osabi (target_desc, osabi_from_tdesc_string (name));
>>   }
>> 
>>   void
>>   set_tdesc_osabi (struct target_desc *target_desc, enum gdb_osabi osabi)
>>   {
>>     target_desc->osabi = osabi;
>>   }
>> 
>> In the gdb/features/ files we call the second of these functions, like
>> this:
>> 
>>   set_tdesc_osabi (result.get (), osabi_from_tdesc_string ("GNU/Linux"));
>> 
>> This can be replaced with a call to the first set_tdesc_osabi
>> function, so lets do that.  I think that this makes the features/ code
>> slightly simpler and easier to understand.
>> 
>> There should be no user visible changes after this commit.
>
> Just a nit but... while at it, couldn't we change them to use the
> enumerator directly, to save lookups at runtime?
>
>    set_tdesc_osabi (result.get (), GDB_OSABI_LINUX);
>
> I guess we would need a enum -> string (GDB_OSABI_LINUX ->
> "GDB_OSABI_LINUX") function, which we have for other enums.  We just
> don't have it for this enum yet.  Another case for adding a "magic enum"
> library :).

This was Luis' request.  See v2 my attempt at this.

Thanks,
Andrew

Re: [PATCH 2/3] gdb: make use of set_tdesc_osabi overload in features/ files

[Simon Marchi]

On 2024-10-09 07:08, Andrew Burgess wrote:
> Simon Marchi <simark@simark.ca> writes:
> 
>> On 2024-10-06 14:37, Andrew Burgess wrote:
>>> There are two versions of the set_tdesc_osabi function in GDB:
>>>
>>>   void
>>>   set_tdesc_osabi (struct target_desc *target_desc, const char *name)
>>>   {
>>>     set_tdesc_osabi (target_desc, osabi_from_tdesc_string (name));
>>>   }
>>>
>>>   void
>>>   set_tdesc_osabi (struct target_desc *target_desc, enum gdb_osabi osabi)
>>>   {
>>>     target_desc->osabi = osabi;
>>>   }
>>>
>>> In the gdb/features/ files we call the second of these functions, like
>>> this:
>>>
>>>   set_tdesc_osabi (result.get (), osabi_from_tdesc_string ("GNU/Linux"));
>>>
>>> This can be replaced with a call to the first set_tdesc_osabi
>>> function, so lets do that.  I think that this makes the features/ code
>>> slightly simpler and easier to understand.
>>>
>>> There should be no user visible changes after this commit.
>>
>> Just a nit but... while at it, couldn't we change them to use the
>> enumerator directly, to save lookups at runtime?
>>
>>    set_tdesc_osabi (result.get (), GDB_OSABI_LINUX);
>>
>> I guess we would need a enum -> string (GDB_OSABI_LINUX ->
>> "GDB_OSABI_LINUX") function, which we have for other enums.  We just
>> don't have it for this enum yet.  Another case for adding a "magic enum"
>> library :).
> 
> This was Luis' request.  See v2 my attempt at this.

Oh, sorry, I did not see v2 yesterday.

Simon

Re: [PATCH 2/3] gdb: make use of set_tdesc_osabi overload in features/ files

[Andrew Burgess]

Simon Marchi <simark@simark.ca> writes:

> On 2024-10-09 07:08, Andrew Burgess wrote:
>> Simon Marchi <simark@simark.ca> writes:
>> 
>>> On 2024-10-06 14:37, Andrew Burgess wrote:
>>>> There are two versions of the set_tdesc_osabi function in GDB:
>>>>
>>>>   void
>>>>   set_tdesc_osabi (struct target_desc *target_desc, const char *name)
>>>>   {
>>>>     set_tdesc_osabi (target_desc, osabi_from_tdesc_string (name));
>>>>   }
>>>>
>>>>   void
>>>>   set_tdesc_osabi (struct target_desc *target_desc, enum gdb_osabi osabi)
>>>>   {
>>>>     target_desc->osabi = osabi;
>>>>   }
>>>>
>>>> In the gdb/features/ files we call the second of these functions, like
>>>> this:
>>>>
>>>>   set_tdesc_osabi (result.get (), osabi_from_tdesc_string ("GNU/Linux"));
>>>>
>>>> This can be replaced with a call to the first set_tdesc_osabi
>>>> function, so lets do that.  I think that this makes the features/ code
>>>> slightly simpler and easier to understand.
>>>>
>>>> There should be no user visible changes after this commit.
>>>
>>> Just a nit but... while at it, couldn't we change them to use the
>>> enumerator directly, to save lookups at runtime?
>>>
>>>    set_tdesc_osabi (result.get (), GDB_OSABI_LINUX);
>>>
>>> I guess we would need a enum -> string (GDB_OSABI_LINUX ->
>>> "GDB_OSABI_LINUX") function, which we have for other enums.  We just
>>> don't have it for this enum yet.  Another case for adding a "magic enum"
>>> library :).
>> 
>> This was Luis' request.  See v2 my attempt at this.
>
> Oh, sorry, I did not see v2 yesterday.

I just realised I actually forgot to tag it v2.  It starts here:

  https://inbox.sourceware.org/gdb-patches/cover.1728407374.git.aburgess@redhat.com/T/#u

totally my fault.  Sorry for that.

Thanks,
Andrew

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

[Andrew Burgess]

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     | 14 ++++++++++++--
 gdbserver/netbsd-aarch64-low.cc  |  2 +-
 gdbserver/netbsd-amd64-low.cc    |  2 +-
 gdbserver/netbsd-i386-low.cc     |  2 +-
 gdbserver/tdesc.cc               |  7 ++++++-
 gdbserver/tdesc.h                |  5 +++--
 gdbserver/win32-i386-low.cc      |  4 ++--
 gdbserver/win32-low.h            |  7 +++++++
 16 files changed, 42 insertions(+), 18 deletions(-)

diff --git a/gdbserver/linux-aarch32-tdesc.cc b/gdbserver/linux-aarch32-tdesc.cc
index b8987752b9f..bff671b4f1e 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, "GNU/Linux");
     }
   return tdesc_aarch32;
 }
diff --git a/gdbserver/linux-aarch64-tdesc.cc b/gdbserver/linux-aarch64-tdesc.cc
index 31ec7854cc0..f6c258f5162 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 (),
+			"GNU/Linux");
 
       tdesc_aarch64_map[features] = tdesc;
     }
diff --git a/gdbserver/linux-arc-low.cc b/gdbserver/linux-arc-low.cc
index dda224f0a74..d382791c4bb 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, "GNU/Linux");
 
   return tdesc.release ();
 }
diff --git a/gdbserver/linux-arm-tdesc.cc b/gdbserver/linux-arm-tdesc.cc
index 559f9b0f3dc..e6bb12bad79 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, "GNU/Linux");
 
       tdesc_arm_list[fp_type] = tdesc;
     }
diff --git a/gdbserver/linux-csky-low.cc b/gdbserver/linux-csky-low.cc
index 2eb5a2df17b..0154516f196 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, "GNU/Linux");
       gdb_assert (!tdesc->expedite_regs.empty ());
     }
 
diff --git a/gdbserver/linux-loongarch-low.cc b/gdbserver/linux-loongarch-low.cc
index 584ea64a7d9..3161d72b733 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, "GNU/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..a02a2bef35d 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, "GNU/Linux");
       gdb_assert (!tdesc->expedite_regs.empty ());
     }
 
diff --git a/gdbserver/linux-tic6x-low.cc b/gdbserver/linux-tic6x-low.cc
index 707be2e7b0f..a6764149454 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, "GNU/Linux");
     }
 
   return *tdesc;
diff --git a/gdbserver/linux-x86-tdesc.cc b/gdbserver/linux-x86-tdesc.cc
index 13c80762605..f6051a6729f 100644
--- a/gdbserver/linux-x86-tdesc.cc
+++ b/gdbserver/linux-x86-tdesc.cc
@@ -26,10 +26,20 @@
 void
 x86_linux_post_init_tdesc (target_desc *tdesc, bool is_64bit)
 {
+  constexpr const char *osabi_name = "GNU/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), osabi_name) == 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_name);
   else
 #endif
-    init_target_desc (tdesc, i386_expedite_regs);
+    init_target_desc (tdesc, i386_expedite_regs, osabi_name);
 }
diff --git a/gdbserver/netbsd-aarch64-low.cc b/gdbserver/netbsd-aarch64-low.cc
index f20a1a71773..c86c46335e0 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, "NetBSD");
 
   current_process ()->tdesc = tdesc;
 }
diff --git a/gdbserver/netbsd-amd64-low.cc b/gdbserver/netbsd-amd64-low.cc
index b3f3aab5ec3..c8c41ae1bc8 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, "NetBSD");
 
   current_process ()->tdesc = tdesc;
 }
diff --git a/gdbserver/netbsd-i386-low.cc b/gdbserver/netbsd-i386-low.cc
index 247a39797c4..f5da9dc449b 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, "NetBSD");
 
   current_process ()->tdesc = tdesc;
 }
diff --git a/gdbserver/tdesc.cc b/gdbserver/tdesc.cc
index 7f11120c318..94ec46deb22 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,
+		  const char *osabi)
 {
   int offset = 0;
 
@@ -88,6 +89,10 @@ 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++]);
+
+  /* If TDESC doesn't already have an osabi set, then set it now.  */
+  gdb_assert (osabi != nullptr);
+  set_tdesc_osabi (tdesc, osabi);
 #endif
 }
 
diff --git a/gdbserver/tdesc.h b/gdbserver/tdesc.h
index 6fc37d038bc..05c48f7c989 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 name of TDESC is set to OSABI.  */
 
 void init_target_desc (struct target_desc *tdesc,
-		       const char **expedite_regs);
+		       const char **expedite_regs,
+		       const char *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..b6cd0429557 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, OSABI_NAME);
   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, OSABI_NAME);
 #ifdef __x86_64__
   wow64_win32_tdesc = tdesc;
 #else
diff --git a/gdbserver/win32-low.h b/gdbserver/win32-low.h
index ff997df0a66..e447cc5f9b2 100644
--- a/gdbserver/win32-low.h
+++ b/gdbserver/win32-low.h
@@ -31,6 +31,13 @@ extern const struct target_desc *win32_tdesc;
 extern const struct target_desc *wow64_win32_tdesc;
 #endif
 
+#ifdef __CYGWIN__
+#define OSABI_NAME "Cygwin"
+constexpr const char *OSABI_NAME = "Cygwin";
+#else
+constexpr const char *OSABI_NAME = "Windows";
+#endif
+
 struct win32_target_ops
 {
   /* Architecture-specific setup.  */
-- 
2.25.4

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

[Luis Machado]

On 10/6/24 19:37, 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     | 14 ++++++++++++--
>  gdbserver/netbsd-aarch64-low.cc  |  2 +-
>  gdbserver/netbsd-amd64-low.cc    |  2 +-
>  gdbserver/netbsd-i386-low.cc     |  2 +-
>  gdbserver/tdesc.cc               |  7 ++++++-
>  gdbserver/tdesc.h                |  5 +++--
>  gdbserver/win32-i386-low.cc      |  4 ++--
>  gdbserver/win32-low.h            |  7 +++++++
>  16 files changed, 42 insertions(+), 18 deletions(-)
> 
> diff --git a/gdbserver/linux-aarch32-tdesc.cc b/gdbserver/linux-aarch32-tdesc.cc
> index b8987752b9f..bff671b4f1e 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, "GNU/Linux");


These changes sound OK to me, but wouldn't it be cleaner to pass, say, an enum for the
osabi, and then later get that translated to the proper XML/string entry?

Maybe share the osabi naming with what gdb has.

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

[Tom Tromey]

>>>>> "Andrew" == Andrew Burgess <aburgess@redhat.com> writes:

Andrew> So what I've done is require the osabi name to be passed to the
Andrew> init_target_desc function.  This is called, I believe, for all
Andrew> targets, in the gdbserver code.

FWIW this seems reasonable to me but I didn't want to approve it until
Luis' comment is discussed.

Tom

[PATCH 0/5] Set osabi in remote target descriptions

[Andrew Burgess]

I tried doing some remote debugging of a Window machine from a Linux
machine and ran into some problems caused by gdbserver failing to set
the osabi in the target description that is sent to GDB.  This series
fixes this issue.

In v2:

  - Patches 3 & 4 are new.  These are refactoring to allow for the
    updated patch 5.

  - Patch 5 is mostly the same, except the osabi is now set based on
    the enum values, rather than the osabi name strings.

Thanks,
Andrew

---

Andrew Burgess (5):
  gdbserver: make arch and osabi names gdb::unique_xmalloc_ptr<char>
  gdb: make use of set_tdesc_osabi overload in features/ files
  gdb: split osabi support between gdb/ and gdbsupport/ directories
  gdb/gdbserver: change shared set_tdesc_osabi to take gdb_osabi
  gdbserver: pass osabi to GDB in target description

 gdb/arch/amd64.c                     |   3 +-
 gdb/arch/i386.c                      |   3 +-
 gdb/arch/tic6x.c                     |   3 +-
 gdb/features/mips-dsp-linux.c        |   2 +-
 gdb/features/mips-linux.c            |   2 +-
 gdb/features/or1k-linux.c            |   2 +-
 gdb/features/sparc/sparc32-solaris.c |   2 +-
 gdb/features/sparc/sparc64-solaris.c |   2 +-
 gdb/osabi.c                          | 120 +++++++--------------------
 gdb/osabi.h                          |  45 ++--------
 gdb/target-descriptions.c            |  11 +--
 gdb/target-descriptions.h            |   1 -
 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                   |  24 +++---
 gdbserver/tdesc.h                    |  11 ++-
 gdbserver/win32-i386-low.cc          |   4 +-
 gdbserver/win32-low.h                |   7 ++
 gdbsupport/Makefile.am               |   1 +
 gdbsupport/Makefile.in               |  15 ++--
 gdbsupport/osabi-common.cc           |  98 ++++++++++++++++++++++
 gdbsupport/osabi-common.def          |  57 +++++++++++++
 gdbsupport/osabi-common.h            |  54 ++++++++++++
 gdbsupport/tdesc.h                   |   4 +-
 34 files changed, 319 insertions(+), 190 deletions(-)
 create mode 100644 gdbsupport/osabi-common.cc
 create mode 100644 gdbsupport/osabi-common.def
 create mode 100644 gdbsupport/osabi-common.h


base-commit: bcb92f7ba7b22ac882c000cabfd7ca8bea47c184
-- 
2.25.4

[PATCH 1/5] gdbserver: make arch and osabi names gdb::unique_xmalloc_ptr<char>

[Andrew Burgess]

Convert target_desc::arch and target_desc::osabi from 'const char*' to
gdb::unique_xmalloc_ptr<char>.  This also allows us to remove the user
defined ~target_desc destructor.

I doubt it ever actually occurred, but in theory at least, there was a
memory leak in set_tdesc_architecture and set_tdesc_osabi where the
member variables were assigned without freeing any previous
value... but I suspect that usually these fields are only set once.

There should be no user visible changes after this commit.

Approved-By: Tom Tromey <tom@tromey.com>
---
 gdbserver/tdesc.cc | 16 +++++-----------
 gdbserver/tdesc.h  |  6 ++----
 2 files changed, 7 insertions(+), 15 deletions(-)

diff --git a/gdbserver/tdesc.cc b/gdbserver/tdesc.cc
index 3265793da96..7f11120c318 100644
--- a/gdbserver/tdesc.cc
+++ b/gdbserver/tdesc.cc
@@ -20,12 +20,6 @@
 
 #ifndef IN_PROCESS_AGENT
 
-target_desc::~target_desc ()
-{
-  xfree ((char *) arch);
-  xfree ((char *) osabi);
-}
-
 bool target_desc::operator== (const target_desc &other) const
 {
   if (reg_defs != other.reg_defs)
@@ -162,7 +156,7 @@ tdesc_compatible_info_arch_name (const tdesc_compatible_info_up &c_info)
 const char *
 tdesc_architecture_name (const struct target_desc *target_desc)
 {
-  return target_desc->arch;
+  return target_desc->arch.get ();
 }
 
 /* See gdbsupport/tdesc.h.  */
@@ -171,7 +165,7 @@ void
 set_tdesc_architecture (struct target_desc *target_desc,
 			const char *name)
 {
-  target_desc->arch = xstrdup (name);
+  target_desc->arch = make_unique_xstrdup (name);
 }
 
 /* See gdbsupport/tdesc.h.  */
@@ -179,7 +173,7 @@ set_tdesc_architecture (struct target_desc *target_desc,
 const char *
 tdesc_osabi_name (const struct target_desc *target_desc)
 {
-  return target_desc->osabi;
+  return target_desc->osabi.get ();
 }
 
 /* See gdbsupport/tdesc.h.  */
@@ -187,7 +181,7 @@ tdesc_osabi_name (const struct target_desc *target_desc)
 void
 set_tdesc_osabi (struct target_desc *target_desc, const char *name)
 {
-  target_desc->osabi = xstrdup (name);
+  target_desc->osabi = make_unique_xstrdup (name);
 }
 
 /* See gdbsupport/tdesc.h.  */
@@ -198,7 +192,7 @@ tdesc_get_features_xml (const target_desc *tdesc)
   /* Either .xmltarget or .features is not NULL.  */
   gdb_assert (tdesc->xmltarget != NULL
 	      || (!tdesc->features.empty ()
-		  && tdesc->arch != NULL));
+		  && tdesc_architecture_name (tdesc) != nullptr));
 
   if (tdesc->xmltarget == NULL)
     {
diff --git a/gdbserver/tdesc.h b/gdbserver/tdesc.h
index 534b8b000f5..6fc37d038bc 100644
--- a/gdbserver/tdesc.h
+++ b/gdbserver/tdesc.h
@@ -54,18 +54,16 @@ struct target_desc final : tdesc_element
   mutable const char *xmltarget = NULL;
 
   /* The value of <architecture> element in the XML, replying GDB.  */
-  const char *arch = NULL;
+  gdb::unique_xmalloc_ptr<char> arch = nullptr;
 
   /* The value of <osabi> element in the XML, replying GDB.  */
-  const char *osabi = NULL;
+  gdb::unique_xmalloc_ptr<char> osabi = nullptr;
 
 public:
   target_desc ()
     : registers_size (0)
   {}
 
-  ~target_desc ();
-
   bool operator== (const target_desc &other) const;
 
   bool operator!= (const target_desc &other) const
-- 
2.25.4

Re: [PATCH 1/5] gdbserver: make arch and osabi names gdb::unique_xmalloc_ptr<char>

[Simon Marchi]

> diff --git a/gdbserver/tdesc.h b/gdbserver/tdesc.h
> index 534b8b000f5..6fc37d038bc 100644
> --- a/gdbserver/tdesc.h
> +++ b/gdbserver/tdesc.h
> @@ -54,18 +54,16 @@ struct target_desc final : tdesc_element
>    mutable const char *xmltarget = NULL;
>  
>    /* The value of <architecture> element in the XML, replying GDB.  */
> -  const char *arch = NULL;
> +  gdb::unique_xmalloc_ptr<char> arch = nullptr;

Just a nit, you don't need to initialize unique_ptrs to nullptr, they
are default initialized to that.

Simon

Re: [PATCH 1/5] gdbserver: make arch and osabi names gdb::unique_xmalloc_ptr<char>

[Andrew Burgess]

Simon Marchi <simark@simark.ca> writes:

>> diff --git a/gdbserver/tdesc.h b/gdbserver/tdesc.h
>> index 534b8b000f5..6fc37d038bc 100644
>> --- a/gdbserver/tdesc.h
>> +++ b/gdbserver/tdesc.h
>> @@ -54,18 +54,16 @@ struct target_desc final : tdesc_element
>>    mutable const char *xmltarget = NULL;
>>  
>>    /* The value of <architecture> element in the XML, replying GDB.  */
>> -  const char *arch = NULL;
>> +  gdb::unique_xmalloc_ptr<char> arch = nullptr;
>
> Just a nit, you don't need to initialize unique_ptrs to nullptr, they
> are default initialized to that.

Fixed.

Thanks,
Andrew

[PATCH 2/5] gdb: make use of set_tdesc_osabi overload in features/ files

[Andrew Burgess]

There are two versions of the set_tdesc_osabi function in GDB:

  void
  set_tdesc_osabi (struct target_desc *target_desc, const char *name)
  {
    set_tdesc_osabi (target_desc, osabi_from_tdesc_string (name));
  }

  void
  set_tdesc_osabi (struct target_desc *target_desc, enum gdb_osabi osabi)
  {
    target_desc->osabi = osabi;
  }

In the gdb/features/ files we call the second of these functions, like
this:

  set_tdesc_osabi (result.get (), osabi_from_tdesc_string ("GNU/Linux"));

This can be replaced with a call to the first set_tdesc_osabi
function, so lets do that.  I think that this makes the features/ code
slightly simpler and easier to understand.

There should be no user visible changes after this commit.

Approved-By: Tom Tromey <tom@tromey.com>
---
 gdb/features/mips-dsp-linux.c        | 2 +-
 gdb/features/mips-linux.c            | 2 +-
 gdb/features/or1k-linux.c            | 2 +-
 gdb/features/sparc/sparc32-solaris.c | 2 +-
 gdb/features/sparc/sparc64-solaris.c | 2 +-
 gdb/target-descriptions.c            | 2 +-
 6 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/gdb/features/mips-dsp-linux.c b/gdb/features/mips-dsp-linux.c
index d8e40283caf..17d30aac68f 100644
--- a/gdb/features/mips-dsp-linux.c
+++ b/gdb/features/mips-dsp-linux.c
@@ -11,7 +11,7 @@ initialize_tdesc_mips_dsp_linux (void)
   target_desc_up result = allocate_target_description ();
   set_tdesc_architecture (result.get (), bfd_scan_arch ("mips"));
 
-  set_tdesc_osabi (result.get (), osabi_from_tdesc_string ("GNU/Linux"));
+  set_tdesc_osabi (result.get (), "GNU/Linux");
 
   struct tdesc_feature *feature;
 
diff --git a/gdb/features/mips-linux.c b/gdb/features/mips-linux.c
index f93eef5e41d..a43526db24e 100644
--- a/gdb/features/mips-linux.c
+++ b/gdb/features/mips-linux.c
@@ -11,7 +11,7 @@ initialize_tdesc_mips_linux (void)
   target_desc_up result = allocate_target_description ();
   set_tdesc_architecture (result.get (), bfd_scan_arch ("mips"));
 
-  set_tdesc_osabi (result.get (), osabi_from_tdesc_string ("GNU/Linux"));
+  set_tdesc_osabi (result.get (), "GNU/Linux");
 
   struct tdesc_feature *feature;
 
diff --git a/gdb/features/or1k-linux.c b/gdb/features/or1k-linux.c
index 24731458de2..551b38f59c0 100644
--- a/gdb/features/or1k-linux.c
+++ b/gdb/features/or1k-linux.c
@@ -11,7 +11,7 @@ initialize_tdesc_or1k_linux (void)
   target_desc_up result = allocate_target_description ();
   set_tdesc_architecture (result.get (), bfd_scan_arch ("or1k"));
 
-  set_tdesc_osabi (result.get (), osabi_from_tdesc_string ("GNU/Linux"));
+  set_tdesc_osabi (result.get (), "GNU/Linux");
 
   struct tdesc_feature *feature;
 
diff --git a/gdb/features/sparc/sparc32-solaris.c b/gdb/features/sparc/sparc32-solaris.c
index dce96851f96..ce57abaaaf2 100644
--- a/gdb/features/sparc/sparc32-solaris.c
+++ b/gdb/features/sparc/sparc32-solaris.c
@@ -11,7 +11,7 @@ initialize_tdesc_sparc32_solaris (void)
   target_desc_up result = allocate_target_description ();
   set_tdesc_architecture (result.get (), bfd_scan_arch ("sparc"));
 
-  set_tdesc_osabi (result.get (), osabi_from_tdesc_string ("Solaris"));
+  set_tdesc_osabi (result.get (), "Solaris");
 
   struct tdesc_feature *feature;
 
diff --git a/gdb/features/sparc/sparc64-solaris.c b/gdb/features/sparc/sparc64-solaris.c
index d030df63c6c..92cc88cd5cf 100644
--- a/gdb/features/sparc/sparc64-solaris.c
+++ b/gdb/features/sparc/sparc64-solaris.c
@@ -11,7 +11,7 @@ initialize_tdesc_sparc64_solaris (void)
   target_desc_up result = allocate_target_description ();
   set_tdesc_architecture (result.get (), bfd_scan_arch ("sparc:v9"));
 
-  set_tdesc_osabi (result.get (), osabi_from_tdesc_string ("Solaris"));
+  set_tdesc_osabi (result.get (), "Solaris");
 
   struct tdesc_feature *feature;
 
diff --git a/gdb/target-descriptions.c b/gdb/target-descriptions.c
index d78da14918f..c165750c6c5 100644
--- a/gdb/target-descriptions.c
+++ b/gdb/target-descriptions.c
@@ -1318,7 +1318,7 @@ class print_c_tdesc : public tdesc_element_visitor
 	&& tdesc_osabi (e) < GDB_OSABI_INVALID)
       {
 	gdb_printf
-	  ("  set_tdesc_osabi (result.get (), osabi_from_tdesc_string (\"%s\"));\n",
+	  ("  set_tdesc_osabi (result.get (), \"%s\");\n",
 	   gdbarch_osabi_name (tdesc_osabi (e)));
 	gdb_printf ("\n");
       }
-- 
2.25.4

[PATCH 3/5] gdb: split osabi support between gdb/ and gdbsupport/ directories

[Andrew Burgess]

In future commits I want to call set_tdesc_osabi from gdbserver/
code.  Currently the only version of set_tdesc_osabi available to
gdbserver takes a string representing the osabi.

The problem with this is that, having lots of calls to set_tdesc_osabi
which all take a string is an invite for a typo to slip in.  This typo
could potentially go unnoticed until someone tries to debug the wrong
combination of GDB and gdbserver, at which point GDB will fail to find
the correct gdbarch because it doesn't understand the osabi string.

It would be better if the set_tdesc_osabi calls in gdbserver could
take an 'enum gdb_osabi' value and then convert this to the "correct"
string internally.  In this was we are guaranteed to always have a
valid, known, osabi string.

This commit splits the osabi related code, which currently lives
entirely on the GDB wide, between gdb/ and gdbsupport/.  I've moved
the enum definition along with the array of osabi names into
gdbsupport/.  Then all the functions that access the names list, and
which convert between names and enum values are also moved.

I've taken the opportunity of this move to add a '.def' file which
contains all the enum names along with the name strings.  This '.def'
file is then used to create 'enum gdb_osabi' as well as the array of
osabi name strings.  By using a '.def' file we know that the enum
order will always match the name string array.

This commit is just a refactor, there are no user visible changes
after this commit.  This commit doesn't change how gdbserver sets the
target description osabi string, that will come in the next commit.
---
 gdb/osabi.c                 | 91 ----------------------------------
 gdb/osabi.h                 | 41 +---------------
 gdbsupport/Makefile.am      |  1 +
 gdbsupport/Makefile.in      | 15 +++---
 gdbsupport/osabi-common.cc  | 98 +++++++++++++++++++++++++++++++++++++
 gdbsupport/osabi-common.def | 57 +++++++++++++++++++++
 gdbsupport/osabi-common.h   | 54 ++++++++++++++++++++
 7 files changed, 220 insertions(+), 137 deletions(-)
 create mode 100644 gdbsupport/osabi-common.cc
 create mode 100644 gdbsupport/osabi-common.def
 create mode 100644 gdbsupport/osabi-common.h

diff --git a/gdb/osabi.c b/gdb/osabi.c
index 8a1efce4599..6c00228fb4a 100644
--- a/gdb/osabi.c
+++ b/gdb/osabi.c
@@ -42,93 +42,6 @@ static const char *gdb_osabi_available_names[GDB_OSABI_INVALID + 3] = {
 };
 static const char *set_osabi_string;
 
-/* Names associated with each osabi.  */
-
-struct osabi_names
-{
-  /* The "pretty" name.  */
-
-  const char *pretty;
-
-  /* The triplet regexp, or NULL if not known.  */
-
-  const char *regexp;
-};
-
-/* This table matches the indices assigned to enum gdb_osabi.  Keep
-   them in sync.  */
-static const struct osabi_names gdb_osabi_names[] =
-{
-  { "unknown", NULL },
-  { "none", NULL },
-
-  { "SVR4", NULL },
-  { "GNU/Hurd", NULL },
-  { "Solaris", NULL },
-  { "GNU/Linux", "linux(-gnu[^-]*)?" },
-  { "FreeBSD", NULL },
-  { "NetBSD", NULL },
-  { "OpenBSD", NULL },
-  { "WindowsCE", NULL },
-  { "DJGPP", NULL },
-  { "Cygwin", NULL },
-  { "Windows", NULL },
-  { "AIX", NULL },
-  { "DICOS", NULL },
-  { "Darwin", NULL },
-  { "OpenVMS", NULL },
-  { "LynxOS178", NULL },
-  { "Newlib", NULL },
-  { "SDE", NULL },
-  { "PikeOS", NULL },
-
-  { "<invalid>", NULL }
-};
-
-const char *
-gdbarch_osabi_name (enum gdb_osabi osabi)
-{
-  if (osabi >= GDB_OSABI_UNKNOWN && osabi < GDB_OSABI_INVALID)
-    return gdb_osabi_names[osabi].pretty;
-
-  return gdb_osabi_names[GDB_OSABI_INVALID].pretty;
-}
-
-/* See osabi.h.  */
-
-const char *
-osabi_triplet_regexp (enum gdb_osabi osabi)
-{
-  if (osabi >= GDB_OSABI_UNKNOWN && osabi < GDB_OSABI_INVALID)
-    return gdb_osabi_names[osabi].regexp;
-
-  return gdb_osabi_names[GDB_OSABI_INVALID].regexp;
-}
-
-/* Lookup the OS ABI corresponding to the specified target description
-   string.  */
-
-enum gdb_osabi
-osabi_from_tdesc_string (const char *name)
-{
-  int i;
-
-  for (i = 0; i < ARRAY_SIZE (gdb_osabi_names); i++)
-    if (strcmp (name, gdb_osabi_names[i].pretty) == 0)
-      {
-	/* See note above: the name table matches the indices assigned
-	   to enum gdb_osabi.  */
-	enum gdb_osabi osabi = (enum gdb_osabi) i;
-
-	if (osabi == GDB_OSABI_INVALID)
-	  return GDB_OSABI_UNKNOWN;
-	else
-	  return osabi;
-      }
-
-  return GDB_OSABI_UNKNOWN;
-}
-
 /* Handler for a given architecture/OS ABI pair.  There should be only
    one handler for a given OS ABI each architecture family.  */
 struct gdb_osabi_handler
@@ -671,10 +584,6 @@ void _initialize_gdb_osabi ();
 void
 _initialize_gdb_osabi ()
 {
-  if (strcmp (gdb_osabi_names[GDB_OSABI_INVALID].pretty, "<invalid>") != 0)
-    internal_error
-      (_("_initialize_gdb_osabi: gdb_osabi_names[] is inconsistent"));
-
   /* Register a generic sniffer for ELF flavoured files.  */
   gdbarch_register_osabi_sniffer (bfd_arch_unknown,
 				  bfd_target_elf_flavour,
diff --git a/gdb/osabi.h b/gdb/osabi.h
index d2b1a359098..984bdd4e5dc 100644
--- a/gdb/osabi.h
+++ b/gdb/osabi.h
@@ -19,35 +19,7 @@
 #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_CYGWIN,
-  GDB_OSABI_WINDOWS,
-  GDB_OSABI_AIX,
-  GDB_OSABI_DICOS,
-  GDB_OSABI_DARWIN,
-  GDB_OSABI_OPENVMS,
-  GDB_OSABI_LYNXOS178,
-  GDB_OSABI_NEWLIB,
-  GDB_OSABI_SDE,
-  GDB_OSABI_PIKEOS,
-
-  GDB_OSABI_INVALID		/* keep this last */
-};
+#include "gdbsupport/osabi-common.h"
 
 /* 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
@@ -69,23 +41,12 @@ void gdbarch_register_osabi (enum bfd_architecture, unsigned long,
 /* Lookup the OS ABI corresponding to the specified BFD.  */
 enum gdb_osabi gdbarch_lookup_osabi (bfd *);
 
-/* Lookup the OS ABI corresponding to the specified target description
-   string.  */
-enum gdb_osabi osabi_from_tdesc_string (const char *text);
-
 /* Return true if there's an OS ABI handler for INFO.  */
 bool has_gdb_osabi_handler (struct gdbarch_info info);
 
 /* Initialize the gdbarch for the specified OS ABI variant.  */
 void gdbarch_init_osabi (struct gdbarch_info, struct gdbarch *);
 
-/* Return the name of the specified OS ABI.  */
-const char *gdbarch_osabi_name (enum gdb_osabi);
-
-/* Return a regular expression that matches the OS part of a GNU
-   configury triplet for the given OSABI.  */
-const char *osabi_triplet_regexp (enum gdb_osabi osabi);
-
 /* Helper routine for ELF file sniffers.  This looks at ABI tag note
    sections to determine the OS ABI from the note.  */
 void generic_elf_osabi_sniff_abi_tag_sections (bfd *, asection *,
diff --git a/gdbsupport/Makefile.am b/gdbsupport/Makefile.am
index 36e561e015d..d2e2299cc3f 100644
--- a/gdbsupport/Makefile.am
+++ b/gdbsupport/Makefile.am
@@ -76,6 +76,7 @@ libgdbsupport_a_SOURCES = \
     job-control.cc \
     netstuff.cc \
     new-op.cc \
+    osabi-common.cc \
     pathstuff.cc \
     print-utils.cc \
     ptid.cc \
diff --git a/gdbsupport/Makefile.in b/gdbsupport/Makefile.in
index 9cff86bd987..c0d406c3a35 100644
--- a/gdbsupport/Makefile.in
+++ b/gdbsupport/Makefile.in
@@ -162,12 +162,13 @@ am_libgdbsupport_a_OBJECTS = agent.$(OBJEXT) btrace-common.$(OBJEXT) \
 	gdb-dlfcn.$(OBJEXT) gdb_obstack.$(OBJEXT) gdb_regex.$(OBJEXT) \
 	gdb_tilde_expand.$(OBJEXT) gdb_wait.$(OBJEXT) \
 	gdb_vecs.$(OBJEXT) job-control.$(OBJEXT) netstuff.$(OBJEXT) \
-	new-op.$(OBJEXT) pathstuff.$(OBJEXT) print-utils.$(OBJEXT) \
-	ptid.$(OBJEXT) rsp-low.$(OBJEXT) run-time-clock.$(OBJEXT) \
-	safe-strerror.$(OBJEXT) scoped_mmap.$(OBJEXT) search.$(OBJEXT) \
-	signals.$(OBJEXT) signals-state-save-restore.$(OBJEXT) \
-	task-group.$(OBJEXT) tdesc.$(OBJEXT) thread-pool.$(OBJEXT) \
-	xml-utils.$(OBJEXT) $(am__objects_1) $(am__objects_2)
+	new-op.$(OBJEXT) osabi-common.$(OBJEXT) pathstuff.$(OBJEXT) \
+	print-utils.$(OBJEXT) ptid.$(OBJEXT) rsp-low.$(OBJEXT) \
+	run-time-clock.$(OBJEXT) safe-strerror.$(OBJEXT) \
+	scoped_mmap.$(OBJEXT) search.$(OBJEXT) signals.$(OBJEXT) \
+	signals-state-save-restore.$(OBJEXT) task-group.$(OBJEXT) \
+	tdesc.$(OBJEXT) thread-pool.$(OBJEXT) xml-utils.$(OBJEXT) \
+	$(am__objects_1) $(am__objects_2)
 libgdbsupport_a_OBJECTS = $(am_libgdbsupport_a_OBJECTS)
 AM_V_P = $(am__v_P_@AM_V@)
 am__v_P_ = $(am__v_P_@AM_DEFAULT_V@)
@@ -433,6 +434,7 @@ libgdbsupport_a_SOURCES = \
     job-control.cc \
     netstuff.cc \
     new-op.cc \
+    osabi-common.cc \
     pathstuff.cc \
     print-utils.cc \
     ptid.cc \
@@ -542,6 +544,7 @@ distclean-compile:
 @AMDEP_TRUE@@am__include@ @am__quote@./$(DEPDIR)/job-control.Po@am__quote@
 @AMDEP_TRUE@@am__include@ @am__quote@./$(DEPDIR)/netstuff.Po@am__quote@
 @AMDEP_TRUE@@am__include@ @am__quote@./$(DEPDIR)/new-op.Po@am__quote@
+@AMDEP_TRUE@@am__include@ @am__quote@./$(DEPDIR)/osabi-common.Po@am__quote@
 @AMDEP_TRUE@@am__include@ @am__quote@./$(DEPDIR)/pathstuff.Po@am__quote@
 @AMDEP_TRUE@@am__include@ @am__quote@./$(DEPDIR)/print-utils.Po@am__quote@
 @AMDEP_TRUE@@am__include@ @am__quote@./$(DEPDIR)/ptid.Po@am__quote@
diff --git a/gdbsupport/osabi-common.cc b/gdbsupport/osabi-common.cc
new file mode 100644
index 00000000000..785eb478664
--- /dev/null
+++ b/gdbsupport/osabi-common.cc
@@ -0,0 +1,98 @@
+/* OS ABI variant handling for GDB and gdbserver.
+
+   Copyright (C) 2001-2024 Free Software Foundation, Inc.
+
+   This file is part of GDB.
+
+   This program is free software; you can redistribute it and/or modify
+   it under the terms of the GNU General Public License as published by
+   the Free Software Foundation; either version 3 of the License, or
+   (at your option) any later version.
+
+   This program is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+   GNU General Public License for more details.
+
+   You should have received a copy of the GNU General Public License
+   along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
+
+#include "gdbsupport/osabi-common.h"
+
+/* Names associated with each osabi.  */
+
+struct osabi_names
+{
+  /* The "pretty" name.  */
+
+  const char *pretty;
+
+  /* The triplet regexp, or NULL if not known.  */
+
+  const char *regexp;
+};
+
+/* This table matches the indices assigned to enum gdb_osabi.  Keep
+   them in sync.  */
+static const struct osabi_names gdb_osabi_names[] =
+{
+#define GDB_OSABI_DEF_FIRST(Enum, Name, Regex)	\
+  { Name, Regex },
+
+#define GDB_OSABI_DEF(Enum, Name, Regex)	\
+  { Name, Regex },
+
+#define GDB_OSABI_DEF_LAST(Enum, Name, Regex)	\
+  { Name, Regex }
+
+#include "gdbsupport/osabi-common.def"
+
+#undef GDB_OSABI_DEF_LAST
+#undef GDB_OSABI_DEF
+#undef GDB_OSABI_DEF_FIRST
+};
+
+/* See gdbsupport/osabi-common.h.  */
+
+const char *
+gdbarch_osabi_name (enum gdb_osabi osabi)
+{
+  if (osabi >= GDB_OSABI_UNKNOWN && osabi < GDB_OSABI_INVALID)
+    return gdb_osabi_names[osabi].pretty;
+
+  return gdb_osabi_names[GDB_OSABI_INVALID].pretty;
+}
+
+/* See gdbsupport/osabi-commomn.h.  */
+
+enum gdb_osabi
+osabi_from_tdesc_string (const char *name)
+{
+  int i;
+
+  for (i = 0; i < ARRAY_SIZE (gdb_osabi_names); i++)
+    if (strcmp (name, gdb_osabi_names[i].pretty) == 0)
+      {
+	/* See note above: the name table matches the indices assigned
+	   to enum gdb_osabi.  */
+	enum gdb_osabi osabi = (enum gdb_osabi) i;
+
+	if (osabi == GDB_OSABI_INVALID)
+	  return GDB_OSABI_UNKNOWN;
+	else
+	  return osabi;
+      }
+
+  return GDB_OSABI_UNKNOWN;
+}
+
+/* See gdbsupport/osabi-common.h.  */
+
+const char *
+osabi_triplet_regexp (enum gdb_osabi osabi)
+{
+  if (osabi >= GDB_OSABI_UNKNOWN && osabi < GDB_OSABI_INVALID)
+    return gdb_osabi_names[osabi].regexp;
+
+  return gdb_osabi_names[GDB_OSABI_INVALID].regexp;
+}
diff --git a/gdbsupport/osabi-common.def b/gdbsupport/osabi-common.def
new file mode 100644
index 00000000000..637da26a050
--- /dev/null
+++ b/gdbsupport/osabi-common.def
@@ -0,0 +1,57 @@
+/* OS ABI variant definitions for GDB and gdbserver.
+
+   Copyright (C) 2001-2024 Free Software Foundation, Inc.
+
+   This file is part of GDB.
+
+   This program is free software; you can redistribute it and/or modify
+   it under the terms of the GNU General Public License as published by
+   the Free Software Foundation; either version 3 of the License, or
+   (at your option) any later version.
+
+   This program is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+   GNU General Public License for more details.
+
+   You should have received a copy of the GNU General Public License
+   along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
+
+/* Each definition in this file is an osabi known to GDB.
+
+   The first argument is used to create the enum name and is appended
+   to 'GDB_OSABI_'.
+
+   The second argument is the osabi name.  These strings can't be
+   changed _ever_ as gdbserver will emit these.  Changing these
+   strings would break compatibility with already released versions of
+   GDB and/or gdbserver.
+
+   The third argument is a regexp which matches against a target
+   triplet.  */
+
+GDB_OSABI_DEF_FIRST (UNKNOWN, "unknown", nullptr)
+
+GDB_OSABI_DEF (NONE, "none", nullptr)
+
+GDB_OSABI_DEF (SVR4, "SVR4", nullptr)
+GDB_OSABI_DEF (HURD, "GNU/Hurd", nullptr)
+GDB_OSABI_DEF (SOLARIS, "Solaris", nullptr)
+GDB_OSABI_DEF (LINUX, "GNU/Linux", "linux(-gnu[^-]*)?")
+GDB_OSABI_DEF (FREEBSD, "FreeBSD", nullptr)
+GDB_OSABI_DEF (NETBSD, "NetBSD", nullptr)
+GDB_OSABI_DEF (OPENBSD, "OpenBSD", nullptr)
+GDB_OSABI_DEF (WINCE, "WindowsCE", nullptr)
+GDB_OSABI_DEF (GO32, "DJGPP", nullptr)
+GDB_OSABI_DEF (CYGWIN, "Cygwin", nullptr)
+GDB_OSABI_DEF (WINDOWS, "Windows", nullptr)
+GDB_OSABI_DEF (AIX, "AIX", nullptr)
+GDB_OSABI_DEF (DICOS, "DICOS", nullptr)
+GDB_OSABI_DEF (DARWIN, "Darwin", nullptr)
+GDB_OSABI_DEF (OPENVMS, "OpenVMS", nullptr)
+GDB_OSABI_DEF (LYNXOS178, "LynxOS178", nullptr)
+GDB_OSABI_DEF (NEWLIB, "Newlib", nullptr)
+GDB_OSABI_DEF (SDE, "SDE", nullptr)
+GDB_OSABI_DEF (PIKEOS, "PikeOS", nullptr)
+
+GDB_OSABI_DEF_LAST (INVALID, "<invalid>", nullptr)
diff --git a/gdbsupport/osabi-common.h b/gdbsupport/osabi-common.h
new file mode 100644
index 00000000000..99318eb0ca7
--- /dev/null
+++ b/gdbsupport/osabi-common.h
@@ -0,0 +1,54 @@
+/* OS ABI variant handling for GDB and gdbserver.
+
+   Copyright (C) 2001-2024 Free Software Foundation, Inc.
+
+   This file is part of GDB.
+
+   This program is free software; you can redistribute it and/or modify
+   it under the terms of the GNU General Public License as published by
+   the Free Software Foundation; either version 3 of the License, or
+   (at your option) any later version.
+
+   This program is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+   GNU General Public License for more details.
+
+   You should have received a copy of the GNU General Public License
+   along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
+
+#ifndef OSABI_COMMON_H
+#define OSABI_COMMON_H
+
+/* List of known OS ABIs.  If you change this, make sure to update the
+   table in osabi-common.cc.  */
+enum gdb_osabi
+{
+#define GDB_OSABI_DEF_FIRST(Enum, Name, Regex)	\
+  GDB_OSABI_ ## Enum = 0,
+
+#define GDB_OSABI_DEF(Enum, Name, Regex)	\
+  GDB_OSABI_ ## Enum,
+
+#define GDB_OSABI_DEF_LAST(Enum, Name, Regex)	\
+  GDB_OSABI_ ## Enum
+
+#include "gdbsupport/osabi-common.def"
+
+#undef GDB_OSABI_DEF_LAST
+#undef GDB_OSABI_DEF
+#undef GDB_OSABI_DEF_FIRST
+};
+
+/* Lookup the OS ABI corresponding to the specified target description
+   string.  */
+enum gdb_osabi osabi_from_tdesc_string (const char *text);
+
+/* Return the name of the specified OS ABI.  */
+const char *gdbarch_osabi_name (enum gdb_osabi);
+
+/* Return a regular expression that matches the OS part of a GNU
+   configury triplet for the given OSABI.  */
+const char *osabi_triplet_regexp (enum gdb_osabi osabi);
+
+#endif /* OSABI_COMMON_H */
-- 
2.25.4

Re: [PATCH 3/5] gdb: split osabi support between gdb/ and gdbsupport/ directories

[Luis Machado]

On 10/8/24 18:11, Andrew Burgess wrote:
> In future commits I want to call set_tdesc_osabi from gdbserver/
> code.  Currently the only version of set_tdesc_osabi available to
> gdbserver takes a string representing the osabi.
> 
> The problem with this is that, having lots of calls to set_tdesc_osabi
> which all take a string is an invite for a typo to slip in.  This typo
> could potentially go unnoticed until someone tries to debug the wrong
> combination of GDB and gdbserver, at which point GDB will fail to find
> the correct gdbarch because it doesn't understand the osabi string.
> 
> It would be better if the set_tdesc_osabi calls in gdbserver could
> take an 'enum gdb_osabi' value and then convert this to the "correct"
> string internally.  In this was we are guaranteed to always have a
> valid, known, osabi string.
> 
> This commit splits the osabi related code, which currently lives
> entirely on the GDB wide, between gdb/ and gdbsupport/.  I've moved
> the enum definition along with the array of osabi names into
> gdbsupport/.  Then all the functions that access the names list, and
> which convert between names and enum values are also moved.
> 
> I've taken the opportunity of this move to add a '.def' file which
> contains all the enum names along with the name strings.  This '.def'
> file is then used to create 'enum gdb_osabi' as well as the array of
> osabi name strings.  By using a '.def' file we know that the enum
> order will always match the name string array.
> 
> This commit is just a refactor, there are no user visible changes
> after this commit.  This commit doesn't change how gdbserver sets the
> target description osabi string, that will come in the next commit.
> ---
>  gdb/osabi.c                 | 91 ----------------------------------
>  gdb/osabi.h                 | 41 +---------------
>  gdbsupport/Makefile.am      |  1 +
>  gdbsupport/Makefile.in      | 15 +++---
>  gdbsupport/osabi-common.cc  | 98 +++++++++++++++++++++++++++++++++++++
>  gdbsupport/osabi-common.def | 57 +++++++++++++++++++++
>  gdbsupport/osabi-common.h   | 54 ++++++++++++++++++++
>  7 files changed, 220 insertions(+), 137 deletions(-)
>  create mode 100644 gdbsupport/osabi-common.cc
>  create mode 100644 gdbsupport/osabi-common.def
>  create mode 100644 gdbsupport/osabi-common.h
> 
> diff --git a/gdb/osabi.c b/gdb/osabi.c
> index 8a1efce4599..6c00228fb4a 100644
> --- a/gdb/osabi.c
> +++ b/gdb/osabi.c
> @@ -42,93 +42,6 @@ static const char *gdb_osabi_available_names[GDB_OSABI_INVALID + 3] = {
>  };
>  static const char *set_osabi_string;
>  
> -/* Names associated with each osabi.  */
> -
> -struct osabi_names
> -{
> -  /* The "pretty" name.  */
> -
> -  const char *pretty;
> -
> -  /* The triplet regexp, or NULL if not known.  */
> -
> -  const char *regexp;
> -};
> -
> -/* This table matches the indices assigned to enum gdb_osabi.  Keep
> -   them in sync.  */
> -static const struct osabi_names gdb_osabi_names[] =
> -{
> -  { "unknown", NULL },
> -  { "none", NULL },
> -
> -  { "SVR4", NULL },
> -  { "GNU/Hurd", NULL },
> -  { "Solaris", NULL },
> -  { "GNU/Linux", "linux(-gnu[^-]*)?" },
> -  { "FreeBSD", NULL },
> -  { "NetBSD", NULL },
> -  { "OpenBSD", NULL },
> -  { "WindowsCE", NULL },
> -  { "DJGPP", NULL },
> -  { "Cygwin", NULL },
> -  { "Windows", NULL },
> -  { "AIX", NULL },
> -  { "DICOS", NULL },
> -  { "Darwin", NULL },
> -  { "OpenVMS", NULL },
> -  { "LynxOS178", NULL },
> -  { "Newlib", NULL },
> -  { "SDE", NULL },
> -  { "PikeOS", NULL },
> -
> -  { "<invalid>", NULL }
> -};
> -
> -const char *
> -gdbarch_osabi_name (enum gdb_osabi osabi)
> -{
> -  if (osabi >= GDB_OSABI_UNKNOWN && osabi < GDB_OSABI_INVALID)
> -    return gdb_osabi_names[osabi].pretty;
> -
> -  return gdb_osabi_names[GDB_OSABI_INVALID].pretty;
> -}
> -
> -/* See osabi.h.  */
> -
> -const char *
> -osabi_triplet_regexp (enum gdb_osabi osabi)
> -{
> -  if (osabi >= GDB_OSABI_UNKNOWN && osabi < GDB_OSABI_INVALID)
> -    return gdb_osabi_names[osabi].regexp;
> -
> -  return gdb_osabi_names[GDB_OSABI_INVALID].regexp;
> -}
> -
> -/* Lookup the OS ABI corresponding to the specified target description
> -   string.  */
> -
> -enum gdb_osabi
> -osabi_from_tdesc_string (const char *name)
> -{
> -  int i;
> -
> -  for (i = 0; i < ARRAY_SIZE (gdb_osabi_names); i++)
> -    if (strcmp (name, gdb_osabi_names[i].pretty) == 0)
> -      {
> -	/* See note above: the name table matches the indices assigned
> -	   to enum gdb_osabi.  */
> -	enum gdb_osabi osabi = (enum gdb_osabi) i;
> -
> -	if (osabi == GDB_OSABI_INVALID)
> -	  return GDB_OSABI_UNKNOWN;
> -	else
> -	  return osabi;
> -      }
> -
> -  return GDB_OSABI_UNKNOWN;
> -}
> -
>  /* Handler for a given architecture/OS ABI pair.  There should be only
>     one handler for a given OS ABI each architecture family.  */
>  struct gdb_osabi_handler
> @@ -671,10 +584,6 @@ void _initialize_gdb_osabi ();
>  void
>  _initialize_gdb_osabi ()
>  {
> -  if (strcmp (gdb_osabi_names[GDB_OSABI_INVALID].pretty, "<invalid>") != 0)
> -    internal_error
> -      (_("_initialize_gdb_osabi: gdb_osabi_names[] is inconsistent"));
> -
>    /* Register a generic sniffer for ELF flavoured files.  */
>    gdbarch_register_osabi_sniffer (bfd_arch_unknown,
>  				  bfd_target_elf_flavour,
> diff --git a/gdb/osabi.h b/gdb/osabi.h
> index d2b1a359098..984bdd4e5dc 100644
> --- a/gdb/osabi.h
> +++ b/gdb/osabi.h
> @@ -19,35 +19,7 @@
>  #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_CYGWIN,
> -  GDB_OSABI_WINDOWS,
> -  GDB_OSABI_AIX,
> -  GDB_OSABI_DICOS,
> -  GDB_OSABI_DARWIN,
> -  GDB_OSABI_OPENVMS,
> -  GDB_OSABI_LYNXOS178,
> -  GDB_OSABI_NEWLIB,
> -  GDB_OSABI_SDE,
> -  GDB_OSABI_PIKEOS,
> -
> -  GDB_OSABI_INVALID		/* keep this last */
> -};
> +#include "gdbsupport/osabi-common.h"
>  
>  /* 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
> @@ -69,23 +41,12 @@ void gdbarch_register_osabi (enum bfd_architecture, unsigned long,
>  /* Lookup the OS ABI corresponding to the specified BFD.  */
>  enum gdb_osabi gdbarch_lookup_osabi (bfd *);
>  
> -/* Lookup the OS ABI corresponding to the specified target description
> -   string.  */
> -enum gdb_osabi osabi_from_tdesc_string (const char *text);
> -
>  /* Return true if there's an OS ABI handler for INFO.  */
>  bool has_gdb_osabi_handler (struct gdbarch_info info);
>  
>  /* Initialize the gdbarch for the specified OS ABI variant.  */
>  void gdbarch_init_osabi (struct gdbarch_info, struct gdbarch *);
>  
> -/* Return the name of the specified OS ABI.  */
> -const char *gdbarch_osabi_name (enum gdb_osabi);
> -
> -/* Return a regular expression that matches the OS part of a GNU
> -   configury triplet for the given OSABI.  */
> -const char *osabi_triplet_regexp (enum gdb_osabi osabi);
> -
>  /* Helper routine for ELF file sniffers.  This looks at ABI tag note
>     sections to determine the OS ABI from the note.  */
>  void generic_elf_osabi_sniff_abi_tag_sections (bfd *, asection *,
> diff --git a/gdbsupport/Makefile.am b/gdbsupport/Makefile.am
> index 36e561e015d..d2e2299cc3f 100644
> --- a/gdbsupport/Makefile.am
> +++ b/gdbsupport/Makefile.am
> @@ -76,6 +76,7 @@ libgdbsupport_a_SOURCES = \
>      job-control.cc \
>      netstuff.cc \
>      new-op.cc \
> +    osabi-common.cc \
>      pathstuff.cc \
>      print-utils.cc \
>      ptid.cc \
> diff --git a/gdbsupport/Makefile.in b/gdbsupport/Makefile.in
> index 9cff86bd987..c0d406c3a35 100644
> --- a/gdbsupport/Makefile.in
> +++ b/gdbsupport/Makefile.in
> @@ -162,12 +162,13 @@ am_libgdbsupport_a_OBJECTS = agent.$(OBJEXT) btrace-common.$(OBJEXT) \
>  	gdb-dlfcn.$(OBJEXT) gdb_obstack.$(OBJEXT) gdb_regex.$(OBJEXT) \
>  	gdb_tilde_expand.$(OBJEXT) gdb_wait.$(OBJEXT) \
>  	gdb_vecs.$(OBJEXT) job-control.$(OBJEXT) netstuff.$(OBJEXT) \
> -	new-op.$(OBJEXT) pathstuff.$(OBJEXT) print-utils.$(OBJEXT) \
> -	ptid.$(OBJEXT) rsp-low.$(OBJEXT) run-time-clock.$(OBJEXT) \
> -	safe-strerror.$(OBJEXT) scoped_mmap.$(OBJEXT) search.$(OBJEXT) \
> -	signals.$(OBJEXT) signals-state-save-restore.$(OBJEXT) \
> -	task-group.$(OBJEXT) tdesc.$(OBJEXT) thread-pool.$(OBJEXT) \
> -	xml-utils.$(OBJEXT) $(am__objects_1) $(am__objects_2)
> +	new-op.$(OBJEXT) osabi-common.$(OBJEXT) pathstuff.$(OBJEXT) \
> +	print-utils.$(OBJEXT) ptid.$(OBJEXT) rsp-low.$(OBJEXT) \
> +	run-time-clock.$(OBJEXT) safe-strerror.$(OBJEXT) \
> +	scoped_mmap.$(OBJEXT) search.$(OBJEXT) signals.$(OBJEXT) \
> +	signals-state-save-restore.$(OBJEXT) task-group.$(OBJEXT) \
> +	tdesc.$(OBJEXT) thread-pool.$(OBJEXT) xml-utils.$(OBJEXT) \
> +	$(am__objects_1) $(am__objects_2)
>  libgdbsupport_a_OBJECTS = $(am_libgdbsupport_a_OBJECTS)
>  AM_V_P = $(am__v_P_@AM_V@)
>  am__v_P_ = $(am__v_P_@AM_DEFAULT_V@)
> @@ -433,6 +434,7 @@ libgdbsupport_a_SOURCES = \
>      job-control.cc \
>      netstuff.cc \
>      new-op.cc \
> +    osabi-common.cc \
>      pathstuff.cc \
>      print-utils.cc \
>      ptid.cc \
> @@ -542,6 +544,7 @@ distclean-compile:
>  @AMDEP_TRUE@@am__include@ @am__quote@./$(DEPDIR)/job-control.Po@am__quote@
>  @AMDEP_TRUE@@am__include@ @am__quote@./$(DEPDIR)/netstuff.Po@am__quote@
>  @AMDEP_TRUE@@am__include@ @am__quote@./$(DEPDIR)/new-op.Po@am__quote@
> +@AMDEP_TRUE@@am__include@ @am__quote@./$(DEPDIR)/osabi-common.Po@am__quote@
>  @AMDEP_TRUE@@am__include@ @am__quote@./$(DEPDIR)/pathstuff.Po@am__quote@
>  @AMDEP_TRUE@@am__include@ @am__quote@./$(DEPDIR)/print-utils.Po@am__quote@
>  @AMDEP_TRUE@@am__include@ @am__quote@./$(DEPDIR)/ptid.Po@am__quote@
> diff --git a/gdbsupport/osabi-common.cc b/gdbsupport/osabi-common.cc
> new file mode 100644
> index 00000000000..785eb478664
> --- /dev/null
> +++ b/gdbsupport/osabi-common.cc
> @@ -0,0 +1,98 @@
> +/* OS ABI variant handling for GDB and gdbserver.
> +
> +   Copyright (C) 2001-2024 Free Software Foundation, Inc.
> +
> +   This file is part of GDB.
> +
> +   This program is free software; you can redistribute it and/or modify
> +   it under the terms of the GNU General Public License as published by
> +   the Free Software Foundation; either version 3 of the License, or
> +   (at your option) any later version.
> +
> +   This program is distributed in the hope that it will be useful,
> +   but WITHOUT ANY WARRANTY; without even the implied warranty of
> +   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> +   GNU General Public License for more details.
> +
> +   You should have received a copy of the GNU General Public License
> +   along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
> +
> +#include "gdbsupport/osabi-common.h"
> +
> +/* Names associated with each osabi.  */
> +
> +struct osabi_names
> +{
> +  /* The "pretty" name.  */
> +
> +  const char *pretty;
> +
> +  /* The triplet regexp, or NULL if not known.  */
> +
> +  const char *regexp;
> +};
> +
> +/* This table matches the indices assigned to enum gdb_osabi.  Keep
> +   them in sync.  */
> +static const struct osabi_names gdb_osabi_names[] =
> +{
> +#define GDB_OSABI_DEF_FIRST(Enum, Name, Regex)	\
> +  { Name, Regex },
> +
> +#define GDB_OSABI_DEF(Enum, Name, Regex)	\
> +  { Name, Regex },
> +
> +#define GDB_OSABI_DEF_LAST(Enum, Name, Regex)	\
> +  { Name, Regex }
> +
> +#include "gdbsupport/osabi-common.def"
> +
> +#undef GDB_OSABI_DEF_LAST
> +#undef GDB_OSABI_DEF
> +#undef GDB_OSABI_DEF_FIRST
> +};
> +
> +/* See gdbsupport/osabi-common.h.  */
> +
> +const char *
> +gdbarch_osabi_name (enum gdb_osabi osabi)
> +{
> +  if (osabi >= GDB_OSABI_UNKNOWN && osabi < GDB_OSABI_INVALID)
> +    return gdb_osabi_names[osabi].pretty;
> +
> +  return gdb_osabi_names[GDB_OSABI_INVALID].pretty;
> +}
> +
> +/* See gdbsupport/osabi-commomn.h.  */
> +
> +enum gdb_osabi
> +osabi_from_tdesc_string (const char *name)
> +{
> +  int i;
> +
> +  for (i = 0; i < ARRAY_SIZE (gdb_osabi_names); i++)
> +    if (strcmp (name, gdb_osabi_names[i].pretty) == 0)
> +      {
> +	/* See note above: the name table matches the indices assigned
> +	   to enum gdb_osabi.  */
> +	enum gdb_osabi osabi = (enum gdb_osabi) i;
> +
> +	if (osabi == GDB_OSABI_INVALID)
> +	  return GDB_OSABI_UNKNOWN;
> +	else
> +	  return osabi;
> +      }
> +
> +  return GDB_OSABI_UNKNOWN;
> +}
> +
> +/* See gdbsupport/osabi-common.h.  */
> +
> +const char *
> +osabi_triplet_regexp (enum gdb_osabi osabi)
> +{
> +  if (osabi >= GDB_OSABI_UNKNOWN && osabi < GDB_OSABI_INVALID)
> +    return gdb_osabi_names[osabi].regexp;
> +
> +  return gdb_osabi_names[GDB_OSABI_INVALID].regexp;
> +}
> diff --git a/gdbsupport/osabi-common.def b/gdbsupport/osabi-common.def
> new file mode 100644
> index 00000000000..637da26a050
> --- /dev/null
> +++ b/gdbsupport/osabi-common.def
> @@ -0,0 +1,57 @@
> +/* OS ABI variant definitions for GDB and gdbserver.
> +
> +   Copyright (C) 2001-2024 Free Software Foundation, Inc.
> +
> +   This file is part of GDB.
> +
> +   This program is free software; you can redistribute it and/or modify
> +   it under the terms of the GNU General Public License as published by
> +   the Free Software Foundation; either version 3 of the License, or
> +   (at your option) any later version.
> +
> +   This program is distributed in the hope that it will be useful,
> +   but WITHOUT ANY WARRANTY; without even the implied warranty of
> +   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> +   GNU General Public License for more details.
> +
> +   You should have received a copy of the GNU General Public License
> +   along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
> +
> +/* Each definition in this file is an osabi known to GDB.
> +
> +   The first argument is used to create the enum name and is appended
> +   to 'GDB_OSABI_'.
> +
> +   The second argument is the osabi name.  These strings can't be
> +   changed _ever_ as gdbserver will emit these.  Changing these
> +   strings would break compatibility with already released versions of
> +   GDB and/or gdbserver.
> +
> +   The third argument is a regexp which matches against a target
> +   triplet.  */
> +
> +GDB_OSABI_DEF_FIRST (UNKNOWN, "unknown", nullptr)
> +
> +GDB_OSABI_DEF (NONE, "none", nullptr)
> +
> +GDB_OSABI_DEF (SVR4, "SVR4", nullptr)
> +GDB_OSABI_DEF (HURD, "GNU/Hurd", nullptr)
> +GDB_OSABI_DEF (SOLARIS, "Solaris", nullptr)
> +GDB_OSABI_DEF (LINUX, "GNU/Linux", "linux(-gnu[^-]*)?")
> +GDB_OSABI_DEF (FREEBSD, "FreeBSD", nullptr)
> +GDB_OSABI_DEF (NETBSD, "NetBSD", nullptr)
> +GDB_OSABI_DEF (OPENBSD, "OpenBSD", nullptr)
> +GDB_OSABI_DEF (WINCE, "WindowsCE", nullptr)
> +GDB_OSABI_DEF (GO32, "DJGPP", nullptr)
> +GDB_OSABI_DEF (CYGWIN, "Cygwin", nullptr)
> +GDB_OSABI_DEF (WINDOWS, "Windows", nullptr)
> +GDB_OSABI_DEF (AIX, "AIX", nullptr)
> +GDB_OSABI_DEF (DICOS, "DICOS", nullptr)
> +GDB_OSABI_DEF (DARWIN, "Darwin", nullptr)
> +GDB_OSABI_DEF (OPENVMS, "OpenVMS", nullptr)
> +GDB_OSABI_DEF (LYNXOS178, "LynxOS178", nullptr)
> +GDB_OSABI_DEF (NEWLIB, "Newlib", nullptr)
> +GDB_OSABI_DEF (SDE, "SDE", nullptr)
> +GDB_OSABI_DEF (PIKEOS, "PikeOS", nullptr)
> +
> +GDB_OSABI_DEF_LAST (INVALID, "<invalid>", nullptr)
> diff --git a/gdbsupport/osabi-common.h b/gdbsupport/osabi-common.h
> new file mode 100644
> index 00000000000..99318eb0ca7
> --- /dev/null
> +++ b/gdbsupport/osabi-common.h
> @@ -0,0 +1,54 @@
> +/* OS ABI variant handling for GDB and gdbserver.
> +
> +   Copyright (C) 2001-2024 Free Software Foundation, Inc.
> +
> +   This file is part of GDB.
> +
> +   This program is free software; you can redistribute it and/or modify
> +   it under the terms of the GNU General Public License as published by
> +   the Free Software Foundation; either version 3 of the License, or
> +   (at your option) any later version.
> +
> +   This program is distributed in the hope that it will be useful,
> +   but WITHOUT ANY WARRANTY; without even the implied warranty of
> +   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> +   GNU General Public License for more details.
> +
> +   You should have received a copy of the GNU General Public License
> +   along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
> +
> +#ifndef OSABI_COMMON_H
> +#define OSABI_COMMON_H
> +
> +/* List of known OS ABIs.  If you change this, make sure to update the
> +   table in osabi-common.cc.  */
> +enum gdb_osabi
> +{
> +#define GDB_OSABI_DEF_FIRST(Enum, Name, Regex)	\
> +  GDB_OSABI_ ## Enum = 0,
> +
> +#define GDB_OSABI_DEF(Enum, Name, Regex)	\
> +  GDB_OSABI_ ## Enum,
> +
> +#define GDB_OSABI_DEF_LAST(Enum, Name, Regex)	\
> +  GDB_OSABI_ ## Enum
> +
> +#include "gdbsupport/osabi-common.def"
> +
> +#undef GDB_OSABI_DEF_LAST
> +#undef GDB_OSABI_DEF
> +#undef GDB_OSABI_DEF_FIRST
> +};
> +
> +/* Lookup the OS ABI corresponding to the specified target description
> +   string.  */
> +enum gdb_osabi osabi_from_tdesc_string (const char *text);
> +
> +/* Return the name of the specified OS ABI.  */
> +const char *gdbarch_osabi_name (enum gdb_osabi);
> +
> +/* Return a regular expression that matches the OS part of a GNU
> +   configury triplet for the given OSABI.  */
> +const char *osabi_triplet_regexp (enum gdb_osabi osabi);
> +
> +#endif /* OSABI_COMMON_H */

Looks good. Thanks for making the change.

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

Re: [PATCH 3/5] gdb: split osabi support between gdb/ and gdbsupport/ directories

[Simon Marchi]

On 2024-10-08 13:11, Andrew Burgess wrote:
> In future commits I want to call set_tdesc_osabi from gdbserver/
> code.  Currently the only version of set_tdesc_osabi available to
> gdbserver takes a string representing the osabi.
> 
> The problem with this is that, having lots of calls to set_tdesc_osabi
> which all take a string is an invite for a typo to slip in.  This typo
> could potentially go unnoticed until someone tries to debug the wrong
> combination of GDB and gdbserver, at which point GDB will fail to find
> the correct gdbarch because it doesn't understand the osabi string.
> 
> It would be better if the set_tdesc_osabi calls in gdbserver could
> take an 'enum gdb_osabi' value and then convert this to the "correct"
> string internally.  In this was we are guaranteed to always have a

was -> way

> valid, known, osabi string.
> 
> This commit splits the osabi related code, which currently lives
> entirely on the GDB wide, between gdb/ and gdbsupport/.  I've moved

wide -> side

> the enum definition along with the array of osabi names into
> gdbsupport/.  Then all the functions that access the names list, and
> which convert between names and enum values are also moved.
> 
> I've taken the opportunity of this move to add a '.def' file which
> contains all the enum names along with the name strings.  This '.def'
> file is then used to create 'enum gdb_osabi' as well as the array of
> osabi name strings.  By using a '.def' file we know that the enum
> order will always match the name string array.
> 
> This commit is just a refactor, there are no user visible changes
> after this commit.  This commit doesn't change how gdbserver sets the
> target description osabi string, that will come in the next commit.
> ---
>  gdb/osabi.c                 | 91 ----------------------------------
>  gdb/osabi.h                 | 41 +---------------
>  gdbsupport/Makefile.am      |  1 +
>  gdbsupport/Makefile.in      | 15 +++---
>  gdbsupport/osabi-common.cc  | 98 +++++++++++++++++++++++++++++++++++++
>  gdbsupport/osabi-common.def | 57 +++++++++++++++++++++
>  gdbsupport/osabi-common.h   | 54 ++++++++++++++++++++

Naming nit: do we really need / want "common" in the gdbsupport file
names?  I think that the file being in gdbsupport already says it's
available to both gdb and gdbserver, so naming "common" seems
redundant (I know we have a bunch of files called "common" already).

> +const char *
> +gdbarch_osabi_name (enum gdb_osabi osabi)
> +{
> +  if (osabi >= GDB_OSABI_UNKNOWN && osabi < GDB_OSABI_INVALID)
> +    return gdb_osabi_names[osabi].pretty;
> +
> +  return gdb_osabi_names[GDB_OSABI_INVALID].pretty;
> +}
> +
> +/* See gdbsupport/osabi-commomn.h.  */

commomn -> common

Simon

[PATCH 4/5] gdb/gdbserver: change shared set_tdesc_osabi to take gdb_osabi

[Andrew Burgess]

There is a single declaration of set_tdesc_osabi that is shared
between gdbserver/ and gdb/, this declaration takes a 'const char *'
argument which is the string representing an osabi.

Then in gdb/ we have an overload of set_tdesc_osabi which takes an
'enum gdb_osabi'.

In this commit I change the shared set_tdesc_osabi to be the version
which takes an 'enum gdb_osabi', and I remove the version which takes
a 'const char *'.  All users of set_tdesc_osabi are updated to pass an
'enum gdb_osabi'.

The features/ code, which is generated from the xml files, requires a
new function to be added to osabi.{c,h} which can return a string
representation of an 'enum gdb_osabi'.  With that new function in
place the features/ code is regenerated.

This change is being made to support the next commit.  In the next
commit gdbserver will be updated to call set_tdesc_osabi in more
cases.  The problem is that gdbserver stores the osabi as a string.
The issue here is that a typo in the gdbserver/ code might go
unnoticed and result in gdbserver sending back an invalid osabi
string.

To fix this we want gdbserver to pass an 'enum gdb_osabi' to the
set_tdesc_osabi function.  With that requirement in place it seems to
make sense if all calls to set_tdesc_osabi pass an 'enum gdb_osabi'.

There should be no user visible changes after this commit.
---
 gdb/arch/amd64.c                     |  3 ++-
 gdb/arch/i386.c                      |  3 ++-
 gdb/arch/tic6x.c                     |  3 ++-
 gdb/features/mips-dsp-linux.c        |  2 +-
 gdb/features/mips-linux.c            |  2 +-
 gdb/features/or1k-linux.c            |  2 +-
 gdb/features/sparc/sparc32-solaris.c |  2 +-
 gdb/features/sparc/sparc64-solaris.c |  2 +-
 gdb/osabi.c                          | 29 ++++++++++++++++++++++++++++
 gdb/osabi.h                          |  4 ++++
 gdb/target-descriptions.c            | 11 ++---------
 gdb/target-descriptions.h            |  1 -
 gdbserver/tdesc.cc                   |  3 ++-
 gdbsupport/tdesc.h                   |  4 +++-
 14 files changed, 51 insertions(+), 20 deletions(-)

diff --git a/gdb/arch/amd64.c b/gdb/arch/amd64.c
index 548b32f252f..a04fcddd215 100644
--- a/gdb/arch/amd64.c
+++ b/gdb/arch/amd64.c
@@ -17,6 +17,7 @@
 
 #include "amd64.h"
 #include "gdbsupport/x86-xstate.h"
+#include "gdbsupport/osabi-common.h"
 #include <stdlib.h>
 
 #include "../features/i386/64bit-avx.c"
@@ -45,7 +46,7 @@ amd64_create_target_description (uint64_t xcr0, bool is_x32, bool is_linux,
 			  is_x32 ? "i386:x64-32" : "i386:x86-64");
 
   if (is_linux)
-    set_tdesc_osabi (tdesc.get (), "GNU/Linux");
+    set_tdesc_osabi (tdesc.get (), GDB_OSABI_LINUX);
 #endif
 
   long regnum = 0;
diff --git a/gdb/arch/i386.c b/gdb/arch/i386.c
index 49375e7c495..72159d8ed70 100644
--- a/gdb/arch/i386.c
+++ b/gdb/arch/i386.c
@@ -18,6 +18,7 @@
 #include "i386.h"
 #include "gdbsupport/tdesc.h"
 #include "gdbsupport/x86-xstate.h"
+#include "gdbsupport/osabi-common.h"
 #include <stdlib.h>
 
 #include "../features/i386/32bit-core.c"
@@ -38,7 +39,7 @@ i386_create_target_description (uint64_t xcr0, bool is_linux, bool segments)
 #ifndef IN_PROCESS_AGENT
   set_tdesc_architecture (tdesc.get (), "i386");
   if (is_linux)
-    set_tdesc_osabi (tdesc.get (), "GNU/Linux");
+    set_tdesc_osabi (tdesc.get (), GDB_OSABI_LINUX);
 #endif
 
   long regnum = 0;
diff --git a/gdb/arch/tic6x.c b/gdb/arch/tic6x.c
index 680a7942070..c1082d48eea 100644
--- a/gdb/arch/tic6x.c
+++ b/gdb/arch/tic6x.c
@@ -16,6 +16,7 @@
    along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
 
 #include "gdbsupport/tdesc.h"
+#include "gdbsupport/osabi-common.h"
 #include "tic6x.h"
 
 #include "../features/tic6x-core.c"
@@ -30,7 +31,7 @@ tic6x_create_target_description (enum c6x_feature feature)
   target_desc_up tdesc = allocate_target_description ();
 
   set_tdesc_architecture (tdesc.get (), "tic6x");
-  set_tdesc_osabi (tdesc.get (), "GNU/Linux");
+  set_tdesc_osabi (tdesc.get (), GDB_OSABI_LINUX);
 
   long regnum = 0;
 
diff --git a/gdb/features/mips-dsp-linux.c b/gdb/features/mips-dsp-linux.c
index 17d30aac68f..4873037b69d 100644
--- a/gdb/features/mips-dsp-linux.c
+++ b/gdb/features/mips-dsp-linux.c
@@ -11,7 +11,7 @@ initialize_tdesc_mips_dsp_linux (void)
   target_desc_up result = allocate_target_description ();
   set_tdesc_architecture (result.get (), bfd_scan_arch ("mips"));
 
-  set_tdesc_osabi (result.get (), "GNU/Linux");
+  set_tdesc_osabi (result.get (), GDB_OSABI_LINUX);
 
   struct tdesc_feature *feature;
 
diff --git a/gdb/features/mips-linux.c b/gdb/features/mips-linux.c
index a43526db24e..5ff2e5fb92d 100644
--- a/gdb/features/mips-linux.c
+++ b/gdb/features/mips-linux.c
@@ -11,7 +11,7 @@ initialize_tdesc_mips_linux (void)
   target_desc_up result = allocate_target_description ();
   set_tdesc_architecture (result.get (), bfd_scan_arch ("mips"));
 
-  set_tdesc_osabi (result.get (), "GNU/Linux");
+  set_tdesc_osabi (result.get (), GDB_OSABI_LINUX);
 
   struct tdesc_feature *feature;
 
diff --git a/gdb/features/or1k-linux.c b/gdb/features/or1k-linux.c
index 551b38f59c0..85a681f89ba 100644
--- a/gdb/features/or1k-linux.c
+++ b/gdb/features/or1k-linux.c
@@ -11,7 +11,7 @@ initialize_tdesc_or1k_linux (void)
   target_desc_up result = allocate_target_description ();
   set_tdesc_architecture (result.get (), bfd_scan_arch ("or1k"));
 
-  set_tdesc_osabi (result.get (), "GNU/Linux");
+  set_tdesc_osabi (result.get (), GDB_OSABI_LINUX);
 
   struct tdesc_feature *feature;
 
diff --git a/gdb/features/sparc/sparc32-solaris.c b/gdb/features/sparc/sparc32-solaris.c
index ce57abaaaf2..70affdbc5b5 100644
--- a/gdb/features/sparc/sparc32-solaris.c
+++ b/gdb/features/sparc/sparc32-solaris.c
@@ -11,7 +11,7 @@ initialize_tdesc_sparc32_solaris (void)
   target_desc_up result = allocate_target_description ();
   set_tdesc_architecture (result.get (), bfd_scan_arch ("sparc"));
 
-  set_tdesc_osabi (result.get (), "Solaris");
+  set_tdesc_osabi (result.get (), GDB_OSABI_SOLARIS);
 
   struct tdesc_feature *feature;
 
diff --git a/gdb/features/sparc/sparc64-solaris.c b/gdb/features/sparc/sparc64-solaris.c
index 92cc88cd5cf..98edabe3864 100644
--- a/gdb/features/sparc/sparc64-solaris.c
+++ b/gdb/features/sparc/sparc64-solaris.c
@@ -11,7 +11,7 @@ initialize_tdesc_sparc64_solaris (void)
   target_desc_up result = allocate_target_description ();
   set_tdesc_architecture (result.get (), bfd_scan_arch ("sparc:v9"));
 
-  set_tdesc_osabi (result.get (), "Solaris");
+  set_tdesc_osabi (result.get (), GDB_OSABI_SOLARIS);
 
   struct tdesc_feature *feature;
 
diff --git a/gdb/osabi.c b/gdb/osabi.c
index 6c00228fb4a..47ef349c6be 100644
--- a/gdb/osabi.c
+++ b/gdb/osabi.c
@@ -524,6 +524,35 @@ generic_elf_osabi_sniffer (bfd *abfd)
 
   return osabi;
 }
+
+/* See osabi.h.  */
+const char *
+gdbarch_osabi_enum_name (enum gdb_osabi osabi)
+{
+  switch (osabi)
+    {
+#define GDB_OSABI_DEF_FIRST(Enum, Name, Regex)	\
+      case GDB_OSABI_ ## Enum:			\
+	return "GDB_OSABI_" #Enum;
+
+#define GDB_OSABI_DEF(Enum, Name, Regex)	\
+      case GDB_OSABI_ ## Enum:			\
+	return "GDB_OSABI_" #Enum;
+
+#define GDB_OSABI_DEF_LAST(Enum, Name, Regex)	\
+      case GDB_OSABI_ ## Enum:			\
+	return "GDB_OSABI_" #Enum;
+
+#include "gdbsupport/osabi-common.def"
+
+#undef GDB_OSABI_DEF_LAST
+#undef GDB_OSABI_DEF
+#undef GDB_OSABI_DEF_FIRST
+    }
+
+  gdb_assert_not_reached ();
+}
+
 \f
 static void
 set_osabi (const char *args, int from_tty, struct cmd_list_element *c)
diff --git a/gdb/osabi.h b/gdb/osabi.h
index 984bdd4e5dc..02ac5503db3 100644
--- a/gdb/osabi.h
+++ b/gdb/osabi.h
@@ -52,4 +52,8 @@ void gdbarch_init_osabi (struct gdbarch_info, struct gdbarch *);
 void generic_elf_osabi_sniff_abi_tag_sections (bfd *, asection *,
 					       enum gdb_osabi *);
 
+/* Return a string version of OSABI.  This is used when generating code
+   which calls set_tdesc_osabi and an 'enum gdb_osabi' value is needed.  */
+const char *gdbarch_osabi_enum_name (enum gdb_osabi osabi);
+
 #endif /* OSABI_H */
diff --git a/gdb/target-descriptions.c b/gdb/target-descriptions.c
index c165750c6c5..1bd22c273a2 100644
--- a/gdb/target-descriptions.c
+++ b/gdb/target-descriptions.c
@@ -1199,12 +1199,6 @@ set_tdesc_architecture (struct target_desc *target_desc,
 
 /* See gdbsupport/tdesc.h.  */
 
-void
-set_tdesc_osabi (struct target_desc *target_desc, const char *name)
-{
-  set_tdesc_osabi (target_desc, osabi_from_tdesc_string (name));
-}
-
 void
 set_tdesc_osabi (struct target_desc *target_desc, enum gdb_osabi osabi)
 {
@@ -1317,9 +1311,8 @@ class print_c_tdesc : public tdesc_element_visitor
     if (tdesc_osabi (e) > GDB_OSABI_UNKNOWN
 	&& tdesc_osabi (e) < GDB_OSABI_INVALID)
       {
-	gdb_printf
-	  ("  set_tdesc_osabi (result.get (), \"%s\");\n",
-	   gdbarch_osabi_name (tdesc_osabi (e)));
+	const char *enum_name = gdbarch_osabi_enum_name (tdesc_osabi (e));
+	gdb_printf ("  set_tdesc_osabi (result.get (), %s);\n", enum_name);
 	gdb_printf ("\n");
       }
 
diff --git a/gdb/target-descriptions.h b/gdb/target-descriptions.h
index d708dbd3589..dc83db0acf2 100644
--- a/gdb/target-descriptions.h
+++ b/gdb/target-descriptions.h
@@ -219,7 +219,6 @@ int tdesc_register_in_reggroup_p (struct gdbarch *gdbarch, int regno,
 
 void set_tdesc_architecture (struct target_desc *,
 			     const struct bfd_arch_info *);
-void set_tdesc_osabi (struct target_desc *, enum gdb_osabi osabi);
 void set_tdesc_property (struct target_desc *,
 			 const char *key, const char *value);
 void tdesc_add_compatible (struct target_desc *,
diff --git a/gdbserver/tdesc.cc b/gdbserver/tdesc.cc
index 7f11120c318..d052f43c76e 100644
--- a/gdbserver/tdesc.cc
+++ b/gdbserver/tdesc.cc
@@ -179,8 +179,9 @@ tdesc_osabi_name (const struct target_desc *target_desc)
 /* See gdbsupport/tdesc.h.  */
 
 void
-set_tdesc_osabi (struct target_desc *target_desc, const char *name)
+set_tdesc_osabi (struct target_desc *target_desc, enum gdb_osabi osabi)
 {
+  const char *name = gdbarch_osabi_name (osabi);
   target_desc->osabi = make_unique_xstrdup (name);
 }
 
diff --git a/gdbsupport/tdesc.h b/gdbsupport/tdesc.h
index fa9431b5b65..0a9be1dc4d1 100644
--- a/gdbsupport/tdesc.h
+++ b/gdbsupport/tdesc.h
@@ -18,6 +18,8 @@
 #ifndef COMMON_TDESC_H
 #define COMMON_TDESC_H
 
+#include "gdbsupport/osabi-common.h"
+
 struct tdesc_feature;
 struct tdesc_type;
 struct tdesc_type_builtin;
@@ -339,7 +341,7 @@ void set_tdesc_architecture (target_desc *target_desc,
 const char *tdesc_architecture_name (const struct target_desc *target_desc);
 
 /* Set TARGET_DESC's osabi by NAME.  */
-void set_tdesc_osabi (target_desc *target_desc, const char *name);
+void set_tdesc_osabi (target_desc *target_desc, enum gdb_osabi osabi);
 
 /* Return the osabi associated with this target description as a string,
    or NULL if no osabi was specified.  */
-- 
2.25.4

Re: [PATCH 4/5] gdb/gdbserver: change shared set_tdesc_osabi to take gdb_osabi

[Luis Machado]

Just nits...

On 10/8/24 18:11, Andrew Burgess wrote:
> There is a single declaration of set_tdesc_osabi that is shared
> between gdbserver/ and gdb/, this declaration takes a 'const char *'
> argument which is the string representing an osabi.
> 
> Then in gdb/ we have an overload of set_tdesc_osabi which takes an
> 'enum gdb_osabi'.
> 
> In this commit I change the shared set_tdesc_osabi to be the version
> which takes an 'enum gdb_osabi', and I remove the version which takes
> a 'const char *'.  All users of set_tdesc_osabi are updated to pass an
> 'enum gdb_osabi'.
> 
> The features/ code, which is generated from the xml files, requires a
> new function to be added to osabi.{c,h} which can return a string
> representation of an 'enum gdb_osabi'.  With that new function in
> place the features/ code is regenerated.
> 
> This change is being made to support the next commit.  In the next
> commit gdbserver will be updated to call set_tdesc_osabi in more
> cases.  The problem is that gdbserver stores the osabi as a string.
> The issue here is that a typo in the gdbserver/ code might go
> unnoticed and result in gdbserver sending back an invalid osabi
> string.
> 
> To fix this we want gdbserver to pass an 'enum gdb_osabi' to the
> set_tdesc_osabi function.  With that requirement in place it seems to
> make sense if all calls to set_tdesc_osabi pass an 'enum gdb_osabi'.
> 
> There should be no user visible changes after this commit.
> ---
>  gdb/arch/amd64.c                     |  3 ++-
>  gdb/arch/i386.c                      |  3 ++-
>  gdb/arch/tic6x.c                     |  3 ++-
>  gdb/features/mips-dsp-linux.c        |  2 +-
>  gdb/features/mips-linux.c            |  2 +-
>  gdb/features/or1k-linux.c            |  2 +-
>  gdb/features/sparc/sparc32-solaris.c |  2 +-
>  gdb/features/sparc/sparc64-solaris.c |  2 +-
>  gdb/osabi.c                          | 29 ++++++++++++++++++++++++++++
>  gdb/osabi.h                          |  4 ++++
>  gdb/target-descriptions.c            | 11 ++---------
>  gdb/target-descriptions.h            |  1 -
>  gdbserver/tdesc.cc                   |  3 ++-
>  gdbsupport/tdesc.h                   |  4 +++-
>  14 files changed, 51 insertions(+), 20 deletions(-)
> 
> diff --git a/gdb/arch/amd64.c b/gdb/arch/amd64.c
> index 548b32f252f..a04fcddd215 100644
> --- a/gdb/arch/amd64.c
> +++ b/gdb/arch/amd64.c
> @@ -17,6 +17,7 @@
>  
>  #include "amd64.h"
>  #include "gdbsupport/x86-xstate.h"
> +#include "gdbsupport/osabi-common.h"
>  #include <stdlib.h>
>  
>  #include "../features/i386/64bit-avx.c"
> @@ -45,7 +46,7 @@ amd64_create_target_description (uint64_t xcr0, bool is_x32, bool is_linux,
>  			  is_x32 ? "i386:x64-32" : "i386:x86-64");
>  
>    if (is_linux)
> -    set_tdesc_osabi (tdesc.get (), "GNU/Linux");
> +    set_tdesc_osabi (tdesc.get (), GDB_OSABI_LINUX);
>  #endif
>  
>    long regnum = 0;
> diff --git a/gdb/arch/i386.c b/gdb/arch/i386.c
> index 49375e7c495..72159d8ed70 100644
> --- a/gdb/arch/i386.c
> +++ b/gdb/arch/i386.c
> @@ -18,6 +18,7 @@
>  #include "i386.h"
>  #include "gdbsupport/tdesc.h"
>  #include "gdbsupport/x86-xstate.h"
> +#include "gdbsupport/osabi-common.h"
>  #include <stdlib.h>
>  
>  #include "../features/i386/32bit-core.c"
> @@ -38,7 +39,7 @@ i386_create_target_description (uint64_t xcr0, bool is_linux, bool segments)
>  #ifndef IN_PROCESS_AGENT
>    set_tdesc_architecture (tdesc.get (), "i386");
>    if (is_linux)
> -    set_tdesc_osabi (tdesc.get (), "GNU/Linux");
> +    set_tdesc_osabi (tdesc.get (), GDB_OSABI_LINUX);
>  #endif
>  
>    long regnum = 0;
> diff --git a/gdb/arch/tic6x.c b/gdb/arch/tic6x.c
> index 680a7942070..c1082d48eea 100644
> --- a/gdb/arch/tic6x.c
> +++ b/gdb/arch/tic6x.c
> @@ -16,6 +16,7 @@
>     along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
>  
>  #include "gdbsupport/tdesc.h"
> +#include "gdbsupport/osabi-common.h"
>  #include "tic6x.h"
>  
>  #include "../features/tic6x-core.c"
> @@ -30,7 +31,7 @@ tic6x_create_target_description (enum c6x_feature feature)
>    target_desc_up tdesc = allocate_target_description ();
>  
>    set_tdesc_architecture (tdesc.get (), "tic6x");
> -  set_tdesc_osabi (tdesc.get (), "GNU/Linux");
> +  set_tdesc_osabi (tdesc.get (), GDB_OSABI_LINUX);
>  
>    long regnum = 0;
>  
> diff --git a/gdb/features/mips-dsp-linux.c b/gdb/features/mips-dsp-linux.c
> index 17d30aac68f..4873037b69d 100644
> --- a/gdb/features/mips-dsp-linux.c
> +++ b/gdb/features/mips-dsp-linux.c
> @@ -11,7 +11,7 @@ initialize_tdesc_mips_dsp_linux (void)
>    target_desc_up result = allocate_target_description ();
>    set_tdesc_architecture (result.get (), bfd_scan_arch ("mips"));
>  
> -  set_tdesc_osabi (result.get (), "GNU/Linux");
> +  set_tdesc_osabi (result.get (), GDB_OSABI_LINUX);
>  
>    struct tdesc_feature *feature;
>  
> diff --git a/gdb/features/mips-linux.c b/gdb/features/mips-linux.c
> index a43526db24e..5ff2e5fb92d 100644
> --- a/gdb/features/mips-linux.c
> +++ b/gdb/features/mips-linux.c
> @@ -11,7 +11,7 @@ initialize_tdesc_mips_linux (void)
>    target_desc_up result = allocate_target_description ();
>    set_tdesc_architecture (result.get (), bfd_scan_arch ("mips"));
>  
> -  set_tdesc_osabi (result.get (), "GNU/Linux");
> +  set_tdesc_osabi (result.get (), GDB_OSABI_LINUX);
>  
>    struct tdesc_feature *feature;
>  
> diff --git a/gdb/features/or1k-linux.c b/gdb/features/or1k-linux.c
> index 551b38f59c0..85a681f89ba 100644
> --- a/gdb/features/or1k-linux.c
> +++ b/gdb/features/or1k-linux.c
> @@ -11,7 +11,7 @@ initialize_tdesc_or1k_linux (void)
>    target_desc_up result = allocate_target_description ();
>    set_tdesc_architecture (result.get (), bfd_scan_arch ("or1k"));
>  
> -  set_tdesc_osabi (result.get (), "GNU/Linux");
> +  set_tdesc_osabi (result.get (), GDB_OSABI_LINUX);
>  
>    struct tdesc_feature *feature;
>  
> diff --git a/gdb/features/sparc/sparc32-solaris.c b/gdb/features/sparc/sparc32-solaris.c
> index ce57abaaaf2..70affdbc5b5 100644
> --- a/gdb/features/sparc/sparc32-solaris.c
> +++ b/gdb/features/sparc/sparc32-solaris.c
> @@ -11,7 +11,7 @@ initialize_tdesc_sparc32_solaris (void)
>    target_desc_up result = allocate_target_description ();
>    set_tdesc_architecture (result.get (), bfd_scan_arch ("sparc"));
>  
> -  set_tdesc_osabi (result.get (), "Solaris");
> +  set_tdesc_osabi (result.get (), GDB_OSABI_SOLARIS);
>  
>    struct tdesc_feature *feature;
>  
> diff --git a/gdb/features/sparc/sparc64-solaris.c b/gdb/features/sparc/sparc64-solaris.c
> index 92cc88cd5cf..98edabe3864 100644
> --- a/gdb/features/sparc/sparc64-solaris.c
> +++ b/gdb/features/sparc/sparc64-solaris.c
> @@ -11,7 +11,7 @@ initialize_tdesc_sparc64_solaris (void)
>    target_desc_up result = allocate_target_description ();
>    set_tdesc_architecture (result.get (), bfd_scan_arch ("sparc:v9"));
>  
> -  set_tdesc_osabi (result.get (), "Solaris");
> +  set_tdesc_osabi (result.get (), GDB_OSABI_SOLARIS);
>  
>    struct tdesc_feature *feature;
>  
> diff --git a/gdb/osabi.c b/gdb/osabi.c
> index 6c00228fb4a..47ef349c6be 100644
> --- a/gdb/osabi.c
> +++ b/gdb/osabi.c
> @@ -524,6 +524,35 @@ generic_elf_osabi_sniffer (bfd *abfd)
>  
>    return osabi;
>  }
> +
> +/* See osabi.h.  */

Missing newline here.

> +const char *
> +gdbarch_osabi_enum_name (enum gdb_osabi osabi)
> +{
> +  switch (osabi)
> +    {
> +#define GDB_OSABI_DEF_FIRST(Enum, Name, Regex)	\
> +      case GDB_OSABI_ ## Enum:			\
> +	return "GDB_OSABI_" #Enum;
> +
> +#define GDB_OSABI_DEF(Enum, Name, Regex)	\
> +      case GDB_OSABI_ ## Enum:			\
> +	return "GDB_OSABI_" #Enum;
> +
> +#define GDB_OSABI_DEF_LAST(Enum, Name, Regex)	\
> +      case GDB_OSABI_ ## Enum:			\
> +	return "GDB_OSABI_" #Enum;
> +
> +#include "gdbsupport/osabi-common.def"
> +
> +#undef GDB_OSABI_DEF_LAST
> +#undef GDB_OSABI_DEF
> +#undef GDB_OSABI_DEF_FIRST
> +    }
> +
> +  gdb_assert_not_reached ();
> +}
> +

Spurious newline above.

>  \f
>  static void
>  set_osabi (const char *args, int from_tty, struct cmd_list_element *c)
> diff --git a/gdb/osabi.h b/gdb/osabi.h
> index 984bdd4e5dc..02ac5503db3 100644
> --- a/gdb/osabi.h
> +++ b/gdb/osabi.h
> @@ -52,4 +52,8 @@ void gdbarch_init_osabi (struct gdbarch_info, struct gdbarch *);
>  void generic_elf_osabi_sniff_abi_tag_sections (bfd *, asection *,
>  					       enum gdb_osabi *);
>  
> +/* Return a string version of OSABI.  This is used when generating code
> +   which calls set_tdesc_osabi and an 'enum gdb_osabi' value is needed.  */
> +const char *gdbarch_osabi_enum_name (enum gdb_osabi osabi);
> +
>  #endif /* OSABI_H */
> diff --git a/gdb/target-descriptions.c b/gdb/target-descriptions.c
> index c165750c6c5..1bd22c273a2 100644
> --- a/gdb/target-descriptions.c
> +++ b/gdb/target-descriptions.c
> @@ -1199,12 +1199,6 @@ set_tdesc_architecture (struct target_desc *target_desc,
>  
>  /* See gdbsupport/tdesc.h.  */
>  
> -void
> -set_tdesc_osabi (struct target_desc *target_desc, const char *name)
> -{
> -  set_tdesc_osabi (target_desc, osabi_from_tdesc_string (name));
> -}
> -
>  void
>  set_tdesc_osabi (struct target_desc *target_desc, enum gdb_osabi osabi)
>  {
> @@ -1317,9 +1311,8 @@ class print_c_tdesc : public tdesc_element_visitor
>      if (tdesc_osabi (e) > GDB_OSABI_UNKNOWN
>  	&& tdesc_osabi (e) < GDB_OSABI_INVALID)
>        {
> -	gdb_printf
> -	  ("  set_tdesc_osabi (result.get (), \"%s\");\n",
> -	   gdbarch_osabi_name (tdesc_osabi (e)));
> +	const char *enum_name = gdbarch_osabi_enum_name (tdesc_osabi (e));
> +	gdb_printf ("  set_tdesc_osabi (result.get (), %s);\n", enum_name);
>  	gdb_printf ("\n");
>        }
>  
> diff --git a/gdb/target-descriptions.h b/gdb/target-descriptions.h
> index d708dbd3589..dc83db0acf2 100644
> --- a/gdb/target-descriptions.h
> +++ b/gdb/target-descriptions.h
> @@ -219,7 +219,6 @@ int tdesc_register_in_reggroup_p (struct gdbarch *gdbarch, int regno,
>  
>  void set_tdesc_architecture (struct target_desc *,
>  			     const struct bfd_arch_info *);
> -void set_tdesc_osabi (struct target_desc *, enum gdb_osabi osabi);
>  void set_tdesc_property (struct target_desc *,
>  			 const char *key, const char *value);
>  void tdesc_add_compatible (struct target_desc *,
> diff --git a/gdbserver/tdesc.cc b/gdbserver/tdesc.cc
> index 7f11120c318..d052f43c76e 100644
> --- a/gdbserver/tdesc.cc
> +++ b/gdbserver/tdesc.cc
> @@ -179,8 +179,9 @@ tdesc_osabi_name (const struct target_desc *target_desc)
>  /* See gdbsupport/tdesc.h.  */
>  
>  void
> -set_tdesc_osabi (struct target_desc *target_desc, const char *name)
> +set_tdesc_osabi (struct target_desc *target_desc, enum gdb_osabi osabi)
>  {
> +  const char *name = gdbarch_osabi_name (osabi);
>    target_desc->osabi = make_unique_xstrdup (name);
>  }
>  
> diff --git a/gdbsupport/tdesc.h b/gdbsupport/tdesc.h
> index fa9431b5b65..0a9be1dc4d1 100644
> --- a/gdbsupport/tdesc.h
> +++ b/gdbsupport/tdesc.h
> @@ -18,6 +18,8 @@
>  #ifndef COMMON_TDESC_H
>  #define COMMON_TDESC_H
>  
> +#include "gdbsupport/osabi-common.h"
> +
>  struct tdesc_feature;
>  struct tdesc_type;
>  struct tdesc_type_builtin;
> @@ -339,7 +341,7 @@ void set_tdesc_architecture (target_desc *target_desc,
>  const char *tdesc_architecture_name (const struct target_desc *target_desc);
>  
>  /* Set TARGET_DESC's osabi by NAME.  */
> -void set_tdesc_osabi (target_desc *target_desc, const char *name);
> +void set_tdesc_osabi (target_desc *target_desc, enum gdb_osabi osabi);
>  
>  /* Return the osabi associated with this target description as a string,
>     or NULL if no osabi was specified.  */

Otherwise looks good to me.

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

Re: [PATCH 4/5] gdb/gdbserver: change shared set_tdesc_osabi to take gdb_osabi

[Simon Marchi]

On 2024-10-08 13:11, Andrew Burgess wrote:
> @@ -339,7 +341,7 @@ void set_tdesc_architecture (target_desc *target_desc,
>  const char *tdesc_architecture_name (const struct target_desc *target_desc);
>  
>  /* Set TARGET_DESC's osabi by NAME.  */
> -void set_tdesc_osabi (target_desc *target_desc, const char *name);
> +void set_tdesc_osabi (target_desc *target_desc, enum gdb_osabi osabi);

The comment needs to be updated.

Simon

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

[Andrew Burgess]

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

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

[Luis Machado]

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>

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

[Simon Marchi]

On 2024-10-08 13:11, Andrew Burgess wrote:
> 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.  */

To maintain the verb tense and tone, I would write "Set the osabi of
TDESC to OSABI".

Maybe something for a subsequent patch: I find
amd64_create_target_description a little odd.  If you pass is_linux ==
true, the function sets the osabi of the tdesc to linux.  Otherwise, no
osabi is set.  So the resulting tdesc is not consistent for linux vs
other OSes, that can be confusing and error prone.  I think we should
either make it take an osabi parameter, and always set the osabi, or not
make it set the osabi at all (transfer that responsibility to the
callers).

Simon

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

[Mark Wielaard]

I am afraid this patch made various buildbot builders on sparc, powerpc,
and s390x unhappy:
https://builder.sourceware.org/buildbot/#/changes/63330

The errors all look similar:

reg-sparc64-generated.cc: In function ‘void init_registers_sparc64()’:
reg-sparc64-generated.cc:215:20: error: too few arguments to function ‘void init_target_desc(target_desc*, const char**, gdb_osabi)’
  215 |   init_target_desc (result, expedite_regs_sparc64);
      |   ~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
In file included from reg-sparc64-generated.cc:24:
../../binutils-gdb/gdbserver/tdesc.h:86:6: note: declared here
   86 | void init_target_desc (struct target_desc *tdesc,
      |      ^~~~~~~~~~~~~~~~

powerpc-32l-generated.cc: In function ‘void init_registers_powerpc_32l()’:
powerpc-32l-generated.cc:189:20: error: too few arguments to function ‘void init_target_desc(target_desc*, const char**, gdb_osabi)’
  189 |   init_target_desc (result, expedite_regs_powerpc_32l);
      |   ~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
In file included from powerpc-32l-generated.cc:24:
../../binutils-gdb/gdbserver/tdesc.h:86:6: note: declared here
   86 | void init_target_desc (struct target_desc *tdesc,
      |      ^~~~~~~~~~~~~~~~

powerpc-64l-generated.cc: In function ‘void init_registers_powerpc_64l()’:
powerpc-64l-generated.cc:189:20: error: too few arguments to function ‘void init_target_desc(target_desc*, const char**, gdb_osabi)’
  189 |   init_target_desc (result, expedite_regs_powerpc_64l);
      |   ~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
In file included from powerpc-64l-generated.cc:24:
../../binutils-gdb/gdbserver/tdesc.h:86:6: note: declared here
   86 | void init_target_desc (struct target_desc *tdesc,
      |      ^~~~~~~~~~~~~~~~

s390-gs-linux64-generated.cc: In function ‘void init_registers_s390_gs_linux64()’:
s390-gs-linux64-generated.cc:299:20: error: too few arguments to function ‘void init_target_desc(target_desc*, const char**, gdb_osabi)’
  299 |   init_target_desc (result, expedite_regs_s390_gs_linux64);
      |   ~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
In file included from s390-gs-linux64-generated.cc:24:
../../binutils-gdb/gdbserver/tdesc.h:86:6: note: declared here
   86 | void init_target_desc (struct target_desc *tdesc,
      |      ^~~~~~~~~~~~~~~~

s390-linux32-generated.cc: In function ‘void init_registers_s390_linux32()’:
s390-linux32-generated.cc:147:20: error: too few arguments to function ‘void init_target_desc(target_desc*, const char**, gdb_osabi)’
  147 |   init_target_desc (result, expedite_regs_s390_linux32);
      |   ~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
In file included from s390-linux32-generated.cc:24:
../../binutils-gdb/gdbserver/tdesc.h:86:6: note: declared here
   86 | void init_target_desc (struct target_desc *tdesc,
      |      ^~~~~~~~~~~~~~~~

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

[Andrew Burgess]

Mark Wielaard <mark@klomp.org> writes:

> I am afraid this patch made various buildbot builders on sparc, powerpc,
> and s390x unhappy:
> https://builder.sourceware.org/buildbot/#/changes/63330

Thanks for bringing this up.  I'm going to revert this patch for now
while I work on a solution.

Thanks,
Andrew


>
> The errors all look similar:
>
> reg-sparc64-generated.cc: In function ‘void init_registers_sparc64()’:
> reg-sparc64-generated.cc:215:20: error: too few arguments to function ‘void init_target_desc(target_desc*, const char**, gdb_osabi)’
>   215 |   init_target_desc (result, expedite_regs_sparc64);
>       |   ~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> In file included from reg-sparc64-generated.cc:24:
> ../../binutils-gdb/gdbserver/tdesc.h:86:6: note: declared here
>    86 | void init_target_desc (struct target_desc *tdesc,
>       |      ^~~~~~~~~~~~~~~~
>
> powerpc-32l-generated.cc: In function ‘void init_registers_powerpc_32l()’:
> powerpc-32l-generated.cc:189:20: error: too few arguments to function ‘void init_target_desc(target_desc*, const char**, gdb_osabi)’
>   189 |   init_target_desc (result, expedite_regs_powerpc_32l);
>       |   ~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> In file included from powerpc-32l-generated.cc:24:
> ../../binutils-gdb/gdbserver/tdesc.h:86:6: note: declared here
>    86 | void init_target_desc (struct target_desc *tdesc,
>       |      ^~~~~~~~~~~~~~~~
>
> powerpc-64l-generated.cc: In function ‘void init_registers_powerpc_64l()’:
> powerpc-64l-generated.cc:189:20: error: too few arguments to function ‘void init_target_desc(target_desc*, const char**, gdb_osabi)’
>   189 |   init_target_desc (result, expedite_regs_powerpc_64l);
>       |   ~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> In file included from powerpc-64l-generated.cc:24:
> ../../binutils-gdb/gdbserver/tdesc.h:86:6: note: declared here
>    86 | void init_target_desc (struct target_desc *tdesc,
>       |      ^~~~~~~~~~~~~~~~
>
> s390-gs-linux64-generated.cc: In function ‘void init_registers_s390_gs_linux64()’:
> s390-gs-linux64-generated.cc:299:20: error: too few arguments to function ‘void init_target_desc(target_desc*, const char**, gdb_osabi)’
>   299 |   init_target_desc (result, expedite_regs_s390_gs_linux64);
>       |   ~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> In file included from s390-gs-linux64-generated.cc:24:
> ../../binutils-gdb/gdbserver/tdesc.h:86:6: note: declared here
>    86 | void init_target_desc (struct target_desc *tdesc,
>       |      ^~~~~~~~~~~~~~~~
>
> s390-linux32-generated.cc: In function ‘void init_registers_s390_linux32()’:
> s390-linux32-generated.cc:147:20: error: too few arguments to function ‘void init_target_desc(target_desc*, const char**, gdb_osabi)’
>   147 |   init_target_desc (result, expedite_regs_s390_linux32);
>       |   ~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> In file included from s390-linux32-generated.cc:24:
> ../../binutils-gdb/gdbserver/tdesc.h:86:6: note: declared here
>    86 | void init_target_desc (struct target_desc *tdesc,
>       |      ^~~~~~~~~~~~~~~~

Re: [PATCH 0/5] Set osabi in remote target descriptions

[Simon Marchi]

On 2024-10-08 13:11, Andrew Burgess wrote:
> I tried doing some remote debugging of a Window machine from a Linux
> machine and ran into some problems caused by gdbserver failing to set
> the osabi in the target description that is sent to GDB.  This series
> fixes this issue.
> 
> In v2:
> 
>   - Patches 3 & 4 are new.  These are refactoring to allow for the
>     updated patch 5.
> 
>   - Patch 5 is mostly the same, except the osabi is now set based on
>     the enum values, rather than the osabi name strings.
> 
> Thanks,
> Andrew

I'm done sending comments, nothing is a blocker, so feel free to address
them or not, then:

Approved-By: Simon Marchi <simon.marchi@efficios.com>

Simon

Re: [PATCH 0/5] Set osabi in remote target descriptions

[Andrew Burgess]

Simon Marchi <simark@simark.ca> writes:

> On 2024-10-08 13:11, Andrew Burgess wrote:
>> I tried doing some remote debugging of a Window machine from a Linux
>> machine and ran into some problems caused by gdbserver failing to set
>> the osabi in the target description that is sent to GDB.  This series
>> fixes this issue.
>> 
>> In v2:
>> 
>>   - Patches 3 & 4 are new.  These are refactoring to allow for the
>>     updated patch 5.
>> 
>>   - Patch 5 is mostly the same, except the osabi is now set based on
>>     the enum values, rather than the osabi name strings.
>> 
>> Thanks,
>> Andrew
>
> I'm done sending comments, nothing is a blocker, so feel free to address
> them or not, then:
>
> Approved-By: Simon Marchi <simon.marchi@efficios.com>

I've addressed all the minor issues you raised and pushed this series.

I've not addressed the osabi being set to GNU/Linux in
amd64_create_target_description, which I agree is a little weird.

I still have an item on my todo list to refactor the x86 target
description creation code, so I've added a note to look at this when I
get to that task (if it's not been addressed earlier), though it'll be
towards the end of this quarter, or Q1 2025 before I'll have time to
look at the target description stuff again I think.

Thanks,
Andrew