Re: [rfc] Swap out current when creating a new architecture

Re: [rfc] Swap out current when creating a new architecture

[Kevin Buettner]

On Sep 26,  1:14am, Andrew Cagney wrote:

> 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?

I've read your patch and it looks okay to me.

I'm wondering though if it might be possible to set current_gdbarch
to the architecture currently getting defined.  This way, it would
be possible to do things like:

    gdbarch->target_long_bit = 8;
    gdbarch->target_long_long_bit = 2*TARGET_LONG_BIT;

It seems to me that the trick is to figure out a clean way to
set it.  It occurred to me that it could be set in gdbarch_alloc(),
but that doesn't really seem too clean... The only other thing I
can think of is to have (the various) <arch>_gdbarch_init() make
an explicit call which'd cause current_gdbarch to be set.  After
that, the <arch>_gdbarch_init() could refer to TARGET_LONG_BIT, etc.
if they wanted to.  Such a call would be optional; if this call
isn't made, then it's not permissible to refer to the macros...

My other thought on this matter is that all of what I just said
is complete nonsense and that we're better off with current_gdbarch
being NULL to avoid referring to a partially defined architecture...
if that's the case, then adding a NULL check to each architecture
method would indeed be a good thing.

Kevin

Re: [rfc] Swap out current when creating a new architecture

[Andrew Cagney]

> I'm wondering though if it might be possible to set current_gdbarch
> to the architecture currently getting defined.  This way, it would
> be possible to do things like:
> 
>     gdbarch->target_long_bit = 8;
>     gdbarch->target_long_long_bit = 2*TARGET_LONG_BIT;
> 

Another option other is to rename the local variable ``gdbarch'' to 
``current_gdbarch''.  Check my other patch to gdbarch_alloc() which was 
forced to do just that.

Personally, though, I'd prefer to be doing none of the above.  Instead 
just eliminate that global current_gdbarch and parameterize everthing 
.... :-)

enjoy,
Andrew

Re: [rfc] Swap out current when creating a new architecture

[Andrew Cagney]

> 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.
> 

This patch is evi^D^D^D nasty.

The CRIS target also dumps core with it applied.  Like rs6000 was, it is 
refering to the previous (current_gdbarch) architecture.

I'll sit on this for a bit longer.  Could I encourage target maintainers 
to check their XXX_gdbarch_init() function for references to 
current_gdbarch.

Andrew

Re: [rfc] Swap out current when creating a new architecture

[Orjan Friberg]

Andrew Cagney wrote:
> 
> The CRIS target also dumps core with it applied.  Like rs6000 was, it is
> refering to the previous (current_gdbarch) architecture.
> 
> I'll sit on this for a bit longer.  Could I encourage target maintainers
> to check their XXX_gdbarch_init() function for references to
> current_gdbarch.

Just a quick recap: what the old code was doing by referring to
current_gdbarch was to avoid changing the ABI (which I infer from the
bfd) if one of the other target specific commands were being used.  The
patch below sets the abfd field in the info struct to exec_bfd before
calling gdbarch_update_p, so that information on the current bfd is
passed along, just as it is when gdbarch_update_p is called from
set_gdbarch_from_file.

Is this an acceptable way of doing it?  I know I'm in the risky business
of dealing with global pointers.  (I'm assuming a commit could go on
both trunk and branch.)


2001-10-01  Orjan Friberg  <orjanf@axis.com>

	* cris-tdep.c (cris_gdbarch_init): Remove reference to current_gdbarch.
	(cris_version_update, cris_mode_update, cris_abi_update): Set abfd in
	info struct to exec_bfd.

