Re: [PATCH 16/32] accel/tcg: Provide default implementation of disas_log

[Philippe Mathieu-Daudé]

On 5/4/24 12:24, Richard Henderson wrote:
> Almost all of the disas_log implementations are identical.
> Unify them within translator_loop.
>
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
>   accel/tcg/translator.c           |  9 ++++++++-
>   target/alpha/translate.c         |  9 ---------
>   target/arm/tcg/translate-a64.c   | 11 -----------
>   target/arm/tcg/translate.c       | 12 ------------
>   target/avr/translate.c           |  8 --------
>   target/cris/translate.c          | 11 -----------
>   target/hexagon/translate.c       |  9 ---------
>   target/hppa/translate.c          |  6 ++++--
>   target/i386/tcg/translate.c      | 11 -----------
>   target/loongarch/tcg/translate.c |  8 --------
>   target/m68k/translate.c          |  9 ---------
>   target/microblaze/translate.c    |  9 ---------
>   target/mips/tcg/translate.c      |  9 ---------
>   target/nios2/translate.c         |  9 ---------
>   target/openrisc/translate.c      | 11 -----------
>   target/ppc/translate.c           |  9 ---------
>   target/riscv/translate.c         | 18 ------------------
>   target/rx/translate.c            |  8 --------
>   target/sh4/translate.c           |  9 ---------
>   target/sparc/translate.c         |  9 ---------
>   target/tricore/translate.c       |  9 ---------
>   target/xtensa/translate.c        |  9 ---------
>   22 files changed, 12 insertions(+), 200 deletions(-)


> diff --git a/target/hppa/translate.c b/target/hppa/translate.c
> index 8a1a8bc3aa..7470795578 100644
> --- a/target/hppa/translate.c
> +++ b/target/hppa/translate.c
> @@ -4815,12 +4815,12 @@ static void hppa_tr_tb_stop(DisasContextBase *dcbase, 
> CPUState *cs)
>       }
>   }
>   
> +#ifdef CONFIG_USER_ONLY
>   static void hppa_tr_disas_log(const DisasContextBase *dcbase,
>                                 CPUState *cs, FILE *logfile)
>   {
>       target_ulong pc = dcbase->pc_first;
>   
> -#ifdef CONFIG_USER_ONLY
>       switch (pc) {
>       case 0x00:
>           fprintf(logfile, "IN:\n0x00000000:  (null)\n");
> @@ -4835,11 +4835,11 @@ static void hppa_tr_disas_log(const DisasContextBase 
> *dcbase,
>           fprintf(logfile, "IN:\n0x00000100:  syscall\n");
>           return;
>       }
> -#endif
>   
>       fprintf(logfile, "IN: %s\n", lookup_symbol(pc));
>       target_disas(logfile, cs, pc, dcbase->tb->size);
>   }
> +#endif
>   
>   static const TranslatorOps hppa_tr_ops = {
>       .init_disas_context = hppa_tr_init_disas_context,
> @@ -4847,7 +4847,9 @@ static const TranslatorOps hppa_tr_ops = {
>       .insn_start         = hppa_tr_insn_start,
>       .translate_insn     = hppa_tr_translate_insn,
>       .tb_stop            = hppa_tr_tb_stop,
> +#ifdef CONFIG_USER_ONLY
>       .disas_log          = hppa_tr_disas_log,

Preferrably rename with '_user'.

> +#endif
>   };


> diff --git a/target/riscv/translate.c b/target/riscv/translate.c
> index 9d57089fcc..9fd1ac1d60 100644
> --- a/target/riscv/translate.c
> +++ b/target/riscv/translate.c
> @@ -20,7 +20,6 @@
>   #include "qemu/log.h"
>   #include "cpu.h"
>   #include "tcg/tcg-op.h"
> -#include "disas/disas.h"
>   #include "exec/cpu_ldst.h"
>   #include "exec/exec-all.h"
>   #include "exec/helper-proto.h"
> @@ -1271,29 +1270,12 @@ static void riscv_tr_tb_stop(DisasContextBase *dcbase, 
> CPUState *cpu)
>       }
>   }
>   
> -static void riscv_tr_disas_log(const DisasContextBase *dcbase,
> -                               CPUState *cpu, FILE *logfile)
> -{
> -#ifndef CONFIG_USER_ONLY
> -    RISCVCPU *rvcpu = RISCV_CPU(cpu);
> -    CPURISCVState *env = &rvcpu->env;
> -#endif
> -
> -    fprintf(logfile, "IN: %s\n", lookup_symbol(dcbase->pc_first));
> -#ifndef CONFIG_USER_ONLY
> -    fprintf(logfile, "Priv: "TARGET_FMT_ld"; Virt: %d\n",
> -            env->priv, env->virt_enabled);

Should we mention the removal of this fprintf() in sysemu?

> -#endif
> -    target_disas(logfile, cpu, dcbase->pc_first, dcbase->tb->size);
> -}
> -
>   static const TranslatorOps riscv_tr_ops = {
>       .init_disas_context = riscv_tr_init_disas_context,
>       .tb_start           = riscv_tr_tb_start,
>       .insn_start         = riscv_tr_insn_start,
>       .translate_insn     = riscv_tr_translate_insn,
>       .tb_stop            = riscv_tr_tb_stop,
> -    .disas_log          = riscv_tr_disas_log,
>   };

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>