Re: [RFC] gdb/riscv: Add target description support

[Andrew Burgess <andrew dot burgess at embecosm dot com>]

* John Baldwin <jhb@FreeBSD.org> [2018-11-08 10:32:57 -0800]:

> On 11/8/18 8:07 AM, Andrew Burgess wrote:
> > This commit adds target description support for riscv.
> > 
> > I've used the split feature approach for specifying the architectural
> > features, and the CSR feature is auto-generated from the riscv-opc.h
> > header file.
> 
> In general this looks fine to me (as far as I am familiar with the
> target descriptions).  The only possible question/comment I have is if
> you considered describing fields of specific registers such as the FP
> status registers or MSTATUS, etc. as fields in the XML to replace the
> current special cases in riscv_print_one_register_info().

I took a look at switching over to using flag fields in the xml
description, but in the end I decided against this.

The main reason is that there appears to be a bug with registers
described as a flag type, I was unable to assign to the register.
What I saw was an invalid cast error.

I defined the type like this:

    <flags id="riscv_fflags" size="1">
      <field name="NX" start="0" end="0"/>
      <field name="UF" start="1" end="1"/>
      <field name="OF" start="2" end="2"/>
      <field name="DZ" start="3" end="3"/>
      <field name="NV" start="4" end="4"/>
    </flags>

    <reg name="fflags" bitsize="32" type="riscv_fflags"/>

But then, when I try to assign to the register I see this error:

    (gdb) set $fflags=0x3
    Invalid cast

This same assignment works fine when the register is a straight
integer.

Next, I actually prefer the current 'info register $fflags' output,
with all of the fields displayed all the time, so taking that away
would feel like a step backward, though I could be convinced if
everyone else feels strongly the other way.

In the end I think that there's some issues that need to be resolved
before we could move to using the <flags> type.  I think this is
something we should consider, but I'd like to kick this down the road
for now.

>                                                            I think the
> XML can't handle enum values as riscv_print_one_register_info() uses for
> some cases, but I think it would be able to handle many of the special
> cases in that function.

I agree, currently we're not going to be able to completely kill the
info registers override, and as a result I'm not that fussed about
having extra cases in here.

> 
> Some related-ish questions (though not about this patch): I wonder if we
> can do things with pseudo registers to automatically derive FFLAGS and
> FRM if a target provides FCSR.

I like this idea a lot, and I definitely plan to implement this, just
because I think it would be a neat feature.  However, my understanding
is that FFLAGS and FRM _are_ real CSRs, at least in the sense that
there's a real CSR offset from which we can read to extract the FFLAGS
or FRM part of FCSR.  So, initially I'd prefer to merge this with
these registers as real registers.

But I think it would be great if for targets that only announce FCSR,
we could automatically emulate FRM and FCSR.

Thank you for your review.  I plan to post a revised patch soon.

Thanks,
Andrew