[rfa/doc] Re: RFC: A change to the way we initialize new gdbarches

[Daniel Jacobowitz <drow at false dot org>]

On Wed, Oct 18, 2006 at 04:48:02PM -0400, Daniel Jacobowitz wrote:
> These changes are a prerequisite for one of the big projects I've been
> working on (self-describing targets), and I'm pretty ashamed of how
> long I've been working on it without getting more of it merged to HEAD,
> so I'm back to straighten out this initialization order issue :-)

And I've now finished adapting all of this work to HEAD, and I'll be
submitting the foundations for it momentarily - I won't say how long
that's taken me...

Joel liked the new approach, and no one has jumped up and shouted that
I'm crazy, which is gratifying in its own way.  So I plan to check this
in, but not without some documentation.  I've added a bit of
documentation of the gdbarch initialization process to gdbint.texinfo
in the attached version.

The highlight of this change is that architectures no longer need to
check the first item on their "arches" argument to fill in their
defaults.  Continuing to do so is a bug, but not a particularly
important one; those cases will now be basically dead, since
in usual usage info.exec_bfd will be set and take precedence.  I'm
not planning to go through and fix every existing architecture,
but I'll fix them as I have occasion to work on them, and I encourage
others to also.

Eli, when you have a chance, could you take a look at the new gdbint
chapter?  After that, I'll commit this.  I've tested it variously on
GNU/Linux for x86_64, ARM, MIPS, and MIPS64.

-- 
Daniel Jacobowitz
CodeSourcery

2006-11-09  Daniel Jacobowitz  <dan@codesourcery.com>

	* arch-utils.c (target_byte_order_user): Renamed from
	target_byte_order.
	(target_byte_order_auto, selected_byte_order): Removed.
	(show_endian): Check target_byte_order_user.
	(set_endian): Always update the architecture.  Set
	target_byte_order_user after success.
	(target_architecture_auto): Removed.
	(target_architecture_user): New.
	(selected_architecture_name, show_architecture): Check it.
	(set_architecture): Set target_architecture_user after success.
	(gdbarch_from_bfd): Check the argument.
	(default_byte_order): New.
	(initialize_current_architecture): Set the global default
	architecture and endianness.
	(gdbarch_info_fill): Remove GDBARCH argument.  Do not check the
	previous architecture.  Use exec_bfd, global selected architecture
	and endianness, and global defaults.
	* arch-utils.h (selected_byte_order): Remove prototype.
	(gdbarch_info_fill): Update.
	* exec.c (exec_file_attach): Update the architecture after removing
	the current file.
	* gdbarch.sh: Update comments.
	(find_arch_by_info): Remove OLD_GDBARCH argument.  Update call to
	gdbarch_info_fill.
	(gdbarch_find_by_info): Update call to find_arch_by_info.
	* gdbarch.h, gdbarch.c: Regenerated.
	* remote-sim.c (gdbsim_open): Use TARGET_BYTE_ORDER.

2006-11-09  Daniel Jacobowitz  <dan@codesourcery.com>

	* gdbint.texinfo (Target Architecture Definition): Add new
	Initializing a New Architecture section.

---
 gdb/arch-utils.c       |  136 ++++++++++++++++++++++++-------------------------
 gdb/arch-utils.h       |   11 +--
 gdb/doc/gdbint.texinfo |   35 ++++++++++++
 gdb/exec.c             |    2 
 gdb/gdbarch.c          |   10 +--
 gdb/gdbarch.h          |    3 -
 gdb/gdbarch.sh         |   13 ++--
 gdb/remote-sim.c       |    2 
 8 files changed, 122 insertions(+), 90 deletions(-)

