[Top][All Lists]
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH] target-arm: cleanup internal resource leaks
|
From: |
Laurent Desnogues |
|
Subject: |
Re: [Qemu-devel] [PATCH] target-arm: cleanup internal resource leaks |
|
Date: |
Wed, 21 Oct 2009 21:20:07 +0200 |
On Mon, Oct 19, 2009 at 1:44 PM, <address@hidden> wrote:
> Current ARM translator code has several places where it leaves
> temporary TCG variables alive. This patch removes all such instances I
> have found so far. Sorry for the mangled inlined patch, the mailserver
> I use doesn't like patches so I've also included it as an attachment
> that hopefully status correctly formatted. Should apply cleanly
> against latest git.
Here are my comments. A lack of comment means I agree :-)
> Signed-off-by: Juha Riihimäki <address@hidden>
> ---
> diff --git a/target-arm/translate.c b/target-arm/translate.c
> index e56082b..bc51bcb 100644
> --- a/target-arm/translate.c
> +++ b/target-arm/translate.c
> @@ -287,6 +287,7 @@ static TCGv_i64 gen_mulu_i64_i32(TCGv a, TCGv b)
> tcg_gen_extu_i32_i64(tmp2, b);
> dead_tmp(b);
> tcg_gen_mul_i64(tmp1, tmp1, tmp2);
> + tcg_temp_free_i64(tmp2);
> return tmp1;
> }
>
> @@ -300,6 +301,7 @@ static TCGv_i64 gen_muls_i64_i32(TCGv a, TCGv b)
> tcg_gen_ext_i32_i64(tmp2, b);
> dead_tmp(b);
> tcg_gen_mul_i64(tmp1, tmp1, tmp2);
> + tcg_temp_free_i64(tmp2);
> return tmp1;
> }
>
> @@ -312,9 +314,11 @@ static void gen_mull(TCGv a, TCGv b)
> tcg_gen_extu_i32_i64(tmp1, a);
> tcg_gen_extu_i32_i64(tmp2, b);
> tcg_gen_mul_i64(tmp1, tmp1, tmp2);
> + tcg_temp_free_i64(tmp2);
> tcg_gen_trunc_i64_i32(a, tmp1);
> tcg_gen_shri_i64(tmp1, tmp1, 32);
> tcg_gen_trunc_i64_i32(b, tmp1);
> + tcg_temp_free_i64(tmp1);
> }
>
> /* Signed 32x32->64 multiply. */
> @@ -326,9 +330,11 @@ static void gen_imull(TCGv a, TCGv b)
> tcg_gen_ext_i32_i64(tmp1, a);
> tcg_gen_ext_i32_i64(tmp2, b);
> tcg_gen_mul_i64(tmp1, tmp1, tmp2);
> + tcg_temp_free_i64(tmp2);
> tcg_gen_trunc_i64_i32(a, tmp1);
> tcg_gen_shri_i64(tmp1, tmp1, 32);
> tcg_gen_trunc_i64_i32(b, tmp1);
> + tcg_temp_free_i64(tmp1);
> }
>
> /* Swap low and high halfwords. */
> @@ -542,11 +548,13 @@ static void gen_arm_parallel_addsub(int op1, int
> op2, TCGv a, TCGv b)
> tmp = tcg_temp_new_ptr();
> tcg_gen_addi_ptr(tmp, cpu_env, offsetof(CPUState, GE));
> PAS_OP(s)
> + tcg_temp_free_ptr(tmp);
> break;
> case 5:
> tmp = tcg_temp_new_ptr();
> tcg_gen_addi_ptr(tmp, cpu_env, offsetof(CPUState, GE));
> PAS_OP(u)
> + tcg_temp_free_ptr(tmp);
> break;
> #undef gen_pas_helper
> #define gen_pas_helper(name) glue(gen_helper_,name)(a, a, b)
> @@ -587,11 +595,13 @@ static void gen_thumb2_parallel_addsub(int op1,
> int op2, TCGv a, TCGv b)
> tmp = tcg_temp_new_ptr();
> tcg_gen_addi_ptr(tmp, cpu_env, offsetof(CPUState, GE));
> PAS_OP(s)
> + tcg_temp_free_ptr(tmp);
> break;
> case 4:
> tmp = tcg_temp_new_ptr();
> tcg_gen_addi_ptr(tmp, cpu_env, offsetof(CPUState, GE));
> PAS_OP(u)
> + tcg_temp_free_ptr(tmp);
> break;
> #undef gen_pas_helper
> #define gen_pas_helper(name) glue(gen_helper_,name)(a, a, b)
> @@ -995,10 +1005,12 @@ static inline void gen_vfp_tosiz(int dp)
> #define VFP_GEN_FIX(name) \
> static inline void gen_vfp_##name(int dp, int shift) \
> { \
> + TCGv tmp_shift = tcg_const_i32(shift); \
> if (dp) \
> - gen_helper_vfp_##name##d(cpu_F0d, cpu_F0d, tcg_const_i32
> (shift), cpu_env);\
> + gen_helper_vfp_##name##d(cpu_F0d, cpu_F0d, tmp_shift,
> cpu_env);\
> else \
> - gen_helper_vfp_##name##s(cpu_F0s, cpu_F0s, tcg_const_i32
> (shift), cpu_env);\
> + gen_helper_vfp_##name##s(cpu_F0s, cpu_F0s, tmp_shift,
> cpu_env);\
> + tcg_temp_free_i32(tmp_shift); \
> }
> VFP_GEN_FIX(tosh)
> VFP_GEN_FIX(tosl)
> @@ -2399,7 +2411,7 @@ static int disas_dsp_insn(CPUState *env,
> DisasContext *s, uint32_t insn)
> instruction is not defined. */
> static int disas_cp_insn(CPUState *env, DisasContext *s, uint32_t
> insn)
> {
> - TCGv tmp;
> + TCGv tmp, tmp2;
> uint32_t rd = (insn >> 12) & 0xf;
> uint32_t cp = (insn >> 8) & 0xf;
> if (IS_USER(s)) {
> @@ -2411,14 +2423,18 @@ static int disas_cp_insn(CPUState *env,
> DisasContext *s, uint32_t insn)
> return 1;
> gen_set_pc_im(s->pc);
> tmp = new_tmp();
> - gen_helper_get_cp(tmp, cpu_env, tcg_const_i32(insn));
> + tmp2 = tcg_const_i32(insn);
> + gen_helper_get_cp(tmp, cpu_env, tmp2);
> + tcg_temp_free(tmp2);
> store_reg(s, rd, tmp);
> } else {
> if (!env->cp[cp].cp_write)
> return 1;
> gen_set_pc_im(s->pc);
> tmp = load_reg(s, rd);
> - gen_helper_set_cp(cpu_env, tcg_const_i32(insn), tmp);
> + tmp2 = tcg_const_i32(insn);
> + gen_helper_set_cp(cpu_env, tmp2, tmp);
> + tcg_temp_free(tmp2);
> dead_tmp(tmp);
> }
> return 0;
> @@ -2449,7 +2465,7 @@ static int cp15_user_ok(uint32_t insn)
> static int disas_cp15_insn(CPUState *env, DisasContext *s, uint32_t
> insn)
> {
> uint32_t rd;
> - TCGv tmp;
> + TCGv tmp, tmp2;
>
> /* M profile cores use memory mapped registers instead of cp15.
> */
> if (arm_feature(env, ARM_FEATURE_M))
> @@ -2478,9 +2494,10 @@ static int disas_cp15_insn(CPUState *env,
> DisasContext *s, uint32_t insn)
> return 0;
> }
> rd = (insn >> 12) & 0xf;
> + tmp2 = tcg_const_i32(insn);
> if (insn & ARM_CP_RW_BIT) {
> tmp = new_tmp();
> - gen_helper_get_cp15(tmp, cpu_env, tcg_const_i32(insn));
> + gen_helper_get_cp15(tmp, cpu_env, tmp2);
> /* If the destination register is r15 then sets condition
> codes. */
> if (rd != 15)
> store_reg(s, rd, tmp);
> @@ -2488,7 +2505,7 @@ static int disas_cp15_insn(CPUState *env,
> DisasContext *s, uint32_t insn)
> dead_tmp(tmp);
> } else {
> tmp = load_reg(s, rd);
> - gen_helper_set_cp15(cpu_env, tcg_const_i32(insn), tmp);
> + gen_helper_set_cp15(cpu_env, tmp2, tmp);
> dead_tmp(tmp);
> /* Normally we would always end the TB here, but Linux
> * arch/arm/mach-pxa/sleep.S expects two instructions
> following
> @@ -2497,6 +2514,7 @@ static int disas_cp15_insn(CPUState *env,
> DisasContext *s, uint32_t insn)
> (insn & 0x0fff0fff) != 0x0e010f10)
> gen_lookup_tb(s);
> }
> + tcg_temp_free_i32(tmp2);
> return 0;
> }
>
> @@ -4676,12 +4694,16 @@ static int disas_neon_data_insn(CPUState *
> env, DisasContext *s, uint32_t insn)
> gen_neon_narrow_satu(size - 1, tmp,
> cpu_V0);
> }
> if (pass == 0) {
> + dead_tmp(tmp2);
This looks wrong if size == 3 since we have TCGV_UNUSED(tmp2).
> tmp2 = tmp;
> } else {
> neon_store_reg(rd, 0, tmp2);
> neon_store_reg(rd, 1, tmp);
> }
> } /* for pass */
> + if (size == 3) {
> + tcg_temp_free_i64(tmp64);
> + }
> } else if (op == 10) {
> /* VSHLL */
> if (q || size == 3)
> @@ -5147,6 +5169,7 @@ static int disas_neon_data_insn(CPUState * env,
> DisasContext *s, uint32_t insn)
> tcg_gen_shli_i64(cpu_V1, cpu_V1, 64 - (imm * 8));
> tcg_gen_shri_i64(tmp64, tmp64, imm * 8);
> tcg_gen_or_i64(cpu_V1, cpu_V1, tmp64);
> + tcg_temp_free_i64(tmp64);
> } else {
> /* BUGFIX */
> neon_load_reg64(cpu_V0, rn);
> @@ -5511,8 +5534,9 @@ static int disas_neon_data_insn(CPUState * env,
> DisasContext *s, uint32_t insn)
> tcg_gen_movi_i32(tmp, 0);
> }
> tmp2 = neon_load_reg(rm, 0);
> - gen_helper_neon_tbl(tmp2, tmp2, tmp, tcg_const_i32(rn),
> - tcg_const_i32(n));
> + TCGv tmp_rn = tcg_const_i32(rn);
> + TCGv tmp_n = tcg_const_i32(n);
I don't know what QEMU coding rules say about declarations in
the middle of the code, but I don't like them too much :-)
> + gen_helper_neon_tbl(tmp2, tmp2, tmp, tmp_rn, tmp_n);
> dead_tmp(tmp);
> if (insn & (1 << 6)) {
> tmp = neon_load_reg(rd, 1);
> @@ -5521,8 +5545,9 @@ static int disas_neon_data_insn(CPUState * env,
> DisasContext *s, uint32_t insn)
> tcg_gen_movi_i32(tmp, 0);
> }
> tmp3 = neon_load_reg(rm, 1);
> - gen_helper_neon_tbl(tmp3, tmp3, tmp, tcg_const_i32(rn),
> - tcg_const_i32(n));
> + gen_helper_neon_tbl(tmp3, tmp3, tmp, tmp_rn, tmp_n);
> + dead_tmp(tmp_n);
> + dead_tmp(tmp_rn);
> neon_store_reg(rd, 0, tmp2);
> neon_store_reg(rd, 1, tmp3);
> dead_tmp(tmp);
> @@ -5660,7 +5685,7 @@ static int disas_coproc_insn(CPUState * env,
> DisasContext *s, uint32_t insn)
> }
>
>
> -/* Store a 64-bit value to a register pair. Clobbers val. */
> +/* Store a 64-bit value to a register pair. Marks val dead. */
> static void gen_storeq_reg(DisasContext *s, int rlow, int rhigh,
> TCGv_i64 val)
> {
> TCGv tmp;
> @@ -5671,6 +5696,7 @@ static void gen_storeq_reg(DisasContext *s, int
> rlow, int rhigh, TCGv_i64 val)
> tcg_gen_shri_i64(val, val, 32);
> tcg_gen_trunc_i64_i32(tmp, val);
> store_reg(s, rhigh, tmp);
> + tcg_temp_free_i64(val);
I'd prefer to see val freed after calls to gen_storeq_reg even
if it means a longer patch. The current situation with regards
to implicit temp new and free is already messy enough.
> }
>
> /* load a 32-bit value from a register and perform a 64-bit
> accumulate. */
> @@ -5685,6 +5711,7 @@ static void gen_addq_lo(DisasContext *s,
> TCGv_i64 val, int rlow)
> tcg_gen_extu_i32_i64(tmp, tmp2);
> dead_tmp(tmp2);
> tcg_gen_add_i64(val, val, tmp);
> + tcg_temp_free_i64(tmp);
> }
>
> /* load and add a 64-bit value from a register pair. */
> @@ -5702,6 +5729,7 @@ static void gen_addq(DisasContext *s, TCGv_i64
> val, int rlow, int rhigh)
> dead_tmp(tmpl);
> dead_tmp(tmph);
> tcg_gen_add_i64(val, val, tmp);
> + tcg_temp_free_i64(tmp);
> }
>
> /* Set N and Z flags from a 64-bit value. */
> @@ -5785,7 +5813,9 @@ static void disas_arm_insn(CPUState * env,
> DisasContext *s)
> addr = load_reg(s, 13);
> } else {
> addr = new_tmp();
> - gen_helper_get_r13_banked(addr, cpu_env, tcg_const_i32
> (op1));
> + TCGv tmp_op1 = tcg_const_i32(op1);
No declaration in code (see above).
> + gen_helper_get_r13_banked(addr, cpu_env, tmp_op1);
> + tcg_temp_free_i32(tmp_op1);
> }
> i = (insn >> 23) & 3;
> switch (i) {
> @@ -5816,7 +5846,9 @@ static void disas_arm_insn(CPUState * env,
> DisasContext *s)
> if (op1 == (env->uncached_cpsr & CPSR_M)) {
> store_reg(s, 13, addr);
> } else {
> - gen_helper_set_r13_banked(cpu_env, tcg_const_i32
> (op1), addr);
> + TCGv tmp_op1 = tcg_const_i32(op1);
Same as above.
> + gen_helper_set_r13_banked(cpu_env, tmp_op1, addr);
> + tcg_temp_free_i32(tmp_op1);
> dead_tmp(addr);
> }
> } else {
> @@ -6058,6 +6090,7 @@ static void disas_arm_insn(CPUState * env,
> DisasContext *s)
> tcg_gen_shri_i64(tmp64, tmp64, 16);
> tmp = new_tmp();
> tcg_gen_trunc_i64_i32(tmp, tmp64);
> + tcg_temp_free_i64(tmp64);
> if ((sh & 2) == 0) {
> tmp2 = load_reg(s, rn);
> gen_helper_add_setq(tmp, tmp, tmp2);
> @@ -6545,10 +6578,12 @@ static void disas_arm_insn(CPUState * env,
> DisasContext *s)
> }
> sh = (insn >> 16) & 0x1f;
> if (sh != 0) {
> + TCGv tmp_sh = tcg_const_i32(sh);
> if (insn & (1 << 22))
> - gen_helper_usat(tmp, tmp,
> tcg_const_i32(sh));
> + gen_helper_usat(tmp, tmp, tmp_sh);
> else
> - gen_helper_ssat(tmp, tmp,
> tcg_const_i32(sh));
> + gen_helper_ssat(tmp, tmp, tmp_sh);
> + tcg_temp_free_i32(tmp_sh);
> }
> store_reg(s, rd, tmp);
> } else if ((insn & 0x00300fe0) == 0x00200f20) {
> @@ -6556,10 +6591,12 @@ static void disas_arm_insn(CPUState * env,
> DisasContext *s)
> tmp = load_reg(s, rm);
> sh = (insn >> 16) & 0x1f;
> if (sh != 0) {
> + TCGv tmp_sh = tcg_const_i32(sh);
> if (insn & (1 << 22))
> - gen_helper_usat16(tmp, tmp,
> tcg_const_i32(sh));
> + gen_helper_usat16(tmp, tmp, tmp_sh);
> else
> - gen_helper_ssat16(tmp, tmp,
> tcg_const_i32(sh));
> + gen_helper_ssat16(tmp, tmp, tmp_sh);
> + tcg_temp_free_i32(tmp_sh);
> }
> store_reg(s, rd, tmp);
> } else if ((insn & 0x00700fe0) == 0x00000fa0) {
> @@ -6631,6 +6668,7 @@ static void disas_arm_insn(CPUState * env,
> DisasContext *s)
> tcg_gen_shri_i64(tmp64, tmp64, 32);
> tmp = new_tmp();
> tcg_gen_trunc_i64_i32(tmp, tmp64);
> + tcg_temp_free_i64(tmp64);
> if (rd != 15) {
> tmp2 = load_reg(s, rd);
> if (insn & (1 << 6)) {
> @@ -6831,7 +6869,9 @@ static void disas_arm_insn(CPUState * env,
> DisasContext *s)
> if (i == 15) {
> gen_bx(s, tmp);
> } else if (user) {
> - gen_helper_set_user_reg(tcg_const_i32
> (i), tmp);
> + TCGv tmp_i = tcg_const_i32(i);
> + gen_helper_set_user_reg(tmp_i, tmp);
> + tcg_temp_free_i32(tmp_i);
> dead_tmp(tmp);
> } else if (i == rn) {
> loaded_var = tmp;
> @@ -6848,7 +6888,9 @@ static void disas_arm_insn(CPUState * env,
> DisasContext *s)
> tcg_gen_movi_i32(tmp, val);
> } else if (user) {
> tmp = new_tmp();
> - gen_helper_get_user_reg(tmp,
> tcg_const_i32(i));
> + TCGv tmp_i = tcg_const_i32(i);
Same as above.
> + gen_helper_get_user_reg(tmp, tmp_i);
> + tcg_temp_free_i32(tmp_i);
> } else {
> tmp = load_reg(s, i);
> }
> @@ -7264,7 +7306,9 @@ static int disas_thumb2_insn(CPUState *env,
> DisasContext *s, uint16_t insn_hw1)
> addr = load_reg(s, 13);
> } else {
> addr = new_tmp();
> - gen_helper_get_r13_banked(addr, cpu_env,
> tcg_const_i32(op));
> + TCGv tmp_op = tcg_const_i32(op);
Same as above.
> + gen_helper_get_r13_banked(addr, cpu_env,
> tmp_op);
> + tcg_temp_free_i32(tmp_op);
> }
> if ((insn & (1 << 24)) == 0) {
> tcg_gen_addi_i32(addr, addr, -8);
> @@ -7284,8 +7328,9 @@ static int disas_thumb2_insn(CPUState *env,
> DisasContext *s, uint16_t insn_hw1)
> if (op == (env->uncached_cpsr & CPSR_M)) {
> store_reg(s, 13, addr);
> } else {
> - gen_helper_set_r13_banked(cpu_env,
> - tcg_const_i32(op), addr);
> + TCGv tmp_op = tcg_const_i32(op);
> + gen_helper_set_r13_banked(cpu_env,
> tmp_op, addr);
> + tcg_temp_free_i32(tmp_op);
> }
> } else {
> dead_tmp(addr);
> @@ -7515,6 +7560,7 @@ static int disas_thumb2_insn(CPUState *env,
> DisasContext *s, uint16_t insn_hw1)
> tcg_gen_shri_i64(tmp64, tmp64, 16);
> tmp = new_tmp();
> tcg_gen_trunc_i64_i32(tmp, tmp64);
> + tcg_temp_free_i64(tmp64);
> if (rs != 15)
> {
> tmp2 = load_reg(s, rs);
> @@ -7673,6 +7719,7 @@ static int disas_thumb2_insn(CPUState *env,
> DisasContext *s, uint16_t insn_hw1)
> tmp = load_reg(s, rn);
> addr = tcg_const_i32(insn & 0xff);
> gen_helper_v7m_msr(cpu_env, addr, tmp);
> + tcg_temp_free_i32(addr);
Shouldn't tmp be freed too?
> gen_lookup_tb(s);
> break;
> }
> @@ -7742,6 +7789,7 @@ static int disas_thumb2_insn(CPUState *env,
> DisasContext *s, uint16_t insn_hw1)
> if (IS_M(env)) {
> addr = tcg_const_i32(insn & 0xff);
> gen_helper_v7m_mrs(tmp, cpu_env, addr);
> + tcg_temp_free_i32(addr);
> } else {
> gen_helper_cpsr_read(tmp);
> }
> @@ -7842,6 +7890,7 @@ static int disas_thumb2_insn(CPUState *env,
> DisasContext *s, uint16_t insn_hw1)
> else
> gen_helper_ssat(tmp, tmp, tmp2);
> }
> + tcg_temp_free_i32(tmp2);
> break;
> }
> store_reg(s, rd, tmp);
> @@ -8614,12 +8663,15 @@ static void disas_thumb_insn(CPUState *env,
> DisasContext *s)
> if (insn & 1) {
> addr = tcg_const_i32(16);
> gen_helper_v7m_msr(cpu_env, addr, tmp);
> + tcg_temp_free_i32(addr);
> }
> /* FAULTMASK */
> if (insn & 2) {
> addr = tcg_const_i32(17);
> gen_helper_v7m_msr(cpu_env, addr, tmp);
> + tcg_temp_free_i32(addr);
> }
> + tcg_temp_free_i32(tmp);
> gen_lookup_tb(s);
> } else {
> if (insn & (1 << 4))
>