exempt invalid built-in calls from -Wrestrict (PR 83655)

In addition to assuming that built-in functions are called with
the correct number of arguments (bug 83603), the restrict pass
also assumes that they are called with arguments of the expected
types.  When a built-in is declared with no prototype and called
with arguments of the wrong type (such as memcpy (3, dst, ""))
the pass crashes GCC with a SEGV.

The attached patch prevents invalid calls from being checked for
-Wrestrict violations.  It's meant to be applied in conjunction
with the patch for bug 83603:
   https://gcc.gnu.org/ml/gcc-patches/2018-01/msg00005.html

A separate fix is to have GCC issue a warning for declarations
of built-ins with no prototype.  I opened bug 83656 to track this
change request to the C front end (C++ already warns for this).

Another improvement is to also warn about calls with the wrong number
or types of arguments when a built-in is declared without a prototype
despite the first warning, similarly to what Clang does.  I opened
bug 83657 to add this warning.

Martin

On 01/02/2018 04:02 PM, Martin Sebor wrote:
> In addition to assuming that built-in functions are called with
> the correct number of arguments (bug 83603), the restrict pass
> also assumes that they are called with arguments of the expected
> types.  When a built-in is declared with no prototype and called
> with arguments of the wrong type (such as memcpy (3, dst, ""))
> the pass crashes GCC with a SEGV.
> 
> The attached patch prevents invalid calls from being checked for
> -Wrestrict violations.  It's meant to be applied in conjunction
> with the patch for bug 83603:
>   https://gcc.gnu.org/ml/gcc-patches/2018-01/msg00005.html
> 
> A separate fix is to have GCC issue a warning for declarations
> of built-ins with no prototype.  I opened bug 83656 to track this
> change request to the C front end (C++ already warns for this).
> 
> Another improvement is to also warn about calls with the wrong number
> or types of arguments when a built-in is declared without a prototype
> despite the first warning, similarly to what Clang does.  I opened
> bug 83657 to add this warning.
> 
> Martin
> 
> gcc-83655.diff
> 
> 
> PR tree-optimization/83655 - ICE on an invalid call to memcpy declared with no prototype
> 
> gcc/testsuite/ChangeLog:
> 
> 	PR tree-optimization/83655
> 	* gcc.dg/Wrestrict-5.c: New test.
> 	* c-c++-common/builtins.c: New test.
> 
> gcc/ChangeLog:
> 
> 	PR tree-optimization/83655
> 	* gimple-ssa-warn-restrict.c (wrestrict_dom_walker::check_call): 
Looks like your ChangeLog is incomplete.  OK with the completed
ChangeLog entry.

Jeff

Martin Sebor <msebor@gmail.com> writes:
> In addition to assuming that built-in functions are called with
> the correct number of arguments (bug 83603), the restrict pass
> also assumes that they are called with arguments of the expected
> types.  When a built-in is declared with no prototype and called
> with arguments of the wrong type (such as memcpy (3, dst, ""))
> the pass crashes GCC with a SEGV.
>
> The attached patch prevents invalid calls from being checked for
> -Wrestrict violations.  It's meant to be applied in conjunction
> with the patch for bug 83603:
>    https://gcc.gnu.org/ml/gcc-patches/2018-01/msg00005.html
>
> A separate fix is to have GCC issue a warning for declarations
> of built-ins with no prototype.  I opened bug 83656 to track this
> change request to the C front end (C++ already warns for this).
>
> Another improvement is to also warn about calls with the wrong number
> or types of arguments when a built-in is declared without a prototype
> despite the first warning, similarly to what Clang does.  I opened
> bug 83657 to add this warning.
>
> Martin
>
> PR tree-optimization/83655 - ICE on an invalid call to memcpy declared with no prototype
>
> gcc/testsuite/ChangeLog:
>
> 	PR tree-optimization/83655
> 	* gcc.dg/Wrestrict-5.c: New test.
> 	* c-c++-common/builtins.c: New test.
>
> gcc/ChangeLog:
>
> 	PR tree-optimization/83655
> 	* gimple-ssa-warn-restrict.c (wrestrict_dom_walker::check_call): 
>
> diff --git a/gcc/gimple-ssa-warn-restrict.c b/gcc/gimple-ssa-warn-restrict.c
> index ac545e4..47d4900 100644
> --- a/gcc/gimple-ssa-warn-restrict.c
> +++ b/gcc/gimple-ssa-warn-restrict.c
> @@ -1688,7 +1688,17 @@ wrestrict_dom_walker::check_call (gcall *call)
>    if (!dstwr && strfun)
>      dstwr = size_one_node;
>  
> -  if (check_bounds_or_overlap (call, dst, src, dstwr, NULL_TREE))
> +  /* DST and SRC can be null for a call with an insufficient number
> +     of arguments to a built-in function declared without a protype.  */

Typo: prototype.

> +  if (!dst || !src)
> +    return;
> +
> +  /* DST, SRC, or DSTWR can also have the wrong type in a call to
> +     a function declared without a prototype.  Avoid checking such
> +     invalid calls.  */
> +  if (TREE_CODE (TREE_TYPE (dst)) != POINTER_TYPE
> +      || TREE_CODE (TREE_TYPE (src)) != POINTER_TYPE
> +      || (dstwr && !INTEGRAL_TYPE_P (TREE_TYPE (dstwr))))
>      return;

