> > There is no testcase for PR24398 nor the OP reporting the problem was > actually solved. Martell?
I'm just re-looking through it now. X86TargetInfo sets LongDoubleFormat = &llvm::APFloat::x87DoubleExtended; X86_64TargetInfo then sets LongDoubleWidth = LongDoubleAlign = 128; X86_32TargetInfo then sets LongDoubleWidth = 96; LongDoubleAlign = 32; >From this I can see that the patch I committed actually doesn't change anything but only breaks mingw x86. I can only see these values changed in Microsoft*TargetInfo classes which is not a parent of MINGW When I submitted the patch I just wanted to explicitly set the values in MinGWX86_64TargetInfo to ensure it was in fact that. It seemed as though it was not inheriting the value from the root parent class somehow. I was told on irc that it did fix the bug which is even stranger. I'm actually at a bit of a loss as to what the proper fix to this is then. Apologies for breaking mingw i686 long double. I will do up a test case and try to find the actual cause of the long double bug and reopen the issue. On Thu, Aug 20, 2015 at 3:09 PM, Yaron Keren <yaron.ke...@gmail.com> wrote: > Hi, I've just done this exactly this in r245618 (32 bit) and r245620 (64 > bits). > > mingw i686 long double values were correct before r245084 and wrong after > it. > mingw x86_64 long double values were not modified at all by r245084 for > the reason you stated, so I agree and do not see how this non-change can > solve anything. There is no testcase for PR24398 nor the OP reporting the > problem was actually solved. Martell? > > About PR24398, long double support was in clang long ago and both code > examples compile and run correctly with current svn (without r245084) and gcc > version 5.1.0 (i686-posix-dwarf-rev0, Built by MinGW-W64 project). > It's not x86_64 but as said r245084 didn't actually modify x86_64 > configuration. > > > > > 2015-08-21 0:52 GMT+03:00 Richard Smith <rich...@metafoo.co.uk>: > >> OK, so here's the problem: >> >> The right way to fix this seems to be to delete the assignments to >> LongDouble* from the MinGWX86_32TargetInfo constructor; the >> X86_32TargetInfo and X86TargetInfo base classes already set them to the >> right values. Likewise we can delete the assignments to LongDouble* from >> the MinGWX86_64TargetInfo constructor for the same reason. >> >> But... that completely reverts Martell's r245084, which apparently fixed >> PR24398. So you two need to figure out what the actual problem is here, and >> what the right fix is. r245084 didn't provide any test cases, and had no >> apparent effect other than to break long double for mingw32; did it really >> fix PR24398 (and if so, how)? >> >> >> On Thu, Aug 20, 2015 at 2:27 PM, Yaron Keren <yaron.ke...@gmail.com> >> wrote: >> >>> OK, based on testing, mingw i686 aligns long doubles to 4 bytes: >>> >>> sh-4.3$ cat < a.cpp >>> #include <iostream> >>> int main() { >>> struct { >>> char c[1]; >>> long double d; >>> } s; >>> std::cout<<&s.c<<std::endl; >>> std::cout<<&s.d<<std::endl; >>> } >>> sh-4.3$ g++ a.cpp&&./a.exe >>> 0x28fea0 >>> 0x28fea4 >>> >>> I'll fix that. >>> >>> >>> 2015-08-21 0:13 GMT+03:00 Richard Smith <rich...@metafoo.co.uk>: >>> >>>> On Wed, Aug 19, 2015 at 11:42 AM, Yaron Keren <yaron.ke...@gmail.com> >>>> wrote: >>>> >>>>> Yes, it looks like a legacy issue. Documentation says so: >>>>> >>>>> *https://gcc.gnu.org/onlinedocs/gcc-4.8.2/gcc/i386-and-x86-64-Options.html >>>>> <https://gcc.gnu.org/onlinedocs/gcc-4.8.2/gcc/i386-and-x86-64-Options.html>* >>>>> >>>>> -m96bit-long-double-m128bit-long-doubleThese switches control the >>>>> size of long double type. The i386 application binary interface >>>>> specifies the size to be 96 bits, so -m96bit-long-double is the >>>>> default in 32-bit mode. >>>>> >>>>> Modern architectures (Pentium and newer) prefer long double to be >>>>> aligned to an 8- or 16-byte boundary. In arrays or structures conforming >>>>> to >>>>> the ABI, this is not possible. So specifying -m128bit-long-double >>>>> aligns long double to a 16-byte boundary by padding the long double with >>>>> an additional 32-bit zero. >>>>> >>>>> In the x86-64 compiler, -m128bit-long-double is the default choice as >>>>> its ABI specifies that long double is aligned on 16-byte boundary. >>>>> >>>>> Notice that neither of these options enable any extra precision over >>>>> the x87 standard of 80 bits for a long double. >>>>> >>>>> *Warning:* if you override the default value for your target ABI, >>>>> this changes the size of structures and arrays containing long double >>>>> variables, >>>>> as well as modifying the function calling convention for functions taking >>>>> long >>>>> double. Hence they are not binary-compatible with code compiled >>>>> without that switch. >>>>> >>>>> And practical testing agrees: >>>>> >>>>> sh-4.3$ cat < a.cpp >>>>> #include <iostream> >>>>> int main() { >>>>> long double a; >>>>> std::cout<<sizeof(a)<<std::endl; >>>>> } >>>>> sh-4.3$ g++ -v >>>>> Using built-in specs. >>>>> COLLECT_GCC=C:\mingw32\bin\g++.exe >>>>> >>>>> COLLECT_LTO_WRAPPER=C:/mingw32/bin/../libexec/gcc/i686-w64-mingw32/5.1.0/lto-wrapper.exe >>>>> Target: i686-w64-mingw32 >>>>> Configured with: ../../../src/gcc-5.1.0/configure >>>>> --host=i686-w64-mingw32 --build=i686-w64-mingw32 --target=i686-w64-mingw32 >>>>> --prefix=/mingw32 >>>>> --with-sysroot=/c/mingw510/i686-510-posix-dwarf-rt_v4-rev0/mingw32 >>>>> --with-gxx-include-dir=/mingw32/i686-w64-mingw32/include/c++ >>>>> --enable-shared --enable-static --disable-multilib >>>>> --enable-languages=c,c++,fortran,objc,obj-c++,lto >>>>> --enable-libstdcxx-time=yes --enable-threads=posix --enable-libgomp >>>>> --enable-libatomic --enable-lto --enable-graphite >>>>> --enable-checking=release >>>>> --enable-fully-dynamic-string --enable-version-specific-runtime-libs >>>>> --disable-sjlj-exceptions --with-dwarf2 --disable-isl-version-check >>>>> --disable-libstdcxx-pch --disable-libstdcxx-debug --enable-bootstrap >>>>> --disable-rpath --disable-win32-registry --disable-nls --disable-werror >>>>> --disable-symvers --with-gnu-as --with-gnu-ld --with-arch=i686 >>>>> --with-tune=generic --with-libiconv --with-system-zlib >>>>> --with-gmp=/c/mingw510/prerequisites/i686-w64-mingw32-static >>>>> --with-mpfr=/c/mingw510/prerequisites/i686-w64-mingw32-static >>>>> --with-mpc=/c/mingw510/prerequisites/i686-w64-mingw32-static >>>>> --with-isl=/c/mingw510/prerequisites/i686-w64-mingw32-static >>>>> --with-pkgversion='i686-posix-dwarf-rev0, Built by MinGW-W64 project' >>>>> --with-bugurl=http://sourceforge.net/projects/mingw-w64 CFLAGS='-O2 >>>>> -pipe -I/c/mingw510/i686-510-posix-dwarf-rt_v4-rev0/mingw32/opt/include >>>>> -I/c/mingw510/prerequisites/i686-zlib-static/include >>>>> -I/c/mingw510/prerequisites/i686-w64-mingw32-static/include' CXXFLAGS='-O2 >>>>> -pipe -I/c/mingw510/i686-510-posix-dwarf-rt_v4-rev0/mingw32/opt/include >>>>> -I/c/mingw510/prerequisites/i686-zlib-static/include >>>>> -I/c/mingw510/prerequisites/i686-w64-mingw32-static/include' CPPFLAGS= >>>>> LDFLAGS='-pipe >>>>> -L/c/mingw510/i686-510-posix-dwarf-rt_v4-rev0/mingw32/opt/lib >>>>> -L/c/mingw510/prerequisites/i686-zlib-static/lib >>>>> -L/c/mingw510/prerequisites/i686-w64-mingw32-static/lib >>>>> -Wl,--large-address-aware' >>>>> Thread model: posix >>>>> gcc version 5.1.0 (i686-posix-dwarf-rev0, Built by MinGW-W64 project) >>>>> >>>>> sh-4.3$ g++ a.cpp >>>>> sh-4.3$ ./a.exe >>>>> 12 >>>>> >>>>> Without the patch clang outputs 16 and seg faults on a boost::math >>>>> example. >>>>> >>>> >>>> None of that answers my question. Our default GCC-compatible behavior >>>> for x86_32 long double is to give it 4-byte alignment, 12-byte size, and >>>> this matches the behavior I observe with GCC. Does MinGW /really/ deviate >>>> from this and give long double a 16-byte alignment? >>>> >>>> >>>>> 2015-08-19 21:29 GMT+03:00 Richard Smith <rich...@metafoo.co.uk>: >>>>> >>>>>> On Wed, Aug 19, 2015 at 10:02 AM, Yaron Keren via cfe-commits < >>>>>> cfe-commits@lists.llvm.org> wrote: >>>>>> >>>>>>> Author: yrnkrn >>>>>>> Date: Wed Aug 19 12:02:32 2015 >>>>>>> New Revision: 245459 >>>>>>> >>>>>>> URL: http://llvm.org/viewvc/llvm-project?rev=245459&view=rev >>>>>>> Log: >>>>>>> According to i686 ABI, long double size on x86 is 12 bytes not 16 >>>>>>> bytes. >>>>>>> See >>>>>>> >>>>>>> https://gcc.gnu.org/onlinedocs/gcc-3.2/gcc/i386-and-x86-64-Options.html >>>>>>> >>>>>>> >>>>>>> Added: >>>>>>> cfe/trunk/test/CodeGen/mingw-long-double-size.c >>>>>>> Modified: >>>>>>> cfe/trunk/lib/Basic/Targets.cpp >>>>>>> >>>>>>> Modified: cfe/trunk/lib/Basic/Targets.cpp >>>>>>> URL: >>>>>>> http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Basic/Targets.cpp?rev=245459&r1=245458&r2=245459&view=diff >>>>>>> >>>>>>> ============================================================================== >>>>>>> --- cfe/trunk/lib/Basic/Targets.cpp (original) >>>>>>> +++ cfe/trunk/lib/Basic/Targets.cpp Wed Aug 19 12:02:32 2015 >>>>>>> @@ -3785,7 +3785,8 @@ class MinGWX86_32TargetInfo : public Win >>>>>>> public: >>>>>>> MinGWX86_32TargetInfo(const llvm::Triple &Triple) >>>>>>> : WindowsX86_32TargetInfo(Triple) { >>>>>>> - LongDoubleWidth = LongDoubleAlign = 128; >>>>>>> + LongDoubleWidth = 96; >>>>>>> + LongDoubleAlign = 128; >>>>>>> >>>>>> >>>>>> Is this really correct? It's deeply suspicious that the size is not a >>>>>> multiple of the alignment. >>>>>> >>>>>> >>>>>>> LongDoubleFormat = &llvm::APFloat::x87DoubleExtended; >>>>>>> } >>>>>>> void getTargetDefines(const LangOptions &Opts, >>>>>>> >>>>>>> Added: cfe/trunk/test/CodeGen/mingw-long-double-size.c >>>>>>> URL: >>>>>>> http://llvm.org/viewvc/llvm-project/cfe/trunk/test/CodeGen/mingw-long-double-size.c?rev=245459&view=auto >>>>>>> >>>>>>> ============================================================================== >>>>>>> --- cfe/trunk/test/CodeGen/mingw-long-double-size.c (added) >>>>>>> +++ cfe/trunk/test/CodeGen/mingw-long-double-size.c Wed Aug 19 >>>>>>> 12:02:32 2015 >>>>>>> @@ -0,0 +1,5 @@ >>>>>>> +// RUN: %clang_cc1 -triple i686-pc-windows-gnu -S %s -o - | >>>>>>> FileCheck %s -check-prefix=CHECK_I686 >>>>>>> +// CHECK_I686: lda,12 >>>>>>> +// RUN: %clang_cc1 -triple x86_64-pc-windows-gnu -S %s -o - | >>>>>>> FileCheck %s -check-prefix=CHECK_X86_64 >>>>>>> +// CHECK_X86_64: lda,16 >>>>>>> +long double lda; >>>>>>> >>>>>>> >>>>>>> _______________________________________________ >>>>>>> cfe-commits mailing list >>>>>>> cfe-commits@lists.llvm.org >>>>>>> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits >>>>>>> >>>>>> >>>>>> >>>> >>
_______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits