tighten up -Wbuiltin-declaration-mismatch (PR 86125)

Message ID a82097ad-5c7d-e718-90c1-66ba6371478b@gmail.com
State New
Headers show
Series
  • tighten up -Wbuiltin-declaration-mismatch (PR 86125)
Related show

Commit Message

Martin Sebor June 13, 2018, 10:34 p.m.
The C implementation of the -Wbuiltin-declaration-mismatch
warning relies on TYPE_MODE to detect incompatibilities
between return and argument types in user declarations of
built-in functions.  That prevents mistakes like

   char* strlen (const char*);

from being detected (where sizeof (char*) == sizeof (size_t)),
while at the same diagnosing similar bugs such as

   char* strcmp (const char*, const char*);

where sizeof (char*) != sizeof (int), and always diagnosing
benign declarations like:

   void strcpy (char*, const char*)

The attached patch tightens up the detection of incompatible
types so that when -Wextra is set it diagnoses more of these
kinds of problems, including mismatches in qualifiers.  (I
added this under -Wextra mainly to avoid the warning with
just -Wall for some of the more benign incompatibilities
like those in const-qualifiers).

This enhancement was prompted by bug 86114.  As it is, it
would not have prevented the ICE in that bug because it does
not reject the incompatible redeclaration (I did that to keep
compatibility with prior GCC versions).  If there is support
for it, though, I think rejecting all incompatible declarations
would be a better solution.  Partly because the middle-end
doesn't seem to fully consider incompatible return types and
so might end up introducing transformations that don't make
sense.  And partly because I think at least the C and POSIX
standard built-in functions should be declared with
the types they are specified.

Martin

Comments

Richard Biener June 14, 2018, 1:36 p.m. | #1
On Thu, Jun 14, 2018 at 12:35 AM Martin Sebor <msebor@gmail.com> wrote:
>
> The C implementation of the -Wbuiltin-declaration-mismatch
> warning relies on TYPE_MODE to detect incompatibilities
> between return and argument types in user declarations of
> built-in functions.  That prevents mistakes like
>
>    char* strlen (const char*);
>
> from being detected (where sizeof (char*) == sizeof (size_t)),
> while at the same diagnosing similar bugs such as
>
>    char* strcmp (const char*, const char*);
>
> where sizeof (char*) != sizeof (int), and always diagnosing
> benign declarations like:
>
>    void strcpy (char*, const char*)
>
> The attached patch tightens up the detection of incompatible
> types so that when -Wextra is set it diagnoses more of these
> kinds of problems, including mismatches in qualifiers.  (I
> added this under -Wextra mainly to avoid the warning with
> just -Wall for some of the more benign incompatibilities
> like those in const-qualifiers).
>
> This enhancement was prompted by bug 86114.  As it is, it
> would not have prevented the ICE in that bug because it does
> not reject the incompatible redeclaration (I did that to keep
> compatibility with prior GCC versions).  If there is support
> for it, though, I think rejecting all incompatible declarations
> would be a better solution.  Partly because the middle-end
> doesn't seem to fully consider incompatible return types and
> so might end up introducing transformations that don't make
> sense.  And partly because I think at least the C and POSIX
> standard built-in functions should be declared with
> the types they are specified.

Why not go the simpler way of doing

  || POINTER_TYPE_P (oldrettype) != POINTER_TYPE_P (newrettype)

after the mode check?  I don't think a mismatch in pointer vs.
non-pointer was a desired loophole.

Richard.

> Martin
Martin Sebor June 14, 2018, 3:34 p.m. | #2
On 06/14/2018 07:36 AM, Richard Biener wrote:
> On Thu, Jun 14, 2018 at 12:35 AM Martin Sebor <msebor@gmail.com> wrote:
>>
>> The C implementation of the -Wbuiltin-declaration-mismatch
>> warning relies on TYPE_MODE to detect incompatibilities
>> between return and argument types in user declarations of
>> built-in functions.  That prevents mistakes like
>>
>>    char* strlen (const char*);
>>
>> from being detected (where sizeof (char*) == sizeof (size_t)),
>> while at the same diagnosing similar bugs such as
>>
>>    char* strcmp (const char*, const char*);
>>
>> where sizeof (char*) != sizeof (int), and always diagnosing
>> benign declarations like:
>>
>>    void strcpy (char*, const char*)
>>
>> The attached patch tightens up the detection of incompatible
>> types so that when -Wextra is set it diagnoses more of these
>> kinds of problems, including mismatches in qualifiers.  (I
>> added this under -Wextra mainly to avoid the warning with
>> just -Wall for some of the more benign incompatibilities
>> like those in const-qualifiers).
>>
>> This enhancement was prompted by bug 86114.  As it is, it
>> would not have prevented the ICE in that bug because it does
>> not reject the incompatible redeclaration (I did that to keep
>> compatibility with prior GCC versions).  If there is support
>> for it, though, I think rejecting all incompatible declarations
>> would be a better solution.  Partly because the middle-end
>> doesn't seem to fully consider incompatible return types and
>> so might end up introducing transformations that don't make
>> sense.  And partly because I think at least the C and POSIX
>> standard built-in functions should be declared with
>> the types they are specified.
>
> Why not go the simpler way of doing
>
>   || POINTER_TYPE_P (oldrettype) != POINTER_TYPE_P (newrettype)
>
> after the mode check?  I don't think a mismatch in pointer vs.
> non-pointer was a desired loophole.

Yes, I don't either.

But detecting only pointer/integer mismatches would still allow
declaring built-ins with incompatible pointer types (e.g., strlen
(void*)), or do anything to remove any of the other warts and
inconsistencies.

As I showed above, the warning allows certain questionable
mismatches (even beyond pointers/integers) while diagnosing
entirely benign ones.  For instance, declaring snprintf that
returns a struct the size of an int is silently accepted
(regardless of its members) but declaring it void is.

Besides finding the most egregious mistakes I would like
the warning to prompt users to declare standard built-ins
with the exact types they are specified with, including const
qualifiers.  It's a gap even in C's weak type system to let
programs that (accidentally or otherwise) declare built-ins
with incompatible types to do conversions that GCC otherwise
takes pains to diagnose (e.g., -Wformat warning about passing
non-char* to %s, or types other than int to %i, etc).

There's a comment in the code about the weak checking being
deliberate (the word harmless here suggests the author may
not have fully appreciated all the conversions it allows):

	  /* Accept harmless mismatch in function types.
	     This is for the ffs and fprintf builtins.  */

If you concerned about the change causing trouble in this
context, then if there are legacy systems that GCC still
supports that declare standard functions with non-standard
signatures they should be taken into consideration somehow.
Maybe by having the warning for them depend on the language
conformance mode (-std=mode).  Since the comment only talks
about ffs and fprintf, perhaps the checking should be relaxed
only for those (and only for the legacy targets).  But
I haven't thought too hard about this and there may be better
solutions if this is something we need to worry about.

Martin
Joseph Myers June 14, 2018, 7:33 p.m. | #3
On Thu, 14 Jun 2018, Martin Sebor wrote:

> There's a comment in the code about the weak checking being
> deliberate (the word harmless here suggests the author may
> not have fully appreciated all the conversions it allows):
> 
> 	  /* Accept harmless mismatch in function types.
> 	     This is for the ffs and fprintf builtins.  */
> 
> If you concerned about the change causing trouble in this
> context, then if there are legacy systems that GCC still
> supports that declare standard functions with non-standard
> signatures they should be taken into consideration somehow.

