On Fri, Oct 06, 2017 at 12:21:00PM +0200, Martin Liška wrote:
> +/* Return true when pointer PTR for a given CUR_OFFSET is already sanitized
> + in a given sanitization context CTX. */
> +
> +static bool
> +has_dominating_ubsan_ptr_check (sanopt_ctx *ctx, tree ptr,
> + offset_int &cur_offset)
> +{
> + bool positive = !wi::neg_p (cur_offset);
> +
> + sanopt_tree_couple couple;
Perhaps s/positive/pos_p/ for the vars too? And remove the empty line
in between bool pos_p = ...; and sanopt_tree_couple couple;
> + couple.ptr = ptr;
> + couple.pos_p = positive;
> +
> + auto_vec<gimple *> &v = ctx->ptr_check_map.get_or_insert (couple);
> + gimple *g = maybe_get_dominating_check (v);
> + if (!g)
> + return false;
> +
> + /* We already have recorded a UBSAN_PTR check for this pointer. Perhaps we
> + can drop this one. But only if this check doesn't specify larger
> offset.
> + */
> + tree offset = gimple_call_arg (g, 1);
> + gcc_assert (TREE_CODE (offset) == INTEGER_CST);
> + offset_int ooffset = wi::sext (wi::to_offset (offset), POINTER_SIZE);
> +
> + if (positive && wi::les_p (cur_offset, ooffset))
> + return true;
> + else if (!positive && wi::les_p (ooffset, cur_offset))
> + return true;
Maybe better as:
if (pos_p)
{
if (wi::les_p (cur_offset, ooffset))
return true;
}
else if (wi::les_p (ooffset, cur_offset))
return true;
?
> +/* Record UBSAN_PTR check of given context CTX. Register pointer PTR on
> + a given OFFSET that it's handled by GIMPLE STMT. */
> +
> +static void
> +record_ubsan_ptr_check_stmt (sanopt_ctx *ctx, gimple *stmt, tree ptr,
> + const offset_int &offset)
> +{
> + bool positive = !wi::neg_p (offset);
> +
> + sanopt_tree_couple couple;
Similarly. Or you could in this case just write couple.pos_p = !wi::neg_p
(offset);
> + couple.ptr = ptr;
> + couple.pos_p = positive;
> +
> + auto_vec<gimple *> &v = ctx->ptr_check_map.get_or_insert (couple);
> + v.safe_push (stmt);
> +}
> +
> + tree cur_offset = gimple_call_arg (stmt, 1);
I wonder if instead of having cur_offset mean offset_int above and
tree here you couldn't use say off for the tree and cur_offset for
the offset_int.
> + offset_int cur_offset_int = wi::sext (wi::to_offset (cur_offset),
> + POINTER_SIZE);
I personally find it more readable to put = on a new line:
offset_int cur_offset_int
= wi::sext (wi::to_offset (cur_offset), POINTER_SIZE);
Though, in this case with the above name changes it could even fit:
offset_int cur_offset = wi::sext (wi::to_offset (off), POINTER_SIZE);
> + offset_int expr_offset = bitpos / BITS_PER_UNIT;
> + offset_int total_offset = expr_offset + cur_offset_int;
And wi::neg_p (expr_offset) can be more cheaply written as bitpos < 0
in all spots below.
What I'd add here is
if (total_offset != wi::sext (total_offset, POINTER_SIZE))
{
record_ubsan_ptr_check_stmt (...);
return false;
}
though (or branch to that stuff at the end of function).
> +
> + /* If BASE is a fixed size automatic variable or
> + global variable defined in the current TU, we don't have
> + to instrument anything if offset is within address
> + of the variable. */
> + if ((VAR_P (base)
> + || TREE_CODE (base) == PARM_DECL
> + || TREE_CODE (base) == RESULT_DECL)
> + && DECL_SIZE_UNIT (base)
> + && TREE_CODE (DECL_SIZE_UNIT (base)) == INTEGER_CST
> + && (!is_global_var (base) || decl_binds_to_current_def_p (base)))
> + {
> + offset_int base_size = wi::to_offset (DECL_SIZE_UNIT (base));
> + if (!wi::neg_p (expr_offset) && wi::les_p (total_offset,
> + base_size))
Similar comment about line breaking, putting && on another line would
make the args to the function on one line.
> + {
> + if (!wi::neg_p (total_offset)
> + && wi::les_p (total_offset, base_size)
> + && wi::fits_shwi_p (total_offset))
Why wi::fits_shwi_p? total_offset is non-negative and smaller or equal
than base_size, so that should be enough to return true.
And if it is to make sure total_offset has not overflown, then that should
be done by the sext, otherwise it will only work for 64-bit arches.
> + return true;
> + }
> + }
> +
> + /* Following expression: UBSAN_PTR (&MEM_REF[ptr + x], y) can be
> + handled as follows:
> +
> + 1) sign (x) == sign (y), then check for dominating check of (x + y)
> + 2) sign (x) != sign (y), then first check if we have a dominating
> + check for ptr + x. If so, then we have 2 situations:
> + a) sign (x) == sign (x + y), here we are done, example:
> + UBSAN_PTR (&MEM_REF[ptr + 100], -50)
> + b) check for dominating check of ptr + x + y.
> + */
> +
> + bool sign_cur_offset = !wi::neg_p (cur_offset_int);
> + bool sign_expr_offset = !wi::neg_p (expr_offset);
> +
> + tree base_addr = build1 (ADDR_EXPR,
> + build_pointer_type (TREE_TYPE (base)),
> + base);
Similarly
tree base_addr
= build1 (ADDR_EXPR, build_pointer_type (TREE_TYPE (base)), base);
is more compact and IMHO readable.
> +
> + bool add = false;
> + if (sign_cur_offset == sign_expr_offset)
> + {
> + if (has_dominating_ubsan_ptr_check (ctx, base_addr, total_offset))
> + return true;
> + else
> + add = true;
> + }
> + else
> + {
> + if (has_dominating_ubsan_ptr_check (ctx, base_addr, expr_offset))
> + {
> + bool sign_total_offset = !wi::neg_p (total_offset);
> + if (sign_expr_offset == sign_total_offset)
> + return true;
> + else
> + {
> + if (has_dominating_ubsan_ptr_check (ctx, base_addr,
> + total_offset))
> + return true;
> + else
> + add = true;
> + }
> + }
> + else
> + {} /* Don't record base_addr + expr_offset, it's not a guarding
> + check. */
I don't like the {} much, just use /* ... */; ?
> + }
> +
> + /* Record a new dominating check for base_addr + total_offset. */
> + if (add
> + && !operand_equal_p (base, base_addr, 0)
> + && wi::fits_shwi_p (total_offset))
Again, see above about fits_shwi_p.
> + record_ubsan_ptr_check_stmt (ctx, stmt, base_addr,
> + total_offset);
> + }
> + }
> +
> + /* For this PTR we don't have any UBSAN_PTR stmts recorded, so there's
> + nothing to optimize yet. */
> + record_ubsan_ptr_check_stmt (ctx, stmt, ptr, cur_offset_int);
> +
> + return false;
> +}
> +
> +#define SIZE_MAX __PTRDIFF_MAX__
Can you call it SMAX or whatever else that doesn't clash with stdint.h
SIZE_MAX that is a different thing?
> +/* { dg-final { scan-assembler-times
> "call\\s*__ubsan_handle_pointer_overflow" 17 } } */
\\s+ instead? You don't want to match call__ubsan_handle_pointer_overflow ,
do you?
Jakub
