[RFC] gdb/riscv: Improved register alias name creation

[Nelson Chu]

Hi Andrew,

The patch is pretty good to me.  Thanks for your quick reply and fix.
I have some ideas and minor things when I try to support debug csr for
now.

On Wed, Jun 10, 2020 at 6:47 AM Andrew Burgess
<andrew.burgess@embecosm.com> wrote:
> * Jim Wilson <jimw@sifive.com> [2020-06-09 13:14:58 -0700]:
> > On Tue, Jun 9, 2020 at 10:30 AM Andrew Burgess
> > <andrew.burgess@embecosm.com> wrote:
> > > Looking then at the final PRIV_SPEC_CLASS_* field for each alias then
> > > we can see that currently we only want to take the alias from
> > > PRIV_SPEC_CLASS_1P11.  For now then this is what I'm using to filter
> > > the aliases within GDB.
> >
> > This will do the right thing, but looks a little funny.  It isn't
> > quite the right way to express what we want.  I do think it is OK for
> > now, but we will have to be careful when maintaining binutils that we
> > don't break this assumption, or remember to update it when necessary.
>
> I agree.  I certainly open to any other ideas.
>
> Without making changes to the DECLARE_CSR_ALIAS macro (and I don't
> know what changes I would make) I saw my options as either:
>
>  - Ignore DECLARE_CSR_ALIAS, and hard code the "approved" aliases into
>    GDB.  Then it'll never break, we just need to remember to update
>    the hard coded list when riscv-opc.h changes, or
>
>  - Filter the alias list from riscv-opc.h.
>
> I went with the second option, partly because, if, from now on RISC-V
> doesn't reuse old CSR offsets for new CSRs, then any new aliases
> should be compatible.... I hope.

I think we already have a consensus - It would be great if we only
support an alias with a new name, points at an existing CSR offset,
and the new name is a synonym for the existing CSR at that offset.
For the current FSF tree, there is only one CSR dscratch is an alias.
But I'm going to add the missing debug CSR to upstream, so there
should be more aliases in the future, and I think all of them follow
our consensus.

DECLARE_CSR_ALIAS(dscratch, CSR_DSCRATCH0, CSR_CLASS_DEBUG,
PRIV_SPEC_CLASS_NONE, PRIV_SPEC_CLASS_NONE)
DECLARE_CSR_ALIAS(mcontrol, CSR_TDATA1, CSR_CLASS_DEBUG,
PRIV_SPEC_CLASS_NONE, PRIV_SPEC_CLASS_NONE)
DECLARE_CSR_ALIAS(icount, CSR_TDATA1, CSR_CLASS_DEBUG,
PRIV_SPEC_CLASS_NONE, PRIV_SPEC_CLASS_NONE)
DECLARE_CSR_ALIAS(itrigger, CSR_TDATA1, CSR_CLASS_DEBUG,
PRIV_SPEC_CLASS_NONE, PRIV_SPEC_CLASS_NONE)
DECLARE_CSR_ALIAS(etrigger, CSR_TDATA1, CSR_CLASS_DEBUG,
PRIV_SPEC_CLASS_NONE, PRIV_SPEC_CLASS_NONE)
DECLARE_CSR_ALIAS(textra32, CSR_TDATA3, CSR_CLASS_DEBUG,
PRIV_SPEC_CLASS_NONE, PRIV_SPEC_CLASS_NONE)
DECLARE_CSR_ALIAS(textra64, CSR_TDATA3, CSR_CLASS_DEBUG,
PRIV_SPEC_CLASS_NONE, PRIV_SPEC_CLASS_NONE)

I think mcontrol, icount, itrigger and etrigger are the aliases to
tdata1.  They all use 0x7a1 offset, and use type [top 5 bits] to
determine which one is used, so I believe they are similar to dscrach
and textra[32|64], and it's fine to define them by DECLARE_CSR_ALIAS.
The problem is that the debug csr and float csr actually belong to the
"unprivileged" CSR rather than privileged ones.  That means, they
should be controlled by the debug specs and float specs rather than
the privileged specs.  And in the future, vector CSR are also the
unprivileged ones controlled by the vector specs.  But for now we
don't have a conclusion on how to let users choose the unprivileged
specs they want.  Therefore, I plan to set their defined and aborted
versions to PRIV_SPEC_CLASS_NONE temporarily, including their aliases
(DECLARE_CSR_ALIAS).  And the PRIV_SPEC_CLASS_NONE will be changed to
DEBUG_SPEC_CLASS_XXX and VECTOR_SPEC_CLASS_XXX in the future,
according to their CSR_CLASS_DEBUG and CSR_CLASS_V.  This will affect
your current proposal, so I have an idea,

How about Gdb creates the aliases just according to the
DECLARE_CSR_ALIAS, and don't need to care about the spec versions for
now.  Binutils have to make sure that the CSR, which is defined by
DECLARE_CSR_ALIAS, must be the case like dscrach and itrigger
mentioned above.  For the ubadaddr, sbadaddr and others, I assume we
will drop them in the future and don't need to worry about them.  So I
prefer to use another macro rather than DECLARE_CSR_ALIAS to define
them.  Maybe DECLARE_CSR_ALIAS_TEMP or DECLARE_CSR_ALIAS_1p10?

I'm fine to use different ALIAS macros, since there are many meanings
for them, and Gdb can choose which ALIAS macros to support.  Maybe
someday Gdb can recognize the PRIV_SPEC_CLASS_XXX, and what we worry
about, like misa and ubadaddr, may not be the troubles anymore.  But
your current proposal is pretty good to me, so we can discuss this
later when we actually meet the problems. :)

Thanks
Nelson