ASAN: handle addressable params (PR sanitize/81040).

On 06/20/2017 11:32 AM, Jakub Jelinek wrote:
> On Tue, Jun 20, 2017 at 11:23:36AM +0200, Martin Liška wrote:
>>> Then something needs to be done for debugging too.  If it is without VTA,
>>> then probably just having DECL_VALUE_EXPR is good enough, otherwise
>>> (VTA) you probably don't want that (or can reset it at that point), but
>>> instead emit after the initialization stmt a debug stmt that the variable
>>> value now lives in a different var.  Though ideally we want the debugger
>>> to be able to also change the value of the var, that might be harder.
>>> With DECL_VALUE_EXPR on the other side the debug info will be incorrect in
>>> the prologue until it is assigned to the slot.
>>
>> Here I'm not sure about how to distinguish whether to build or not to build
>> the debug statement. According to flag_var_tracking?
> 
> More like if (target_for_debug_bind (arg))
> And if that is false, just make sure DECL_VALUE_EXPR is set to var.
> 
>> You mean something like:
>> g = gimple_build_debug_bind (arg, var, g);
>> ?
> 
> Well, there is no stmt, so the last argument would be just NULL.
> 
>>> I don't understand the distinction.  If you turn the original parm
>>> for complex/vector DECL_GIMPLE_REG_P, you should need the exact same code
>>> (but I think it would be better to use the default SSA_NAME of the PARM_DECL
>>> if it is a gimple reg type, rather than use the PARM_DECL itself
>>> and wait for update_ssa).
>>
>> Yes, the test-case /gcc/testsuite/g++.dg/asan/function-argument-3.C fails for me
>> as one needs to have a temporary SSA name, otherwise:
>>
>> /home/marxin/Programming/gcc/gcc/testsuite/g++.dg/asan/function-argument-3.C:13:1: error: invalid rhs for gimple memory store
>>  foo (v4si arg)
>>  ^~~
>> arg
>>
>> arg
>>
>> # .MEM_4 = VDEF <.MEM_1(D)>
>> arg = arg;
>> during GIMPLE pass: sanopt
>>
>> If I see correctly the function in my test-case does not have default def SSA name for the parameter.
>> Thus I guess I need to create a SSA name?
> 
> I'd expect if you have DECL_GIMPLE_REG_P set on the PARM_DECL and
> use the default def, you shouldn't run into this.
> 
> 	Jakub
> 

Good I fixed that in v2, that passes regression tests.
Ale objections should be resolved in the version.

Ready for trunk?
Martin

PING^1

On 06/20/2017 03:06 PM, Martin Liška wrote:
> On 06/20/2017 11:32 AM, Jakub Jelinek wrote:
>> On Tue, Jun 20, 2017 at 11:23:36AM +0200, Martin Liška wrote:
>>>> Then something needs to be done for debugging too.  If it is without VTA,
>>>> then probably just having DECL_VALUE_EXPR is good enough, otherwise
>>>> (VTA) you probably don't want that (or can reset it at that point), but
>>>> instead emit after the initialization stmt a debug stmt that the variable
>>>> value now lives in a different var.  Though ideally we want the debugger
>>>> to be able to also change the value of the var, that might be harder.
>>>> With DECL_VALUE_EXPR on the other side the debug info will be incorrect in
>>>> the prologue until it is assigned to the slot.
>>>
>>> Here I'm not sure about how to distinguish whether to build or not to build
>>> the debug statement. According to flag_var_tracking?
>>
>> More like if (target_for_debug_bind (arg))
>> And if that is false, just make sure DECL_VALUE_EXPR is set to var.
>>
>>> You mean something like:
>>> g = gimple_build_debug_bind (arg, var, g);
>>> ?
>>
>> Well, there is no stmt, so the last argument would be just NULL.
>>
>>>> I don't understand the distinction.  If you turn the original parm
>>>> for complex/vector DECL_GIMPLE_REG_P, you should need the exact same code
>>>> (but I think it would be better to use the default SSA_NAME of the PARM_DECL
>>>> if it is a gimple reg type, rather than use the PARM_DECL itself
>>>> and wait for update_ssa).
>>>
>>> Yes, the test-case /gcc/testsuite/g++.dg/asan/function-argument-3.C fails for me
>>> as one needs to have a temporary SSA name, otherwise:
>>>
>>> /home/marxin/Programming/gcc/gcc/testsuite/g++.dg/asan/function-argument-3.C:13:1: error: invalid rhs for gimple memory store
>>>  foo (v4si arg)
>>>  ^~~
>>> arg
>>>
>>> arg
>>>
>>> # .MEM_4 = VDEF <.MEM_1(D)>
>>> arg = arg;
>>> during GIMPLE pass: sanopt
>>>
>>> If I see correctly the function in my test-case does not have default def SSA name for the parameter.
>>> Thus I guess I need to create a SSA name?
>>
>> I'd expect if you have DECL_GIMPLE_REG_P set on the PARM_DECL and
>> use the default def, you shouldn't run into this.
>>
>> 	Jakub
>>
> 
> Good I fixed that in v2, that passes regression tests.
> Ale objections should be resolved in the version.
> 
> Ready for trunk?
> Martin
>

