Re: [Qemu-devel] [RFC PATCH v2 00/39] rewrite MMX/SSE instruction translation

[Richard Henderson]

On 8/9/19 9:12 PM, Jan Bobek wrote:
> This is a v2 of the patch series posted in [1]. Patches 1-9 are just
> cleanups; patches 10-39 are something actually interesting. Compared
> to v1, I started using preprocessor more extensively to generate
> repetitive boilerplate code; opinions/alternatives are welcome and
> appreciated.

This is tricky.  I'm not keen on code entirely expanded via macros because it
becomes extremely difficult to debug.  All statements get recorded at the same
line of the location of the expansion, which makes the gdb "step" command
finish the entire function because there is no next line.

Once upon a time I wrote some code that's extremely macro crazy:

https://sourceware.org/git/?p=glibc.git;a=blob_plain;f=soft-fp/op-common.h;hb=HEAD

It has been extremely difficult to maintain over the years.

We have just recently gotten rid of some of the macros in the softmmu code

https://patchwork.ozlabs.org/project/qemu-devel/list/?series=105441

replacing most of them with inline functions.

A lot of what you have needs very little adjustment to address the debugging
problem.  E.g.

> +#define INSNOP_INIT(opT, init_stmt)                                \
> +    static int insnop_init(opT)(CPUX86State *env, DisasContext *s, \
> +                                int modrm, insnop_t(opT) *op)      \
> +    {                                                              \
> +        init_stmt;                                                 \
> +    }
....
> +INSNOP(
> +    M, TCGv,
> +    do {
> +        if (decode_modrm_mod(env, s, modrm) == 3) {
> +            INSNOP_INIT_FAIL;
> +        } else {
> +            INSNOP_INIT_OK(s->A0);
> +        }
> +    } while (0),
> +    do {
> +        assert(*op == s->A0);
> +        gen_lea_modrm(env, s, modrm);
> +    } while (0),
> +    INSNOP_FINALIZE_NOOP)

Rearrange this as

#define INSNOP_INIT(OPT) \
    static bool insnop_##OPT##_init(CPUX86State *env, DisasContext *s, \
                                    int modrm, insnop_##OPT##_t *op)

#define INSNOP_PREPARE(OPT) \
    static void insnop_##OPT##_prepare(CPUX86State *env, DisasContext *s, \
                                       int modrm, insnop_##OPT##_t *op)

INSNOP_INIT(M)
{
    if (decode_modrm_mod(env, s, modrm) == 3) {
        INSNOP_INIT_FAIL;
    } else {
        INSNOP_INIT_OK(s->A0);
    }
}

INSNOP_PREPARE(M)
{
    assert(*op == s->A0);
    gen_lea_modrm(env, s, modrm);
}

etc and suddenly the entire expansion does not occur on a single line.

Further specific commentary to follow.


r~