Index: cris-tdep.c
===================================================================
RCS file: /cvs/src/src/gdb/cris-tdep.c,v
retrieving revision 1.4
diff -c -3 -p -r1.4 cris-tdep.c
*** cris-tdep.c 2001/09/24 08:04:10     1.4
--- cris-tdep.c 2001/10/01 10:58:11
*************** cris_version_update (char *ignore_args, 
*** 3596,3601 ****
--- 3596,3605 ----
    
        /* Update the current architecture, if needed.  */
        memset (&info, 0, sizeof info);
+ 
+       /* Supply the current bfd (if any).  */
+       info.abfd = exec_bfd;
+ 
        if (!gdbarch_update_p (info))
          internal_error (__FILE__, __LINE__, "cris_gdbarch_update:
failed to update architecture.");
      }  
*************** cris_mode_update (char *ignore_args, int
*** 3614,3619 ****
--- 3618,3627 ----
    
        /* Update the current architecture, if needed.  */
        memset (&info, 0, sizeof info);
+ 
+       /* Supply the current bfd (if any).  */
+       info.abfd = exec_bfd;
+ 
        if (!gdbarch_update_p (info))
          internal_error (__FILE__, __LINE__, "cris_gdbarch_update:
failed to update architecture.");
      }
*************** cris_abi_update (char *ignore_args, int 
*** 3632,3637 ****
--- 3640,3649 ----
    
        /* Update the current architecture, if needed.  */
        memset (&info, 0, sizeof info);
+ 
+       /* Supply the current bfd (if any).  */
+       info.abfd = exec_bfd;
+ 
        if (!gdbarch_update_p (info))
          internal_error (__FILE__, __LINE__, "cris_gdbarch_update:
failed to update architecture.");
      }
*************** cris_gdbarch_init (struct gdbarch_info i
*** 3736,3748 ****
            /* Unknown bfd flavour.  Assume it's the new ABI.  */
            cris_abi = CRIS_ABI_V2;
          }
-     }
-   else if (gdbarch_tdep (current_gdbarch))
-     {
-       /* No bfd available.  Stick with whatever ABI we're currently
using.  
-          (This is to avoid changing the ABI when the user updates the 
-          architecture with the 'set cris-version' command.)  */
-       cris_abi = gdbarch_tdep (current_gdbarch)->cris_abi;
      }
    else
      {
--- 3748,3753 ----


-- 
Orjan Friberg              E-mail: orjan.friberg@axis.com
Axis Communications AB     Phone:  +46 46 272 17 68

Re: [rfc] Swap out current when creating a new architecture

[Andrew Cagney]

> 
> Just a quick recap: what the old code was doing by referring to
> current_gdbarch was to avoid changing the ABI (which I infer from the
> bfd) if one of the other target specific commands were being used.  The
> patch below sets the abfd field in the info struct to exec_bfd before
> calling gdbarch_update_p, so that information on the current bfd is
> passed along, just as it is when gdbarch_update_p is called from
> set_gdbarch_from_file.
> 
> Is this an acceptable way of doing it?  I know I'm in the risky business
> of dealing with global pointers.  (I'm assuming a commit could go on
> both trunk and branch.)

Nice bug! I need to think about this some more though.

As for the branch, no need as I'm not committing my change to that.

Andrew

Re: [rfc] Swap out current when creating a new architecture

[Andrew Cagney]

> Just a quick recap: what the old code was doing by referring to
> current_gdbarch was to avoid changing the ABI (which I infer from the
> bfd) if one of the other target specific commands were being used.  The
> patch below sets the abfd field in the info struct to exec_bfd before
> calling gdbarch_update_p, so that information on the current bfd is
> passed along, just as it is when gdbarch_update_p is called from
> set_gdbarch_from_file.
> 
> Is this an acceptable way of doing it?  I know I'm in the risky business
> of dealing with global pointers.  (I'm assuming a commit could go on
> both trunk and branch.)
> 
> Nice bug! I need to think about this some more though.

Really nice bug!

The proposed change unfortunatly isn't right for reasons similar to the 
original current_gdbarch problem.  The exec file might belong to a 
completely different architecture.  Consider a sequence like:

	(gdb) file hello.d10v
	(gdb) info architecture
	The current architecture is d10v
	(gdb) set cris-mode CRIS_MODE_USER

For MIPS, the problem has so far been avoided by coding functions as:

	if (set by user)
	  return user value;
	else
	  return value from architecture;

I should note though, that even this will eventually have problems - it 
affects all MIPS architectures - but at least it gets over the immediate 
hurdle.

enjoy,
Andrew

Re: [rfc] Swap out current when creating a new architecture

[Orjan Friberg]

Andrew Cagney wrote:
> 
> Really nice bug!
> 
> The proposed change unfortunatly isn't right for reasons similar to the
> original current_gdbarch problem.  The exec file might belong to a
> completely different architecture.  Consider a sequence like:
> 
>         (gdb) file hello.d10v
>         (gdb) info architecture
>         The current architecture is d10v
>         (gdb) set cris-mode CRIS_MODE_USER

Hm, I had a feeling I was just moving the problem elsewhere, though I
didn't think of the case of having gdb configured for two
architectures.  But with multi-arch I guess this is not just a remote
possibility.

> For MIPS, the problem has so far been avoided by coding functions as:
> 
>         if (set by user)
>           return user value;
>         else
>           return value from architecture;
> 

From looking at the MIPS target just now I thought it would suffer from
the same problem when it comes to guessing the ABI (since info.abfd is
used if present).  But then I saw that the functions tied to the MIPS
specific user-commands don't call gdbarch_update, they change
current_gdbarch directly.  I'm going to try something similar.

Thanks,
Orjan

-- 
Orjan Friberg              E-mail: orjan.friberg@axis.com
Axis Communications AB     Phone:  +46 46 272 17 68