Re: [hatari-devel] Bug in FPU code

[Eero Tamminen <oak@xxxxxxxxxxxxxx>]

Hi,

On tiistai 06 maaliskuu 2012, Andreas Grabher wrote:
> 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.

At least they're inefficient... converting 32-bit longs in Atari FPU double 
to a string and then using sscanf() to convert that to host long
double, and vice verse, using printf() and assumption about libc scientific
notation output format.

On quick look I didn't see anything similarly obviously wrong with them
as with the function your previous patch fixed.

You may want to debug those functions by printing out the ASCII
format values to see whether they match the value the packed decimal
is supposed to have.


	- Eero

> 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