This is the mail archive of the gcc-patches@gcc.gnu.org mailing list for the GCC project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: [PATCH] Add signed integer overflow checking to ubsan


On 11/27/13 01:13, Marek Polacek wrote:
On Fri, Nov 22, 2013 at 10:54:16AM +0100, Marek Polacek wrote:
Hi!

Working virtually out of Pago Pago.

The following is the implementation of the signed integer overflow
checking for the UndefinedBehaviorSanitizer.  I wrote some of the
generic bits; Jakub did the i?86 handlind/optabs as well as VRP/fold
bits.

I'd like to ping this patch.  Here's a rebased version that contains
a fix for miscompilation with -Os -m32.

2013-11-27  Jakub Jelinek  <jakub@redhat.com>
             Marek Polacek  <polacek@redhat.com>

         * opts.c (common_handle_option): Handle
         -fsanitize=signed-integer-overflow.
         * config/i386/i386.md (addv<mode>4, subv<mode>4, mulv<mode>4,
         negv<mode>3, negv<mode>3_1): Define expands.
         (*addv<mode>4, *subv<mode>4, *mulv<mode>4, *negv<mode>3): Define
         insns.
        	* sanitizer.def (BUILT_IN_UBSAN_HANDLE_ADD_OVERFLOW,
         BUILT_IN_UBSAN_HANDLE_SUB_OVERFLOW,
         BUILT_IN_UBSAN_HANDLE_MUL_OVERFLOW,
         BUILT_IN_UBSAN_HANDLE_NEGATE_OVERFLOW): Define.
         * ubsan.h (PROB_VERY_UNLIKELY, PROB_EVEN, PROB_VERY_LIKELY,
         PROB_ALWAYS): Define.
         (ubsan_build_overflow_builtin): Declare.
        	* gimple-fold.c (gimple_fold_stmt_to_constant_1): Add folding of
         internal functions.
         * ubsan.c (PROB_VERY_UNLIKELY): Don't define here.
         (ubsan_build_overflow_builtin): New function.
         (instrument_si_overflow): Likewise.
         (ubsan_pass): Add signed integer overflow checking.
         (gate_ubsan): Enable the pass also when SANITIZE_SI_OVERFLOW.
         * flag-types.h (enum sanitize_code): Add SANITIZE_SI_OVERFLOW.
         * internal-fn.c: Include ubsan.h and target.h.
         (ubsan_expand_si_overflow_addsub_check): New function.
         (ubsan_expand_si_overflow_neg_check): Likewise.
        	(ubsan_expand_si_overflow_mul_check): Likewise.
         (expand_UBSAN_CHECK_ADD): Likewise.
         (expand_UBSAN_CHECK_SUB): Likewise.
         (expand_UBSAN_CHECK_MUL): Likewise.
         * fold-const.c (fold_binary_loc): Don't fold A + (-B) -> A - B and
         (-A) + B -> B - A when doing the signed integer overflow checking.
         * internal-fn.def (UBSAN_CHECK_ADD, UBSAN_CHECK_SUB, UBSAN_CHECK_MUL):
         Define.
         * tree-vrp.c (extract_range_basic): Handle internal calls.
         * optabs.def (addv4_optab, subv4_optab, mulv4_optab, negv4_optab): New
         optabs.
c-family/
         * c-gimplify.c (c_gimplify_expr): If doing the integer-overflow
         sanitization, call unsigned_type_for only when !TYPE_OVERFLOW_WRAPS.
testsuite/
         * c-c++-common/ubsan/overflow-mul-2.c: New test.
         * c-c++-common/ubsan/overflow-add-1.c: New test.
         * c-c++-common/ubsan/overflow-add-2.c: New test.
         * c-c++-common/ubsan/overflow-mul-1.c: New test.
         * c-c++-common/ubsan/overflow-sub-1.c: New test.
         * c-c++-common/ubsan/overflow-sub-2.c: New test.
         * c-c++-common/ubsan/overflow-negate-1.c: New test.
Perhaps split this patch into two parts which can be reviewed independently, but go into the tree at the same time. The obvious hope would be that Uros or one of the other x86 backend folks could chime in on that part.




--- gcc/ubsan.h.mp	2013-11-27 08:46:28.046629473 +0100
+++ gcc/ubsan.h	2013-11-27 08:46:57.578753342 +0100
@@ -21,6 +21,12 @@ along with GCC; see the file COPYING3.
  #ifndef GCC_UBSAN_H
  #define GCC_UBSAN_H

+/* From predict.c.  */
+#define PROB_VERY_UNLIKELY	(REG_BR_PROB_BASE / 2000 - 1)
+#define PROB_EVEN		(REG_BR_PROB_BASE / 2)
+#define PROB_VERY_LIKELY	(REG_BR_PROB_BASE - PROB_VERY_UNLIKELY)
+#define PROB_ALWAYS		(REG_BR_PROB_BASE)
Seems like this should factor out rather than get duplicated.


