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

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

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>