This is the mail archive of the
binutils@sourceware.org
mailing list for the binutils project.
Re: [PATCH v3 3/4] RISC-V: Support the read-only CSR checking.
- From: Jim Wilson <jimw at sifive dot com>
- To: Nelson Chu <nelson dot chu at sifive dot com>
- Cc: Binutils <binutils at sourceware dot org>
- Date: Fri, 31 Jan 2020 18:38:18 -0800
- Subject: Re: [PATCH v3 3/4] RISC-V: Support the read-only CSR checking.
- References: <1576473701-11072-1-git-send-email-nelson.chu@sifive.com> <1576473701-11072-4-git-send-email-nelson.chu@sifive.com>
On Sun, Dec 15, 2019 at 9:22 PM Nelson Chu <nelson.chu@sifive.com> wrote:
> gas/
> * config/tc-riscv.c (insn_with_csr): New boolean to indicate we are
> assembling instruction with CSR.
> (enum csr_insn_type): New enum is used to classify the CSR instruction.
> (riscv_csr_insn_type, riscv_csr_read_only_check): New functions. These
> are used to check if we write a read-only CSR by the CSR instruction.
> (riscv_ip): Call riscv_csr_read_only_check after parsing all arguments.
> * testsuite/gas/riscv/priv-reg-fail-read-only-01.s: New testcase. Test
> all CSR for the read-only CSR checking.
> * testsuite/gas/riscv/priv-reg-fail-read-only-01.d: Likewise.
> * testsuite/gas/riscv/priv-reg-fail-read-only-01.l: Likewise.
> * testsuite/gas/riscv/priv-reg-fail-read-only-02.s: New testcase. Test
> all CSR instructions for the read-only CSR checking.
> * testsuite/gas/riscv/priv-reg-fail-read-only-02.d: Likewise.
> * testsuite/gas/riscv/priv-reg-fail-read-only-02.l: Likewise.
This makes insn_with_csr a global variable. It looks like it should
be local to riscv_ip.
The testcases are using 1.12 draft csrs, which makes it dependent on
the first patch in the series.
> + error = _("Read-only CSR is used");
The error looks a little odd to me. Using implies that you are
reading it, which is OK for a read-only CSR. I would suggest "written
to" instead of "used". That makes the error more obvious.
Also, maybe this should be a warning instead of an error. This is a
valid instruction after all, it is just one that will always give a
run-time trap. But it is possible that people might be writing code
this way for a reason, for instance, maybe a hardware verification
testsuite to test whether the instruction does give a trap exactly as
it is supposed to.
There is a function without an explanatory comment before it which is
a minor issue.
Otherwise this looks OK.
Jim