| Message ID | a82097ad-5c7d-e718-90c1-66ba6371478b@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | tighten up -Wbuiltin-declaration-mismatch (PR 86125) | expand |
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
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
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.
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
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.
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?
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.
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
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.
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
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);
+}
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.
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
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
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
On 7/5/18 2:22 PM, Jeff Law wrote: > 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 :-) I missed this response this summer and as I got busy with other things forgot to follow up, but PR 88886 that was just filed today reminded me that this still is a problem. Since the PR is a GCC 9 regression, as is 86308, I'd like to revive this patch. It seems that the only thing that prevented its approval was the use of a static local variable and so... > 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. ...I added the GTY marker to the variable. >> 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 *. There can be only one FILE* and GCC assumes it's the one from the first declaration of a file I/O built-in it sees, and warns for all others. In other words, FILE == struct SomeFile but FILE != struct OtherFile. That's the purpose of the static last_fileptr_type variable. Attached is an updated patch with the GTY marker, plus a new test for bug 88886. 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 c/86125 - missing -Wbuiltin-declaration-mismatch on a mismatched return type PR c/88886 - [9 Regression] ice in get_constant, at c-family/c-format.c:292 gcc/c/ChangeLog: PR c/86125 PR c/88886 PR middle-end/86308 * 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 PR c/88886 PR middle-end/86308 * gcc.dg/Wbuiltin-declaration-mismatch-6.c: New test. * gcc.dg/Wbuiltin-declaration-mismatch-7.c: New test. * gcc.dg/Wbuiltin-declaration-mismatch-8.c: New test. * gcc.dg/Wbuiltin-declaration-mismatch-9.c: New test. * gcc.dg/Wbuiltin-declaration-mismatch-10.c: New test. * gcc.dg/builtins-69.c: New test. * gcc.dg/Wint-conversion-2.c: Add expected warning. diff --git a/gcc/c/c-decl.c b/gcc/c/c-decl.c index 57049f80073..77420c0fd81 100644 --- a/gcc/c/c-decl.c +++ b/gcc/c/c-decl.c @@ -1631,43 +1631,91 @@ c_bind (location_t loc, tree decl, bool is_global) bind (DECL_NAME (decl), decl, scope, false, nested, loc); } + +/* Stores the first FILE* argument type (whatever it is) seen in + a declaration of a file I/O built-in. Subsequent declarations + of such built-ins are expected to refer to it rather than to + fileptr_type_node which is just void* (or to any other type). + Used only by match_builtin_function_types. */ + +static GTY(()) tree last_fileptr_type; + /* 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. */ + and argument types provided that the type modes match. Set *STRICT + and *ARGNO to the expected argument type and number in case of + an argument type mismatch or null and zero otherwise. Return + 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); + + /* 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 (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*. */ + 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); @@ -1881,14 +1929,26 @@ diagnose_mismatched_decls (tree newdecl, tree olddecl, if (TREE_CODE (olddecl) == FUNCTION_DECL && fndecl_built_in_p (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" mismatches 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 { + /* 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. */ const char *header = header_for_builtin_fn (DECL_FUNCTION_CODE (olddecl)); location_t loc = DECL_SOURCE_LOCATION (newdecl); @@ -1905,11 +1965,25 @@ diagnose_mismatched_decls (tree newdecl, tree olddecl, inform (&richloc, "%qD is declared in header %qs", olddecl, header); } - /* 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. */ 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/testsuite/gcc.dg/Wbuiltin-declaration-mismatch-10.c b/gcc/testsuite/gcc.dg/Wbuiltin-declaration-mismatch-10.c new file mode 100644 index 00000000000..58de1d48b6a --- /dev/null +++ b/gcc/testsuite/gcc.dg/Wbuiltin-declaration-mismatch-10.c @@ -0,0 +1,10 @@ +/* PR c/86308 - ICE in verify_gimple calling an invalid index() declaration + { dg-do compile } + { dg-options "-Wall" } */ + +int index (int, int); /* { dg-warning "conflicting types for built-in function .index.; expected .char \\\*\\\(const char \\\*, int\\\)." } */ + +int foo (const short *a) +{ + return a[index (0, 0)]; +} diff --git a/gcc/testsuite/gcc.dg/Wbuiltin-declaration-mismatch-6.c b/gcc/testsuite/gcc.dg/Wbuiltin-declaration-mismatch-6.c new file mode 100644 index 00000000000..32af9febfaf --- /dev/null +++ b/gcc/testsuite/gcc.dg/Wbuiltin-declaration-mismatch-6.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-7.c b/gcc/testsuite/gcc.dg/Wbuiltin-declaration-mismatch-7.c new file mode 100644 index 00000000000..77a4bfff4d3 --- /dev/null +++ b/gcc/testsuite/gcc.dg/Wbuiltin-declaration-mismatch-7.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-8.c b/gcc/testsuite/gcc.dg/Wbuiltin-declaration-mismatch-8.c new file mode 100644 index 00000000000..d06af6528fb --- /dev/null +++ b/gcc/testsuite/gcc.dg/Wbuiltin-declaration-mismatch-8.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-9.c b/gcc/testsuite/gcc.dg/Wbuiltin-declaration-mismatch-9.c new file mode 100644 index 00000000000..f0c1ce33267 --- /dev/null +++ b/gcc/testsuite/gcc.dg/Wbuiltin-declaration-mismatch-9.c @@ -0,0 +1,12 @@ +/* PR c/88886 - ice in get_constant, at c-family/c-format.c:292 + { dg-do compile } + { dg-options "-Wall" } */ + +int sscanf (long, unsigned[], ...); /* { dg-warning "conflicting types for built-in function .sscanf.; expected .int\\\(const char \\\*, const char \\\*, ...\\\)." } */ + +void a (void) +{ + sscanf (0, + "" /* { dg-warning "passing argument 2 of .sscanf. from incompatible pointer type" } */ + ); +} diff --git a/gcc/testsuite/gcc.dg/Wint-conversion-2.c b/gcc/testsuite/gcc.dg/Wint-conversion-2.c index 0c9dac44c41..bf590a7bcd7 100644 --- a/gcc/testsuite/gcc.dg/Wint-conversion-2.c +++ b/gcc/testsuite/gcc.dg/Wint-conversion-2.c @@ -1,8 +1,9 @@ -/* PR middle-end/86202 */ +/* PR middle-end/86202 - ICE in get_range_info calling an invalid memcpy() + declaration */ /* { dg-do compile } */ /* { dg-options "-Wint-conversion" } */ -void *memcpy (void *, void *, __SIZE_TYPE__ *); +void *memcpy (void *, void *, __SIZE_TYPE__ *); /* { dg-warning "conflicting types for built-in function .memcpy." } */ void *a, *b; void f (void) { diff --git a/gcc/testsuite/gcc.dg/builtins-69.c b/gcc/testsuite/gcc.dg/builtins-69.c new file mode 100644 index 00000000000..26dfb3bfc1b --- /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); +}
Ping: https://gcc.gnu.org/ml/gcc-patches/2019-01/msg00969.html This patch fixes a couple of P2 regressions. It was first submitted last summer and mostly reviewed by Jeff but then slipped through the cracks. Can someone pick it up in Jeff's absence? On 1/16/19 5:41 PM, Martin Sebor wrote: > On 7/5/18 2:22 PM, Jeff Law wrote: >> 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 >> :-) > > I missed this response this summer and as I got busy with other > things forgot to follow up, but PR 88886 that was just filed today > reminded me that this still is a problem. Since the PR is a GCC 9 > regression, as is 86308, I'd like to revive this patch. It seems > that the only thing that prevented its approval was the use of > a static local variable and so... > >> 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. > > ...I added the GTY marker to the variable. > >>> 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 *. > > There can be only one FILE* and GCC assumes it's the one from > the first declaration of a file I/O built-in it sees, and warns > for all others. In other words, FILE == struct SomeFile but > FILE != struct OtherFile. That's the purpose of the static > last_fileptr_type variable. > > Attached is an updated patch with the GTY marker, plus a new > test for bug 88886. > > Martin
On Wed, 23 Jan 2019, Martin Sebor wrote:
> Ping: https://gcc.gnu.org/ml/gcc-patches/2019-01/msg00969.html
This patch is OK.
On 1/23/19 4:41 PM, Joseph Myers wrote: > On Wed, 23 Jan 2019, Martin Sebor wrote: > >> Ping: https://gcc.gnu.org/ml/gcc-patches/2019-01/msg00969.html > > This patch is OK. Thanks. I committed it in r268251. As a heads up, before I did that I noticed a failure in gcc.c-torture/execute/eeprof-1.c due to the test declaring the __cyg_profile_func_{enter,exit} functions to take a pair of function pointers rather than void* as expected. I fixed the failure by changing the function declarations but it makes me wonder if some other tests for non-x86 targets that I didn't build might have similar problems. I'll deal with those if/when that happens. Martin
On Wed, Jan 23, 2019 at 11:41:55PM +0000, Joseph Myers wrote: > On Wed, 23 Jan 2019, Martin Sebor wrote: > > > Ping: https://gcc.gnu.org/ml/gcc-patches/2019-01/msg00969.html > > This patch is OK. Note, the testcase FAILs on i686-linux: +FAIL: gcc.dg/Wbuiltin-declaration-mismatch-7.c (test for warnings, line 20) size_t is unsigned int there I believe. Jakub
On Fri, 25 Jan 2019 at 06:40, Jakub Jelinek <jakub@redhat.com> wrote: > > On Wed, Jan 23, 2019 at 11:41:55PM +0000, Joseph Myers wrote: > > On Wed, 23 Jan 2019, Martin Sebor wrote: > > > > > Ping: https://gcc.gnu.org/ml/gcc-patches/2019-01/msg00969.html > > > > This patch is OK. > > Note, the testcase FAILs on i686-linux: > +FAIL: gcc.dg/Wbuiltin-declaration-mismatch-7.c (test for warnings, line 20) > size_t is unsigned int there I believe. > On aarch64: /gcc/testsuite/gcc.dg/Wbuiltin-declaration-mismatch-7.c:16:5: warning: conflicting types for built-in function 'vfprintf'; expected 'int(void *, const char *, __va_list)' [-Wbuiltin-declaration-mismatch] /gcc/testsuite/gcc.dg/Wbuiltin-declaration-mismatch-7.c:24:5: warning: conflicting types for built-in function 'vfscanf'; expected 'int(void *, const char *, __va_list)' [-Wbuiltin-declaration-mismatch] Both errors are also present on arm, as well as: FAIL: gcc.dg/Wbuiltin-declaration-mismatch-7.c (test for warnings, line 20) (for lack of warning apparently) > Jakub
It's also broken the build of the glibc testsuite, e.g.: ../time/time.h:88:15: error: mismatch in argument 1 type of built-in function 'strftime'; expected 'char *' [-Werror=builtin-declaration-mismatch] 88 | extern size_t strftime (char *__restrict __s, size_t __maxsize, (presence or absence of qualifiers on a parameter is not part of the function type and should not be compared here).
On Sat, Jan 26, 2019 at 01:25:04AM +0000, Joseph Myers wrote: > It's also broken the build of the glibc testsuite, e.g.: > > ../time/time.h:88:15: error: mismatch in argument 1 type of built-in function 'strftime'; expected 'char *' [-Werror=builtin-declaration-mismatch] > 88 | extern size_t strftime (char *__restrict __s, size_t __maxsize, > > (presence or absence of qualifiers on a parameter is not part of the > function type and should not be compared here). There are several further issues, even after fixing the toplevel qualifiers on arguments as well as return type + fixing up the testcases, there is another problem, the code has the magic handling only for fileptr_type_node, but we actually need handling for all builtin_structptr_types struct types, otherwise e.g. for strftime we warn that the last argument should not be const struct tm *, but const void *, when it actually should be const struct tm *. Working on it. Jakub
On Sat, Jan 26, 2019 at 01:35:05PM +0100, Jakub Jelinek wrote: > On Sat, Jan 26, 2019 at 01:25:04AM +0000, Joseph Myers wrote: > > It's also broken the build of the glibc testsuite, e.g.: > > > > ../time/time.h:88:15: error: mismatch in argument 1 type of built-in function 'strftime'; expected 'char *' [-Werror=builtin-declaration-mismatch] > > 88 | extern size_t strftime (char *__restrict __s, size_t __maxsize, > > > > (presence or absence of qualifiers on a parameter is not part of the > > function type and should not be compared here). > > There are several further issues, even after fixing the toplevel qualifiers on > arguments as well as return type + fixing up the testcases, there is another > problem, the code has the magic handling only for fileptr_type_node, but we > actually need handling for all builtin_structptr_types struct types, > otherwise e.g. for strftime we warn that the last argument should not be > const struct tm *, but const void *, when it actually should be const struct > tm *. It is actually worse than that, because unlike C++, in C fileptr_type_node is a mere void * and const_tm_ptr_type_node a mere const_ptr_type_node. So, on /* PR c/86125 */ /* { dg-do compile } */ /* { dg-options "-Wbuiltin-declaration-mismatch -Wextra -Wno-ignored-qualifiers" } */ typedef __SIZE_TYPE__ size_t; struct FILE; struct tm; struct fenv_t; struct fexcept_t; typedef struct FILE FILE; typedef struct fenv_t fenv_t; typedef struct fexcept_t fexcept_t; typedef const int cint; size_t strftime (char *__restrict, const size_t, const char *__restrict, /* { dg-bogus "mismatch in argument 1 type of built-in function" } */ const struct tm *__restrict) __attribute__((nothrow)); int fprintf (struct FILE *, const char *const, ...); /* { dg-bogus "mismatch in argument 2 type of built-in function" } */ cint putc (int, struct FILE *); /* { dg-bogus "mismatch in return type of built-in function" } */ cint fegetenv (fenv_t *); /* { dg-bogus "mismatch in argument 1 type of built-in function" } */ cint fesetenv (const fenv_t *); /* { dg-bogus "mismatch in return type of built-in function" } */ int fegetexceptflag (fexcept_t *, const int); /* { dg-bogus "mismatch in argument 1 type of built-in function" } */ int fesetexceptflag (const fexcept_t *, const int); /* { dg-bogus "mismatch in argument 1 type of built-in function" } */ we get among all the other bogus warnings Wbuiltin-declaration-mismatch-11.c:21:5: warning: mismatch in argument 1 type of built-in function ‘fegetexceptflag’; expected ‘struct FILE *’ [-Wbuiltin-declaration-mismatch] 21 | int fegetexceptflag (fexcept_t *, const int); /* { dg-bogus "mismatch in argument 1 type of built-in function" } */ | ^~~~~~~~~~~~~~~ Suggesting one uses struct FILE * argument to fegetexceptflag is not a very good suggestion. Jakub
On 1/25/19 6:25 PM, Joseph Myers wrote: > It's also broken the build of the glibc testsuite, e.g.: > > ../time/time.h:88:15: error: mismatch in argument 1 type of built-in function 'strftime'; expected 'char *' [-Werror=builtin-declaration-mismatch] > 88 | extern size_t strftime (char *__restrict __s, size_t __maxsize, > > (presence or absence of qualifiers on a parameter is not part of the > function type and should not be compared here). Not sure I see which pointer the warning is complaining about but detecting mismatches in constness is intentional. Users will want to know when they accidentally swapped the arguments of, say, memcpy, and declared it as: memcpy (const char*, char*, size_t); Martin
On Mon, 28 Jan 2019, Martin Sebor wrote: > On 1/25/19 6:25 PM, Joseph Myers wrote: > > It's also broken the build of the glibc testsuite, e.g.: > > > > ../time/time.h:88:15: error: mismatch in argument 1 type of built-in > > function 'strftime'; expected 'char *' > > [-Werror=builtin-declaration-mismatch] > > 88 | extern size_t strftime (char *__restrict __s, size_t __maxsize, > > > > (presence or absence of qualifiers on a parameter is not part of the > > function type and should not be compared here). > > Not sure I see which pointer the warning is complaining about but > detecting mismatches in constness is intentional. Users will want > to know when they accidentally swapped the arguments of, say, > memcpy, and declared it as: It's about qualifiers on the pointer itself (which do not affect the function's type, although such qualifiers in the function definition affect code inside the function), not qualifiers on the pointer target type. GCC can't assume whether system headers do or do not have the "restrict" qualification on some parameters that's included in C99 (essentially as documentation of aliasing requirements for parameters for those functions), nor do GCC's built-in function declarations have that "restrict" anywhere. But it's complaining here about a mismatch between "char *restrict" and "char *".
On 1/28/19 9:43 AM, Joseph Myers wrote: > On Mon, 28 Jan 2019, Martin Sebor wrote: > >> On 1/25/19 6:25 PM, Joseph Myers wrote: >>> It's also broken the build of the glibc testsuite, e.g.: >>> >>> ../time/time.h:88:15: error: mismatch in argument 1 type of built-in >>> function 'strftime'; expected 'char *' >>> [-Werror=builtin-declaration-mismatch] >>> 88 | extern size_t strftime (char *__restrict __s, size_t __maxsize, >>> >>> (presence or absence of qualifiers on a parameter is not part of the >>> function type and should not be compared here). >> >> Not sure I see which pointer the warning is complaining about but >> detecting mismatches in constness is intentional. Users will want >> to know when they accidentally swapped the arguments of, say, >> memcpy, and declared it as: > > It's about qualifiers on the pointer itself (which do not affect the > function's type, although such qualifiers in the function definition > affect code inside the function), not qualifiers on the pointer target > type. GCC can't assume whether system headers do or do not have the > "restrict" qualification on some parameters that's included in C99 > (essentially as documentation of aliasing requirements for parameters for > those functions), nor do GCC's built-in function declarations have that > "restrict" anywhere. But it's complaining here about a mismatch between > "char *restrict" and "char *". I see. That's certainly not intentional. Looks like we need better tests. None of the ones I added focus on exercising qualifiers, and I can't find any others for the warning that do either. Martin
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\\)." } */