Re: [PATCH] Clean-up C's builtin function type matching.

[Roger Sayle <roger at eyesopen dot com>]

I'll try my best to respond to your issues as best I can.  However,
there's one issue that I'd like to make crystal clear from the start.
My proposed patch only affects re-declarations of builtins as functions
in the C front-end.  Redeclarations of builtins as non-functions (as
in Gerald's e-mail) and redeclarations in the C++ front-end (as in
Gabriel's e-mail) are handled by completely independent pieces of code
and the concerns voiced are unrelated to the proposed patch.

On 21 Jul 2003, Gabriel Dos Reis wrote:
> Fergus Henderson <fjh@cs.mu.OZ.AU> writes:
> | > Plain gcc issues
> | >   x.c:1: warning: built-in function `exp' declared as non-function
> | > and gcc -Wshadow issues
> | >   x.c:1: warning: built-in function `exp' declared as non-function
> | >   x.c: In function `main':
> | >   x.c:4: warning: declaration of `sin' shadows a global declaration
> | >   <built-in>:0: warning: shadowed declaration is here
> | > all of which I have been told are wrong as long as <math.h> hasn't been
> | > #included.
> |
> | The warnings about `exp' are definitely justified, even if <math.h>
> | hasn't been included, since the program fails to conform to the C standard.
> |
> | The warnings about `sin' are a bit more debatable.
>
> It certainly exhibits a *bug* in the way built-ins are being handled.

The code in question that explicitly tests for a redeclaration of a
built-in as a non-function type is handled between lines 770 and 787
of c-decl.c.  If you look at the code or even read the wording of these
error messages, you'll see they are not some side-effect of obscure
interactions in the front-end, but some one has gone out of their way
to issue these diagnostics.

>  /* New decl is completely inconsistent with the old one =>
>     tell caller to replace the old one.
>     This is always an error except in the case of shadowing a builtin.  */
>  if (TREE_CODE (olddecl) != TREE_CODE (newdecl))
>    {
>      if (TREE_CODE (olddecl) == FUNCTION_DECL
>          && DECL_BUILT_IN (olddecl))
>        {
>          /* If you declare a built-in or predefined function name as static,
>             the old definition is overridden,
>             but optionally warn this was a bad choice of name.  */
>          if (!TREE_PUBLIC (newdecl))
>            {
>              if (warn_shadow)
>                warning ("%Hshadowing built-in function '%D'",
>                         &DECL_SOURCE_LOCATION (newdecl), newdecl);
>            }
>          else
>            warning ("%Hbuilt-in function '%D' declared as non-function",
>                     &DECL_SOURCE_LOCATION (newdecl), newdecl);
>        }


Feel free to argue about the logic itself, but the relevant code in the
C front-end is quite transparent and can easily be fixed on whatever
semantics are decided upon.

A short term fix may be to change "extern int exp" to "static int exp"
and add "-Wno-shadow" as a command line option.  Alternatively,
"-ffree-standing" or "-fno-builtin" will also disable the builtins.
Finally, is a sharper scalpel is required, "-fno-builtin-exp" and
"-fno-builtin-sin" will also work.


As for Gaby's comments, yes that definitely looks like a real bug
in g++'s built-in handling (which as I've mentioned bears very
little similarity with C's).  It looks like things must have changed
recently in the namespace handling in the C++ front-end, because
your example also fails the other way.

double fabs(double);

namespace std
{
  using ::fabs;
}

int main()
{
  double (*p)(double) = std::fabs;
}


Given that this worked has worked in the past, i.e. I've confirmed that
it worked fine in gcc-3.2, we need to track down which recent patch has
introduced this regression.  I believe I can fix things, but it'll be
important to understand what's changed that broke the original
implementation.


Its also a pity the C++ folks are so unresponsive when asking for
technical help to fix these kinds of g++ bugs.  I've two analyzed
PRs waiting for feedback from someone who understands C++ type
conversion machinery (perhaps I should just reassign them to the
C++ category in bugzilla?):
http://gcc.gnu.org/ml/gcc-patches/2003-07/msg00596.html
http://gcc.gnu.org/ml/gcc-patches/2003-06/msg03179.html

Roger
--
Roger Sayle,                         E-mail: roger@eyesopen.com
OpenEye Scientific Software,         WWW: http://www.eyesopen.com/
Suite 1107, 3600 Cerrillos Road,     Tel: (+1) 505-473-7385
Santa Fe, New Mexico, 87507.         Fax: (+1) 505-473-0833