[RISCV] Fix disassembling of fence instruction with invalid field

[apazos]

Summary

Instruction with 0 in fence field being disassembled as fence , iorw.
Printing "unknown" to match GAS behavior.

This bug was uncovered by a LLVM MC Disassembler Protocol Buffer Fuzzer
for the RISC-V assembly language.

[asb]

Looking at the current spec draft, it's not clear that a zero fence argument is actually invalid (though it may be non-sensical and impossible to generate from the assembler). I note that gas will emit "unknown" for the operand when disassembling. I'm not fully sure what the best solution is here, so first wanted to check if there was part of the spec I might have missed that discussed all-zero predecessor/successor fence arguments.

[apazos]

The description of the instruction in RISC-V User-Level ISA V2.2 page 20 does not help much.

Right now we disassemble it with no fence argument string:

echo "0x0f 0x00 0xf0 0x00" | lvm-mc -disassemble -triple=riscv32

.text
fence   , iorw

And if you take that string and re-assemble it, "fence , iorw" will fail to assemble.

When fence argument is 0 GAS disassembles as unknown:
0: 00f0000f fence unknown, iorw

But GAS will not reassemble this string:
{standard input}:1: Error: illegal operands `fence unknown,iorw'

That is why I treated it as invalid.

[kito-cheng]

Hi Ana:

Does your point is fence , iorw is accepted by GAS? If so, I've fixed that and empty rounding mode problem within same patch.

[1] https://github.com/bminor/binutils-gdb/commit/bfb218e3e404a6168888df51c03827eacde9ceea

[apazos]

Printing unknown when disassembling instruction with 0 field.

[asb]

Hi Ana, just some minor comments. With tweaks along the suggested lines, this looks good to me.

lib/Target/RISCV/InstPrinter/RISCVInstPrinter.cpp

104–105 ↗

(On Diff #168390)

How about if ((FenceArg & 0xf) == 0)?

I think it might be worth making this a little more resilient. FenceArg shouldn't be more than 4 bits, but we might either assert that FenceArg >> 4 is 0, or perhaps change the condition for "unknown" to if ((FenceArg & 0xf) == 0).

test/MC/Disassembler/RISCV/unknown-fence-field.txt

1–2 ↗

(On Diff #168390)

No need for -mattr=+f,+d

8–9 ↗

(On Diff #168390)

It would be good to have a fence unknown, unknown test as well

[apazos]

Good comments, will make the updates.

[apazos]

Updated patch based on review comment.

llvm/trunk/lib/Target/RISCV/InstPrinter/RISCVInstPrinter.cpp

void RISCVInstPrinter::printCSRSystemRegister(const MCInst *MI, unsigned OpNo,
   else
     O << Imm;
 }
 
 void RISCVInstPrinter::printFenceArg(const MCInst *MI, unsigned OpNo,
                                      const MCSubtargetInfo &STI,
                                      raw_ostream &O) {
   unsigned FenceArg = MI->getOperand(OpNo).getImm();
+  assert (((FenceArg >> 4) == 0) && "Invalid immediate in printFenceArg");
+
   if ((FenceArg & RISCVFenceField::I) != 0)
     O << 'i';
   if ((FenceArg & RISCVFenceField::O) != 0)
     O << 'o';
   if ((FenceArg & RISCVFenceField::R) != 0)
     O << 'r';
   if ((FenceArg & RISCVFenceField::W) != 0)
     O << 'w';
+  if (FenceArg == 0)
+    O << "unknown";
 }
 
 void RISCVInstPrinter::printFRMArg(const MCInst *MI, unsigned OpNo,
                                    const MCSubtargetInfo &STI, raw_ostream &O) {
   auto FRMArg =
       static_cast<RISCVFPRndMode::RoundingMode>(MI->getOperand(OpNo).getImm());
   O << RISCVFPRndMode::roundingModeToString(FRMArg);
 }

llvm/trunk/test/MC/Disassembler/RISCV/unknown-fence-field.txt

+# RUN: llvm-mc -disassemble -triple=riscv32 < %s 2>&1 | FileCheck %s
+# RUN: llvm-mc -disassemble -triple=riscv64 < %s 2>&1 | FileCheck %s
+#
+# Test generated by a LLVM MC Disassembler Protocol Buffer Fuzzer
+# for the RISC-V assembly language.
+
+# This decodes as fence , iorw with invalid fence field as 0.
+[0x0f 0x00 0xf0 0x00]
+# CHECK: fence unknown, iorw

llvm/trunk/test/MC/RISCV/rv32i-invalid.s

 # RUN: not llvm-mc -triple riscv32 < %s 2>&1 | FileCheck %s
 
 # Out of range immediates
 ## fencearg
 fence iorw, iore # CHECK: :[[@LINE]]:13: error: operand must be formed of letters selected in-order from 'iorw'
 fence wr, wr # CHECK: :[[@LINE]]:7: error: operand must be formed of letters selected in-order from 'iorw'
 fence rw, rr # CHECK: :[[@LINE]]:11: error: operand must be formed of letters selected in-order from 'iorw'
 fence 1, rw # CHECK: :[[@LINE]]:7: error: operand must be formed of letters selected in-order from 'iorw'
+fence unknown, unknown # CHECK: :[[@LINE]]:7: error: operand must be formed of letters selected in-order from 'iorw'
 
 ## uimm5
 slli a0, a0, 32 # CHECK: :[[@LINE]]:14: error: immediate must be an integer in the range [0, 31]
 srli a0, a0, -1 # CHECK: :[[@LINE]]:14: error: immediate must be an integer in the range [0, 31]
 srai a0, a0, -19 # CHECK: :[[@LINE]]:14: error: immediate must be an integer in the range [0, 31]
 csrrwi a1, 0x1, -1 # CHECK: :[[@LINE]]:17: error: immediate must be an integer in the range [0, 31]
 csrrsi t1, 999, 32 # CHECK: :[[@LINE]]:17: error: immediate must be an integer in the range [0, 31]
 csrrci x0, 43, -90 # CHECK: :[[@LINE]]:16: error: immediate must be an integer in the range [0, 31]