Re: MIPS: Handle the DSP registers

[Daniel Jacobowitz <drow at false dot org>]

On Wed, Mar 26, 2008 at 04:58:28PM +0000, Maciej W. Rozycki wrote:
>  Well, I am actually unsure whether I really *need* it (which probably 
> means I do not), but it does not look like a good engineering practice to 
> have this initial meta-state.  I suppose no description should be 
> available before a target has been connected to or a native debuggee 
> process started so that it is seen when some code somewhere tries to 
> access this effectively inexistent state.

There will be no description unless the architecture supplies a
default one.

The initialization sequence does not look like it would if I'd been
writing a debugger from scratch, I admit.  It was very tangled up
but this is the best compromise I could come up with.

> > ...this.  The register numbers don't have to be different.  You've
> > added a new internal register numbering for Linux; that isn't
> > necessary.  Where are Linux and bare metal different?  Is it in an
> > existing stub, and if so, what registers does that stub report?
> 
>  DSP registers were added pretty late in the game when CP0 registers had 
> already been defined for bare iron, so there is no way to have the same 
> numbers for Linux and bare iron (unless we want to have a hole in the 
> Linux register space).  They were added at the end of the already defined 
> ranges, respectively which is I think reasonable.  And these numbers have 
> to match ptrace() and the existing remote debug stubs (e.g. YAMON), don't 
> they?
> 
> > GDB internal register numbers are used for two purposes.  They are the
> > default for the remote P/p and T packet numbering, and the g/G packet
> > ordering.  If a target description describes the registers,
> > tdesc_remote_register_number is used instead.  And they are passed to
> > GDB functions and determine the register cache layout.  This is
> > internal; just be consistent.
> 
>  Obviously the numbers returned by tdesc_remote_register_number() are 
> purely internal, but the "legacy" ones are seen externally as mentioned 
> above; or am I getting confused here?

You've got this backwards.  The numbers _taken_ by
tdesc_remote_register_number are internal.  The numbers returned by it
are the external interface with a remote stub.

If the remote stub implements the new registers and p/P, or returns
the new registers in T responses (both reasonably unlikely) then the
numbers matter.  If it only implements the new registers and g/G,
the numbers do not matter but the ordering of non-zero-sized registers
determines the layout of the g/G packet.

Ptrace isn't affected by these numbers at all; translation is done
by mips_linux_register_addr for instance.

If there are no existing remote stubs (ignoring gdbserver, here, newer
versions of gdbserver will use XML) that transmit the new
registers, then there's no need to give them any particular numbers.
If there are existing stubs you want to preserve, then we can add
a translation method if you can tell us what the actual register
layout you want is.

> > The regcache.c/regcache.h bits are now unused; they were for your
> > inf-ptrace.c change.
> 
>  The MDI change still relies on these -- I'll migrate them there, thanks 
> for spotting.
> 
>  Hmm, actually I have just spotted I have already committed a build-fix 
> change to remote-mips.c that I submitted a while ago that makes the file 
> rely on the function; still it is not built without the MDI change, so I 
> propose not to take them out and wait for MDI to get in.

Does the MDI patch rely on passing "1" to the new function?  If not,
please just use regcache_invalidate.  Speaking about good engineering
design, a function which marks cached data as valid without putting
in any data is pretty suspicious.

>  The kernel is wrong -- with MIPS64 processors additional HIx/LOx 
> registers are 64-bit as the traditional HI/LO ones (which, if you notice 
> instruction encoding, could as well be called HI0/LO0 now) -- all the four 
> provide the same semantics.  The control register is 32-bit-wide in all 
> cases.

Then an N32 GDB is not going to be able to read these registers unless
we create a regset for them, FYI.  ptrace takes and returns longs on
n32, not long longs.  I experimented with using long long, but it was
a terrible mess.

>  It just looked wrong to me to depend on a preprocessor macro where we 
> strive to make GDB multi-platform.  Your point is of course valid, but 
> after a little thought I believe my change is right anyway.  Given a 
> 64-bit kernel and an (n)64 or n32 GDB binary you may want to debug an o32 
> executable; I have not investigated how ptrace() is meant to work in such 
> a scenario and I do not insist on getting rid of the macro dependency with 
> this patch, but this is something to be kept in mind in my opinion.

That exact requirement is why it's written the way it is.  If GDB is
64-bit, then we can always read 64-bit registers from the target; the
pseudo ABI registers will be the right size for o32.

>  These are the "ACcumulator eXtension" registers as defined by the 
> SmartMIPS ASE.  One is added per each HI/LO pair; though to the best of my 
> knowledge nobody has manufactured a MIPS core with the DSP and SmartMIPS 
> ASEs both included at the same time, hence only a single ACX register (for 
> the traditional HI/LO pair) has ever been in implemented in existing 
> hardware.

I see, thanks.

>  I have noticed I missed register format descriptions (what are they for 
> anyway? -- that don't seem to be documented in the obvious places) and 

They're for gdbserver; they define its register cache.  If I wrote the
missing XML files for all existing gdbserver targets, I could replace
them with pregenerated C files using xsltproc (just like I did for the
ones currently in features/*.c); I haven't gotten round to it.

> fixed a couple of issues that I discovered in testing too.  Here is a new 
> version.  As native regression testing takes quite a while, I will do this 
> once we agree on a reasonable candidate for commission.

Sounds fine.  If you'll forgive me, I'll wait to look at the patch
until we resolve the issues above.

-- 
Daniel Jacobowitz
CodeSourcery