This is the mail archive of the insight@sources.redhat.com mailing list for the Insight project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]

Pending GDB change


Just FYI,

Attatched are two e-mails that discuss changes that are going to
directly affect insight.

The first is to do with the registers[] array.  The last time I looked,
insight poked around in the registers[] array directly.  The intention
is to deprecate that entire interface.

The second describes a structural change to gdb.  For insight this
points the way towards a more legetimate mechanism for accessing
registers.

enjoy,
	Andrew


David Taylor wrote:
> 
>     From: Andrew Cagney <ac131313@cygnus.com>
>     Date: Tue, 27 Feb 2001 20:38:30 -0500
> 
>     David Taylor wrote:
> 
>     > I propose that we:
>     >
>     > . add register_fetched
> 
>     David,
> 
>     The functionality of register_fetched() overlaps with
>     set_register_cached() and supply_register().  Rather than add a
>     redundant method, could an existing interface be used or the current
>     interfaces rationalized slightly?
> 
> Andrew,
> 
> Supply register does more than register_fetched; register_fetched only
> affects register_valid -- the register must have been supplied via
> some other method.

Yes, that is what I mean by their functionality overlapping.  There is
much legacy code of the form:

	blat the registers[] array
	set register_valid[] to 1

The accepted ``correct fix'' for that code is to replace it with
supply_register().  It is just an unfortunate reality that no one has
the time to do the work. I have a feeling that, right now GDB is
deleting this code faster than it is being fixed!

Given that reailty, if this code is going to be changed, I would prefer
it changed in a way that made it very clear that this is not the correct
way to do things.  Taking a lead from DEPRECATED_SERIAL_FD I think those
blocks can be converted to something like:

	for (;;)
	  *deprecated_register_buffer(...) = ...;
	deprecated_register_valid();

> set_register_cached is -- with one exception -- only used within
> regcache.c.  The one exception is remote.c (remote_fetch_registers).
> 
> I feel that a function should be created (register_unavailable?) and
> the call in remote.c to set_register_cached replaced with that call.
> Then set_register_cached should be made static.
> 
> To call set_register_cached, you have to know what the meanings are of
> the various possible values of register_valid.  This is knowledge that
> shouldn't really exist outside of regcache.c.

Yes.  My suggestion, hinted at in the last e-mail, would be to combine
the two disjoint operations:

	supply_register(....);
	set_register_cache(..., -1)

into an atomic cache transaction:

	supply_unavailable_register(....)

>     Keep in mind that the long term goal is to tighten regcache's interface
>     signficantly.  That is, eliminate register_valid[], registers[] and
>     possibly even set_register_cached() replacing them with a small set of
>     functions such as:
>             supply_register()
>             supply_unavailable_register()
>     If you are thinking of proposing further changes then you may want to
>     keep that in mind.
> 
> My change advances the goal of eliminating register_valid!  It reduces
> significantly the number of files that even know that register_valid
> exists!  Outside of regcache.c, only two files would reference it (for
> a total of three references).  Those two files could also have their
> references to it removed with a little bit more work.

I agree, eliminating register_valid[] and registers[] are important
goals.  The thing to be wary of is a change that just substitutes
something like registers[] and register_valid[] without actually
improving the overall quality of the code.  I think it is important to
ensure that any proposed change to GDB moves its overall code quality
forward (and not just sideways).

Grubbing around for other cases.

--

Grubbing through the code, the other case where register_valid[] is
being blatted is:

  /* These (for the most part) are pseudo registers and are obtained
     by other means.  Those that aren't are already handled by the
     code above.  */
  for (regi = IA64_GR32_REGNUM; regi <= IA64_GR127_REGNUM; regi++)
    register_valid[regi] = 1;
  for (regi = IA64_PR0_REGNUM; regi <= IA64_PR63_REGNUM; regi++)
    register_valid[regi] = 1;
  for (regi = IA64_VFP_REGNUM; regi <= NUM_REGS; regi++)
    register_valid[regi] = 1;

I'm guessing that code somewhere is looking at register_valid[] to
decide if a pseudo is available.  The actual value (hopefully) being
supplied by the pseudo-register method.

While that particular hack could eventually be fixed in other ways
(short term by having the ``register_cached()'' method ask gdbarch if a
pseudo is valid; and long term by having the register_valid() method
bound to a frame and always asking gdbarch if i is valid) that ain't
going to solve the hear and now.

Given that, I think this case (pseudo) is different/separate to the
legacy code case, I don't think it makes sense to use an interface like
deprecated_register_valid().  Instead, I think a separate:

	pseudo_register_valid(REGNUM)

Interface should be available.  I think this also helps to differentiate
the more recent pseudo from legacy code.

--

The other case I'm aware of, is with registers[] and core-gdb poking
around in that instead of using the read/write register functions.  I
think, there, the deprecated_register_buffer() method would be fine. 
That code should be using the read_register*() functions and not pokeing
around in that buffer.

>     With regard to regcache.h, yes the two clash.  It both moves code around
>     and changes the set_register_cached() interface.  If anything regcache.h
>     makes life easier because it is finally clear what the regcache
>     interfaces really are.
> 
>             Andrew
> 
> What change is this that you are referring to?  The message with subject
> 
>     [rfc] Re-merged regcache.h patch

I also mentioned:

        o       The changes adding:
+ #define REGISTER_CACHE_UNAVAILABLE (1)
+ #define REGISTER_CACHE_VALID       (0)
+ #define REGISTER_CACHE_INVALID     (-1)
                (or enum equivalents) were stripped out.
                These changes badly clashed with Davids
                proposed register_*() change so, I'll
                leave them for later.

I was actually wrong here.  The changes were only in my local sandpit
and not in the original patch.

