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);
+}
