[Bug c/46926] New: Paired sin() cos() calls optimized to sincos() call.

[Bug c/46926] New: Paired sin() cos() calls optimized to sincos() call.

[jameskuyper at verizon dot net]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=46926

           Summary: Paired sin() cos() calls optimized to sincos() call.
           Product: gcc
           Version: 4.4.4
            Status: UNCONFIRMED
          Severity: normal
          Priority: P3
         Component: c
        AssignedTo: unassigned@gcc.gnu.org
        ReportedBy: jameskuyper@verizon.net


I discovered this problem when porting from a mandriva Linux system using gcc
4.2.2 to a centos linux system using gcc 4.4.4. After much simplification, I
can now demonstrate the problem with an extremely short example program:

#include <math.h>
#include <stdlib.h>
void sincos(double val, double *sin_val, double *cos_val)
{
    *sin_val = sin(val);
    *cos_val = cos(val);

    return;
}

int func(
    double * pd
){
    double cosine, sine;

    /* must use value accessed through pointer; bug disappears
     * without the pointer access.
     */
    cosine = cos(*pd);
    sine = sin(*pd);

    /* Need to use cosine[] and sine[], or optimizer drops them. */
    return cosine < 0.0 && sine > 0.0 ? EXIT_FAILURE : EXIT_SUCCESS;
}

int main (void)
{
   double d=0.0;
   return func(&d) ;
}

In the original program, the sincos() function defined above occurred in a
third-party library designed for use with a fully conforming implementation of
C90. Such implementations cannot provide a sincos() function in their standard
C library in any way that would conflict with a user-defined function of the
same name. I compiled and linked this program using the following command:

    gcc -ansi -pedantic -O1 -g sincos_prob.c -lm -o sincos_prob

When I execute the program, it produces a bus error. gdb indicates that the bus
error occurs inside a recursive (!?) call to sincos(). The problem does not
come up if I lower the optimization level to -O0.

The reason appears to be that, at optimization levels of 1 or higher, paired
calls to sin() and cos(), like those that occur in two seperate locations
above, are replaced with a single call to sincos() - in itself, a seemingly
reasonable thing to do. The sincos() definition provided by the third party
library is used in place of the one provided as an extension in the C standard
library, which also seems reasonable. Where this all goes wrong is inside the
body of the third-party library's definition of sincos(). The sin()/cos()
optimization turns that function into an infinitely recursive call to itself.

Because sincos() is not a C standard library routine, the code given above does
not violate any constraint, and has well defined behavior - behavior which does
not include generating a bus error.

When invoked in a mode that's supposed to be fully conforming to either C90 or
C99, the compiler should not generate spurious calls to functions whose names
are not reserved to the implementation. Use of a reserved name, such as
__sincos(), seems to me like it would be the simplest fix for this problem.

[Bug c/46926] Paired sin() cos() calls optimized to sincos() call.

