diff mbox

Go patch committed: Add explicit checks for division zero and overflow

Message ID mcrd3728ay8.fsf@dhcp-172-18-216-180.mtv.corp.google.com
State New
Headers show

Commit Message

Ian Lance Taylor April 20, 2012, 7:22 p.m. UTC
This patch to the Go compiler and runtime adds explicit checks for
division by zero and for division overflow (INT_MIN / -1).

In Go, a division by zero is supposed to produce a panic; that did not
happen on processors like PPC that do not issue a SIGFPE signal on
division by zero.  This patch fixes that problem.

In Go, a division overflow is supposed to wrap.  On x86 processors it
would raise a SIGFPE signal leading to a panic.  This patch fixes that
problem as well.

This patch introduces new two command line options to control this
behaviour.  Currently the compiler will always generate these checks.
It would be better to only generate the checks for processors where they
are needed.  That would require somehow communicating this fact to the
driver or the frontend.  That is something for the future.

Bootstrapped and ran Go testsuite on x86_64-unknown-linux-gnu and
powerpc64-unknown-linux-gnu.  Committed to mainline and 4.7 branch.

Ian


2012-04-20  Ian Lance Taylor  <iant@google.com>

	* lang.opt: Add -fgo-check-divide-zero and
	-fgo-check-divide-overflow.
	* gccgo.texi (Invoking gccgo): Document new options.

Comments

Joseph Myers April 24, 2012, 9:38 p.m. UTC | #1
On Fri, 20 Apr 2012, Ian Lance Taylor wrote:

> In Go, a division overflow is supposed to wrap.  On x86 processors it
> would raise a SIGFPE signal leading to a panic.  This patch fixes that
> problem as well.

It's a pity that this change is Go-specific.  For -fwrapv, INT_MIN / -1 
and INT_MIN % -1 should both have defined modulo semantics for C and C++ 
(similarly of course for all signed integer types, not just int).  Ideally 
of course the semantics for this would be encoded properly in GENERIC / 
GIMPLE, but until then it would make sense to have a global flag, set by 
both -fwrapv and the Go front end, to enable those checks in the 
language-independent compiler.  See bug 30484.
diff mbox

Patch

Index: gcc/go/lang.opt
===================================================================
--- gcc/go/lang.opt	(revision 184521)
+++ gcc/go/lang.opt	(working copy)
@@ -1,6 +1,6 @@ 
 ; lang.opt -- Options for the gcc Go front end.
 
-; Copyright (C) 2009, 2010, 2011 Free Software Foundation, Inc.
+; Copyright (C) 2009, 2010, 2011, 2012 Free Software Foundation, Inc.
 ;
 ; This file is part of GCC.
 ;
@@ -37,6 +37,14 @@  Wall
 Go
 ; Documented in c.opt
 
+fgo-check-divide-zero
+Go Var(go_check_divide_zero) Init(1)
+Add explicit checks for division by zero
+
+fgo-check-divide-overflow
+Go Var(go_check_divide_overflow) Init(1)
+Add explicit checks for division overflow in INT_MIN / -1
+
 fgo-dump-
 Go Joined RejectNegative
 -fgo-dump-<type>	Dump Go frontend internal information
Index: gcc/go/gccgo.texi
===================================================================
--- gcc/go/gccgo.texi	(revision 184521)
+++ gcc/go/gccgo.texi	(working copy)
@@ -12,7 +12,7 @@ 
 @include gcc-common.texi
 
 @c Copyright years for this manual.
-@set copyrights-go 2010
+@set copyrights-go 2010, 2011, 2012
 
 @copying
 @c man begin COPYRIGHT
@@ -174,6 +174,31 @@  By default @command{gccgo} will warn abo
 more return parameters but lack an explicit @code{return} statement.
 This warning may be disabled using
 @option{-fno-require-return-statement}.
