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

[Jason Merrill <jason@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 4988 bytes --]

On 11/02/2016 02:11 AM, Bernd Edlinger wrote:
> On 11/01/16 19:15, Bernd Edlinger wrote:
>> On 11/01/16 18:11, Jason Merrill wrote:
>>> On Tue, Nov 1, 2016 at 11:45 AM, Bernd Edlinger
>>> <bernd.edlinger@hotmail.de> wrote:
>>>> On 11/01/16 16:20, Jason Merrill wrote:
>>>>> On 10/17/2016 03:18 PM, Bernd Edlinger wrote:
>>>>> I'm not even sure we need a new warning.  Can we combine this warning
>>>>> with the block that currently follows?
>>>>
>>>> After 20 years of not having a warning on that,
>>>> an implicitly enabled warning would at least break lots of bogus
>>>> test cases.
>>>
>>> Would it, though?  Which test cases still break with the current patch?
>>
>> Less than before, but there are still at least a few of them.
>>
>> I can make a list and send it tomorrow.
>
> FAIL: g++.dg/cpp1y/lambda-generic-udt.C  -std=gnu++14 (test for excess
> errors)
> FAIL: g++.dg/cpp1y/lambda-generic-xudt.C  -std=gnu++14 (test for excess
> errors)
> FAIL: g++.dg/init/new15.C  -std=c++11 (test for excess errors)
> FAIL: g++.dg/init/new15.C  -std=c++14 (test for excess errors)
> FAIL: g++.dg/init/new15.C  -std=c++98 (test for excess errors)
> FAIL: g++.dg/ipa/inline-1.C  -std=gnu++11 (test for excess errors)
> FAIL: g++.dg/ipa/inline-1.C  -std=gnu++14 (test for excess errors)
> FAIL: g++.dg/ipa/inline-1.C  -std=gnu++98 (test for excess errors)
> FAIL: g++.dg/ipa/inline-2.C  -std=gnu++11 (test for excess errors)
> FAIL: g++.dg/ipa/inline-2.C  -std=gnu++14 (test for excess errors)
> FAIL: g++.dg/ipa/inline-2.C  -std=gnu++98 (test for excess errors)
> FAIL: g++.dg/tc1/dr20.C  -std=c++11 (test for excess errors)
> FAIL: g++.dg/tc1/dr20.C  -std=c++14 (test for excess errors)
> FAIL: g++.dg/tc1/dr20.C  -std=c++98 (test for excess errors)
> FAIL: g++.dg/tree-ssa/inline-1.C  -std=gnu++11 (test for excess errors)
> FAIL: g++.dg/tree-ssa/inline-1.C  -std=gnu++14 (test for excess errors)
> FAIL: g++.dg/tree-ssa/inline-1.C  -std=gnu++98 (test for excess errors)
> FAIL: g++.dg/tree-ssa/inline-2.C  -std=gnu++11 (test for excess errors)
> FAIL: g++.dg/tree-ssa/inline-2.C  -std=gnu++14 (test for excess errors)
> FAIL: g++.dg/tree-ssa/inline-2.C  -std=gnu++98 (test for excess errors)
> FAIL: g++.dg/lto/20080908-1 cp_lto_20080908-1_0.o assemble, -O0 -flto
> -flto-partition=1to1 -fno-use-linker-plugin
> FAIL: g++.dg/lto/20080908-1 cp_lto_20080908-1_0.o assemble, -O0 -flto
> -flto-partition=none -fuse-linker-plugin
> FAIL: g++.dg/lto/20080908-1 cp_lto_20080908-1_0.o assemble, -O0 -flto
> -fuse-linker-plugin -fno-fat-lto-objects
> FAIL: g++.dg/lto/20080908-1 cp_lto_20080908-1_0.o assemble, -O2 -flto
> -flto-partition=1to1 -fno-use-linker-plugin
> FAIL: g++.dg/lto/20080908-1 cp_lto_20080908-1_0.o assemble, -O2 -flto
> -flto-partition=none -fuse-linker-plugin -fno-fat-lto-objects
> FAIL: g++.dg/lto/20080908-1 cp_lto_20080908-1_0.o assemble, -O2 -flto
> -fuse-linker-plugin
> FAIL: g++.dg/lto/pr68811 cp_lto_pr68811_0.o assemble, -O2
> FAIL: g++.old-deja/g++.law/except1.C  -std=gnu++11 (test for excess errors)
> FAIL: g++.old-deja/g++.law/except1.C  -std=gnu++14 (test for excess errors)
> FAIL: g++.old-deja/g++.law/except1.C  -std=gnu++98 (test for excess errors)
> FAIL: g++.old-deja/g++.mike/p700.C  -std=gnu++11 (test for excess errors)
> FAIL: g++.old-deja/g++.mike/p700.C  -std=gnu++14 (test for excess errors)
> FAIL: g++.old-deja/g++.mike/p700.C  -std=gnu++98 (test for excess errors)
> FAIL: g++.old-deja/g++.other/builtins10.C  -std=c++11 (test for excess
> errors)
> FAIL: g++.old-deja/g++.other/builtins10.C  -std=c++14 (test for excess
> errors)
> FAIL: g++.old-deja/g++.other/realloc.C  -std=c++11 (test for excess errors)
> FAIL: g++.old-deja/g++.other/realloc.C  -std=c++14 (test for excess errors)
> FAIL: g++.old-deja/g++.other/realloc.C  -std=c++98 (test for excess errors)
> FAIL: g++.old-deja/g++.other/vbase5.C  -std=c++11 (test for excess errors)
> FAIL: g++.old-deja/g++.other/vbase5.C  -std=c++14 (test for excess errors)
> FAIL: g++.old-deja/g++.other/vbase5.C  -std=c++98 (test for excess errors)
>
>
> The lto test case does emit the warning when assembling, but
> it still produces an executable and even executes it.
>
> Also g++.dg/cpp1y/lambda-generic-udt.C, g++.dg/tc1/dr20.C
> and g++.old-deja/g++.other/vbase5.C are execution tests.
>
> So I was wrong to assume these were all compile-only tests.
>
> I think that list should be fixable, if we decide to enable
> the warning by default.

