Re: [PATCH] ASAN: handle addressable params (PR sanitize/81040).

["Martin Liška" <mliska@suse.cz>]

On 06/19/2017 04:13 PM, Jakub Jelinek wrote:
> On Mon, Jun 19, 2017 at 03:50:42PM +0200, Martin Liška wrote:
>> @@ -858,6 +862,117 @@ sanitize_asan_mark_poison (void)
>>      }
>>  }
>>  
> 
> Missing function comment.
> 
>> +static tree
>> +rewrite_usage_of_param (tree *op, int *walk_subtrees, void *data)
>> +{
>> +  struct walk_stmt_info *wi = (struct walk_stmt_info *) data;
>> +  std::pair<tree, tree> *replacement = (std::pair<tree, tree> *)wi->info;
> 
> Missing space after )
> 
>> +
>> +  if (*op == replacement->first)
>> +    {
>> +      *op = replacement->second;
>> +      *walk_subtrees = 0;
>> +    }
>> +
>> +  return NULL;
>> +}
> 
>> +static void
>> +sanitize_rewrite_addressable_params (function *fun)
>> +{
>> +  basic_block entry_bb = NULL;
>> +
>> +  for (tree arg = DECL_ARGUMENTS (current_function_decl);
>> +       arg; arg = DECL_CHAIN (arg))
>> +    {
>> +      if (TREE_ADDRESSABLE (arg) && !TREE_ADDRESSABLE (TREE_TYPE (arg)))
>> +	{
>> +	  /* The parameter is no longer addressable.  */
>> +	  tree type = TREE_TYPE (arg);
>> +	  TREE_ADDRESSABLE (arg) = 0;
>> +
>> +	  /* 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;
> 
> I think it is highly inefficient to walk the whole IL for every addressable
> argument.  Can't you first find out what PARM_DECLs you need to change,
> stick the corresponding VAR_DECL somewhere (dunno, e.g. a vector with pairs
> perhaps sorted by DECL_UID, or stick it into DECL_VALUE_EXPR or whatever),
> then if you create at least one, walk whole IL and replace all the
> PARM_DECLs you want to replace, then finally clear the TREE_ADDRESSABLE
> flag for all of them and emit the initialization sequence?

Yes, this is doable. I've done that.

> 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?

You mean something like:
g = gimple_build_debug_bind (arg, var, g);
?

> 
>> +
>> +	  gimple_add_tmp_var (var);
>> +
>> +	  if (dump_file)
>> +	    fprintf (dump_file,
>> +		     "Rewritting parameter whos address is taken: %s\n",
>> +		     IDENTIFIER_POINTER (DECL_NAME (arg)));
> 
> s/tting/ting/, s/whos/whose/ 
>> +
>> +	  gimple_seq stmts = NULL;
>> +
>> +	  /* 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.  */
>> +	      tree tmp = make_ssa_name (type);
>> +	      gimple *g = gimple_build_assign (tmp, arg);
>> +	      gimple_set_location (g, DECL_SOURCE_LOCATION (arg));
>> +	      gimple_seq_add_stmt (&stmts, g);
>> +	      g = gimple_build_assign (var, tmp);
>> +	      gimple_set_location (g, DECL_SOURCE_LOCATION (arg));
>> +	      gimple_seq_add_stmt (&stmts, g);
>> +	    }
>> +	  else
>> +	    {
>> +	      gimple *g = gimple_build_assign (var, arg);
>> +	      gimple_set_location (g, DECL_SOURCE_LOCATION (arg));
>> +	      gimple_seq_add_stmt (&stmts, g);
>> +	    }
> 
> 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?

Thanks,
Martin



> 
> 	Jakub
>