riscvarchive / riscv-binutils-gdb

RISC-V backports for binutils-gdb. Development is done upstream at the FSF.
GNU General Public License v2.0
148 stars 232 forks source link

Riscv binutils 2.33.1 rvv 0.8.x error report #195

Closed Nelson1225 closed 4 years ago

Nelson1225 commented 4 years ago

Do not merge these commits. I just record them here, and appreciate for any feedback :)

Currently, RISC-V assembler just report "Error: unrecognized opcode" or "Error: illegal operands" for lots of error cases when parsing the instructions (riscv_ip). It would be nice if we can give more error information to user. MIPS is doing this good, so the basic idea is come from them.

There are five commits to improve the current error report,

  1. RISC-V: Improve the assembler's error reporting
  2. RISC-V: Replace as_bad with set_insn_error to avoid redundant error messages
  3. RISC-V: Add new error type to handle the internal error for opcode table
  4. RISC-V: Replace as_bad with set_insn_error for RVV imm fields
  5. RISC-V: Improve the assembler's error reporting for RVV

The first three commits are more suited to commit to upstream, and the last two commits are used to improve the error report for RVV. If everyone is fine for these commits, then I will send the first three commits to the upstream. Please see the details in the commits' comment, thanks :)

Basically, there are three stages for parsing one instruction. First, we find all candidates which have the same opcode name, but have different formats in the riscv_opcodes table. If the opcode name isn't defined in the table, then we should report "Error: unrecognized opcode". Second, we check the insn_class and xlen_requirement of the opcode candidate to determine whether it is valid for the current -march setting and RV32/RV64. If it is invalid, then we should report "Error: invalid instruction ...". After that, we check if each operand is legal, otherwise, the "Error: illegal operand" should be reported. We compare the args, which describe the syntax of operands, in the opcode table. And then call the match_func to do the final checking. The match_func have many uses, but we only need to report the error message for which are used to check the instruction constraints. As for the args comparing, it is more complicated, so I report at least which operand is invalid for them. Beside, the error information may be set multiple times during parsing. For now I just report the error for the last opcode candidate.

All of the above mentioned are implemented in the first commit. Now user can get more information about their wrong assembly instruction,

"Error: illegal operand rd can not be x0 `c.addiw x0,10'" (match_rd_nonzero)
"Error: illegal operand crs2 can not be x0 `c.mv x1,x0'" (match_c_add)
"Error: illegal operand 2 `fence w,'" (empty operand)
"Error: illegal operand 1 `c.srai x1,0x10'" (rd must be x8-x15)

The second and third commits are used to avoid redundant error messages. Consider the following case:

.insn r MADD, 0x0, 0x4, a0, a1, a2, a3 # F2 field must be 0...3

then the assembler reports the following errors:

test.s: Assembler messages:
test.s:1: Error: bad value for funct2 field, value must be 0...3
test.s:1: Error: bad value for funct2 field, value must be 0...3
test.s:1: Error: bad value for funct2 field, value must be 0...3
test.s:1: Error: bad value for funct2 field, value must be 0...3
test.s:1: Error: bad value for funct2 field, value must be 0...3
test.s:1: Error: bad value for funct2 field, value must be 0...3
test.s:1: Error: bad value for funct2 field, value must be 0...3
test.s:1: Error: bad value for funct2 field, value must be 0...3
test.s:1: Error: bad value for funct2 field, value must be 0...3
test.s:1: Error: bad value for funct2 field, value must be 0...3
test.s:1: Error: bad value for funct2 field, value must be 0...3
test.s:1: Error: bad value for funct2 field, value must be 0...3
test.s:1: Error: bad value for funct2 field, value must be 0...3
test.s:1: Error: bad value for funct2 field, value must be 0...3
test.s:1: Error: bad value for funct2 field, value must be 0...3
test.s:1: Error: bad value for funct2 field, value must be 0...3
test.s:1: Error: illegal operands `r MADD,0x0,0x4,a0,a1,a2,a3'

There are lots of redundant messages reported since we use as_bad to report the error for each opcode candidate. Therefore, replace as_bad with set_insn_error to avoid the redundant message and then only report the last useful one.

test.s: Assembler messages:
test.s:1: Error: illegal operand F2 (funct2) must be 0...3 `r MADD,0x0,0x4,a0,a1,a2,a3'

But there are one issue that we may need to resolve in the future,

.insn r  0x33, 0x0, 0x128, a0, a1, a2

The exactly invalid operand is Funct7 field for the above code, but now the assembler will report the invalid one is Funct2. The reason is that we always report the last error message for the last candidate. I have no idea that how to improve this now.

Nelson1225 commented 4 years ago

The match_func have many uses,

As mentioned above, we only need to report the error message for which are used to check the instruction constraints.

Nelson1225 commented 4 years ago

I prefer to close this PR for now. I should try to upstream the parts of this PR, and then implement the remaining parts to rvv-0.9.x or newer version. This feature seems not so urgent or necessary, so I close this until we need it again. Thanks.