Re: [Qemu-devel] [PATCH] target-arm: cleanup internal resource leaks

[Juha.Riihimaki]

Thanks for your feedback. I'll iron out the issues you mentioned,  
please look at my further inlined comments below.


Cheers,
Juha

On Oct 21, 2009, at 22:20, ext Laurent Desnogues wrote:

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

You're right. However, looking at the surrounding code a bit closer I  
began to wonder if it works correctly at all since tmp2 is used as a  
shift value if size < 2 but it is also used to store the result of the  
first pass. Therefore the result of the first pass is used as a shift  
value during second pass... perhaps not correct? Wouldn't it make more  
sense to store the result of the first pass in the lower half of the  
destination neon register directly during the first pass? I see no  
point in keeping it in a temporary variable and only store both halves  
of the destination neon register during the second pass. Especially  
since there is no memory access involved, everything is done in  
registers. What do you think?

>> @@ -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 :-)

Ah, sorry. I forgot to clean up my own cleanups ;) I'll fix this and  
the other instances you noticed.

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

Certainly. I guess the whole file could be patched to remove all such  
implicit allocations and deallocations and make everything explicit;  
that should clear things up and make it more readable. I could do that  
as well. I implemented this change to make it behave like the other  
store_reg functions in the target-arm/translate.c, I thought having  
methods with similar functionality behaving differently in regards to  
the arguments would make things even messier.

>> @@ -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?

Correct. I'll add that.