Hi!
On Tue, Jun 11, 2013 at 09:14:36PM +0200, Marc Glisse wrote:
> Hello,
>
> couple comments (not a true review)
Thanks anyway ;).
> On Tue, 11 Jun 2013, Marek Polacek wrote:
>
> >+tree
> >+ubsan_instrument_division (location_t loc, tree op0, tree op1)
> >+{
> >+ tree t, tt;
> >+ tree type0 = TREE_TYPE (op0);
> >+ tree type1 = TREE_TYPE (op1);
>
> Can the 2 types be different? I thought divisions had homogeneous
> arguments, and the instrumentation was done late enough to avoid any
> potential issue, but maybe not...
Yeah, they can; they only have to be of arithmetic type.
> >+ tree type1_zero_cst = build_int_cst (type1, 0);
>
> It is a bit funny to do that before the following test ;-)
Perhaps, yes. Moved it...
> >+ if (TREE_CODE (type0) != INTEGER_TYPE
> >+ || TREE_CODE (type1) != INTEGER_TYPE)
> >+ return NULL_TREE;
..here.
> >+ /* We check INT_MIN / -1 only for signed types. */
> >+ if (!TYPE_UNSIGNED (type0) && !TYPE_UNSIGNED (type1))
> >+ {
> >+ tt = fold_build2 (EQ_EXPR, boolean_type_node, op1,
> >+ build_int_cst (type1, -1));
> >+ t = fold_build2 (EQ_EXPR, boolean_type_node, op0,
> >+ TYPE_MIN_VALUE (type0));
> >+ t = fold_build2 (TRUTH_AND_EXPR, boolean_type_node, t, tt);
> >+ }
> >+ else
> >+ t = type1_zero_cst;
> >+ tt = fold_build2 (EQ_EXPR, boolean_type_node,
> >+ op1, type1_zero_cst);
> >+ t = fold_build2 (TRUTH_OR_EXPR, boolean_type_node, tt, t);
>
> If you wrote the comparison with 0 first, you could put the OR in
> the signed branch instead of relying on folding |0, no?
Duh, indeed. Will adjust. Thanks!
Marek