On Tue, Jun 20, 2017 at 03:06:56PM +0200, Martin Liška wrote:
> +/* Rewrite all usages of tree OP which is a PARM_DECL with a VAR_DECL
> +   that is it's DECL_VALUE_EXPR.  */
> +
> +static tree
> +rewrite_usage_of_param (tree *op, int *walk_subtrees, void *)
> +{
> +  if (TREE_CODE (*op) == PARM_DECL && DECL_VALUE_EXPR (*op) != NULL_TREE)

DECL_VALUE_EXPR testing is costly (it is a hash table lookup).
Therefore you should test DECL_HAS_VALUE_EXPR_P (*op) after checking
== PARM_DECL.  And DECL_HAS_VALUE_EXPR_P should apply non-NULL
DECL_VALUE_EXPR.
That said, I wonder if we don't create DECL_VALUE_EXPR for PARM_DECLs in
other parts of the compiler, whether it wouldn't be safer to also test here
after == PARM_DECL and DECL_HAS_VALUE_EXPR_P check whether *op is in
addressable_params hash table.

> +    {
> +      *op = DECL_VALUE_EXPR (*op);
> +      *walk_subtrees = 0;
> +    }
> +
> +  return NULL;
> +}
> +
> +/* For a given function FUN, rewrite all addressable parameters so that
> +   a new automatic variable is introduced.  Right after function entry
> +   a parameter is assigned to the variable.  */
> +
> +static void
> +sanitize_rewrite_addressable_params (function *fun)
> +{
> +  gimple *g;
> +  gimple_seq stmts = NULL;
> +  auto_vec<tree> addressable_params;

You don't really use the addressable_params vector anywhere, right?
Except for:

> +
> +  for (tree arg = DECL_ARGUMENTS (current_function_decl);
> +       arg; arg = DECL_CHAIN (arg))
> +    {
> +      if (TREE_ADDRESSABLE (arg) && !TREE_ADDRESSABLE (TREE_TYPE (arg)))
> +	{
> +	  TREE_ADDRESSABLE (arg) = 0;
> +	  /* The parameter is no longer addressable.  */
> +	  tree type = TREE_TYPE (arg);
> +	  addressable_params.safe_push (arg);

pushing stuff into it and later

> +  if (addressable_params.is_empty ())
> +    return;

If you only need that, a bool flag if any params have been changed is
enough.  But see above whether it wouldn't be safer to use a hash table
to verify it.  Plus, I think it would be desirable to clear
DECL_HAS_VALUE_EXPR_P and SET_DECL_VALUE_EXPR to NULL afterwards
if (target_for_debug_bind (arg)) - whch can be done either the with vec
or with a hash table traversal, for that we don't care about the ordering.

> +
> +	  /* Create a new automatic variable.  */
> +	  tree var = build_decl (DECL_SOURCE_LOCATION (arg),
> +				 VAR_DECL, DECL_NAME (arg), type);
> +	  TREE_ADDRESSABLE (var) = 1;
> +	  DECL_ARTIFICIAL (var) = 1;
> +	  DECL_SEEN_IN_BIND_EXPR_P (var) = 0;

This is 0 already from build_decl, IMHO no need to set it.

> +	  gimple_add_tmp_var (var);
> +
> +	  if (dump_file)
> +	    fprintf (dump_file,
> +		     "Rewriting parameter whose address is taken: %s\n",
> +		     IDENTIFIER_POINTER (DECL_NAME (arg)));
> +
> +	  SET_DECL_VALUE_EXPR (arg, var);

But obviously you miss setting DECL_HAS_VALUE_EXPR_P here.

> +	  /* Assign value of parameter to newly created variable.  */
> +	  if ((TREE_CODE (type) == COMPLEX_TYPE
> +	       || TREE_CODE (type) == VECTOR_TYPE))
> +	    {
> +	      /* We need to create a SSA name that will be used for the
> +		 assignment.  */

Why don't you just set DECL_GIMPLE_REG_P (arg) = 1; for
COMPLEX_TYPE/VECTOR_TYPE?  The arg is going to be only used to copy it into
the new var.  And then just use get_or_create_ssa_default_def,
regardless of whether if is complex/vector or other.

> +  /* Replace all usages of PARM_DECLs with the newly
> +     created variable VAR.  */
> +  basic_block bb;
> +  FOR_EACH_BB_FN (bb, fun)
> +    {
> +      gimple_stmt_iterator gsi;
> +      for (gsi = gsi_start_bb (bb); !gsi_end_p (gsi); gsi_next (&gsi))
> +	{
> +	  gimple *stmt = gsi_stmt (gsi);
> +	  gimple_stmt_iterator it = gsi_for_stmt (stmt);
> +	  walk_gimple_stmt (&it, NULL, rewrite_usage_of_param, NULL);
> +	}
> +      for (gsi = gsi_start_phis (bb); !gsi_end_p (gsi); gsi_next (&gsi))
> +	{
> +	  gphi *phi = dyn_cast<gphi *> (gsi_stmt (gsi));
> +	  for (unsigned i = 0; i < gimple_phi_num_args (phi); ++i)
> +	    {
> +	      hash_set<tree> visited_nodes;
> +	      walk_tree (gimple_phi_arg_def_ptr (phi, i),
> +			 rewrite_usage_of_param, NULL, &visited_nodes);
> +	    }

Doesn't walk_gimple_stmt on the PHI handle this?

	Jakub