For fprintf and various related functions, you need to allow for the 
built-in function being declared without the compiler knowing what the 
FILE type will end up being a typedef for - i.e., you can't avoid having 
some special case somewhere for built-in functions using 
fileptr_type_node, if nothing else.

For other functions, the issue as per 
<https://gcc.gnu.org/ml/gcc-patches/2001-04/msg00435.html> may well have 
been bad types in some system headers such as "char *sprintf();".  To the 
extent that any such systems with bad (but ABI-compatible) types for some 
functions in system headers are still supported as GCC targets, I think it 
would be reasonable to say that fixincludes, not extra laxity in built-in 
function types, is the way to deal with the bad types in headers.
Martin Sebor June 14, 2018, 8:32 p.m. | #4
On 06/14/2018 01:33 PM, Joseph Myers wrote:
> On Thu, 14 Jun 2018, Martin Sebor wrote:
>
>> There's a comment in the code about the weak checking being
>> deliberate (the word harmless here suggests the author may
>> not have fully appreciated all the conversions it allows):
>>
>> 	  /* Accept harmless mismatch in function types.
>> 	     This is for the ffs and fprintf builtins.  */
>>
>> If you concerned about the change causing trouble in this
>> context, then if there are legacy systems that GCC still
>> supports that declare standard functions with non-standard
>> signatures they should be taken into consideration somehow.
>
> For fprintf and various related functions, you need to allow for the
> built-in function being declared without the compiler knowing what the
> FILE type will end up being a typedef for - i.e., you can't avoid having
> some special case somewhere for built-in functions using
> fileptr_type_node, if nothing else.

