open-watcom / open-watcom-v2

Open Watcom V2.0 - Source code repository, Wiki, Latest Binary build, Archived builds including all installers for download.
Other
988 stars 163 forks source link

WCC 80-bit long double support -fld is not supported by the code generator #1200

Open joncampbell123 opened 10 months ago

joncampbell123 commented 10 months ago

If the -fld switch is used to enable the full 80-bit long double format, the compiler will either crash with an internal error or produce buggy code that causes crashes if any function passes a long double as a parameter to a function call.

Perhaps this issue has been lurking for awhile considering that -fld is not listed by the compiler when printing out all the options, and that the compiler by default treats long double as an alias to double (as Microsoft C++ does) meaning both data types are the 64-bit double floating point precision data type by default.

The problem seems to stem from code within bld/cg that is called assuming that everything it is asked to resolve or handle is 8 bytes or less when deciding which registers or stack arrangement to do. 10 bytes are needed for 80-bit long double.

joncampbell123 commented 10 months ago

More details while discussing this request: https://github.com/open-watcom/open-watcom-v2/pull/1198

Test case: https://github.com/joncampbell123/doslib/blob/master/hw/dos/testprna.c

joncampbell123 commented 10 months ago

The design of the x86 CG seems to indicate that it was never intended to handle the FL type, and there is a lot written around handling "long double" as 64-bit double. This will require a lot of steady hacking.

Also I noticed that sizeof(long double) == 16 when -fld is given. 6 bytes of padding.

joncampbell123 commented 10 months ago

