Re: [PATCH 3/3] [asan] Instrument built-in memory access function calls

[Jakub Jelinek <jakub at redhat dot com>]

On Fri, Oct 26, 2012 at 12:23:41AM +0200, Dodji Seketeli wrote:
> +/* Instrument the strlen builtin call pointed to by ITER.
> +
> +   This function instruments the access to the first byte of the
> +   argument, right before the call.  After the call it instruments the
> +   access to the last byte of the argument; it uses the result of the
> +   call to deduce the offset of that last byte.  */
> +
> +static void
> +instrument_strlen_call (gimple_stmt_iterator *iter)
> +{
> +  gimple call = gsi_stmt (*iter);
> +  gcc_assert (is_gimple_call (call));
> +
> +  tree callee = gimple_call_fndecl (call);
> +  gcc_assert (is_builtin_fn (callee)
> +	      && DECL_BUILT_IN_CLASS (callee) == BUILT_IN_NORMAL
> +	      && DECL_FUNCTION_CODE (callee) == BUILT_IN_STRLEN);
> +
> +  location_t location = gimple_location (call);

I'd call the var just loc, that is far more commonly used and shorter.

> +  /* If we initially had an instruction like:
> +
> +	 int n = strlen (str)
> +
> +     we now want to instrument the access to str[n-1], after the
> +     instruction above, if n is greater than 0.  */
> +
> +  /* So let's build the access to str[n-1], in a if (n > 0) {} block.  */
> +
> +  tree len = gimple_call_lhs (call);

I'd add
  if (len == NULL_TREE)
    return;
just in case.  While strlen is a pure function, not sure if some pass isn't
just clearing the lhs as unneeded without removing it immediately.

> +  /*  if (len > 0); */
> +  gimple cond = gimple_build_cond (GT_EXPR, len,
> +				   build_int_cst (size_type_node, 0),
> +				   NULL, NULL);

But more importantly, you shouldn't guard it with len > 0, and shouldn't
test str[n-1], but str[n] (also adjust the comment above).
Because strlen must read even the '\0' byte at str[n].


> +    case BUILT_IN_ATOMIC_FETCH_OR_16:
> +      {
> +	dest = gimple_call_arg (call, 0);
> +	/* So DEST represents the address of a memory location.
> +	   instrument_derefs wants the memory location, so lets
> +	   dereference the address DEST before handing it to
> +	   instrument_derefs.  */
> +	if (TREE_CODE (dest) == ADDR_EXPR)
> +	  dest = TREE_OPERAND (dest, 0);
> +	else if (TREE_CODE (dest) == SSA_NAME)
> +	  dest = build2 (MEM_REF, TREE_TYPE (TREE_TYPE (dest)),
> +			 dest, build_int_cst (TREE_TYPE (dest), 0));
> +	else
> +	  gcc_unreachable ();
> +
> +	instrument_derefs (iter, dest, location, is_store);
> +	return true;
> +      }

{} around this is unneeded I think.

Otherwise looks good.

	Jakub