Hello,

couple comments (not a true review)

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...

+  tree type1_zero_cst = build_int_cst (type1, 0);

It is a bit funny to do that before the following test ;-)

+  if (TREE_CODE (type0) != INTEGER_TYPE
+      || TREE_CODE (type1) != INTEGER_TYPE)
+    return NULL_TREE;
+
+  /* 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 NULL_TREE;
+
+  /* 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?

+  tt = builtin_decl_explicit (BUILT_IN_UBSAN_HANDLE_DIVREM_OVERFLOW);
+  tt = build_call_expr_loc (loc, tt, 0);
+  t = fold_build3 (COND_EXPR, void_type_node, t, tt, void_zero_node);
+
+  return t;
+}

--
Marc Glisse

Reply via email to