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
