Re: [Qemu-devel] [PATCH 1/7] disas: Create factored out fn for monitor and target disas

[Claudio Fontana]

On 05.05.2015 06:44, Peter Crosthwaite wrote:
> The monitor_ and target_ disas function do mostly the same thing.
> One dissambles target instructions on behalf of the log, the other
> for the monitor command "xp/i" and friends.
> 
> There is a #if defined TARGET_FOO switch duplicated between both
> functions and arch-specific setup for disas is copied between the two.
> Factor out this duped code for arches that are consistent between
> monitor and target disas, that is I386, SPARC, M68K, MIPS, SH4, S390X,
> MOXIE and LM32.
> 
> No functional change.
> 
> Signed-off-by: Peter Crosthwaite <address@hidden>
> ---
>  disas.c | 148 
> ++++++++++++++++++++++++++--------------------------------------
>  1 file changed, 61 insertions(+), 87 deletions(-)
> 
> diff --git a/disas.c b/disas.c
> index 44a019a..1c80567 100644
> --- a/disas.c
> +++ b/disas.c
> @@ -187,6 +187,55 @@ static int print_insn_od_target(bfd_vma pc, 
> disassemble_info *info)
>      return print_insn_objdump(pc, info, "OBJD-T");
>  }
>  
> +static inline void
> +target_disas_set_info(int (**print_insn)(bfd_vma pc, disassemble_info *info),
> +                      CPUDebug *s, CPUArchState *env, int flags)
> +{
> +    s->env = env;
> +    s->info.print_address_func = generic_print_target_address;
> +
> +#ifdef TARGET_WORDS_BIGENDIAN
> +    s->info.endian = BFD_ENDIAN_BIG;
> +#else
> +    s->info.endian = BFD_ENDIAN_LITTLE;
> +#endif
> +#if defined(TARGET_I386)
> +    if (flags == 2) {
> +        s->info.mach = bfd_mach_x86_64;
> +    } else if (flags == 1) {
> +        s->info.mach = bfd_mach_i386_i8086;
> +    } else {
> +        s->info.mach = bfd_mach_i386_i386;
> +    }
> +    *print_insn = print_insn_i386;
> +#elif defined(TARGET_SPARC)
> +    *print_insn = print_insn_sparc;
> +#ifdef TARGET_SPARC64
> +    s->info.mach = bfd_mach_sparc_v9b;
> +#endif
> +#elif defined(TARGET_M68K)
> +    *print_insn = print_insn_m68k;
> +#elif defined(TARGET_MIPS)
> +#ifdef TARGET_WORDS_BIGENDIAN
> +    *print_insn = print_insn_big_mips;
> +#else
> +    *print_insn = print_insn_little_mips;
> +#endif
> +#elif defined(TARGET_SH4)
> +    s->info.mach = bfd_mach_sh4;
> +    *print_insn = print_insn_sh;
> +#elif defined(TARGET_S390X)
> +    s->info.mach = bfd_mach_s390_64;
> +    *print_insn = print_insn_s390;
> +#elif defined(TARGET_MOXIE)
> +    s->info.mach = bfd_arch_moxie;
> +    *print_insn = print_insn_moxie;
> +#elif defined(TARGET_LM32)
> +    s->info.mach = bfd_mach_lm32;
> +    *print_insn = print_insn_lm32;
> +#endif
> +}
> +
>  /* Disassemble this for me please... (debugging). 'flags' has the following
>     values:
>      i386 - 1 means 16 bit code, 2 means 64 bit code
> @@ -205,27 +254,13 @@ void target_disas(FILE *out, CPUArchState *env, 
> target_ulong code,
>  
>      INIT_DISASSEMBLE_INFO(s.info, out, fprintf);
>  
> -    s.env = env;
> +    target_disas_set_info(&print_insn, &s, env, flags);
> +
>      s.info.read_memory_func = target_read_memory;
>      s.info.buffer_vma = code;
>      s.info.buffer_length = size;
> -    s.info.print_address_func = generic_print_target_address;
>  
> -#ifdef TARGET_WORDS_BIGENDIAN
> -    s.info.endian = BFD_ENDIAN_BIG;
> -#else
> -    s.info.endian = BFD_ENDIAN_LITTLE;
> -#endif
> -#if defined(TARGET_I386)
> -    if (flags == 2) {
> -        s.info.mach = bfd_mach_x86_64;
> -    } else if (flags == 1) {
> -        s.info.mach = bfd_mach_i386_i8086;
> -    } else {
> -        s.info.mach = bfd_mach_i386_i386;
> -    }
> -    print_insn = print_insn_i386;
> -#elif defined(TARGET_ARM)
> +#if defined(TARGET_ARM)
>      if (flags & 4) {
>          /* We might not be compiled with the A64 disassembler
>           * because it needs a C++ compiler; in that case we will
> @@ -246,11 +281,6 @@ void target_disas(FILE *out, CPUArchState *env, 
> target_ulong code,
>          s.info.endian = BFD_ENDIAN_BIG;
>  #endif
>      }
> -#elif defined(TARGET_SPARC)
> -    print_insn = print_insn_sparc;
> -#ifdef TARGET_SPARC64
> -    s.info.mach = bfd_mach_sparc_v9b;
> -#endif
>  #elif defined(TARGET_PPC)
>      if ((flags >> 16) & 1) {
>          s.info.endian = BFD_ENDIAN_LITTLE;
> @@ -267,17 +297,6 @@ void target_disas(FILE *out, CPUArchState *env, 
> target_ulong code,
>      }
>      s.info.disassembler_options = (char *)"any";
>      print_insn = print_insn_ppc;
> -#elif defined(TARGET_M68K)
> -    print_insn = print_insn_m68k;
> -#elif defined(TARGET_MIPS)
> -#ifdef TARGET_WORDS_BIGENDIAN
> -    print_insn = print_insn_big_mips;
> -#else
> -    print_insn = print_insn_little_mips;
> -#endif
> -#elif defined(TARGET_SH4)
> -    s.info.mach = bfd_mach_sh4;
> -    print_insn = print_insn_sh;
>  #elif defined(TARGET_ALPHA)
>      s.info.mach = bfd_mach_alpha_ev6;
>      print_insn = print_insn_alpha;
> @@ -289,18 +308,9 @@ void target_disas(FILE *out, CPUArchState *env, 
> target_ulong code,
>          s.info.mach = bfd_mach_cris_v32;
>          print_insn = print_insn_crisv32;
>      }
> -#elif defined(TARGET_S390X)
> -    s.info.mach = bfd_mach_s390_64;
> -    print_insn = print_insn_s390;
>  #elif defined(TARGET_MICROBLAZE)
>      s.info.mach = bfd_arch_microblaze;
>      print_insn = print_insn_microblaze;
> -#elif defined(TARGET_MOXIE)
> -    s.info.mach = bfd_arch_moxie;
> -    print_insn = print_insn_moxie;
> -#elif defined(TARGET_LM32)
> -    s.info.mach = bfd_mach_lm32;
> -    print_insn = print_insn_lm32;
>  #endif
>      if (print_insn == NULL) {
>          print_insn = print_insn_od_target;
> @@ -452,10 +462,12 @@ void monitor_disas(Monitor *mon, CPUArchState *env,
>  {
>      int count, i;
>      CPUDebug s;
> -    int (*print_insn)(bfd_vma pc, disassemble_info *info);
> +    int (*print_insn)(bfd_vma pc, disassemble_info *info) = NULL;
>  
>      INIT_DISASSEMBLE_INFO(s.info, (FILE *)mon, monitor_fprintf);
>  
> +    target_disas_set_info(&print_insn, &s, env, flags);
> +
>      s.env = env;
>      monitor_disas_is_physical = is_physical;
>      s.info.read_memory_func = monitor_read_memory;
> @@ -463,29 +475,10 @@ void monitor_disas(Monitor *mon, CPUArchState *env,
>  
>      s.info.buffer_vma = pc;
>  
> -#ifdef TARGET_WORDS_BIGENDIAN
> -    s.info.endian = BFD_ENDIAN_BIG;
> -#else
> -    s.info.endian = BFD_ENDIAN_LITTLE;
> -#endif
> -#if defined(TARGET_I386)
> -    if (flags == 2) {
> -        s.info.mach = bfd_mach_x86_64;
> -    } else if (flags == 1) {
> -        s.info.mach = bfd_mach_i386_i8086;
> -    } else {
> -        s.info.mach = bfd_mach_i386_i386;
> -    }
> -    print_insn = print_insn_i386;
> -#elif defined(TARGET_ARM)
> +#if defined(TARGET_ARM)
>      print_insn = print_insn_arm;
>  #elif defined(TARGET_ALPHA)
>      print_insn = print_insn_alpha;
> -#elif defined(TARGET_SPARC)
> -    print_insn = print_insn_sparc;
> -#ifdef TARGET_SPARC64
> -    s.info.mach = bfd_mach_sparc_v9b;
> -#endif
>  #elif defined(TARGET_PPC)
>      if (flags & 0xFFFF) {
>          /* If we have a precise definition of the instruction set, use it. */
> @@ -501,31 +494,12 @@ void monitor_disas(Monitor *mon, CPUArchState *env,
>          s.info.endian = BFD_ENDIAN_LITTLE;
>      }
>      print_insn = print_insn_ppc;
> -#elif defined(TARGET_M68K)
> -    print_insn = print_insn_m68k;
> -#elif defined(TARGET_MIPS)
> -#ifdef TARGET_WORDS_BIGENDIAN
> -    print_insn = print_insn_big_mips;
> -#else
> -    print_insn = print_insn_little_mips;
> -#endif
> -#elif defined(TARGET_SH4)
> -    s.info.mach = bfd_mach_sh4;
> -    print_insn = print_insn_sh;
> -#elif defined(TARGET_S390X)
> -    s.info.mach = bfd_mach_s390_64;
> -    print_insn = print_insn_s390;
> -#elif defined(TARGET_MOXIE)
> -    s.info.mach = bfd_arch_moxie;
> -    print_insn = print_insn_moxie;
> -#elif defined(TARGET_LM32)
> -    s.info.mach = bfd_mach_lm32;
> -    print_insn = print_insn_lm32;
> -#else
> -    monitor_printf(mon, "0x" TARGET_FMT_lx
> -                   ": Asm output not supported on this arch\n", pc);
> -    return;
>  #endif
> +    if (!print_insn) {
> +        monitor_printf(mon, "0x" TARGET_FMT_lx
> +                       ": Asm output not supported on this arch\n", pc);
> +        return;
> +    }
>  
>      for(i = 0; i < nb_insn; i++) {
>       monitor_printf(mon, "0x" TARGET_FMT_lx ":  ", pc);
> 

Note: tested only the ARMv8 path of this one.

Reviewed-by: Claudio Fontana <address@hidden>