On Tue, Jun 16, 2015 at 11:42 PM, Markus Armbruster <[email protected]> wrote: > Peter Crosthwaite <[email protected]> writes: > >> On Tue, Jun 16, 2015 at 8:09 AM, Markus Armbruster <[email protected]> wrote: >>> Peter Crosthwaite <[email protected]> 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 <[email protected]> >>>> Reviewed-by: Andreas Färber <[email protected]> >>>> Signed-off-by: Peter Crosthwaite <[email protected]> >>>> --- >>>> 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. >
Thanks, as it is queued I will leave it and I have made a note in my todos to follow this up. Regards, Peter >> 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. >
