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

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

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.

Attachment: changelog-pr71973.txt
Description: changelog-pr71973.txt

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]