Hmm, I thought fileptr_type_node was a node for FILE*, but it's
actually the same as ptr_type_node, i.e., void*, so the built-in
fprintf expects a void* argument (and declaring it to take a FILE*
triggers the new warning).  That seems odd.  What purpose does it
serve?  (AFAICS, it's not used anywhere except jit).

I would expect fileptr_type_node to be set to correspond to
FILE* once FILE is declared.  Why is that not done?

> For other functions, the issue as per
> <https://gcc.gnu.org/ml/gcc-patches/2001-04/msg00435.html> may well have
> been bad types in some system headers such as "char *sprintf();".  To the
> extent that any such systems with bad (but ABI-compatible) types for some
> functions in system headers are still supported as GCC targets, I think it
> would be reasonable to say that fixincludes, not extra laxity in built-in
> function types, is the way to deal with the bad types in headers.

Sounds good.

Martin
Joseph Myers June 14, 2018, 8:37 p.m. | #5
On Thu, 14 Jun 2018, Martin Sebor wrote:

> Hmm, I thought fileptr_type_node was a node for FILE*, but it's
> actually the same as ptr_type_node, i.e., void*, so the built-in
> fprintf expects a void* argument (and declaring it to take a FILE*
> triggers the new warning).  That seems odd.  What purpose does it
> serve?  (AFAICS, it's not used anywhere except jit).
> 
> I would expect fileptr_type_node to be set to correspond to
> FILE* once FILE is declared.  Why is that not done?

The built-in functions (which use fileptr_type_node via BT_FILEPTR in 
builtin-types.def) are defined long before any definition of FILE is 
known.  FILE may be a macro not just a typedef, and the user is free to 
have an unrelated definition of FILE if they don't include <stdio.h> or 
another standard header defining FILE in that translation unit.
Martin Sebor June 14, 2018, 9:38 p.m. | #6
On 06/14/2018 02:37 PM, Joseph Myers wrote:
> On Thu, 14 Jun 2018, Martin Sebor wrote:
>
>> Hmm, I thought fileptr_type_node was a node for FILE*, but it's
>> actually the same as ptr_type_node, i.e., void*, so the built-in
>> fprintf expects a void* argument (and declaring it to take a FILE*
>> triggers the new warning).  That seems odd.  What purpose does it
>> serve?  (AFAICS, it's not used anywhere except jit).
>>
>> I would expect fileptr_type_node to be set to correspond to
>> FILE* once FILE is declared.  Why is that not done?
>
> The built-in functions (which use fileptr_type_node via BT_FILEPTR in
> builtin-types.def) are defined long before any definition of FILE is
> known.  FILE may be a macro not just a typedef, and the user is free to
> have an unrelated definition of FILE if they don't include <stdio.h> or
> another standard header defining FILE in that translation unit.

Okay, thanks for the explanation.  I realize the built-ins
are declared before any other declarations have been seen
but I'm not sure I completely agree with everything else
you said.

The standard says FILE is a type so I don't think it can
be a macro that expands to something other than a type
named FILE.  The spec doesn't prevent programs from
#undef-ing FILE so implementations cannot define it as
one and rely on it staying defined(*).  So even if FILE
is a macro, it still (also) has to be an identifier of
a type with that name.

FILE can be a typedef for some other type, but I'm not sure
I see how that creates a problem.  fileptr_type_node could
refer to anything at all (even ptr_type_node) initially and
be set to refer to the first file-scope type or typedef
named FILE seen in a translation unit.  It shouldn't matter
if that happens to be a type that's unrelated to the stdio
FILE.  Is there something I'm missing that makes this approach
not feasible?  (If I'm wrong about FILE being a macro to some
unrelated name then that would sink this idea.)

Martin

[*] By referring to the first group of identifiers I assume
7.1.3, p3 refers to macros specified to be defined in standard
headers, i.e., bullet 3 on the list above.  The second group
of identifiers that can be defined as macros on that list (in
bullet 6) refers to other identifiers such as functions and
types, and those are not subject to paragraph 3.  Do you read
it differently?
Joseph Myers June 14, 2018, 10:52 p.m. | #7
On Thu, 14 Jun 2018, Martin Sebor wrote:

> The standard says FILE is a type so I don't think it can
> be a macro that expands to something other than a type
> named FILE.  The spec doesn't prevent programs from
> #undef-ing FILE so implementations cannot define it as
> one and rely on it staying defined(*).  So even if FILE
> is a macro, it still (also) has to be an identifier of
> a type with that name.

You can #undef it and then use FILE yourself as a block-scope identifier.  
I don't think it's at all clear that you can #undef it and then use FILE 
and expect to get the semantics of the standard type FILE (unlike for 
standard functions).

> FILE can be a typedef for some other type, but I'm not sure
> I see how that creates a problem.  fileptr_type_node could
> refer to anything at all (even ptr_type_node) initially and
> be set to refer to the first file-scope type or typedef
> named FILE seen in a translation unit.  It shouldn't matter
> if that happens to be a type that's unrelated to the stdio
> FILE.  Is there something I'm missing that makes this approach
> not feasible?  (If I'm wrong about FILE being a macro to some
> unrelated name then that would sink this idea.)

Setting the fileptr_type_node object to point to a different node would 
have no effect on the pointers to that particular node nested within 
various function types.  You'd need to modify lots of pointers within 
those function types, or reconstruct those function types and change the 
types of the built-in functions.
Martin Sebor June 14, 2018, 11:55 p.m. | #8
On 06/14/2018 04:52 PM, Joseph Myers wrote:
> On Thu, 14 Jun 2018, Martin Sebor wrote:
>
>> The standard says FILE is a type so I don't think it can
>> be a macro that expands to something other than a type
>> named FILE.  The spec doesn't prevent programs from
>> #undef-ing FILE so implementations cannot define it as
>> one and rely on it staying defined(*).  So even if FILE
>> is a macro, it still (also) has to be an identifier of
>> a type with that name.
>
> You can #undef it and then use FILE yourself as a block-scope identifier.
> I don't think it's at all clear that you can #undef it and then use FILE
> and expect to get the semantics of the standard type FILE (unlike for
> standard functions).

I'm not sure I know what you mean here.  Say we have:

   #include <stdio.h>
   #undef FILE

   void f (void)
   {
     extern FILE *fp;
     fprintf (fp, "");
   }

Are you saying it's not clear that this is well-defined?

>> FILE can be a typedef for some other type, but I'm not sure
>> I see how that creates a problem.  fileptr_type_node could
>> refer to anything at all (even ptr_type_node) initially and
>> be set to refer to the first file-scope type or typedef
>> named FILE seen in a translation unit.  It shouldn't matter
>> if that happens to be a type that's unrelated to the stdio
>> FILE.  Is there something I'm missing that makes this approach
>> not feasible?  (If I'm wrong about FILE being a macro to some
>> unrelated name then that would sink this idea.)
>
> Setting the fileptr_type_node object to point to a different node would
> have no effect on the pointers to that particular node nested within
> various function types.  You'd need to modify lots of pointers within
> those function types, or reconstruct those function types and change the
> types of the built-in functions.

Yeah, that sound like a lot of fiddling.  I suppose what
I should have said is that whatever fileptr_type_node pointed
to would be modified in place to refer to the new node.  I.e.,
initialize it to some placeholder kind of an object (a copy
of ptr_type_node like in the C++ FE) and modify the object
once a definition were seen.  Would that work?

Martin
Joseph Myers June 15, 2018, 8:37 p.m. | #9
On Thu, 14 Jun 2018, Martin Sebor wrote:

> > You can #undef it and then use FILE yourself as a block-scope identifier.
> > I don't think it's at all clear that you can #undef it and then use FILE
> > and expect to get the semantics of the standard type FILE (unlike for
> > standard functions).
> 
> I'm not sure I know what you mean here.  Say we have:
> 
>   #include <stdio.h>
>   #undef FILE
> 
>   void f (void)
>   {
>     extern FILE *fp;
>     fprintf (fp, "");
>   }
> 
> Are you saying it's not clear that this is well-defined?

Yes.

> > Setting the fileptr_type_node object to point to a different node would
> > have no effect on the pointers to that particular node nested within
> > various function types.  You'd need to modify lots of pointers within
> > those function types, or reconstruct those function types and change the
> > types of the built-in functions.
> 
> Yeah, that sound like a lot of fiddling.  I suppose what
> I should have said is that whatever fileptr_type_node pointed
> to would be modified in place to refer to the new node.  I.e.,
> initialize it to some placeholder kind of an object (a copy
> of ptr_type_node like in the C++ FE) and modify the object
> once a definition were seen.  Would that work?

Maybe - you'd have to watch out for interactions with type hashing etc. 
(modifying a type in place like that is risky because of the possibility 
it breaks something that was depending on the contents of the type).

Safer would be to have special cases for lax type comparisons for built-in 
functions that are limited to the case where one of the types is 
fileptr_type_node and the other is some other pointer-to-object type.
Martin Sebor June 15, 2018, 9:55 p.m. | #10
On 06/15/2018 02:37 PM, Joseph Myers wrote:
> On Thu, 14 Jun 2018, Martin Sebor wrote:
>
>>> You can #undef it and then use FILE yourself as a block-scope identifier.
>>> I don't think it's at all clear that you can #undef it and then use FILE
>>> and expect to get the semantics of the standard type FILE (unlike for
>>> standard functions).
>>
>> I'm not sure I know what you mean here.  Say we have:
>>
>>   #include <stdio.h>
>>   #undef FILE
>>
>>   void f (void)
>>   {
>>     extern FILE *fp;
>>     fprintf (fp, "");
>>   }
>>
>> Are you saying it's not clear that this is well-defined?
>
> Yes.

I'm still not sure I see why but after sleeping on it I've come
to see that FILE being a typedef makes it just as bad as if it
were a macro.  I.e., fprintf can be declared to take the underlying
type instead of FILE* and there's no way for GCC
to know that they're the same:

   struct _StdioFile;
   int fprintf (struct _StdioFile*, const char*, ...);
   typedef struct _StdioFile FILE;

>>> Setting the fileptr_type_node object to point to a different node would
>>> have no effect on the pointers to that particular node nested within
>>> various function types.  You'd need to modify lots of pointers within
>>> those function types, or reconstruct those function types and change the
>>> types of the built-in functions.
>>
>> Yeah, that sound like a lot of fiddling.  I suppose what
>> I should have said is that whatever fileptr_type_node pointed
>> to would be modified in place to refer to the new node.  I.e.,
>> initialize it to some placeholder kind of an object (a copy
>> of ptr_type_node like in the C++ FE) and modify the object
>> once a definition were seen.  Would that work?
>
> Maybe - you'd have to watch out for interactions with type hashing etc.
> (modifying a type in place like that is risky because of the possibility
> it breaks something that was depending on the contents of the type).
>
> Safer would be to have special cases for lax type comparisons for built-in
> functions that are limited to the case where one of the types is
> fileptr_type_node and the other is some other pointer-to-object type.

I agree.  I think the warning needs to allow for the first argument
to fprintf et al. to be any arbitrary object pointer type, including
void*.  Let me go with that.

Martin
Martin Sebor June 26, 2018, 11:32 p.m. | #11
Attached is an updated patch to tighten up the warning and also
prevent ICEs in the middle-end like in PR 86308 or PR 86202.

I took Richard's suggestion to add the POINTER_TYPE_P() check
to detect pointer/integer conflicts.  That also avoids the ICEs
above.

I also dealt with the fileptr_type_node problem so that file
I/O built-ins can be declared to take any object pointer type
as an argument, and that argument has to be the same for all
them.

I'm not too happy about the interaction with -Wextra but short
of enabling the stricter checks even without it or introducing
multiple levels for -Wbuiltin-declaration-mismatch I don't see
a good alternative.

Martin
PR c/86125 - missing -Wbuiltin-declaration-mismatch on a mismatched return type
PR middle-end/86308 - ICE in verify_gimple calling index() with an invalid declaration
PR middle-end/86202 - ICE in get_range_info calling an invalid memcpy() declaration

gcc/c/ChangeLog:

	PR c/86125
	PR middle-end/86202
	PR middle-end/86308
	* c-decl.c (match_builtin_function_types): Add arguments.
	(diagnose_mismatched_decls): Diagnose mismatched declarations
	of built-ins more strictly.
	* doc/invoke.texi (-Wbuiltin-declaration-mismatch): Update.

gcc/testsuite/ChangeLog:

	PR c/86125
	PR middle-end/86202
	PR middle-end/86308
	* gcc.dg/Wbuiltin-declaration-mismatch.c: New test.
	* gcc.dg/Wbuiltin-declaration-mismatch-2.c: New test.
	* gcc.dg/Wbuiltin-declaration-mismatch-3.c: New test.
	* gcc.dg/Wbuiltin-declaration-mismatch-4.c: New test.
	* gcc.dg/builtins-69.c: New test.

diff --git a/gcc/c/c-decl.c b/gcc/c/c-decl.c
index af16cfd..6c9e667 100644
--- a/gcc/c/c-decl.c
+++ b/gcc/c/c-decl.c
@@ -1628,43 +1628,82 @@ c_bind (location_t loc, tree decl, bool is_global)
   bind (DECL_NAME (decl), decl, scope, false, nested, loc);
 }
 
+
 /* Subroutine of compare_decls.  Allow harmless mismatches in return
    and argument types provided that the type modes match.  This function
-   return a unified type given a suitable match, and 0 otherwise.  */
+   returns a unified type given a suitable match, and 0 otherwise.  */
 
 static tree
-match_builtin_function_types (tree newtype, tree oldtype)
+match_builtin_function_types (tree newtype, tree oldtype,
+			      tree *strict, unsigned *argno)
 {
-  tree newrettype, oldrettype;
-  tree newargs, oldargs;
-  tree trytype, tryargs;
-
   /* Accept the return type of the new declaration if same modes.  */
-  oldrettype = TREE_TYPE (oldtype);
-  newrettype = TREE_TYPE (newtype);
+  tree oldrettype = TREE_TYPE (oldtype);
+  tree newrettype = TREE_TYPE (newtype);
+
+  *argno = 0;
+  *strict = NULL_TREE;
 
   if (TYPE_MODE (oldrettype) != TYPE_MODE (newrettype))
     return NULL_TREE;
 
-  oldargs = TYPE_ARG_TYPES (oldtype);
-  newargs = TYPE_ARG_TYPES (newtype);
-  tryargs = newargs;
+  if (!comptypes (oldrettype, newrettype))
+    *strict = oldrettype;
+
+  tree oldargs = TYPE_ARG_TYPES (oldtype);
+  tree newargs = TYPE_ARG_TYPES (newtype);
+  tree tryargs = newargs;
 
-  while (oldargs || newargs)
+  for (unsigned i = 1; oldargs || newargs; ++i)
     {
       if (!oldargs
 	  || !newargs
 	  || !TREE_VALUE (oldargs)
-	  || !TREE_VALUE (newargs)
-	  || TYPE_MODE (TREE_VALUE (oldargs))
-	     != TYPE_MODE (TREE_VALUE (newargs)))
+	  || !TREE_VALUE (newargs))
 	return NULL_TREE;
 
+      tree oldtype = TREE_VALUE (oldargs);
+      tree newtype = TREE_VALUE (newargs);
+
+      /* Fail for types with incompatible modes/sizes.  */
+      if (TYPE_MODE (TREE_VALUE (oldargs))
+	  != TYPE_MODE (TREE_VALUE (newargs)))
+	return NULL_TREE;
+
+      /* Fail for function and object pointer mismatches.  */
+      if (FUNCTION_POINTER_TYPE_P (oldtype) != FUNCTION_POINTER_TYPE_P (newtype)
+	  || POINTER_TYPE_P (oldtype) != POINTER_TYPE_P (newtype))
+	return NULL_TREE;
+
+      if (oldtype == fileptr_type_node)
+	{
+	  /* Store the first FILE* argument type seen (whatever it is),
+	     and expect any subsequent declarations of file I/O built-ins
+	     to refer to it rather than to fileptr_type_node which is just
+	     void*.  */
+	  static tree last_fileptr_type;
+	  if (last_fileptr_type)
+	    {
+	      if (!comptypes (last_fileptr_type, newtype))
+		{
+		  *argno = i;
+		  *strict = last_fileptr_type;
+		}
+	    }
+	  else
+	    last_fileptr_type = newtype;
+	}
+      else if (!*strict && !comptypes (oldtype, newtype))
+	{
+	  *argno = i;
+	  *strict = oldtype;
+	}
+
       oldargs = TREE_CHAIN (oldargs);
       newargs = TREE_CHAIN (newargs);
     }
 
-  trytype = build_function_type (newrettype, tryargs);
+  tree trytype = build_function_type (newrettype, tryargs);
 
   /* Allow declaration to change transaction_safe attribute.  */
   tree oldattrs = TYPE_ATTRIBUTES (oldtype);
@@ -1874,9 +1913,18 @@ diagnose_mismatched_decls (tree newdecl, tree olddecl,
       if (TREE_CODE (olddecl) == FUNCTION_DECL
 	  && DECL_BUILT_IN (olddecl) && !C_DECL_DECLARED_BUILTIN (olddecl))
 	{
-	  /* Accept harmless mismatch in function types.
-	     This is for the ffs and fprintf builtins.  */
-	  tree trytype = match_builtin_function_types (newtype, oldtype);
+	  /* Accept "harmless" mismatch in function types such as
+	     missing qualifiers or pointer vs same size integer
+	     mismatches.  This is for the ffs and fprintf builtins.
+	     However, with -Wextra in effect, diagnose return and
+	     argument types that are incompatible according to
+	     language rules. */
+	  tree mismatch_expect;
+	  unsigned mismatch_argno;
+
+	  tree trytype = match_builtin_function_types (newtype, oldtype,
+						       &mismatch_expect,
+						       &mismatch_argno);
 
 	  if (trytype && comptypes (newtype, trytype))
 	    *oldtypep = oldtype = trytype;
@@ -1885,11 +1933,30 @@ diagnose_mismatched_decls (tree newdecl, tree olddecl,
 	      /* If types don't match for a built-in, throw away the
 		 built-in.  No point in calling locate_old_decl here, it
 		 won't print anything.  */
-	      warning (OPT_Wbuiltin_declaration_mismatch,
-		       "conflicting types for built-in function %q+D",
-		       newdecl);
+	      warning_at (DECL_SOURCE_LOCATION (newdecl),
+			  OPT_Wbuiltin_declaration_mismatch,
+			  "conflicting types for built-in function %qD; "
+			  "expected %qT",
+			  newdecl, oldtype);
 	      return false;
 	    }
+
+	  if (mismatch_expect && extra_warnings)
+	    {
+	      /* If types match only loosely, print a warning but accept
+		 the redeclaration.  */
+	      location_t newloc = DECL_SOURCE_LOCATION (newdecl);
+	      if (mismatch_argno)
+		warning_at (newloc, OPT_Wbuiltin_declaration_mismatch,
+			    "mismatch in argument %u type of built-in "
+			    "function %qD; expected %qT",
+			    mismatch_argno, newdecl, mismatch_expect);
+	      else
+		warning_at (newloc, OPT_Wbuiltin_declaration_mismatch,
+			    "mismatch in return type of built-in "
+			    "function %qD; expected %qT",
+			    newdecl, mismatch_expect);
+	    }
 	}
       else if (TREE_CODE (olddecl) == FUNCTION_DECL
 	       && DECL_IS_BUILTIN (olddecl))
diff --git a/gcc/doc/invoke.texi b/gcc/doc/invoke.texi
index f231da3..6146e0e 100644
--- a/gcc/doc/invoke.texi
+++ b/gcc/doc/invoke.texi
@@ -6570,8 +6570,10 @@ attributes.
 @item -Wno-builtin-declaration-mismatch
 @opindex Wno-builtin-declaration-mismatch
 @opindex Wbuiltin-declaration-mismatch
-Warn if a built-in function is declared with the wrong signature or 
-as non-function.
+Warn if a built-in function is declared as a non-function or as a function
+with a grossly mismatched signature.  Subtle mismatches such as in pointer
+qualifiers or integer signedness are only diagnosed when @option{-Wextra}
+is enabled as well.
 This warning is enabled by default.
 
 @item -Wno-builtin-macro-redefined
diff --git a/gcc/testsuite/gcc.dg/Wbuiltin-declaration-mismatch-2.c b/gcc/testsuite/gcc.dg/Wbuiltin-declaration-mismatch-2.c
new file mode 100644
index 0000000..32af9fe
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/Wbuiltin-declaration-mismatch-2.c
@@ -0,0 +1,18 @@
+/* PR c/86125 - missing -Wbuiltin-declaration-mismatch on a mismatched
+   return type
+   Verify that declarations of file I/O built-ins with an arbitrary
+   object pointer do not trigger -Wbuiltin-declaration-mismatch.
+   { dg-do compile }
+   { dg-options "-Wbuiltin-declaration-mismatch -Wextra" } */
+
+typedef __SIZE_TYPE__ size_t;
+
+struct StdioFile;
+
+int fprintf (struct StdioFile*, const char*, ...);
+int vfprintf (struct StdioFile*, const char*, __builtin_va_list);
+int fputc (int, struct StdioFile*);
+int fputs (const char*, struct StdioFile*);
+int fscanf (struct StdioFile*, const char*, ...);
+int vfscanf (struct StdioFile*, const char*, __builtin_va_list);
+size_t fwrite (const void*, size_t, size_t, struct StdioFile*);
diff --git a/gcc/testsuite/gcc.dg/Wbuiltin-declaration-mismatch-3.c b/gcc/testsuite/gcc.dg/Wbuiltin-declaration-mismatch-3.c
new file mode 100644
index 0000000..77a4bff
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/Wbuiltin-declaration-mismatch-3.c
@@ -0,0 +1,26 @@
+/* PR c/86125 - missing -Wbuiltin-declaration-mismatch on a mismatched
+   return type
+   Verify that a declaration of vfprintf() with withe the wrong last
+   argument triggers -Wbuiltin-declaration-mismatch even without -Wextra.
+   { dg-do compile }
+   { dg-options "-Wbuiltin-declaration-mismatch" } */
+
+struct StdioFile;
+
+typedef __SIZE_TYPE__ size_t;
+
+struct StdioFile;
+
+int fprintf (struct StdioFile*, const char*);   /* { dg-warning "conflicting types for built-in function .fprintf.; expected .int\\\(\[a-z_\]+ \\\*, const char \\\*, \.\.\.\\\)." } */
+
+int vfprintf (struct StdioFile*, const char*, ...);   /* { dg-warning "conflicting types for built-in function .vfprintf.; expected .int\\\(\[a-z_\]+ \\\*, const char \\\*, \[a-z_\]+ \\\*\\\)." } */
+
+int fputc (char, struct StdioFile*);   /* { dg-warning "conflicting types for built-in function .fputc.; expected .int\\\(int,  void \\\*\\\)." } */
+
+size_t fputs (const char*, struct StdioFile*);   /* { dg-warning "conflicting types for built-in function .fputs.; expected .int\\\(const char \\\*, \[a-z_\]+ \\\*\\\)." } */
+
+int fscanf (struct StdioFile*, const char*, size_t, ...);   /* { dg-warning "conflicting types for built-in function .fscanf.; expected .int\\\(\[a-z_\]+ \\\*, const char \\\*, \.\.\.\\\)." } */
+
+int vfscanf (struct StdioFile*, const char*, ...);   /* { dg-warning "conflicting types for built-in function .vfscanf.; expected .int\\\(\[a-z_\]+ \\\*, const char \\\*, \[a-z_\]+ \\\*\\\)." } */
+
+size_t fwrite (const void*, size_t, size_t, struct StdioFile);    /* { dg-warning "conflicting types for built-in function .fwrite.; expected .\(long \)?unsigned int\\\(const void \\\*, \(long \)?unsigned int, *\(long \)?unsigned int, *\[a-z_\]+ \\\*\\\)." } */
diff --git a/gcc/testsuite/gcc.dg/Wbuiltin-declaration-mismatch-4.c b/gcc/testsuite/gcc.dg/Wbuiltin-declaration-mismatch-4.c
new file mode 100644
index 0000000..d06af6528
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/Wbuiltin-declaration-mismatch-4.c
@@ -0,0 +1,26 @@
+/* PR c/86125 - missing -Wbuiltin-declaration-mismatch on a mismatched
+   return type
+   Verify that declarations of file I/O built-ins with different
+   definitions of struct FILE triggers -Wbuiltin-declaration-mismatch
+   when -Wextra is specified.
+   { dg-do compile }
+   { dg-options "-Wall -Wbuiltin-declaration-mismatch" } */
+
+struct FooFile;
+int fputc (int, struct FooFile*);
+
+typedef struct FooFile AlsoFooFile;
+int fprintf (AlsoFooFile*, const char*, ...);
+
+typedef AlsoFooFile* FooFilePtr;
+int fscanf (FooFilePtr, const char*, ...);
+
+/* No warning here (-Wextra not specified).  */
+struct BarFile;
+int vfprintf (struct BarFile*, const char*, __builtin_va_list);
+
+
+/* Set -Wextra and verify -Wbuiltin-declaration-mismatch is issued.  */
+#pragma GCC diagnostic warning "-Wextra"
+
+int fputs (const char*, struct BarFile*);   /* { dg-warning "mismatch in argument 2 type of built-in function .fputs.; expected .struct FooFile \\\*." } */
diff --git a/gcc/testsuite/gcc.dg/Wbuiltin-declaration-mismatch.c b/gcc/testsuite/gcc.dg/Wbuiltin-declaration-mismatch.c
new file mode 100644
index 0000000..498876f
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/Wbuiltin-declaration-mismatch.c
@@ -0,0 +1,17 @@
+/* PR c/86125 - missing -Wbuiltin-declaration-mismatch on a mismatched
+   return type
+   { dg-do compile }
+   { dg-options "-Wbuiltin-declaration-mismatch -Wextra" } */
+
+char* strlen (const char*);   /* { dg-warning "mismatch in return type of built-in function .strlen.; expected .(long )?unsigned int." } */
+
+enum E { e };
+enum E strcmp (const char*, const char*);   /* { dg-warning "mismatch in return type of built-in function .strcmp.; expected .int." } */
+
+char* strcpy (char*, char*);   /* { dg-warning "mismatch in argument 2 type of built-in function .strcpy.; expected .const char \\*." } */
+
+char* strcat (const char*, char*);   /* { dg-warning "mismatch in argument 1 type of built-in function .strcat.; expected .char \\*." } */
+
+void strchr (const char*, int);      /* { dg-warning "conflicting types for built-in function .strchr.; expected .char \\*\\(const char \\*, int\\)." } */
+
+int fprintf (int, const char*, ...);   /* { dg-warning "conflicting types for built-in function .fprintf.; " } */ 
diff --git a/gcc/testsuite/gcc.dg/builtins-69.c b/gcc/testsuite/gcc.dg/builtins-69.c
new file mode 100644
index 0000000..26dfb3b
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/builtins-69.c
@@ -0,0 +1,22 @@
+/* PR middle-end/86308 - ICE in verify_gimple calling index() with
+   an invalid declaration
+   { dg-do compile }
+   { dg-options "-O2 -Wall" }  */
+
+int index (int, int);   /* { dg-warning "conflicting types for built-in function .index.; expected .char \\\*\\\(const char \\\*, int\\\)." } */
+
+int test_index (void)
+{
+  return index (0, 0);
+}
+
+
+/* PR middle-end/86202 - ICE in get_range_info calling an invalid memcpy()
+   declaration */
+
+void *memcpy (void *, void *, __SIZE_TYPE__ *);   /* { dg-warning "conflicting types for built-in function .memcpy.; expected .void \\\*\\\(void \\\*, const void \\\*, \(long \)?unsigned int\\\)." } */
+
+void test_memcpy (void *p, void *q, __SIZE_TYPE__ *r)
+{
+  memcpy (p, q, r);
+}
Joseph Myers June 27, 2018, 8:22 p.m. | #12
On Tue, 26 Jun 2018, Martin Sebor wrote:

> Attached is an updated patch to tighten up the warning and also
> prevent ICEs in the middle-end like in PR 86308 or PR 86202.

This patch adds two arguments to match_builtin_function_types, but doesn't 
update the comment on that function to define their semantics.
Jeff Law June 28, 2018, 5:20 a.m. | #13
On 06/26/2018 05:32 PM, Martin Sebor wrote:
> Attached is an updated patch to tighten up the warning and also
> prevent ICEs in the middle-end like in PR 86308 or PR 86202.
> 
> I took Richard's suggestion to add the POINTER_TYPE_P() check
> to detect pointer/integer conflicts.  That also avoids the ICEs
> above.
> 
> I also dealt with the fileptr_type_node problem so that file
> I/O built-ins can be declared to take any object pointer type
> as an argument, and that argument has to be the same for all
> them.
> 
> I'm not too happy about the interaction with -Wextra but short
> of enabling the stricter checks even without it or introducing
> multiple levels for -Wbuiltin-declaration-mismatch I don't see
> a good alternative.
> 
> Martin
> 
> gcc-86125.diff
> 
> 
> PR c/86125 - missing -Wbuiltin-declaration-mismatch on a mismatched return type
> PR middle-end/86308 - ICE in verify_gimple calling index() with an invalid declaration
> PR middle-end/86202 - ICE in get_range_info calling an invalid memcpy() declaration
> 
> gcc/c/ChangeLog:
> 
> 	PR c/86125
> 	PR middle-end/86202
> 	PR middle-end/86308
> 	* c-decl.c (match_builtin_function_types): Add arguments.
> 	(diagnose_mismatched_decls): Diagnose mismatched declarations
> 	of built-ins more strictly.
> 	* doc/invoke.texi (-Wbuiltin-declaration-mismatch): Update.
> 
> gcc/testsuite/ChangeLog:
> 
> 	PR c/86125
> 	PR middle-end/86202
> 	PR middle-end/86308
> 	* gcc.dg/Wbuiltin-declaration-mismatch.c: New test.
> 	* gcc.dg/Wbuiltin-declaration-mismatch-2.c: New test.
> 	* gcc.dg/Wbuiltin-declaration-mismatch-3.c: New test.
> 	* gcc.dg/Wbuiltin-declaration-mismatch-4.c: New test.
> 	* gcc.dg/builtins-69.c: New test.

[ ... ]

> 
> diff --git a/gcc/c/c-decl.c b/gcc/c/c-decl.c
> index af16cfd..6c9e667 100644
> --- a/gcc/c/c-decl.c
> +++ b/gcc/c/c-decl.c
> @@ -1628,43 +1628,82 @@ c_bind (location_t loc, tree decl, bool is_global)
>    bind (DECL_NAME (decl), decl, scope, false, nested, loc);
>  }
>  
> +
>  /* Subroutine of compare_decls.  Allow harmless mismatches in return
>     and argument types provided that the type modes match.  This function
> -   return a unified type given a suitable match, and 0 otherwise.  */
> +   returns a unified type given a suitable match, and 0 otherwise.  */
>  
>  static tree
> -match_builtin_function_types (tree newtype, tree oldtype)
> +match_builtin_function_types (tree newtype, tree oldtype,
> +			      tree *strict, unsigned *argno)
As Joseph notes, you need to update the function comment here.

[ ... ]

>> +	  /* Store the first FILE* argument type seen (whatever it is),
> +	     and expect any subsequent declarations of file I/O built-ins
> +	     to refer to it rather than to fileptr_type_node which is just
> +	     void*.  */
> +	  static tree last_fileptr_type;
Is this actually safe?  Isn't the type in GC memory?  And if so, what
prevents it from being GC'd?  At the least I think you need to register
this as a GC root.  Why are we handling fileptr_types specially here to
begin with?

jeff
Martin Sebor June 28, 2018, 3:14 p.m. | #14
On 06/27/2018 11:20 PM, Jeff Law wrote:
> On 06/26/2018 05:32 PM, Martin Sebor wrote:
>> Attached is an updated patch to tighten up the warning and also
>> prevent ICEs in the middle-end like in PR 86308 or PR 86202.
>>
>> I took Richard's suggestion to add the POINTER_TYPE_P() check
>> to detect pointer/integer conflicts.  That also avoids the ICEs
>> above.
>>
>> I also dealt with the fileptr_type_node problem so that file
>> I/O built-ins can be declared to take any object pointer type
>> as an argument, and that argument has to be the same for all
>> them.
>>
>> I'm not too happy about the interaction with -Wextra but short
>> of enabling the stricter checks even without it or introducing
>> multiple levels for -Wbuiltin-declaration-mismatch I don't see
>> a good alternative.
>>
>> Martin
>>
>> gcc-86125.diff
>>
>>
>> PR c/86125 - missing -Wbuiltin-declaration-mismatch on a mismatched return type
>> PR middle-end/86308 - ICE in verify_gimple calling index() with an invalid declaration
>> PR middle-end/86202 - ICE in get_range_info calling an invalid memcpy() declaration
>>
>> gcc/c/ChangeLog:
>>
>> 	PR c/86125
>> 	PR middle-end/86202
>> 	PR middle-end/86308
>> 	* c-decl.c (match_builtin_function_types): Add arguments.
>> 	(diagnose_mismatched_decls): Diagnose mismatched declarations
>> 	of built-ins more strictly.
>> 	* doc/invoke.texi (-Wbuiltin-declaration-mismatch): Update.
>>
>> gcc/testsuite/ChangeLog:
>>
>> 	PR c/86125
>> 	PR middle-end/86202
>> 	PR middle-end/86308
>> 	* gcc.dg/Wbuiltin-declaration-mismatch.c: New test.
>> 	* gcc.dg/Wbuiltin-declaration-mismatch-2.c: New test.
>> 	* gcc.dg/Wbuiltin-declaration-mismatch-3.c: New test.
>> 	* gcc.dg/Wbuiltin-declaration-mismatch-4.c: New test.
>> 	* gcc.dg/builtins-69.c: New test.
>
> [ ... ]
>
>>
>> diff --git a/gcc/c/c-decl.c b/gcc/c/c-decl.c
>> index af16cfd..6c9e667 100644
>> --- a/gcc/c/c-decl.c
>> +++ b/gcc/c/c-decl.c
>> @@ -1628,43 +1628,82 @@ c_bind (location_t loc, tree decl, bool is_global)
>>    bind (DECL_NAME (decl), decl, scope, false, nested, loc);
>>  }
>>
>> +
>>  /* Subroutine of compare_decls.  Allow harmless mismatches in return
>>     and argument types provided that the type modes match.  This function
>> -   return a unified type given a suitable match, and 0 otherwise.  */
>> +   returns a unified type given a suitable match, and 0 otherwise.  */
>>
>>  static tree
>> -match_builtin_function_types (tree newtype, tree oldtype)
>> +match_builtin_function_types (tree newtype, tree oldtype,
>> +			      tree *strict, unsigned *argno)
> As Joseph notes, you need to update the function comment here.
>
> [ ... ]
>
>>> +	  /* Store the first FILE* argument type seen (whatever it is),
>> +	     and expect any subsequent declarations of file I/O built-ins
>> +	     to refer to it rather than to fileptr_type_node which is just
>> +	     void*.  */
>> +	  static tree last_fileptr_type;
> Is this actually safe?  Isn't the type in GC memory?  And if so, what
> prevents it from being GC'd?  At the least I think you need to register
> this as a GC root.  Why are we handling fileptr_types specially here to
> begin with?

IIUC, garbage collection runs after front end processing (between
separate passes) so the node should not be freed while the front
end is holding on to it.  There are other examples in the FE of
similar static usage (e.g., in c-format.c).

The code detects mismatches between arguments to different file
I/O functions, as in:

   struct SomeFile;

   // okay, FILE is struct SomeFile
   int fputc (int, struct SomeFile*);

   struct OtherFile;
   int fputs (const char*, struct OtherFile*);   // warning

Martin
Jeff Law July 5, 2018, 8:22 p.m. | #15
On 06/28/2018 09:14 AM, Martin Sebor wrote:
> On 06/27/2018 11:20 PM, Jeff Law wrote:
>> On 06/26/2018 05:32 PM, Martin Sebor wrote:
>>> Attached is an updated patch to tighten up the warning and also
>>> prevent ICEs in the middle-end like in PR 86308 or PR 86202.
>>>
>>> I took Richard's suggestion to add the POINTER_TYPE_P() check
>>> to detect pointer/integer conflicts.  That also avoids the ICEs
>>> above.
>>>
>>> I also dealt with the fileptr_type_node problem so that file
>>> I/O built-ins can be declared to take any object pointer type
>>> as an argument, and that argument has to be the same for all
>>> them.
>>>
>>> I'm not too happy about the interaction with -Wextra but short
>>> of enabling the stricter checks even without it or introducing
>>> multiple levels for -Wbuiltin-declaration-mismatch I don't see
>>> a good alternative.
>>>
>>> Martin
>>>
>>> gcc-86125.diff
>>>
>>>
>>> PR c/86125 - missing -Wbuiltin-declaration-mismatch on a mismatched
>>> return type
>>> PR middle-end/86308 - ICE in verify_gimple calling index() with an
>>> invalid declaration
>>> PR middle-end/86202 - ICE in get_range_info calling an invalid
>>> memcpy() declaration
>>>
>>> gcc/c/ChangeLog:
>>>
>>>     PR c/86125
>>>     PR middle-end/86202
>>>     PR middle-end/86308
>>>     * c-decl.c (match_builtin_function_types): Add arguments.
>>>     (diagnose_mismatched_decls): Diagnose mismatched declarations
>>>     of built-ins more strictly.
>>>     * doc/invoke.texi (-Wbuiltin-declaration-mismatch): Update.
>>>
>>> gcc/testsuite/ChangeLog:
>>>
>>>     PR c/86125
>>>     PR middle-end/86202
>>>     PR middle-end/86308
>>>     * gcc.dg/Wbuiltin-declaration-mismatch.c: New test.
>>>     * gcc.dg/Wbuiltin-declaration-mismatch-2.c: New test.
>>>     * gcc.dg/Wbuiltin-declaration-mismatch-3.c: New test.
>>>     * gcc.dg/Wbuiltin-declaration-mismatch-4.c: New test.
>>>     * gcc.dg/builtins-69.c: New test.
>>
>> [ ... ]
>>
>>>
>>> diff --git a/gcc/c/c-decl.c b/gcc/c/c-decl.c
>>> index af16cfd..6c9e667 100644
>>> --- a/gcc/c/c-decl.c
>>> +++ b/gcc/c/c-decl.c
>>> @@ -1628,43 +1628,82 @@ c_bind (location_t loc, tree decl, bool
>>> is_global)
>>>    bind (DECL_NAME (decl), decl, scope, false, nested, loc);
>>>  }
>>>
>>> +
>>>  /* Subroutine of compare_decls.  Allow harmless mismatches in return
>>>     and argument types provided that the type modes match.  This
>>> function
>>> -   return a unified type given a suitable match, and 0 otherwise.  */
>>> +   returns a unified type given a suitable match, and 0 otherwise.  */
>>>
>>>  static tree
>>> -match_builtin_function_types (tree newtype, tree oldtype)
>>> +match_builtin_function_types (tree newtype, tree oldtype,
>>> +                  tree *strict, unsigned *argno)
>> As Joseph notes, you need to update the function comment here.
>>
>> [ ... ]
>>
>>>> +      /* Store the first FILE* argument type seen (whatever it is),
>>> +         and expect any subsequent declarations of file I/O built-ins
>>> +         to refer to it rather than to fileptr_type_node which is just
>>> +         void*.  */
>>> +      static tree last_fileptr_type;
>> Is this actually safe?  Isn't the type in GC memory?  And if so, what
>> prevents it from being GC'd?  At the least I think you need to register
>> this as a GC root.  Why are we handling fileptr_types specially here to
>> begin with?
> 
> IIUC, garbage collection runs after front end processing (between
> separate passes) so the node should not be freed while the front
> end is holding on to it.  There are other examples in the FE of
> similar static usage (e.g., in c-format.c).You've stuffed a potentially GC'd object into a static and that's going
to trigger a "is this correct/safe" discussion every time it's noticed :-)

