public inbox for gcc-patches@gcc.gnu.org
 help / color / mirror / Atom feed
* [PATCH] Implement -fsanitize=float-divide-by-zero
@ 2014-04-28 19:10 Marek Polacek
  2014-04-28 20:18 ` Marc Glisse
  0 siblings, 1 reply; 8+ messages in thread
From: Marek Polacek @ 2014-04-28 19:10 UTC (permalink / raw)
  To: GCC Patches; +Cc: Jakub Jelinek, Joseph S. Myers

This patch implements -fsanitize=float-divide-by-zero option that can
be used to detect division by zero even when using floating types.
Most of the code in ubsan_instrument_division was ready for this
so this was mainly about handling REAL_TYPE there. 

Since division by a floating point zero can be a valid way of
obtaining infinities and NaNs, I'm not 100% sure this ought to be
enabled by default (that is, enabled when -fsanitize=undefined is
specified).

Regtested/bootstrapped/ran bootstrap-ubsan on x86_64-linux, ok for
trunk?

2014-04-28  Marek Polacek  <polacek@redhat.com>

	* flag-types.h (enum sanitize_code): Add SANITIZE_FLOAT_DIVIDE and
	or it into SANITIZE_UNDEFINED.
	* opts.c (common_handle_option): Add -fsanitize=float-divide-by-zero.
c-family/
	* c-ubsan.c (ubsan_instrument_division): Handle REAL_TYPEs.  Perform
	INT_MIN / -1 sanitization only for integer types.
c/
	* c-typeck.c (build_binary_op): Call ubsan_instrument_division
	also when SANITIZE_FLOAT_DIVIDE is on.
cp/
	* typeck.c (cp_build_binary_op): Call ubsan_instrument_division
	even when SANITIZE_FLOAT_DIVIDE is on.  Set doing_div_or_mod even
	for non-integer types.
testsuite/
	* c-c++-common/ubsan/div-by-zero-5.c: Fix formatting.
	* c-c++-common/ubsan/float-div-by-zero-1.c: New test.

diff --git gcc/c-family/c-ubsan.c gcc/c-family/c-ubsan.c
index e4f6f32..a039792 100644
--- gcc/c-family/c-ubsan.c
+++ gcc/c-family/c-ubsan.c
@@ -46,15 +46,21 @@ ubsan_instrument_division (location_t loc, tree op0, tree op1)
   gcc_assert (TYPE_MAIN_VARIANT (TREE_TYPE (op0))
 	      == TYPE_MAIN_VARIANT (TREE_TYPE (op1)));
 
-  /* TODO: REAL_TYPE is not supported yet.  */
-  if (TREE_CODE (type) != INTEGER_TYPE)
+  if (TREE_CODE (type) == INTEGER_TYPE
+      && (flag_sanitize & SANITIZE_DIVIDE))
+    t = fold_build2 (EQ_EXPR, boolean_type_node,
+		     op1, build_int_cst (type, 0));
+  else if (TREE_CODE (type) == REAL_TYPE
+	   && (flag_sanitize & SANITIZE_FLOAT_DIVIDE))
+    t = fold_build2 (EQ_EXPR, boolean_type_node,
+		     op1, build_real (type, dconst0));
+  else
     return NULL_TREE;
 
-  t = fold_build2 (EQ_EXPR, boolean_type_node,
-		    op1, build_int_cst (type, 0));
-
   /* We check INT_MIN / -1 only for signed types.  */
-  if (!TYPE_UNSIGNED (type))
+  if (TREE_CODE (type) == INTEGER_TYPE
+      && (flag_sanitize & SANITIZE_DIVIDE)
+      && !TYPE_UNSIGNED (type))
     {
       tree x;
       tt = fold_build2 (EQ_EXPR, boolean_type_node, op1,
diff --git gcc/c/c-typeck.c gcc/c/c-typeck.c
index 62c72df..8df544b 100644
--- gcc/c/c-typeck.c
+++ gcc/c/c-typeck.c
@@ -10995,7 +10995,8 @@ build_binary_op (location_t location, enum tree_code code,
 	return error_mark_node;
     }
 
-  if ((flag_sanitize & (SANITIZE_SHIFT | SANITIZE_DIVIDE))
+  if ((flag_sanitize & (SANITIZE_SHIFT | SANITIZE_DIVIDE
+			| SANITIZE_FLOAT_DIVIDE))
       && current_function_decl != 0
       && !lookup_attribute ("no_sanitize_undefined",
 			    DECL_ATTRIBUTES (current_function_decl))
@@ -11006,7 +11007,8 @@ build_binary_op (location_t location, enum tree_code code,
       op1 = c_save_expr (op1);
       op0 = c_fully_fold (op0, false, NULL);
       op1 = c_fully_fold (op1, false, NULL);
-      if (doing_div_or_mod && (flag_sanitize & SANITIZE_DIVIDE))
+      if (doing_div_or_mod && (flag_sanitize & (SANITIZE_DIVIDE
+						| SANITIZE_FLOAT_DIVIDE)))
 	instrument_expr = ubsan_instrument_division (location, op0, op1);
       else if (doing_shift && (flag_sanitize & SANITIZE_SHIFT))
 	instrument_expr = ubsan_instrument_shift (location, code, op0, op1);
diff --git gcc/cp/typeck.c gcc/cp/typeck.c
index 9a80727..99b4ce6 100644
--- gcc/cp/typeck.c
+++ gcc/cp/typeck.c
@@ -4112,10 +4112,7 @@ cp_build_binary_op (location_t location,
 	  enum tree_code tcode0 = code0, tcode1 = code1;
 	  tree cop1 = fold_non_dependent_expr_sfinae (op1, tf_none);
 	  cop1 = maybe_constant_value (cop1);
-
-	  if (tcode0 == INTEGER_TYPE)
-	    doing_div_or_mod = true;
-
+	  doing_div_or_mod = true;
 	  warn_for_div_by_zero (location, cop1);
 
 	  if (tcode0 == COMPLEX_TYPE || tcode0 == VECTOR_TYPE)
@@ -4155,9 +4152,7 @@ cp_build_binary_op (location_t location,
       {
 	tree cop1 = fold_non_dependent_expr_sfinae (op1, tf_none);
 	cop1 = maybe_constant_value (cop1);
-
-	if (code0 == INTEGER_TYPE)
-	  doing_div_or_mod = true;
+	doing_div_or_mod = true;
 	warn_for_div_by_zero (location, cop1);
       }
 
@@ -4904,7 +4899,8 @@ cp_build_binary_op (location_t location,
   if (build_type == NULL_TREE)
     build_type = result_type;
 
-  if ((flag_sanitize & (SANITIZE_SHIFT | SANITIZE_DIVIDE))
+  if ((flag_sanitize & (SANITIZE_SHIFT | SANITIZE_DIVIDE
+			| SANITIZE_FLOAT_DIVIDE))
       && !processing_template_decl
       && current_function_decl != 0
       && !lookup_attribute ("no_sanitize_undefined",
@@ -4918,7 +4914,8 @@ cp_build_binary_op (location_t location,
 								  tf_none));
       op1 = maybe_constant_value (fold_non_dependent_expr_sfinae (op1,
 								  tf_none));
-      if (doing_div_or_mod && (flag_sanitize & SANITIZE_DIVIDE))
+      if (doing_div_or_mod && (flag_sanitize & (SANITIZE_DIVIDE
+						| SANITIZE_FLOAT_DIVIDE)))
 	{
 	  /* For diagnostics we want to use the promoted types without
 	     shorten_binary_op.  So convert the arguments to the
diff --git gcc/flag-types.h gcc/flag-types.h
index fc3261b..525b3a5 100644
--- gcc/flag-types.h
+++ gcc/flag-types.h
@@ -228,9 +228,11 @@ enum sanitize_code {
   SANITIZE_SI_OVERFLOW = 1 << 9,
   SANITIZE_BOOL = 1 << 10,
   SANITIZE_ENUM = 1 << 11,
+  SANITIZE_FLOAT_DIVIDE = 1 << 12,
   SANITIZE_UNDEFINED = SANITIZE_SHIFT | SANITIZE_DIVIDE | SANITIZE_UNREACHABLE
 		       | SANITIZE_VLA | SANITIZE_NULL | SANITIZE_RETURN
 		       | SANITIZE_SI_OVERFLOW | SANITIZE_BOOL | SANITIZE_ENUM
+		       | SANITIZE_FLOAT_DIVIDE
 };
 
 /* flag_vtable_verify initialization levels. */
diff --git gcc/opts.c gcc/opts.c
index 1873b96..3c214f0 100644
--- gcc/opts.c
+++ gcc/opts.c
@@ -1461,6 +1461,8 @@ common_handle_option (struct gcc_options *opts,
 		sizeof "signed-integer-overflow" -1 },
 	      { "bool", SANITIZE_BOOL, sizeof "bool" - 1 },
 	      { "enum", SANITIZE_ENUM, sizeof "enum" - 1 },
+	      { "float-divide-by-zero", SANITIZE_FLOAT_DIVIDE,
+		sizeof "float-divide-by-zero" - 1 },
 	      { NULL, 0, 0 }
 	    };
 	    const char *comma;
diff --git gcc/testsuite/c-c++-common/ubsan/div-by-zero-5.c gcc/testsuite/c-c++-common/ubsan/div-by-zero-5.c
index 7a28bac..bb391c5 100644
--- gcc/testsuite/c-c++-common/ubsan/div-by-zero-5.c
+++ gcc/testsuite/c-c++-common/ubsan/div-by-zero-5.c
@@ -1,4 +1,4 @@
-/* { dg-do compile} */
+/* { dg-do compile } */
 /* { dg-options "-fsanitize=integer-divide-by-zero" } */
 
 void
diff --git gcc/testsuite/c-c++-common/ubsan/float-div-by-zero-1.c gcc/testsuite/c-c++-common/ubsan/float-div-by-zero-1.c
index e69de29..30bc090 100644
--- gcc/testsuite/c-c++-common/ubsan/float-div-by-zero-1.c
+++ gcc/testsuite/c-c++-common/ubsan/float-div-by-zero-1.c
@@ -0,0 +1,25 @@
+/* { dg-do run } */
+/* { dg-options "-fsanitize=float-divide-by-zero" } */
+
+int
+main (void)
+{
+  volatile float a = 1.3f;
+  volatile double b = 0.0;
+  volatile int c = 4;
+
+  a / b;
+  a / 0.0;
+  2.7f / b;
+  3.6 / (b = 0.0, b);
+  c / b;
+  b / c;
+
+  return 0;
+}
+
+/* { dg-output "division by zero\[^\n\r]*(\n|\r\n|\r)" } */
+/* { dg-output "\[^\n\r]*division by zero\[^\n\r]*(\n|\r\n|\r)" } */
+/* { dg-output "\[^\n\r]*division by zero\[^\n\r]*(\n|\r\n|\r)" } */
+/* { dg-output "\[^\n\r]*division by zero\[^\n\r]*(\n|\r\n|\r)" } */
+/* { dg-output "\[^\n\r]*division by zero\[^\n\r]*" } */

	Marek

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] Implement -fsanitize=float-divide-by-zero
  2014-04-28 19:10 [PATCH] Implement -fsanitize=float-divide-by-zero Marek Polacek
@ 2014-04-28 20:18 ` Marc Glisse
  0 siblings, 0 replies; 8+ messages in thread