Isn't the canonical way of handling this kind of thing to use:

  if (!gimple_call_builtin_p (call, BUILT_IN_NORMAL)
    return;

instead of:

  tree func = gimple_call_fndecl (call);
  if (!func || DECL_BUILT_IN_CLASS (func) != BUILT_IN_NORMAL)
    return;

?  

Thanks,
Richard

On 01/04/2018 10:52 AM, Richard Sandiford wrote:
> Martin Sebor <msebor@gmail.com> writes:
>> In addition to assuming that built-in functions are called with
>> the correct number of arguments (bug 83603), the restrict pass
>> also assumes that they are called with arguments of the expected
>> types.  When a built-in is declared with no prototype and called
>> with arguments of the wrong type (such as memcpy (3, dst, ""))
>> the pass crashes GCC with a SEGV.
>>
>> The attached patch prevents invalid calls from being checked for
>> -Wrestrict violations.  It's meant to be applied in conjunction
>> with the patch for bug 83603:
>>    https://gcc.gnu.org/ml/gcc-patches/2018-01/msg00005.html
>>
>> A separate fix is to have GCC issue a warning for declarations
>> of built-ins with no prototype.  I opened bug 83656 to track this
>> change request to the C front end (C++ already warns for this).
>>
>> Another improvement is to also warn about calls with the wrong number
>> or types of arguments when a built-in is declared without a prototype
>> despite the first warning, similarly to what Clang does.  I opened
>> bug 83657 to add this warning.
>>
>> Martin
>>
>> PR tree-optimization/83655 - ICE on an invalid call to memcpy declared with no prototype
>>
>> gcc/testsuite/ChangeLog:
>>
>> 	PR tree-optimization/83655
>> 	* gcc.dg/Wrestrict-5.c: New test.
>> 	* c-c++-common/builtins.c: New test.
>>
>> gcc/ChangeLog:
>>
>> 	PR tree-optimization/83655
>> 	* gimple-ssa-warn-restrict.c (wrestrict_dom_walker::check_call):
>>
>> diff --git a/gcc/gimple-ssa-warn-restrict.c b/gcc/gimple-ssa-warn-restrict.c
>> index ac545e4..47d4900 100644
>> --- a/gcc/gimple-ssa-warn-restrict.c
>> +++ b/gcc/gimple-ssa-warn-restrict.c
>> @@ -1688,7 +1688,17 @@ wrestrict_dom_walker::check_call (gcall *call)
>>    if (!dstwr && strfun)
>>      dstwr = size_one_node;
>>
>> -  if (check_bounds_or_overlap (call, dst, src, dstwr, NULL_TREE))
>> +  /* DST and SRC can be null for a call with an insufficient number
>> +     of arguments to a built-in function declared without a protype.  */
>
> Typo: prototype.

Let me fix that.

>
>> +  if (!dst || !src)
>> +    return;
>> +
>> +  /* DST, SRC, or DSTWR can also have the wrong type in a call to
>> +     a function declared without a prototype.  Avoid checking such
>> +     invalid calls.  */
>> +  if (TREE_CODE (TREE_TYPE (dst)) != POINTER_TYPE
>> +      || TREE_CODE (TREE_TYPE (src)) != POINTER_TYPE
>> +      || (dstwr && !INTEGRAL_TYPE_P (TREE_TYPE (dstwr))))
>>      return;
>
> Isn't the canonical way of handling this kind of thing to use:
>
>   if (!gimple_call_builtin_p (call, BUILT_IN_NORMAL)
>     return;
>
> instead of:
>
>   tree func = gimple_call_fndecl (call);
>   if (!func || DECL_BUILT_IN_CLASS (func) != BUILT_IN_NORMAL)
>     return;
>
> ?

It may be how optimization passes deal with it but it would be
a poor choice in a warning pass because it would suppress
the checking of even valid calls to built-ins declared without
a prototype.  For instance, without -Wrestrict, GCC silently
accepts this code:

   void* memcpy ();

   void f (char *d)
   {
     memcpy (d, d + 2, 5);
   }

With -Wrestict, and without relying on gimple_call_fndecl(),
the overlap in the call is diagnosed, similarly to how the buffer
overflow in this example is diagnosed by -Wstringop-overflow:

   char a[4];

   void f (const char *s)
   {
     memcpy (a, s, 32);
   }

I think gimple_call_fndecl() is overly restrictive by insisting
on compatibility between integer types like int and long.  It
has the consequence of disabling optimizations like strlen that
warnings then rely on to help detect bugs.  In effect, it makes
the safety hole that declaring built-ins without a prototype opens
up that much bigger.  For example, because the strlen pass ignores
the call to memcpy below (8 is an int which is incompatible with
the size_t argument) the overlapping call to strcpy isn't diagnosed.

   char* memcpy ();
   char* strcpy ();

   char a[8];

   void f (const char *s)
   {
     memcpy (a, "12345", 8);

     strcpy (a + 3, a);
   }

Ironically, the bug is diagnosed when memcpy() isn't declared
at all -- because GCC uses the prototype of __builtin_memcpy()
to convert the arguments to the expected types, which in turn
lets gimple_call_fndecl() treat the call as one to the built-in
and the strlen pass work its magic.  There's no reason why GCC
couldn't do the same thing  even when the functions are declared
without a prototype, like Clang does.

Declaring functions without a prototype has been deprecated for
years because it defeats the type system.  There isn't a lot GCC
can do to mitigate the problems for user-defined functions, but
it could do more to help avoid the problem with built-ins.

Sorry if this seems like a long-winded response to a simple
question.  I feel strongly about the subject.

Martin