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

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

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>