On Tue, May 5, 2015 at 10:43 AM, Richard Henderson <r...@twiddle.net> wrote: > On 05/05/2015 10:19 AM, Peter Maydell wrote: >> On 5 May 2015 at 05:45, Peter Crosthwaite <crosthwaitepe...@gmail.com> wrote: >>> Add the ARM specific disassembly flags setup, so ARM can be correctly >>> disassembled from the monitor. >>> >>> Signed-off-by: Peter Crosthwaite <crosthwaite.pe...@gmail.com> >>> --- >>> monitor.c | 11 +++++++++++ >>> 1 file changed, 11 insertions(+) >>> >>> diff --git a/monitor.c b/monitor.c >>> index d831d98..9d9f1e2 100644 >>> --- a/monitor.c >>> +++ b/monitor.c >>> @@ -1217,6 +1217,17 @@ static void memory_dump(Monitor *mon, int count, int >>> format, int wsize, >>> int flags; >>> flags = 0; >>> env = mon_get_cpu(); >>> +#ifdef TARGET_ARM >>> + if (env->thumb) { >>> + flags |= 1; >>> + } >>> + if (env->bswap_code) { >>> + flags |= 2; >>> + } >>> + if (env->aarch64) { >>> + flags |= 4; >>> + } >>> +#endif >> >> monitor.c has no business poking around in the CPU state >> internals like this... You probably want a CPU method >> get_disas_flags() or something. >> >> -- PMM >> > > While this patch set does improve the current dismal state of affairs, I think > the ideal solution is a cpu method that takes care of all the disassembly info > setup. > > Indeed, the flags setup becomes less obscure when, instead of > > #ifdef TARGET_I386 > if (wsize == 2) { > flags = 1; > } else if (wsize == 4) { > flags = 0; > } else { > /* as default we use the current CS size */ > flags = 0; > if (env) { > #ifdef TARGET_X86_64 > if ((env->efer & MSR_EFER_LMA) && > (env->segs[R_CS].flags & DESC_L_MASK)) > flags = 2; > else > #endif > if (!(env->segs[R_CS].flags & DESC_B_MASK)) > flags = 1; > } > } > > in one place and > > #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; > > in another, we merge the two so that we get > > s.info.mach = bfd_mach_i386_i8086; > if (env->hflags & (1U << HF_CS32_SHIFT)) { > s.info.mach = bfd_mach_i386_i386; > } > #ifdef TARGET_X86_64 > if (env->hflags & (1U << HF_CS64_SHIFT)) { > s.info.mach = bfd_mach_x86_64; > } > #endif > > > I'm not sure what the right interface for this is, exactly. But I'd think > that > the CPUDebug structure would be initialized in the caller -- target or monitor > -- while the mach and whatnot get filled in by the cpu hook. Maybe even have > the cpu hook return the print_insn function to use. >
So something else I just thought of and started worrying about, is the the target_disas path is driving some of the flags from the disas context, whereas a hook will only have access to the CPUState/env. Can we rely on the env/CPUState always being up to date during target_disas (which happens at translate time?) or will we need to go field by field to make sure any env updates explicitly occur before target_disas? Regards, Peter > > > r~ >