| Message ID | 20170425104338.31984-1-rth@twiddle.net |
|---|---|
| State | New |
| Headers | show |
Richard Henderson <rth@twiddle.net> writes: > Users of tcg_gen_atomic_cmpxchg and do_atomic_op rightfully utilize > the output. Even though this code is dead, it gets translated, and > without the initialization we encounter a tcg_error. > > Reported-by: Nikunj A Dadhania <nikunj@linux.vnet.ibm.com> > Signed-off-by: Richard Henderson <rth@twiddle.net> With this the tcg_error goes away. But then powernv skiboot code [1] enters into infinite loop. Basically, in target/ppc/translate.c:gen_conditional_store(), setcond_tl will always fail, and CRF_EQ_BIT will never be set, the lock will never be taken. So "make check" still fails at powernv serial test. ./configure --target-list=ppc64-softmmu --cc=clang --host-cc=clang && make && make check > --- > tcg/tcg-op.c | 6 ++++++ > 1 file changed, 6 insertions(+) > > diff --git a/tcg/tcg-op.c b/tcg/tcg-op.c > index 95a39b7..6b1f415 100644 > --- a/tcg/tcg-op.c > +++ b/tcg/tcg-op.c > @@ -2861,6 +2861,9 @@ void tcg_gen_atomic_cmpxchg_i64(TCGv_i64 retv, TCGv addr, TCGv_i64 cmpv, > #endif > #else > gen_helper_exit_atomic(tcg_ctx.tcg_env); > + /* Produce a result, so that we have a well-formed opcode stream > + with respect to uses of the result in the (dead) code following. */ > + tcg_gen_movi_i64(retv, 0); > #endif /* CONFIG_ATOMIC64 */ > } else { > TCGv_i32 c32 = tcg_temp_new_i32(); > @@ -2966,6 +2969,9 @@ static void do_atomic_op_i64(TCGv_i64 ret, TCGv addr, TCGv_i64 val, > #endif > #else > gen_helper_exit_atomic(tcg_ctx.tcg_env); > + /* Produce a result, so that we have a well-formed opcode stream > + with respect to uses of the result in the (dead) code following. */ > + tcg_gen_movi_i64(ret, 0); > #endif /* CONFIG_ATOMIC64 */ > } else { > TCGv_i32 v32 = tcg_temp_new_i32(); > -- Regards, Nikunj 1. https://github.com/open-power/skiboot/blob/master/asm/lock.S#L36
On 04/25/2017 01:21 PM, Nikunj A Dadhania wrote: > Richard Henderson <rth@twiddle.net> writes: > >> Users of tcg_gen_atomic_cmpxchg and do_atomic_op rightfully utilize >> the output. Even though this code is dead, it gets translated, and >> without the initialization we encounter a tcg_error. >> >> Reported-by: Nikunj A Dadhania <nikunj@linux.vnet.ibm.com> >> Signed-off-by: Richard Henderson <rth@twiddle.net> > > With this the tcg_error goes away. > > But then powernv skiboot code [1] enters into infinite loop. Basically, > in target/ppc/translate.c:gen_conditional_store(), setcond_tl will > always fail, and CRF_EQ_BIT will never be set, the lock will never be > taken. The setcond_tl *shouldn't* always fail. If that's the case, then we have another bug in the !parallel_cpus code path for gen_conditional_store. r~
Richard Henderson <rth@twiddle.net> writes: > Users of tcg_gen_atomic_cmpxchg and do_atomic_op rightfully utilize > the output. Even though this code is dead, it gets translated, and > without the initialization we encounter a tcg_error. > > Reported-by: Nikunj A Dadhania <nikunj@linux.vnet.ibm.com> > Signed-off-by: Richard Henderson <rth@twiddle.net> Tested-by: Nikunj A Dadhania <nikunj@linux.vnet.ibm.com> > --- > tcg/tcg-op.c | 6 ++++++ > 1 file changed, 6 insertions(+) > > diff --git a/tcg/tcg-op.c b/tcg/tcg-op.c > index 95a39b7..6b1f415 100644 > --- a/tcg/tcg-op.c > +++ b/tcg/tcg-op.c > @@ -2861,6 +2861,9 @@ void tcg_gen_atomic_cmpxchg_i64(TCGv_i64 retv, TCGv addr, TCGv_i64 cmpv, > #endif > #else > gen_helper_exit_atomic(tcg_ctx.tcg_env); > + /* Produce a result, so that we have a well-formed opcode stream > + with respect to uses of the result in the (dead) code following. */ > + tcg_gen_movi_i64(retv, 0); > #endif /* CONFIG_ATOMIC64 */ > } else { > TCGv_i32 c32 = tcg_temp_new_i32(); > @@ -2966,6 +2969,9 @@ static void do_atomic_op_i64(TCGv_i64 ret, TCGv addr, TCGv_i64 val, > #endif > #else > gen_helper_exit_atomic(tcg_ctx.tcg_env); > + /* Produce a result, so that we have a well-formed opcode stream > + with respect to uses of the result in the (dead) code following. */ > + tcg_gen_movi_i64(ret, 0); > #endif /* CONFIG_ATOMIC64 */ > } else { > TCGv_i32 v32 = tcg_temp_new_i32(); > -- > 2.9.3
On 25 April 2017 at 11:43, Richard Henderson <rth@twiddle.net> wrote: > Users of tcg_gen_atomic_cmpxchg and do_atomic_op rightfully utilize > the output. Even though this code is dead, it gets translated, and > without the initialization we encounter a tcg_error. > > Reported-by: Nikunj A Dadhania <nikunj@linux.vnet.ibm.com> > Signed-off-by: Richard Henderson <rth@twiddle.net> > --- > tcg/tcg-op.c | 6 ++++++ > 1 file changed, 6 insertions(+) > > diff --git a/tcg/tcg-op.c b/tcg/tcg-op.c > index 95a39b7..6b1f415 100644 > --- a/tcg/tcg-op.c > +++ b/tcg/tcg-op.c > @@ -2861,6 +2861,9 @@ void tcg_gen_atomic_cmpxchg_i64(TCGv_i64 retv, TCGv addr, TCGv_i64 cmpv, > #endif > #else > gen_helper_exit_atomic(tcg_ctx.tcg_env); > + /* Produce a result, so that we have a well-formed opcode stream > + with respect to uses of the result in the (dead) code following. */ > + tcg_gen_movi_i64(retv, 0); > #endif /* CONFIG_ATOMIC64 */ > } else { > TCGv_i32 c32 = tcg_temp_new_i32(); > @@ -2966,6 +2969,9 @@ static void do_atomic_op_i64(TCGv_i64 ret, TCGv addr, TCGv_i64 val, > #endif > #else > gen_helper_exit_atomic(tcg_ctx.tcg_env); > + /* Produce a result, so that we have a well-formed opcode stream > + with respect to uses of the result in the (dead) code following. */ > + tcg_gen_movi_i64(ret, 0); > #endif /* CONFIG_ATOMIC64 */ > } else { > TCGv_i32 v32 = tcg_temp_new_i32(); > -- Tested-by: Peter Maydell <peter.maydell@linaro.org> Without this patch an AArch64 QEMU crashes on startup if I build it with clang and with optimization enabled. We should probably get this into master sooner rather than later... thanks -- PMM
diff --git a/tcg/tcg-op.c b/tcg/tcg-op.c index 95a39b7..6b1f415 100644 --- a/tcg/tcg-op.c +++ b/tcg/tcg-op.c @@ -2861,6 +2861,9 @@ void tcg_gen_atomic_cmpxchg_i64(TCGv_i64 retv, TCGv addr, TCGv_i64 cmpv, #endif #else gen_helper_exit_atomic(tcg_ctx.tcg_env); + /* Produce a result, so that we have a well-formed opcode stream + with respect to uses of the result in the (dead) code following. */ + tcg_gen_movi_i64(retv, 0); #endif /* CONFIG_ATOMIC64 */ } else { TCGv_i32 c32 = tcg_temp_new_i32(); @@ -2966,6 +2969,9 @@ static void do_atomic_op_i64(TCGv_i64 ret, TCGv addr, TCGv_i64 val, #endif #else gen_helper_exit_atomic(tcg_ctx.tcg_env); + /* Produce a result, so that we have a well-formed opcode stream + with respect to uses of the result in the (dead) code following. */ + tcg_gen_movi_i64(ret, 0); #endif /* CONFIG_ATOMIC64 */ } else { TCGv_i32 v32 = tcg_temp_new_i32();
Users of tcg_gen_atomic_cmpxchg and do_atomic_op rightfully utilize the output. Even though this code is dead, it gets translated, and without the initialization we encounter a tcg_error. Reported-by: Nikunj A Dadhania <nikunj@linux.vnet.ibm.com> Signed-off-by: Richard Henderson <rth@twiddle.net> --- tcg/tcg-op.c | 6 ++++++ 1 file changed, 6 insertions(+)