Re: [PATCH v2] target/ppc: check tb_env != 0 before printing TBU/TBL/DECR

[Daniel Henrique Barboza]

On 7/14/22 13:17, Matheus K. Ferst wrote:
> On 14/07/2022 10:35, Daniel Henrique Barboza wrote:
> > On 7/13/22 15:38, Matheus Ferst wrote:
> > > When using "-machine none", env->tb_env is not allocated, causing the
> > > segmentation fault reported in issue #85 (launchpad bug #811683). To
> > > avoid this problem, check if the pointer != NULL before calling the
> > > methods to print TBU/TBL/DECR.
> > >
> > > Resolves: https://gitlab.com/qemu-project/qemu/-/issues/85
> > > Signed-off-by: Matheus Ferst <matheus.ferst@eldorado.org.br>
> > > ---
> > > v2:
> > >   - Added checks in monitor_get_decr, monitor_get_tbu, and monitor_get_tbl.
> > >   - Link to v1: 
> > > https://lists.gnu.org/archive/html/qemu-ppc/2022-07/msg00173.html
> > > ---
> > >   target/ppc/cpu_init.c | 16 ++++++++--------
> > >   target/ppc/monitor.c  |  9 +++++++++
> > >   2 files changed, 17 insertions(+), 8 deletions(-)
> > >
> > > diff --git a/target/ppc/cpu_init.c b/target/ppc/cpu_init.c
> > > index 86ad28466a..7e96baac9f 100644
> > > --- a/target/ppc/cpu_init.c
> > > +++ b/target/ppc/cpu_init.c
> > > @@ -7476,18 +7476,18 @@ void ppc_cpu_dump_state(CPUState *cs, FILE *f, int 
> > > flags)
> > >                    "%08x iidx %d didx %d\n",
> > >                    env->msr, env->spr[SPR_HID0], env->hflags,
> > >                    cpu_mmu_index(env, true), cpu_mmu_index(env, false));
> > > -#if !defined(NO_TIMER_DUMP)
> >
> > Why did you remove the NO_TIMER_DUMP check? Is it redundant with the env->tb_env
> > check?
>
> This is the only reference to this macro since it was added in d9bce9d99f46. I 
> suppose it was manually defined, but the only discussion[1] I could find around 
> this patch doesn't mention it. I don't see any other reason to define it other 
> than avoiding segfaults in machines that don't allocate env_tb, but we can keep 
> it if you prefer.

Thanks for the explanation. And nah, no need to keep it around since you're
already gating the code by checking  env->tb_env.


Daniel


> > > -    qemu_fprintf(f, "TB %08" PRIu32 " %08" PRIu64
> > > +    if (env->tb_env) {
> > > +        qemu_fprintf(f, "TB %08" PRIu32 " %08" PRIu64
> > >   #if !defined(CONFIG_USER_ONLY)
> > > -                 " DECR " TARGET_FMT_lu
> > > +                     " DECR " TARGET_FMT_lu
> > >   #endif
> > > -                 "\n",
> > > -                 cpu_ppc_load_tbu(env), cpu_ppc_load_tbl(env)
> > > +                     "\n",
> > > +                     cpu_ppc_load_tbu(env), cpu_ppc_load_tbl(env)
> > >   #if !defined(CONFIG_USER_ONLY)
> > > -                 , cpu_ppc_load_decr(env)
> > > -#endif
> > > -        );
> > > +                     , cpu_ppc_load_decr(env)
> > >   #endif
> > > +            );
> > > +    }
> >
> > Not really a problem with your patch, but since you're changing this code, can
> > you please cleanse it from evil? I mean, look at this:
> >
> >
> >      if (env->tb_env) {
> >          qemu_fprintf(f, "TB %08" PRIu32 " %08" PRIu64
> > #if !defined(CONFIG_USER_ONLY)
> >                       " DECR " TARGET_FMT_lu
> > #endif
> >                       "\n",
> >                       cpu_ppc_load_tbu(env), cpu_ppc_load_tbl(env)
> > #if !defined(CONFIG_USER_ONLY)
> >                       , cpu_ppc_load_decr(env)
> > #endif
> >              );
> >      }
> >
> >
> > 2 ifdef macros in the middle of qemu_fprintf() params? With one line starting
> > with a ', '?  Why are we trading sanity for 3 lines of code repetition?
> >
> > We can --at least-- do something like this:
> >
> >      if (env->tb_env) {
> > #if !defined(CONFIG_USER_ONLY)
> >          qemu_fprintf(f, "TB %08" PRIu32 " %08" PRIu64
> >                       " DECR " TARGET_FMT_lu
> >                       "\n",
> >                       cpu_ppc_load_tbu(env), cpu_ppc_load_tbl(env),
> >                       cpu_ppc_load_decr(env));
> > #else
> >          qemu_fprintf(f, "TB %08" PRIu32 " %08" PRIu64
> >                       "\n",
> >                       cpu_ppc_load_tbu(env), cpu_ppc_load_tbl(env));
> > #endif
> >      }
> >
> >
> > Thanks,
> >
> >
> > Daniel
>
> Sure, I'll change that in v3.
>
> [1] https://lists.gnu.org/archive/html/qemu-devel/2007-03/msg00239.html
>
> Thanks,
> Matheus K. Ferst
> Instituto de Pesquisas ELDORADO <http://www.eldorado.org.br/>
> Analista de Software
> Aviso Legal - Disclaimer <https://www.eldorado.org.br/disclaimer.html>