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
