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

[Jan Bobek <jan.bobek@gmail.com>]

[-- Attachment #1.1: Type: text/plain, Size: 3368 bytes --]

On 8/10/19 7:35 PM, Richard Henderson wrote:
> 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.

Thank you, that's exactly the feedback I'm looking for! I've played
with the preprocessor in the past just to try out what's possible, but
I've never maintained code that uses it as extensively as this
series. It didn't occur to me that there would be a problem with
stepping it in gdb, for example, but now it seems obvious.

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

I'll have to look at it and see how exactly it's done; perhaps I'll
find something that's applicable to my case, too.

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

That makes complete sense, thank you! I'll keep the debugging issue in
mind.

> Further specific commentary to follow.

Looking forward to it!

-Jan


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]