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

[Andrew Burgess <andrew.burgess@embecosm.com>]

* Nelson Chu <nelson.chu@sifive.com> [2020-06-10 17:31:00 +0800]:

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

Nelson,

Thanks for your feedback.  Do you plan to post a patch introducing the
DECLARE_CSR_ALIAS_* macros soon(ish) or should I go ahead with the
current version of my patch, and if/when you add the new macros you
can update GDB to only use the one we care about, and drop the version
check at that point?

Thanks,
Andrew