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

[Bastian Koppelmann <kbastian@mail.uni-paderborn.de>]

Hi Peter,

On 9/17/19 3:45 PM, Peter Maydell wrote:
> On Tue, 17 Sep 2019 at 14:06, Konopik, Andreas <andreas.konopik@fau.de> 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)

thanks for the patch. I'll add it to my queue.


>
>
> 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.


Yes, this is not intended to stay this way. However, it was a necessary 
change for the translate_loop conversion. I'll try removing ctx->env 
from TriCore.

Cheers,

Bastian


>
> thanks
> -- PMM
>