public inbox for gcc-patches@gcc.gnu.org
 help / color / mirror / Atom feed
* 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
* [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

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-29  8:37 [PATCH] Implement -fsanitize=float-divide-by-zero 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
  -- strict thread matches above, loose matches on Subject: below --
2014-04-28 19:10 Marek Polacek
2014-04-28 20:18 ` Marc Glisse

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