From: Marc Glisse @ 2014-04-28 20:18 UTC (permalink / raw)
  To: Marek Polacek; +Cc: GCC Patches, Jakub Jelinek, Joseph S. Myers

On Mon, 28 Apr 2014, Marek Polacek wrote:

> This patch implements -fsanitize=float-divide-by-zero option that can
> be used to detect division by zero even when using floating types.
> Most of the code in ubsan_instrument_division was ready for this
> so this was mainly about handling REAL_TYPE there.

Ideally this would all be unneeded, you would compile your program with 
pragma stdc fenv_access on, on glibc you would call 
feenableexcept(FE_DIVBYZERO) at the beginning of the program, done (I may 
have listed the wrong function, I am always confused by those names).

But I guess we won't be there anytime soon, so in the mean time...

> Since division by a floating point zero can be a valid way of
> obtaining infinities and NaNs, I'm not 100% sure this ought to be
> enabled by default (that is, enabled when -fsanitize=undefined is
> specified).

Please don't enable it with "undefined". As you say, it is well defined 
(except when finite-math-only is in effect). If you want a meta-category 
for this kind of valid thing, maybe -fsanitize=unusual or 
-fsanitize=suspicious.

-- 
Marc Glisse

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] Implement -fsanitize=float-divide-by-zero
  2014-04-29  8:59 ` Jakub Jelinek
  2014-04-29  9:37   ` Marek Polacek
