Hi!
On Thu, Jun 19, 2014 at 04:56:53PM +0200, Marek Polacek wrote:
Thanks for working on this.
> --- gcc/asan.c
> +++ gcc/asan.c
> @@ -2761,6 +2761,9 @@ pass_sanopt::execute (function *fun)
> case IFN_UBSAN_NULL:
> ubsan_expand_null_ifn (gsi);
> break;
> + case IFN_UBSAN_BOUNDS:
> + ubsan_expand_bounds_btn (&gsi);
> + break;
> default:
Why *_btn instead of *_ifn ?
> +static tree
> +ubsan_walk_array_refs_r (tree *tp, int *walk_subtrees, void *data)
> +{
> + struct pointer_set_t *pset = (struct pointer_set_t *) data;
> +
> + if (TREE_CODE (*tp) == BIND_EXPR)
> + {
I think it would be worth adding here a comment why do you handle BIND_EXPR
here, that it doesn't walk the vars, but only their initializers etc. and
thus in order to prevent walking DECL_INITIAL of TREE_STATIC decls
we have to duplicate this part of walk_tree.
> + for (tree decl = BIND_EXPR_VARS (*tp); decl; decl = DECL_CHAIN (decl))
> + {
> + if (TREE_STATIC (decl))
> + {
> + *walk_subtrees = 0;
> + continue;
> + }
> + walk_tree (&DECL_INITIAL (decl), ubsan_walk_array_refs_r, NULL, pset);
> + walk_tree (&DECL_SIZE (decl), ubsan_walk_array_refs_r, NULL, pset);
> + walk_tree (&DECL_SIZE_UNIT (decl), ubsan_walk_array_refs_r, NULL,
> pset);
Shouldn't that use pset, pset); or data, pset); ?
Also, too long lines (at least the last one, first one likely too).
> + tree bound = TYPE_MAX_VALUE (domain);
> + if (ignore_off_by_one)
> + bound = fold_build2 (PLUS_EXPR, TREE_TYPE (bound), bound,
> + build_int_cst (TREE_TYPE (bound), 1));
> +
> + /* Detect flexible array members and suchlike. */
> + tree base = get_base_address (array);
> + if (base && TREE_CODE (base) == INDIRECT_REF)
I'd check also == MEM_REF here, while the FEs often use INDIRECT_REFs,
there are already spots where it creates MEM_REFs.
> +void
> +ubsan_maybe_instrument_array_ref (tree *expr_p, bool ignore_off_by_one)
> +{
> + if (!ubsan_array_ref_instrumented_p (*expr_p)
> + && current_function_decl != 0
Please use != NULL_TREE.
> + && !lookup_attribute ("no_sanitize_undefined",
> + DECL_ATTRIBUTES (current_function_decl)))
> + {
> + tree t = copy_node (*expr_p);
> + tree op0 = TREE_OPERAND (t, 0);
> + tree op1 = TREE_OPERAND (t, 1);
Please don't call copy_node until you know you want to instrument it.
I.e.
tree op0 = TREE_OPERAND (*expr_p, 0);
tree op1 = TREE_OPERAND (*expr_p, 1);
> + tree e = ubsan_instrument_bounds (EXPR_LOCATION (t), op0, &op1,
s/t/*expr_p/ above.
> + ignore_off_by_one);
> + if (e != NULL_TREE)
> + {
and only here add:
tree t = copy_node (*expr_p);
> + TREE_OPERAND (t, 1) = build2 (COMPOUND_EXPR, TREE_TYPE (op1),
> + e, op1);
> + *expr_p = t;
> + }
> + }
> +}
> --- gcc/tree-pretty-print.c
> +++ gcc/tree-pretty-print.c
> @@ -3218,6 +3218,13 @@ print_call_name (pretty_printer *buffer, tree node,
> int flags)
> {
> tree op0 = node;
>
> + if (node == NULL_TREE)
> + {
> + /* TODO Print builtin name. */
> + pp_string (buffer, "<internal function call>");
Use internal_fn_name function?
Jakub
