Bug 46926 - Paired sin() cos() calls optimized to sincos() call.
Summary: Paired sin() cos() calls optimized to sincos() call.
Status: NEW
Alias: None
Product: gcc
Classification: Unclassified
Component: middle-end (show other bugs)
Version: 4.4.4
: P3 normal
Target Milestone: ---
Assignee: Not yet assigned to anyone
URL:
Keywords:
Depends on:
Blocks: 31249
  Show dependency treegraph
 
Reported: 2010-12-13 17:11 UTC by James Kuyper Jr.
Modified: 2019-08-21 04:15 UTC (History)
6 users (show)

See Also:
Host:
Target:
Build:
Known to work:
Known to fail:
Last reconfirmed: 2011-01-02 17:19:22


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description James Kuyper Jr. 2010-12-13 17:11:35 UTC
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.
Comment 1 Andrew Pinski 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.
Comment 2 James Kuyper Jr. 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.
Comment 3 jsm-csl@polyomino.org.uk 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.
Comment 4 Richard Biener 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.
Comment 5 Jakub Jelinek 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.
Comment 6 Richard Biener 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.
Comment 7 jsm-csl@polyomino.org.uk 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.