@ 2014-05-02 15:51   ` Joseph S. Myers
  1 sibling, 0 replies; 8+ messages in thread
From: Joseph S. Myers @ 2014-05-02 15:51 UTC (permalink / raw)
  To: Jakub Jelinek; +Cc: Tobias Burnus, Marc Glisse, Marek Polacek, GCC Patches

On Tue, 29 Apr 2014, Jakub Jelinek wrote:

> To my surprise, the wording in C99 or C++11 make even floating point
> division by zero undefined behavior, but I think generally at least for IEEE
> floating point semantics it is well defined, thus I think we shouldn't
> include it in -fsanitize=undefined.

Making Annex F explicitly take precedence over the generic text is DR#442.  
(This precedence issue applies to out-of-range conversions from floating 
point to integer as well as such things as floating-point division by 
zero: the generic text makes such out-of-range conversions undefined, but 
Annex F says they raise "invalid" and return an unspecified value.)

-- 
Joseph S. Myers
joseph@codesourcery.com

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] Implement -fsanitize=float-divide-by-zero
  2014-04-30  6:56     ` Jakub Jelinek
@ 2014-04-30  7:50       ` Marek Polacek
  0 siblings, 0 replies; 8+ messages in thread
From: Marek Polacek @ 2014-04-30  7:50 UTC (permalink / raw)
  To: Jakub Jelinek; +Cc: Tobias Burnus, Marc Glisse, GCC Patches, Joseph S. Myers

On Wed, Apr 30, 2014 at 08:55:10AM +0200, Jakub Jelinek wrote:
> Please assign the result of the divisions to some other volatile variables,
> otherwise I don't see why the compiler couldn't optimize them away all.
> 
> Otherwise looks good to me.

Done, thanks.

	Marek

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] Implement -fsanitize=float-divide-by-zero
  2014-04-29  9:37   ` Marek Polacek
