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

[Jakub Jelinek <jakub at redhat dot com>]

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

> +
> +	  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).

	Jakub