Page MenuHomePhabricator

[RISCV] Fix disassembling of fence instruction with invalid field

Authored by apazos on Sep 7 2018, 5:24 PM.



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.

Diff Detail


Event Timeline

apazos created this revision.Sep 7 2018, 5:24 PM
asb added a reviewer: asb.Sep 13 2018, 7:18 AM

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.

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

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.

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.


apazos updated this revision to Diff 168390.Oct 4 2018, 3:13 PM
apazos retitled this revision from [RISCV] Fix decoding of fence instruction with invalid field to [RISCV] Fix disassembling of fence instruction with invalid field.
apazos edited the summary of this revision. (Show Details)

Printing unknown when disassembling instruction with 0 field.

asb added a comment.Oct 11 2018, 4:34 AM

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

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).

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

Good comments, will make the updates.

apazos updated this revision to Diff 169322.Oct 11 2018, 3:47 PM

Updated patch based on review comment.

This revision was not accepted when it landed; it landed in state Needs Review.Oct 11 2018, 3:52 PM
This revision was automatically updated to reflect the committed changes.