Re: [PATCH v3 2/4] RISC-V: Support the ISA-dependent CSR checking.

[Jim Wilson <jimw at sifive dot com>]

On Sun, Dec 15, 2019 at 9:23 PM Nelson Chu <nelson.chu@sifive.com> wrote:
>         gas/
>         * config/tc-riscv.c (enum riscv_csr_class): New enum.  Used to decide
>         whether or not this CSR is legal in the current ISA string.
>         (riscv_csr_extra): New structure to hold all extra information of CSR.
>         (riscv_init_csr_hash): New function.  According to the DECLARE_CSR and
>         DECLARE_CSR_ALIAS, insert CSR extra information into csr_extra_hash.
>         Call hash_reg_name to insert CSR address into reg_names_hash.
>         (md_begin): Call riscv_init_csr_hashes for each DECLARE_CSR.
>         (reg_csr_lookup_internal, riscv_csr_class_check): New functions.
>         Decide whether the CSR is valid according to the `csr_extra_hash`.
>         * testsuite/gas/riscv/priv-reg-fail-fext.d: New testcase.  The source
>         file is `priv-reg-all.s`, and the ISA is rv32i without f-ext, so the
>         f-ext CSR are not allowed.
>         * testsuite/gas/riscv/priv-reg-fail-fext.l: Likewise.
>         * testsuite/gas/riscv/priv-reg-fail-rv32-only.d: New testcase.  The
>         source file is `priv-reg-all.s`, and the ISA is rv64if, so the
>         rv32-only CSR are not allowed.
>         * testsuite/gas/riscv/priv-reg-fail-rv32-only.l: Likewise.
>
>         include/
>         * opcode/riscv-opc.h: Extend DECLARE_CSR and DECLARE_CSR_ALIAS to
>         record riscv_csr_class.
>
>         opcodes/
>         * riscv-dis.c (print_insn_args): Updated since the DECLARE_CSR is changed.
>
>         gdb/
>         * riscv-tdep.c: Updated since the DECLARE_CSR is changed.
>         * riscv-tdep.h: Likewise.
>         * features/riscv/rebuild-csr-xml.sh: Generate the 64bit-csr.xml without
>         rv32-only CSR.
>         * features/riscv/64bit-csr.xml: Regernated.
>
>         binutils/
>         * dwarf.c: Updated since the DECLARE_CSR is changed.

Overall, this one looks OK to me.

But there are gdb changes that require gdb review and approval.  It is
OK to send an email to both binutils and gdb-patches at the same time
when it needs approval from both.  And unfortunately I'm not a gdb
reviewer but I can give a thumbs up on the gdb patches.

This conflicts with the first patch which isn't approved yet because
of questions about which version we are implementing, and whether we
are implementing a draft.

And there are more xml file changes, which ideally should be updated
in qemu too, but we can probably handle that as a separate issue.

I see some functions missing comments.  This is a minor problem, but I
think the style guidelines still require an explanatory comment before
each function.

When I ran the testsuite for a riscv64-elf target, the csr-dw-regnums
test fails.
/home/jimw/FOSS/BINUTILS/binutils-gdb/gas/testsuite/gas/riscv/csr-dw-regnums.s:53:
Error: unknown register `cycleh'
followed by a lot of similar errors.  This will need to use
-march=rv32if as is already done with priv-reg-all.d.

Jim