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

[Richard Henderson]

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.

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


r~