>   +/* Character array containing the current state of each register
>   +   (unavailable<0, valid=0, invalid>0). */

See inferior.h:register_valid.  I'll fix the comment.  Thanks for
noticing this.

----

Any way, to summarize, I'm proposing:

	replace
		supply_register();
		set_register_cached();
	with:
		supply_unavailable_register()

	replace
		blat registers[];
		register_valid[] = 1;
	with
		blat deprecated_register_buffer(...);
		depreciated_register_valid/fetched(...);
	where this I/F would only work on raw registers.

	replace
		registers_valid[pseudo] = 1;
	with
		pseudo_register_valid/fetched(REGNUM)
	where the register must be a pseudo.

	replace remaining
		... = registers[];
	with
		... = depreciated_register_buffer();

I think this is a reasonable balance between the immediate desire of
eliminate the globals registers_valid[] and registers[] and the long
term objective of eliminating code that blats arbitrary bytes in the
registers[] array.

	Andrew





Hello,

(For all two of you that haven't figured it out, I'm back at working on
multi-arch :-)

This e-mail first discusses multi-arch and some of its requirements.  It
then goes on to propose a number of significant but related structural
changes to GDB that help it take a step closer to being multi-arch.

----

Multi-arch is the grand plan that one day GDB will be able to do things
like debug a target that contains several different architectures.  The
classic embedded example would be the Play Station 2 while the UNIX
example is for platforms like Solaris where both 32 and 64 bit binaries
are present.

Several steps in this plan were identified (with any long term project,
the more distant the work, the more vague the details :-):

	o	Replace hardwired macros with run-time
		tests/calls to an architecture vector.

	o	Modify GDB's configury so that
		different architectures could
		be incorporated into a single GDB.

	o	Modify GDB so that it can debug
		a system that contains several
		different architectures.

		This was identified as the tricky bit.

Right now, I think things are about ready for step two (hopefully fairly
straight forward) so it is time to plan for step three.

Hmm.

----

When multi-arch was first discussed it was fairly clear that GDB was
going to need to be parameterize with something.  At the time, it was
thought that during stage three everything would be parameterized with
either a large architecture vector or an even larger merged architecture
/ target vector.

Hmm.

----

In the mean time, Kevin B has been looking at TPID, that global integer
which is used to identify the current thread.  A pretty logical chain of
thought leads to the conclusion that, since each thread can have a
different architecture, each thread should have an arcitecture bound to
it.

So rather than parameterize everything with the architecture, why not
instead parameterize everything with a thread object.

Hmm.

----

One of the very long term goals of GDB is to be able to debug RPC code. 
Looking at what appears to be a simple debugging example:

	(gdb) step
	10	do_rpc_to_machine_foo();
	(gdb) step
	do_rpc_to_machine_foo() at foo.c:50
	50	printf ("Hello world\n");
	(gdb)

Examing the stack frame, and remembering the idea is that this is RPC
code:

	(gdb) where
	#0  do_rpc_to_machine_foo() at foo::foo.c:50
	#1  0x804883d in main () at blah::blah.c:10
	(gdb) info architecture
	The architecture is foo.
	(gdb) up
	#1  0x804883d in main () at blah::blah.c:10
	10	do_rpc_to_machine_foo();
	(gdb) info architecture
	The architecture is blah

The architecture is clearly bound to the frame rather than the thread. 
A thread has a frame,  a frame has an architecture.  So why not pass the
frame around everywhere?

Hmm.

----

Looking at the existing code base,I think it is clear that much it isn't
worried about threads or architectures.  Rather it is worried about
frames.  To do a stack backtrack, you need a frame.  To list the current
source, you (often - PIC) need a frame. To examine variables you need a
frame.  To do an inferior function call, you need a frame.

In fact, when the target stops, the second thing GDB does (after
checking that the stop is for real) is to create a frame.  On a target
that doesn't have a frame, GDB fakes it.  If GDB can't fake a frame,
things get pretty sick.

In a way, this shouldn't be unexpected.  GDB is a debugger designed for
debugging procedural languages that, in some way resemble C.  Much of a
procedural language revolves around the stack frame.

Hmm.

----

A number of GDB's existing targets have limited runtime support for
architecture variants (THUMB/ARM and MIPS16/MIPS).  For these targets
the ISA may change between stack frames.  At present this is implemented
by wrapping all architecture specific code in ``if (ISA_A) ... else
...;''.  Such code would clearly immediatly benefit from a change that
bound the architecture to the frame.

Hmm.

----

Given all this, I'd like to propose the following structural changes to
GDB.

	o	The frame have an architecture
		attached to it.

		As an intermediate hack, current
		architecture and current frame would
		remain as globals.

	o	All the functions that apply to
		the frame be parameterized with a
		frame argument and modified to
		use the frame's architecture.

	o	(Per previous e-mail)
		The frame and its registers be more
		clearly separated from the target
		(in particular the regcache).

		Most calls that go directly to the
		regcache will instead go via the
		current frame.

		A consequence of this is that the
		current need for the RAW / PSEUDO
		/ NATURAL register mess will be
		eliminated.  Yessss!

While looking simple, these changes are certainly everything but.  Every
frame / regcache / memcache access will need to be examined / modified. 
Fortunately, most of these uses can be examined independently so the
work can be carried out incrementally.

Clearly this change, on its own, won't be sufficient to make GDB
multi-arch.  I would argue, however, that like the initial multi-arch
work, it is a clear step in the right direction.

With that in mind, I'm looking for comments, questions and suggestions.  

----

Finally.  I'd like to thank Jim Blandy, David Taylor, Fernando Nasser,
Kevin Buettner, Elena Zannoni and Michael Snyder (who else? Red Hat) for
providing the support and suggestions needed to develop this idea.

	Andrew




Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]