Yes it's true that GC only happens at well known points and if an object
lives entirely in the front-end you can probably get away without the
GTY marker.  But then you have to actually prove there's nothing in the
middle/back ends that potentially call into this code.

I generally dislike that approach because it's bad from a long term
maintenance standpoint.  It's an implementation constraint that someone
has to remember forever to avoid hard to find bugs from being introduced.

Another way to help alleviate these concerns would be to assign the
object NULL once we're done parsing.

Or you can add a GTY marker.  There's a bit of overhead to this since
the GC system has to walk through all the registered roots.

Or you can conditionalize the code on some other variable which
indicates whether or not the parser is still running. "the_parser" might
be usable for this purpose.


> 
> The code detects mismatches between arguments to different file
> I/O functions, as in:
> 
>   struct SomeFile;
> 
>   // okay, FILE is struct SomeFile
>   int fputc (int, struct SomeFile*);
> 
>   struct OtherFile;
>   int fputs (const char*, struct OtherFile*);   // warning
I must be missing something.  What makes the first OK and the second not
OK?  ISTM they most both be a FILE *.

jeff

Patch

PR c/86125 - missing -Wbuiltin-declaration-mismatch on a mismatched return type

gcc/c/ChangeLog:

	PR c/86125
	* c-decl.c (match_builtin_function_types): Add arguments.
	(diagnose_mismatched_decls): Diagnose mismatched declarations
	of built-ins more strictly.

