[rfa] Don't refer to previous arch in rs6000_gdbarch_init()

[rfa] Don't refer to previous arch in rs6000_gdbarch_init()

[Andrew Cagney]

Hello,

The attatched patch removes a reference to the old architecture (via 
TDEP -> gdbarch_tdep (current_gdbarch)) in rs6000_gdbarch_init().

Given that the previous architecture may not be a member of the rs6000 
family, using a value from the previous architectures tdep makes for a 
high risk operation.  I think rs6000 should instead arrange to keep a 
local copy of any stuff it wants to refer to.

Is this change to address the immediate problem ok?

	Andrew
From ac131313@cygnus.com Tue Sep 25 20:43:00 2001
From: Andrew Cagney <ac131313@cygnus.com>
To: gdb-patches@sources.redhat.com
Subject: [patch] Don't refer to current architecture when allocating a new one
Date: Tue, 25 Sep 2001 20:43:00 -0000
Message-id: <3BB14EC1.7060704@cygnus.com>
X-SW-Source: 2001-09/msg00341.html
Content-length: 948

Hello,

The attached fixes a potentially nasty but some what theoretical bug. 
The code in gdbarch_alloc() was refering to macros such as 
``TARGET_LONG_BIT'' when initializing various fields vis:

	gdbarch->target_long_long_bit = 2*TARGET_LONG_BIT;

Unfortunatly such macros refer to the global ``current_gdbarch'' and 
hence the code picks up the value of TARGET_LONG_BIT from the previous 
and not this new architecture :-(  The obvious fix would be to just not 
use the macros.  Unfortunatly due to the way a non-multi-arch 
configuration works, that isn't possible.

The attached addresses the problem by naming local variable designating 
the new architecture ``curent_gdbarch''.  That way all macros refer to 
that local variable and not the global.

As an asside, people multi-arching targets should be aware of this 
potential problem in their code.  I've a follow on patch to ensure it 
doesn't happen but it is some what prutal.

Andrew
From ac131313@cygnus.com Tue Sep 25 22:14:00 2001
From: Andrew Cagney <ac131313@cygnus.com>
To: gdb-patches@sources.redhat.com
Subject: [rfc] Swap out current when creating a new architecture
Date: Tue, 25 Sep 2001 22:14:00 -0000
Message-id: <3BB16441.30805@cygnus.com>
X-SW-Source: 2001-09/msg00342.html
Content-length: 861

Hello,

The attached changes the run-time environment within which a new 
architectures are created.  Briefly the simplified sequence:

- call XXX_gdbarch_init()
- swap out old architecture
- install new architecture

is changed to:

- swap out old architecture
- call XX_gdbarch_init()
- install new architecture

This has the effect of making current_gdbarch invalid for the lifetime 
of the XXX_gdbarch_init() call.

The motivation behind this change is to stop XXXX_gdbarch_init() 
functions refering (unintentionally I suspect) to the previous 
architecture.  I think it is proving effective since it has so far 
flushed out two bugs.

I can think of one additional tweek: add a ``gdb_assert (gdbarch != 
NULL)'' to each architecture method.  Without it a XXX_gdbarch_init() 
function that tries to use current_gdbarch will dump core :-/

thoughts?
Andrew
From ac131313@cygnus.com Tue Sep 25 23:22:00 2001
From: Andrew Cagney <ac131313@cygnus.com>
To: gdb-patches@sources.redhat.com
Subject: [patch] Sort ``maint print architecture'' output
Date: Tue, 25 Sep 2001 23:22:00 -0000
Message-id: <3BB1740E.70601@cygnus.com>
X-SW-Source: 2001-09/msg00343.html
Content-length: 179

Hello,

The attached tweek sorts the output from ``maint print architecture''. 
It makes finding the value of one of those macro's much easier!

I'll check it in shortly.

Andrew
From kevinb@cygnus.com Wed Sep 26 00:30:00 2001
From: Kevin Buettner <kevinb@cygnus.com>
To: gdb-patches@sources.redhat.com
Subject: [PATCH] solib-svr4.[hc]: Use gdbarch data mechanism...
Date: Wed, 26 Sep 2001 00:30:00 -0000
Message-id: <1010926073025.ZM625@ocotillo.lan>
X-SW-Source: 2001-09/msg00344.html
Content-length: 11120

I've just committed the patch below.

It changes solib-svr4.c to use the gdbarch data mechanism instead of
the swap mechanism for managing architecture specific link_map_offset
fetchers.  I believe this patch addresses Andrew's concerns in

    http://sources.redhat.com/ml/gdb-patches/2001-09/msg00296.html

I noticed something a bit odd which I'd like to point out (mainly to
Andrew).  init_fetch_link_map_offsets() now looks like this:

    static void *
    init_fetch_link_map_offsets (struct gdbarch *gdbarch)
    {
      struct link_map_offsets *(*flmo) =
	gdbarch_data (fetch_link_map_offsets_gdbarch_data);

      if (flmo == NULL)
	return legacy_fetch_link_map_offsets;
      else
	return flmo;
    }

