On Sat, Jun 08, 2013 at 07:48:27PM +0200, Marc Glisse wrote:
> >+/* Instrument division by zero and INT_MIN / -1. */
> >+
> >+tree
> >+ubsan_instrument_division (location_t loc, enum tree_code code,
> >+ tree op0, tree op1)
> >+{
> >+ tree t, tt;
> >+ tree orig = build2 (code, TREE_TYPE (op0), op0, op1);
> >+
> >+ if (TREE_CODE (TREE_TYPE (op0)) != INTEGER_TYPE
> >+ || TREE_CODE (TREE_TYPE (op1)) != INTEGER_TYPE)
> >+ return orig;
>
> Type promotion means that INTEGRAL_TYPE_P wouldn't catch anything
> this doesn't?
This is after type promotion, and e.g. cp_build_binary_op already checks
for == INTEGER_TYPE, so I think enums/bools won't show up here.
> >+ /* If we *know* that the divisor is not -1 or 0, we don't have to
> >+ instrument this expression.
> >+ ??? We could use decl_constant_value to cover up more cases. */
> >+ if (TREE_CODE (op1) == INTEGER_CST
> >+ && integer_nonzerop (op1)
> >+ && !integer_minus_onep (op1))
> >+ return orig;
>
> This is just to speed up compilation a bit, I assume, since fold
> would remove the instrumentation anyway.
Yeah.
>
> >+ tt = fold_build2 (EQ_EXPR, boolean_type_node, op1,
> >+ integer_minus_one_node);
>
> Don't we usually try to have both operands of a comparison of the
> same type?
Not just usually, it really has to be build_int_cst (TREE_TYPE (op1), -1).
And, more importantly, at least in cp_build_binary_op the calls need to be
moved further down in the function, at least after if (processing_template_decl)
but e.g. for division the trouble is that shorten_binary_op is performed
before actually promoting one or both operand to the result_type. I guess
for the diagnostics which prints the types, it would be best to diagnose
using the promoted types and result_type constructed out of that, but
without shorten_binary_op etc., that is just an optimization I think.
So, maybe record the original result_type before shortening, and if
shortening changed that, convert the arguments for the instrumentation only
to the original result_type, otherwise use the conversion done normally.
For shifts this isn't a big deal, because they always use result_type of the
first operand after promotion, and the ubsan handler wants to see two types
there (the question is, does it want for the shift amount look for the
original shift count type, or the one converted to int)?
Also, perhaps it would be better if these ubsan_instrument* functions
didn't return a COMPOUND_EXPR, but instead just the lhs of that (i.e. the
actual instrumentation) and let the caller set some var to that and if that
var is non-NULL, after building the binary operation build a COMPOUND_EXPR
with lhs being the instrumentation and rhs the binary operation itself.
>
> >+ t = fold_build2 (EQ_EXPR, boolean_type_node, op0,
> >+ TYPE_MIN_VALUE (TREE_TYPE (op0)));
>
> I didn't see where this test was restricted to the signed case
> (0u/-1 is well defined)?
>
> >+ t = fold_build2 (TRUTH_AND_EXPR, boolean_type_node, t, tt);
> >+ tt = build2 (EQ_EXPR, boolean_type_node,
> >+ op1, integer_zero_node);
>
> Why not fold this one?
Sure. And yeah, the INT_MIN/-1 checking needs to be done for signed types
only.
> >+tree
> >+ubsan_instrument_shift (location_t loc, enum tree_code code,
> >+ tree op0, tree op1)
> >+{
> >+ tree t, tt = NULL_TREE;
> >+ tree orig = build2 (code, TREE_TYPE (op0), op0, op1);
> >+ tree uprecm1 = build_int_cst (unsigned_type_for (TREE_TYPE (op1)),
> >+ TYPE_PRECISION (TREE_TYPE (op0)) - 1);
> >+ tree precm1 = build_int_cst (TREE_TYPE (op1),
> >+ TYPE_PRECISION (TREE_TYPE (op0)) - 1);
>
> (if we later want to extend this to vector-scalar shifts,
> element_precision will be better than TYPE_PRECISION)
>
> Name unsigned_type_for (TREE_TYPE (op1)) and TYPE_PRECISION
> (TREE_TYPE (op0)) that are used several times?
Yes. Note that for vector-scalar shifts (and even vector-vector) we'd need
library support to handle that.
Jakub
