Bug 60637 - --fast-math breaks std::signbit function
Summary: --fast-math breaks std::signbit function
Status: RESOLVED FIXED
Alias: None
Product: gcc
Classification: Unclassified
Component: libstdc++ (show other bugs)
Version: 4.8.2
: P3 normal
Target Milestone: 4.9.4
Assignee: Jonathan Wakely
URL:
Keywords: wrong-code
Depends on:
Blocks:
 
Reported: 2014-03-24 17:40 UTC by magicstix
Modified: 2016-01-18 17:16 UTC (History)
3 users (show)

See Also:
Host:
Target:
Build:
Known to work: 6.0
Known to fail:
Last reconfirmed: 2016-01-12 00:00:00


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description magicstix 2014-03-24 17:40:48 UTC
On GCC 4.4.6 and GCC 4.8.2, the std::signbit gives invalid results when code is compiled with the --fast-math option. 

The following minimal test case can replicate the problem:


#include <iostream>
#include <cmath>

int main(int argc, char** argv)
{
   long double ld = -5.3165867831218916301793863361917824e-2467L;
   std::cout << std::signbit(ld) << std::endl;

   return 0;
}

When compiled with --fast-math, this test case prints out an invalid result of 0.

I first noticed the problem in working with the boost libraries (specifically their bessel function implementation). The boost dev-team was able to narrow down the problem to the std::signbit function. 

The bug has been replicated on RHEL 6.3's stock GCC 4.4.6 and a compiled-from-source GCC 4.8.2. The development system exhibiting the behavior is a stock 64-bit RHEL 6.3 Linux install.

The bug has also been found to occur when compiling the code using Intel's C++ compiler using -std=c++11 (which relies on GCC's standard C++ libraries). 

I also carried out a sanity test on GCC 4.1.2 and found that the problem *does not* occur, indicating a regression at some point between 4.1.2 and 4.4.6.

My output from gcc -v:
Target: x86_64-unknown-linux-gnu
Configured with: ../configure --disable-multilib
Thread model: posix
gcc version 4.8.2 (GCC)
Comment 1 Daniel Krügler 2014-03-24 20:25:16 UTC
This seems to be fixed in 4.9.0 trunk. My guess is that this was solved via bug 58625.
Comment 2 Marc Glisse 2014-03-24 20:31:38 UTC
IIUC (with gcc-4.9), signbit is not part of C++98, and with -std=c++11 things work. In C++98 mode, libstdc++ (sometimes) provides std::signbit as an extension, which calls __builtin_signbit which takes a double as argument; the conversion yields -infinity, which with -ffast-math is undefined behavior.

I don't know the history of why libstdc++ provides non-standard functions in namespace std, but since it does, it should probably use the same code as C++11, or at least add 'l' after __builtin_signbit (use the safest of the 3 versions).
Comment 3 Jonathan Wakely 2016-01-08 01:20:05 UTC
Thanks to PR 36757 __builtin_signbit is now generic, so this is fixed on trunk.
Comment 4 Jonathan Wakely 2016-01-12 19:06:54 UTC
I think we can fix the C++98 std::signbit(_Tp) with:

      typedef typename __gnu_cxx::__promote<_Tp>::__type __type;
      return sizeof(__type) == sizeof(long double)
	? __builtin_signbitl(__type(__f))
	: sizeof(__type) == sizeof(double)
	? __builtin_signbit(__type(__f))
	: __builtin_signbitf(__type(__f));