Latest test: The same internal error occurs if I try to pass the address of a long double constant to a function call, i.e. declare a long double constant "a" (which doesn't trigger the error) and then pass &a to a function with a parameter of type "long double *".

joncampbell123 commented 10 months ago

I can't even declare the constant and try to pass it off as typecast (unsigned char*)(&lv) :laughing: oh come on now!

static void TEST(const unsigned char *x,size_t sz) { ... }

define TEST(x) {\

    const double dv = x;\
    const long double lv = x ## l;\
    TEST((const unsigned char*)(&dv),sizeof(dv)); \
    TEST((const unsigned char*)(&lv),sizeof(lv)); }

You can declare, but not touch or even look at it! :rofl:

joncampbell123 commented 10 months ago

Putting the two into a struct makes it worse because taking the address of the struct triggers the error too.

I finally found the one way that OW allows me with -fld to declare a long double and obtain it's memory address though.

Printing out the bytes in memory, it looks like sizeof(long double) == 10 as expected but OW is really just formatting it as 64-bit double with an extra 16 bits of zero.

https://github.com/joncampbell123/doslib/blob/master/hw/dos/testprnb.c

image

Having attempted to modify OW last night to properly support long double internally (and failing), I can tell that the cause is the function in cfloat that converts to 64-bit double even for long double, and that the internal structs meant to hold constants are designed only for up to 64 bits. So to support 80-bit long double it helps to first produce a constant value correctly.

joncampbell123 commented 10 months ago

Compare that to the same program compiled with GCC:

bash-5.0# linux-host/testprnb 
0x0x402030: 3ff0000000000000
0x0x402020: 0000000000003fff8000000000000000

GCC treats long double as 16 bytes long probably for alignment reasons.

joncampbell123 commented 10 months ago

It might help if Open Watcom were to begin to compile itself with -fld if 80-bit long double support is to be properly fully supported. At least the parts that need it.

rdos314 commented 10 months ago

I agree that OW should support long double. I've had issues with this before when doing calculations that would have improved with long double. Also, long double is the default format of the FPU, and when using double or float the precision of calculations are essentially reduced.

joncampbell123 commented 10 months ago

@jmalak How easy would it be to add a built-in compiler typedef that is always the 80-bit long double type regardless of the -fld switch?

I suggest a typedef name like "__long_double" so support can be added at specific points without adding -fld everywhere.

I'm willing to submit pull requests over time to help fix up long double given available free time.

I also suggest that since -fld changes sizeof(long double) that it should set a preprocessor macro. I suggest __LONGDOUBLE_\, so headers can make the appropriate adjustments as required.

joncampbell123 commented 10 months ago

I agree that OW should support long double. I've had issues with this before when doing calculations that would have improved with long double. Also, long double is the default format of the FPU, and when using double or float the precision of calculations are essentially reduced.

Indeed. It also affects emulators like DOSBox and DOSBox-X where Windows builds have minor FPU precision errors because Microsoft long ago (during the Win16 to Win32 transition) decided to make "long double" an alias to 64-bit "double" as OW does by default now.

jmalak commented 10 months ago

I don't agree that CG is not designed to handled 80-bit FP. Most of code is there but some parts are not finished. CRTL contains nearly 100 % support for 80-bit FP. I cannot do estimation of effort I am not expert on FPU and math. But I tried to prepare CG code to this work on 80-bit FP by macro FP80BIT_DEVELOPMENT in _cgstd.h that is visible parts which need some work and can be simply switch on/off. C and C++ front ends should be also ready for 80-bit FP. Some work on C and C++ run-time libraries will be necessary, but I think it is smallest part. Main work is finish CG.

jmalak commented 10 months ago

Note to long double type size and alignment. long double type has size 10 bytes but on 32-bit stack has size 12 bytes and memory allocation should be 16 bytes. Alignment should be 16 bytes. For 16-bit size is 10 bytes too and stack size is 10 bytes. allocation and alignment should be 16 bytes too.

rdos314 commented 10 months ago

It's my impression that function calls should use floating point stack registers as the first priority, and only pass them by pointers when necessary. I'm pretty sure "pragma aux" supports this.

jmalak commented 10 months ago

Yes, 80-bit FP is based on FP stack registers and input/output memory variables passed by pointers not by value. Front-end #pragma aux fully support 80-bit FP.

joncampbell123 commented 10 months ago

Note to long double type size and alignment. long double type has size 10 bytes but on 32-bit stack has size 12 bytes and memory allocation should be 16 bytes. Alignment should be 16 bytes. For 16-bit size is 10 bytes too and stack size is 10 bytes. allocation and alignment should be 16 bytes too.

How's this? (as a diff)

diff --git a/bld/cc/h/target32.h b/bld/cc/h/target32.h index dc90fed8ba..35b8f50558 100644 --- a/bld/cc/h/target32.h +++ b/bld/cc/h/target32.h @@ -47,13 +47,13 @@

define TARGET_UINT 4

define TARGET_FLOAT 4

define TARGET_DOUBLE 8

-#define TARGET_LDOUBLE 10 +#define TARGET_LDOUBLE 16 / memory allocation /

define TARGET_FCOMPLEX 8

define TARGET_DCOMPLEX 16

-#define TARGET_LDCOMPLEX 20 +#define TARGET_LDCOMPLEX 32 / memory allocation /

define TARGET_FIMAGINARY 4

define TARGET_DIMAGINARY 8

-#define TARGET_LDIMAGINARY 10 +#define TARGET_LDIMAGINARY 16 / memory allocation /

define TARGET_BOOL 1

define TARGET_WCHAR 2

define TARGET_BITFIELD 4

Although it only appears to affect sizeof() while memory allocation for long double is still always 8 bytes in both 16-bit and 32-bit compiles of the test program.

joncampbell123 commented 10 months ago

Notice that despite -fld and the change to make long double 16 bytes from wcc386, the two variables are 8 bytes apart and obviously overlap one another.

image

joncampbell123 commented 10 months ago

It seems even with -fld, when declaring a static const long double constant with the 'l' suffix, the constant that works it's way down to EmitDQuad() in cc/c/cgendata.c gets only a QDT_DOUBLE type and therefore only counts 8 bytes even though it should be a QDT_LONG_DOUBLE and 10/16 for 16/32 bit compilation with -fld.

joncampbell123 commented 10 months ago

Take your time, I'll move on to other things in the meantime.

jmalak commented 10 months ago

All this need to switch to 80-bit LD as soon as CG will be ready. First must be fixed CG and next FE.