--- gcc/gimple-fold.c.mp	2013-11-27 08:46:27.979629191 +0100
+++ gcc/gimple-fold.c	2013-11-27 08:46:57.556753251 +0100
@@ -2660,8 +2660,30 @@ gimple_fold_stmt_to_constant_1 (gimple s
  	tree fn;

  	if (gimple_call_internal_p (stmt))
-	  /* No folding yet for these functions.  */
-	  return NULL_TREE;
+	  {
+	    enum tree_code subcode = ERROR_MARK;
+	    switch (gimple_call_internal_fn (stmt))
+	      {
+	      case IFN_UBSAN_CHECK_ADD: subcode = PLUS_EXPR; break;
+	      case IFN_UBSAN_CHECK_SUB: subcode = MINUS_EXPR; break;
+	      case IFN_UBSAN_CHECK_MUL: subcode = MULT_EXPR; break;
Minor detail, put the case value and associated codes on separate lines.

  case FU:
    code;
    more code
    break;
  case BAR
    blah;
    break;

--- gcc/internal-fn.c.mp	2013-11-27 08:46:28.014629338 +0100
+++ gcc/internal-fn.c	2013-11-27 08:46:57.559753263 +0100
@@ -31,6 +31,8 @@ along with GCC; see the file COPYING3.
  #include "gimple-expr.h"
  #include "is-a.h"
  #include "gimple.h"
+#include "ubsan.h"
+#include "target.h"

  /* The names of each internal function, indexed by function number.  */
  const char *const internal_fn_name_array[] = {
@@ -153,6 +155,306 @@ expand_UBSAN_NULL (gimple stmt ATTRIBUTE
    gcc_unreachable ();
  }

+/* Add sub/add overflow checking to the statement STMT.
+   CODE says whether the operation is +, or -.  */
+
+void
+ubsan_expand_si_overflow_addsub_check (tree_code code, gimple stmt)
Does this need to use Jakub's recent changes for pr58864 (pending stack adjustment stuff). The code seems to have the same kind of structure Jakub was cleaning up already.


+
+/* Add negate overflow checking to the statement STMT.  */
+
+void
+ubsan_expand_si_overflow_neg_check (gimple stmt)
Similarly.



+/* Add mul overflow checking to the statement STMT.  */
+
+void
+ubsan_expand_si_overflow_mul_check (gimple stmt)
Similarly.


--- gcc/fold-const.c.mp	2013-11-27 08:46:27.941629031 +0100
+++ gcc/fold-const.c	2013-11-27 08:46:57.548753215 +0100
@@ -10335,14 +10335,16 @@ fold_binary_loc (location_t loc,

      case PLUS_EXPR:
        /* A + (-B) -> A - B */
-      if (TREE_CODE (arg1) == NEGATE_EXPR)
+      if (TREE_CODE (arg1) == NEGATE_EXPR
+	  && (flag_sanitize & SANITIZE_SI_OVERFLOW) == 0)
  	return fold_build2_loc (loc, MINUS_EXPR, type,
  			    fold_convert_loc (loc, type, arg0),
  			    fold_convert_loc (loc, type,
  					      TREE_OPERAND (arg1, 0)));
        /* (-A) + B -> B - A */
        if (TREE_CODE (arg0) == NEGATE_EXPR
-	  && reorder_operands_p (TREE_OPERAND (arg0, 0), arg1))
+	  && reorder_operands_p (TREE_OPERAND (arg0, 0), arg1)
+	  && (flag_sanitize & SANITIZE_SI_OVERFLOW) == 0)
  	return fold_build2_loc (loc, MINUS_EXPR, type,
  			    fold_convert_loc (loc, type, arg1),
  			    fold_convert_loc (loc, type,
Presumably because you want to see the original arithmetic to instrument it?

--- gcc/tree-vrp.c.mp	2013-11-27 08:46:28.043629459 +0100
+++ gcc/tree-vrp.c	2013-11-27 08:46:57.570753307 +0100
@@ -3757,6 +3757,40 @@ extract_range_basic (value_range_t *vr,
  	  break;
  	}
      }
+  else if (is_gimple_call (stmt)
+	   && gimple_call_internal_p (stmt))
+    {
+      enum tree_code subcode = ERROR_MARK;
+      switch (gimple_call_internal_fn (stmt))
+	{
+	case IFN_UBSAN_CHECK_ADD: subcode = PLUS_EXPR; break;
+	case IFN_UBSAN_CHECK_SUB: subcode = MINUS_EXPR; break;
+	case IFN_UBSAN_CHECK_MUL: subcode = MULT_EXPR; break;
+	default: break;
Formatting again.


Overall the stuff outside the i386 directory looks pretty good, though it needs some minor updates. I'd suggest extracting the i386 bits and pinging them as a separate patch in the hope that we'll get Uros's attention.

Jeff


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]