Re: [PATCH 1/1] disas/riscv: Guard dec->cfg dereference for host disassemble

[LIU Zhiwei]

On 2024/12/6 11:36, Richard Henderson wrote:

> On
>       12/5/24 21:24, LIU Zhiwei wrote:
>       
> > For riscv host, it will set dec->cfg to
> >         zero. Thus we shuld guard
> >         
> >         the dec->cfg deference for riscv host disassemble.
> >         
> >         
> >         And in general, we should only use dec->cfg for target in
> >         three cases:
> >         
> >         
> >         1) For not incompatible encodings, such as zcmp/zcmt/zfinx.
> >         
> >         2) For maybe-ops encodings, they are better to be disassembled
> >         to
> >         
> >             the "real" extensions, such as zicfiss. The guard of
> >         dec->zimop
> >         
> >             and dec->zcmop is for comment and avoid check for every
> >         extension
> >         
> >             that encoded in maybe-ops area.
> >         
> >         3) For custom encodings, we have to use dec->cfg to
> >         disassemble
> >         
> >             custom encodings using the same encoding area.
> >         
> >         
> >         Signed-off-by: LIU Zhiwei <zhiwei_liu@linux.alibaba.com>
> >         
> >         Suggested-by: Richard Henderson
> >         <richard.henderson@linaro.org>
> >         
> >       
>       
>       ...
>       
>       
> > @@ -5112,28 +5112,28 @@ static GString
> >         *format_inst(size_t tab, rv_decode *dec)
> >         
> >                       g_string_append(buf,
> >         rv_ireg_name_sym[dec->rs2]);
> >         
> >                       break;
> >         
> >                   case '3':
> >         
> >         -            if (dec->cfg->ext_zfinx) {
> >         
> >         +            if (dec->cfg &&
> >         dec->cfg->ext_zfinx) {
> >         
> >                           g_string_append(buf,
> >         rv_ireg_name_sym[dec->rd]);
> >         
> >                       } else {
> >         
> >                           g_string_append(buf,
> >         rv_freg_name_sym[dec->rd]);
> >         
> >                       }
> >         
> >                       break;
> >         
> >                   case '4':
> >         
> >         -            if (dec->cfg->ext_zfinx) {
> >         
> >         +            if (dec->cfg &&
> >         dec->cfg->ext_zfinx) {
> >         
> >                           g_string_append(buf,
> >         rv_ireg_name_sym[dec->rs1]);
> >         
> >                       } else {
> >         
> >                           g_string_append(buf,
> >         rv_freg_name_sym[dec->rs1]);
> >         
> >                       }
> >         
> >                       break;
> >         
> >                   case '5':
> >         
> >         -            if (dec->cfg->ext_zfinx) {
> >         
> >         +            if (dec->cfg &&
> >         dec->cfg->ext_zfinx) {
> >         
> >                           g_string_append(buf,
> >         rv_ireg_name_sym[dec->rs2]);
> >         
> >                       } else {
> >         
> >                           g_string_append(buf,
> >         rv_freg_name_sym[dec->rs2]);
> >         
> >                       }
> >         
> >                       break;
> >         
> >                   case '6':
> >         
> >         -            if (dec->cfg->ext_zfinx) {
> >         
> >         +            if (dec->cfg &&
> >         dec->cfg->ext_zfinx) {
> >         
> >                           g_string_append(buf,
> >         rv_ireg_name_sym[dec->rs3]);
> >         
> >                       } else {
> >         
> >                           g_string_append(buf,
> >         rv_freg_name_sym[dec->rs3]);
> >         
> >       
>       
>       These are the only tests of cfg that are required.
>       
>     
> None of
>       the other standard isa extensions overlap.
>       
>     
Both zcmt and zcmp are not compatible with Zcd, as they reuse some encodings from c.fsdsp.

Zimop or Zcmop also overlap with other isa extensions, as they are maybe-ops. Other extensions
such as zicfiss will reuse their encodings.

I think we had better disassemble them to zicifss if it has been implemented on the target cpu. Otherwise
we disassemble them to maybe-ops.

>       
> > @@ -5439,7 +5439,8 @@ static GString
> >         *disasm_inst(rv_isa isa, uint64_t pc, rv_inst inst,
> >         
> >                   const rv_opcode_data *opcode_data =
> >         decoders[i].opcode_data;
> >         
> >                   void (*decode_func)(rv_decode *, rv_isa) =
> >         decoders[i].decode_func;
> >         
> >           -        if (guard_func(cfg)) {
> >         
> >         +        /* always_true_p don't dereference cfg */
> >         
> >         +        if (((i == 0) || cfg) && guard_func(cfg)) {
> >         
> >       
>       
>       This should be i == 0 || (cfg && guard_func(cfg)).
>       
>

OK. Although I think they are both right.

Thanks,
Zhiwei

>       
>       
>       r~
>       
>