warn on returning alloca and VLA (PR 71924, 90549)

-Wreturn-local-addr detects a subset of instances of returning
the address of a local object from a function but the warning
doesn't try to handle alloca or VLAs, or some non-trivial cases
of ordinary automatic variables[1].

The attached patch extends the implementation of the warning to
detect those.  It still doesn't detect instances where the address
is the result of a built-in such strcpy[2].

Tested on x86_64-linux.

Martin

[1] For example, this is only diagnosed with the patch:

   void* f (int i)
   {
     struct S { int a[2]; } s[2];
     return &s->a[i];
   }

[2] The following is not diagnosed even with the patch:

   void sink (void*);

   void* f (int i)
   {
     char a[6];
     char *p = __builtin_strcpy (a, "123");
     sink (p);
     return p;
   }

I would expect detecting to be possible and useful.  Maybe as
a follow-up.

On 5/22/19 3:34 PM, Martin Sebor wrote:
> -Wreturn-local-addr detects a subset of instances of returning
> the address of a local object from a function but the warning
> doesn't try to handle alloca or VLAs, or some non-trivial cases
> of ordinary automatic variables[1].
> 
> The attached patch extends the implementation of the warning to
> detect those.  It still doesn't detect instances where the address
> is the result of a built-in such strcpy[2].
> 
> Tested on x86_64-linux.
> 
> Martin
> 
> [1] For example, this is only diagnosed with the patch:
> 
>   void* f (int i)
>   {
>     struct S { int a[2]; } s[2];
>     return &s->a[i];
>   }
> 
> [2] The following is not diagnosed even with the patch:
> 
>   void sink (void*);
> 
>   void* f (int i)
>   {
>     char a[6];
>     char *p = __builtin_strcpy (a, "123");
>     sink (p);
>     return p;
>   }
> 
> I would expect detecting to be possible and useful.  Maybe as
> a follow-up.
> 
> gcc-71924.diff
> 
> PR middle-end/71924 - missing -Wreturn-local-addr returning alloca result
> PR middle-end/90549 - missing -Wreturn-local-addr maybe returning an address of a local array plus offset
> 
> gcc/ChangeLog:
> 
> 	PR c/71924
> 	* gimple-ssa-isolate-paths.c (is_addr_local): New function.
> 	(warn_return_addr_local_phi_arg, warn_return_addr_local): Same.
> 	(find_implicit_erroneous_behavior): Call warn_return_addr_local_phi_arg.
> 	(find_explicit_erroneous_behavior): Call warn_return_addr_local.
> 
> gcc/testsuite/ChangeLog:
> 
> 	PR c/71924
> 	* gcc.dg/Wreturn-local-addr-2.c: New test.
> 	* gcc.dg/Walloca-4.c: Prune expected warnings.
> 	* gcc.dg/pr41551.c: Same.
> 	* gcc.dg/pr59523.c: Same.
> 	* gcc.dg/tree-ssa/pr88775-2.c: Same.
> 	* gcc.dg/winline-7.c: Same.
> 
> diff --git a/gcc/gimple-ssa-isolate-paths.c b/gcc/gimple-ssa-isolate-paths.c
> index 33fe352bb23..2933ecf502e 100644
> --- a/gcc/gimple-ssa-isolate-paths.c
> +++ b/gcc/gimple-ssa-isolate-paths.c
> @@ -341,6 +341,135 @@ stmt_uses_0_or_null_in_undefined_way (gimple *stmt)
>    return false;
>  }
>  
> +/* Return true if EXPR is a expression of pointer type that refers
> +   to the address of a variable with automatic storage duration.
> +   If so, set *PLOC to the location of the object or the call that
> +   allocated it (for alloca and VLAs).  When PMAYBE is non-null,
> +   also consider PHI statements and set *PMAYBE when some but not
> +   all arguments of such statements refer to local variables, and
> +   to clear it otherwise.  */
> +
> +static bool
> +is_addr_local (tree exp, location_t *ploc, bool *pmaybe = NULL,
> +	       hash_set<gphi *> *visited = NULL)
> +{
> +  if (TREE_CODE (exp) == ADDR_EXPR)
> +    {
> +      tree baseaddr = get_base_address (TREE_OPERAND (exp, 0));
> +      if (TREE_CODE (baseaddr) == MEM_REF)
> +	return is_addr_local (TREE_OPERAND (baseaddr, 0), ploc, pmaybe, visited);
> +
> +      if ((!VAR_P (baseaddr)
> +	   || is_global_var (baseaddr))
> +	  && TREE_CODE (baseaddr) != PARM_DECL)
> +	return false;
> +
> +      *ploc = DECL_SOURCE_LOCATION (baseaddr);
> +      return true;
> +    }
> +
> +  if (TREE_CODE (exp) == SSA_NAME)
> +    {
> +      gimple *def_stmt = SSA_NAME_DEF_STMT (exp);
> +      enum gimple_code code = gimple_code (def_stmt);
> +
> +      if (is_gimple_assign (def_stmt))
> +	{
> +	  tree type = TREE_TYPE (gimple_assign_lhs (def_stmt));
> +	  if (POINTER_TYPE_P (type))
> +	    {
> +	      tree ptr = gimple_assign_rhs1 (def_stmt);
> +	      return is_addr_local (ptr, ploc, pmaybe, visited);
> +	    }
> +	  return false;
> +	}
> +
> +      if (code == GIMPLE_CALL
> +	  && gimple_call_builtin_p (def_stmt))
> +	{
> +	  tree fn = gimple_call_fndecl (def_stmt);
> +	  int code = DECL_FUNCTION_CODE (fn);
> +	  if (code != BUILT_IN_ALLOCA
> +	      && code != BUILT_IN_ALLOCA_WITH_ALIGN)
> +	    return false;
> +
> +	  *ploc = gimple_location (def_stmt);
> +	  return true;
> +	}
> +
> +      if (code == GIMPLE_PHI && pmaybe)
> +	{
> +	  unsigned count = 0;
> +	  gphi *phi_stmt = as_a <gphi *> (def_stmt);
> +
> +	  unsigned nargs = gimple_phi_num_args (phi_stmt);
> +	  for (unsigned i = 0; i < nargs; ++i)
> +	    {
> +	      if (!visited->add (phi_stmt))
> +		{
> +		  tree arg = gimple_phi_arg_def (phi_stmt, i);
> +		  if (is_addr_local (arg, ploc, pmaybe, visited))
> +		    ++count;
> +		}
> +	    }
> +
> +	  *pmaybe = count && count < nargs;
> +	  return count != 0;
> +	}
> +    }
> +
> +  return false;
> +}
Is there some reason you didn't query the alias oracle here?  It would
seem a fairly natural fit.  Ultimately given a pointer (which will be an
SSA_NAME) you want to ask whether or not it conclusively points into the
stack.

That would seem to dramatically simplify is_addr_local.

The rest looks pretty reasonable.

Jeff

On 5/29/19 11:45 AM, Jeff Law wrote:
> On 5/22/19 3:34 PM, Martin Sebor wrote:
>> -Wreturn-local-addr detects a subset of instances of returning
>> the address of a local object from a function but the warning
>> doesn't try to handle alloca or VLAs, or some non-trivial cases
>> of ordinary automatic variables[1].
>>
>> The attached patch extends the implementation of the warning to
>> detect those.  It still doesn't detect instances where the address
>> is the result of a built-in such strcpy[2].
>>
>> Tested on x86_64-linux.
>>
>> Martin
>>
>> [1] For example, this is only diagnosed with the patch:
>>
>>    void* f (int i)
>>    {
>>      struct S { int a[2]; } s[2];
>>      return &s->a[i];
>>    }
>>
>> [2] The following is not diagnosed even with the patch:
>>
>>    void sink (void*);
>>
>>    void* f (int i)
>>    {
>>      char a[6];
>>      char *p = __builtin_strcpy (a, "123");
>>      sink (p);
>>      return p;
>>    }
>>
>> I would expect detecting to be possible and useful.  Maybe as
>> a follow-up.
>>
>> gcc-71924.diff
>>
>> PR middle-end/71924 - missing -Wreturn-local-addr returning alloca result
>> PR middle-end/90549 - missing -Wreturn-local-addr maybe returning an address of a local array plus offset
>>
>> gcc/ChangeLog:
>>
>> 	PR c/71924
>> 	* gimple-ssa-isolate-paths.c (is_addr_local): New function.
>> 	(warn_return_addr_local_phi_arg, warn_return_addr_local): Same.
>> 	(find_implicit_erroneous_behavior): Call warn_return_addr_local_phi_arg.
>> 	(find_explicit_erroneous_behavior): Call warn_return_addr_local.
>>
>> gcc/testsuite/ChangeLog:
>>
>> 	PR c/71924
>> 	* gcc.dg/Wreturn-local-addr-2.c: New test.
>> 	* gcc.dg/Walloca-4.c: Prune expected warnings.
>> 	* gcc.dg/pr41551.c: Same.
>> 	* gcc.dg/pr59523.c: Same.
>> 	* gcc.dg/tree-ssa/pr88775-2.c: Same.
>> 	* gcc.dg/winline-7.c: Same.
>>
>> diff --git a/gcc/gimple-ssa-isolate-paths.c b/gcc/gimple-ssa-isolate-paths.c
>> index 33fe352bb23..2933ecf502e 100644
>> --- a/gcc/gimple-ssa-isolate-paths.c
>> +++ b/gcc/gimple-ssa-isolate-paths.c
>> @@ -341,6 +341,135 @@ stmt_uses_0_or_null_in_undefined_way (gimple *stmt)
>>     return false;
>>   }
>>   
>> +/* Return true if EXPR is a expression of pointer type that refers
>> +   to the address of a variable with automatic storage duration.
>> +   If so, set *PLOC to the location of the object or the call that
>> +   allocated it (for alloca and VLAs).  When PMAYBE is non-null,
>> +   also consider PHI statements and set *PMAYBE when some but not
>> +   all arguments of such statements refer to local variables, and
>> +   to clear it otherwise.  */
>> +
>> +static bool
>> +is_addr_local (tree exp, location_t *ploc, bool *pmaybe = NULL,
>> +	       hash_set<gphi *> *visited = NULL)
>> +{
>> +  if (TREE_CODE (exp) == ADDR_EXPR)
>> +    {
>> +      tree baseaddr = get_base_address (TREE_OPERAND (exp, 0));
>> +      if (TREE_CODE (baseaddr) == MEM_REF)
>> +	return is_addr_local (TREE_OPERAND (baseaddr, 0), ploc, pmaybe, visited);
>> +
>> +      if ((!VAR_P (baseaddr)
>> +	   || is_global_var (baseaddr))
>> +	  && TREE_CODE (baseaddr) != PARM_DECL)
>> +	return false;
>> +
>> +      *ploc = DECL_SOURCE_LOCATION (baseaddr);
>> +      return true;
>> +    }
>> +
>> +  if (TREE_CODE (exp) == SSA_NAME)
>> +    {
>> +      gimple *def_stmt = SSA_NAME_DEF_STMT (exp);
>> +      enum gimple_code code = gimple_code (def_stmt);
>> +
>> +      if (is_gimple_assign (def_stmt))
>> +	{
>> +	  tree type = TREE_TYPE (gimple_assign_lhs (def_stmt));
>> +	  if (POINTER_TYPE_P (type))
>> +	    {
>> +	      tree ptr = gimple_assign_rhs1 (def_stmt);
>> +	      return is_addr_local (ptr, ploc, pmaybe, visited);
>> +	    }
>> +	  return false;
>> +	}
>> +
>> +      if (code == GIMPLE_CALL
>> +	  && gimple_call_builtin_p (def_stmt))
>> +	{
>> +	  tree fn = gimple_call_fndecl (def_stmt);
>> +	  int code = DECL_FUNCTION_CODE (fn);
>> +	  if (code != BUILT_IN_ALLOCA
>> +	      && code != BUILT_IN_ALLOCA_WITH_ALIGN)
>> +	    return false;
>> +
>> +	  *ploc = gimple_location (def_stmt);
>> +	  return true;
>> +	}
>> +
>> +      if (code == GIMPLE_PHI && pmaybe)
>> +	{
>> +	  unsigned count = 0;
>> +	  gphi *phi_stmt = as_a <gphi *> (def_stmt);
>> +
>> +	  unsigned nargs = gimple_phi_num_args (phi_stmt);
>> +	  for (unsigned i = 0; i < nargs; ++i)
>> +	    {
>> +	      if (!visited->add (phi_stmt))
>> +		{
>> +		  tree arg = gimple_phi_arg_def (phi_stmt, i);
>> +		  if (is_addr_local (arg, ploc, pmaybe, visited))
>> +		    ++count;
>> +		}
>> +	    }
>> +
>> +	  *pmaybe = count && count < nargs;
>> +	  return count != 0;
>> +	}
>> +    }
>> +
>> +  return false;
>> +}
> Is there some reason you didn't query the alias oracle here?  It would
> seem a fairly natural fit.  Ultimately given a pointer (which will be an
> SSA_NAME) you want to ask whether or not it conclusively points into the
> stack.
> 
> That would seem to dramatically simplify is_addr_local.

I did think about it but decided against changing the existing
design (iterating over PHI arguments), for a couple of reasons:

1) It feels like a bigger change than my simple "bug fix" calls
    for.
2) I'm not familiar enough with the alias oracle to say for sure
    if it can be used to give the same results as the existing
    implementation.  I.e., make it possible to identify and
    isolate each path that returns a local address (rather than
    just answering: yes, this pointer may point to some local
    in this function).

If the alias oracle can be used to give the same results without
excessive false positives then I think it would be fine to make
use of it.  Is that something you consider a prerequisite for
this change or should I look into it as a followup?

FWIW, I'm working on enhancing this to detect returning freed
pointers (under a different option).  That seems like a good
opportunity to also look into making use of the alias oracle.

Besides these enhancements, there's also a request to diagnose
dereferencing pointers to compound literals whose lifetime has
ended (PR 89990), or more generally, those to any such local
object.  It's about preventing essentially the same problem
(accessing bad data or corrupting others) but it seems that
both the analysis and the handling will be sufficiently
different to consider implementing it somewhere else.  What
are your thoughts on that?

Martin

> 
> The rest looks pretty reasonable.
> 
> Jeff
>

On 5/30/19 8:52 AM, Martin Sebor wrote:
> On 5/29/19 11:45 AM, Jeff Law wrote:
>> On 5/22/19 3:34 PM, Martin Sebor wrote:
>>> -Wreturn-local-addr detects a subset of instances of returning
>>> the address of a local object from a function but the warning
>>> doesn't try to handle alloca or VLAs, or some non-trivial cases
>>> of ordinary automatic variables[1].
>>>
>>> The attached patch extends the implementation of the warning to
>>> detect those.  It still doesn't detect instances where the address
>>> is the result of a built-in such strcpy[2].
>>>
>>> Tested on x86_64-linux.
>>>
>>> Martin
>>>
>>> [1] For example, this is only diagnosed with the patch:
>>>
>>>    void* f (int i)
>>>    {
>>>      struct S { int a[2]; } s[2];
>>>      return &s->a[i];
>>>    }
>>>
>>> [2] The following is not diagnosed even with the patch:
>>>
>>>    void sink (void*);
>>>
>>>    void* f (int i)
>>>    {
>>>      char a[6];
>>>      char *p = __builtin_strcpy (a, "123");
>>>      sink (p);
>>>      return p;
>>>    }
>>>
>>> I would expect detecting to be possible and useful.  Maybe as
>>> a follow-up.
>>>
>>> gcc-71924.diff
>>>
>>> PR middle-end/71924 - missing -Wreturn-local-addr returning alloca
>>> result
>>> PR middle-end/90549 - missing -Wreturn-local-addr maybe returning an
>>> address of a local array plus offset
>>>
>>> gcc/ChangeLog:
>>>
>>>     PR c/71924
>>>     * gimple-ssa-isolate-paths.c (is_addr_local): New function.
>>>     (warn_return_addr_local_phi_arg, warn_return_addr_local): Same.
>>>     (find_implicit_erroneous_behavior): Call
>>> warn_return_addr_local_phi_arg.
>>>     (find_explicit_erroneous_behavior): Call warn_return_addr_local.
>>>
>>> gcc/testsuite/ChangeLog:
>>>
>>>     PR c/71924
>>>     * gcc.dg/Wreturn-local-addr-2.c: New test.
>>>     * gcc.dg/Walloca-4.c: Prune expected warnings.
>>>     * gcc.dg/pr41551.c: Same.
>>>     * gcc.dg/pr59523.c: Same.
>>>     * gcc.dg/tree-ssa/pr88775-2.c: Same.
>>>     * gcc.dg/winline-7.c: Same.
>>>
>>> diff --git a/gcc/gimple-ssa-isolate-paths.c
>>> b/gcc/gimple-ssa-isolate-paths.c
>>> index 33fe352bb23..2933ecf502e 100644
>>> --- a/gcc/gimple-ssa-isolate-paths.c
>>> +++ b/gcc/gimple-ssa-isolate-paths.c
>>> @@ -341,6 +341,135 @@ stmt_uses_0_or_null_in_undefined_way (gimple
>>> *stmt)
>>>     return false;
>>>   }
>>>   +/* Return true if EXPR is a expression of pointer type that refers
>>> +   to the address of a variable with automatic storage duration.
>>> +   If so, set *PLOC to the location of the object or the call that
>>> +   allocated it (for alloca and VLAs).  When PMAYBE is non-null,
>>> +   also consider PHI statements and set *PMAYBE when some but not
>>> +   all arguments of such statements refer to local variables, and
>>> +   to clear it otherwise.  */
>>> +
>>> +static bool
>>> +is_addr_local (tree exp, location_t *ploc, bool *pmaybe = NULL,
>>> +           hash_set<gphi *> *visited = NULL)
>>> +{
>>> +  if (TREE_CODE (exp) == ADDR_EXPR)
>>> +    {
>>> +      tree baseaddr = get_base_address (TREE_OPERAND (exp, 0));
>>> +      if (TREE_CODE (baseaddr) == MEM_REF)
>>> +    return is_addr_local (TREE_OPERAND (baseaddr, 0), ploc, pmaybe,
>>> visited);
>>> +
>>> +      if ((!VAR_P (baseaddr)
>>> +       || is_global_var (baseaddr))
>>> +      && TREE_CODE (baseaddr) != PARM_DECL)
>>> +    return false;
>>> +
>>> +      *ploc = DECL_SOURCE_LOCATION (baseaddr);
>>> +      return true;
>>> +    }
>>> +
>>> +  if (TREE_CODE (exp) == SSA_NAME)
>>> +    {
>>> +      gimple *def_stmt = SSA_NAME_DEF_STMT (exp);
>>> +      enum gimple_code code = gimple_code (def_stmt);
>>> +
>>> +      if (is_gimple_assign (def_stmt))
>>> +    {
>>> +      tree type = TREE_TYPE (gimple_assign_lhs (def_stmt));
>>> +      if (POINTER_TYPE_P (type))
>>> +        {
>>> +          tree ptr = gimple_assign_rhs1 (def_stmt);
>>> +          return is_addr_local (ptr, ploc, pmaybe, visited);
>>> +        }
>>> +      return false;
>>> +    }
>>> +
>>> +      if (code == GIMPLE_CALL
>>> +      && gimple_call_builtin_p (def_stmt))
>>> +    {
>>> +      tree fn = gimple_call_fndecl (def_stmt);
>>> +      int code = DECL_FUNCTION_CODE (fn);
>>> +      if (code != BUILT_IN_ALLOCA
>>> +          && code != BUILT_IN_ALLOCA_WITH_ALIGN)
>>> +        return false;
>>> +
>>> +      *ploc = gimple_location (def_stmt);
>>> +      return true;
>>> +    }
>>> +
>>> +      if (code == GIMPLE_PHI && pmaybe)
>>> +    {
>>> +      unsigned count = 0;
>>> +      gphi *phi_stmt = as_a <gphi *> (def_stmt);
>>> +
>>> +      unsigned nargs = gimple_phi_num_args (phi_stmt);
>>> +      for (unsigned i = 0; i < nargs; ++i)
>>> +        {
>>> +          if (!visited->add (phi_stmt))
>>> +        {
>>> +          tree arg = gimple_phi_arg_def (phi_stmt, i);
>>> +          if (is_addr_local (arg, ploc, pmaybe, visited))
>>> +            ++count;
>>> +        }
>>> +        }
>>> +
>>> +      *pmaybe = count && count < nargs;
>>> +      return count != 0;
>>> +    }
>>> +    }
>>> +
>>> +  return false;
>>> +}
>> Is there some reason you didn't query the alias oracle here?  It would
>> seem a fairly natural fit.  Ultimately given a pointer (which will be an
>> SSA_NAME) you want to ask whether or not it conclusively points into the
>> stack.
>>
>> That would seem to dramatically simplify is_addr_local.
> 
> I did think about it but decided against changing the existing
> design (iterating over PHI arguments), for a couple of reasons:
> 
> 1) It feels like a bigger change than my simple "bug fix" calls
>    for.
I suspect the net result will be simpler though ;-)  I think you just
get a pt solution and iterate over the things the solution says the
pointer can point to.