@ 2014-04-30  6:56     ` Jakub Jelinek
  2014-04-30  7:50       ` Marek Polacek
  0 siblings, 1 reply; 8+ messages in thread
From: Jakub Jelinek @ 2014-04-30  6:56 UTC (permalink / raw)
  To: Marek Polacek; +Cc: Tobias Burnus, Marc Glisse, GCC Patches, Joseph S. Myers

On Tue, Apr 29, 2014 at 11:34:50AM +0200, Marek Polacek wrote:
> Ran ubsan testsuite (-m32/-m64) + bootstrap-ubsan on x86_64-linux, ok now?
> 
> 2014-04-29  Marek Polacek  <polacek@redhat.com>
> 
> 	* gcc.c (sanitize_spec_function): Handle SANITIZE_FLOAT_DIVIDE.
> 	* builtins.def: Initialize builtins even for SANITIZE_FLOAT_DIVIDE.
> 	* flag-types.h (enum sanitize_code): Add SANITIZE_FLOAT_DIVIDE.
> 	* opts.c (common_handle_option): Add -fsanitize=float-divide-by-zero.
> c-family/
> 	* c-ubsan.c (ubsan_instrument_division): Handle REAL_TYPEs.  Perform
> 	INT_MIN / -1 sanitization only for integer types.
> c/
> 	* c-typeck.c (build_binary_op): Call ubsan_instrument_division
> 	also when SANITIZE_FLOAT_DIVIDE is on.
> cp/
> 	* typeck.c (cp_build_binary_op): Call ubsan_instrument_division
> 	even when SANITIZE_FLOAT_DIVIDE is on.  Set doing_div_or_mod even
> 	for non-integer types.
> testsuite/
> 	* c-c++-common/ubsan/div-by-zero-5.c: Fix formatting.
> 	* c-c++-common/ubsan/float-div-by-zero-1.c: New test.

> +int
> +main (void)
> +{
> +  volatile float a = 1.3f;
> +  volatile double b = 0.0;
> +  volatile int c = 4;
> +
> +  a / b;
> +  a / 0.0;
> +  2.7f / b;
> +  3.6 / (b = 0.0, b);
> +  c / b;
> +  b / c;

Please assign the result of the divisions to some other volatile variables,
otherwise I don't see why the compiler couldn't optimize them away all.

Otherwise looks good to me.

	Jakub

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] Implement -fsanitize=float-divide-by-zero
  2014-04-29  8:59 ` Jakub Jelinek
@ 2014-04-29  9:37   ` Marek Polacek
  2014-04-30  6:56     ` Jakub Jelinek
  2014-05-02 15:51   ` Joseph S. Myers
  1 sibling, 1 reply; 8+ messages in thread
From: Marek Polacek @ 2014-04-29  9:37 UTC (permalink / raw)
  To: Jakub Jelinek; +Cc: Tobias Burnus, Marc Glisse, GCC Patches, Joseph S. Myers

