Re: [RESEND PATCH v9 16/28] target/loongarch: Add disassembler

[Richard Henderson]

On 11/8/21 4:08 AM, Song Gao wrote:
> +/* operand extractors */
> +#define IM_12 12
> +#define IM_14 14
> +#define IM_15 15
> +#define IM_16 16
> +#define IM_20 20
> +#define IM_21 21
> +#define IM_26 26
> +
> +static uint32_t operand_r1(uint32_t insn)
> +{
> +    return insn & 0x1f;
> +}
> +
> +static uint32_t operand_r2(uint32_t insn)
> +{
> +    return (insn >> 5) & 0x1f;
> +}
> +
> +static uint32_t operand_r3(uint32_t insn)
> +{
> +    return (insn >> 10) & 0x1f;
> +}
> +
> +static uint32_t operand_r4(uint32_t insn)
> +{
> +    return (insn >> 15) & 0x1f;
> +}
> +
> +static uint32_t operand_u6(uint32_t insn)
> +{
> +    return (insn >> 10) & 0x3f;
> +}
> +
> +static uint32_t operand_bw1(uint32_t insn)
> +{
> +    return (insn >> 10) & 0x1f;
> +}
> +
> +static uint32_t operand_bw2(uint32_t insn)
> +{
> +    return (insn >> 16) & 0x1f;
> +}
> +
> +static uint32_t operand_bd1(uint32_t insn)
> +{
> +    return (insn >> 10) & 0x3f;
> +}
> +
> +static uint32_t operand_bd2(uint32_t insn)
> +{
> +    return (insn >> 16) & 0x3f;
> +}
> +
> +static uint32_t operand_sa(uint32_t insn)
> +{
> +    return (insn >> 15) & 0x3;
> +}
> +
> +static int32_t operand_im20(uint32_t insn)
> +{
> +    int32_t imm = (int32_t)((insn >> 5) & 0xfffff);
> +    return imm > (1 << 19) ? imm - (1 << 20) : imm;
> +}
> +
> +static int32_t operand_im16(uint32_t insn)
> +{
> +    int32_t imm = (int32_t)((insn >> 10) & 0xffff);
> +    return imm > (1 << 15) ? imm - (1 << 16) : imm;
> +}
> +
> +static int32_t operand_im14(uint32_t insn)
> +{
> +    int32_t imm = (int32_t)((insn >> 10) & 0x3fff);
> +    return imm > (1 << 13) ? imm - (1 << 14) : imm;
> +}
> +
> +static int32_t operand_im12(uint32_t insn)
> +{
> +    int32_t imm = (int32_t)((insn >> 10) & 0xfff);
> +    return imm > (1 << 11) ? imm - (1 << 12) : imm;
> +}
> +
> +static uint32_t operand_cd(uint32_t insn)
> +{
> +    return insn & 0x7;
> +}
> +
> +static uint32_t operand_cj(uint32_t insn)
> +{
> +    return (insn >> 5) & 0x7;
> +}
> +
> +static uint32_t operand_code(uint32_t insn)
> +{
> +    return insn & 0x7fff;
> +}
> +
> +static int32_t operand_whint(uint32_t insn)
> +{
> +    int32_t imm = (int32_t)(insn & 0x7fff);
> +    return imm > (1 << 14) ? imm - (1 << 15) : imm;
> +}
> +
> +static int32_t operand_ofs21(uint32_t insn)
> +{
> +    int32_t imm = (((int32_t)insn & 0x1f) << 16) |
> +        ((insn >> 10) & 0xffff);
> +    return imm > (1 << 20) ? imm - (1 << 21) : imm;
> +}
> +
> +static int32_t operand_ofs26(uint32_t insn)
> +{
> +    int32_t imm = (((int32_t)insn & 0x3ff) << 16) |
> +        ((insn >> 10) & 0xffff);
> +    return imm > (1 << 25) ? imm - (1 << 26) : imm;
> +}
> +
> +static uint32_t operand_fcond(uint32_t insn)
> +{
> +    return (insn >> 15) & 0x1f;
> +}
> +
> +static uint32_t operand_sel(uint32_t insn)
> +{
> +    return (insn >> 15) & 0x7;
> +}
> +
> +/* decode operands */
> +static void decode_insn_operands(la_decode *dec)
> +{
> +    uint32_t insn = dec->insn;
> +    switch (dec->codec) {
> +    case la_codec_2r:
> +        dec->r1 = operand_r1(insn);
> +        dec->r2 = operand_r2(insn);
> +        break;
> +    case la_codec_2r_u5:
> +        dec->r1 = operand_r1(insn);
> +        dec->r2 = operand_r2(insn);
> +        dec->r3 = operand_r3(insn);
> +        break;
> +    case la_codec_2r_u6:
> +        dec->r1 = operand_r1(insn);
> +        dec->r2 = operand_r2(insn);
> +        dec->r3 = operand_u6(insn);
> +        break;
> +    case la_codec_2r_2bw:
> +        dec->r1 = operand_r1(insn);
> +        dec->r2 = operand_r2(insn);
> +        dec->r3 = operand_bw1(insn);
> +        dec->r4 = operand_bw2(insn);
> +        break;
> +    case la_codec_2r_2bd:
> +        dec->r1 = operand_r1(insn);
> +        dec->r2 = operand_r2(insn);
> +        dec->r3 = operand_bd1(insn);
> +        dec->r4 = operand_bd2(insn);
> +        break;
> +    case la_codec_3r:
> +        dec->r1 = operand_r1(insn);
> +        dec->r2 = operand_r2(insn);
> +        dec->r3 = operand_r3(insn);
> +        break;
> +    case la_codec_3r_rd0:
> +        dec->r1 = 0;
> +        dec->r2 = operand_r2(insn);
> +        dec->r3 = operand_r3(insn);
> +        break;
> +    case la_codec_3r_sa:
> +        dec->r1 = operand_r1(insn);
> +        dec->r2 = operand_r2(insn);
> +        dec->r3 = operand_r3(insn);
> +        dec->r4 = operand_sa(insn);
> +        break;
> +    case la_codec_4r:
> +        dec->r1 = operand_r1(insn);
> +        dec->r2 = operand_r2(insn);
> +        dec->r3 = operand_r3(insn);
> +        dec->r4 = operand_r4(insn);
> +        break;
> +    case la_codec_r_im20:
> +        dec->r1 = operand_r1(insn);
> +        dec->imm = operand_im20(insn);
> +        dec->bit = IM_20;
> +        break;
> +    case la_codec_2r_im16:
> +        dec->r1 = operand_r1(insn);
> +        dec->r2 = operand_r2(insn);
> +        dec->imm = operand_im16(insn);
> +        dec->bit = IM_16;
> +        break;
> +    case la_codec_2r_im14:
> +        dec->r1 = operand_r1(insn);
> +        dec->r2 = operand_r2(insn);
> +        dec->imm = operand_im14(insn);
> +        dec->bit = IM_14;
> +        break;
> +    case la_codec_2r_im12:
> +        dec->r1 = operand_r1(insn);
> +        dec->r2 = operand_r2(insn);
> +        dec->imm = operand_im12(insn);
> +        dec->bit = IM_12;
> +        break;
> +    case la_codec_r_cd:
> +        dec->r1 = operand_cd(insn);
> +        dec->r2 = operand_r2(insn);
> +        break;
> +    case la_codec_r_cj:
> +        dec->r1 = operand_r1(insn);
> +        dec->r2 = operand_cj(insn);
> +        break;
> +    case la_codec_code:
> +        dec->code = operand_code(insn);
> +        break;
> +    case la_codec_whint:
> +        dec->imm = operand_whint(insn);
> +        dec->bit = IM_15;
> +        break;
> +    case la_codec_r_ofs21:
> +        dec->imm = operand_ofs21(insn);
> +        dec->bit = IM_21;
> +        dec->r2 = operand_r2(insn);
> +        break;
> +    case la_codec_cj_ofs21:
> +        dec->imm = operand_ofs21(insn);
> +        dec->bit = IM_21;
> +        dec->r2 = operand_cj(insn);
> +        break;
> +    case la_codec_ofs26:
> +        dec->imm = operand_ofs26(insn);
> +        dec->bit = IM_26;
> +        break;
> +    case la_codec_cond:
> +        dec->r1 = operand_cd(insn);
> +        dec->r2 = operand_r2(insn);
> +        dec->r3 = operand_r3(insn);
> +        dec->r4 = operand_fcond(insn);
> +        break;
> +    case la_codec_sel:
> +        dec->r1 = operand_r1(insn);
> +        dec->r2 = operand_r2(insn);
> +        dec->r3 = operand_r3(insn);
> +        dec->r4 = operand_sel(insn);
> +        break;
> +    }
> +}