> 2) I'm not familiar enough with the alias oracle to say for sure
>    if it can be used to give the same results as the existing
>    implementation.  I.e., make it possible to identify and
>    isolate each path that returns a local address (rather than
>    just answering: yes, this pointer may point to some local
>    in this function).
Precision of the oracle is certainly the big question.


> 
> If the alias oracle can be used to give the same results without
> excessive false positives then I think it would be fine to make
> use of it.  Is that something you consider a prerequisite for
> this change or should I look into it as a followup?
I think we should explore it a bit before making a final decision.  It
may guide us for other work in this space (like detecting escaping
locals).   I think a dirty prototype to see if it's even in the right
ballpark would make sense.



> 
> FWIW, I'm working on enhancing this to detect returning freed
> pointers (under a different option).  That seems like a good
> opportunity to also look into making use of the alias oracle.
Yes.  I think there's two interesting cases here to ponder.  If we free
a pointer that must point to a local, then we can warn & trap.  If we
free a pointer that may point to a local, then we can only warn (unless
we can isolate the path).


> 
> Besides these enhancements, there's also a request to diagnose
> dereferencing pointers to compound literals whose lifetime has
> ended (PR 89990), or more generally, those to any such local
> object.  It's about preventing essentially the same problem
> (accessing bad data or corrupting others) but it seems that
> both the analysis and the handling will be sufficiently
> different to consider implementing it somewhere else.  What
> are your thoughts on that?
I think the tough problem here is we lose binding scopes as we lower
into gimple, so solving it in the general case may be tough.  We've
started adding some clobbers into the IL to denote object death points,
but I'm not sure if they're sufficient to implement this kind of warning.

Jeff

On 5/30/19 9:13 AM, Jeff Law wrote:
> On 5/30/19 8:52 AM, Martin Sebor wrote:
>> On 5/29/19 11:45 AM, Jeff Law wrote:
>>> On 5/22/19 3:34 PM, Martin Sebor wrote:
>>>> -Wreturn-local-addr detects a subset of instances of returning
>>>> the address of a local object from a function but the warning
>>>> doesn't try to handle alloca or VLAs, or some non-trivial cases
>>>> of ordinary automatic variables[1].
>>>>
>>>> The attached patch extends the implementation of the warning to
>>>> detect those.  It still doesn't detect instances where the address
>>>> is the result of a built-in such strcpy[2].
>>>>
>>>> Tested on x86_64-linux.
>>>>
>>>> Martin
>>>>
>>>> [1] For example, this is only diagnosed with the patch:
>>>>
>>>>     void* f (int i)
>>>>     {
>>>>       struct S { int a[2]; } s[2];
>>>>       return &s->a[i];
>>>>     }
>>>>
>>>> [2] The following is not diagnosed even with the patch:
>>>>
>>>>     void sink (void*);
>>>>
>>>>     void* f (int i)
>>>>     {
>>>>       char a[6];
>>>>       char *p = __builtin_strcpy (a, "123");
>>>>       sink (p);
>>>>       return p;
>>>>     }
>>>>
>>>> I would expect detecting to be possible and useful.  Maybe as
>>>> a follow-up.
>>>>
>>>> gcc-71924.diff
>>>>
>>>> PR middle-end/71924 - missing -Wreturn-local-addr returning alloca
>>>> result
>>>> PR middle-end/90549 - missing -Wreturn-local-addr maybe returning an
>>>> address of a local array plus offset
>>>>
>>>> gcc/ChangeLog:
>>>>
>>>>      PR c/71924
>>>>      * gimple-ssa-isolate-paths.c (is_addr_local): New function.
>>>>      (warn_return_addr_local_phi_arg, warn_return_addr_local): Same.
>>>>      (find_implicit_erroneous_behavior): Call
>>>> warn_return_addr_local_phi_arg.
>>>>      (find_explicit_erroneous_behavior): Call warn_return_addr_local.
>>>>
>>>> gcc/testsuite/ChangeLog:
>>>>
>>>>      PR c/71924
>>>>      * gcc.dg/Wreturn-local-addr-2.c: New test.
>>>>      * gcc.dg/Walloca-4.c: Prune expected warnings.
>>>>      * gcc.dg/pr41551.c: Same.
>>>>      * gcc.dg/pr59523.c: Same.
>>>>      * gcc.dg/tree-ssa/pr88775-2.c: Same.
>>>>      * gcc.dg/winline-7.c: Same.
>>>>
>>>> diff --git a/gcc/gimple-ssa-isolate-paths.c
>>>> b/gcc/gimple-ssa-isolate-paths.c
>>>> index 33fe352bb23..2933ecf502e 100644
>>>> --- a/gcc/gimple-ssa-isolate-paths.c
>>>> +++ b/gcc/gimple-ssa-isolate-paths.c
>>>> @@ -341,6 +341,135 @@ stmt_uses_0_or_null_in_undefined_way (gimple
>>>> *stmt)
>>>>      return false;
>>>>    }
>>>>    +/* Return true if EXPR is a expression of pointer type that refers
>>>> +   to the address of a variable with automatic storage duration.
>>>> +   If so, set *PLOC to the location of the object or the call that
>>>> +   allocated it (for alloca and VLAs).  When PMAYBE is non-null,
>>>> +   also consider PHI statements and set *PMAYBE when some but not
>>>> +   all arguments of such statements refer to local variables, and
>>>> +   to clear it otherwise.  */
>>>> +
>>>> +static bool
>>>> +is_addr_local (tree exp, location_t *ploc, bool *pmaybe = NULL,
>>>> +           hash_set<gphi *> *visited = NULL)
>>>> +{
>>>> +  if (TREE_CODE (exp) == ADDR_EXPR)
>>>> +    {
>>>> +      tree baseaddr = get_base_address (TREE_OPERAND (exp, 0));
>>>> +      if (TREE_CODE (baseaddr) == MEM_REF)
>>>> +    return is_addr_local (TREE_OPERAND (baseaddr, 0), ploc, pmaybe,
>>>> visited);
>>>> +
>>>> +      if ((!VAR_P (baseaddr)
>>>> +       || is_global_var (baseaddr))
>>>> +      && TREE_CODE (baseaddr) != PARM_DECL)
>>>> +    return false;
>>>> +
>>>> +      *ploc = DECL_SOURCE_LOCATION (baseaddr);
>>>> +      return true;
>>>> +    }
>>>> +
>>>> +  if (TREE_CODE (exp) == SSA_NAME)
>>>> +    {
>>>> +      gimple *def_stmt = SSA_NAME_DEF_STMT (exp);
>>>> +      enum gimple_code code = gimple_code (def_stmt);
>>>> +
>>>> +      if (is_gimple_assign (def_stmt))
>>>> +    {
>>>> +      tree type = TREE_TYPE (gimple_assign_lhs (def_stmt));
>>>> +      if (POINTER_TYPE_P (type))
>>>> +        {
>>>> +          tree ptr = gimple_assign_rhs1 (def_stmt);
>>>> +          return is_addr_local (ptr, ploc, pmaybe, visited);
>>>> +        }
>>>> +      return false;
>>>> +    }
>>>> +
>>>> +      if (code == GIMPLE_CALL
>>>> +      && gimple_call_builtin_p (def_stmt))
>>>> +    {
>>>> +      tree fn = gimple_call_fndecl (def_stmt);
>>>> +      int code = DECL_FUNCTION_CODE (fn);
>>>> +      if (code != BUILT_IN_ALLOCA
>>>> +          && code != BUILT_IN_ALLOCA_WITH_ALIGN)
>>>> +        return false;
>>>> +
>>>> +      *ploc = gimple_location (def_stmt);
>>>> +      return true;
>>>> +    }
>>>> +
>>>> +      if (code == GIMPLE_PHI && pmaybe)
>>>> +    {
>>>> +      unsigned count = 0;
>>>> +      gphi *phi_stmt = as_a <gphi *> (def_stmt);
>>>> +
>>>> +      unsigned nargs = gimple_phi_num_args (phi_stmt);
>>>> +      for (unsigned i = 0; i < nargs; ++i)
>>>> +        {
>>>> +          if (!visited->add (phi_stmt))
>>>> +        {
>>>> +          tree arg = gimple_phi_arg_def (phi_stmt, i);
>>>> +          if (is_addr_local (arg, ploc, pmaybe, visited))
>>>> +            ++count;
>>>> +        }
>>>> +        }
>>>> +
>>>> +      *pmaybe = count && count < nargs;
>>>> +      return count != 0;
>>>> +    }
>>>> +    }
>>>> +
>>>> +  return false;
>>>> +}
>>> Is there some reason you didn't query the alias oracle here?  It would
>>> seem a fairly natural fit.  Ultimately given a pointer (which will be an
>>> SSA_NAME) you want to ask whether or not it conclusively points into the
>>> stack.
>>>
>>> That would seem to dramatically simplify is_addr_local.
>>
>> I did think about it but decided against changing the existing
>> design (iterating over PHI arguments), for a couple of reasons:
>>
>> 1) It feels like a bigger change than my simple "bug fix" calls
>>     for.
> I suspect the net result will be simpler though ;-)  I think you just
> get a pt solution and iterate over the things the solution says the
> pointer can point to.
> 
> 
>> 2) I'm not familiar enough with the alias oracle to say for sure
>>     if it can be used to give the same results as the existing
>>     implementation.  I.e., make it possible to identify and
>>     isolate each path that returns a local address (rather than
>>     just answering: yes, this pointer may point to some local
>>     in this function).
> Precision of the oracle is certainly the big question.
> 
> 
>>
>> If the alias oracle can be used to give the same results without
>> excessive false positives then I think it would be fine to make
>> use of it.  Is that something you consider a prerequisite for
>> this change or should I look into it as a followup?
> I think we should explore it a bit before making a final decision.  It
> may guide us for other work in this space (like detecting escaping
> locals).   I think a dirty prototype to see if it's even in the right
> ballpark would make sense.

Okay, let me look into it.

>> FWIW, I'm working on enhancing this to detect returning freed
>> pointers (under a different option).  That seems like a good
>> opportunity to also look into making use of the alias oracle.
> Yes.  I think there's two interesting cases here to ponder.  If we free
> a pointer that must point to a local, then we can warn & trap.  If we
> free a pointer that may point to a local, then we can only warn (unless
> we can isolate the path).

I wasn't actually thinking of freeing locals but it sounds like
a useful enhancement as well.  Thanks for the suggestion! :)

To be clear, what I'm working on is detecting:

   void* f (void *p)
   {
     free (p);   // note: pointer was freed here
     // ...
     return p;   // warning: returning a freed pointer
   }

>> Besides these enhancements, there's also a request to diagnose
>> dereferencing pointers to compound literals whose lifetime has
>> ended (PR 89990), or more generally, those to any such local
>> object.  It's about preventing essentially the same problem
>> (accessing bad data or corrupting others) but it seems that
>> both the analysis and the handling will be sufficiently
>> different to consider implementing it somewhere else.  What
>> are your thoughts on that?
> I think the tough problem here is we lose binding scopes as we lower
> into gimple, so solving it in the general case may be tough.  We've
> started adding some clobbers into the IL to denote object death points,
> but I'm not sure if they're sufficient to implement this kind of warning.

I was afraid that might be a problem.

Thanks
Martin

On 5/30/19 9:34 AM, Martin Sebor wrote:

>>> If the alias oracle can be used to give the same results without
>>> excessive false positives then I think it would be fine to make
>>> use of it.  Is that something you consider a prerequisite for
>>> this change or should I look into it as a followup?
>> I think we should explore it a bit before making a final decision.  It
>> may guide us for other work in this space (like detecting escaping
>> locals).   I think a dirty prototype to see if it's even in the right
>> ballpark would make sense.
> 
> Okay, let me look into it.
Sounds good.  Again, go with a quick prototype to see if it's likely
feasible.  The tests you've added should dramatically help evaluating if
the oracle is up to the task.

> 
>>> FWIW, I'm working on enhancing this to detect returning freed
>>> pointers (under a different option).  That seems like a good
>>> opportunity to also look into making use of the alias oracle.
>> Yes.  I think there's two interesting cases here to ponder.  If we free
>> a pointer that must point to a local, then we can warn & trap.  If we
>> free a pointer that may point to a local, then we can only warn (unless
>> we can isolate the path).
> 
> I wasn't actually thinking of freeing locals but it sounds like
> a useful enhancement as well.  Thanks for the suggestion! :)
> 
> To be clear, what I'm working on is detecting:
> 
>   void* f (void *p)
>   {
>     free (p);   // note: pointer was freed here
>     // ...
>     return p;   // warning: returning a freed pointer
>   }
Ah, we were talking about different things. Though what you're doing
might be better modeled in a true global static analyzer as a
use-after-free problem.  My sense is that translation-unit local version
of that problem really isn't that useful in practice.  THough I guess
there isn't anything bad with having a TU local version.


> 
>>> Besides these enhancements, there's also a request to diagnose
>>> dereferencing pointers to compound literals whose lifetime has
>>> ended (PR 89990), or more generally, those to any such local
>>> object.  It's about preventing essentially the same problem
>>> (accessing bad data or corrupting others) but it seems that
>>> both the analysis and the handling will be sufficiently
>>> different to consider implementing it somewhere else.  What
>>> are your thoughts on that?
>> I think the tough problem here is we lose binding scopes as we lower
>> into gimple, so solving it in the general case may be tough.  We've
>> started adding some clobbers into the IL to denote object death points,
>> but I'm not sure if they're sufficient to implement this kind of warning.
> 
> I was afraid that might be a problem.
Way back in the early days of tree-ssa we kept the binding scopes.  But
that proved problematical in various ways.  Mostly they just got in the
way of analysis an optimization and we spent far too much time in the
optimizers working around them or removing those which were empty.

They'd be helpful in this kind of analysis, stack slot sharing vs the
inliner and a couple other problems.  I don't know if the pendulum has
moved far enough to revisit :-)

jeff

On Thu, May 30, 2019 at 12:16 PM Jeff Law <law@redhat.com> wrote:
>
> On 5/30/19 9:34 AM, Martin Sebor wrote:
>
> >>> If the alias oracle can be used to give the same results without
> >>> excessive false positives then I think it would be fine to make
> >>> use of it.  Is that something you consider a prerequisite for
> >>> this change or should I look into it as a followup?
> >> I think we should explore it a bit before making a final decision.  It
> >> may guide us for other work in this space (like detecting escaping
> >> locals).   I think a dirty prototype to see if it's even in the right
> >> ballpark would make sense.
> >
> > Okay, let me look into it.
> Sounds good.  Again, go with a quick prototype to see if it's likely
> feasible.  The tests you've added should dramatically help evaluating if
> the oracle is up to the task.
>
> >
> >>> FWIW, I'm working on enhancing this to detect returning freed
> >>> pointers (under a different option).  That seems like a good
> >>> opportunity to also look into making use of the alias oracle.
> >> Yes.  I think there's two interesting cases here to ponder.  If we free
> >> a pointer that must point to a local, then we can warn & trap.  If we
> >> free a pointer that may point to a local, then we can only warn (unless
> >> we can isolate the path).
> >
> > I wasn't actually thinking of freeing locals but it sounds like
> > a useful enhancement as well.  Thanks for the suggestion! :)
> >
> > To be clear, what I'm working on is detecting:
> >
> >   void* f (void *p)
> >   {
> >     free (p);   // note: pointer was freed here
> >     // ...
> >     return p;   // warning: returning a freed pointer
> >   }
> Ah, we were talking about different things. Though what you're doing
> might be better modeled in a true global static analyzer as a
> use-after-free problem.  My sense is that translation-unit local version
> of that problem really isn't that useful in practice.  THough I guess
> there isn't anything bad with having a TU local version.
>
>
> >
> >>> Besides these enhancements, there's also a request to diagnose
> >>> dereferencing pointers to compound literals whose lifetime has
> >>> ended (PR 89990), or more generally, those to any such local
> >>> object.  It's about preventing essentially the same problem
> >>> (accessing bad data or corrupting others) but it seems that
> >>> both the analysis and the handling will be sufficiently
> >>> different to consider implementing it somewhere else.  What
> >>> are your thoughts on that?
> >> I think the tough problem here is we lose binding scopes as we lower
> >> into gimple, so solving it in the general case may be tough.  We've
> >> started adding some clobbers into the IL to denote object death points,
> >> but I'm not sure if they're sufficient to implement this kind of warning.
> >
> > I was afraid that might be a problem.
> Way back in the early days of tree-ssa we kept the binding scopes.  But
> that proved problematical in various ways.  Mostly they just got in the
> way of analysis an optimization and we spent far too much time in the
> optimizers working around them or removing those which were empty.
>
> They'd be helpful in this kind of analysis, stack slot sharing vs the
> inliner and a couple other problems.  I don't know if the pendulum has
> moved far enough to revisit :-)

Why wouldn't clobbers be sufficient?

Jaason

On 5/30/19 10:15 AM, Jeff Law wrote:
> On 5/30/19 9:34 AM, Martin Sebor wrote:
> 
>>>> If the alias oracle can be used to give the same results without
>>>> excessive false positives then I think it would be fine to make
>>>> use of it.  Is that something you consider a prerequisite for
>>>> this change or should I look into it as a followup?
>>> I think we should explore it a bit before making a final decision.  It
>>> may guide us for other work in this space (like detecting escaping
>>> locals).   I think a dirty prototype to see if it's even in the right
>>> ballpark would make sense.
>>
>> Okay, let me look into it.
> Sounds good.  Again, go with a quick prototype to see if it's likely
> feasible.  The tests you've added should dramatically help evaluating if
> the oracle is up to the task.

So to expand on what I said on the phone when we spoke: the problem
I quickly ran into with the prototype is that I wasn't able to find
a way to identify pointers to alloca/VLA storage.

In the the points-to solution for the pointer being returned they
both have the vars_contains_escaped_heap flag set.  That seems like
an omission that shouldn't be hard to fix, but on its own, I don't
think it would be sufficient.

In the IL a VLA is represented as a pointer to an array, but when
returning a pointer into a VLA (at some offset so it's an SSA_NAME),
the pointer's point-to solution doesn't include the VLA pointer or
(AFAICS) make it possible to tell even that it is a VLA.  For example
here:

   f (int n)
   {
     int * p;
     int[0:D.1912] * a.1;
     sizetype _1;
     void * saved_stack.3_3;
     sizetype _6;

     <bb 2> [local count: 1073741824]:
     saved_stack.3_3 = __builtin_stack_save ();
     _1 = (sizetype) n_2(D);
     _6 = _1 * 4;
     a.1_8 = __builtin_alloca_with_align (_6, 32);
     p_9 = a.1_8 + _6;
     __builtin_stack_restore (saved_stack.3_3);
     return p_9;
   }

p_9's solution's is:

   p_9, points-to vars: { D.1925 } (escaped, escaped heap)

I couldn't find out how to determine that D.1925 is a VLA (or even
what it is).  It's not among the function's local variables that
FOR_EACH_LOCAL_DECL iterates over.

By replacing the VLA in the test case with a call to malloc I get
this for the returned p_7:

   p_7, points-to NULL, points-to vars: { D.1916 } (escaped, escaped heap)

Again, I see no way to quickly tell that this pointer points into
the block returned from malloc.

I'm sure Richard will correct me if there is a simple way to do it
but I couldn't find one.

If there is a way to identify that the returned pointer is for
a VLA (or alloca), for parity with the current patch we also
need to find the location of the VLA declaration or the alloca
call so that the warning could point to it.  Unless there's
a straight path from the mysterious D.1925 above to that
decl/call statement, finding it would likely require some
sor of traversal.  At that point I wouldn't be surprised if
the complexity of the solution didn't approach or even exceed
the current approach.

Martin

