Re: [Qemu-devel] [PATCH 4/4] target-mips: Fix MIPS_DEBUG.

[Peter Maydell]

On 31 December 2011 04:54, Richard Henderson <address@hidden> wrote:
> The macro uses the DisasContext.  Pass it around as needed.
>
> Signed-off-by: Richard Henderson <address@hidden>
> ---
>  target-mips/translate.c |   80 
> ++++++++++++++++++++++++++---------------------
>  1 files changed, 44 insertions(+), 36 deletions(-)
>
> diff --git a/target-mips/translate.c b/target-mips/translate.c
> index 8908c8c..11272b6 100644
> --- a/target-mips/translate.c
> +++ b/target-mips/translate.c
> @@ -1489,7 +1489,8 @@ static void gen_arith_imm (CPUState *env, DisasContext 
> *ctx, uint32_t opc,
>  }
>
>  /* Logic with immediate operand */
> -static void gen_logic_imm (CPUState *env, uint32_t opc, int rt, int rs, 
> int16_t imm)
> +static void gen_logic_imm (CPUState *env, DisasContext *ctx, uint32_t opc,
> +                           int rt, int rs, int16_t imm)
>  {
>     target_ulong uimm;
>     const char *opn = "imm logic";

Am I missing something, or does gen_logic_imm() not actually use env at all?
Maybe we should be replacing env with ctx in the parameter list in some/most
of these functions rather than adding it?
(This suggestion is mostly because I kind of like the approach to translate.c
that avoids passingn CPUState wherever possible, to reduce the kind of subtle
bug where you use something in CPUState that isn't actually valid.)

> @@ -9919,19 +9922,24 @@ static void gen_ldst_multiple (DisasContext *ctx, 
> uint32_t opc, int reglist,
>     switch (opc) {
>     case LWM32:
>         gen_helper_lwm(t0, t1, t2);
> +        opn = "lwm";
>         break;
>     case SWM32:
>         gen_helper_swm(t0, t1, t2);
> +        opn = "swm";
>         break;
>  #ifdef TARGET_MIPS64
>     case LDM:
>         gen_helper_ldm(t0, t1, t2);
> +        opn = "ldm";
>         break;
>     case SDM:
>         gen_helper_sdm(t0, t1, t2);
> +        opn = "sdm";
>         break;
>  #endif
>     }
> +    (void)opn;
>     MIPS_DEBUG("%s, %x, %d(%s)", opn, reglist, offset, regnames[base]);
>     tcg_temp_free(t0);
>     tcg_temp_free(t1);

Maybe this bit should be split out from the bulk of the "just add ctx
to function signatures and calls" mechanical change?

-- PMM