[pinskia at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=46926

--- Comment #1 from Andrew Pinski <pinskia at gcc dot gnu.org> 2010-12-13 17:23:05 UTC ---
I think this is invalid as GNU/Linux defaults to including sincos as a builtin.
 If you want to disable the builtin then use -fno-builtin-sincos.

[Bug c/46926] Paired sin() cos() calls optimized to sincos() call.

[jameskuyper at verizon dot net]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=46926

--- Comment #2 from James Kuyper Jr. <jameskuyper at verizon dot net> 2010-12-13 18:41:55 UTC ---
info gcc says:

Functions which would normally be built in but do not have
     semantics defined by ISO C (such as `alloca' and `ffs') are not
     built-in functions with `-ansi' is used.

[Bug c/46926] Paired sin() cos() calls optimized to sincos() call.

[joseph at codesourcery dot com]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=46926

--- Comment #3 from joseph at codesourcery dot com <joseph at codesourcery dot com> 2010-12-13 18:48:22 UTC ---
On Mon, 13 Dec 2010, pinskia at gcc dot gnu.org wrote:

> I think this is invalid as GNU/Linux defaults to including sincos as a 
> builtin.  If you want to disable the builtin then use 
> -fno-builtin-sincos.

It seems valid to me.  The options specified included -ansi (i.e. 
-std=c90) which implies -fno-builtin-sincos.

Whether GCC *generates* calls to a function when that function does not 
appear in the source code is independent of how it *handles* calls to a 
function in the source code, and -fno-builtin-* deals with the latter 
only.  It's always OK to generate calls to C90 function such as memcpy 
(standards.texi list the subset that may be used in freestanding mode) or 
to C99 functions when in C99 mode or to reserved-namespace functions in 
libgcc.  It's not OK to generate calls to non-reserved-namespace libc/libm 
functions, when in a strict conformance mode such as -std=c90/-std=c99, if 
those functions are outside the language version specified and the calls 
do not appear in the source code.  This includes sincos.

[Bug c/46926] Paired sin() cos() calls optimized to sincos() call.

[rguenth at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=46926

Richard Guenther <rguenth at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |jsm28 at gcc dot gnu.org,
                   |                            |rguenth at gcc dot gnu.org

--- Comment #4 from Richard Guenther <rguenth at gcc dot gnu.org> 2010-12-16 14:25:12 UTC ---
Hum.  We generate calls to sincos() if TARGET_HAS_SINCOS is true and a call
to cexp() if TARGET_C99_FUNCTIONS is true.  We do so in two steps.  First
we generate a call to a GCC internal builtin cexpi () which is a complex
exponentation with just an imaginary argument.  Then during expansion we
expand that to either sincos() or cexp().  If you look at
expand_builtin_cexpi then it is obvious that we'd ICE if TARGET_HAS_SINCOS
is true but the sincos builtin is not available.  Thus I conclude that
the sincos builtin is available when it really is should not be (with -ansi),
which makes this a C frontend bug(?).  It would of course start to ICE
if TARGET_HAS_SINCOS is true but the builtin isn't there, but that's another
issue.

Thus, I'm not sure how the frontend communicates -ansi effects to the
middle-end.
We have:

DEF_EXT_LIB_BUILTIN    (BUILT_IN_SINCOS, "sincos",
BT_FN_VOID_DOUBLE_DOUBLEPTR_DOUBLEPTR, ATTR_MATHFN_FPROUNDING_STORE)

but built_in_decls has a decl for it.

[Bug c/46926] Paired sin() cos() calls optimized to sincos() call.

[jakub at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=46926

Jakub Jelinek <jakub at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |jakub at gcc dot gnu.org

--- Comment #5 from Jakub Jelinek <jakub at gcc dot gnu.org> 2010-12-16 14:49:39 UTC ---
That is what the built_in_decls vs. implicit_built_in_decls distinction is for.
Except that for sincos, being a POSIX but not C function,
implicit_built_in_decls[BUILT_IN_SINCOS] is always NULL.

[Bug c/46926] Paired sin() cos() calls optimized to sincos() call.

[rguenth at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=46926

--- Comment #6 from Richard Guenther <rguenth at gcc dot gnu.org> 2010-12-16 15:24:23 UTC ---
(In reply to comment #5)
> That is what the built_in_decls vs. implicit_built_in_decls distinction is for.
> Except that for sincos, being a POSIX but not C function,
> implicit_built_in_decls[BUILT_IN_SINCOS] is always NULL.

Which is why we have TARGET_HAS_SINCOS ...

I don't see an existing way to pass the requested information from the
C frontend.  We also can't avoid the transformation based on the
function name as that function might be called by a user function named
sincos.

Maybe the _EXT builtins shouldn't be in built_in_decls as well unless
-std=gnuXX is used.  I suppose -fno-builtin-sincos does work as a
workaround though.

[Bug c/46926] Paired sin() cos() calls optimized to sincos() call.

[joseph at codesourcery dot com]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=46926

--- Comment #7 from joseph at codesourcery dot com <joseph at codesourcery dot com> 2010-12-16 15:30:17 UTC ---
On Thu, 16 Dec 2010, rguenth at gcc dot gnu.org wrote:

> Hum.  We generate calls to sincos() if TARGET_HAS_SINCOS is true and a call
> to cexp() if TARGET_C99_FUNCTIONS is true.  We do so in two steps.  First
> we generate a call to a GCC internal builtin cexpi () which is a complex
> exponentation with just an imaginary argument.  Then during expansion we
> expand that to either sincos() or cexp().  If you look at
> expand_builtin_cexpi then it is obvious that we'd ICE if TARGET_HAS_SINCOS
> is true but the sincos builtin is not available.  Thus I conclude that
> the sincos builtin is available when it really is should not be (with -ansi),
> which makes this a C frontend bug(?).  It would of course start to ICE
> if TARGET_HAS_SINCOS is true but the builtin isn't there, but that's another
> issue.

It is correct that __builtin_sincos is always available and that it may 
generate a call to sincos - even when the sincos built-in function isn't 
available.  But for code just calling sin and cos, calls to sincos 
shouldn't be generated in standards conformance modes where the name 
sincos isn't reserved (this is independent of -fno-builtin-sincos, which 
really should only affect the handling of source code that explicitly uses 
the function sincos).  Similarly for cexp outside C99 mode.

> Thus, I'm not sure how the frontend communicates -ansi effects to the
> middle-end.

Well, it will disable sincos and cexp as built-in functions as appropriate 
(while leaving the __builtin_ versions), but since -fno-builtin-* will 
also do that and is logically independent of the information we want 
(about what functions it is OK to generate calls to when those calls did 
not appear in the source code) that may not be quite the thing to check.  
Obviously the middle-end - which for these purposes includes the 
implementations of the TARGET_HAS_SINCOS and TARGET_C99_FUNCTIONS macros - 
can't depend on things such as flag_iso or flag_isoc99.

There's already an IMPLICIT argument to DEF_BUILTIN.  If its documented 
semantics are followed, for sincos you'd have "TARGET_HAS_SINCOS && 
!flag_iso", and for cexp "TARGET_C99_FUNCTIONS && (flag_isoc99 || 
!flag_iso)" (for DEF_C99_C90RES_BUILTIN, I think the existing use of 
TARGET_C99_FUNCTIONS on its own is correct, however).  But then you'd need 
to work out what impact this might have elsewhere in the compiler, and fix 
the places currently checking TARGET_HAS_SINCOS or TARGET_C99_FUNCTIONS 
directly.

[Bug c/46926] Paired sin() cos() calls optimized to sincos() call.

[aph at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=46926

Andrew Haley <aph at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|UNCONFIRMED                 |NEW
   Last reconfirmed|                            |2011.01.02 17:19:22
                 CC|                            |aph at gcc dot gnu.org
     Ever Confirmed|0                           |1