All of this operand extraction has already been done by decodetree.  We do not need to 
re-do it.

> +/* format instruction */
> +static void append(char *s1, const char *s2, size_t n)
> +{
> +    size_t l1 = strlen(s1);
> +    if (n - l1 - 1 > 0) {
> +        strncat(s1, s2, n - l1);
> +    }
> +}
> +
> +static void format_insn(char *buf, size_t buflen,  const char* name,
> +                        const char *fmt, la_decode *dec)
> +{
> +    char tmp[16];

Better to use GString, with no pre-set buflen in the caller.  Or simply output to 
fprintf_func directly; there's no real need to cache it here.

I suggest a better organization is

typedef struct {
    disassemble_info *info;
    uint32_t insn;
} DisasContext;

#define output(C, INSN, FMT, ...) \
    (C)->info->fprintf_func((C)->info->stream, "%08x  %-9s " FMT, \
                            (C)->insn, INSN, ##__VA_ARGS__)

static void output_rdrjrk(DisasContext *ctx,
                          const arg_fmt_rdrjrk *a,
                          const char *mnemonic)
{
    output(ctx, mnemonic, "%s,%s,%s",
           regnames[a->rd], regnames[a->rj], regnames[a->rk]);
}

#define INSN(insn, type) \
    static void trans_##insn(DisasContext *ctx, arg_fmt_##type *a)
    { output_##type(ctx, a, #insn); }

INSN(add_w, rdrjrk)

etc.

This way you have type checking between insns.decode and the list of INSN macros.  The 
complete output for the instruction is done via a single printf.

> +int print_insn_loongarch(bfd_vma memaddr, struct disassemble_info *info)
> +{
> +    bfd_byte buffer[INSNLEN];
> +    unsigned long insn;

uint32_t insn.

> +    int status;
> +
> +    status = (*info->read_memory_func)(memaddr, buffer, INSNLEN, info);
> +    if (status == 0) {
> +        insn = (uint32_t) bfd_getl32(buffer);
> +        (*info->fprintf_func)(info->stream, "%08" PRIx64 " ", insn);

This errors out, because of mismatch between "unsigned long" and "uint64_t" on 32-bit 
hosts.  But better to fold into the single output, per above.

> +    } else {
> +        (*info->memory_error_func)(status, memaddr, info);
> +        return -1;
> +    }
> +
> +    dec.pc = memaddr;
> +    dec.insn = insn;
> +    if (!decode(info, insn)) {
> +        (*info->fprintf_func)(info->stream, "illegal");

Using the above scheme, this becomes

    output(&ctx, illegal, "")


r~