Yes, either by fixing the prototypes or disabling the warning.

>> If we remove the DECL_ANTICIPATED check, I see some failures in
>> builtin* tests due to missing extern "C".  That seems appropriate at
>> file scope, but I'm not sure it's right for namespace std.
>
> Interesting, what have you done?

The attached.  But now that you've found that the existing warning deals 
with people doing silly things like redeclaring __builtin_* I guess 
let's leave it alone, and add the warning to the DECL_ANTICIPATED block 
the way you proposed.

Jason


[-- Attachment #2: rem-ant.diff --]
[-- Type: text/x-patch, Size: 1413 bytes --]

commit d93d421435f8c38b2019526c7645a59e79a92cc5
Author: Jason Merrill <jason@redhat.com>
Date:   Tue Nov 1 17:16:12 2016 -0400

    rem-ant

diff --git a/gcc/cp/decl.c b/gcc/cp/decl.c
index ecf4d14..963087d 100644
--- a/gcc/cp/decl.c
+++ b/gcc/cp/decl.c
@@ -1485,10 +1485,6 @@ duplicate_decls (tree newdecl, tree olddecl, bool newdecl_is_friend)
 	}
       else if (!types_match)
 	{
-	  /* Avoid warnings redeclaring built-ins which have not been
-	     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;
@@ -1521,8 +1517,8 @@ duplicate_decls (tree newdecl, tree olddecl, bool newdecl_is_friend)
 	      }
 	    else if (! same_type_p (TREE_VALUE (t1), TREE_VALUE (t2)))
 	      break;
-	    }
-	  else if ((DECL_EXTERN_C_P (newdecl)
+
+	  if ((DECL_EXTERN_C_P (newdecl)
 	       && DECL_EXTERN_C_P (olddecl))
 	      || compparms (TYPE_ARG_TYPES (TREE_TYPE (newdecl)),
 			    TYPE_ARG_TYPES (TREE_TYPE (olddecl))))
@@ -1540,7 +1536,7 @@ duplicate_decls (tree newdecl, tree olddecl, bool newdecl_is_friend)
                          : G_("shadowing library function %q#D"), olddecl);
 	    }
 	  else
-	    /* Discard the old built-in function.  */
+	    /* Not a duplicate.  */
 	    return NULL_TREE;
 
 	  /* Replace the old RTL to avoid problems with inlining.  */