Peter Crosthwaite <peter.crosthwa...@xilinx.com> writes: > On Tue, Jun 16, 2015 at 8:09 AM, Markus Armbruster <arm...@redhat.com> wrote: >> Peter Crosthwaite <crosthwaitepe...@gmail.com> writes: >> >>> The monitor currently has one helper, mon_get_cpu() which will return >>> an env pointer. The target specific users of this API want an env, but >>> all the target agnostic users really just want the cpu pointer. These >>> users then need to use the target-specifically defined ENV_GET_CPU to >>> navigate back up to the CPU from the ENV. Split the API for the two >>> uses cases to remove all need for ENV_GET_CPU. >>> >>> Reviewed-by: Richard Henderson <r...@twiddle.net> >>> Reviewed-by: Andreas Färber <afaer...@suse.de> >>> Signed-off-by: Peter Crosthwaite <crosthwaite.pe...@gmail.com> >>> --- >>> Changed since v1: >>> s/mon_get_env/mon_get_cpu_env (Andreas review) >>> Avoid C99 declaration (RH review) >>> --- >>> monitor.c | 65 >>> ++++++++++++++++++++++++++++----------------------------------- >>> 1 file changed, 29 insertions(+), 36 deletions(-) >>> >> [...] >>> @@ -1208,16 +1203,14 @@ static void monitor_printc(Monitor *mon, int c) >>> static void memory_dump(Monitor *mon, int count, int format, int wsize, >>> hwaddr addr, int is_physical) >>> { >>> - CPUArchState *env; >>> int l, line_size, i, max_digits, len; >>> uint8_t buf[16]; >>> uint64_t v; >>> >>> if (format == 'i') { >>> - int flags; >>> - flags = 0; >>> - env = mon_get_cpu(); >>> + int flags = 0; >>> #ifdef TARGET_I386 >>> + CPUArchState *env = mon_get_cpu_env(); >>> if (wsize == 2) { >>> flags = 1; >>> } else if (wsize == 4) { >>> @@ -1238,10 +1231,11 @@ static void memory_dump(Monitor *mon, int count, >>> int format, int wsize, >>> } >>> #endif >>> #ifdef TARGET_PPC >>> + CPUArchState *env = mon_get_cpu_env(); >>> flags = msr_le << 16; >>> flags |= env->bfd_mach; >>> #endif >>> - monitor_disas(mon, env, addr, count, is_physical, flags); >>> + monitor_disas(mon, mon_get_cpu_env(), addr, count, is_physical, >>> flags); >>> return; >>> } >>> >>> @@ -1280,8 +1274,7 @@ static void memory_dump(Monitor *mon, int count, int >>> format, int wsize, >>> if (is_physical) { >>> cpu_physical_memory_read(addr, buf, l); >>> } else { >>> - env = mon_get_cpu(); >>> - if (cpu_memory_rw_debug(ENV_GET_CPU(env), addr, buf, l, 0) < >>> 0) { >>> + if (cpu_memory_rw_debug(mon_get_cpu(), addr, buf, l, 0) < 0) { >>> monitor_printf(mon, " Cannot access memory\n"); >>> break; >>> } >> >> You call mon_get_cpu_env() and mon_get_cpu() multiple times in this >> function, and declare CPUArchState *env twice (which rang my shadowing >> alarm bell; fortunately the two are under mutually exclusive #ifdef). >> Why not simply do >> >> CPUState *cpu = mon_get_cpu(); > > This we can do easily and the choice of the existing code is pure > personal preference. I generally prefer inline calls to trivial > getters for a low number of call sites as I think code is slightly > easier to read when you don't have to do a local variable lookup on > where all the function args are coming from.
Point taken. The getter isn't quite trivial, though: cpu_synchronize_state(). >> CPUArchState *env = mon_get_cpu_env(); >> > > So the multiple decl of env is now needed to avoid an unused variable > werror for non-x86-non-PPC builds when considering the change in P2. > Not strictly needed until P2, but doing it this way straight up > slightly minimises churn. The ENV_GET_CPU removal and the change in P2 > remove the only two unconditional uses of env forcing this variable to > go local to its #ifdef usages. It also has the advantage that only > those two arches use the env at all. Envlessness is a good thing for > common code so prefer to leave the multi-defs in target specific code > with a plan to get rid of them one by one with X86/PPC specific > patches that operate solely on their respective #ifdef sections. > > FWIW, the follow up to this series adds the infrastructure needed for > getting rid of these #ifdef sections (once I muster the courage to > patch X86 and PPC): > > https://lists.gnu.org/archive/html/qemu-devel/2015-05/msg04742.html > >> right at the beginning and be done with it? >> >> Not a big deal, I'm willing to take this patch through my tree as is. >> > > Let me know if you need respin for the first change. I can turn it Your choice. As I said, I'm willing to take it as is. > around quickly, i'd really like to get this one through as its > blocking a lot of the multi-arch work! Aiming for a pull request this week.