This is the mail archive of the
gcc-patches@gcc.gnu.org
mailing list for the GCC project.
Re: [PATCH, C++] Warn on redefinition of builtin functions (PR c++/71973)
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.