r245459 - According to i686 ABI, long double size on x86 is 12 bytes not 16 bytes.

Sat Sep 19 05:24:03 PDT 2015

For reference It was raised again here also
https://github.com/Alexpux/MINGW-packages/issues/722
<https://github.com/Alexpux/MINGW-packages/issues/722#issuecomment-141642907>

"that is extended from X86TargetInfo
Which sets LongDoubleFormat = &llvm::APFloat::x87DoubleExtended;"

On Saturday, September 19, 2015, Yaron Keren <yaron.keren at gmail.com
<javascript:_e(%7B%7D,'cvml','yaron.keren at gmail.com');>> wrote:

> Thanks, I have replied there.
>
> 2015-09-19 13:33 GMT+03:00 Hal Finkel <hfinkel at anl.gov>:
>
>> FYI: https://llvm.org/bugs/show_bug.cgi?id=24398 was just reopened
>> pointing to a lack of resolution here.
>>
>>  -Hal
>>
>> ----- Original Message -----
>> > From: "Yaron Keren via cfe-commits" <cfe-commits at lists.llvm.org>
>> > To: "Martell Malone" <martellmalone at gmail.com>
>> > Cc: "Richard Smith" <richard at metafoo.co.uk>, "cfe-commits" <
>> cfe-commits at lists.llvm.org>
>> > Sent: Friday, August 21, 2015 2:47:50 AM
>> > Subject: Re: r245459 - According to i686 ABI, long double size on x86
>> is 12 bytes not 16 bytes.
>> >
>> >
>> >
>> >
>> > The testcase from r245459 was not reverted and still in SVN.
>> >
>> >
>> >
>> >
>> >
>> > 2015-08-21 2:05 GMT+03:00 Martell Malone < martellmalone at gmail.com >
>> > :
>> >
>> >
>> >
>> >
>> >
>> > I feel very silly now.
>> > After testing the testcase again on svn it still works.
>> > It appears the OP was looking for this patch to go onto the 3.6
>> > branch and was applying my patch to that.
>> >
>> >
>> > I'll know in future to recheck the testcase afterwards myself in
>> > future.
>> >
>> >
>> > Apologies for the noise guys.
>> >
>> >
>> > Yaron I think the test case from r245459 would be useful to ensure it
>> > is never broken in the future?
>> >
>> > Would you be able to recommit the test case?
>> >
>> >
>> >
>> > Kind Regards
>> >
>> > Martell
>> >
>> >
>> >
>> >
>> >
>> >
>> >
>> > On Thu, Aug 20, 2015 at 3:57 PM, Martell Malone <
>> > martellmalone at gmail.com > wrote:
>> >
>> >
>> >
>> >
>> >
>> > 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_32 TargetInfo 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.keren at 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 b oth
>> > 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 < richard at 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.keren at 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 < richard at metafoo.co.uk > :
>> >
>> >
>> >
>> >
>> >
>> >
>> > On Wed, Aug 19, 2015 at 11:42 AM, Yaron Keren < yaron.keren at 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
>> >
>> >
>> >
>> > -m96bit-long-double -m128bit-long-double These 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 < richard at metafoo.co.uk > :
>> >
>> >
>> >
>> >
>> >
>> >
>> > On Wed, Aug 19, 2015 at 10:02 AM, Yaron Keren via cfe-commits <
>> > cfe-commits at 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 at lists.llvm.org
>> > http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
>> >
>> >
>> >
>> >
>> >
>> >
>> > _______________________________________________
>> > cfe-commits mailing list
>> > cfe-commits at lists.llvm.org
>> > http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
>> >
>>
>> --
>> Hal Finkel
>> Assistant Computational Scientist
>> Leadership Computing Facility
>> Argonne National Laboratory
>>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20150919/d1fa5619/attachment-0001.html>