The idea behind this function is to simply return the value set by
a call to set_solib_svr4_fetch_link_map_offsets() if such a call
was made, or to return legacy_fetch_link_map_offsets otherwise.

The odd thing about it is that the gdbarch parameter is never used. 
Instead, it makes use of the fact that current_gdbarch has been set to
the new architecture (and that gdbarch_data queries this new
architecture).  Perhaps this is nothing to worry about, but I had
to stare at gdbarch.c for a while to convince myself that it would
work as I wanted it to.  I'd feel better about the whole thing if
there were a version of gdbarch_data() which would take a
struct gdbarch * as an argument.  Maybe something like the following?

    void *
    get_gdbarch_data (struct gdbarch *gdbarch, struct gdbarch_data *data)
    {
      gdb_assert (data->index < gdbarch->nr_data);
      return gdbarch->data[data->index];
    }

(It seems to me that this provides a nice complement to set_gdbarch_data().)

Anyway, with the above in place, I could then do

    static void *
    init_fetch_link_map_offsets (struct gdbarch *gdbarch)
    {
      struct link_map_offsets *(*flmo) =
	get_gdbarch_data (gdbarch, fetch_link_map_offsets_gdbarch_data);
      ...

and would no longer care so much about the initialization details
in gdbarch.c (specifically gdbarch_update_p()).

	* solib-svr4.h (set_solib_svr4_fetch_link_map_offsets): Add
	argument ``gdbarch''.
	* solib-svr4.c (SVR4_FETCH_LINK_MAP_OFFSETS): Change default value.
	(default_svr4_fetch_link_map_offsets): Rename to
	legacy_fetch_link_map_offsets().
	(svr4_fetch_link_map_offsets): New function.
	(fetch_link_map_offsets, fetch_link_map_offsets_init): Deleted.
	(fetch_link_map_offsets_gdbarch_data): New static global.
	(set_solib_svr4_fetch_link_map_offsets): Add argument ``gdbarch''.
	Revise to invoke set_gdbarch_data().
	(init_fetch_link_map_offsets): Change return type and add an
	argument so that it may be used as a gdbarch_data initializer.
	(_initialize_svr4_solib): Eliminate use of gdbarch swap mechanism.
	Use gdbarch data mechanism instead.

Index: solib-svr4.c
===================================================================
RCS file: /cvs/src/src/gdb/solib-svr4.c,v
retrieving revision 1.18
diff -u -p -r1.18 solib-svr4.c
--- solib-svr4.c	2001/09/20 20:07:55	1.18
+++ solib-svr4.c	2001/09/26 06:59:17
@@ -58,28 +58,21 @@
 #include "solib-svr4.h"
 
 #ifndef SVR4_FETCH_LINK_MAP_OFFSETS
-#define SVR4_FETCH_LINK_MAP_OFFSETS() fetch_link_map_offsets ()
+#define SVR4_FETCH_LINK_MAP_OFFSETS() svr4_fetch_link_map_offsets ()
 #endif
 
-static struct link_map_offsets *default_svr4_fetch_link_map_offsets (void);
+static struct link_map_offsets *svr4_fetch_link_map_offsets (void);
+static struct link_map_offsets *legacy_fetch_link_map_offsets (void);
 
-/* fetch_link_map_offsets is the pointer to the architecture specific
-   link map offsets fetching function.  It uses the gdbarch_swap
-   mechanism to change its value when the architecture changes.  */
-static struct link_map_offsets *(*fetch_link_map_offsets)(void) = 
-  default_svr4_fetch_link_map_offsets;
-
-/* fetch_link_map_offsets_init is like the above, but obtains its
-   value from a call to set_solib_svr4_fetch_link_map_offsets().
-   This latter function is intended to be called from a *_gdbarch_init()
-   function.  The value of ``fetch_link_map_offsets_init'' is used
-   to actually set ``fetch_link_map_offsets'' when the architecture
-   is installed.  */
-static struct link_map_offsets *(*fetch_link_map_offsets_init)(void) = 0;
+/* fetch_link_map_offsets_gdbarch_data is a handle used to obtain the
+   architecture specific link map offsets fetching function.  */
 
+static struct gdbarch_data *fetch_link_map_offsets_gdbarch_data;
+
 /* legacy_svr4_fetch_link_map_offsets_hook is a pointer to a function
    which is used to fetch link map offsets.  It will only be set
    by solib-legacy.c, if at all. */
+
 struct link_map_offsets *(*legacy_svr4_fetch_link_map_offsets_hook)(void) = 0;
 
 /* Link map info to include in an allocated so_list entry */
@@ -144,27 +137,6 @@ static char *main_name_list[] =
 };
 
 
-/* Fetch (and possibly build) an appropriate link_map_offsets structure
-   for native targets using struct definitions from link.h.  
-   
-   Note: For non-native targets (i.e. cross-debugging situations),
-   you need to define a target specific fetch_link_map_offsets()
-   function and call set_solib_svr4_fetch_link_map_offsets () to
-   register this function.  */
-
-static struct link_map_offsets *
-default_svr4_fetch_link_map_offsets (void)
-{
-  if (legacy_svr4_fetch_link_map_offsets_hook)
-    return legacy_svr4_fetch_link_map_offsets_hook ();
-  else
-    {
-      internal_error (__FILE__, __LINE__,
-"default_svr4_fetch_link_map_offsets called without legacy link_map support enabled.");
-      return 0;
-    }
-}
-
 /* Macro to extract an address from a solib structure.
    When GDB is configured for some 32-bit targets (e.g. Solaris 2.7
    sparc), BFD is configured to handle 64-bit targets, so CORE_ADDR is
@@ -1654,35 +1626,78 @@ svr4_relocate_section_addresses (struct 
   sec->endaddr += LM_ADDR (so);
 }
 
+/* Fetch a link_map_offsets structure for native targets using struct
+   definitions from link.h.  See solib-legacy.c for the function
+   which does the actual work.
+   
+   Note: For non-native targets (i.e. cross-debugging situations),
+   a target specific fetch_link_map_offsets() function should be
+   defined and registered via set_solib_svr4_fetch_link_map_offsets().  */
+
+static struct link_map_offsets *
+legacy_fetch_link_map_offsets (void)
+{
+  if (legacy_svr4_fetch_link_map_offsets_hook)
+    return legacy_svr4_fetch_link_map_offsets_hook ();
+  else
+    {
+      internal_error (__FILE__, __LINE__,
+                      "legacy_fetch_link_map_offsets called without legacy "
+		      "link_map support enabled.");
+      return 0;
+    }
+}
+
+/* Fetch a link_map_offsets structure using the method registered in the
+   architecture vector.  */
+
+static struct link_map_offsets *
+svr4_fetch_link_map_offsets (void)
+{
+  struct link_map_offsets *(*flmo)(void) =
+    gdbarch_data (fetch_link_map_offsets_gdbarch_data);
+
+  if (flmo == NULL)
+    {
+      internal_error (__FILE__, __LINE__, 
+                      "svr4_fetch_link_map_offsets: fetch_link_map_offsets "
+		      "method not defined for this architecture.");
+      return 0;
+    }
+  else
+    return (flmo ());
+}
+
 /* set_solib_svr4_fetch_link_map_offsets() is intended to be called by
-   a <arch>_gdbarch_init() function.  It uses ``fetch_link_map_offsets_init''
-   to temporarily hold a pointer to the link map offsets fetcher for
-   a particular architecture.  Once the architecture is actually installed,
-   init_fetch_link_map_offsets(), below, will be called to install this
-   value in ``fetch_link_map_offsets''.  After that, the gdbarch_swap
-   machinery will manage the contents of this variable whenever the
-   architecture changes.  */
+   a <arch>_gdbarch_init() function.  It is used to establish an
+   architecture specific link_map_offsets fetcher for the architecture
+   being defined.  */
 
 void
-set_solib_svr4_fetch_link_map_offsets (struct link_map_offsets *(*flmo) (void))
+set_solib_svr4_fetch_link_map_offsets (struct gdbarch *gdbarch,
+                                       struct link_map_offsets *(*flmo) (void))
 {
-  fetch_link_map_offsets_init = flmo;
+  set_gdbarch_data (gdbarch, fetch_link_map_offsets_gdbarch_data, flmo);
 }
 
-/* Initialize the value of ``fetch_link_map_offsets'' when a new
-   architecture is created.  set_solib_svr4_fetch_link_map_offsets()
-   is used to set the value that ``fetch_link_map_offsets'' should
-   be initialized to.  */
+/* Initialize the architecture specific link_map_offsets fetcher. 
+   This is called after <arch>_gdbarch_init() has set up its struct
+   gdbarch for the new architecture, so care must be taken to use the
+   value set by set_solib_svr4_fetch_link_map_offsets(), above.  We
+   do, however, attempt to provide a reasonable alternative (for
+   native targets anyway) if the <arch>_gdbarch_init() fails to call
+   set_solib_svr4_fetch_link_map_offsets().  */
 
