Re: [hatari-devel] Bug in FPU code

[Andreas Grabher <andreas.grabher@xxxxxxxxxxxx>]

I did more research on the FPU code today, trying to fix 68030/68882.
I found the problem: it's one or both of the functions "to_pack" and "from_pack". Replacing them with the fixed to_exten and from_exten makes the FPU pass the test (obviously the test does not do any calculations with the values, just copying them forth and back to/from the FPU register).
This is of course no valid patch, as it will break handling of packed format values (break the broken handling ...).

Maybe someone can tell what's wrong with these functions? They look a little "strange" to me, but for now i can't make any diagnosis, as i do not yet understand the packed decimal format of the 68882.


Am 06.03.2012 um 18:53 schrieb Andreas Grabher:

> I'm happy my patch is useful!
> About the endianness and long double issue:
> I think this code will work on all little endian systems as long as the size of long double is >= 10 bytes. Bigger size should not be a problem (for example on my system long double is defined 16 bytes long as reported by printf("Size: %lu\n", sizeof(long double);
> 
> For big endian systems we would need a slightly different function and also take into account the actual size of long double. Please correct me if i'm wrong.
> 
> I think a bigger problem than endianness will be systems that define long double as < 10 bytes.
> 
> 
> Am 05.03.2012 um 22:44 schrieb Thomas Huth:
> 
>> Am Mon, 05 Mar 2012 22:05:34 +0100
>> schrieb Laurent Sallafranque <laurent.sallafranque@xxxxxxx>:
>> 
>>> Hi,
>>> 
>>> Eero, Thomas, Nicolas, I've tested the patch with the esquimau demo,
>>> but I haven't seen any change. (Eskimau does a lot of FPU).
>>> 
>>> As I'm not a specialist of #if USE_LONG_DOUBLE defines and the code 
>>> after, I prefer asking you your advice on this patch before uploading 
>>> anything.
>>> 
>>> Is the fix OK for any case of #if USE_LONG_DOUBLE ? (on every
>>> computer, ... little / big endian, ...) ?
>>> 
>>> At least, the patch doesn't seem to break anything, but I've done
>>> only one test.
>> 
>> Well, by closely looking at the source code, you can see that the
>> original code is obviously wrong:
>> 
>>  ... long double src ...
>>  register uae_u16 *finalword = (uae_u16 *)(&src + 8);
>> 
>> Here they "try" to access the memory at location &src + 8 ... which
>> should obviously be 8 _bytes_ after &src, but since &src is a "long
>> double" pointer, they rather access the memory at 8 * 8 = 64 bytes
>> after &src.
>> 
>> So yes, please apply the patch, IMHO Andreas did a very good job here
>> by spotting this bug. As an alternative, you could also cast &src to
>> a "uint8_t *" before adding the "8", that should fix this issue, too.
>> 
>> And don't worry about breaking the endianess, the original code here was
>> not endianess safe, either, so the endianess issues should maybe be
>> fixed with another patch later.
>> 
>> Thomas
>> 
>> 
>> 
>> 
> 
> 
>