On Tue, Apr 29, 2014 at 10:46:06AM +0200, Jakub Jelinek wrote:
> On Tue, Apr 29, 2014 at 10:27:58AM +0200, Tobias Burnus wrote:
> > Thus, I think Fortran users would also prefer not to have
> > -fsanitize=undefined implying trapping dividing by zero.
> > 
> > Thus, I wonder whether it wouldn't be more useful to provide a command-line option
> > to make the floating-point exceptions signalling - and to remind the users to link
> > libbacktrace to get the trace.
> 
> That can be done independently with a different option, but -fsanitize=...
> IMNSHO definitely shouldn't do anything with floating point control
> registers.
> 
> To my surprise, the wording in C99 or C++11 make even floating point
> division by zero undefined behavior, but I think generally at least for IEEE
> floating point semantics it is well defined, thus I think we shouldn't
> include it in -fsanitize=undefined.

Thanks for comments.  The following is thus implementation of
-fsanitize=float-divide-by-zero that is not enabled by default.  For
this to work well I had to uglify builtins.def and gcc.c a little bit
(hopefully this will be the sole ubsan option that is not included in
SANITIZE_UNDEFINED).

Ran ubsan testsuite (-m32/-m64) + bootstrap-ubsan on x86_64-linux, ok now?

2014-04-29  Marek Polacek  <polacek@redhat.com>

	* gcc.c (sanitize_spec_function): Handle SANITIZE_FLOAT_DIVIDE.
	* builtins.def: Initialize builtins even for SANITIZE_FLOAT_DIVIDE.
	* flag-types.h (enum sanitize_code): Add SANITIZE_FLOAT_DIVIDE.
	* opts.c (common_handle_option): Add -fsanitize=float-divide-by-zero.
c-family/
	* c-ubsan.c (ubsan_instrument_division): Handle REAL_TYPEs.  Perform
	INT_MIN / -1 sanitization only for integer types.
c/
	* c-typeck.c (build_binary_op): Call ubsan_instrument_division
	also when SANITIZE_FLOAT_DIVIDE is on.
cp/
	* typeck.c (cp_build_binary_op): Call ubsan_instrument_division
	even when SANITIZE_FLOAT_DIVIDE is on.  Set doing_div_or_mod even
	for non-integer types.
testsuite/
	* c-c++-common/ubsan/div-by-zero-5.c: Fix formatting.
	* c-c++-common/ubsan/float-div-by-zero-1.c: New test.

diff --git gcc/builtins.def gcc/builtins.def
index 5b902d8..d400ecb 100644
--- gcc/builtins.def
+++ gcc/builtins.def
@@ -176,7 +176,7 @@ along with GCC; see the file COPYING3.  If not see
   DEF_BUILTIN (ENUM, "__builtin_" NAME, BUILT_IN_NORMAL, TYPE, TYPE,    \
 	       true, true, true, ATTRS, true, \
 	      (flag_sanitize & (SANITIZE_ADDRESS | SANITIZE_THREAD \
-				| SANITIZE_UNDEFINED)))
+				| SANITIZE_UNDEFINED | SANITIZE_FLOAT_DIVIDE)))
 
 #undef DEF_CILKPLUS_BUILTIN
 #define DEF_CILKPLUS_BUILTIN(ENUM, NAME, TYPE, ATTRS)  \
diff --git gcc/c-family/c-ubsan.c gcc/c-family/c-ubsan.c
index e4f6f32..a039792 100644
--- gcc/c-family/c-ubsan.c
+++ gcc/c-family/c-ubsan.c
@@ -46,15 +46,21 @@ ubsan_instrument_division (location_t loc, tree op0, tree op1)
   gcc_assert (TYPE_MAIN_VARIANT (TREE_TYPE (op0))
 	      == TYPE_MAIN_VARIANT (TREE_TYPE (op1)));
 
-  /* TODO: REAL_TYPE is not supported yet.  */
-  if (TREE_CODE (type) != INTEGER_TYPE)
+  if (TREE_CODE (type) == INTEGER_TYPE
+      && (flag_sanitize & SANITIZE_DIVIDE))
+    t = fold_build2 (EQ_EXPR, boolean_type_node,
+		     op1, build_int_cst (type, 0));
+  else if (TREE_CODE (type) == REAL_TYPE
+	   && (flag_sanitize & SANITIZE_FLOAT_DIVIDE))
+    t = fold_build2 (EQ_EXPR, boolean_type_node,
+		     op1, build_real (type, dconst0));
+  else
     return NULL_TREE;
 
