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

[Andreas Färber]

Am 13.12.2011 17:26, schrieb Paul Brook:
>>>>> 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.
>>>
>>> Sure it does
>>
>> No, start by reading the git summary. These four patches don't touch
>> target-* at all.
>>
>> This is intentionally NOT some Coccinelle script running wild doing
>> refactorings. That's what I would call "blindly".
> 
> IIUC these four patches do precicely nothing.

No, only patch 4/4 does nothing if not enabled.

Patches 1-3 are actively used by all targets.

And specifically, patch 2/4 is highly likely to break if kept
out-of-tree when someone adds tcg_gen_somethingclever_tl(). Blue (cc'ed)
has been asking me to convert macros into inline functions all over
OpenBIOS with no practical runtime change, so the overall change does
not seem Wrong(tm), just a matter of taste and (here) allowing
additional features. Is it the MAKE+GET combo that disturbs you there?
If so, do you have a better suggestion? TGCV_Ixx_TO_TL(foo) that can
compile to (foo) by default?

>>> Ther are three ways to resolve a mismatch:
>>> - Change everything to TCGv_i32.
>>> - Change everything to TCGv.
>>> - Add an explicit extension/truncation (compiles to no-op on 32-bit
>>> targets).
>>>
>>> There's no way of the developer of a 32-bit architecture to know
>>
>> Again, that's where we disagree:
>>
>> The whole point of TCGv and tl is to have variable-sized operations
>> scaling with target_long.
> 
> So you're claiming that a 32-bit only target can somehow distinguish between 
> a 
> 32-bit value, and a value that is architecturally defined to always be 
> 32-bit.  
> I don't believe any useful determination can be made in this case.
> 
> For targets with different target_ulong variants simply building those 
> variants with the existing checks will already highlight any mismatches.
> 
> AFAICS the best you can do is say that 32-bit only targets should never use 
> TCGv. While that might be a nice idea in theory, the opposite is true for the 
> current code base.  This is because the original TCG implementation did not 
> do 
> any static typing, i.e. everything was TCGv.  It was only later that I gave 
> TCG variables a static size.  I see no practical harm from leaving that 
> as-is.  
> Introducing a substantial amount of churn and extra complication to achieve a 
> purely theoretical goal is a bad idea.
> 
>> I have already given four examples to Peter, that you quoted previously.
> 
> The examples I quoted where where TCGv and TCGv_i32 were mixed, but I don't 
> see how any of those could possibly cause incorrect behovior.  If the two 
> were 
> ever different then this would already fail to compile.
> 
> So I'll ask again: Please give a worked example of a programming error that 
> is 
> caught by your new restrictions. Feel free to use hypothetical code and/or 
> targets.

We don't seem to be getting anywhere with this discussion...

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

Once again, there are targets - RL78, 78K0, 78K0S, 78K0R, STM8, who
knows how many others - that have registers of width < 32 that *never*
scale with physical address width. This I know for sure as the
developer. Thus, using tl functions that will scale to 64 bits is
undoubtedly Wrong(tm) and I strive to get my new code Right(tm).

For example, the TARGET_78K0 / TARGET_RL78 Processor Status Word is 8
bits, always (same for the ST7 / STM8 Accumulator and Condition Code
Register). Therefore i32 as our lowest supported temporary size. The PSW
contains a two-bit GPR bank number, still i32 once extracted. I use it
to calculate a target address for tcg_gen_qemu_{ld,st}*(), which must be
TCGv, so may scale to 64 bits. So I must be careful about TCGv and
TCGv_i32 a) for the signature of my static helper functions, b) for the
per-instruction temporary variables and c) for the TCG functions called
with those variables.

Whether or not you understand this forward-looking desire of mine, for
the current (not historic!) code base I would theoretically like some
warning mechanism like:

#if TCGv debug feature desired for this target
#if TCGv argument was passed a TCGv_i32 variable then
#warning Ouch, expected TCGv but got a TCGv_i32.
#endif
#endif

In practice, I don't see any other way than making TCGv and TCGv_i32
incompatible with each other by using different structs with different
member names, like TCGv_i32/i64 do, which - yes, here unnecessarily -
then leads to a compilation error even with --disable-werror.

Neither warning nor error should be enabled on the default, optimized
build IMO (just like --enable-debug-tcg isn't, and that we have in-tree
despite occasional --enable-debug-tcg build breakages that Stefan W. and
others have volunteered to fix from time to time).

Me, I don't see any reason to activate this for existing m68k, so we
could easily have this special debug #define only enabled for select
targets (mine) by configure iff --enable-debug-tcg is passed.
No existing target would break that way and I of course intend to
maintain this feature.

Now, do you have a better solution how to do this or not?
If yes, please share.
If not, do you have a suggestion how to change my patches so that
prerequisite parts thereof become acceptable for you or any other
maintainer to apply?

Anything else - repeating that this doesn't make a difference for your
favorite targets, asking me for examples over and over - doesn't help.

Thanks in advance,

Andreas