-static void
-init_fetch_link_map_offsets (void)
+static void *
+init_fetch_link_map_offsets (struct gdbarch *gdbarch)
 {
-  if (fetch_link_map_offsets_init != NULL)
-    fetch_link_map_offsets = fetch_link_map_offsets_init;
-  else
-    fetch_link_map_offsets = default_svr4_fetch_link_map_offsets;
+  struct link_map_offsets *(*flmo) =
+    gdbarch_data (fetch_link_map_offsets_gdbarch_data);
 
-  fetch_link_map_offsets_init = NULL;
+  if (flmo == NULL)
+    return legacy_fetch_link_map_offsets;
+  else
+    return flmo;
 }
 
 static struct target_so_ops svr4_so_ops;
@@ -1690,9 +1705,8 @@ static struct target_so_ops svr4_so_ops;
 void
 _initialize_svr4_solib (void)
 {
-  register_gdbarch_swap (&fetch_link_map_offsets,
-                         sizeof (fetch_link_map_offsets),
-			 init_fetch_link_map_offsets);
+  fetch_link_map_offsets_gdbarch_data =
+    register_gdbarch_data (init_fetch_link_map_offsets, 0);
 
   svr4_so_ops.relocate_section_addresses = svr4_relocate_section_addresses;
   svr4_so_ops.free_so = svr4_free_so;
@@ -1706,4 +1720,3 @@ _initialize_svr4_solib (void)
   /* FIXME: Don't do this here.  *_gdbarch_init() should set so_ops. */
   current_target_so_ops = &svr4_so_ops;
 }
-
Index: solib-svr4.h
===================================================================
RCS file: /cvs/src/src/gdb/solib-svr4.h,v
retrieving revision 1.2
diff -u -p -r1.2 solib-svr4.h
--- solib-svr4.h	2001/03/10 06:17:20	1.2
+++ solib-svr4.h	2001/09/26 06:59:17
@@ -64,8 +64,13 @@ struct link_map_offsets
     int l_name_size;
   };
 
-extern void set_solib_svr4_fetch_link_map_offsets (
-  struct link_map_offsets *(*func) (void));
+/* set_solib_svr4_fetch_link_map_offsets() is intended to be called by
+   a <arch>_gdbarch_init() function.  It is used to establish an
+   architecture specific link_map_offsets fetcher for the architecture
+   being defined.  */
+
+extern void set_solib_svr4_fetch_link_map_offsets
+  (struct gdbarch *gdbarch, struct link_map_offsets *(*func) (void));
 
 /* legacy_svr4_fetch_link_map_offsets_hook is a pointer to a function
    which is used to fetch link map offsets.  It will only be set

Re: [rfa] Don't refer to previous arch in rs6000_gdbarch_init()

[Kevin Buettner]

On Sep 25, 10:03pm, Andrew Cagney wrote:

> The attatched patch removes a reference to the old architecture (via 
> TDEP -> gdbarch_tdep (current_gdbarch)) in rs6000_gdbarch_init().
> 
> Given that the previous architecture may not be a member of the rs6000 
> family, using a value from the previous architectures tdep makes for a 
> high risk operation.  I think rs6000 should instead arrange to keep a 
> local copy of any stuff it wants to refer to.

I agree.  I do wonder though why the original author of this code (Nick D,
I think) found the need to refer to the old architecture.  The
comment says the following:

  /* Check word size.  If INFO is from a binary file, infer it from that,
     else use the previously-inferred size. */

But I don't see the motivation for using a previously-inferred size. 
Here is my reasoning...

rs6000_gdbarch_init() will sometimes be called without having an
executable to work with, but, if I'm not mistaken, it'll always
get called again once the user issues a ``file'' command (or an
implicit file command is performed as a result of providing the
executable file to gdb via the command line used to start up gdb).

So, by the time we get anywhere close to code which cares about
the word size, rs6000_gdbarch_init() will have been called in
a fashion such that one of the other two paths which set the word
size will have been executed.  Furthermore, it is not subsequently
called (prior to running the target) in such a way to require the 
need for the last chance setting of the word size (which is the
case that you're fixing).

Also, as you've observed, using the previously-inferred size could
be dangerous since the old architecture might be completely unrelated
to rs6000 or powerpc.  (It could get set to some oldball value which'd
make no sense at all for these architectures.)

> Is this change to address the immediate problem ok?

Yes.  But please change the comment mentioned above to read something
like this...

  /* Check word size.  If INFO is from a binary file, infer it from that,
     else choose a likely default.  */

(Feel free to improve upon the wording...)

> 	* rs6000-tdep.c (rs6000_gdbarch_init): Don't use the previous
> 	architecture to infer the wordsize.  Previous architecture may not
> 	be a PowerPC.

Thanks,

Kevin

Re: [rfa] Don't refer to previous arch in rs6000_gdbarch_init()

[Andrew Cagney]

> 
> Yes.  But please change the comment mentioned above to read something
> like this...
> 
>   /* Check word size.  If INFO is from a binary file, infer it from that,
>      else choose a likely default.  */
> 

Done.
	Andrew