[rfc] [17/17] Get rid of current_gdbarch in go32-nat.c

[rfc] [17/17] Get rid of current_gdbarch in go32-nat.c

[Markus Deuling]

[-- Attachment #1: Type: text/plain, Size: 353 bytes --]

Hi,

this patch gets rid of some of the current_gdbarch's in go32-nat.c
Is this ok to commit?

ChangeLog: 


	* go32-nat.c (fetch_register, go32_fetch_registers, store_register)
	(go32_store_registers): Use get_regcache_arch to get at the current
	architecture by regcache.



-- 
Markus Deuling
GNU Toolchain for Linux on Cell BE
deuling@de.ibm.com




[-- Attachment #2: diff-go32 --]
[-- Type: text/plain, Size: 1733 bytes --]

diff -urpN src/gdb/go32-nat.c dev/gdb/go32-nat.c
--- src/gdb/go32-nat.c	2007-08-23 20:08:33.000000000 +0200
+++ dev/gdb/go32-nat.c	2007-10-11 10:36:08.000000000 +0200
@@ -465,7 +465,7 @@ go32_wait (ptid_t ptid, struct target_wa
 static void
 fetch_register (struct regcache *regcache, int regno)
 {
-  if (regno < gdbarch_fp0_regnum (current_gdbarch))
+  if (regno < gdbarch_fp0_regnum (get_regcache_arch (regcache)))
     regcache_raw_supply (regcache, regno,
 			 (char *) &a_tss + regno_mapping[regno].tss_ofs);
   else if (i386_fp_regnum_p (regno) || i386_fpc_regnum_p (regno))
@@ -482,7 +482,8 @@ go32_fetch_registers (struct regcache *r
     fetch_register (regcache, regno);
   else
     {
-      for (regno = 0; regno < gdbarch_fp0_regnum (current_gdbarch); regno++)
+      for (regno = 0;
+	   regno < gdbarch_fp0_regnum (get_regcache_arch (regcache)); regno++)
 	fetch_register (regcache, regno);
       i387_supply_fsave (regcache, -1, &npx);
     }
@@ -491,7 +492,7 @@ go32_fetch_registers (struct regcache *r
 static void
 store_register (const struct regcache *regcache, int regno)
 {
-  if (regno < gdbarch_fp0_regnum (current_gdbarch))
+  if (regno < gdbarch_fp0_regnum (get_regcache_arch (regcache)))
     regcache_raw_collect (regcache, regno,
 			  (char *) &a_tss + regno_mapping[regno].tss_ofs);
   else if (i386_fp_regnum_p (regno) || i386_fpc_regnum_p (regno))
@@ -510,7 +511,7 @@ go32_store_registers (struct regcache *r
     store_register (regcache, regno);
   else
     {
-      for (r = 0; r < gdbarch_fp0_regnum (current_gdbarch); r++)
+      for (r = 0; r < gdbarch_fp0_regnum (get_regcache_arch (regcache)); r++)
 	store_register (regcache, r);
       i387_collect_fsave (regcache, -1, &npx);
     }

Re: [rfc] [17/17] Get rid of current_gdbarch in go32-nat.c

[Eli Zaretskii]

> Date: Thu, 11 Oct 2007 10:54:25 +0200
> From: Markus Deuling <deuling@de.ibm.com>
> CC: Ulrich Weigand <uweigand@de.ibm.com>
> 
> this patch gets rid of some of the current_gdbarch's in go32-nat.c
> Is this ok to commit?
> 
> ChangeLog: 
> 
> 
> 	* go32-nat.c (fetch_register, go32_fetch_registers, store_register)
> 	(go32_store_registers): Use get_regcache_arch to get at the current
> 	architecture by regcache.

Sorry for asking this so late, but could you please explain the
reason(s) why these changes are a good idea, i.e. what potential
problem(s) are they trying to solve?  If I tell you that the go32
(a.k.a. DJGPP) native build of GDB supports only a single
architecture, would those reason(s) still hold?

Re: [rfc] [17/17] Get rid of current_gdbarch in go32-nat.c

[Markus Deuling]

Hi Eli,

Eli Zaretskii schrieb:
>> 	* go32-nat.c (fetch_register, go32_fetch_registers, store_register)
>> 	(go32_store_registers): Use get_regcache_arch to get at the current
>> 	architecture by regcache.
> 
> Sorry for asking this so late, but could you please explain the
> reason(s) why these changes are a good idea, i.e. what potential
> problem(s) are they trying to solve?  If I tell you that the go32
> (a.k.a. DJGPP) native build of GDB supports only a single
> architecture, would those reason(s) still hold?
> 

sorry for the late respone. I've been on vacation.
Please see here: http://sourceware.org/ml/gdb-patches/2007-10/msg00108.html

What I'll try to achieve is to get rid of the global variable current_gdbarch to have a real per-frame architecture. 

-- 
Markus Deuling
GNU Toolchain for Linux on Cell BE
deuling@de.ibm.com

Re: [rfc] [17/17] Get rid of current_gdbarch in go32-nat.c

[Eli Zaretskii]

> Date: Mon, 22 Oct 2007 08:07:40 +0200
> From: Markus Deuling <deuling@de.ibm.com>
> CC: gdb-patches@sourceware.org, uweigand@de.ibm.com
> 
> > Sorry for asking this so late, but could you please explain the
> > reason(s) why these changes are a good idea, i.e. what potential
> > problem(s) are they trying to solve?  If I tell you that the go32
> > (a.k.a. DJGPP) native build of GDB supports only a single
> > architecture, would those reason(s) still hold?
> > 
> 
> sorry for the late respone. I've been on vacation.
> Please see here: http://sourceware.org/ml/gdb-patches/2007-10/msg00108.html

Thanks for the pointer.  Unfortunately, it does not answer my question
above.  Perhaps the earlier thread (to which it refers without stating
a URL) does, in which case I'd like to read that earlier thread.

> What I'll try to achieve is to get rid of the global variable current_gdbarch to have a real per-frame architecture. 

Yes, but why?  It looks like getting rid of current_gdbarch is needed
to support the situation where multiple architectures are supported in
the same session (or maybe even in the same executable?).  That is why
I asked the second question above: the DJGPP native build of GDB
supports only a single architecture, and will ever support only that
single architecture.  So the question is: is there any particular
reason to get rid of current_gdbarch in go32-nat.c?

Re: [rfc] [17/17] Get rid of current_gdbarch in go32-nat.c

[Markus Deuling]

H Eli,

Eli Zaretskii schrieb:
>>
>>> Sorry for asking this so late, but could you please explain the
>>> reason(s) why these changes are a good idea, i.e. what potential
>>> problem(s) are they trying to solve?  If I tell you that the go32
>>> (a.k.a. DJGPP) native build of GDB supports only a single
>>> architecture, would those reason(s) still hold?
please see my explanation below.

> Thanks for the pointer.  Unfortunately, it does not answer my question
> above.  Perhaps the earlier thread (to which it refers without stating
> a URL) does, in which case I'd like to read that earlier thread.
You're right. This is the original posting from august this year:
http://sourceware.org/ml/gdb-patches/2007-08/msg00034.html

> 
>> What I'll try to achieve is to get rid of the global variable current_gdbarch to have a real per-frame architecture. 
> Yes, but why?  It looks like getting rid of current_gdbarch is needed
> to support the situation where multiple architectures are supported in
> the same session (or maybe even in the same executable?).  That is why
> I asked the second question above: the DJGPP native build of GDB
> supports only a single architecture, and will ever support only that
> single architecture.  So the question is: is there any particular
> reason to get rid of current_gdbarch in go32-nat.c?

current_gdbarch will disappear somewhen and GDB will work on a per-frame architecture base. There should
be no need to have some global variables for keeping track of current architecture, etc anymore (which is for my opinion
very fault-prone anyway). 

There is not only an advantage with our "combined binaries" which consist of two different architectures in one binary.
At the end one should be able to build GDB with all (nearly all ?!?) targets enabled and then debug all kinds of supported
binaries without the need to recompile or even restart it. This is a cool feature ;-) Whereas I cannot imagine a scenario
where GDB debugs for example an x86 binary and a PowerPC binary in one session.

Thats the reason to replace current_gdbarch piece by piece by appropriate other methods like get_frame_arch or get_regcache_arch
or (what is still under discussion) via a gdbarch pointer in struct objfiles. 

I'm not familiar with the particularities of DJGPP but I hope I was able to answer your question. 


-- 
Markus Deuling
GNU Toolchain for Linux on Cell BE
deuling@de.ibm.com

Re: [rfc] [17/17] Get rid of current_gdbarch in go32-nat.c

[Eli Zaretskii]

> Date: Tue, 23 Oct 2007 12:19:23 +0200
> From: Markus Deuling <deuling@de.ibm.com>
> CC: gdb-patches@sourceware.org, uweigand@de.ibm.com
> 
> > Thanks for the pointer.  Unfortunately, it does not answer my question
> > above.  Perhaps the earlier thread (to which it refers without stating
> > a URL) does, in which case I'd like to read that earlier thread.
> You're right. This is the original posting from august this year:
> http://sourceware.org/ml/gdb-patches/2007-08/msg00034.html

Well, that just says its purpose is to get rid of current_gdbarch, but
there's no serious discussion of the issue.  So I'm still wondering:
was this discussed, and if so, where can I read it?

> current_gdbarch will disappear somewhen and GDB will work on a per-frame architecture base.

I can understand why this is a Good Thing for ports that can actually
support multiple architectures.  But why is this a good idea for
single-architecture ports?  You are replacing a variable reference
with a function call, which is a slowdown.  That's the downside;
what's the upside, please?

To me, this looks like yet another case of good intentions that could
well lead to undesirable consequences for no good reason.  Like the
decision to link against every possible *read.c binary reader, no
matter whether the port has a need for it, it's an annoyance, both
because it's inelegant to bloat a program for want of generality, and
because it causes some very practical problems (link time, executable
size, long GDB startup time when debugging itself, etc.).  For
example, the DJGPP port of GNU ld was constantly complaining about
overflow in source line table (DJGPP uses COFF for binary files), just
because it linked in useless for DJGPP modules such as elfread.c,
mdebugread.c, mipsread.c, xcoffread.c, etc.  I no longer actively use
DJGPP, but I remember the annoyance to this day.

> There should
> be no need to have some global variables for keeping track of current architecture, etc anymore (which is for my opinion
> very fault-prone anyway). 

For ports that support only one architecture, this variable can be
computed once and then reused.  What's wrong with storing such values
in a global variable?

> At the end one should be able to build GDB with all (nearly all ?!?) targets enabled

Good God! you don't really mean that, do you?  What kind of bloated
GDB executable will we have when this happens?

> This is a cool feature ;-) Whereas I cannot imagine a scenario
> where GDB debugs for example an x86 binary and a PowerPC binary in one session.

If we cannot imagine such a scenario, why are we trying to support it?

Re: [rfc] [17/17] Get rid of current_gdbarch in go32-nat.c

[Daniel Jacobowitz]

On Tue, Oct 23, 2007 at 10:53:03PM +0200, Eli Zaretskii wrote:
> Well, that just says its purpose is to get rid of current_gdbarch, but
> there's no serious discussion of the issue.  So I'm still wondering:
> was this discussed, and if so, where can I read it?

I believe it was Andrew's goal as long ago as when he started adding
"gdbarch" arguments to methods.  I can't think of any specific
discussion, but I've been aware of this trend as long as I can
remember working on GDB.

> I can understand why this is a Good Thing for ports that can actually
> support multiple architectures.  But why is this a good idea for
> single-architecture ports?  You are replacing a variable reference
> with a function call, which is a slowdown.  That's the downside;
> what's the upside, please?

Even a single-architecture port may have more than one
current_gdbarch.  A gdbarch is fine-grained and e.g. different
executables can lead to different gdbarches.  So a GDB for DJGPP which
supported debugging two programs at once might need more than one
"current" gdbarch.

Also, getting rid of current_gdbarch is hard.  If we leave it
in some targets then we have to continue making it work; it'll
creep back in to ports that were trying to get rid of it.  I
think having more than one way to do this is not worthwhile.

> Good God! you don't really mean that, do you?  What kind of bloated
> GDB executable will we have when this happens?

FYI, I'd love to ship a single GDB binary that supported multiple
targets.  That's practical for our case.  I don't know if we would
turn on all targets or just a set list.

> > This is a cool feature ;-) Whereas I cannot imagine a scenario
> > where GDB debugs for example an x86 binary and a PowerPC binary in one session.
> 
> If we cannot imagine such a scenario, why are we trying to support it?

I can imagine it; people build such systems today, and some of
CodeSourcery's customers are interested in debugging them.  Of course,
that's not a terribly frequent example, but there's all sorts of
weird hybrids out there.  An ARM and a signal processor or a PowerPC
and eight SPUs are both real cases I'm familiar with.

-- 
Daniel Jacobowitz
CodeSourcery

Re: [rfc] [17/17] Get rid of current_gdbarch in go32-nat.c

[Eli Zaretskii]

> Date: Tue, 23 Oct 2007 17:15:28 -0400
> From: Daniel Jacobowitz <drow@false.org>
> Cc: Markus Deuling <deuling@de.ibm.com>, gdb-patches@sourceware.org,
> 	uweigand@de.ibm.com
> 
> Even a single-architecture port may have more than one
> current_gdbarch.  A gdbarch is fine-grained and e.g. different
> executables can lead to different gdbarches.  So a GDB for DJGPP which
> supported debugging two programs at once might need more than one
> "current" gdbarch.

Can you please describe an example where this is possible?  Perhaps I
don't understand what is gdbarch, but I think this is impossible with
DJGPP.

Re: [rfc] [17/17] Get rid of current_gdbarch in go32-nat.c

[Daniel Jacobowitz]

On Wed, Oct 24, 2007 at 06:04:58AM +0200, Eli Zaretskii wrote:
> > Date: Tue, 23 Oct 2007 17:15:28 -0400
> > From: Daniel Jacobowitz <drow@false.org>
> > Cc: Markus Deuling <deuling@de.ibm.com>, gdb-patches@sourceware.org,
> > 	uweigand@de.ibm.com
> > 
> > Even a single-architecture port may have more than one
> > current_gdbarch.  A gdbarch is fine-grained and e.g. different
> > executables can lead to different gdbarches.  So a GDB for DJGPP which
> > supported debugging two programs at once might need more than one
> > "current" gdbarch.
> 
> Can you please describe an example where this is possible?  Perhaps I
> don't understand what is gdbarch, but I think this is impossible with
> DJGPP.

A gdbarch includes lots of information.  Some examples:

  - What registers are available, including pseudo-registers and
  emulated registers.  E.g., if DJGPP supports running binaries
  with and without MMX support available more than one gdbarch might
  be needed.

  - The sizes of basic types.  E.g., if some versions of DJGPP used
  a 64-bit long double and other versions used an 80-bit long double.
  This transition seems to happen on many platforms at least once.

  - Mapping of debug info numbers to register numbers.  E.g., if two
  compilers used different numbers this might be handled by setting
  a different function pointer in the gdbarch depending on the loaded
  binary.

Maybe none of these apply now, but they may someday.

-- 
Daniel Jacobowitz
CodeSourcery

Re: [rfc] [17/17] Get rid of current_gdbarch in go32-nat.c

[Ulrich Weigand]

Daniel Jacobowitz wrote:
> On Tue, Oct 23, 2007 at 10:53:03PM +0200, Eli Zaretskii wrote:
> > I can understand why this is a Good Thing for ports that can actually
> > support multiple architectures.  But why is this a good idea for
> > single-architecture ports?  You are replacing a variable reference
> > with a function call, which is a slowdown.  That's the downside;
> > what's the upside, please?
> 
> Even a single-architecture port may have more than one
> current_gdbarch.  A gdbarch is fine-grained and e.g. different
> executables can lead to different gdbarches.  So a GDB for DJGPP which
> supported debugging two programs at once might need more than one
> "current" gdbarch.
> 
> Also, getting rid of current_gdbarch is hard.  If we leave it
> in some targets then we have to continue making it work; it'll
> creep back in to ports that were trying to get rid of it.  I
> think having more than one way to do this is not worthwhile.

Yes, that's my main concern as well.

However, getting back to the specific case of the go32-nat.c changes:

-  if (regno < gdbarch_fp0_regnum (current_gdbarch))
+  if (regno < gdbarch_fp0_regnum (get_regcache_arch (regcache)))

I actually agree with Eli that this is probably not the right way;
instead the "gdbarch_fp0_regnum (current_gdbarch)" should be replaced
by simply I386_ST0_REGNUM -- this is the only value gdbarch_fp0_regnum
can ever have on i386 targets.  (Of course, this will also get rid
of this particular use of current_gdbarch, which is my main concern.)

> > Good God! you don't really mean that, do you?  What kind of bloated
> > GDB executable will we have when this happens?
> 
> FYI, I'd love to ship a single GDB binary that supported multiple
> targets.  That's practical for our case.  I don't know if we would
> turn on all targets or just a set list.

Actually, I'm just preparing to send out a patch set to do just that :-)

The only remaining prerequisites are that the solib patches are applies,
and the final remaining TM file tm-frv.h is removed.

The patches will provide the --enable-targets= configure option, which
will work just the same as this option today works with binutils: you
will always get the "main" target (--target ...), and in addition
support for all targets listed with --enable-targets= is compiled in.
A special case is --enable-targets=all, which will support all targets
GDB has.  Those extra targets can be remote targets only; only the
main target can be the native target (when appropriate).

Bye,
Ulrich

-- 
  Dr. Ulrich Weigand
  GNU Toolchain for Linux on System z and Cell BE
  Ulrich.Weigand@de.ibm.com

Re: [rfc] [17/17] Get rid of current_gdbarch in go32-nat.c

[Eli Zaretskii]

> Date: Wed, 24 Oct 2007 07:47:45 -0400
> From: Daniel Jacobowitz <drow@false.org>
> Cc: deuling@de.ibm.com, gdb-patches@sourceware.org, uweigand@de.ibm.com
> 
> A gdbarch includes lots of information.  Some examples:
> 
>   - What registers are available, including pseudo-registers and
>   emulated registers.  E.g., if DJGPP supports running binaries
>   with and without MMX support available more than one gdbarch might
>   be needed.
> 
>   - The sizes of basic types.  E.g., if some versions of DJGPP used
>   a 64-bit long double and other versions used an 80-bit long double.
>   This transition seems to happen on many platforms at least once.
> 
>   - Mapping of debug info numbers to register numbers.  E.g., if two
>   compilers used different numbers this might be handled by setting
>   a different function pointer in the gdbarch depending on the loaded
>   binary.

Thanks, I'm convinced.

Btw, the above sounds like something that would be good to have in
gdbint.texinfo, if you get my drift ;-)