Index: src/gdb/arch-utils.c
===================================================================
--- src.orig/gdb/arch-utils.c	2006-11-09 14:07:12.000000000 -0500
+++ src/gdb/arch-utils.c	2006-11-09 14:14:33.000000000 -0500
@@ -335,24 +335,7 @@ generic_instruction_nullified (struct gd
 
 /* Functions to manipulate the endianness of the target.  */
 
-/* ``target_byte_order'' is only used when non- multi-arch.
-   Multi-arch targets obtain the current byte order using the
-   TARGET_BYTE_ORDER gdbarch method.
-
-   The choice of initial value is entirely arbitrary.  During startup,
-   the function initialize_current_architecture() updates this value
-   based on default byte-order information extracted from BFD.  */
-static int target_byte_order = BFD_ENDIAN_BIG;
-static int target_byte_order_auto = 1;
-
-enum bfd_endian
-selected_byte_order (void)
-{
-  if (target_byte_order_auto)
-    return BFD_ENDIAN_UNKNOWN;
-  else
-    return target_byte_order;
-}
+static int target_byte_order_user = BFD_ENDIAN_UNKNOWN;
 
 static const char endian_big[] = "big";
 static const char endian_little[] = "little";
@@ -372,7 +355,7 @@ static void
 show_endian (struct ui_file *file, int from_tty, struct cmd_list_element *c,
 	     const char *value)
 {
-  if (target_byte_order_auto)
+  if (target_byte_order_user != BFD_ENDIAN_UNKNOWN)
     if (TARGET_BYTE_ORDER == BFD_ENDIAN_BIG)
       fprintf_unfiltered (file, _("The target endianness is set automatically "
 				  "(currently big endian)\n"));
@@ -391,31 +374,37 @@ show_endian (struct ui_file *file, int f
 static void
 set_endian (char *ignore_args, int from_tty, struct cmd_list_element *c)
 {
+  struct gdbarch_info info;
+
+  gdbarch_info_init (&info);
+
   if (set_endian_string == endian_auto)
     {
-      target_byte_order_auto = 1;
+      target_byte_order_user = BFD_ENDIAN_UNKNOWN;
+      if (! gdbarch_update_p (info))
+	internal_error (__FILE__, __LINE__,
+			_("set_endian: architecture update failed"));
     }
   else if (set_endian_string == endian_little)
     {
-      struct gdbarch_info info;
-      target_byte_order_auto = 0;
-      gdbarch_info_init (&info);
       info.byte_order = BFD_ENDIAN_LITTLE;
       if (! gdbarch_update_p (info))
 	printf_unfiltered (_("Little endian target not supported by GDB\n"));
+      else
+	target_byte_order_user = BFD_ENDIAN_LITTLE;
     }
   else if (set_endian_string == endian_big)
     {
-      struct gdbarch_info info;
-      target_byte_order_auto = 0;
-      gdbarch_info_init (&info);
       info.byte_order = BFD_ENDIAN_BIG;
       if (! gdbarch_update_p (info))
 	printf_unfiltered (_("Big endian target not supported by GDB\n"));
+      else
+	target_byte_order_user = BFD_ENDIAN_BIG;
     }
   else
     internal_error (__FILE__, __LINE__,
 		    _("set_endian: bad value"));
+
   show_endian (gdb_stdout, from_tty, NULL, NULL);
 }
 
@@ -423,14 +412,14 @@ set_endian (char *ignore_args, int from_
 
 enum set_arch { set_arch_auto, set_arch_manual };
 
-static int target_architecture_auto = 1;
+static const struct bfd_arch_info *target_architecture_user;
 
 static const char *set_architecture_string;
 
 const char *
 selected_architecture_name (void)
 {
-  if (target_architecture_auto)
+  if (target_architecture_user == NULL)
     return NULL;
   else
     return set_architecture_string;
@@ -445,7 +434,7 @@ show_architecture (struct ui_file *file,
 {
   const char *arch;
   arch = TARGET_ARCHITECTURE->printable_name;
-  if (target_architecture_auto)
+  if (target_architecture_user == NULL)
     fprintf_filtered (file, _("\
 The target architecture is set automatically (currently %s)\n"), arch);
   else
@@ -460,20 +449,25 @@ The target architecture is assumed to be
 static void
 set_architecture (char *ignore_args, int from_tty, struct cmd_list_element *c)
 {
+  struct gdbarch_info info;
+
+  gdbarch_info_init (&info);
+
   if (strcmp (set_architecture_string, "auto") == 0)
     {
-      target_architecture_auto = 1;
+      target_architecture_user = NULL;
+      if (!gdbarch_update_p (info))
+	internal_error (__FILE__, __LINE__,
+			_("could not select an architecture automatically"));
     }
   else
     {
-      struct gdbarch_info info;
-      gdbarch_info_init (&info);
       info.bfd_arch_info = bfd_scan_arch (set_architecture_string);
       if (info.bfd_arch_info == NULL)
 	internal_error (__FILE__, __LINE__,
 			_("set_architecture: bfd_scan_arch failed"));
       if (gdbarch_update_p (info))
-	target_architecture_auto = 0;
+	target_architecture_user = info.bfd_arch_info;
       else
 	printf_unfiltered (_("Architecture `%s' not recognized.\n"),
 			   set_architecture_string);
@@ -530,6 +524,13 @@ gdbarch_from_bfd (bfd *abfd)
   struct gdbarch *new_gdbarch;
   struct gdbarch_info info;
 
+  /* If we call gdbarch_find_by_info without filling in info.abfd,
+     then it will use the global exec_bfd.  That's fine if we don't
+     have one of those either.  And that's the only time we should
+     reach here with a NULL ABFD argument - when we are discarding
+     the executable.  */
+  gdb_assert (abfd != NULL || exec_bfd == NULL);
+
   gdbarch_info_init (&info);
   info.abfd = abfd;
   return gdbarch_find_by_info (info);
@@ -567,6 +568,8 @@ static const bfd_target *default_bfd_vec
 static const bfd_target *default_bfd_vec;
 #endif
 
+static int default_byte_order = BFD_ENDIAN_UNKNOWN;
+
 void
 initialize_current_architecture (void)
 {
@@ -577,10 +580,7 @@ initialize_current_architecture (void)
   gdbarch_info_init (&info);
   
   /* Find a default architecture. */
-  if (info.bfd_arch_info == NULL
-      && default_bfd_arch != NULL)
-    info.bfd_arch_info = default_bfd_arch;
-  if (info.bfd_arch_info == NULL)
+  if (default_bfd_arch == NULL)
     {
       /* Choose the architecture by taking the first one
 	 alphabetically. */
@@ -594,30 +594,32 @@ initialize_current_architecture (void)
       if (chosen == NULL)
 	internal_error (__FILE__, __LINE__,
 			_("initialize_current_architecture: No arch"));
-      info.bfd_arch_info = bfd_scan_arch (chosen);
-      if (info.bfd_arch_info == NULL)
+      default_bfd_arch = bfd_scan_arch (chosen);
+      if (default_bfd_arch == NULL)
 	internal_error (__FILE__, __LINE__,
 			_("initialize_current_architecture: Arch not found"));
     }
 
+  info.bfd_arch_info = default_bfd_arch;
+
   /* Take several guesses at a byte order.  */
-  if (info.byte_order == BFD_ENDIAN_UNKNOWN
+  if (default_byte_order == BFD_ENDIAN_UNKNOWN
       && default_bfd_vec != NULL)
     {
       /* Extract BFD's default vector's byte order. */
       switch (default_bfd_vec->byteorder)
 	{
 	case BFD_ENDIAN_BIG:
-	  info.byte_order = BFD_ENDIAN_BIG;
+	  default_byte_order = BFD_ENDIAN_BIG;
 	  break;
 	case BFD_ENDIAN_LITTLE:
-	  info.byte_order = BFD_ENDIAN_LITTLE;
+	  default_byte_order = BFD_ENDIAN_LITTLE;
 	  break;
 	default:
 	  break;
 	}
     }
-  if (info.byte_order == BFD_ENDIAN_UNKNOWN)
+  if (default_byte_order == BFD_ENDIAN_UNKNOWN)
     {
       /* look for ``*el-*'' in the target name. */
       const char *chp;
@@ -625,14 +627,16 @@ initialize_current_architecture (void)
       if (chp != NULL
 	  && chp - 2 >= target_name
 	  && strncmp (chp - 2, "el", 2) == 0)
-	info.byte_order = BFD_ENDIAN_LITTLE;
+	default_byte_order = BFD_ENDIAN_LITTLE;
     }
-  if (info.byte_order == BFD_ENDIAN_UNKNOWN)
+  if (default_byte_order == BFD_ENDIAN_UNKNOWN)
     {
       /* Wire it to big-endian!!! */
-      info.byte_order = BFD_ENDIAN_BIG;
+      default_byte_order = BFD_ENDIAN_BIG;
     }
 
+  info.byte_order = default_byte_order;
+
   if (! gdbarch_update_p (info))
     internal_error (__FILE__, __LINE__,
 		    _("initialize_current_architecture: Selection of "
@@ -674,48 +678,46 @@ gdbarch_info_init (struct gdbarch_info *
 }
 
 /* Similar to init, but this time fill in the blanks.  Information is
-   obtained from the specified architecture, global "set ..." options,
-   and explicitly initialized INFO fields.  */
+   obtained from the global "set ..." options and explicitly
+   initialized INFO fields.  */
 
 void
-gdbarch_info_fill (struct gdbarch *gdbarch, struct gdbarch_info *info)
+gdbarch_info_fill (struct gdbarch_info *info)
 {
+  /* Check for the current file.  */
+  if (info->abfd == NULL)
+    info->abfd = exec_bfd;
+
   /* "(gdb) set architecture ...".  */
   if (info->bfd_arch_info == NULL
-      && !target_architecture_auto
-      && gdbarch != NULL)
-    info->bfd_arch_info = gdbarch_bfd_arch_info (gdbarch);
+      && target_architecture_user)
+    info->bfd_arch_info = target_architecture_user;
   if (info->bfd_arch_info == NULL
       && info->abfd != NULL
       && bfd_get_arch (info->abfd) != bfd_arch_unknown
       && bfd_get_arch (info->abfd) != bfd_arch_obscure)
     info->bfd_arch_info = bfd_get_arch_info (info->abfd);
-  if (info->bfd_arch_info == NULL
-      && gdbarch != NULL)
-    info->bfd_arch_info = gdbarch_bfd_arch_info (gdbarch);
+  /* From the default.  */
+  if (info->bfd_arch_info == NULL)
+    info->bfd_arch_info = default_bfd_arch;
 
   /* "(gdb) set byte-order ...".  */
   if (info->byte_order == BFD_ENDIAN_UNKNOWN
-      && !target_byte_order_auto
-      && gdbarch != NULL)
-    info->byte_order = gdbarch_byte_order (gdbarch);
+      && target_byte_order_user != BFD_ENDIAN_UNKNOWN)
+    info->byte_order = target_byte_order_user;
   /* From the INFO struct.  */
   if (info->byte_order == BFD_ENDIAN_UNKNOWN
       && info->abfd != NULL)
     info->byte_order = (bfd_big_endian (info->abfd) ? BFD_ENDIAN_BIG
-		       : bfd_little_endian (info->abfd) ? BFD_ENDIAN_LITTLE
-		       : BFD_ENDIAN_UNKNOWN);
-  /* From the current target.  */
-  if (info->byte_order == BFD_ENDIAN_UNKNOWN
-      && gdbarch != NULL)
-    info->byte_order = gdbarch_byte_order (gdbarch);
+			: bfd_little_endian (info->abfd) ? BFD_ENDIAN_LITTLE
+			: BFD_ENDIAN_UNKNOWN);
+  /* From the default.  */
+  if (info->byte_order == BFD_ENDIAN_UNKNOWN)
+    info->byte_order = default_byte_order;
 
   /* "(gdb) set osabi ...".  Handled by gdbarch_lookup_osabi.  */
   if (info->osabi == GDB_OSABI_UNINITIALIZED)
     info->osabi = gdbarch_lookup_osabi (info->abfd);
-  if (info->osabi == GDB_OSABI_UNINITIALIZED
-      && gdbarch != NULL)
-    info->osabi = gdbarch_osabi (gdbarch);
 
   /* Must have at least filled in the architecture.  */
   gdb_assert (info->bfd_arch_info != NULL);
Index: src/gdb/arch-utils.h
===================================================================
--- src.orig/gdb/arch-utils.h	2006-11-09 14:07:12.000000000 -0500
+++ src/gdb/arch-utils.h	2006-11-09 14:14:33.000000000 -0500
@@ -126,10 +126,6 @@ extern int generic_instruction_nullified
 
 extern int legacy_register_sim_regno (int regnum);
 
-/* Return the selected byte order, or BFD_ENDIAN_UNKNOWN if no byte
-   order was explicitly selected.  */
-extern enum bfd_endian selected_byte_order (void);
-
 /* Return the selected architecture's name, or NULL if no architecture
    was explicitly selected.  */
 extern const char *selected_architecture_name (void);
@@ -141,10 +137,9 @@ extern const char *selected_architecture
 extern void gdbarch_info_init (struct gdbarch_info *info);
 
 /* Similar to init, but this time fill in the blanks.  Information is
-   obtained from the specified architecture, global "set ..." options,
-   and explicitly initialized INFO fields.  */
-extern void gdbarch_info_fill (struct gdbarch *gdbarch,
-			       struct gdbarch_info *info);
+   obtained from the global "set ..." options and explicitly
+   initialized INFO fields.  */
+extern void gdbarch_info_fill (struct gdbarch_info *info);
 
 /* Return the architecture for ABFD.  If no suitable architecture
    could be find, return NULL.  */
Index: src/gdb/exec.c
===================================================================
--- src.orig/gdb/exec.c	2006-11-09 14:07:12.000000000 -0500
+++ src/gdb/exec.c	2006-11-09 14:14:33.000000000 -0500
@@ -188,6 +188,8 @@ exec_file_attach (char *filename, int fr
     {
       if (from_tty)
         printf_unfiltered (_("No executable file now.\n"));
+
+      set_gdbarch_from_file (NULL);
     }
   else
     {
Index: src/gdb/gdbarch.c
===================================================================
--- src.orig/gdb/gdbarch.c	2006-11-09 14:07:12.000000000 -0500
+++ src/gdb/gdbarch.c	2006-11-09 14:14:33.000000000 -0500
@@ -3987,7 +3987,7 @@ gdbarch_list_lookup_by_info (struct gdba
    that there is no current architecture.  */
 
 static struct gdbarch *
-find_arch_by_info (struct gdbarch *old_gdbarch, struct gdbarch_info info)
+find_arch_by_info (struct gdbarch_info info)
 {
   struct gdbarch *new_gdbarch;
   struct gdbarch_registration *rego;
@@ -3997,9 +3997,9 @@ find_arch_by_info (struct gdbarch *old_g
   gdb_assert (current_gdbarch == NULL);
 
   /* Fill in missing parts of the INFO struct using a number of
-     sources: "set ..."; INFOabfd supplied; and the existing
-     architecture.  */
-  gdbarch_info_fill (old_gdbarch, &info);
+     sources: "set ..."; INFOabfd supplied; and the global
+     defaults.  */
+  gdbarch_info_fill (&info);
 
   /* Must have found some sort of architecture. */
   gdb_assert (info.bfd_arch_info != NULL);
@@ -4130,7 +4130,7 @@ gdbarch_find_by_info (struct gdbarch_inf
   struct gdbarch *old_gdbarch = current_gdbarch_swap_out_hack ();
 
   /* Find the specified architecture.  */
-  struct gdbarch *new_gdbarch = find_arch_by_info (old_gdbarch, info);
+  struct gdbarch *new_gdbarch = find_arch_by_info (info);
 
   /* Restore the existing architecture.  */
   gdb_assert (current_gdbarch == NULL);
Index: src/gdb/gdbarch.h
===================================================================
--- src.orig/gdb/gdbarch.h	2006-11-09 14:07:12.000000000 -0500
+++ src/gdb/gdbarch.h	2006-11-09 14:14:33.000000000 -0500
@@ -1420,8 +1420,7 @@ extern struct gdbarch_tdep *gdbarch_tdep
    ``struct gdbarch'' for this architecture.
 
    The INFO parameter is, as far as possible, be pre-initialized with
-   information obtained from INFO.ABFD or the previously selected
-   architecture.
+   information obtained from INFO.ABFD or the global defaults.
 
    The ARCHES parameter is a linked list (sorted most recently used)
    of all the previously created architures for this architecture
Index: src/gdb/gdbarch.sh
===================================================================
--- src.orig/gdb/gdbarch.sh	2006-11-09 14:07:12.000000000 -0500
+++ src/gdb/gdbarch.sh	2006-11-09 14:14:33.000000000 -0500
@@ -943,8 +943,7 @@ extern struct gdbarch_tdep *gdbarch_tdep
    \`\`struct gdbarch'' for this architecture.
 
    The INFO parameter is, as far as possible, be pre-initialized with
-   information obtained from INFO.ABFD or the previously selected
-   architecture.
+   information obtained from INFO.ABFD or the global defaults.
 
    The ARCHES parameter is a linked list (sorted most recently used)
    of all the previously created architures for this architecture
@@ -2050,7 +2049,7 @@ gdbarch_list_lookup_by_info (struct gdba
    that there is no current architecture.  */
 
 static struct gdbarch *
-find_arch_by_info (struct gdbarch *old_gdbarch, struct gdbarch_info info)
+find_arch_by_info (struct gdbarch_info info)
 {
   struct gdbarch *new_gdbarch;
   struct gdbarch_registration *rego;
@@ -2060,9 +2059,9 @@ find_arch_by_info (struct gdbarch *old_g
   gdb_assert (current_gdbarch == NULL);
 
   /* Fill in missing parts of the INFO struct using a number of
-     sources: "set ..."; INFOabfd supplied; and the existing
-     architecture.  */
-  gdbarch_info_fill (old_gdbarch, &info);
+     sources: "set ..."; INFOabfd supplied; and the global
+     defaults.  */
+  gdbarch_info_fill (&info);
 
   /* Must have found some sort of architecture. */
   gdb_assert (info.bfd_arch_info != NULL);
@@ -2193,7 +2192,7 @@ gdbarch_find_by_info (struct gdbarch_inf
   struct gdbarch *old_gdbarch = current_gdbarch_swap_out_hack ();
 
   /* Find the specified architecture.  */
-  struct gdbarch *new_gdbarch = find_arch_by_info (old_gdbarch, info);
+  struct gdbarch *new_gdbarch = find_arch_by_info (info);
 
   /* Restore the existing architecture.  */
   gdb_assert (current_gdbarch == NULL);
Index: src/gdb/remote-sim.c
===================================================================
--- src.orig/gdb/remote-sim.c	2006-11-09 14:07:12.000000000 -0500
+++ src/gdb/remote-sim.c	2006-11-09 14:14:33.000000000 -0500
@@ -506,7 +506,7 @@ gdbsim_open (char *args, int from_tty)
   strcpy (arg_buf, "gdbsim");	/* 7 */
   /* Specify the byte order for the target when it is both selectable
      and explicitly specified by the user (not auto detected). */
-  switch (selected_byte_order ())
+  switch (TARGET_BYTE_ORDER)
     {
     case BFD_ENDIAN_BIG:
       strcat (arg_buf, " -E big");
Index: src/gdb/doc/gdbint.texinfo
===================================================================
--- src.orig/gdb/doc/gdbint.texinfo	2006-11-09 14:14:55.000000000 -0500
+++ src/gdb/doc/gdbint.texinfo	2006-11-09 14:33:05.000000000 -0500
@@ -2722,6 +2722,41 @@ architecture, a warning will be issued a
 with the defaults already established for @var{gdbarch}.
 @end deftypefun
 
+@section Initializing a New Architecture
+
+Each @code{gdbarch} is associated with a single @sc{bfd} architecture,
+via a @code{bfd_arch_@var{arch}} constant.  The @code{gdbarch} is
+registered by a call to @code{register_gdbarch_init}, usually from
+the file's @code{_initialize_@var{filename}} routine, which will
+be automatically called during @value{GDBN} startup.  The arguments
+are a @sc{bfd} architecture constant and an initialization function.
+
+The initialization function has this type:
+
+@smallexample
+static struct gdbarch *
+@var{arch}_gdbarch_init (struct gdbarch_info info,
+                         struct gdbarch_list *arches)
+@end smallexample
+
+The @var{info} argument contains parameters used to select the correct
+architecture, and @var{arches} is a list of architectures which
+have already been created with the same @code{bfd_arch_@var{arch}}
+value.
+
+The initialization function should first make sure that @var{info}
+is acceptable, and return @code{NULL} if it is not.  Then, it should
+search through @var{arches} for an exact match to @var{info}, and
+return one if found.  Lastly, if no exact match was found, it should
+create a new architecture based on @var{info} and return it.
+
+Only information in @var{info} should be used to choose the new
+architecture.  Historically, @var{info} could be sparse, and
+defaults would be collected from the first element on @var{arches}.
+However, @value{GDBN} now fills in @var{info} more thoroughly,
+so new @code{gdbarch} initialization functions should not take
+defaults from @var{arches}.
+
 @section Registers and Memory
 
 @value{GDBN}'s model of the target machine is rather simple.