Re: [Qemu-devel] [Qemu-discuss] Segmentation fault on target tricore

[Peter Maydell]

On Tue, 17 Sep 2019 at 14:06, Konopik, Andreas <address@hidden> wrote:
>
> >> Using gdb and valgrind I found out that:
> >> - 'gen_mtcr()' and 'gen_mfcr()' access uninitialized values, i.e.
> >> CSFRs,
> >> which leads to the Segfault
> >> - The uninitialised values were created by stack allocation of
> >> DisasContext in 'gen_intermediate_code()'
> >
> > This definitely sounds like a bug: do you have a stack
> > backtrace from valgrind or gdb of the bad access and the
> > segfault?
> >
> > [...]
> > Thread 3 "qemu-system-tri" received signal SIGSEGV, Segmentation fault.
> > [Switching to Thread 0x7ffff10a4700 (LWP 146730)]
> > 0x00005555556edb67 in gen_mfcr (ret=0xab0, offset=<optimized out>,
> >    ctx=<optimized out>)
> >    at /home/akonopik/qemu_src/target/tricore/cpu.h:274
> > 274       return (env->features & (1ULL << feature)) != 0;
> > (gdb) bt
> > #0  0x00005555556edb67 in gen_mfcr
> >    (ret=0xab0, offset=<optimized out>, ctx=<optimized out>)
> >    at /home/akonopik/qemu_src/target/tricore/cpu.h:274

It looks like tricore_tr_init_disas_context() isn't
initializing ctx->env. If this is the problem then this
patch ought to fix it:

diff --git a/target/tricore/translate.c b/target/tricore/translate.c
index c574638c9f7..305d896cd2c 100644
--- a/target/tricore/translate.c
+++ b/target/tricore/translate.c
@@ -8793,6 +8793,7 @@ static void
tricore_tr_init_disas_context(DisasContextBase *dcbase,
     CPUTriCoreState *env = cs->env_ptr;
     ctx->mem_idx = cpu_mmu_index(env, false);
     ctx->hflags = (uint32_t)ctx->base.tb->flags;
+    ctx->env = env;
 }

 static void tricore_tr_tb_start(DisasContextBase *db, CPUState *cpu)


Aside to Bastian: passing the CPU env pointer into the
DisasContext isn't ideal, because it makes it quite easy
for translate.c code to accidentally use fields of the
env struct that aren't valid for use at translate time.
I think the only uses of ctx->env you have are for checking
feature bits -- I recommend following what target/arm does here:
 * in tricore_tr_init_disas_context(), instead of copying the
   env pointer, just copy env->features into ctx->features
 * have a tricore_dc_feature(DisasContext *dc, int feature)
   that checks for the feature bit in dc->features

That way you only have access in translate.c code to
information that's safe to bake into generated code, and
it's harder to accidentally introduce bugs where generated
code depends on CPU state that isn't kept in the TB flags.

thanks
-- PMM