>>>> FWIW, I'm working on enhancing this to detect returning freed
>>>> pointers (under a different option).  That seems like a good
>>>> opportunity to also look into making use of the alias oracle.
>>> Yes.  I think there's two interesting cases here to ponder.  If we free
>>> a pointer that must point to a local, then we can warn & trap.  If we
>>> free a pointer that may point to a local, then we can only warn (unless
>>> we can isolate the path).
>>
>> I wasn't actually thinking of freeing locals but it sounds like
>> a useful enhancement as well.  Thanks for the suggestion! :)
>>
>> To be clear, what I'm working on is detecting:
>>
>>    void* f (void *p)
>>    {
>>      free (p);   // note: pointer was freed here
>>      // ...
>>      return p;   // warning: returning a freed pointer
>>    }
> Ah, we were talking about different things. Though what you're doing
> might be better modeled in a true global static analyzer as a
> use-after-free problem.  My sense is that translation-unit local version
> of that problem really isn't that useful in practice.  THough I guess
> there isn't anything bad with having a TU local version.
> 
> 
>>
>>>> Besides these enhancements, there's also a request to diagnose
>>>> dereferencing pointers to compound literals whose lifetime has
>>>> ended (PR 89990), or more generally, those to any such local
>>>> object.  It's about preventing essentially the same problem
>>>> (accessing bad data or corrupting others) but it seems that
>>>> both the analysis and the handling will be sufficiently
>>>> different to consider implementing it somewhere else.  What
>>>> are your thoughts on that?
>>> I think the tough problem here is we lose binding scopes as we lower
>>> into gimple, so solving it in the general case may be tough.  We've
>>> started adding some clobbers into the IL to denote object death points,
>>> but I'm not sure if they're sufficient to implement this kind of warning.
>>
>> I was afraid that might be a problem.
> Way back in the early days of tree-ssa we kept the binding scopes.  But
> that proved problematical in various ways.  Mostly they just got in the
> way of analysis an optimization and we spent far too much time in the
> optimizers working around them or removing those which were empty.
> 
> They'd be helpful in this kind of analysis, stack slot sharing vs the
> inliner and a couple other problems.  I don't know if the pendulum has
> moved far enough to revisit :-)
> 
> jeff
>

On 5/30/19 11:23 AM, Jason Merrill wrote:
> On Thu, May 30, 2019 at 12:16 PM Jeff Law <law@redhat.com> wrote:
>>
>> On 5/30/19 9:34 AM, Martin Sebor wrote:
>>
>>>>> If the alias oracle can be used to give the same results without
>>>>> excessive false positives then I think it would be fine to make
>>>>> use of it.  Is that something you consider a prerequisite for
>>>>> this change or should I look into it as a followup?
>>>> I think we should explore it a bit before making a final decision.  It
>>>> may guide us for other work in this space (like detecting escaping
>>>> locals).   I think a dirty prototype to see if it's even in the right
>>>> ballpark would make sense.
>>>
>>> Okay, let me look into it.
>> Sounds good.  Again, go with a quick prototype to see if it's likely
>> feasible.  The tests you've added should dramatically help evaluating if
>> the oracle is up to the task.
>>
>>>
>>>>> FWIW, I'm working on enhancing this to detect returning freed
>>>>> pointers (under a different option).  That seems like a good
>>>>> opportunity to also look into making use of the alias oracle.
>>>> Yes.  I think there's two interesting cases here to ponder.  If we free
>>>> a pointer that must point to a local, then we can warn & trap.  If we
>>>> free a pointer that may point to a local, then we can only warn (unless
>>>> we can isolate the path).
>>>
>>> I wasn't actually thinking of freeing locals but it sounds like
>>> a useful enhancement as well.  Thanks for the suggestion! :)
>>>
>>> To be clear, what I'm working on is detecting:
>>>
>>>   void* f (void *p)
>>>   {
>>>     free (p);   // note: pointer was freed here
>>>     // ...
>>>     return p;   // warning: returning a freed pointer
>>>   }
>> Ah, we were talking about different things. Though what you're doing
>> might be better modeled in a true global static analyzer as a
>> use-after-free problem.  My sense is that translation-unit local version
>> of that problem really isn't that useful in practice.  THough I guess
>> there isn't anything bad with having a TU local version.
>>
>>
>>>
>>>>> Besides these enhancements, there's also a request to diagnose
>>>>> dereferencing pointers to compound literals whose lifetime has
>>>>> ended (PR 89990), or more generally, those to any such local
>>>>> object.  It's about preventing essentially the same problem
>>>>> (accessing bad data or corrupting others) but it seems that
>>>>> both the analysis and the handling will be sufficiently
>>>>> different to consider implementing it somewhere else.  What
>>>>> are your thoughts on that?
>>>> I think the tough problem here is we lose binding scopes as we lower
>>>> into gimple, so solving it in the general case may be tough.  We've
>>>> started adding some clobbers into the IL to denote object death points,
>>>> but I'm not sure if they're sufficient to implement this kind of warning.
>>>
>>> I was afraid that might be a problem.
>> Way back in the early days of tree-ssa we kept the binding scopes.  But
>> that proved problematical in various ways.  Mostly they just got in the
>> way of analysis an optimization and we spent far too much time in the
>> optimizers working around them or removing those which were empty.
>>
>> They'd be helpful in this kind of analysis, stack slot sharing vs the
>> inliner and a couple other problems.  I don't know if the pendulum has
>> moved far enough to revisit :-)
> 
> Why wouldn't clobbers be sufficient?
I haven't looked into the clobbers in any detail.  If we're aggressively
emitting them at the end of object life, then they may be sufficient to
start tackling these issues.

jeff

On 5/30/19 4:56 PM, Martin Sebor wrote:
> On 5/30/19 10:15 AM, Jeff Law wrote:
>> On 5/30/19 9:34 AM, Martin Sebor wrote:
>>
>>>>> If the alias oracle can be used to give the same results without
>>>>> excessive false positives then I think it would be fine to make
>>>>> use of it.  Is that something you consider a prerequisite for
>>>>> this change or should I look into it as a followup?
>>>> I think we should explore it a bit before making a final decision.  It
>>>> may guide us for other work in this space (like detecting escaping
>>>> locals).   I think a dirty prototype to see if it's even in the right
>>>> ballpark would make sense.
>>>
>>> Okay, let me look into it.
>> Sounds good.  Again, go with a quick prototype to see if it's likely
>> feasible.  The tests you've added should dramatically help evaluating if
>> the oracle is up to the task.
> 
> So to expand on what I said on the phone when we spoke: the problem
> I quickly ran into with the prototype is that I wasn't able to find
> a way to identify pointers to alloca/VLA storage.
Your analysis matches my very quick read of the aliasing code.  It may
be the case that the Steensgaard patent got in the way here.


> 
> In the the points-to solution for the pointer being returned they
> both have the vars_contains_escaped_heap flag set.  That seems like
> an omission that shouldn't be hard to fix, but on its own, I don't
> think it would be sufficient.
RIght.  In theory the result of an alloca call shouldn't alias anything
in the heap -- but there were some implementations of alloca that were
built on top of malloc (ugh).  That flag may be catering to that case.

But in the case of a __builtin_alloca that shouldn't apply.  Hmm.  That
ultimately might be a bug.



> 
> In the IL a VLA is represented as a pointer to an array, but when
> returning a pointer into a VLA (at some offset so it's an SSA_NAME),
> the pointer's point-to solution doesn't include the VLA pointer or
> (AFAICS) make it possible to tell even that it is a VLA.  For example
> here:
> 
>   f (int n)
>   {
>     int * p;
>     int[0:D.1912] * a.1;
>     sizetype _1;
>     void * saved_stack.3_3;
>     sizetype _6;
> 
>     <bb 2> [local count: 1073741824]:
>     saved_stack.3_3 = __builtin_stack_save ();
>     _1 = (sizetype) n_2(D);
>     _6 = _1 * 4;
>     a.1_8 = __builtin_alloca_with_align (_6, 32);
>     p_9 = a.1_8 + _6;
>     __builtin_stack_restore (saved_stack.3_3);
>     return p_9;
>   }
> 
> p_9's solution's is:
> 
>   p_9, points-to vars: { D.1925 } (escaped, escaped heap)
> 
> I couldn't find out how to determine that D.1925 is a VLA (or even
> what it is).  It's not among the function's local variables that
> FOR_EACH_LOCAL_DECL iterates over.
It's possible that decl was created internally as part of the alias
oracle's analysis.

See make_heapvar in tree-ssa-structalias.c
> 
> By replacing the VLA in the test case with a call to malloc I get
> this for the returned p_7:
> 
>   p_7, points-to NULL, points-to vars: { D.1916 } (escaped, escaped heap)
> 
> Again, I see no way to quickly tell that this pointer points into
> the block returned from malloc.
If there's a way to make that determination it'd have to be on the
variable since the pt_solution flag bits don't carry a storage class
directly.

You might try to get a handle on those decls and dump them to see if
there's anything easily identifiable.  But it may be easier to dig into
the code that creates them.  A real quick scan of the aliasing code also
shows the "variable_info" structure.  It's private to the aliasing code,
but might guide you at things to look at.

Regardless, I don't see an immediate path forward  using the oracle to
identify objects in the stack for your patch.  WHich is unfortunate.




Jeff

