On Tue, Oct 23, 2012 at 03:11:29PM +0200, Dodji Seketeli wrote:
> * asan.c (insert_if_then_before_iter)
> (instrument_mem_region_access)
> (maybe_instrument_builtin_call, maybe_instrument_call): New static
Why not just write it:
* asan.c (insert_if_then_before_iter, instrument_mem_region_access,
maybe_instrument_builtin_call, maybe_instrument_call): New static
?
> functions.
+ tree pointed_to_type = TREE_TYPE (TREE_TYPE (base));
Shouldn't pointed_to_type be always char_type_node?
I mean it shouldn't be VOID_TYPE, even when the argument is (void *),
etc.
> + /* The 'then block' of the 'if (len != 0) condition is where
> + we'll generate the asan instrumentation code now. */
> + gsi = gsi_start_bb (then_bb);
> +
> + /* Instrument the beginning of the memory region to be accessed,
> + and arrange for the rest of the intrumentation code to be
> + inserted in the then block *after* the current gsi. */
> + build_check_stmt (base, &gsi, location, is_store,
> + int_size_in_bytes (pointed_to_type));
> + gsi = gsi_last_bb (then_bb);
> + }
> + else
> + {
> + /* Instrument the beginning of the memory region to be
> + accessed. */
> + build_check_stmt (base, iter, location, is_store,
> + int_size_in_bytes (pointed_to_type));
> + gsi = *iter;
> + }
Is there any reason why you can't call build_check_stmt just once, after
the conditional? I.e. do
...
gsi = gsi_start_bb (then_bb);
}
else
gsi = *iter;
build_check_stmt (base, &gsi, location, is_store,
int_size_in_bytes (pointed_to_type));
> + /* instrument access at _2; */
> + gsi_next (&gsi);
> + tree end = gimple_assign_lhs (region_end);
> + build_check_stmt (end, &gsi, location, is_store,
Can't you just pass gimple_assign_lhs (region_end) as first
argument to build_check_stmt? And again, I think you want
to test a single byte there, not more.
> + int_size_in_bytes (TREE_TYPE (end)));
> + switch (DECL_FUNCTION_CODE (callee))
> + {
> + /* (s, s, n) style memops. */
> + case BUILT_IN_BCMP:
> + case BUILT_IN_MEMCMP:
> + /* These cannot be safely instrumented as their length parameter
> + is just a mere limit.
> +
> + case BUILT_IN_STRNCASECMP:
> + case BUILT_IN_STRNCMP: */
I think these comments make the code less readable instead of more readable,
I'd move the comments why something can't be instrumented to the default:
case.
On the other side, you IMHO want to handle here also __atomic_* and __sync_*
builtins (not by using instrument_mem_region_access, but
just instrument_derefs (if the argument is ADDR_EXPR, on what it points to,
otherwise if it is SSA_NAME, on MEM_REF created for it).
Jakub