gcc/testsuite/ChangeLog:

	PR c/86125
	* gcc.dg/Wbuiltin-declaration-mismatch.c: New test.

Index: gcc/c/c-decl.c
===================================================================
--- gcc/c/c-decl.c	(revision 261551)
+++ gcc/c/c-decl.c	(working copy)
@@ -1630,41 +1630,54 @@  c_bind (location_t loc, tree decl, bool is_global)
 
 /* Subroutine of compare_decls.  Allow harmless mismatches in return
    and argument types provided that the type modes match.  This function
-   return a unified type given a suitable match, and 0 otherwise.  */
+   returns a unified type given a suitable match, and 0 otherwise.  */
 
 static tree
-match_builtin_function_types (tree newtype, tree oldtype)
+match_builtin_function_types (tree newtype, tree oldtype,
+			      tree *strict, unsigned *argno)
 {
-  tree newrettype, oldrettype;
-  tree newargs, oldargs;
-  tree trytype, tryargs;
-
   /* Accept the return type of the new declaration if same modes.  */
-  oldrettype = TREE_TYPE (oldtype);
-  newrettype = TREE_TYPE (newtype);
+  tree oldrettype = TREE_TYPE (oldtype);
+  tree newrettype = TREE_TYPE (newtype);
 
+  *argno = 0;
+  *strict = NULL_TREE;
+
   if (TYPE_MODE (oldrettype) != TYPE_MODE (newrettype))
     return NULL_TREE;
 
-  oldargs = TYPE_ARG_TYPES (oldtype);
-  newargs = TYPE_ARG_TYPES (newtype);
-  tryargs = newargs;
+  if (!comptypes (oldrettype, newrettype))
+    *strict = oldrettype;
 
-  while (oldargs || newargs)
+  tree oldargs = TYPE_ARG_TYPES (oldtype);
+  tree newargs = TYPE_ARG_TYPES (newtype);
+  tree tryargs = newargs;
+
+  for (unsigned i = 1; oldargs || newargs; ++i)
     {
       if (!oldargs
 	  || !newargs
 	  || !TREE_VALUE (oldargs)
-	  || !TREE_VALUE (newargs)
-	  || TYPE_MODE (TREE_VALUE (oldargs))
-	     != TYPE_MODE (TREE_VALUE (newargs)))
+	  || !TREE_VALUE (newargs))
 	return NULL_TREE;
 
+      tree oldtype = TREE_VALUE (oldargs);
+      tree newtype = TREE_VALUE (newargs);
+      if (TYPE_MODE (TREE_VALUE (oldargs))
+	  != TYPE_MODE (TREE_VALUE (newargs)))
+	return NULL_TREE;
+
+      if (!*strict && !comptypes (oldtype, newtype))
+	{
+	  *argno = i;
+	  *strict = oldtype;
+	}
+
       oldargs = TREE_CHAIN (oldargs);
       newargs = TREE_CHAIN (newargs);
     }
 
