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