Comment 5 Marc Glisse 2016-01-12 20:12:36 UTC
(In reply to Jonathan Wakely from comment #4)
> I think we can fix the C++98 std::signbit(_Tp) with:
> 
>       typedef typename __gnu_cxx::__promote<_Tp>::__type __type;
>       return sizeof(__type) == sizeof(long double)
> 	? __builtin_signbitl(__type(__f))
> 	: sizeof(__type) == sizeof(double)
> 	? __builtin_signbit(__type(__f))
> 	: __builtin_signbitf(__type(__f));

Why do you want to use a different code for C++98 and C++11? The best solution would be to remove the declaration in C++98, since it is not standard, but if we have to keep it, it would seem easier to share the code with C++11, no?
Comment 6 Jonathan Wakely 2016-01-12 22:47:29 UTC
(In reply to Marc Glisse from comment #5)
> Why do you want to use a different code for C++98 and C++11? The best
> solution would be to remove the declaration in C++98, since it is not
> standard, but if we have to keep it,

Yes, we can't really remove it now, we have no idea how much code is using it.

> it would seem easier to share the code
> with C++11, no?

Maybe. Since I don't plan to change trunk (because the c++98 signbit function template works correctly there) I wanted to make a less obtrusive change than replacing the existing function with the c++11 versions. By just fixing the body we don't change what is declared, we just make it work correctly.

Just sharing the code with C++11 might not be straightforward, because I don't know if Solaris 12 declares std::signbit for C++98 mode. If it doesn't, then we can't just reuse the same code, because that code relies on declarations in Solaris' <math.h>

That might not be a problem, if _GLIBCXX_USE_C99_MATH is not declared for Solaris 12, because we wouldn't declare std::signbit at all in c++98 mode ... but I don't know, and would rather not take risks or require new configure checks on the stable 4.9 and 5 release branches.
Comment 7 Jonathan Wakely 2016-01-12 22:50:41 UTC
To be clear, I'm not opposed to reusing the C++11 code for the non-standard C++98 functions (all of them, not just signbit). That seems like a reasonable project to undertake in stage 1, if someone is interested, but that wouldn't help fix this bug in 4.9 and 5.
Comment 8 Jonathan Wakely 2016-01-18 16:28:47 UTC
Author: redi
Date: Mon Jan 18 16:28:16 2016
New Revision: 232531

URL: https://gcc.gnu.org/viewcvs?rev=232531&root=gcc&view=rev
Log:
Fix C++98 std::signbit<long double>

	PR libstdc++/60637
	* include/c_global/cmath (signbit) [__cplusplus < 201103L]: Use
	__builtin_signbitf or __builtin_signbitl as appropriate.
	* testsuite/26_numerics/headers/cmath/60637.cc: New.

Added:
    branches/gcc-4_9-branch/libstdc++-v3/testsuite/26_numerics/headers/cmath/60637.cc
Modified:
    branches/gcc-4_9-branch/libstdc++-v3/ChangeLog
    branches/gcc-4_9-branch/libstdc++-v3/include/c_global/cmath
Comment 9 Jonathan Wakely 2016-01-18 16:29:21 UTC
Author: redi
Date: Mon Jan 18 16:28:48 2016
New Revision: 232532

URL: https://gcc.gnu.org/viewcvs?rev=232532&root=gcc&view=rev
Log:
Fix C++98 std::signbit<long double>

	PR libstdc++/60637
	* include/c_global/cmath (signbit) [__cplusplus < 201103L]: Use
	__builtin_signbitf or __builtin_signbitl as appropriate.
	* testsuite/26_numerics/headers/cmath/60637.cc: New.

Added:
    branches/gcc-5-branch/libstdc++-v3/testsuite/26_numerics/headers/cmath/60637.cc
Modified:
    branches/gcc-5-branch/libstdc++-v3/ChangeLog
    branches/gcc-5-branch/libstdc++-v3/include/c_global/cmath
Comment 10 Jonathan Wakely 2016-01-18 16:54:55 UTC
Fixed for 4.9.4 and 5.4
Comment 11 Jonathan Wakely 2016-01-18 17:16:13 UTC
Author: redi
Date: Mon Jan 18 17:15:42 2016
New Revision: 232534

URL: https://gcc.gnu.org/viewcvs?rev=232534&root=gcc&view=rev
Log:
Add test for PR 60637

	PR libstdc++/60637
	* testsuite/26_numerics/headers/cmath/60637.cc: Add test.

Added:
    trunk/libstdc++-v3/testsuite/26_numerics/headers/cmath/60637.cc
Modified:
    trunk/libstdc++-v3/ChangeLog