+
+@item -fgo-check-divide-zero
+@cindex @option{-fgo-check-divide-zero}
+@cindex @option{-fno-go-check-divide-zero}
+Add explicit checks for division by zero.  In Go a division (or
+modulos) by zero causes a panic.  On Unix systems this is detected in
+the runtime by catching the @code{SIGFPE} signal.  Some processors,
+such as PowerPC, do not generate a SIGFPE on division by zero.  Some
+runtimes do not generate a signal that can be caught.  On those
+systems, this option may be used.  Or the checks may be removed via
+@option{-fno-go-check-divide-zero}.  This option is currently on by
+default, but in the future may be off by default on systems that do
+not require it.
+
+@item -fgo-check-divide-overflow
+@cindex @option{-fgo-check-divide-overflow}
+@cindex @option{-fno-go-check-divide-overflow}
+Add explicit checks for division overflow.  For example, division
+overflow occurs when computing @code{INT_MIN / -1}.  In Go this should
+be wrapped, to produce @code{INT_MIN}.  Some processors, such as x86,
+generate a trap on division overflow.  On those systems, this option
+may be used.  Or the checks may be removed via
+@option{-fno-go-check-divide-overflow}.  This option is currently on
+by default, but in the future may be off by default on systems that do
+not require it.
 @end table
 
 @c man end
Index: gcc/go/gofrontend/gogo.h
===================================================================
--- gcc/go/gofrontend/gogo.h	(revision 185080)
+++ gcc/go/gofrontend/gogo.h	(working copy)
@@ -2791,6 +2791,9 @@  static const int RUNTIME_ERROR_MAKE_MAP_
 // Channel capacity out of bounds in make: negative or overflow.
 static const int RUNTIME_ERROR_MAKE_CHAN_OUT_OF_BOUNDS = 9;
 
+// Division by zero.
+static const int RUNTIME_ERROR_DIVISION_BY_ZERO = 10;
+
 // This is used by some of the langhooks.
 extern Gogo* go_get_gogo();
 
