matt-kempster/m2c

Incorrect reordering of function calls & memory reads

zbanks opened this issue · 0 comments

From Discord:

I think mips2c is generating incorrect code. For func_80133038 in MM, it gives

extern ? D_801C5C9C;

s32 func_80133038(GlobalContext *globalCtx, s8 *arg1, struct_80133038_arg2 *arg2) {
    u8 temp_s0;
    u8 temp_v0;

    do {
        temp_v0 = *arg1;
        temp_s0 = *(&D_801C5C9C + temp_v0);
        arg1 = &arg1[temp_s0];
    } while ((&D_801C5C50)[temp_v0](globalCtx, &arg1, arg2) == 0);
    return arg2->unkC;
}

but what ended up matching was

s32 func_80133038(GlobalContext* globalCtx, u8* arg1, struct_80133038_arg2* arg2) {
    u8 size;
    s32 ret;

    do {
        size = D_801C5C9C[*arg1];
        ret = (*D_801C5C50[*arg1])(globalCtx, &arg1, arg2);
        arg1 += size;
    } while (ret == 0);

    return arg2->unkC;
}

mips2c is changing arg1 before the function call when what's actually happening is it changes after the function call.

This was narrowed down to needing a prevent_later_function_calls() on reads.
However, it's tricky to find the right balance of when to add this. Fixing func_80133038 usually makes typical output worse -- emitting incorrect code seems to be rare.
I think theoretically prevent_later_function_calls() would need to be added for all reads (which definitely makes the output worse).