-  trytype = build_function_type (newrettype, tryargs);
+  tree trytype = build_function_type (newrettype, tryargs);
 
   /* Allow declaration to change transaction_safe attribute.  */
   tree oldattrs = TYPE_ATTRIBUTES (oldtype);
@@ -1874,10 +1887,19 @@  diagnose_mismatched_decls (tree newdecl, tree oldd
       if (TREE_CODE (olddecl) == FUNCTION_DECL
 	  && DECL_BUILT_IN (olddecl) && !C_DECL_DECLARED_BUILTIN (olddecl))
 	{
-	  /* Accept harmless mismatch in function types.
-	     This is for the ffs and fprintf builtins.  */
-	  tree trytype = match_builtin_function_types (newtype, oldtype);
+	  /* Accept "harmless" mismatch in function types such as
+	     missing qualifiers or pointer vs same size integer
+	     mismatches.  This is for the ffs and fprintf builtins.
+	     However, with -Wextra in effect, diagnose return and
+	     argument types that are incompatible according to
+	     language rules. */
+	  tree mismatch_expect;
+	  unsigned mismatch_argno;
 
+	  tree trytype = match_builtin_function_types (newtype, oldtype,
+						       &mismatch_expect,
+						       &mismatch_argno);
+
 	  if (trytype && comptypes (newtype, trytype))
 	    *oldtypep = oldtype = trytype;
 	  else
@@ -1885,11 +1907,30 @@  diagnose_mismatched_decls (tree newdecl, tree oldd
 	      /* If types don't match for a built-in, throw away the
 		 built-in.  No point in calling locate_old_decl here, it
 		 won't print anything.  */
-	      warning (OPT_Wbuiltin_declaration_mismatch,
-		       "conflicting types for built-in function %q+D",
-		       newdecl);
+	      warning_at (DECL_SOURCE_LOCATION (newdecl),
+			  OPT_Wbuiltin_declaration_mismatch,
+			  "conflicting types for built-in function %qD; "
+			  "expected %qT",
+			  newdecl, oldtype);
 	      return false;
 	    }
+
+	  if (mismatch_expect && extra_warnings)
+	    {
+	      /* If types match only loosely, print a warning but accept
+		 the redeclaration.  */
+	      location_t newloc = DECL_SOURCE_LOCATION (newdecl);
+	      if (mismatch_argno)
+		warning_at (newloc, OPT_Wbuiltin_declaration_mismatch,
+			    "mismatch in argument %u type of built-in "
+			    "function %qD; expected %qT",
+			    mismatch_argno, newdecl, mismatch_expect);
+	      else
+		warning_at (newloc, OPT_Wbuiltin_declaration_mismatch,
+			    "mismatch in return type of built-in "
+			    "function %qD; expected %qT",
+			    newdecl, mismatch_expect);
+	    }
 	}
       else if (TREE_CODE (olddecl) == FUNCTION_DECL
 	       && DECL_IS_BUILTIN (olddecl))
Index: gcc/testsuite/gcc.dg/Wbuiltin-declaration-mismatch.c
===================================================================
--- gcc/testsuite/gcc.dg/Wbuiltin-declaration-mismatch.c	(nonexistent)
+++ gcc/testsuite/gcc.dg/Wbuiltin-declaration-mismatch.c	(working copy)
@@ -0,0 +1,15 @@ 
+/* PR c/86125 - missing -Wbuiltin-declaration-mismatch on a mismatched
+   return type
+   { dg-do compile }
+   { dg-options "-Wbuiltin-declaration-mismatch -Wextra" } */
+
+char* strlen (const char*);   /* { dg-warning "mismatch in return type of built-in function .strlen.; expected .(long )?unsigned int." } */
+
+enum E { e };
+enum E strcmp (const char*, const char*);   /* { dg-warning "mismatch in return type of built-in function .strcmp.; expected .int." } */
+
+char* strcpy (char*, char*);   /* { dg-warning "mismatch in argument 2 type of built-in function .strcpy.; expected .const char \\*." } */
+
+char* strcat (const char*, char*);   /* { dg-warning "mismatch in argument 1 type of built-in function .strcat.; expected .char \\*." } */
+
+void strchr (const char*, int);      /* { dg-warning "conflicting types for built-in function .strchr.; expected .char \\*\\(const char \\*, int\\)." } */