Re: [Qemu-devel] [PATCH 0/4] tcg: Add debug facilities for TCGv

[Andreas Färber]

Am 12.12.2011 00:28, schrieb Paul Brook:
>>> What mismatches does this catch that the existing debug code doesn't?
>>
>> Cf. patch 4/4:
>>
>> TCGv tmp = tcg_temp_new_i32();
>> tcg_temp_free_i32(tmp);
>>
>> TCGv_i32 tmp2 = tcg_temp_new();
>> tcg_temp_free(tmp2);
> 
> Why is this a problem?  If TARGET_LONG_BITS==32 then tcg_temp_free and 
> tcg_temp_free_i32 are synonyms, and everything is happy.
> 
> If TARGET_LONG_BITS==64 then we already flag this as an error.
> 
>> Try compiling --target-list=arm-softmmu --enable-debug-tcg with my
>> series and DEBUG_TCGV_TL uncommented, and you'll see for yourself.
>> There's too many to mention and for me to actually fix. You'll have to
>> deal with it for ARMv8 at some point and this series hopefully helps.
> 
> That's exactly why I think this patch is a bad idea.
> 
> If a target always has TARGET_LONG_BITS==32 then it doesn't matter if we mix 
> TCGv and TCGv_i32.

To me and my perfectionism it does matter.

This is not about fixing some user-visible runtime bug, it's about
making the developer (me) aware of unintended type mixups.

> Trying to make a 32-bit target "64-bit safe" without actually implementing 
> the 
> 64-bit target is a complete waste of time.

That's where we disagree. I rather do things right from the start than
leaving the cleanup work to someone else later on.

> You've almost no chance of getting
> it right.  In some cases the correct answer will be to use 32-bit arithmetic, 
> then sign/zero extend the result. In other cases the correct answer will be 
> to 
> perform word size arithmetic.  Blindly picking one just makes the bugs harder 
> to find later.

This series picks nothing blindly. It provides an optional facility for
the developer to be made aware of uses of tl where i32/i64 was intended
(or vice versa), nothing changes at runtime.

Whether and in which way the developer addresses the issues shown that
way is up to the developer, and as any line added to a new target must
be decided on a case-by-case basis.

For me the most annoying issue was that tcg_gen_qemu_{ld,st}* needs TCGv.

> If you're trying to add support for targets where the primary word size is 
> neither 32 nor 64 then that's a completely different problem, and probably 
> not 
> one that's worth solving.  In practice your port is going to end up using 64-
> bit arithmetic and explicitly compensating for the excess precision where 
> necessary.

That's a different issue and not being addressed here.

My point was that I have an inheritance hierarchy where (as opposed to,
e.g., ppc and ppc64) both end up as TARGET_LONG_BITS==32, so I do not
get a check for free by compiling both, and I do not want to introduce
an artificial TARGET_LONG_BITS==64 architecture just to check that my
tl==i32 target code is free of typos.
Same issue if you pick only --target-list=<some>-softmmu BTW.

Just like with softfloat [u]int16 etc. types it's just too easy to
forget _t somewhere (here: _i32) and to end up with a type you don't want.

If you have a better proposal how to introduce the checks I want, please
let us hear it.

If no one has, I don't see how this series would hurt (1-3 refactoring,
4 not enabled by default) while providing useful new debug facilities to
developers of new targets.

Andreas