On 5/22/19 3:34 PM, Martin Sebor wrote:
> -Wreturn-local-addr detects a subset of instances of returning
> the address of a local object from a function but the warning
> doesn't try to handle alloca or VLAs, or some non-trivial cases
> of ordinary automatic variables[1].
> 
> The attached patch extends the implementation of the warning to
> detect those.  It still doesn't detect instances where the address
> is the result of a built-in such strcpy[2].
> 
> Tested on x86_64-linux.
> 
> Martin
> 
> [1] For example, this is only diagnosed with the patch:
> 
>   void* f (int i)
>   {
>     struct S { int a[2]; } s[2];
>     return &s->a[i];
>   }
> 
> [2] The following is not diagnosed even with the patch:
> 
>   void sink (void*);
> 
>   void* f (int i)
>   {
>     char a[6];
>     char *p = __builtin_strcpy (a, "123");
>     sink (p);
>     return p;
>   }
> 
> I would expect detecting to be possible and useful.  Maybe as
> a follow-up.
> 
> gcc-71924.diff
> 
> PR middle-end/71924 - missing -Wreturn-local-addr returning alloca result
> PR middle-end/90549 - missing -Wreturn-local-addr maybe returning an address of a local array plus offset
> 
> gcc/ChangeLog:
> 
> 	PR c/71924
> 	* gimple-ssa-isolate-paths.c (is_addr_local): New function.
> 	(warn_return_addr_local_phi_arg, warn_return_addr_local): Same.
> 	(find_implicit_erroneous_behavior): Call warn_return_addr_local_phi_arg.
> 	(find_explicit_erroneous_behavior): Call warn_return_addr_local.
> 
> gcc/testsuite/ChangeLog:
> 
> 	PR c/71924
> 	* gcc.dg/Wreturn-local-addr-2.c: New test.
> 	* gcc.dg/Walloca-4.c: Prune expected warnings.
> 	* gcc.dg/pr41551.c: Same.
> 	* gcc.dg/pr59523.c: Same.
> 	* gcc.dg/tree-ssa/pr88775-2.c: Same.
> 	* gcc.dg/winline-7.c: Same.
> 
> diff --git a/gcc/gimple-ssa-isolate-paths.c b/gcc/gimple-ssa-isolate-paths.c
> index 33fe352bb23..2933ecf502e 100644
> --- a/gcc/gimple-ssa-isolate-paths.c
> +++ b/gcc/gimple-ssa-isolate-paths.c
> @@ -341,6 +341,135 @@ stmt_uses_0_or_null_in_undefined_way (gimple *stmt)
>    return false;
>  }
>  
> +/* Return true if EXPR is a expression of pointer type that refers
> +   to the address of a variable with automatic storage duration.
> +   If so, set *PLOC to the location of the object or the call that
> +   allocated it (for alloca and VLAs).  When PMAYBE is non-null,
> +   also consider PHI statements and set *PMAYBE when some but not
> +   all arguments of such statements refer to local variables, and
> +   to clear it otherwise.  */
> +
> +static bool
> +is_addr_local (tree exp, location_t *ploc, bool *pmaybe = NULL,
> +	       hash_set<gphi *> *visited = NULL)
> +{
> +  if (TREE_CODE (exp) == SSA_NAME)
> +    {
> +      gimple *def_stmt = SSA_NAME_DEF_STMT (exp);
> +      enum gimple_code code = gimple_code (def_stmt);
> +
> +      if (is_gimple_assign (def_stmt))
> +	{
> +	  tree type = TREE_TYPE (gimple_assign_lhs (def_stmt));
> +	  if (POINTER_TYPE_P (type))
> +	    {
> +	      tree ptr = gimple_assign_rhs1 (def_stmt);
> +	      return is_addr_local (ptr, ploc, pmaybe, visited);
> +	    }
> +	  return false;
> +	}
So this is going to recurse on the rhs1 of something like
POINTER_PLUS_EXPR, that's a good thing :-)   But isn't it non-selective
about the codes where we recurse?

Consider

  ptr = (cond) ? res1 : res2

I think we'll end up recursing on the condition rather than looking at
res1 and res2.


I suspect there are a very limited number of expression codes that
appear on the RHS where we'd want to recurse on one or both operands.

POINTER_PLUS_EXPR, NOP_EXPR, maybe COND_EXPR (where you have to recurse
on both and logically and the result), BIT_AND (maybe we masked off some
bits in an address).  That's probably about it :-)

Are there any other codes you've seen or think would be useful in
practice to recurse through?  I'd rather list them explicitly rather
than just recurse down through every rhs1 we encounter.



> +
> +      if (code == GIMPLE_PHI && pmaybe)
> +	{
> +	  unsigned count = 0;
> +	  gphi *phi_stmt = as_a <gphi *> (def_stmt);
> +
> +	  unsigned nargs = gimple_phi_num_args (phi_stmt);
> +	  for (unsigned i = 0; i < nargs; ++i)
> +	    {
> +	      if (!visited->add (phi_stmt))
> +		{
> +		  tree arg = gimple_phi_arg_def (phi_stmt, i);
> +		  if (is_addr_local (arg, ploc, pmaybe, visited))
> +		    ++count;
> +		}
> +	    }
So imagine

p = phi (x1, x1, x2)

Where both x1 and x2 would pass is_addr_local.  I think this code would
incorrectly set pmaybe.

We process the first instance of x1, find it is local and bump count.
When we encounter the second instance, it's in our map and we do
nothing.  THen we process x2 and bump count again.  So count would be 2.
 But count is going to be less than nargs so we'll set *pmaybe to true.

ISTM you'd have to record the result for each phi argument and query
that to determine if you should bump the counter if you've already
visited the argument.

It also seems to me that you can break the loop as soon as you've got a
nonzero count and get a false return from is_addr_local.   Not sure if
that'll matter in practice or not.

The other approach here (and I'm not suggesting you implement it) would
be to use the propagation engine.  That's probably overkill here since
we'd probably end up computing a whole lot of things we don't need.  My
sense is an on-demand approach like you've done is probably less
computationally expensive.

jeff

On Fri, May 31, 2019 at 5:35 PM Jeff Law <law@redhat.com> wrote:>
> On 5/30/19 4:56 PM, Martin Sebor wrote:
> > On 5/30/19 10:15 AM, Jeff Law wrote:
> >> On 5/30/19 9:34 AM, Martin Sebor wrote:
> >>
> >>>>> If the alias oracle can be used to give the same results without
> >>>>> excessive false positives then I think it would be fine to make
> >>>>> use of it.  Is that something you consider a prerequisite for
> >>>>> this change or should I look into it as a followup?
> >>>> I think we should explore it a bit before making a final decision.  It
> >>>> may guide us for other work in this space (like detecting escaping
> >>>> locals).   I think a dirty prototype to see if it's even in the right
> >>>> ballpark would make sense.
> >>>
> >>> Okay, let me look into it.
> >> Sounds good.  Again, go with a quick prototype to see if it's likely
> >> feasible.  The tests you've added should dramatically help evaluating if
> >> the oracle is up to the task.
> >
> > So to expand on what I said on the phone when we spoke: the problem
> > I quickly ran into with the prototype is that I wasn't able to find
> > a way to identify pointers to alloca/VLA storage.
> Your analysis matches my very quick read of the aliasing code.  It may
> be the case that the Steensgaard patent got in the way here.
>
> >
> > In the the points-to solution for the pointer being returned they
> > both have the vars_contains_escaped_heap flag set.  That seems like
> > an omission that shouldn't be hard to fix, but on its own, I don't
> > think it would be sufficient.
> RIght.  In theory the result of an alloca call shouldn't alias anything
> in the heap -- but there were some implementations of alloca that were
> built on top of malloc (ugh).  That flag may be catering to that case.
>
> But in the case of a __builtin_alloca that shouldn't apply.  Hmm.  That
> ultimately might be a bug.
>
> >
> > In the IL a VLA is represented as a pointer to an array, but when
> > returning a pointer into a VLA (at some offset so it's an SSA_NAME),
> > the pointer's point-to solution doesn't include the VLA pointer or
> > (AFAICS) make it possible to tell even that it is a VLA.  For example
> > here:
> >
> >   f (int n)
> >   {
> >     int * p;
> >     int[0:D.1912] * a.1;
> >     sizetype _1;
> >     void * saved_stack.3_3;
> >     sizetype _6;
> >
> >     <bb 2> [local count: 1073741824]:
> >     saved_stack.3_3 = __builtin_stack_save ();
> >     _1 = (sizetype) n_2(D);
> >     _6 = _1 * 4;
> >     a.1_8 = __builtin_alloca_with_align (_6, 32);
> >     p_9 = a.1_8 + _6;
> >     __builtin_stack_restore (saved_stack.3_3);
> >     return p_9;
> >   }
> >
> > p_9's solution's is:
> >
> >   p_9, points-to vars: { D.1925 } (escaped, escaped heap)
> >
> > I couldn't find out how to determine that D.1925 is a VLA (or even
> > what it is).  It's not among the function's local variables that
> > FOR_EACH_LOCAL_DECL iterates over.
> It's possible that decl was created internally as part of the alias
> oracle's analysis.

Yes.  Note that only the UID was reserved the fake decl doesn't
live on.

Note that for the testcase above the "local" alloca storage escapes
which means you run into a catch-22 here given points-to computes
a conservative correct solution and  you want to detect escaping
locals.  Usually detecting a pointer to local storage can be done
by using ptr_deref_may_alias_global_p but of course in this
case the storage was marked global by PTA itself (and our PTA
is not flow-sensitive and it doesn't distinguish an escape through
a return stmt from an escape through a call which is relevant
even for local storage).

Feature-wise the PTA solver is missing sth like a "filter"
we could put in front of return stmts that doesn't let
addresses of locals leak.  The simplest way of implementing
this might be to not include 'returns' in the constraints at all
(in non-IPA mode) and handle them by post-processing the
solver result.  That gets us some additional flow-sensitivity
and a way to filter locals.  Let me see if I can cook up this.

That may ultimatively also help the warning code where you
then can use ptr_deref_may_alias_global_p.

Sth like the attached - completely untested (the
is_global_var check is likely too simplistic...).  It does
the job on alloca for me.

p_5, points-to NULL, points-to vars: { D.1913 }
_6, points-to NULL, points-to vars: { D.1913 }

foo (int n)
{
  void * p;
  long unsigned int _1;
  void * _6;

  <bb 2> :
  _1 = (long unsigned int) n_2(D);
  p_5 = __builtin_alloca (_1);
  _6 = p_5;
  return p_5;


Richard.

> See make_heapvar in tree-ssa-structalias.c
> >
> > By replacing the VLA in the test case with a call to malloc I get
> > this for the returned p_7:
> >
> >   p_7, points-to NULL, points-to vars: { D.1916 } (escaped, escaped heap)
> >
> > Again, I see no way to quickly tell that this pointer points into
> > the block returned from malloc.
> If there's a way to make that determination it'd have to be on the
> variable since the pt_solution flag bits don't carry a storage class
> directly.
>
> You might try to get a handle on those decls and dump them to see if
> there's anything easily identifiable.  But it may be easier to dig into
> the code that creates them.  A real quick scan of the aliasing code also
> shows the "variable_info" structure.  It's private to the aliasing code,
> but might guide you at things to look at.
>
> Regardless, I don't see an immediate path forward  using the oracle to
> identify objects in the stack for your patch.  WHich is unfortunate.
>
>
>
>
> Jeff

On Mon, Jun 3, 2019 at 11:37 AM Richard Biener
<richard.guenther@gmail.com> wrote:
>
> On Fri, May 31, 2019 at 5:35 PM Jeff Law <law@redhat.com> wrote:>
> > On 5/30/19 4:56 PM, Martin Sebor wrote:
> > > On 5/30/19 10:15 AM, Jeff Law wrote:
> > >> On 5/30/19 9:34 AM, Martin Sebor wrote:
> > >>
> > >>>>> If the alias oracle can be used to give the same results without
> > >>>>> excessive false positives then I think it would be fine to make
> > >>>>> use of it.  Is that something you consider a prerequisite for
> > >>>>> this change or should I look into it as a followup?
> > >>>> I think we should explore it a bit before making a final decision.  It
> > >>>> may guide us for other work in this space (like detecting escaping
> > >>>> locals).   I think a dirty prototype to see if it's even in the right
> > >>>> ballpark would make sense.
> > >>>
> > >>> Okay, let me look into it.
> > >> Sounds good.  Again, go with a quick prototype to see if it's likely
> > >> feasible.  The tests you've added should dramatically help evaluating if
> > >> the oracle is up to the task.
> > >
> > > So to expand on what I said on the phone when we spoke: the problem
> > > I quickly ran into with the prototype is that I wasn't able to find
> > > a way to identify pointers to alloca/VLA storage.
> > Your analysis matches my very quick read of the aliasing code.  It may
> > be the case that the Steensgaard patent got in the way here.
> >
> > >
> > > In the the points-to solution for the pointer being returned they
> > > both have the vars_contains_escaped_heap flag set.  That seems like
> > > an omission that shouldn't be hard to fix, but on its own, I don't
> > > think it would be sufficient.
> > RIght.  In theory the result of an alloca call shouldn't alias anything
> > in the heap -- but there were some implementations of alloca that were
> > built on top of malloc (ugh).  That flag may be catering to that case.
> >
> > But in the case of a __builtin_alloca that shouldn't apply.  Hmm.  That
> > ultimately might be a bug.
> >
> > >
> > > In the IL a VLA is represented as a pointer to an array, but when
> > > returning a pointer into a VLA (at some offset so it's an SSA_NAME),
> > > the pointer's point-to solution doesn't include the VLA pointer or
> > > (AFAICS) make it possible to tell even that it is a VLA.  For example
> > > here:
> > >
> > >   f (int n)
> > >   {
> > >     int * p;
> > >     int[0:D.1912] * a.1;
> > >     sizetype _1;
> > >     void * saved_stack.3_3;
> > >     sizetype _6;
> > >
> > >     <bb 2> [local count: 1073741824]:
> > >     saved_stack.3_3 = __builtin_stack_save ();
> > >     _1 = (sizetype) n_2(D);
> > >     _6 = _1 * 4;
> > >     a.1_8 = __builtin_alloca_with_align (_6, 32);
> > >     p_9 = a.1_8 + _6;
> > >     __builtin_stack_restore (saved_stack.3_3);
> > >     return p_9;
> > >   }
> > >
> > > p_9's solution's is:
> > >
> > >   p_9, points-to vars: { D.1925 } (escaped, escaped heap)
> > >
> > > I couldn't find out how to determine that D.1925 is a VLA (or even
> > > what it is).  It's not among the function's local variables that
> > > FOR_EACH_LOCAL_DECL iterates over.
> > It's possible that decl was created internally as part of the alias
> > oracle's analysis.
>
> Yes.  Note that only the UID was reserved the fake decl doesn't
> live on.
>
> Note that for the testcase above the "local" alloca storage escapes
> which means you run into a catch-22 here given points-to computes
> a conservative correct solution and  you want to detect escaping
> locals.  Usually detecting a pointer to local storage can be done
> by using ptr_deref_may_alias_global_p but of course in this
> case the storage was marked global by PTA itself (and our PTA
> is not flow-sensitive and it doesn't distinguish an escape through
> a return stmt from an escape through a call which is relevant
> even for local storage).
>
> Feature-wise the PTA solver is missing sth like a "filter"
> we could put in front of return stmts that doesn't let
> addresses of locals leak.  The simplest way of implementing
> this might be to not include 'returns' in the constraints at all
> (in non-IPA mode) and handle them by post-processing the
> solver result.  That gets us some additional flow-sensitivity
> and a way to filter locals.  Let me see if I can cook up this.
>
> That may ultimatively also help the warning code where you
> then can use ptr_deref_may_alias_global_p.
>
> Sth like the attached - completely untested (the
> is_global_var check is likely too simplistic...).  It does
> the job on alloca for me.
>
> p_5, points-to NULL, points-to vars: { D.1913 }
> _6, points-to NULL, points-to vars: { D.1913 }
>
> foo (int n)
> {
>   void * p;
>   long unsigned int _1;
>   void * _6;
>
>   <bb 2> :
>   _1 = (long unsigned int) n_2(D);
>   p_5 = __builtin_alloca (_1);
>   _6 = p_5;
>   return p_5;

I am testing the following fixed and more complete patch (still
the actual return values we process optimally is restricted).
I'm not sure whether it will really help real-world testcases
since the lack of flow-sensitivity in general and the lack of
distinguishing between escapes via calls vs. escapes via
return pessimizes things (escapes to global memory complicates
things there).  Also in IPA mode we cannot post-process
returns like in the hack^Wpatch, a "filter" op would need to be
added, complicating the solver.

Bootstrap / regtest running on x86_64-unknown-linux-gnu.

Richard.

2019-06-03  Richard Biener  <rguenther@suse.de>

        * tree-ssa-structalias.c: Include tree-cfg.h.
        (make_heapvar): Do not make heap vars artificial.
        (find_func_aliases_for_builtin_call): Handle stack allocation
        functions.
        (find_func_aliases): Delay processing of simple enough returns
        in non-IPA mode.
        (set_uids_in_ptset): Adjust.
        (find_what_var_points_to): Likewise.
        (compute_points_to_sets): Post-process return statements,
        amending the escaped solution.

        * gcc.dg/tree-ssa/alias-37.c: New testcase.

>
> Richard.
>
> > See make_heapvar in tree-ssa-structalias.c
> > >
> > > By replacing the VLA in the test case with a call to malloc I get
> > > this for the returned p_7:
> > >
> > >   p_7, points-to NULL, points-to vars: { D.1916 } (escaped, escaped heap)
> > >
> > > Again, I see no way to quickly tell that this pointer points into
> > > the block returned from malloc.
> > If there's a way to make that determination it'd have to be on the
> > variable since the pt_solution flag bits don't carry a storage class
> > directly.
> >
> > You might try to get a handle on those decls and dump them to see if
> > there's anything easily identifiable.  But it may be easier to dig into
> > the code that creates them.  A real quick scan of the aliasing code also
> > shows the "variable_info" structure.  It's private to the aliasing code,
> > but might guide you at things to look at.
> >
> > Regardless, I don't see an immediate path forward  using the oracle to
> > identify objects in the stack for your patch.  WHich is unfortunate.
> >
> >
> >
> >
> > Jeff

On 6/3/19 3:37 AM, Richard Biener wrote:
> On Fri, May 31, 2019 at 5:35 PM Jeff Law <law@redhat.com> wrote:>
>> On 5/30/19 4:56 PM, Martin Sebor wrote:
>>> On 5/30/19 10:15 AM, Jeff Law wrote:
>>>> On 5/30/19 9:34 AM, Martin Sebor wrote:
>>>>
>>>>>>> If the alias oracle can be used to give the same results without
>>>>>>> excessive false positives then I think it would be fine to make
>>>>>>> use of it.  Is that something you consider a prerequisite for
>>>>>>> this change or should I look into it as a followup?
>>>>>> I think we should explore it a bit before making a final decision.  It
>>>>>> may guide us for other work in this space (like detecting escaping
>>>>>> locals).   I think a dirty prototype to see if it's even in the right
>>>>>> ballpark would make sense.
>>>>>
>>>>> Okay, let me look into it.
>>>> Sounds good.  Again, go with a quick prototype to see if it's likely
>>>> feasible.  The tests you've added should dramatically help evaluating if
>>>> the oracle is up to the task.
>>>
>>> So to expand on what I said on the phone when we spoke: the problem
>>> I quickly ran into with the prototype is that I wasn't able to find
>>> a way to identify pointers to alloca/VLA storage.
>> Your analysis matches my very quick read of the aliasing code.  It may
>> be the case that the Steensgaard patent got in the way here.
>>
>>>
>>> In the the points-to solution for the pointer being returned they
>>> both have the vars_contains_escaped_heap flag set.  That seems like
>>> an omission that shouldn't be hard to fix, but on its own, I don't
>>> think it would be sufficient.
>> RIght.  In theory the result of an alloca call shouldn't alias anything
>> in the heap -- but there were some implementations of alloca that were
>> built on top of malloc (ugh).  That flag may be catering to that case.
>>
>> But in the case of a __builtin_alloca that shouldn't apply.  Hmm.  That
>> ultimately might be a bug.
>>
>>>
>>> In the IL a VLA is represented as a pointer to an array, but when
>>> returning a pointer into a VLA (at some offset so it's an SSA_NAME),
>>> the pointer's point-to solution doesn't include the VLA pointer or
>>> (AFAICS) make it possible to tell even that it is a VLA.  For example
>>> here:
>>>
>>>   f (int n)
>>>   {
>>>     int * p;
>>>     int[0:D.1912] * a.1;
>>>     sizetype _1;
>>>     void * saved_stack.3_3;
>>>     sizetype _6;
>>>
>>>     <bb 2> [local count: 1073741824]:
>>>     saved_stack.3_3 = __builtin_stack_save ();
>>>     _1 = (sizetype) n_2(D);
>>>     _6 = _1 * 4;
>>>     a.1_8 = __builtin_alloca_with_align (_6, 32);
>>>     p_9 = a.1_8 + _6;
>>>     __builtin_stack_restore (saved_stack.3_3);
>>>     return p_9;
>>>   }
>>>
>>> p_9's solution's is:
>>>
>>>   p_9, points-to vars: { D.1925 } (escaped, escaped heap)
>>>
>>> I couldn't find out how to determine that D.1925 is a VLA (or even
>>> what it is).  It's not among the function's local variables that
>>> FOR_EACH_LOCAL_DECL iterates over.
>> It's possible that decl was created internally as part of the alias
>> oracle's analysis.
> 
> Yes.  Note that only the UID was reserved the fake decl doesn't
> live on.
> 
> Note that for the testcase above the "local" alloca storage escapes
> which means you run into a catch-22 here given points-to computes
> a conservative correct solution and  you want to detect escaping
> locals.  Usually detecting a pointer to local storage can be done
> by using ptr_deref_may_alias_global_p but of course in this
> case the storage was marked global by PTA itself (and our PTA
> is not flow-sensitive and it doesn't distinguish an escape through
> a return stmt from an escape through a call which is relevant
> even for local storage).
Good point.  The inability to tell why something escaped is another
significant hurdle.

> 
> Feature-wise the PTA solver is missing sth like a "filter"
> we could put in front of return stmts that doesn't let
> addresses of locals leak.  The simplest way of implementing
> this might be to not include 'returns' in the constraints at all
> (in non-IPA mode) and handle them by post-processing the
> solver result.  That gets us some additional flow-sensitivity
> and a way to filter locals.  Let me see if I can cook up this.
Another thought would be to somehow flag how the pointer escaped.  ie,
was it as an argument, return value or stored to memory?  Though in the
end that level of detail may not be useful, so collecting it may not be
worth the effort.

> 
> That may ultimatively also help the warning code where you
> then can use ptr_deref_may_alias_global_p.
Another thought would be to use the alias oracle to prune what we look
at.  If we ask the oracle and the value doesn't escape, then it's not
worth walking up the use-def chain to see where it came from.


Jeff

On 5/31/19 2:46 PM, Jeff Law wrote:
> On 5/22/19 3:34 PM, Martin Sebor wrote:
>> -Wreturn-local-addr detects a subset of instances of returning
>> the address of a local object from a function but the warning
>> doesn't try to handle alloca or VLAs, or some non-trivial cases
>> of ordinary automatic variables[1].
>>
>> The attached patch extends the implementation of the warning to
>> detect those.  It still doesn't detect instances where the address
>> is the result of a built-in such strcpy[2].
>>
>> Tested on x86_64-linux.
>>
>> Martin
>>
>> [1] For example, this is only diagnosed with the patch:
>>
>>    void* f (int i)
>>    {
>>      struct S { int a[2]; } s[2];
>>      return &s->a[i];
>>    }
>>
>> [2] The following is not diagnosed even with the patch:
>>
>>    void sink (void*);
>>
>>    void* f (int i)
>>    {
>>      char a[6];
>>      char *p = __builtin_strcpy (a, "123");
>>      sink (p);
>>      return p;
>>    }
>>
>> I would expect detecting to be possible and useful.  Maybe as
>> a follow-up.
>>
>> gcc-71924.diff
>>
>> PR middle-end/71924 - missing -Wreturn-local-addr returning alloca result
>> PR middle-end/90549 - missing -Wreturn-local-addr maybe returning an address of a local array plus offset
>>
>> gcc/ChangeLog:
>>
>> 	PR c/71924
>> 	* gimple-ssa-isolate-paths.c (is_addr_local): New function.
>> 	(warn_return_addr_local_phi_arg, warn_return_addr_local): Same.
>> 	(find_implicit_erroneous_behavior): Call warn_return_addr_local_phi_arg.
>> 	(find_explicit_erroneous_behavior): Call warn_return_addr_local.
>>
>> gcc/testsuite/ChangeLog:
>>
>> 	PR c/71924
>> 	* gcc.dg/Wreturn-local-addr-2.c: New test.
>> 	* gcc.dg/Walloca-4.c: Prune expected warnings.
>> 	* gcc.dg/pr41551.c: Same.
>> 	* gcc.dg/pr59523.c: Same.
>> 	* gcc.dg/tree-ssa/pr88775-2.c: Same.
>> 	* gcc.dg/winline-7.c: Same.
>>
>> diff --git a/gcc/gimple-ssa-isolate-paths.c b/gcc/gimple-ssa-isolate-paths.c
>> index 33fe352bb23..2933ecf502e 100644
>> --- a/gcc/gimple-ssa-isolate-paths.c
>> +++ b/gcc/gimple-ssa-isolate-paths.c
>> @@ -341,6 +341,135 @@ stmt_uses_0_or_null_in_undefined_way (gimple *stmt)
>>     return false;
>>   }
>>   
>> +/* Return true if EXPR is a expression of pointer type that refers
>> +   to the address of a variable with automatic storage duration.
>> +   If so, set *PLOC to the location of the object or the call that
>> +   allocated it (for alloca and VLAs).  When PMAYBE is non-null,
>> +   also consider PHI statements and set *PMAYBE when some but not
>> +   all arguments of such statements refer to local variables, and
>> +   to clear it otherwise.  */
>> +
>> +static bool
>> +is_addr_local (tree exp, location_t *ploc, bool *pmaybe = NULL,
>> +	       hash_set<gphi *> *visited = NULL)
>> +{
>> +  if (TREE_CODE (exp) == SSA_NAME)
>> +    {
>> +      gimple *def_stmt = SSA_NAME_DEF_STMT (exp);
>> +      enum gimple_code code = gimple_code (def_stmt);
>> +
>> +      if (is_gimple_assign (def_stmt))
>> +	{
>> +	  tree type = TREE_TYPE (gimple_assign_lhs (def_stmt));
>> +	  if (POINTER_TYPE_P (type))
>> +	    {
>> +	      tree ptr = gimple_assign_rhs1 (def_stmt);
>> +	      return is_addr_local (ptr, ploc, pmaybe, visited);
>> +	    }
>> +	  return false;
>> +	}
> So this is going to recurse on the rhs1 of something like
> POINTER_PLUS_EXPR, that's a good thing :-)   But isn't it non-selective
> about the codes where we recurse?
> 
> Consider
> 
>    ptr = (cond) ? res1 : res2
> 
> I think we'll end up recursing on the condition rather than looking at
> res1 and res2.
> 
> 
> I suspect there are a very limited number of expression codes that
> appear on the RHS where we'd want to recurse on one or both operands.
> 
> POINTER_PLUS_EXPR, NOP_EXPR, maybe COND_EXPR (where you have to recurse
> on both and logically and the result), BIT_AND (maybe we masked off some
> bits in an address).  That's probably about it :-)
> 
> Are there any other codes you've seen or think would be useful in
> practice to recurse through?  I'd rather list them explicitly rather
> than just recurse down through every rhs1 we encounter.

I don't have a list of codes to test for.  I initially contemplated
enumerating them but in the end decided the pointer type check would
be sufficient.  I wouldn't expect a COND_EXPR here.  Don't they get
transformed into PHIs?  In all my tests they do and and running
the whole test suite with an assert that it doesn't come up doesn't
expose any either.  (I left the assert for COND_EXPR there.)  If
a COND_EXPR really can come up in a GIMPLE assignment here can you
please show me how so I can add a test for it?

I've added tests to exercise all C expressions that evaluate to
pointers.  I don't know of any others where what you bring up
should be a concern and I don't want to try to hardwire tests for
any that I can't to exercise in the testsuite or don't know how.
If you know of some I'm happy to add them and adjust the code.

>> +
>> +      if (code == GIMPLE_PHI && pmaybe)
>> +	{
>> +	  unsigned count = 0;
>> +	  gphi *phi_stmt = as_a <gphi *> (def_stmt);
>> +
>> +	  unsigned nargs = gimple_phi_num_args (phi_stmt);
>> +	  for (unsigned i = 0; i < nargs; ++i)
>> +	    {
>> +	      if (!visited->add (phi_stmt))
>> +		{
>> +		  tree arg = gimple_phi_arg_def (phi_stmt, i);
>> +		  if (is_addr_local (arg, ploc, pmaybe, visited))
>> +		    ++count;
>> +		}
>> +	    }
> So imagine
> 
> p = phi (x1, x1, x2)
> 
> Where both x1 and x2 would pass is_addr_local.  I think this code would
> incorrectly set pmaybe.

I suppose it would but this happens readily with or without my
patch, for example here:

   int* f (int i)
   {
     int a[2], b[2];
     int *p = i ? a : b;
     return p;
   }

We end up with two instances of "function may return address
of local variable," one for each local, because
find_implicit_erroneous_behavior only issues the "may return"
kind of warning.

I don't particularly like this -- that's why I added PMAYBE to
the new code.  But to avoid mission creep I'd decided not draw
the line there and avoid trying to fix the problem completely.
(I've enhanced this in the attached update.)

> 
> We process the first instance of x1, find it is local and bump count.
> When we encounter the second instance, it's in our map and we do
> nothing.  THen we process x2 and bump count again.  So count would be 2.
>   But count is going to be less than nargs so we'll set *pmaybe to true.
> 
> ISTM you'd have to record the result for each phi argument and query
> that to determine if you should bump the counter if you've already
> visited the argument.

I suspect that no solution will be perfect, at a minimum because
multiple return statements sometimes get merged into one, so what
in the source code is a single return that definitely returns
the address a local may end up merged with one that returns
a null (and triggering a maybe kind of warning).  I have also
seen warnings in some non-trivial test cases that suggest that
some return statements get split up into two where a "may return"
could turn into a "definitely does return."

> It also seems to me that you can break the loop as soon as you've got a
> nonzero count and get a false return from is_addr_local.   Not sure if
> that'll matter in practice or not.

This is only possible if we're willing to lose some detail (i.e.,
if we are willing to only point to one variable when returning
the address of two or more).  I don't find that very user-friendly.

To wrap up, while I would have preferred taking a simpler approach
at first and making bigger design changes later, I have redesigned
the warning for better accuracy as you suggested above.

The attached revised patch first collects the return statements
in a hash table along with the locations of the local variables
whose addresses each statement returns, and then issues just one
warning for each statement, listing all the locals it refers to
in subsequent notes.

In addition, since it was nearly trivial, I also added checking
for returning addresses of locals via calls to built-ins like
memcpy.

I have beefed up the test suite to exercise non-trivial functions
involving different kinds of expressions and statements, including
loops.  Except for one xfail due to missing location information
(bug 90735 - missing location in -Wreturn-local-addr on a function
with two return statements​) I haven't found any issues.
The improved warning does find many more problems than the current
solution.

> The other approach here (and I'm not suggesting you implement it) would
> be to use the propagation engine.  That's probably overkill here since
> we'd probably end up computing a whole lot of things we don't need.  My
> sense is an on-demand approach like you've done is probably less
> computationally expensive.

I'm certainly not opposed to making further improvements (as I
mentioned, I'd like to add checking for returning freed pointers
and freeing locals as you suggested).  But I would prefer to make
these in incremental steps rather than growing what was at first
meant to be just small bug fix/enhancement for alloca and VLAs.

The attached update has been tested on x86_64-linux.

Martin

On Mon, Jun 3, 2019 at 1:27 PM Richard Biener
<richard.guenther@gmail.com> wrote:
>
> On Mon, Jun 3, 2019 at 11:37 AM Richard Biener
> <richard.guenther@gmail.com> wrote:
> >
> > On Fri, May 31, 2019 at 5:35 PM Jeff Law <law@redhat.com> wrote:>
> > > On 5/30/19 4:56 PM, Martin Sebor wrote:
> > > > On 5/30/19 10:15 AM, Jeff Law wrote:
> > > >> On 5/30/19 9:34 AM, Martin Sebor wrote:
> > > >>
> > > >>>>> If the alias oracle can be used to give the same results without
> > > >>>>> excessive false positives then I think it would be fine to make
> > > >>>>> use of it.  Is that something you consider a prerequisite for
> > > >>>>> this change or should I look into it as a followup?
> > > >>>> I think we should explore it a bit before making a final decision.  It
> > > >>>> may guide us for other work in this space (like detecting escaping
> > > >>>> locals).   I think a dirty prototype to see if it's even in the right
> > > >>>> ballpark would make sense.
> > > >>>
> > > >>> Okay, let me look into it.
> > > >> Sounds good.  Again, go with a quick prototype to see if it's likely
> > > >> feasible.  The tests you've added should dramatically help evaluating if
> > > >> the oracle is up to the task.
> > > >
> > > > So to expand on what I said on the phone when we spoke: the problem
> > > > I quickly ran into with the prototype is that I wasn't able to find
> > > > a way to identify pointers to alloca/VLA storage.
> > > Your analysis matches my very quick read of the aliasing code.  It may
> > > be the case that the Steensgaard patent got in the way here.
> > >
> > > >
> > > > In the the points-to solution for the pointer being returned they
> > > > both have the vars_contains_escaped_heap flag set.  That seems like
> > > > an omission that shouldn't be hard to fix, but on its own, I don't
> > > > think it would be sufficient.
> > > RIght.  In theory the result of an alloca call shouldn't alias anything
> > > in the heap -- but there were some implementations of alloca that were
> > > built on top of malloc (ugh).  That flag may be catering to that case.
> > >
> > > But in the case of a __builtin_alloca that shouldn't apply.  Hmm.  That
> > > ultimately might be a bug.
> > >
> > > >
> > > > In the IL a VLA is represented as a pointer to an array, but when
> > > > returning a pointer into a VLA (at some offset so it's an SSA_NAME),
> > > > the pointer's point-to solution doesn't include the VLA pointer or
> > > > (AFAICS) make it possible to tell even that it is a VLA.  For example
> > > > here:
> > > >
> > > >   f (int n)
> > > >   {
> > > >     int * p;
> > > >     int[0:D.1912] * a.1;
> > > >     sizetype _1;
> > > >     void * saved_stack.3_3;
> > > >     sizetype _6;
> > > >
> > > >     <bb 2> [local count: 1073741824]:
> > > >     saved_stack.3_3 = __builtin_stack_save ();
> > > >     _1 = (sizetype) n_2(D);
> > > >     _6 = _1 * 4;
> > > >     a.1_8 = __builtin_alloca_with_align (_6, 32);
> > > >     p_9 = a.1_8 + _6;
> > > >     __builtin_stack_restore (saved_stack.3_3);
> > > >     return p_9;
> > > >   }
> > > >
> > > > p_9's solution's is:
> > > >
> > > >   p_9, points-to vars: { D.1925 } (escaped, escaped heap)
> > > >
> > > > I couldn't find out how to determine that D.1925 is a VLA (or even
> > > > what it is).  It's not among the function's local variables that
> > > > FOR_EACH_LOCAL_DECL iterates over.
> > > It's possible that decl was created internally as part of the alias
> > > oracle's analysis.
> >
> > Yes.  Note that only the UID was reserved the fake decl doesn't
> > live on.
> >
> > Note that for the testcase above the "local" alloca storage escapes
> > which means you run into a catch-22 here given points-to computes
> > a conservative correct solution and  you want to detect escaping
> > locals.  Usually detecting a pointer to local storage can be done
> > by using ptr_deref_may_alias_global_p but of course in this
> > case the storage was marked global by PTA itself (and our PTA
> > is not flow-sensitive and it doesn't distinguish an escape through
> > a return stmt from an escape through a call which is relevant
> > even for local storage).
> >
> > Feature-wise the PTA solver is missing sth like a "filter"
> > we could put in front of return stmts that doesn't let
> > addresses of locals leak.  The simplest way of implementing
> > this might be to not include 'returns' in the constraints at all
> > (in non-IPA mode) and handle them by post-processing the
> > solver result.  That gets us some additional flow-sensitivity
> > and a way to filter locals.  Let me see if I can cook up this.
> >
> > That may ultimatively also help the warning code where you
> > then can use ptr_deref_may_alias_global_p.
> >
> > Sth like the attached - completely untested (the
> > is_global_var check is likely too simplistic...).  It does
> > the job on alloca for me.
> >
> > p_5, points-to NULL, points-to vars: { D.1913 }
> > _6, points-to NULL, points-to vars: { D.1913 }
> >
> > foo (int n)
> > {
> >   void * p;
> >   long unsigned int _1;
> >   void * _6;
> >
> >   <bb 2> :
> >   _1 = (long unsigned int) n_2(D);
> >   p_5 = __builtin_alloca (_1);
> >   _6 = p_5;
> >   return p_5;
>
> I am testing the following fixed and more complete patch (still
> the actual return values we process optimally is restricted).
> I'm not sure whether it will really help real-world testcases
> since the lack of flow-sensitivity in general and the lack of
> distinguishing between escapes via calls vs. escapes via
> return pessimizes things (escapes to global memory complicates
> things there).  Also in IPA mode we cannot post-process
> returns like in the hack^Wpatch, a "filter" op would need to be
> added, complicating the solver.
>
> Bootstrap / regtest running on x86_64-unknown-linux-gnu.

Needs another iteration since it miscompiles gengtype and

struct S { int *mem; };

struct S * __attribute__((noinline,noipa))
foo ()
{
  struct S *s = __builtin_malloc (sizeof (struct S));
  s->mem = __builtin_malloc (sizeof (int));
  s->mem[0] = 1;
  return s;
}

int
main()
{
  struct S *s = foo();
  if (s->mem[0] != 1)
    __builtin_abort ();
  return 0;
}

Richard.

> Richard.
>
> 2019-06-03  Richard Biener  <rguenther@suse.de>
>
>         * tree-ssa-structalias.c: Include tree-cfg.h.
>         (make_heapvar): Do not make heap vars artificial.
>         (find_func_aliases_for_builtin_call): Handle stack allocation
>         functions.
>         (find_func_aliases): Delay processing of simple enough returns
>         in non-IPA mode.
>         (set_uids_in_ptset): Adjust.
>         (find_what_var_points_to): Likewise.
>         (compute_points_to_sets): Post-process return statements,
>         amending the escaped solution.
>
>         * gcc.dg/tree-ssa/alias-37.c: New testcase.
>
> >
> > Richard.
> >
> > > See make_heapvar in tree-ssa-structalias.c
> > > >
> > > > By replacing the VLA in the test case with a call to malloc I get
> > > > this for the returned p_7:
> > > >
> > > >   p_7, points-to NULL, points-to vars: { D.1916 } (escaped, escaped heap)
> > > >
> > > > Again, I see no way to quickly tell that this pointer points into
> > > > the block returned from malloc.
> > > If there's a way to make that determination it'd have to be on the
> > > variable since the pt_solution flag bits don't carry a storage class
> > > directly.
> > >
> > > You might try to get a handle on those decls and dump them to see if
> > > there's anything easily identifiable.  But it may be easier to dig into
> > > the code that creates them.  A real quick scan of the aliasing code also
> > > shows the "variable_info" structure.  It's private to the aliasing code,
> > > but might guide you at things to look at.
> > >
> > > Regardless, I don't see an immediate path forward  using the oracle to
> > > identify objects in the stack for your patch.  WHich is unfortunate.
> > >
> > >
> > >
> > >
> > > Jeff

On 6/3/19 5:24 PM, Martin Sebor wrote:
> On 5/31/19 2:46 PM, Jeff Law wrote:
>> On 5/22/19 3:34 PM, Martin Sebor wrote:
>>> -Wreturn-local-addr detects a subset of instances of returning
>>> the address of a local object from a function but the warning
>>> doesn't try to handle alloca or VLAs, or some non-trivial cases
>>> of ordinary automatic variables[1].
>>>
>>> The attached patch extends the implementation of the warning to
>>> detect those.  It still doesn't detect instances where the address
>>> is the result of a built-in such strcpy[2].
>>>
>>> Tested on x86_64-linux.
>>>
>>> Martin
>>>
>>> [1] For example, this is only diagnosed with the patch:
>>>
>>>    void* f (int i)
>>>    {
>>>      struct S { int a[2]; } s[2];
>>>      return &s->a[i];
>>>    }
>>>
>>> [2] The following is not diagnosed even with the patch:
>>>
>>>    void sink (void*);
>>>
>>>    void* f (int i)
>>>    {
>>>      char a[6];
>>>      char *p = __builtin_strcpy (a, "123");
>>>      sink (p);
>>>      return p;
>>>    }
>>>
>>> I would expect detecting to be possible and useful.  Maybe as
>>> a follow-up.
>>>
>>> gcc-71924.diff
>>>
>>> PR middle-end/71924 - missing -Wreturn-local-addr returning alloca 
>>> result
>>> PR middle-end/90549 - missing -Wreturn-local-addr maybe returning an 
>>> address of a local array plus offset
>>>
>>> gcc/ChangeLog:
>>>
>>>     PR c/71924
>>>     * gimple-ssa-isolate-paths.c (is_addr_local): New function.
>>>     (warn_return_addr_local_phi_arg, warn_return_addr_local): Same.
>>>     (find_implicit_erroneous_behavior): Call 
>>> warn_return_addr_local_phi_arg.
>>>     (find_explicit_erroneous_behavior): Call warn_return_addr_local.
>>>
>>> gcc/testsuite/ChangeLog:
>>>
>>>     PR c/71924
>>>     * gcc.dg/Wreturn-local-addr-2.c: New test.
>>>     * gcc.dg/Walloca-4.c: Prune expected warnings.
>>>     * gcc.dg/pr41551.c: Same.
>>>     * gcc.dg/pr59523.c: Same.
>>>     * gcc.dg/tree-ssa/pr88775-2.c: Same.
>>>     * gcc.dg/winline-7.c: Same.
>>>
>>> diff --git a/gcc/gimple-ssa-isolate-paths.c 
>>> b/gcc/gimple-ssa-isolate-paths.c
>>> index 33fe352bb23..2933ecf502e 100644
>>> --- a/gcc/gimple-ssa-isolate-paths.c
>>> +++ b/gcc/gimple-ssa-isolate-paths.c
>>> @@ -341,6 +341,135 @@ stmt_uses_0_or_null_in_undefined_way (gimple 
>>> *stmt)
>>>     return false;
>>>   }
>>> +/* Return true if EXPR is a expression of pointer type that refers
>>> +   to the address of a variable with automatic storage duration.
>>> +   If so, set *PLOC to the location of the object or the call that
>>> +   allocated it (for alloca and VLAs).  When PMAYBE is non-null,
>>> +   also consider PHI statements and set *PMAYBE when some but not
>>> +   all arguments of such statements refer to local variables, and
>>> +   to clear it otherwise.  */
>>> +
>>> +static bool
>>> +is_addr_local (tree exp, location_t *ploc, bool *pmaybe = NULL,
>>> +           hash_set<gphi *> *visited = NULL)
>>> +{
>>> +  if (TREE_CODE (exp) == SSA_NAME)
>>> +    {
>>> +      gimple *def_stmt = SSA_NAME_DEF_STMT (exp);
>>> +      enum gimple_code code = gimple_code (def_stmt);
>>> +
>>> +      if (is_gimple_assign (def_stmt))
>>> +    {
>>> +      tree type = TREE_TYPE (gimple_assign_lhs (def_stmt));
>>> +      if (POINTER_TYPE_P (type))
>>> +        {
>>> +          tree ptr = gimple_assign_rhs1 (def_stmt);
>>> +          return is_addr_local (ptr, ploc, pmaybe, visited);
>>> +        }
>>> +      return false;
>>> +    }
>> So this is going to recurse on the rhs1 of something like
>> POINTER_PLUS_EXPR, that's a good thing :-)   But isn't it non-selective
>> about the codes where we recurse?
>>
>> Consider
>>
>>    ptr = (cond) ? res1 : res2
>>
>> I think we'll end up recursing on the condition rather than looking at
>> res1 and res2.
>>
>>
>> I suspect there are a very limited number of expression codes that
>> appear on the RHS where we'd want to recurse on one or both operands.
>>
>> POINTER_PLUS_EXPR, NOP_EXPR, maybe COND_EXPR (where you have to recurse
>> on both and logically and the result), BIT_AND (maybe we masked off some
>> bits in an address).  That's probably about it :-)
>>
>> Are there any other codes you've seen or think would be useful in
>> practice to recurse through?  I'd rather list them explicitly rather
>> than just recurse down through every rhs1 we encounter.
> 
> I don't have a list of codes to test for.  I initially contemplated
> enumerating them but in the end decided the pointer type check would
> be sufficient.  I wouldn't expect a COND_EXPR here.  Don't they get
> transformed into PHIs?  In all my tests they do and and running
> the whole test suite with an assert that it doesn't come up doesn't
> expose any either.  (I left the assert for COND_EXPR there.)  If
> a COND_EXPR really can come up in a GIMPLE assignment here can you
> please show me how so I can add a test for it?
> 
> I've added tests to exercise all C expressions that evaluate to
> pointers.  I don't know of any others where what you bring up
> should be a concern and I don't want to try to hardwire tests for
> any that I can't to exercise in the testsuite or don't know how.
> If you know of some I'm happy to add them and adjust the code.
> 
>>> +
>>> +      if (code == GIMPLE_PHI && pmaybe)
>>> +    {
>>> +      unsigned count = 0;
>>> +      gphi *phi_stmt = as_a <gphi *> (def_stmt);
>>> +
>>> +      unsigned nargs = gimple_phi_num_args (phi_stmt);
>>> +      for (unsigned i = 0; i < nargs; ++i)
>>> +        {
>>> +          if (!visited->add (phi_stmt))
>>> +        {
>>> +          tree arg = gimple_phi_arg_def (phi_stmt, i);
>>> +          if (is_addr_local (arg, ploc, pmaybe, visited))
>>> +            ++count;
>>> +        }
>>> +        }
>> So imagine
>>
>> p = phi (x1, x1, x2)
>>
>> Where both x1 and x2 would pass is_addr_local.  I think this code would
>> incorrectly set pmaybe.
> 
> I suppose it would but this happens readily with or without my
> patch, for example here:
> 
>    int* f (int i)
>    {
>      int a[2], b[2];
>      int *p = i ? a : b;
>      return p;
>    }
> 
> We end up with two instances of "function may return address
> of local variable," one for each local, because
> find_implicit_erroneous_behavior only issues the "may return"
> kind of warning.
> 
> I don't particularly like this -- that's why I added PMAYBE to
> the new code.  But to avoid mission creep I'd decided not draw
> the line there and avoid trying to fix the problem completely.
> (I've enhanced this in the attached update.)
> 
>>
>> We process the first instance of x1, find it is local and bump count.
>> When we encounter the second instance, it's in our map and we do
>> nothing.  THen we process x2 and bump count again.  So count would be 2.
>>   But count is going to be less than nargs so we'll set *pmaybe to true.
>>
>> ISTM you'd have to record the result for each phi argument and query
>> that to determine if you should bump the counter if you've already
>> visited the argument.
> 
> I suspect that no solution will be perfect, at a minimum because
> multiple return statements sometimes get merged into one, so what
> in the source code is a single return that definitely returns
> the address a local may end up merged with one that returns
> a null (and triggering a maybe kind of warning).  I have also
> seen warnings in some non-trivial test cases that suggest that
> some return statements get split up into two where a "may return"
> could turn into a "definitely does return."
> 
>> It also seems to me that you can break the loop as soon as you've got a
>> nonzero count and get a false return from is_addr_local.   Not sure if
>> that'll matter in practice or not.
> 
> This is only possible if we're willing to lose some detail (i.e.,
> if we are willing to only point to one variable when returning
> the address of two or more).  I don't find that very user-friendly.
> 
> To wrap up, while I would have preferred taking a simpler approach
> at first and making bigger design changes later, I have redesigned
> the warning for better accuracy as you suggested above.
> 
> The attached revised patch first collects the return statements
> in a hash table along with the locations of the local variables
> whose addresses each statement returns, and then issues just one
> warning for each statement, listing all the locals it refers to
> in subsequent notes.
> 
> In addition, since it was nearly trivial, I also added checking
> for returning addresses of locals via calls to built-ins like
> memcpy.

I didn't fully retest the patch after a minor tweak and sure enough,
the tweak was wrong.  Please apply the following on top of the patch.
(We only want to consider the argument of the call.  The LHS is
the call itself.)

diff --git a/gcc/gimple-ssa-isolate-paths.c b/gcc/gimple-ssa-isolate-paths.c
index eb8e754870f..bb9616f26ec 100644
--- a/gcc/gimple-ssa-isolate-paths.c
+++ b/gcc/gimple-ssa-isolate-paths.c
@@ -475,14 +475,9 @@ is_addr_local (gimple *return_stmt, tree exp, 
location_t *ploc,
             case BUILT_IN_STRNCAT_CHK:
             case BUILT_IN_STRNCPY:
             case BUILT_IN_STRNCPY_CHK:
-             /* Check both the argument and the return value.  */
-             return (is_addr_local (return_stmt,
-                                    gimple_call_arg (def_stmt, 0),
-                                    ploc, plocmap, visited)
-                     || is_addr_local (return_stmt,
-                                       gimple_call_lhs (def_stmt),
-                                       ploc, plocmap, visited));
-
+             return is_addr_local (return_stmt,
+                                   gimple_call_arg (def_stmt, 0),
+                                   ploc, plocmap, visited);
             default:
               return false;
             }
> 
> I have beefed up the test suite to exercise non-trivial functions
> involving different kinds of expressions and statements, including
> loops.  Except for one xfail due to missing location information
> (bug 90735 - missing location in -Wreturn-local-addr on a function
> with two return statements​) I haven't found any issues.
> The improved warning does find many more problems than the current
> solution.
> 
>> The other approach here (and I'm not suggesting you implement it) would
>> be to use the propagation engine.  That's probably overkill here since
>> we'd probably end up computing a whole lot of things we don't need.  My
>> sense is an on-demand approach like you've done is probably less
>> computationally expensive.
> 
> I'm certainly not opposed to making further improvements (as I
> mentioned, I'd like to add checking for returning freed pointers
> and freeing locals as you suggested).  But I would prefer to make
> these in incremental steps rather than growing what was at first
> meant to be just small bug fix/enhancement for alloca and VLAs.
> 
> The attached update has been tested on x86_64-linux.
> 
> Martin

On 6/14/19 2:59 PM, Jeff Law wrote:
> On 6/4/19 1:40 PM, Martin Sebor wrote:
>> On 6/3/19 5:24 PM, Martin Sebor wrote:
>>> On 5/31/19 2:46 PM, Jeff Law wrote:
>>>> On 5/22/19 3:34 PM, Martin Sebor wrote:
>>>>> -Wreturn-local-addr detects a subset of instances of returning
>>>>> the address of a local object from a function but the warning
>>>>> doesn't try to handle alloca or VLAs, or some non-trivial cases
>>>>> of ordinary automatic variables[1].
>>>>>
>>>>> The attached patch extends the implementation of the warning to
>>>>> detect those.  It still doesn't detect instances where the address
>>>>> is the result of a built-in such strcpy[2].
>>>>>
>>>>> Tested on x86_64-linux.
>>>>>
>>>>> Martin
>>>>>
>>>>> [1] For example, this is only diagnosed with the patch:
>>>>>
>>>>>     void* f (int i)
>>>>>     {
>>>>>       struct S { int a[2]; } s[2];
>>>>>       return &s->a[i];
>>>>>     }
>>>>>
>>>>> [2] The following is not diagnosed even with the patch:
>>>>>
>>>>>     void sink (void*);
>>>>>
>>>>>     void* f (int i)
>>>>>     {
>>>>>       char a[6];
>>>>>       char *p = __builtin_strcpy (a, "123");
>>>>>       sink (p);
>>>>>       return p;
>>>>>     }
>>>>>
>>>>> I would expect detecting to be possible and useful.  Maybe as
>>>>> a follow-up.
>>>>>
>>>>> gcc-71924.diff
>>>>>
>>>>> PR middle-end/71924 - missing -Wreturn-local-addr returning alloca
>>>>> result
>>>>> PR middle-end/90549 - missing -Wreturn-local-addr maybe returning an
>>>>> address of a local array plus offset
>>>>>
>>>>> gcc/ChangeLog:
>>>>>
>>>>>      PR c/71924
>>>>>      * gimple-ssa-isolate-paths.c (is_addr_local): New function.
>>>>>      (warn_return_addr_local_phi_arg, warn_return_addr_local): Same.
>>>>>      (find_implicit_erroneous_behavior): Call
>>>>> warn_return_addr_local_phi_arg.
>>>>>      (find_explicit_erroneous_behavior): Call warn_return_addr_local.
>>>>>
>>>>> gcc/testsuite/ChangeLog:
>>>>>
>>>>>      PR c/71924
>>>>>      * gcc.dg/Wreturn-local-addr-2.c: New test.
>>>>>      * gcc.dg/Walloca-4.c: Prune expected warnings.
>>>>>      * gcc.dg/pr41551.c: Same.
>>>>>      * gcc.dg/pr59523.c: Same.
>>>>>      * gcc.dg/tree-ssa/pr88775-2.c: Same.
>>>>>      * gcc.dg/winline-7.c: Same.
>>>>>
>>>>> diff --git a/gcc/gimple-ssa-isolate-paths.c
>>>>> b/gcc/gimple-ssa-isolate-paths.c
>>>>> index 33fe352bb23..2933ecf502e 100644
>>>>> --- a/gcc/gimple-ssa-isolate-paths.c
>>>>> +++ b/gcc/gimple-ssa-isolate-paths.c
>>>>> @@ -341,6 +341,135 @@ stmt_uses_0_or_null_in_undefined_way (gimple
>>>>> *stmt)
>>>>>      return false;
>>>>>    }
>>>>> +/* Return true if EXPR is a expression of pointer type that refers
>>>>> +   to the address of a variable with automatic storage duration.
>>>>> +   If so, set *PLOC to the location of the object or the call that
>>>>> +   allocated it (for alloca and VLAs).  When PMAYBE is non-null,
>>>>> +   also consider PHI statements and set *PMAYBE when some but not
>>>>> +   all arguments of such statements refer to local variables, and
>>>>> +   to clear it otherwise.  */
>>>>> +
>>>>> +static bool
>>>>> +is_addr_local (tree exp, location_t *ploc, bool *pmaybe = NULL,
>>>>> +           hash_set<gphi *> *visited = NULL)
>>>>> +{
>>>>> +  if (TREE_CODE (exp) == SSA_NAME)
>>>>> +    {
>>>>> +      gimple *def_stmt = SSA_NAME_DEF_STMT (exp);
>>>>> +      enum gimple_code code = gimple_code (def_stmt);
>>>>> +
>>>>> +      if (is_gimple_assign (def_stmt))
>>>>> +    {
>>>>> +      tree type = TREE_TYPE (gimple_assign_lhs (def_stmt));
>>>>> +      if (POINTER_TYPE_P (type))
>>>>> +        {
>>>>> +          tree ptr = gimple_assign_rhs1 (def_stmt);
>>>>> +          return is_addr_local (ptr, ploc, pmaybe, visited);
>>>>> +        }
>>>>> +      return false;
>>>>> +    }
>>>> So this is going to recurse on the rhs1 of something like
>>>> POINTER_PLUS_EXPR, that's a good thing :-)   But isn't it non-selective
>>>> about the codes where we recurse?
>>>>
>>>> Consider
>>>>
>>>>     ptr = (cond) ? res1 : res2
>>>>
>>>> I think we'll end up recursing on the condition rather than looking at
>>>> res1 and res2.
>>>>
>>>>
>>>> I suspect there are a very limited number of expression codes that
>>>> appear on the RHS where we'd want to recurse on one or both operands.
>>>>
>>>> POINTER_PLUS_EXPR, NOP_EXPR, maybe COND_EXPR (where you have to recurse
>>>> on both and logically and the result), BIT_AND (maybe we masked off some
>>>> bits in an address).  That's probably about it :-)
>>>>
>>>> Are there any other codes you've seen or think would be useful in
>>>> practice to recurse through?  I'd rather list them explicitly rather
>>>> than just recurse down through every rhs1 we encounter.
>>>
>>> I don't have a list of codes to test for.  I initially contemplated
>>> enumerating them but in the end decided the pointer type check would
>>> be sufficient.  I wouldn't expect a COND_EXPR here.  Don't they get
>>> transformed into PHIs?  In all my tests they do and and running
>>> the whole test suite with an assert that it doesn't come up doesn't
>>> expose any either.  (I left the assert for COND_EXPR there.)  If
>>> a COND_EXPR really can come up in a GIMPLE assignment here can you
>>> please show me how so I can add a test for it?
> A COND_EXPR on the RHS of an assignment is valid gimple.  That's what we
> need to consider here -- what is and what is not valid gimple.  And its
> more likely that PHIs will be transformed into RHS COND_EXPRs -- that's
> standard practice for if-conversion.
> 
> Gosh, how to get one?  It happens all the time :-)  Since I know DOM so
> well, I just shoved a quick assert into optimize_stmt to abort if we
> encounter a gimple assignment where the RHS is a COND_EXPR.  It blew up
> instantly building libgcc :-)
> 
> COmpile the attached code with -O2 -m32.

I wasn't saying it's not valid Gimple, just that I hadn't seen it
come up here despite compiling Glibc and the kernel with the patched
GCC.  The only codes I saw are these:

   addr_expr, array_ref, bit_and_expr, component_ref, max_expr,
   mem_ref, nop_expr, parm_decl, pointer_plus_expr, ssa_name,
   and var_decl

What I was asking for is a test case that makes COND_EXPR come up
in this context.  But I managed to find one by triggering the ICE
with GDB.  The file reduced to the following test case:

   extern struct S s;   // S has to be an incomplete type

   void* f (int n)
   {
     void *p;
     int x = 0;

     for (int i = n; i >= 0; i--)
       {
         p = &s;
         if (p == (void*)-1)
            x = 1;
         else if (p)
            return p;
       }

     return x ? (void*)-1 : 0;
   }

and the dump:

   <bb 6> [local count: 59055800]:
   # x_10 = PHI <1(5), 0(2)>
   _5 = x_10 != 0 ? -1B : 0B;

   <bb 7> [local count: 114863532]:
   # _3 = PHI <&s(4), _5(6), &s(3)>
   return _3;

It seems a little odd that the COND_EXPR disappears when either
of the constant addresses becomes the address of an object (or
the result of alloca), and also when the number of iterations
of the loop is constant.  That's probably why it so rarely comes
up in this context.

That said, even though I've seen a few references to COND_EXPR
in the midle-end I have been assuming that they, in general, do
get transformed into PHIs.  But checking the internals manual
I found in Section 12.6.3 this:

   A C ?: expression is converted into an if statement with each
   branch assigning to the same temporary. ... The GIMPLE level
   if-conversion pass re-introduces ?: expression, if appropriate.
   It is used to vectorize loops with conditions using vector
   conditional operations.

This GDB test case is likely the result of this reintroduction.

> You'll see them for stuff like this:
> 
> 
> ;;   basic block 24, loop depth 0
> ;;    pred:       23
>    _244 = _1 == 0 ? 1 : 2;
> 
> In a few spots.
> 
> 
> In addition to COND_EXPR, I'd be worried about VEC_COND_EXPR,
> VEC_PERM_EXPR and a host of others.
> 
> And in a more general sense, this kind of permissiveness is not future
> proof.  What happens if someone needs to add another EXPR node that is
> valid on the RHS where such recursion is undesirable?  How are they
> supposed to know that we've got this permissive recursive call and that
> it's going to do the wrong thing?  And if it's an EXPR node with no
> arguments, then we're going to do a read out of the bounds of the object
> and all bets are off at that point (yes we have zero operand EXPR nodes,
> but thankfully I don't think they should up in the contexts you care about).

Sure.  When things are hardcoded this way the same argument can
be made both ways.  If we just handle A and B and then someone
adds an X that matches one of the two, it won't be handled. Either
way, some work is necessary to deal with the new Z.  One way to
avoid it would be by providing an API to query this property of
a code (e.g., a function like int gimple_result_aliases_operand
(gimple *stmt, int opno)).

> I'd be much more comfortable with a tight predicate controlling recursion.

I've added ADDR_EXPR, COND_EXPR, MAX_EXPR, MIN_EXPR, NOP_EXPR, and
POINTER_PLUS_EXPR based on the survey I did.  I don't have tests for
the rest so I've left them out for now.  If/when I come up with them
I'll add them.

>>>>> +
>>>>> +      if (code == GIMPLE_PHI && pmaybe)
>>>>> +    {
>>>>> +      unsigned count = 0;
>>>>> +      gphi *phi_stmt = as_a <gphi *> (def_stmt);
>>>>> +
>>>>> +      unsigned nargs = gimple_phi_num_args (phi_stmt);
>>>>> +      for (unsigned i = 0; i < nargs; ++i)
>>>>> +        {
>>>>> +          if (!visited->add (phi_stmt))
>>>>> +        {
>>>>> +          tree arg = gimple_phi_arg_def (phi_stmt, i);
>>>>> +          if (is_addr_local (arg, ploc, pmaybe, visited))
>>>>> +            ++count;
>>>>> +        }
>>>>> +        }
>>>> So imagine
>>>>
>>>> p = phi (x1, x1, x2)
>>>>
>>>> Where both x1 and x2 would pass is_addr_local.  I think this code would
>>>> incorrectly set pmaybe.
>>>
>>> I suppose it would but this happens readily with or without my
>>> patch, for example here:
> But those seem like distinct issues.  The one I pointed out looks like a
> pretty simple case to handle.  It's just a logic error.

It wasn't a logic error.  I simply didn't think it was necessary
to worry about the accuracy of an inherently inaccurate warning,
and I didn't want make more intrusive changes than was called for
by my simple fix/enhancement.  I was also keeping in mind your
preference for smaller change sets.  But since you insist I went
ahead and improved things.  The warning is all the better for it,
and the changes I made exposed a number of other bugs in GCC.
At the same time, it seems that the bar for a simple bug fixes
and small enhancements should not be in excess of what's possible
by the original design.

Anyway, attached is an updated revision with the recursion limited
to just the codes you mentioned, and hopefully with an acceptable
accuracy.  The patch depends on a fix for the hash_map bug 90923
that I just posted:
   https://gcc.gnu.org/ml/gcc-patches/2019-06/msg01105.html

Martin

> 
>>>
>>>     int* f (int i)
>>>     {
>>>       int a[2], b[2];
>>>       int *p = i ? a : b;
>>>       return p;
>>>     }
>>>
>>> We end up with two instances of "function may return address
>>> of local variable," one for each local, because
>>> find_implicit_erroneous_behavior only issues the "may return"
>>> kind of warning.
> This looks more like a failing of the core approach in the erroneous
> path code.
> 
>>>
>>> I suspect that no solution will be perfect, at a minimum because
>>> multiple return statements sometimes get merged into one, so what
>>> in the source code is a single return that definitely returns
>>> the address a local may end up merged with one that returns
>>> a null (and triggering a maybe kind of warning).  I have also
>>> seen warnings in some non-trivial test cases that suggest that
>>> some return statements get split up into two where a "may return"
>>> could turn into a "definitely does return."
> Certainly.  I don't expect any solution will be perfect here, but I
> think fixing the logic error in the noted loop would help
> 
>>>
>>>> It also seems to me that you can break the loop as soon as you've got a
>>>> nonzero count and get a false return from is_addr_local.   Not sure if
>>>> that'll matter in practice or not.
>>>
>>> This is only possible if we're willing to lose some detail (i.e.,
>>> if we are willing to only point to one variable when returning
>>> the address of two or more).  I don't find that very user-friendly.
> ACK.  Ignore that comment then.
> 
> 
> Jeff
>

Ping: did my reply and updated patch resolve your concerns?
   https://gcc.gnu.org/ml/gcc-patches/2019-06/msg01106.html

On 6/18/19 9:19 PM, Martin Sebor wrote:
> On 6/14/19 2:59 PM, Jeff Law wrote:
>> On 6/4/19 1:40 PM, Martin Sebor wrote:
>>> On 6/3/19 5:24 PM, Martin Sebor wrote:
>>>> On 5/31/19 2:46 PM, Jeff Law wrote:
>>>>> On 5/22/19 3:34 PM, Martin Sebor wrote:
>>>>>> -Wreturn-local-addr detects a subset of instances of returning
>>>>>> the address of a local object from a function but the warning
>>>>>> doesn't try to handle alloca or VLAs, or some non-trivial cases
>>>>>> of ordinary automatic variables[1].
>>>>>>
>>>>>> The attached patch extends the implementation of the warning to
>>>>>> detect those.  It still doesn't detect instances where the address
>>>>>> is the result of a built-in such strcpy[2].
>>>>>>
>>>>>> Tested on x86_64-linux.
>>>>>>
>>>>>> Martin
>>>>>>
>>>>>> [1] For example, this is only diagnosed with the patch:
>>>>>>
>>>>>>     void* f (int i)
>>>>>>     {
>>>>>>       struct S { int a[2]; } s[2];
>>>>>>       return &s->a[i];
>>>>>>     }
>>>>>>
>>>>>> [2] The following is not diagnosed even with the patch:
>>>>>>
>>>>>>     void sink (void*);
>>>>>>
>>>>>>     void* f (int i)
>>>>>>     {
>>>>>>       char a[6];
>>>>>>       char *p = __builtin_strcpy (a, "123");
>>>>>>       sink (p);
>>>>>>       return p;
>>>>>>     }
>>>>>>
>>>>>> I would expect detecting to be possible and useful.  Maybe as
>>>>>> a follow-up.
>>>>>>
>>>>>> gcc-71924.diff
>>>>>>
>>>>>> PR middle-end/71924 - missing -Wreturn-local-addr returning alloca
>>>>>> result
>>>>>> PR middle-end/90549 - missing -Wreturn-local-addr maybe returning an
>>>>>> address of a local array plus offset
>>>>>>
>>>>>> gcc/ChangeLog:
>>>>>>
>>>>>>      PR c/71924
>>>>>>      * gimple-ssa-isolate-paths.c (is_addr_local): New function.
>>>>>>      (warn_return_addr_local_phi_arg, warn_return_addr_local): Same.
>>>>>>      (find_implicit_erroneous_behavior): Call
>>>>>> warn_return_addr_local_phi_arg.
>>>>>>      (find_explicit_erroneous_behavior): Call warn_return_addr_local.
>>>>>>
>>>>>> gcc/testsuite/ChangeLog:
>>>>>>
>>>>>>      PR c/71924
>>>>>>      * gcc.dg/Wreturn-local-addr-2.c: New test.
>>>>>>      * gcc.dg/Walloca-4.c: Prune expected warnings.
>>>>>>      * gcc.dg/pr41551.c: Same.
>>>>>>      * gcc.dg/pr59523.c: Same.
>>>>>>      * gcc.dg/tree-ssa/pr88775-2.c: Same.
>>>>>>      * gcc.dg/winline-7.c: Same.
>>>>>>
>>>>>> diff --git a/gcc/gimple-ssa-isolate-paths.c
>>>>>> b/gcc/gimple-ssa-isolate-paths.c
>>>>>> index 33fe352bb23..2933ecf502e 100644
>>>>>> --- a/gcc/gimple-ssa-isolate-paths.c
>>>>>> +++ b/gcc/gimple-ssa-isolate-paths.c
>>>>>> @@ -341,6 +341,135 @@ stmt_uses_0_or_null_in_undefined_way (gimple
>>>>>> *stmt)
>>>>>>      return false;
>>>>>>    }
>>>>>> +/* Return true if EXPR is a expression of pointer type that refers
>>>>>> +   to the address of a variable with automatic storage duration.
>>>>>> +   If so, set *PLOC to the location of the object or the call that
>>>>>> +   allocated it (for alloca and VLAs).  When PMAYBE is non-null,
>>>>>> +   also consider PHI statements and set *PMAYBE when some but not
>>>>>> +   all arguments of such statements refer to local variables, and
>>>>>> +   to clear it otherwise.  */
>>>>>> +
>>>>>> +static bool
>>>>>> +is_addr_local (tree exp, location_t *ploc, bool *pmaybe = NULL,
>>>>>> +           hash_set<gphi *> *visited = NULL)
>>>>>> +{
>>>>>> +  if (TREE_CODE (exp) == SSA_NAME)
>>>>>> +    {
>>>>>> +      gimple *def_stmt = SSA_NAME_DEF_STMT (exp);
>>>>>> +      enum gimple_code code = gimple_code (def_stmt);
>>>>>> +
>>>>>> +      if (is_gimple_assign (def_stmt))
>>>>>> +    {
>>>>>> +      tree type = TREE_TYPE (gimple_assign_lhs (def_stmt));
>>>>>> +      if (POINTER_TYPE_P (type))
>>>>>> +        {
>>>>>> +          tree ptr = gimple_assign_rhs1 (def_stmt);
>>>>>> +          return is_addr_local (ptr, ploc, pmaybe, visited);
>>>>>> +        }
>>>>>> +      return false;
>>>>>> +    }
>>>>> So this is going to recurse on the rhs1 of something like
>>>>> POINTER_PLUS_EXPR, that's a good thing :-)   But isn't it 
>>>>> non-selective
>>>>> about the codes where we recurse?
>>>>>
>>>>> Consider
>>>>>
>>>>>     ptr = (cond) ? res1 : res2
>>>>>
>>>>> I think we'll end up recursing on the condition rather than looking at
>>>>> res1 and res2.
>>>>>
>>>>>
>>>>> I suspect there are a very limited number of expression codes that
>>>>> appear on the RHS where we'd want to recurse on one or both operands.
>>>>>
>>>>> POINTER_PLUS_EXPR, NOP_EXPR, maybe COND_EXPR (where you have to 
>>>>> recurse
>>>>> on both and logically and the result), BIT_AND (maybe we masked off 
>>>>> some
>>>>> bits in an address).  That's probably about it :-)
>>>>>
>>>>> Are there any other codes you've seen or think would be useful in
>>>>> practice to recurse through?  I'd rather list them explicitly rather
>>>>> than just recurse down through every rhs1 we encounter.
>>>>
>>>> I don't have a list of codes to test for.  I initially contemplated
>>>> enumerating them but in the end decided the pointer type check would
>>>> be sufficient.  I wouldn't expect a COND_EXPR here.  Don't they get
>>>> transformed into PHIs?  In all my tests they do and and running
>>>> the whole test suite with an assert that it doesn't come up doesn't
>>>> expose any either.  (I left the assert for COND_EXPR there.)  If
>>>> a COND_EXPR really can come up in a GIMPLE assignment here can you
>>>> please show me how so I can add a test for it?
>> A COND_EXPR on the RHS of an assignment is valid gimple.  That's what we
>> need to consider here -- what is and what is not valid gimple.  And its
>> more likely that PHIs will be transformed into RHS COND_EXPRs -- that's
>> standard practice for if-conversion.
>>
>> Gosh, how to get one?  It happens all the time :-)  Since I know DOM so
>> well, I just shoved a quick assert into optimize_stmt to abort if we
>> encounter a gimple assignment where the RHS is a COND_EXPR.  It blew up
>> instantly building libgcc :-)
>>
>> COmpile the attached code with -O2 -m32.
> 
> I wasn't saying it's not valid Gimple, just that I hadn't seen it
> come up here despite compiling Glibc and the kernel with the patched
> GCC.  The only codes I saw are these:
> 
>    addr_expr, array_ref, bit_and_expr, component_ref, max_expr,
>    mem_ref, nop_expr, parm_decl, pointer_plus_expr, ssa_name,
>    and var_decl
> 
> What I was asking for is a test case that makes COND_EXPR come up
> in this context.  But I managed to find one by triggering the ICE
> with GDB.  The file reduced to the following test case:
> 
>    extern struct S s;   // S has to be an incomplete type
> 
>    void* f (int n)
>    {
>      void *p;
>      int x = 0;
> 
>      for (int i = n; i >= 0; i--)
>        {
>          p = &s;
>          if (p == (void*)-1)
>             x = 1;
>          else if (p)
>             return p;
>        }
> 
>      return x ? (void*)-1 : 0;
>    }
> 
> and the dump:
> 
>    <bb 6> [local count: 59055800]:
>    # x_10 = PHI <1(5), 0(2)>
>    _5 = x_10 != 0 ? -1B : 0B;
> 
>    <bb 7> [local count: 114863532]:
>    # _3 = PHI <&s(4), _5(6), &s(3)>
>    return _3;
> 
> It seems a little odd that the COND_EXPR disappears when either
> of the constant addresses becomes the address of an object (or
> the result of alloca), and also when the number of iterations
> of the loop is constant.  That's probably why it so rarely comes
> up in this context.
> 
> That said, even though I've seen a few references to COND_EXPR
> in the midle-end I have been assuming that they, in general, do
> get transformed into PHIs.  But checking the internals manual
> I found in Section 12.6.3 this:
> 
>    A C ?: expression is converted into an if statement with each
>    branch assigning to the same temporary. ... The GIMPLE level
>    if-conversion pass re-introduces ?: expression, if appropriate.
>    It is used to vectorize loops with conditions using vector
>    conditional operations.
> 
> This GDB test case is likely the result of this reintroduction.
> 
>> You'll see them for stuff like this:
>>
>>
>> ;;   basic block 24, loop depth 0
>> ;;    pred:       23
>>    _244 = _1 == 0 ? 1 : 2;
>>
>> In a few spots.
>>
>>
>> In addition to COND_EXPR, I'd be worried about VEC_COND_EXPR,
>> VEC_PERM_EXPR and a host of others.
>>
>> And in a more general sense, this kind of permissiveness is not future
>> proof.  What happens if someone needs to add another EXPR node that is
>> valid on the RHS where such recursion is undesirable?  How are they
>> supposed to know that we've got this permissive recursive call and that
>> it's going to do the wrong thing?  And if it's an EXPR node with no
>> arguments, then we're going to do a read out of the bounds of the object
>> and all bets are off at that point (yes we have zero operand EXPR nodes,
>> but thankfully I don't think they should up in the contexts you care 
>> about).
> 
> Sure.  When things are hardcoded this way the same argument can
> be made both ways.  If we just handle A and B and then someone
> adds an X that matches one of the two, it won't be handled. Either
> way, some work is necessary to deal with the new Z.  One way to
> avoid it would be by providing an API to query this property of
> a code (e.g., a function like int gimple_result_aliases_operand
> (gimple *stmt, int opno)).
> 
>> I'd be much more comfortable with a tight predicate controlling 
>> recursion.
> 
> I've added ADDR_EXPR, COND_EXPR, MAX_EXPR, MIN_EXPR, NOP_EXPR, and
> POINTER_PLUS_EXPR based on the survey I did.  I don't have tests for
> the rest so I've left them out for now.  If/when I come up with them
> I'll add them.
> 
>>>>>> +
>>>>>> +      if (code == GIMPLE_PHI && pmaybe)
>>>>>> +    {
>>>>>> +      unsigned count = 0;
>>>>>> +      gphi *phi_stmt = as_a <gphi *> (def_stmt);
>>>>>> +
>>>>>> +      unsigned nargs = gimple_phi_num_args (phi_stmt);
>>>>>> +      for (unsigned i = 0; i < nargs; ++i)
>>>>>> +        {
>>>>>> +          if (!visited->add (phi_stmt))
>>>>>> +        {
>>>>>> +          tree arg = gimple_phi_arg_def (phi_stmt, i);
>>>>>> +          if (is_addr_local (arg, ploc, pmaybe, visited))
>>>>>> +            ++count;
>>>>>> +        }
>>>>>> +        }
>>>>> So imagine
>>>>>
>>>>> p = phi (x1, x1, x2)
>>>>>
>>>>> Where both x1 and x2 would pass is_addr_local.  I think this code 
>>>>> would
>>>>> incorrectly set pmaybe.
>>>>
>>>> I suppose it would but this happens readily with or without my
>>>> patch, for example here:
>> But those seem like distinct issues.  The one I pointed out looks like a
>> pretty simple case to handle.  It's just a logic error.
> 
> It wasn't a logic error.  I simply didn't think it was necessary
> to worry about the accuracy of an inherently inaccurate warning,
> and I didn't want make more intrusive changes than was called for
> by my simple fix/enhancement.  I was also keeping in mind your
> preference for smaller change sets.  But since you insist I went
> ahead and improved things.  The warning is all the better for it,
> and the changes I made exposed a number of other bugs in GCC.
> At the same time, it seems that the bar for a simple bug fixes
> and small enhancements should not be in excess of what's possible
> by the original design.
> 
> Anyway, attached is an updated revision with the recursion limited
> to just the codes you mentioned, and hopefully with an acceptable
> accuracy.  The patch depends on a fix for the hash_map bug 90923
> that I just posted:
>    https://gcc.gnu.org/ml/gcc-patches/2019-06/msg01105.html
> 
> Martin
> 
>>
>>>>
>>>>     int* f (int i)
>>>>     {
>>>>       int a[2], b[2];
>>>>       int *p = i ? a : b;
>>>>       return p;
>>>>     }
>>>>
>>>> We end up with two instances of "function may return address
>>>> of local variable," one for each local, because
>>>> find_implicit_erroneous_behavior only issues the "may return"
>>>> kind of warning.
>> This looks more like a failing of the core approach in the erroneous
>> path code.
>>
>>>>
>>>> I suspect that no solution will be perfect, at a minimum because
>>>> multiple return statements sometimes get merged into one, so what
>>>> in the source code is a single return that definitely returns
>>>> the address a local may end up merged with one that returns
>>>> a null (and triggering a maybe kind of warning).  I have also
>>>> seen warnings in some non-trivial test cases that suggest that
>>>> some return statements get split up into two where a "may return"
>>>> could turn into a "definitely does return."
>> Certainly.  I don't expect any solution will be perfect here, but I
>> think fixing the logic error in the noted loop would help
>>
>>>>
>>>>> It also seems to me that you can break the loop as soon as you've 
>>>>> got a
>>>>> nonzero count and get a false return from is_addr_local.   Not sure if
>>>>> that'll matter in practice or not.
>>>>
>>>> This is only possible if we're willing to lose some detail (i.e.,
>>>> if we are willing to only point to one variable when returning
>>>> the address of two or more).  I don't find that very user-friendly.
>> ACK.  Ignore that comment then.
>>
>>
>> Jeff
>>
>

On 6/18/19 9:19 PM, Martin Sebor wrote:
> On 6/14/19 2:59 PM, Jeff Law wrote:
[ big snip ]
>> A COND_EXPR on the RHS of an assignment is valid gimple.  That's what we
>> need to consider here -- what is and what is not valid gimple.  And its
>> more likely that PHIs will be transformed into RHS COND_EXPRs -- that's
>> standard practice for if-conversion.
>>
>> Gosh, how to get one?  It happens all the time :-)  Since I know DOM so
>> well, I just shoved a quick assert into optimize_stmt to abort if we
>> encounter a gimple assignment where the RHS is a COND_EXPR.  It blew up
>> instantly building libgcc :-)
>>
>> COmpile the attached code with -O2 -m32.
> 
> I wasn't saying it's not valid Gimple, just that I hadn't seen it
> come up here despite compiling Glibc and the kernel with the patched
> GCC.  The only codes I saw are these:
> 
>   addr_expr, array_ref, bit_and_expr, component_ref, max_expr,
>   mem_ref, nop_expr, parm_decl, pointer_plus_expr, ssa_name,
>   and var_decl
The only one here that's really surprising is the MAX_EXPR.  But it is
what it is.

> 
> What I was asking for is a test case that makes COND_EXPR come up
> in this context.  But I managed to find one by triggering the ICE
> with GDB.  The file reduced to the following test case:
Sorry.  email can be a tough medium to nail down specific details.

> 
>   extern struct S s;   // S has to be an incomplete type
> 
>   void* f (int n)
>   {
>     void *p;
>     int x = 0;
> 
>     for (int i = n; i >= 0; i--)
>       {
>         p = &s;
>         if (p == (void*)-1)
>            x = 1;
>         else if (p)
>            return p;
>       }
> 
>     return x ? (void*)-1 : 0;
>   }
> 
> and the dump:
> 
>   <bb 6> [local count: 59055800]:
>   # x_10 = PHI <1(5), 0(2)>
>   _5 = x_10 != 0 ? -1B : 0B;
> 
>   <bb 7> [local count: 114863532]:
>   # _3 = PHI <&s(4), _5(6), &s(3)>
>   return _3;
> 
> It seems a little odd that the COND_EXPR disappears when either
> of the constant addresses becomes the address of an object (or
> the result of alloca), and also when the number of iterations
> of the loop is constant.  That's probably why it so rarely comes
> up in this context.
Going into phiopt2 we have:

;;   basic block 6, loop depth 0
;;    pred:       5
  if (x_1 != 0)
    goto <bb 8>; [71.00%]
  else
    goto <bb 7>; [29.00%]
;;    succ:       8
;;                7

;;   basic block 7, loop depth 0
;;    pred:       6
;;    succ:       8

;;   basic block 8, loop depth 0
;;    pred:       3
;;                7
;;                6
  # _3 = PHI <&s(3), 0B(7), -1B(6)>
  return _3;

The subgraph starting at block #6 is a classic case for turning branchy
code into straightline code using a COND_EXPR on the RHS of an
assignment.  So you end up with something like this:

;;   basic block 6, loop depth 0
;;    pred:       5
  _5 = x_1 != 0 ? -1B : 0B;
;;    succ:       7

;;   basic block 7, loop depth 0
;;    pred:       3
;;                6
  # _3 = PHI <&s(3), _5(6)>
  return _3;


Now for this specific case within phiopt we are limited to cases there
the result is 0/1 or 0/-1.  That's why you get something different when
you exchange one of the constants for the address of an object, or
anything else for that matter.

This is all a bit academic -- the key point is that we can have a
COND_EXPR on the RHS of an assignment.  That's allowed by gimple.

Sadly this is also likely one of the places where target characteristics
come into play -- targets define a BRANCH_COST which can significantly
change the decisions for the initial generation of conditionals.  It's
one of the things that makes writing  tests for jump threading, if
conversion and other optimizations so damn painful -- on one target
we'll have a series of conditional jumps, on anothers we may have a
series of logicals, potentially with COND_EXPRs.


> 
> That said, even though I've seen a few references to COND_EXPR
> in the midle-end I have been assuming that they, in general, do
> get transformed into PHIs.  But checking the internals manual
> I found in Section 12.6.3 this:
> 
>   A C ?: expression is converted into an if statement with each
>   branch assigning to the same temporary. ... The GIMPLE level
>   if-conversion pass re-introduces ?: expression, if appropriate.
>   It is used to vectorize loops with conditions using vector
>   conditional operations.
> 
> This GDB test case is likely the result of this reintroduction.
Nope.  It happens much earlier in the pipeline :-)


>>
>> And in a more general sense, this kind of permissiveness is not future
>> proof.  What happens if someone needs to add another EXPR node that is
>> valid on the RHS where such recursion is undesirable?  How are they
>> supposed to know that we've got this permissive recursive call and that
>> it's going to do the wrong thing?  And if it's an EXPR node with no
>> arguments, then we're going to do a read out of the bounds of the object
>> and all bets are off at that point (yes we have zero operand EXPR nodes,
>> but thankfully I don't think they should up in the contexts you care about).
>>
> 
> Sure.  When things are hardcoded this way the same argument can
> be made both ways.  If we just handle A and B and then someone
> adds an X that matches one of the two, it won't be handled. Either
> way, some work is necessary to deal with the new Z.  One way to
> avoid it would be by providing an API to query this property of
> a code (e.g., a function like int gimple_result_aliases_operand
> (gimple *stmt, int opno)).
Adding a new opcode could have caused the original code to generate
incorrect code.  In the new version an indirect opcode would just result
in something we refuse to analyze -- so we could miss a warning, but
more importantly we would _not_ transform the return statement so we'd
be safe WRT correct code generation.

WRT making an API to do these kinds of queries.  Sure.  It's a fairly
natural extension to stuff we've done in the past.  You could (for
example) wrap up all kinds existing properties of nodes into an API.
You'll see that's already done in a fairly ad-hoc fashion, but it's not
even close to being pervasive or standard practice.

I would certainly applaud bringing some sanity to this kind of issue and
iterating towards a better place.  But it's ugly, painful work with
little visible end user benefit.

> 
>> But those seem like distinct issues.  The one I pointed out looks like a
>> pretty simple case to handle.  It's just a logic error.
> 
> It wasn't a logic error.  I simply didn't think it was necessary
> to worry about the accuracy of an inherently inaccurate warning,
> and I didn't want make more intrusive changes than was called for
> by my simple fix/enhancement.  I was also keeping in mind your
> preference for smaller change sets.  But since you insist I went
> ahead and improved things.  The warning is all the better for it,
> and the changes I made exposed a number of other bugs in GCC.
> At the same time, it seems that the bar for a simple bug fixes
> and small enhancements should not be in excess of what's possible
> by the original design.
It looked like a logic error to me that could be easily fixed.  I didn't
want to make it out to a huge deal.  ANd yes, there's a natural tension
between addressing small stuff as you see them in a larger kit and
breaking things down and iterating.  There aren't hard and fast rules here.



> 
> 
> gcc-71924.diff
> 
> PR middle-end/71924 - missing -Wreturn-local-addr returning alloca result
> PR middle-end/90549 - missing -Wreturn-local-addr maybe returning an address of a local array plus offset
> 
> gcc/ChangeLog:
> 
> 	PR middle-end/71924
> 	PR middle-end/90549
> 	* gimple-ssa-isolate-paths.c (isolate_path): Add attribute.
> 	(args_loc_t): New type.
> 	(args_loc_t, locmap_t): same.
> 	(diag_returned_locals): New function.
> 	(is_addr_local): Same.
> 	(handle_return_addr_local_phi_arg, warn_return_addr_local): Same.
> 	(find_implicit_erroneous_behavior): Call warn_return_addr_local_phi_arg.
> 	(find_explicit_erroneous_behavior): Call warn_return_addr_local.
> 
> libgcc/ChangeLog:
> 	* generic-morestack.c: Disable -fisolate-erroneous-paths-dereference
> 	to get around PR libgcc/90918.
> 
> gcc/testsuite/ChangeLog:
> 
> 	PR middle-end/71924
> 	PR middle-end/90549
> 	* gcc.dg/Wreturn-local-addr-2.c: New test.
> 	* gcc.dg/Wreturn-local-addr-4.c: New test.
> 	* gcc.dg/Wreturn-local-addr-5.c: New test.
> 	* gcc.dg/Wreturn-local-addr-6.c: New test.
> 	* gcc.dg/Wreturn-local-addr-7.c: New test.
> 	* gcc.dg/Wreturn-local-addr-8.c: New test.
> 	* gcc.dg/Walloca-4.c: Prune expected warnings.
> 	* gcc.dg/pr41551.c: Same.
> 	* gcc.dg/pr59523.c: Same.
> 	* gcc.dg/tree-ssa/pr88775-2.c: Same.
> 	* gcc.dg/winline-7.c: Same.
> 
> diff --git a/gcc/gimple-ssa-isolate-paths.c b/gcc/gimple-ssa-isolate-paths.c
> index 33fe352bb23..13b1f5f2349 100644
> --- a/gcc/gimple-ssa-isolate-paths.c
> +++ b/gcc/gimple-ssa-isolate-paths.c
> @@ -130,7 +130,7 @@ insert_trap (gimple_stmt_iterator *si_p, tree op)
>  
>     Return BB'.  */
>  
> -basic_block
> +ATTRIBUTE_RETURNS_NONNULL basic_block
>  isolate_path (basic_block bb, basic_block duplicate,
>  	      edge e, gimple *stmt, tree op, bool ret_zero)
>  {
> @@ -341,6 +341,283 @@ stmt_uses_0_or_null_in_undefined_way (gimple *stmt)
>    return false;
>  }
>  
> +/* Describes the property of a return statement that may return
> +   the address of one or more local variables.  */
> +struct args_loc_t
> +{
> +  args_loc_t (): nargs (), locvec (), ptr (&ptr)
> +  {
> +    locvec.create (4);
> +  }
> +
> +  args_loc_t (const args_loc_t &rhs)
> +    : nargs (rhs.nargs), locvec (rhs.locvec.copy ()), ptr (&ptr) { }
> +
> +  args_loc_t& operator= (const args_loc_t &rhs)
> +  {
> +    nargs = rhs.nargs;
> +    locvec.release ();
> +    locvec = rhs.locvec.copy ();
> +    return *this;
> +  }
> +
> +  ~args_loc_t ()
> +  {
> +    locvec.release ();
> +    gcc_assert (ptr == &ptr);
> +  }
> +
> +  /* For a PHI in a return statement its number of arguments.  When less
> +     than LOCVEC.LENGTH () implies that an address of local may but need
> +     not be returned by the statement.  Otherwise it implies it definitely
> +     is returned.  Used to issue "may be" vs "is" diagnostics.  */
> +  unsigned nargs;
> +  /* The locations of local variables/alloca calls returned by the return
> +     statement.  Avoid using auto_vec here since it's not safe to copy due
> +     to pr90904.  */
> +  vec <location_t> locvec;
> +  void *ptr;
> +};
Make this a class, please.  We use "struct" for POD.

Is there a strong need for an overloaded operator?  Our guidelines
generally discourage operator overloads.  This one seems on the border
to me.  Others may have different ideas where the line is.  If we really
don't need the overloaded operator, then we can avoid the controversy
completely.


> +
> +/* Return true if EXPR is a expression of pointer type that refers
> +   to the address of a variable with automatic storage duration.
> +   If so, add an entry to *PLOCMAP and insert INTO PLOCMAP->LOCVEC
> +   the locations of the corresponding local variables whose address
> +   is returned by the statement.  VISITED is a bitmap of PHI nodes
> +   already visited by recursive calls.  */
> +
> +static bool
> +is_addr_local (gimple *return_stmt, tree exp, locmap_t *plocmap,
> +	       hash_set<gphi *> *visited)
> +{
> +  if (TREE_CODE (exp) == ADDR_EXPR)
> +    {
> +      tree baseaddr = get_base_address (TREE_OPERAND (exp, 0));
> +      if (TREE_CODE (baseaddr) == MEM_REF)
> +	return is_addr_local (return_stmt, TREE_OPERAND (baseaddr, 0),
> +			      plocmap, visited);
> +
> +      if ((!VAR_P (baseaddr)
> +	   || is_global_var (baseaddr))
> +	  && TREE_CODE (baseaddr) != PARM_DECL)
> +	return false;
> +
> +      args_loc_t &argsloc = plocmap->get_or_insert (return_stmt);
> +      argsloc.locvec.safe_push (DECL_SOURCE_LOCATION (baseaddr));
> +      return true;
> +    }
> +
> +  if (!POINTER_TYPE_P (TREE_TYPE (exp)))
> +    return false;
> +
> +  if (TREE_CODE (exp) == SSA_NAME)
> +    {
> +      gimple *def_stmt = SSA_NAME_DEF_STMT (exp);
> +      enum gimple_code code = gimple_code (def_stmt);
> +
> +      if (is_gimple_assign (def_stmt))
> +	{
> +	  tree type = TREE_TYPE (gimple_assign_lhs (def_stmt));
> +	  if (POINTER_TYPE_P (type))
> +	    {
> +	      tree_code code = gimple_assign_rhs_code (def_stmt);
> +	      tree ptr1 = NULL_TREE, ptr2 = NULL_TREE;
> +	      if (code == COND_EXPR)
> +		{
> +		  ptr1 = gimple_assign_rhs2 (def_stmt);
> +		  ptr2 = gimple_assign_rhs3 (def_stmt);
> +		}
> +	      else if (code == MAX_EXPR || code == MIN_EXPR)
> +		{
> +		  ptr1 = gimple_assign_rhs1 (def_stmt);
> +		  ptr2 = gimple_assign_rhs2 (def_stmt);
> +		}
> +	      else if (code == ADDR_EXPR
> +		       || code == NOP_EXPR
> +		       || code == POINTER_PLUS_EXPR)
> +		ptr1 = gimple_assign_rhs1 (def_stmt);
> +
> +	      /* Avoid short-circuiting the logical OR result in case
> +		 both oerands refer to local variables, in which case
> +		 both should be identified in the warning.  */
> +	      bool res1 = false, res2 = false;
> +	      if (ptr1)
> +		res1 = is_addr_local (return_stmt, ptr1, plocmap, visited);
> +	      if (ptr2)
> +		res2 = is_addr_local (return_stmt, ptr2, plocmap, visited);
> +	      return res1 || res2;
> +	    }
> +	  return false;
> +	}
s/oerands/operands/  a few lines up.

So how do you deal with the case where one operand of a
{MIN,MAX,COND}_EXPR is the address of a local, but the other is not?  It
seems like we'll end up returning true from this function in that case
and potentially trigger changing the return value to NULL.  Or am I
missing something?





Jeff

On 6/26/19 6:11 PM, Jeff Law wrote:
> On 6/18/19 9:19 PM, Martin Sebor wrote:
>> On 6/14/19 2:59 PM, Jeff Law wrote:
> [ big snip ]
>>> A COND_EXPR on the RHS of an assignment is valid gimple.  That's what we
>>> need to consider here -- what is and what is not valid gimple.  And its
>>> more likely that PHIs will be transformed into RHS COND_EXPRs -- that's
>>> standard practice for if-conversion.
>>>
>>> Gosh, how to get one?  It happens all the time :-)  Since I know DOM so
>>> well, I just shoved a quick assert into optimize_stmt to abort if we
>>> encounter a gimple assignment where the RHS is a COND_EXPR.  It blew up
>>> instantly building libgcc :-)
>>>
>>> COmpile the attached code with -O2 -m32.
>>
>> I wasn't saying it's not valid Gimple, just that I hadn't seen it
>> come up here despite compiling Glibc and the kernel with the patched
>> GCC.  The only codes I saw are these:
>>
>>    addr_expr, array_ref, bit_and_expr, component_ref, max_expr,
>>    mem_ref, nop_expr, parm_decl, pointer_plus_expr, ssa_name,
>>    and var_decl
> The only one here that's really surprising is the MAX_EXPR.  But it is
> what it is.
> 
>>
>> What I was asking for is a test case that makes COND_EXPR come up
>> in this context.  But I managed to find one by triggering the ICE
>> with GDB.  The file reduced to the following test case:
> Sorry.  email can be a tough medium to nail down specific details.
> 
>>
>>    extern struct S s;   // S has to be an incomplete type
>>
>>    void* f (int n)
>>    {
>>      void *p;
>>      int x = 0;
>>
>>      for (int i = n; i >= 0; i--)
>>        {
>>          p = &s;
>>          if (p == (void*)-1)
>>             x = 1;
>>          else if (p)
>>             return p;
>>        }
>>
>>      return x ? (void*)-1 : 0;
>>    }
>>
>> and the dump:
>>
>>    <bb 6> [local count: 59055800]:
>>    # x_10 = PHI <1(5), 0(2)>
>>    _5 = x_10 != 0 ? -1B : 0B;
>>
>>    <bb 7> [local count: 114863532]:
>>    # _3 = PHI <&s(4), _5(6), &s(3)>
>>    return _3;
>>
>> It seems a little odd that the COND_EXPR disappears when either
>> of the constant addresses becomes the address of an object (or
>> the result of alloca), and also when the number of iterations
>> of the loop is constant.  That's probably why it so rarely comes
>> up in this context.
> Going into phiopt2 we have:
> 
> ;;   basic block 6, loop depth 0
> ;;    pred:       5
>    if (x_1 != 0)
>      goto <bb 8>; [71.00%]
>    else
>      goto <bb 7>; [29.00%]
> ;;    succ:       8
> ;;                7
> 
> ;;   basic block 7, loop depth 0
> ;;    pred:       6
> ;;    succ:       8
> 
> ;;   basic block 8, loop depth 0
> ;;    pred:       3
> ;;                7
> ;;                6
>    # _3 = PHI <&s(3), 0B(7), -1B(6)>
>    return _3;
> 
> The subgraph starting at block #6 is a classic case for turning branchy
> code into straightline code using a COND_EXPR on the RHS of an
> assignment.  So you end up with something like this:
> 
> ;;   basic block 6, loop depth 0
> ;;    pred:       5
>    _5 = x_1 != 0 ? -1B : 0B;
> ;;    succ:       7
> 
> ;;   basic block 7, loop depth 0
> ;;    pred:       3
> ;;                6
>    # _3 = PHI <&s(3), _5(6)>
>    return _3;
> 
> 
> Now for this specific case within phiopt we are limited to cases there
> the result is 0/1 or 0/-1.  That's why you get something different when
> you exchange one of the constants for the address of an object, or
> anything else for that matter.
> 
> This is all a bit academic -- the key point is that we can have a
> COND_EXPR on the RHS of an assignment.  That's allowed by gimple.
> 
> Sadly this is also likely one of the places where target characteristics
> come into play -- targets define a BRANCH_COST which can significantly
> change the decisions for the initial generation of conditionals.  It's
> one of the things that makes writing  tests for jump threading, if
> conversion and other optimizations so damn painful -- on one target
> we'll have a series of conditional jumps, on anothers we may have a
> series of logicals, potentially with COND_EXPRs.
> 
> 
>>
>> That said, even though I've seen a few references to COND_EXPR
>> in the midle-end I have been assuming that they, in general, do
>> get transformed into PHIs.  But checking the internals manual
>> I found in Section 12.6.3 this:
>>
>>    A C ?: expression is converted into an if statement with each
>>    branch assigning to the same temporary. ... The GIMPLE level
>>    if-conversion pass re-introduces ?: expression, if appropriate.
>>    It is used to vectorize loops with conditions using vector
>>    conditional operations.
>>
>> This GDB test case is likely the result of this reintroduction.
> Nope.  It happens much earlier in the pipeline :-)
> 
> 
>>>
>>> And in a more general sense, this kind of permissiveness is not future
>>> proof.  What happens if someone needs to add another EXPR node that is
>>> valid on the RHS where such recursion is undesirable?  How are they
>>> supposed to know that we've got this permissive recursive call and that
>>> it's going to do the wrong thing?  And if it's an EXPR node with no
>>> arguments, then we're going to do a read out of the bounds of the object
>>> and all bets are off at that point (yes we have zero operand EXPR nodes,
>>> but thankfully I don't think they should up in the contexts you care about).
>>>
>>
>> Sure.  When things are hardcoded this way the same argument can
>> be made both ways.  If we just handle A and B and then someone
>> adds an X that matches one of the two, it won't be handled. Either
>> way, some work is necessary to deal with the new Z.  One way to
>> avoid it would be by providing an API to query this property of
>> a code (e.g., a function like int gimple_result_aliases_operand
>> (gimple *stmt, int opno)).
> Adding a new opcode could have caused the original code to generate
> incorrect code.  In the new version an indirect opcode would just result
> in something we refuse to analyze -- so we could miss a warning, but
> more importantly we would _not_ transform the return statement so we'd
> be safe WRT correct code generation.
> 
> WRT making an API to do these kinds of queries.  Sure.  It's a fairly
> natural extension to stuff we've done in the past.  You could (for
> example) wrap up all kinds existing properties of nodes into an API.
> You'll see that's already done in a fairly ad-hoc fashion, but it's not
> even close to being pervasive or standard practice.
> 
> I would certainly applaud bringing some sanity to this kind of issue and
> iterating towards a better place.  But it's ugly, painful work with
> little visible end user benefit.
> 
>>
>>> But those seem like distinct issues.  The one I pointed out looks like a
>>> pretty simple case to handle.  It's just a logic error.
>>
>> It wasn't a logic error.  I simply didn't think it was necessary
>> to worry about the accuracy of an inherently inaccurate warning,
>> and I didn't want make more intrusive changes than was called for
>> by my simple fix/enhancement.  I was also keeping in mind your
>> preference for smaller change sets.  But since you insist I went
>> ahead and improved things.  The warning is all the better for it,
>> and the changes I made exposed a number of other bugs in GCC.
>> At the same time, it seems that the bar for a simple bug fixes
>> and small enhancements should not be in excess of what's possible
>> by the original design.
> It looked like a logic error to me that could be easily fixed.  I didn't
> want to make it out to a huge deal.  ANd yes, there's a natural tension
> between addressing small stuff as you see them in a larger kit and
> breaking things down and iterating.  There aren't hard and fast rules here.
> 
> 
> 
>>
>>
>> gcc-71924.diff
>>
>> PR middle-end/71924 - missing -Wreturn-local-addr returning alloca result
>> PR middle-end/90549 - missing -Wreturn-local-addr maybe returning an address of a local array plus offset
>>
>> gcc/ChangeLog:
>>
>> 	PR middle-end/71924
>> 	PR middle-end/90549
>> 	* gimple-ssa-isolate-paths.c (isolate_path): Add attribute.
>> 	(args_loc_t): New type.
>> 	(args_loc_t, locmap_t): same.
>> 	(diag_returned_locals): New function.
>> 	(is_addr_local): Same.
>> 	(handle_return_addr_local_phi_arg, warn_return_addr_local): Same.
>> 	(find_implicit_erroneous_behavior): Call warn_return_addr_local_phi_arg.
>> 	(find_explicit_erroneous_behavior): Call warn_return_addr_local.
>>
>> libgcc/ChangeLog:
>> 	* generic-morestack.c: Disable -fisolate-erroneous-paths-dereference
>> 	to get around PR libgcc/90918.
>>
>> gcc/testsuite/ChangeLog:
>>
>> 	PR middle-end/71924
>> 	PR middle-end/90549
>> 	* gcc.dg/Wreturn-local-addr-2.c: New test.
>> 	* gcc.dg/Wreturn-local-addr-4.c: New test.
>> 	* gcc.dg/Wreturn-local-addr-5.c: New test.
>> 	* gcc.dg/Wreturn-local-addr-6.c: New test.
>> 	* gcc.dg/Wreturn-local-addr-7.c: New test.
>> 	* gcc.dg/Wreturn-local-addr-8.c: New test.
>> 	* gcc.dg/Walloca-4.c: Prune expected warnings.
>> 	* gcc.dg/pr41551.c: Same.
>> 	* gcc.dg/pr59523.c: Same.
>> 	* gcc.dg/tree-ssa/pr88775-2.c: Same.
>> 	* gcc.dg/winline-7.c: Same.
>>
>> diff --git a/gcc/gimple-ssa-isolate-paths.c b/gcc/gimple-ssa-isolate-paths.c
>> index 33fe352bb23..13b1f5f2349 100644
>> --- a/gcc/gimple-ssa-isolate-paths.c
>> +++ b/gcc/gimple-ssa-isolate-paths.c
>> @@ -130,7 +130,7 @@ insert_trap (gimple_stmt_iterator *si_p, tree op)
>>   
>>      Return BB'.  */
>>   
>> -basic_block
>> +ATTRIBUTE_RETURNS_NONNULL basic_block
>>   isolate_path (basic_block bb, basic_block duplicate,
>>   	      edge e, gimple *stmt, tree op, bool ret_zero)
>>   {
>> @@ -341,6 +341,283 @@ stmt_uses_0_or_null_in_undefined_way (gimple *stmt)
>>     return false;
>>   }
>>   
>> +/* Describes the property of a return statement that may return
>> +   the address of one or more local variables.  */
>> +struct args_loc_t
>> +{
>> +  args_loc_t (): nargs (), locvec (), ptr (&ptr)
>> +  {
>> +    locvec.create (4);
>> +  }
>> +
>> +  args_loc_t (const args_loc_t &rhs)
>> +    : nargs (rhs.nargs), locvec (rhs.locvec.copy ()), ptr (&ptr) { }
>> +
>> +  args_loc_t& operator= (const args_loc_t &rhs)
>> +  {
>> +    nargs = rhs.nargs;
>> +    locvec.release ();
>> +    locvec = rhs.locvec.copy ();
>> +    return *this;
>> +  }
>> +
>> +  ~args_loc_t ()
>> +  {
>> +    locvec.release ();
>> +    gcc_assert (ptr == &ptr);
>> +  }
>> +
>> +  /* For a PHI in a return statement its number of arguments.  When less
>> +     than LOCVEC.LENGTH () implies that an address of local may but need
>> +     not be returned by the statement.  Otherwise it implies it definitely
>> +     is returned.  Used to issue "may be" vs "is" diagnostics.  */
>> +  unsigned nargs;
>> +  /* The locations of local variables/alloca calls returned by the return
>> +     statement.  Avoid using auto_vec here since it's not safe to copy due
>> +     to pr90904.  */
>> +  vec <location_t> locvec;
>> +  void *ptr;
>> +};
> Make this a class, please.  We use "struct" for POD.

I've made this change.

> Is there a strong need for an overloaded operator?  Our guidelines
> generally discourage operator overloads.  This one seems on the border
> to me.  Others may have different ideas where the line is.  If we really
> don't need the overloaded operator, then we can avoid the controversy
> completely.

The copy assignment operator is necessary for the type to be stored
in a hash_map.  Otherwise, the implicitly generated assignment will
be used instead which is unsafe in vec and results in memory
corription (I opened bug 90904 for this).  After working around this
bug by providing the assignment operator I then ran into the hash_map
bug 90959 that also results in memory corruption.  I spent a few fun
hours tracking down the GCC miscompilations that they led to.

The golden rule in C++ is that every type should either be safe to
copy and assign with the expected semantics or made not assignable
and not copyable by declaring the copy ctor and copy assignment
operator private (or deleted in more modern C++).  It would be very
helpful to detect this kind of problem (classes that aren't safely
assignable and copyable because they acquire/release resources in
ctors/dtors).  Not just to us in GCC but I'm sure to others as well.
It's something I've been hoping to implement for a while now.

>> +
>> +/* Return true if EXPR is a expression of pointer type that refers
>> +   to the address of a variable with automatic storage duration.
>> +   If so, add an entry to *PLOCMAP and insert INTO PLOCMAP->LOCVEC
>> +   the locations of the corresponding local variables whose address
>> +   is returned by the statement.  VISITED is a bitmap of PHI nodes
>> +   already visited by recursive calls.  */
>> +
>> +static bool
>> +is_addr_local (gimple *return_stmt, tree exp, locmap_t *plocmap,
>> +	       hash_set<gphi *> *visited)
>> +{
>> +  if (TREE_CODE (exp) == ADDR_EXPR)
>> +    {
>> +      tree baseaddr = get_base_address (TREE_OPERAND (exp, 0));
>> +      if (TREE_CODE (baseaddr) == MEM_REF)
>> +	return is_addr_local (return_stmt, TREE_OPERAND (baseaddr, 0),
>> +			      plocmap, visited);
>> +
>> +      if ((!VAR_P (baseaddr)
>> +	   || is_global_var (baseaddr))
>> +	  && TREE_CODE (baseaddr) != PARM_DECL)
>> +	return false;
>> +
>> +      args_loc_t &argsloc = plocmap->get_or_insert (return_stmt);
>> +      argsloc.locvec.safe_push (DECL_SOURCE_LOCATION (baseaddr));
>> +      return true;
>> +    }
>> +
>> +  if (!POINTER_TYPE_P (TREE_TYPE (exp)))
>> +    return false;
>> +
>> +  if (TREE_CODE (exp) == SSA_NAME)
>> +    {
>> +      gimple *def_stmt = SSA_NAME_DEF_STMT (exp);
>> +      enum gimple_code code = gimple_code (def_stmt);
>> +
>> +      if (is_gimple_assign (def_stmt))
>> +	{
>> +	  tree type = TREE_TYPE (gimple_assign_lhs (def_stmt));
>> +	  if (POINTER_TYPE_P (type))
>> +	    {
>> +	      tree_code code = gimple_assign_rhs_code (def_stmt);
>> +	      tree ptr1 = NULL_TREE, ptr2 = NULL_TREE;
>> +	      if (code == COND_EXPR)
>> +		{
>> +		  ptr1 = gimple_assign_rhs2 (def_stmt);
>> +		  ptr2 = gimple_assign_rhs3 (def_stmt);
>> +		}
>> +	      else if (code == MAX_EXPR || code == MIN_EXPR)
>> +		{
>> +		  ptr1 = gimple_assign_rhs1 (def_stmt);
>> +		  ptr2 = gimple_assign_rhs2 (def_stmt);
>> +		}
>> +	      else if (code == ADDR_EXPR
>> +		       || code == NOP_EXPR
>> +		       || code == POINTER_PLUS_EXPR)
>> +		ptr1 = gimple_assign_rhs1 (def_stmt);
>> +
>> +	      /* Avoid short-circuiting the logical OR result in case
>> +		 both oerands refer to local variables, in which case
>> +		 both should be identified in the warning.  */
>> +	      bool res1 = false, res2 = false;
>> +	      if (ptr1)
>> +		res1 = is_addr_local (return_stmt, ptr1, plocmap, visited);
>> +	      if (ptr2)
>> +		res2 = is_addr_local (return_stmt, ptr2, plocmap, visited);
>> +	      return res1 || res2;
>> +	    }
>> +	  return false;
>> +	}
> s/oerands/operands/  a few lines up.
> 
> So how do you deal with the case where one operand of a
> {MIN,MAX,COND}_EXPR is the address of a local, but the other is not?  It
> seems like we'll end up returning true from this function in that case
> and potentially trigger changing the return value to NULL.  Or am I
> missing something?

I only briefly considered MIN/MAX but because in C and C++ the less
than and greater than expressions are only defined for pointers into
the same array/object or subobject and I didn't ponder it too hard.
(In C they're undefined otherwise, in C++ the result is unspecified.)

But you bring it up because you think the address should be returned
regardless, even if the expression is invalid (or if it's COND_EXPR
with mixed local/nonlocal addresses) so I've changed it to do that
for all these explicitly handled codes and added tests to verify it.

Retested on x86_64-linux.

Martin

On 6/30/19 3:50 PM, Martin Sebor wrote:
> On 6/26/19 6:11 PM, Jeff Law wrote:
[ Another big snip ]
> 
>> Is there a strong need for an overloaded operator?  Our guidelines
>> generally discourage operator overloads.  This one seems on the border
>> to me.  Others may have different ideas where the line is.  If we really
>> don't need the overloaded operator, then we can avoid the controversy
>> completely.
> 
> The copy assignment operator is necessary for the type to be stored
> in a hash_map.  Otherwise, the implicitly generated assignment will
> be used instead which is unsafe in vec and results in memory
> corription (I opened bug 90904 for this).  After working around this
> bug by providing the assignment operator I then ran into the hash_map
> bug 90959 that also results in memory corruption.  I spent a few fun
> hours tracking down the GCC miscompilations that they led to.
OK.  THanks for explaining why we need the copy assignment operator.


> 
> The golden rule in C++ is that every type should either be safe to
> copy and assign with the expected semantics or made not assignable
> and not copyable by declaring the copy ctor and copy assignment
> operator private (or deleted in more modern C++).  It would be very
> helpful to detect this kind of problem (classes that aren't safely
> assignable and copyable because they acquire/release resources in
> ctors/dtors).  Not just to us in GCC but I'm sure to others as well.
> It's something I've been hoping to implement for a while now.
You've got my blessing to do that :-)

>>
>> So how do you deal with the case where one operand of a
>> {MIN,MAX,COND}_EXPR is the address of a local, but the other is not?  It
>> seems like we'll end up returning true from this function in that case
>> and potentially trigger changing the return value to NULL.  Or am I
>> missing something?
> 
> I only briefly considered MIN/MAX but because in C and C++ the less
> than and greater than expressions are only defined for pointers into
> the same array/object or subobject and I didn't ponder it too hard.
> (In C they're undefined otherwise, in C++ the result is unspecified.)
> 
> But you bring it up because you think the address should be returned
> regardless, even if the expression is invalid (or if it's COND_EXPR
> with mixed local/nonlocal addresses) so I've changed it to do that
> for all these explicitly handled codes and added tests to verify it.
THanks.

So in the thread for strlen/sprintf the issue around unbounded walks up
through PHI argument's SSA_NAME_DEF_STMT was raised.

I think we've got two options here.

One would be to hold this patch until we sort out what the bounds should
look like, then adjust this patch to do something similar.

Or go forward with this patch, then come back and adjust the walker once
we have settled on solution in the other thread.

I'm OK with either.  Your choice.

jeff

On 7/2/19 2:59 PM, Jeff Law wrote:
> On 6/30/19 3:50 PM, Martin Sebor wrote:
>> On 6/26/19 6:11 PM, Jeff Law wrote:
> [ Another big snip ]
>>
>>> Is there a strong need for an overloaded operator?  Our guidelines
>>> generally discourage operator overloads.  This one seems on the border
>>> to me.  Others may have different ideas where the line is.  If we really
>>> don't need the overloaded operator, then we can avoid the controversy
>>> completely.
>>
>> The copy assignment operator is necessary for the type to be stored
>> in a hash_map.  Otherwise, the implicitly generated assignment will
>> be used instead which is unsafe in vec and results in memory
>> corription (I opened bug 90904 for this).  After working around this
>> bug by providing the assignment operator I then ran into the hash_map
>> bug 90959 that also results in memory corruption.  I spent a few fun
>> hours tracking down the GCC miscompilations that they led to.
> OK.  THanks for explaining why we need the copy assignment operator.
> 
> 
>>
>> The golden rule in C++ is that every type should either be safe to
>> copy and assign with the expected semantics or made not assignable
>> and not copyable by declaring the copy ctor and copy assignment
>> operator private (or deleted in more modern C++).  It would be very
>> helpful to detect this kind of problem (classes that aren't safely
>> assignable and copyable because they acquire/release resources in
>> ctors/dtors).  Not just to us in GCC but I'm sure to others as well.
>> It's something I've been hoping to implement for a while now.
> You've got my blessing to do that :-)
> 
>>>
>>> So how do you deal with the case where one operand of a
>>> {MIN,MAX,COND}_EXPR is the address of a local, but the other is not?  It
>>> seems like we'll end up returning true from this function in that case
>>> and potentially trigger changing the return value to NULL.  Or am I
>>> missing something?
>>
>> I only briefly considered MIN/MAX but because in C and C++ the less
>> than and greater than expressions are only defined for pointers into
>> the same array/object or subobject and I didn't ponder it too hard.
>> (In C they're undefined otherwise, in C++ the result is unspecified.)
>>
>> But you bring it up because you think the address should be returned
>> regardless, even if the expression is invalid (or if it's COND_EXPR
>> with mixed local/nonlocal addresses) so I've changed it to do that
>> for all these explicitly handled codes and added tests to verify it.
> THanks.
> 
> So in the thread for strlen/sprintf the issue around unbounded walks up
> through PHI argument's SSA_NAME_DEF_STMT was raised.
> 
> I think we've got two options here.
> 
> One would be to hold this patch until we sort out what the bounds should
> look like, then adjust this patch to do something similar.
> 
> Or go forward with this patch, then come back and adjust the walker once
> we have settled on solution in the other thread.
> 
> I'm OK with either.  Your choice.

I committed the patch as is until the question of which algorithms
to put the limit on is answered (as discussed in the thread Re:
sprintf/strlen integration).

Martin