Re: [PATCH] tighten up -Wbuiltin-declaration-mismatch (PR 86125)

[Jeff Law <law at redhat dot com>]

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