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

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\\)." } */