-  t = fold_build2 (EQ_EXPR, boolean_type_node,
-		    op1, build_int_cst (type, 0));
-
   /* We check INT_MIN / -1 only for signed types.  */
-  if (!TYPE_UNSIGNED (type))
+  if (TREE_CODE (type) == INTEGER_TYPE
+      && (flag_sanitize & SANITIZE_DIVIDE)
+      && !TYPE_UNSIGNED (type))
     {
       tree x;
       tt = fold_build2 (EQ_EXPR, boolean_type_node, op1,
diff --git gcc/c/c-typeck.c gcc/c/c-typeck.c
index 62c72df..8df544b 100644
--- gcc/c/c-typeck.c
+++ gcc/c/c-typeck.c
@@ -10995,7 +10995,8 @@ build_binary_op (location_t location, enum tree_code code,
 	return error_mark_node;
     }
 
-  if ((flag_sanitize & (SANITIZE_SHIFT | SANITIZE_DIVIDE))
+  if ((flag_sanitize & (SANITIZE_SHIFT | SANITIZE_DIVIDE
+			| SANITIZE_FLOAT_DIVIDE))
       && current_function_decl != 0
       && !lookup_attribute ("no_sanitize_undefined",
 			    DECL_ATTRIBUTES (current_function_decl))
@@ -11006,7 +11007,8 @@ build_binary_op (location_t location, enum tree_code code,
       op1 = c_save_expr (op1);
       op0 = c_fully_fold (op0, false, NULL);
       op1 = c_fully_fold (op1, false, NULL);
-      if (doing_div_or_mod && (flag_sanitize & SANITIZE_DIVIDE))
+      if (doing_div_or_mod && (flag_sanitize & (SANITIZE_DIVIDE
+						| SANITIZE_FLOAT_DIVIDE)))
 	instrument_expr = ubsan_instrument_division (location, op0, op1);
       else if (doing_shift && (flag_sanitize & SANITIZE_SHIFT))
 	instrument_expr = ubsan_instrument_shift (location, code, op0, op1);
diff --git gcc/cp/typeck.c gcc/cp/typeck.c
index 9a80727..99b4ce6 100644
--- gcc/cp/typeck.c
+++ gcc/cp/typeck.c
@@ -4112,10 +4112,7 @@ cp_build_binary_op (location_t location,
 	  enum tree_code tcode0 = code0, tcode1 = code1;
 	  tree cop1 = fold_non_dependent_expr_sfinae (op1, tf_none);
 	  cop1 = maybe_constant_value (cop1);
-
-	  if (tcode0 == INTEGER_TYPE)
-	    doing_div_or_mod = true;
-
+	  doing_div_or_mod = true;
 	  warn_for_div_by_zero (location, cop1);
 
 	  if (tcode0 == COMPLEX_TYPE || tcode0 == VECTOR_TYPE)
@@ -4155,9 +4152,7 @@ cp_build_binary_op (location_t location,
       {
 	tree cop1 = fold_non_dependent_expr_sfinae (op1, tf_none);
 	cop1 = maybe_constant_value (cop1);
-
-	if (code0 == INTEGER_TYPE)
-	  doing_div_or_mod = true;
+	doing_div_or_mod = true;
 	warn_for_div_by_zero (location, cop1);
       }
 
@@ -4904,7 +4899,8 @@ cp_build_binary_op (location_t location,
   if (build_type == NULL_TREE)
     build_type = result_type;
 
-  if ((flag_sanitize & (SANITIZE_SHIFT | SANITIZE_DIVIDE))
+  if ((flag_sanitize & (SANITIZE_SHIFT | SANITIZE_DIVIDE
+			| SANITIZE_FLOAT_DIVIDE))
       && !processing_template_decl
       && current_function_decl != 0
       && !lookup_attribute ("no_sanitize_undefined",
@@ -4918,7 +4914,8 @@ cp_build_binary_op (location_t location,
 								  tf_none));
       op1 = maybe_constant_value (fold_non_dependent_expr_sfinae (op1,
 								  tf_none));
-      if (doing_div_or_mod && (flag_sanitize & SANITIZE_DIVIDE))
+      if (doing_div_or_mod && (flag_sanitize & (SANITIZE_DIVIDE
+						| SANITIZE_FLOAT_DIVIDE)))
 	{
 	  /* For diagnostics we want to use the promoted types without
 	     shorten_binary_op.  So convert the arguments to the
diff --git gcc/flag-types.h gcc/flag-types.h
index fc3261b..caf4039 100644
--- gcc/flag-types.h
+++ gcc/flag-types.h
@@ -228,6 +228,7 @@ enum sanitize_code {
   SANITIZE_SI_OVERFLOW = 1 << 9,
   SANITIZE_BOOL = 1 << 10,
   SANITIZE_ENUM = 1 << 11,
+  SANITIZE_FLOAT_DIVIDE = 1 << 12,
   SANITIZE_UNDEFINED = SANITIZE_SHIFT | SANITIZE_DIVIDE | SANITIZE_UNREACHABLE
 		       | SANITIZE_VLA | SANITIZE_NULL | SANITIZE_RETURN
 		       | SANITIZE_SI_OVERFLOW | SANITIZE_BOOL | SANITIZE_ENUM
diff --git gcc/gcc.c gcc/gcc.c
index e5130d1..7bea6d7 100644
--- gcc/gcc.c
+++ gcc/gcc.c
@@ -8170,7 +8170,7 @@ sanitize_spec_function (int argc, const char **argv)
   if (strcmp (argv[0], "thread") == 0)
     return (flag_sanitize & SANITIZE_THREAD) ? "" : NULL;
   if (strcmp (argv[0], "undefined") == 0)
-    return ((flag_sanitize & SANITIZE_UNDEFINED)
+    return ((flag_sanitize & (SANITIZE_UNDEFINED | SANITIZE_FLOAT_DIVIDE))
 	    && !flag_sanitize_undefined_trap_on_error) ? "" : NULL;
   if (strcmp (argv[0], "leak") == 0)
     return ((flag_sanitize
diff --git gcc/opts.c gcc/opts.c
index 1873b96..3c214f0 100644
--- gcc/opts.c
+++ gcc/opts.c
@@ -1461,6 +1461,8 @@ common_handle_option (struct gcc_options *opts,
 		sizeof "signed-integer-overflow" -1 },
 	      { "bool", SANITIZE_BOOL, sizeof "bool" - 1 },
 	      { "enum", SANITIZE_ENUM, sizeof "enum" - 1 },
+	      { "float-divide-by-zero", SANITIZE_FLOAT_DIVIDE,
+		sizeof "float-divide-by-zero" - 1 },
 	      { NULL, 0, 0 }
 	    };
 	    const char *comma;
diff --git gcc/testsuite/c-c++-common/ubsan/div-by-zero-5.c gcc/testsuite/c-c++-common/ubsan/div-by-zero-5.c
index 7a28bac..bb391c5 100644
--- gcc/testsuite/c-c++-common/ubsan/div-by-zero-5.c
+++ gcc/testsuite/c-c++-common/ubsan/div-by-zero-5.c
@@ -1,4 +1,4 @@
-/* { dg-do compile} */
+/* { dg-do compile } */
 /* { dg-options "-fsanitize=integer-divide-by-zero" } */
 
 void
diff --git gcc/testsuite/c-c++-common/ubsan/float-div-by-zero-1.c gcc/testsuite/c-c++-common/ubsan/float-div-by-zero-1.c
index e69de29..30bc090 100644
--- gcc/testsuite/c-c++-common/ubsan/float-div-by-zero-1.c
+++ gcc/testsuite/c-c++-common/ubsan/float-div-by-zero-1.c
@@ -0,0 +1,25 @@
+/* { dg-do run } */
+/* { dg-options "-fsanitize=float-divide-by-zero" } */
+
+int
+main (void)
+{
+  volatile float a = 1.3f;
+  volatile double b = 0.0;
+  volatile int c = 4;
+
+  a / b;
+  a / 0.0;
+  2.7f / b;
+  3.6 / (b = 0.0, b);
+  c / b;
+  b / c;
+
+  return 0;
+}
+
+/* { dg-output "division by zero\[^\n\r]*(\n|\r\n|\r)" } */
+/* { dg-output "\[^\n\r]*division by zero\[^\n\r]*(\n|\r\n|\r)" } */
+/* { dg-output "\[^\n\r]*division by zero\[^\n\r]*(\n|\r\n|\r)" } */
+/* { dg-output "\[^\n\r]*division by zero\[^\n\r]*(\n|\r\n|\r)" } */
+/* { dg-output "\[^\n\r]*division by zero\[^\n\r]*" } */

	Marek

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] Implement -fsanitize=float-divide-by-zero
  2014-04-29  8:37 Tobias Burnus
@ 2014-04-29  8:59 ` Jakub Jelinek
  2014-04-29  9:37   ` Marek Polacek
  2014-05-02 15:51   ` Joseph S. Myers
  0 siblings, 2 replies; 8+ messages in thread
From: Jakub Jelinek @ 2014-04-29  8:59 UTC (permalink / raw)
  To: Tobias Burnus; +Cc: Marc Glisse, Marek Polacek, GCC Patches, Joseph S. Myers

On Tue, Apr 29, 2014 at 10:27:58AM +0200, Tobias Burnus wrote:
> Thus, I think Fortran users would also prefer not to have
> -fsanitize=undefined implying trapping dividing by zero.
> 
> Thus, I wonder whether it wouldn't be more useful to provide a command-line option
> to make the floating-point exceptions signalling - and to remind the users to link
> libbacktrace to get the trace.

That can be done independently with a different option, but -fsanitize=...
IMNSHO definitely shouldn't do anything with floating point control
registers.

To my surprise, the wording in C99 or C++11 make even floating point
division by zero undefined behavior, but I think generally at least for IEEE
floating point semantics it is well defined, thus I think we shouldn't
include it in -fsanitize=undefined.

	Jakub

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] Implement -fsanitize=float-divide-by-zero
@ 2014-04-29  8:37 Tobias Burnus
  2014-04-29  8:59 ` Jakub Jelinek
  0 siblings, 1 reply; 8+ messages in thread
From: Tobias Burnus @ 2014-04-29  8:37 UTC (permalink / raw)
  To: Marc Glisse, Marek Polacek, GCC Patches, Jakub Jelinek, Joseph S. Myers

Marc Glisse wrote:
> On Mon, 28 Apr 2014, Marek Polacek wrote:
>
> > This patch implements -fsanitize=float-divide-by-zero option that can
> > be used to detect division by zero even when using floating types.
> > Most of the code in ubsan_instrument_division was ready for this
> > so this was mainly about handling REAL_TYPE there.
>
> Ideally this would all be unneeded, you would compile your program with
> pragma stdc fenv_access on, on glibc you would call feenableexcept(FE_DIVBYZERO)
> at the beginning of the program, done (I may have listed the wrong function,
> I am always confused by those names).
>
>
> But I guess we won't be there anytime soon, so in the mean time...

I think there is difference between enabling the detection of division by zero
or invalid in the command line and doing so in the source code. The command line
option will be useful for detecting problems with the code while the source code
version (using fenv.h on C/C++ and the IEEE modules in Fortran) is useful for
handling exceptions explicitly.


gfortran users would simply use:

$ gfortran -ffpe-trap=zero -g  foo.f90  && ./a.out
Program received signal SIGFPE: Floating-point exception - erroneous arithmetic operation.

Backtrace for this error:
#0  0x7F8FF0C103F7
#1  0x7F8FF0C10A0E
#2  0x3C4FA3299F
#3  0x40070D in MAIN__ at foo.f90:3
Floating point exception

Thus, I think Fortran users would also prefer not to have
-fsanitize=undefined implying trapping dividing by zero.


In addition, having trapping enabled in the CPU should generate faster code
than having the additional checks explicitly.

The biggest advantage of having some -f* option is that one does not need to
modify the source code and that it provides some backtracing. (Actually, UBSAN
doesn't show a trace - but just the file and line; that's not very helpful if
it triggers in a very often called inline function in a header file.)

Thus, I wonder whether it wouldn't be more useful to provide a command-line option
to make the floating-point exceptions signalling - and to remind the users to link
libbacktrace to get the trace.

Tobias

PS: gfortran uses libgfortran/config/fpu-*.c to enable the trapping, with versions
for sysv (solaris/FreeBSD), glibc, aix and x86*. It currently uses a home-grown
backtrace support instead of libbacktrace, cf. PR fortran/54572.

^ permalink raw reply	[flat|nested] 8+ messages in thread

end of thread, other threads:[~2014-05-02 15:51 UTC | newest]

Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2014-04-28 19:10 [PATCH] Implement -fsanitize=float-divide-by-zero Marek Polacek
2014-04-28 20:18 ` Marc Glisse
2014-04-29  8:37 Tobias Burnus
2014-04-29  8:59 ` Jakub Jelinek
2014-04-29  9:37   ` Marek Polacek
2014-04-30  6:56     ` Jakub Jelinek
2014-04-30  7:50       ` Marek Polacek
2014-05-02 15:51   ` Joseph S. Myers

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).