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

Martell Malone via cfe-commits cfe-commits at lists.llvm.org
Thu Aug 20 16:05:09 PDT 2015 

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_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.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 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 <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
>>>>>> <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 <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
>>>>>>>>
>>>>>>>
>>>>>>>
>>>>>
>>>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20150820/25d1393c/attachment-0001.html>

More information about the cfe-commits mailing list