| Message ID | HE1PR0701MB2169DB217F73D8111DA9F690E4C70@HE1PR0701MB2169.eurprd07.prod.outlook.com |
|---|---|
| State | New |
| Headers | show |
Hi Bernd, Not familiar with this area but one comment below... On 06/10/16 15:12, 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. > > > The reg-test also revealed a test case that is IMO invalid, and > depends on the incorrect merging of a builtin definition with a friend > function see testsuite/g++.dg/lookup/extern-c-redecl4.C: > This test case does no longer compile, because the builtin > fails to have an explicit declaration. > > > Bootstrap and reg-testing on x86_64-pc-linux-gnu. > OK for trunk? > > > Thanks > Bernd. @@ -1553,7 +1588,7 @@ duplicate_decls (tree newdecl, tree olddecl, bool /* Whether or not the builtin can throw exceptions has no bearing on this declarator. */ - TREE_NOTHROW (olddecl) = 0; + TREE_NOTHROW (olddecl) = TREE_NOTHROW (newdecl); The comment would need to be updated I think. Kyrill
On 10/06/16 16:14, Kyrill Tkachov wrote: > > @@ -1553,7 +1588,7 @@ duplicate_decls (tree newdecl, tree olddecl, bool > > /* Whether or not the builtin can throw exceptions has no > bearing on this declarator. */ > - TREE_NOTHROW (olddecl) = 0; > + TREE_NOTHROW (olddecl) = TREE_NOTHROW (newdecl); > > The comment would need to be updated I think. Probably, yes. Actually the code did *not* do what the comment said, and effectively set the nothrow attribute to zero, thus the eh handlers were emitted when not needed. And IMHO the new code does now literally do what the comment said. At this point there follow 1000+ lines of code, in the same function that merge olddecl into newdecl and back again. The code is dependent on the types_match variable, and in the end newdecl is free'd an olddecl returned. At some places the code is impossible to understand: Look for the memcpy :-/ I *think* the intention is to merge the attribute from the builtin when the header file is not explicitly giving, some or all attributes, when the parameters match. But when the parameters do not match, the header file changes the builtin's signature, and overrides the builtin attributes more or less with defaults or whatever is in the header file. Bernd.
On 10/06/16 22:37, Bernd Edlinger wrote: > On 10/06/16 16:14, Kyrill Tkachov wrote: >> >> @@ -1553,7 +1588,7 @@ duplicate_decls (tree newdecl, tree olddecl, bool >> >> /* Whether or not the builtin can throw exceptions has no >> bearing on this declarator. */ >> - TREE_NOTHROW (olddecl) = 0; >> + TREE_NOTHROW (olddecl) = TREE_NOTHROW (newdecl); >> >> The comment would need to be updated I think. > > Probably, yes. > > Actually the code did *not* do what the comment said, and > effectively set the nothrow attribute to zero, thus > the eh handlers were emitted when not needed. > > And IMHO the new code does now literally do what the comment > said. > > At this point there follow 1000+ lines of code, in the same > function that merge olddecl into newdecl and back again. > > The code is dependent on the types_match variable, > and in the end newdecl is free'd an olddecl returned. > > At some places the code is impossible to understand: > Look for the memcpy :-/ > > I *think* the intention is to merge the attribute from the > builtin when the header file is not explicitly giving, > some or all attributes, when the parameters match. > > But when the parameters do not match, the header file > changes the builtin's signature, and overrides the > builtin attributes more or less with defaults or > whatever is in the header file. > > A few more thoughts, that may help to clarify a few things here. Regarding this hunk: else if (! same_type_p (TREE_VALUE (t1), TREE_VALUE (t2))) break; + if (t1 || t2 + || ! same_type_p (TREE_TYPE (TREE_TYPE (olddecl)), + TREE_TYPE (TREE_TYPE (newdecl)))) + warning_at (DECL_SOURCE_LOCATION (newdecl), + OPT_Wbuiltin_function_redefined, + "declaration of %q+#D conflicts with built-in " + "declaration %q#D", newdecl, olddecl); } else if ((DECL_EXTERN_C_P (newdecl) meanwhile I start to think that the "if" here is unnecessary, because if decls_match returns false, the declarations are certainly different. And the warning is thus already justified at this point. Removing the if changes nothing, the condition is always satisfied. Regarding this hunk: /* Whether or not the builtin can throw exceptions has no bearing on this declarator. */ - TREE_NOTHROW (olddecl) = 0; + TREE_NOTHROW (olddecl) = TREE_NOTHROW (newdecl); You may ask, why the old code was working most of the time. I think, usually, when types_match == true, there happens another assignment to TREE_NOTHROW, later in that function around line 2183: /* Merge the type qualifiers. */ if (TREE_READONLY (newdecl)) TREE_READONLY (olddecl) = 1; if (TREE_THIS_VOLATILE (newdecl)) TREE_THIS_VOLATILE (olddecl) = 1; if (TREE_NOTHROW (newdecl)) TREE_NOTHROW (olddecl) = 1; This is in a big "if (types_match)", so I think that explains, why the old code did work normally, and why it fails if the parameter don't match, but I still have no idea what to say in the comment, except that the code should exactly do what the comment above says. Bernd.
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.
On Sun, 16 Oct 2016, Bernd Edlinger wrote: > 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. char *const * is the POSIX type. (It can't be const char ** or const char *const * because you can't convert char ** implicitly to those types in ISO C.) You'd need to check the list archives for rationale for the built-in functions doing something different. > 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. The matching needs to be loose because of functions using types such as FILE * where the compiler doesn't know the exact contents of the type when processing built-in function definitions. (Historically there were also issues with pre-ISO headers, but that may be less relevant now.)
Index: gcc/builtin-types.def =================================================================== --- gcc/builtin-types.def (revision 240799) +++ gcc/builtin-types.def (working copy) @@ -103,6 +103,7 @@ DEF_PRIMITIVE_TYPE (BT_COMPLEX_LONGDOUBLE, complex DEF_PRIMITIVE_TYPE (BT_PTR, ptr_type_node) DEF_PRIMITIVE_TYPE (BT_FILEPTR, fileptr_type_node) +DEF_PRIMITIVE_TYPE (BT_CONST_TM_PTR, const_tm_ptr_type_node) DEF_PRIMITIVE_TYPE (BT_CONST_PTR, const_ptr_type_node) DEF_PRIMITIVE_TYPE (BT_VOLATILE_PTR, build_pointer_type @@ -511,8 +512,8 @@ DEF_FUNCTION_TYPE_4 (BT_FN_SIZE_CONST_PTR_SIZE_SIZ BT_SIZE, BT_CONST_PTR, BT_SIZE, BT_SIZE, BT_FILEPTR) DEF_FUNCTION_TYPE_4 (BT_FN_INT_STRING_SIZE_CONST_STRING_VALIST_ARG, BT_INT, BT_STRING, BT_SIZE, BT_CONST_STRING, BT_VALIST_ARG) -DEF_FUNCTION_TYPE_4 (BT_FN_SIZE_STRING_SIZE_CONST_STRING_CONST_PTR, - BT_SIZE, BT_STRING, BT_SIZE, BT_CONST_STRING, BT_CONST_PTR) +DEF_FUNCTION_TYPE_4 (BT_FN_SIZE_STRING_SIZE_CONST_STRING_CONST_TM_PTR, + BT_SIZE, BT_STRING, BT_SIZE, BT_CONST_STRING, BT_CONST_TM_PTR) DEF_FUNCTION_TYPE_4 (BT_FN_PTR_PTR_CONST_PTR_SIZE_SIZE, BT_PTR, BT_PTR, BT_CONST_PTR, BT_SIZE, BT_SIZE) DEF_FUNCTION_TYPE_4 (BT_FN_PTR_PTR_INT_SIZE_SIZE, Index: gcc/builtins.def =================================================================== --- gcc/builtins.def (revision 240799) +++ gcc/builtins.def (working copy) @@ -866,7 +866,7 @@ DEF_GCC_BUILTIN (BUILT_IN_RETURN_ADDRESS, " DEF_GCC_BUILTIN (BUILT_IN_SAVEREGS, "saveregs", BT_FN_PTR_VAR, ATTR_NULL) DEF_GCC_BUILTIN (BUILT_IN_SETJMP, "setjmp", BT_FN_INT_PTR, ATTR_RT_NOTHROW_LEAF_LIST) DEF_EXT_LIB_BUILTIN (BUILT_IN_STRFMON, "strfmon", BT_FN_SSIZE_STRING_SIZE_CONST_STRING_VAR, ATTR_FORMAT_STRFMON_NOTHROW_3_4) -DEF_LIB_BUILTIN (BUILT_IN_STRFTIME, "strftime", BT_FN_SIZE_STRING_SIZE_CONST_STRING_CONST_PTR, ATTR_FORMAT_STRFTIME_NOTHROW_3_0) +DEF_LIB_BUILTIN (BUILT_IN_STRFTIME, "strftime", BT_FN_SIZE_STRING_SIZE_CONST_STRING_CONST_TM_PTR, ATTR_FORMAT_STRFTIME_NOTHROW_3_0) DEF_GCC_BUILTIN (BUILT_IN_TRAP, "trap", BT_FN_VOID, ATTR_NORETURN_NOTHROW_LEAF_LIST) DEF_GCC_BUILTIN (BUILT_IN_UNREACHABLE, "unreachable", BT_FN_VOID, ATTR_CONST_NORETURN_NOTHROW_LEAF_LIST) DEF_GCC_BUILTIN (BUILT_IN_UNWIND_INIT, "unwind_init", BT_FN_VOID, ATTR_NULL) Index: gcc/c-family/c-common.c =================================================================== --- gcc/c-family/c-common.c (revision 240799) +++ gcc/c-family/c-common.c (working copy) @@ -5619,9 +5619,13 @@ c_common_nodes_and_builtins (void) } if (c_dialect_cxx ()) - /* For C++, make fileptr_type_node a distinct void * type until - FILE type is defined. */ - fileptr_type_node = build_variant_type_copy (ptr_type_node); + { + /* For C++, make fileptr_type_node a distinct void * type until + FILE type is defined. */ + fileptr_type_node = build_variant_type_copy (ptr_type_node); + /* Likewise for const struct tm*. */ + const_tm_ptr_type_node = build_variant_type_copy (const_ptr_type_node); + } record_builtin_type (RID_VOID, NULL, void_type_node); Index: gcc/c-family/c.opt =================================================================== --- gcc/c-family/c.opt (revision 240799) +++ gcc/c-family/c.opt (working copy) @@ -323,6 +323,10 @@ Wframe-address C ObjC C++ ObjC++ Var(warn_frame_address) Warning LangEnabledBy(C ObjC C++ ObjC++,Wall) Warn when __builtin_frame_address or __builtin_return_address is used unsafely. +Wbuiltin-function-redefined +C++ ObjC++ Var(warn_builtin_function_redefined) Warning LangEnabledBy(C++ ObjC++,Wall) +Warn when a built-in function is redefined. + Wbuiltin-macro-redefined C ObjC C++ ObjC++ CPP(warn_builtin_macro_redefined) CppReason(CPP_W_BUILTIN_MACRO_REDEFINED) Var(cpp_warn_builtin_macro_redefined) Init(1) Warning Warn when a built-in preprocessor macro is undefined or redefined. Index: gcc/cp/decl.c =================================================================== --- gcc/cp/decl.c (revision 240799) +++ gcc/cp/decl.c (working copy) @@ -1468,10 +1468,15 @@ duplicate_decls (tree newdecl, tree olddecl, bool explicitly declared. */ if (DECL_ANTICIPATED (olddecl)) { - /* Deal with fileptr_type_node. FILE type is not known - at the time we create the builtins. */ tree t1, t2; + /* A new declaration doesn't match a built-in one unless it + is also extern "C". */ + gcc_assert (DECL_IS_BUILTIN (olddecl)); + gcc_assert (DECL_EXTERN_C_P (olddecl)); + if (!DECL_EXTERN_C_P (newdecl)) + return NULL_TREE; + for (t1 = TYPE_ARG_TYPES (TREE_TYPE (newdecl)), t2 = TYPE_ARG_TYPES (TREE_TYPE (olddecl)); t1 || t2; @@ -1478,6 +1483,8 @@ duplicate_decls (tree newdecl, tree olddecl, bool t1 = TREE_CHAIN (t1), t2 = TREE_CHAIN (t2)) if (!t1 || !t2) break; + /* Deal with fileptr_type_node. FILE type is not known + at the time we create the builtins. */ else if (TREE_VALUE (t2) == fileptr_type_node) { tree t = TREE_VALUE (t1); @@ -1498,8 +1505,36 @@ duplicate_decls (tree newdecl, tree olddecl, bool TYPE_ARG_TYPES (TREE_TYPE (olddecl)) = oldargs; } } + /* Likewise for const struct tm*. */ + else if (TREE_VALUE (t2) == const_tm_ptr_type_node) + { + tree t = TREE_VALUE (t1); + + if (TYPE_PTR_P (t) + && TYPE_IDENTIFIER (TREE_TYPE (t)) + == get_identifier ("tm") + && compparms (TREE_CHAIN (t1), TREE_CHAIN (t2))) + { + tree oldargs = TYPE_ARG_TYPES (TREE_TYPE (olddecl)); + + TYPE_ARG_TYPES (TREE_TYPE (olddecl)) + = TYPE_ARG_TYPES (TREE_TYPE (newdecl)); + types_match = decls_match (newdecl, olddecl); + if (types_match) + return duplicate_decls (newdecl, olddecl, + newdecl_is_friend); + TYPE_ARG_TYPES (TREE_TYPE (olddecl)) = oldargs; + } + } else if (! same_type_p (TREE_VALUE (t1), TREE_VALUE (t2))) break; + if (t1 || t2 + || ! same_type_p (TREE_TYPE (TREE_TYPE (olddecl)), + TREE_TYPE (TREE_TYPE (newdecl)))) + warning_at (DECL_SOURCE_LOCATION (newdecl), + OPT_Wbuiltin_function_redefined, + "declaration of %q+#D conflicts with built-in " + "declaration %q#D", newdecl, olddecl); } else if ((DECL_EXTERN_C_P (newdecl) && DECL_EXTERN_C_P (olddecl)) @@ -1553,7 +1588,7 @@ duplicate_decls (tree newdecl, tree olddecl, bool /* Whether or not the builtin can throw exceptions has no bearing on this declarator. */ - TREE_NOTHROW (olddecl) = 0; + TREE_NOTHROW (olddecl) = TREE_NOTHROW (newdecl); if (DECL_THIS_STATIC (newdecl) && !DECL_THIS_STATIC (olddecl)) { Index: gcc/doc/invoke.texi =================================================================== --- gcc/doc/invoke.texi (revision 240799) +++ gcc/doc/invoke.texi (working copy) @@ -256,7 +256,7 @@ Objective-C and Objective-C++ Dialects}. -pedantic-errors @gol -w -Wextra -Wall -Waddress -Waggregate-return @gol -Wno-aggressive-loop-optimizations -Warray-bounds -Warray-bounds=@var{n} @gol --Wno-attributes -Wbool-compare -Wbool-operation @gol +-Wno-attributes -Wbool-compare -Wbool-operation -Wbuiltin-function-redefined @gol -Wno-builtin-macro-redefined -Wc90-c99-compat -Wc99-c11-compat @gol -Wc++-compat -Wc++11-compat -Wc++14-compat -Wcast-align -Wcast-qual @gol -Wchar-subscripts -Wclobbered -Wcomment -Wconditionally-supported @gol @@ -3668,6 +3668,7 @@ Options} and @ref{Objective-C and Objective-C++ Di -Warray-bounds=1 @r{(only with} @option{-O2}@r{)} @gol -Wbool-compare @gol -Wbool-operation @gol +-Wbuiltin-function-redefined @r{(C++ and Objective-C++ only)} @gol -Wc++11-compat -Wc++14-compat@gol -Wchar-subscripts @gol -Wcomment @gol @@ -5730,6 +5731,13 @@ unrecognized attributes, function attributes appli etc. This does not stop errors for incorrect use of supported attributes. +@item -Wbuiltin-function-redefined @r{(C++ and Objective-C++ only)} +@opindex Wbuiltin-function-redefined +@opindex Wno-builtin-function-redefined +Do warn if built-in functions are redefined. This option is only +supported for C++ and Objective-C++. It is implied by @option{-Wall}, +which can be disabled with @option{-Wno-builtin-function-redefined}. + @item -Wno-builtin-macro-redefined @opindex Wno-builtin-macro-redefined @opindex Wbuiltin-macro-redefined Index: gcc/lto/lto-lang.c =================================================================== --- gcc/lto/lto-lang.c (revision 240799) +++ gcc/lto/lto-lang.c (working copy) @@ -1266,6 +1266,10 @@ lto_init (void) always use the C definition here in lto1. */ gcc_assert (fileptr_type_node == ptr_type_node); gcc_assert (TYPE_MAIN_VARIANT (fileptr_type_node) == ptr_type_node); + /* Likewise for const struct tm*. */ + gcc_assert (const_tm_ptr_type_node == const_ptr_type_node); + gcc_assert (TYPE_MAIN_VARIANT (const_tm_ptr_type_node) + == const_ptr_type_node); ptrdiff_type_node = integer_type_node; Index: gcc/testsuite/g++.dg/lookup/extern-c-redecl4.C =================================================================== --- gcc/testsuite/g++.dg/lookup/extern-c-redecl4.C (revision 240799) +++ gcc/testsuite/g++.dg/lookup/extern-c-redecl4.C (working copy) @@ -3,7 +3,6 @@ // { dg-options "" } // { dg-do compile } -// { dg-final { scan-assembler "call\[\t \]+\[^\$\]*?_Z4forkv" { target i?86-*-* x86_64-*-* } } } class frok { @@ -14,5 +13,5 @@ class frok void foo () { - fork (); + fork (); // { dg-error "was not declared in this scope" } } Index: gcc/testsuite/g++.dg/pr71973-1.C =================================================================== --- gcc/testsuite/g++.dg/pr71973-1.C (revision 0) +++ gcc/testsuite/g++.dg/pr71973-1.C (working copy) @@ -0,0 +1,14 @@ +// { dg-do compile } +// { dg-options "-Wall -fdump-tree-eh" } + +extern "C" +void fork () // { dg-warning "conflicts with built-in declaration" } +__attribute__ ((__nothrow__)); + +void foo () throw () +{ + fork (); +} + +// { dg-final { scan-tree-dump-not "eh_dispatch" "eh" } } +// { dg-final { scan-tree-dump-not "resx" "eh" } } Index: gcc/testsuite/g++.dg/pr71973-2.C =================================================================== --- gcc/testsuite/g++.dg/pr71973-2.C (revision 0) +++ gcc/testsuite/g++.dg/pr71973-2.C (working copy) @@ -0,0 +1,18 @@ +// { dg-do compile } +// { dg-options "-Wall -fdump-tree-eh" } + +typedef __SIZE_TYPE__ size_t; +struct tm; + +extern "C" +size_t strftime (char*, size_t, const char*, const struct tm*) +__attribute__ ((__nothrow__)); + +void foo () throw () +{ + strftime (0,0,0,0); // { dg-warning "null argument where non-null required" } + // { dg-warning "too many arguments for format" "" { target *-*-* } .-1 } +} + +// { dg-final { scan-tree-dump-not "eh_dispatch" "eh" } } +// { dg-final { scan-tree-dump-not "resx" "eh" } } Index: gcc/tree-core.h =================================================================== --- gcc/tree-core.h (revision 240799) +++ gcc/tree-core.h (working copy) @@ -612,6 +612,7 @@ enum tree_index { TI_VA_LIST_FPR_COUNTER_FIELD, TI_BOOLEAN_TYPE, TI_FILEPTR_TYPE, + TI_CONST_TM_PTR_TYPE, TI_POINTER_SIZED_TYPE, TI_POINTER_BOUNDS_TYPE, Index: gcc/tree.c =================================================================== --- gcc/tree.c (revision 240799) +++ gcc/tree.c (working copy) @@ -6007,6 +6007,7 @@ free_lang_data (void) /* Create gimple variants for common types. */ ptrdiff_type_node = integer_type_node; fileptr_type_node = ptr_type_node; + const_tm_ptr_type_node = const_ptr_type_node; /* Reset some langhooks. Do not reset types_compatible_p, it may still be used indirectly via the get_alias_set langhook. */ @@ -10311,6 +10312,7 @@ build_common_tree_nodes (bool signed_char) const_ptr_type_node = build_pointer_type (build_type_variant (void_type_node, 1, 0)); fileptr_type_node = ptr_type_node; + const_tm_ptr_type_node = const_ptr_type_node; pointer_sized_int_node = build_nonstandard_integer_type (POINTER_SIZE, 1); Index: gcc/tree.h =================================================================== --- gcc/tree.h (revision 240799) +++ gcc/tree.h (working copy) @@ -3657,6 +3657,8 @@ tree_operand_check_code (const_tree __t, enum tree #define va_list_fpr_counter_field global_trees[TI_VA_LIST_FPR_COUNTER_FIELD] /* The C type `FILE *'. */ #define fileptr_type_node global_trees[TI_FILEPTR_TYPE] +/* The C type `const struct tm *'. */ +#define const_tm_ptr_type_node global_trees[TI_CONST_TM_PTR_TYPE] #define pointer_sized_int_node global_trees[TI_POINTER_SIZED_TYPE] #define boolean_type_node global_trees[TI_BOOLEAN_TYPE]
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. The reg-test also revealed a test case that is IMO invalid, and depends on the incorrect merging of a builtin definition with a friend function see testsuite/g++.dg/lookup/extern-c-redecl4.C: This test case does no longer compile, because the builtin fails to have an explicit declaration. Bootstrap and reg-testing on x86_64-pc-linux-gnu. OK for trunk? Thanks Bernd. gcc: 2016-10-06 Bernd Edlinger <bernd.edlinger@hotmail.de> PR c++/71973 * doc/invoke.texi: Document -Wbuiltin-function-redefined. * builtin-types.def (BT_CONST_TM_PTR): New primitive type. (BT_FN_SIZE_STRING_SIZE_CONST_STRING_CONST_PTR): Removed. (BT_FN_SIZE_STRING_SIZE_CONST_STRING_CONST_TM_PTR): New function type. * builtins.def (strftime): Update builtin function. * tree-core.h (TI_CONST_TM_PTR_TYPE): New enum value. * tree.h (const_tm_ptr_type_node): New type node. * tree.c (free_lang_data, build_common_tree_nodes): Initialize const_tm_ptr_type_node. c-family: 2016-10-06 Bernd Edlinger <bernd.edlinger@hotmail.de> PR c++/71973 * c.opt (Wbuiltin-function-redefined): New warning. * c-common.c (c_common_nodes_and_builtins): Initialize const_tm_ptr_type_node. cp: 2016-10-06 Bernd Edlinger <bernd.edlinger@hotmail.de> PR c++/71973 * decl.c (duplicate_decls): Warn when a built-in function is redefined. Don't overload builtin functions with C++ functions. Handle const_tm_ptr_type_node like file_ptr_node. Copy the TREE_NOTHROW flag unmodified to the old decl. lto: 2016-10-06 Bernd Edlinger <bernd.edlinger@hotmail.de> PR c++/71973 * lto-lang.c (lto_init): Assert const_tm_ptr_type_node is sane. testsuite: 2016-10-06 Bernd Edlinger <bernd.edlinger@hotmail.de> PR c++/71973 * g++.dg/pr71973-1.C: New test. * g++.dg/pr71973-2.C: New test. * g++.dg/lookup/extern-c-redecl4.C: Adjust test expectations.