Re: [Qemu-devel] [PATCH v8 04/23] RISC-V Disassembler

[Michael Clark]

On Sat, Apr 28, 2018 at 12:26 AM, Peter Maydell <address@hidden>
wrote:

> On 2 March 2018 at 13:51, Michael Clark <address@hidden> wrote:
> > The RISC-V disassembler has no dependencies outside of the 'disas'
> > directory so it can be applied independently. The majority of the
> > disassembler is machine-generated from instruction set metadata:
> >
> > - https://github.com/michaeljclark/riscv-meta
>
> > +        case 5:
> > +            if (isa == rv128) {
> > +                op = rv_op_c_sqsp;
> > +            } else {
> > +                op = rv_op_c_fsdsp; break;
> > +            }
> > +        case 6: op = rv_op_c_swsp; break;
>
> Coverity (CID1390575) points out that for the
> case 5 / isa == rv128 codepath, we set op, and
> then fall through into case 6 which overwrites it.
>
> Is there a missing "break" statement here? (If the
> fallthrough is deliberate it should be marked with a
> /* fallthrough */ comment.)
>

Thanks!

The disassembler is largely machine generated but the generator has an
issue with ISAs > 2 on a single set of encodings i.e. RVC. The generator
was originally coded for 1 or 2 encodings but I later added rv128 to the
opcodes metadata. I manually edited the generated code to add rv128
disassembly support for compressed instructions, and obviously made a
mistake in hand transcription (the reason I went to the effort of making
the generator). The compressed instructions overload the same encodings
with different meanings for the 3 ISAs, and this is where the generator
fell down, so it was only rv128 compressed instructions that I hand edited.
My own propensity to make mistakes is what attracts me to machine generated
code and of course lots of testing.

128-bit support would be interesting. Fabrice's RISCVEMU supports RV128
however it is an interpreter. If the large guest code is factored
correctly, then we could support RV128 on 64-bit hosts much like we can
support 64-bit targets on 32-bit hosts.

I've just spun and sent a patch for review. Given it's only broken on rv128
I'll just add it to the existing series.

Regards,
Michael.