Re: [PATCH, C++] Warn on redefinition of builtin functions (PR c++/71973)

[Bernd Edlinger <bernd dot edlinger at hotmail dot de>]

On 10/06/16 16:11, Bernd Edlinger wrote:
> Hi!
>
> Currently C++ does not warn at all when built-in functions are
> re-defined with a different signature, while C does warn on that
> even without -Wall.
>
> Thus I'd like to propose a -Wall enabled warning for that in C++ only.
>
> Initially I tried to warn unconditionally but that made too  many tests
> in the C++ testsuite emit that warning :-(
>
> So making the warning dependent on -Wall is a compromise due
> to the very many compile only tests, that use this "feature".
>
> There is also a wrong-code side on this redefinition, because
> even if the new function has the nothrow attribute the code is
> generated as if it could throw.  Fixed as well.
>
> This is an updated version of the patch that was posted
> here: https://gcc.gnu.org/ml/gcc-patches/2016-08/msg01463.html
> ... but got no response so far.
>
> I have seen a few warnings in system headers, when -Wsystem-headers
> is used, and fixed most of them, for instance strftime got a
> redefinition warning, because the const struct tm* parameter has to be
> handled as the FILE* in other builtin functions.
>
> As a little surprise, it turned out, there were warnings missing
> from strftime in C due to the type conflict when merging the builtin
> with the actual declaration, see the format warnings in the test case
> testsuite/g++.dg/pr71973-2.C, which were not working previously in C++.
>
> Then there are cases, where a builtin function and C++ overloads
> have the same name like the abs function from cstdlib.  Due to
> missing strictness in duplicate_decls the builtin got merged with
> one of the C++ overloads.  That was visible due to the new warning.
>
> I believe a builtin function always needs an extern "C" declaration.
> Additional C++ overloads are possible, but do not redefine the builtin
> function, and create additional overloads.
>
> However these warnings remain, and I have no idea if the header
> file is correct or the warning:
>
> /usr/include/unistd.h:551:12: warning: declaration of 'int execve(const
> char*, char* const*, char* const*)' conflicts with built-in declaration
> 'int execve(const char*, const char**, const char**)'
> [-Wbuiltin-function-redefined]
>  extern int execve (const char *__path, char *const __argv[],
>             ^~~~~~
> /usr/include/unistd.h:563:12: warning: declaration of 'int execv(const
> char*, char* const*)' conflicts with built-in declaration 'int
> execv(const char*, const char**)' [-Wbuiltin-function-redefined]
>  extern int execv (const char *__path, char *const __argv[])
>             ^~~~~
> /usr/include/unistd.h:578:12: warning: declaration of 'int execvp(const
> char*, char* const*)' conflicts with built-in declaration 'int
> execvp(const char*, const char**)' [-Wbuiltin-function-redefined]
>  extern int execvp (const char *__file, char *const __argv[])
>             ^~~~~~
>
> Interesting is that, these do not happen with -std=c++03/11/14 but only
> with -std=gnu++03/11/14.  I could not figure out how to resolve that.
>
> As said, this is only visible with -Wsystem-headers, and would not
> happen, if the declaration in unistd.h would match the builtin function
> prototype.
>

Just in case anybody wants to know, I digged into the warnings
about execv/e/p, and found the following:


First these builtins are declared with DEF_EXT_LIB_BUILTIN, and thus
the builtin function without __builtin_ in the name is only visible with
gnu extensions, therefore the warning does not happen with -std=c++XX
only with -std=gnu++XX.

Second, the declaration in the glibc header files simply look wrong,
because the type of argv, and envp is "char *const *" while the
builtin function wants "const char**", thus only the array of char*
itself is const, not the actual char stings they point to.

Third, in C the  builtins are not diagnosed, because C does only look
at the mode of the parameters see match_builtin_function_types in
c/c-decl.c, which may itself be wrong, because that makes an ABI
decision dependent on the mode of the parameter.

What was broken, because of that mismatch?

Except the missing warning about the redeclared builtin function, there
was also some hidden bug in the execv-builtins:

That is with -fprofile-arcs the execv/e/p is not replaced
with a call to __gcov_execv/e/p, and thus if a C++ program is
left through one of these functions the profile information is
not saved.

This is not fixed with the patch as is:

It would of course be fixed by changing the glibc headers, or if it
turns out that the GCC built-in is actually using the wrong prototype
that would also be possible to fix.

Any ideas which way to go on here?

Maybe a new fix-include rule?


Thanks
Bernd.