The testcase from r245459 was not reverted and still in SVN.
2015-08-21 2:05 GMT+03:00 Martell Malone <martellmal...@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 <martellmal...@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.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
