[Top][All Lists]

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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

From: Bastian Koppelmann
Subject: Re: [Qemu-discuss] [Qemu-devel] Segmentation fault on target tricore
Date: Wed, 18 Sep 2019 17:54:18 +0200
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101 Thunderbird/68.1.0

Hi Peter,

On 9/17/19 3:45 PM, Peter Maydell wrote:
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.
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

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.



-- PMM

reply via email to

[Prev in Thread] Current Thread [Next in Thread]