Index: gcc/go/gofrontend/expressions.cc
===================================================================
--- gcc/go/gofrontend/expressions.cc	(revision 186511)
+++ gcc/go/gofrontend/expressions.cc	(working copy)
@@ -5647,6 +5647,7 @@  Binary_expression::do_get_tree(Translate
   enum tree_code code;
   bool use_left_type = true;
   bool is_shift_op = false;
+  bool is_idiv_op = false;
   switch (this->op_)
     {
     case OPERATOR_EQEQ:
@@ -5689,11 +5690,15 @@  Binary_expression::do_get_tree(Translate
 	if (t->float_type() != NULL || t->complex_type() != NULL)
 	  code = RDIV_EXPR;
 	else
-	  code = TRUNC_DIV_EXPR;
+	  {
+	    code = TRUNC_DIV_EXPR;
+	    is_idiv_op = true;
+	  }
       }
       break;
     case OPERATOR_MOD:
       code = TRUNC_MOD_EXPR;
+      is_idiv_op = true;
       break;
     case OPERATOR_LSHIFT:
       code = LSHIFT_EXPR;
@@ -5714,6 +5719,7 @@  Binary_expression::do_get_tree(Translate
       go_unreachable();
     }
 
+  location_t gccloc = this->location().gcc_location();
   tree type = use_left_type ? TREE_TYPE(left) : TREE_TYPE(right);
 
   if (this->left_->type()->is_string_type())
@@ -5741,28 +5747,27 @@  Binary_expression::do_get_tree(Translate
     }
 
   tree eval_saved = NULL_TREE;
-  if (is_shift_op)
+  if (is_shift_op
+      || (is_idiv_op && (go_check_divide_zero || go_check_divide_overflow)))
     {
       // Make sure the values are evaluated.
-      if (!DECL_P(left) && TREE_SIDE_EFFECTS(left))
+      if (!DECL_P(left))
 	{
 	  left = save_expr(left);
 	  eval_saved = left;
 	}
-      if (!DECL_P(right) && TREE_SIDE_EFFECTS(right))
+      if (!DECL_P(right))
 	{
 	  right = save_expr(right);
 	  if (eval_saved == NULL_TREE)
 	    eval_saved = right;
 	  else
-	    eval_saved = fold_build2_loc(this->location().gcc_location(),
-                                         COMPOUND_EXPR,
+	    eval_saved = fold_build2_loc(gccloc, COMPOUND_EXPR,
 					 void_type_node, eval_saved, right);
 	}
     }
 
-  tree ret = fold_build2_loc(this->location().gcc_location(),
-			     code,
+  tree ret = fold_build2_loc(gccloc, code,
 			     compute_type != NULL_TREE ? compute_type : type,
 			     left, right);
 
@@ -5780,39 +5785,116 @@  Binary_expression::do_get_tree(Translate
       tree compare = fold_build2(LT_EXPR, boolean_type_node, right,
 				 build_int_cst_type(TREE_TYPE(right), bits));
 
-      tree overflow_result = fold_convert_loc(this->location().gcc_location(),
-					      TREE_TYPE(left),
+      tree overflow_result = fold_convert_loc(gccloc, TREE_TYPE(left),
 					      integer_zero_node);
       if (this->op_ == OPERATOR_RSHIFT
 	  && !this->left_->type()->integer_type()->is_unsigned())
 	{
 	  tree neg =
-            fold_build2_loc(this->location().gcc_location(), LT_EXPR,
-                            boolean_type_node, left,
-                            fold_convert_loc(this->location().gcc_location(),
-                                             TREE_TYPE(left),
+            fold_build2_loc(gccloc, LT_EXPR, boolean_type_node,
+			    left,
+                            fold_convert_loc(gccloc, TREE_TYPE(left),
                                              integer_zero_node));
 	  tree neg_one =
-            fold_build2_loc(this->location().gcc_location(),
-                            MINUS_EXPR, TREE_TYPE(left),
-                            fold_convert_loc(this->location().gcc_location(),
-                                             TREE_TYPE(left),
+            fold_build2_loc(gccloc, MINUS_EXPR, TREE_TYPE(left),
+                            fold_convert_loc(gccloc, TREE_TYPE(left),
                                              integer_zero_node),
-                            fold_convert_loc(this->location().gcc_location(),
-                                             TREE_TYPE(left),
+                            fold_convert_loc(gccloc, TREE_TYPE(left),
                                              integer_one_node));
 	  overflow_result =
-            fold_build3_loc(this->location().gcc_location(), COND_EXPR,
-                            TREE_TYPE(left), neg, neg_one,
-                            overflow_result);
+            fold_build3_loc(gccloc, COND_EXPR, TREE_TYPE(left),
+			    neg, neg_one, overflow_result);
 	}
 
-      ret = fold_build3_loc(this->location().gcc_location(), COND_EXPR,
-                            TREE_TYPE(left), compare, ret, overflow_result);
+      ret = fold_build3_loc(gccloc, COND_EXPR, TREE_TYPE(left),
+			    compare, ret, overflow_result);
+
+      if (eval_saved != NULL_TREE)
+	ret = fold_build2_loc(gccloc, COMPOUND_EXPR, TREE_TYPE(ret),
+			      eval_saved, ret);
+    }
+
+  // Add checks for division by zero and division overflow as needed.
+  if (is_idiv_op)
+    {
+      if (go_check_divide_zero)
+	{
+	  // right == 0
+	  tree check = fold_build2_loc(gccloc, EQ_EXPR, boolean_type_node,
+				       right,
+				       fold_convert_loc(gccloc,
+							TREE_TYPE(right),
+							integer_zero_node));
+
+	  // __go_runtime_error(RUNTIME_ERROR_DIVISION_BY_ZERO), 0
+	  int errcode = RUNTIME_ERROR_DIVISION_BY_ZERO;
+	  tree panic = fold_build2_loc(gccloc, COMPOUND_EXPR, TREE_TYPE(ret),
+				       Gogo::runtime_error(errcode,
+							   this->location()),
+				       fold_convert_loc(gccloc, TREE_TYPE(ret),
+							integer_zero_node));
+
+	  // right == 0 ? (__go_runtime_error(...), 0) : ret
+	  ret = fold_build3_loc(gccloc, COND_EXPR, TREE_TYPE(ret),
+				check, panic, ret);
+	}
+
+      if (go_check_divide_overflow)
+	{
+	  // right == -1
+	  // FIXME: It would be nice to say that this test is expected
+	  // to return false.
+	  tree m1 = integer_minus_one_node;
+	  tree check = fold_build2_loc(gccloc, EQ_EXPR, boolean_type_node,
+				       right,
+				       fold_convert_loc(gccloc,
+							TREE_TYPE(right),
+							m1));
+
+	  tree overflow;
+	  if (TYPE_UNSIGNED(TREE_TYPE(ret)))
+	    {
+	      // An unsigned -1 is the largest possible number, so
+	      // dividing is always 1 or 0.
+	      tree cmp = fold_build2_loc(gccloc, EQ_EXPR, boolean_type_node,
+					 left, right);
+	      if (this->op_ == OPERATOR_DIV)
+		overflow = fold_build3_loc(gccloc, COND_EXPR, TREE_TYPE(ret),
+					   cmp,
+					   fold_convert_loc(gccloc,
+							    TREE_TYPE(ret),
+							    integer_one_node),
+					   fold_convert_loc(gccloc,
+							    TREE_TYPE(ret),
+							    integer_zero_node));
+	      else
+		overflow = fold_build3_loc(gccloc, COND_EXPR, TREE_TYPE(ret),
+					   cmp,
+					   fold_convert_loc(gccloc,
+							    TREE_TYPE(ret),
+							    integer_zero_node),
+					   left);
+	    }
+	  else
+	    {
+	      // Computing left / -1 is the same as computing - left,
+	      // which does not overflow since Go sets -fwrapv.
+	      if (this->op_ == OPERATOR_DIV)
+		overflow = fold_build1_loc(gccloc, NEGATE_EXPR, TREE_TYPE(left),
+					   left);
+	      else
+		overflow = integer_zero_node;
+	    }
+	  overflow = fold_convert_loc(gccloc, TREE_TYPE(ret), overflow);
+
+	  // right == -1 ? - left : ret
+	  ret = fold_build3_loc(gccloc, COND_EXPR, TREE_TYPE(ret),
+				check, overflow, ret);
+	}
 
       if (eval_saved != NULL_TREE)
-	ret = fold_build2_loc(this->location().gcc_location(), COMPOUND_EXPR,
-			      TREE_TYPE(ret), eval_saved, ret);
+	ret = fold_build2_loc(gccloc, COMPOUND_EXPR, TREE_TYPE(ret),
+			      eval_saved, ret);
     }
 
   return ret;
Index: libgo/runtime/go-runtime-error.c
===================================================================
--- libgo/runtime/go-runtime-error.c	(revision 184521)
+++ libgo/runtime/go-runtime-error.c	(working copy)
@@ -46,7 +46,10 @@  enum
   MAKE_MAP_OUT_OF_BOUNDS = 8,
 
   /* Channel capacity out of bounds in make: negative or overflow.  */
-  MAKE_CHAN_OUT_OF_BOUNDS = 9
+  MAKE_CHAN_OUT_OF_BOUNDS = 9,
+
+  /* Integer division by zero.  */
+  DIVISION_BY_ZERO = 10
 };
 
 extern void __go_runtime_error () __attribute__ ((noreturn));
@@ -78,6 +81,9 @@  __go_runtime_error (int i)
     case MAKE_CHAN_OUT_OF_BOUNDS:
       runtime_panicstring ("make chan len out of range");
 
+    case DIVISION_BY_ZERO:
+      runtime_panicstring ("integer divide by zero");
+
     default:
       runtime_panicstring ("unknown runtime error");
     }