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.
> 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
around quickly, i'd really like to get this one through as its
blocking a lot of the multi-arch work!
Regards,
Peter
> [...]
>
