diff mbox

Add FE_NOMASK_ENV return value test

Message ID 000001cf7f27$1cc0c040$564240c0$@com
State New
Headers show

Commit Message

Wilco June 3, 2014, 12:26 p.m. UTC 
> Joseph Myers wrote:
> On Mon, 2 Jun 2014, Wilco wrote:
> 
> > > > From: Joseph Myers wrote:
> > > > On Fri, 23 May 2014, Wilco wrote:
> > > >
> > > > > This patch rewrites feupdateenv to improve performance by avoiding
> > > > > unnecessary FPSCR reads/writes and to fix bug 16918
> > > > > (https://sourceware.org/bugzilla/show_bug.cgi?id=16918).
> > > >
> > > > It would be desirable to add an architecture-independent testcase that, if
> > > > FE_NOMASK_ENV is defined and feupdateenv (FE_NOMASK_ENV) succeeds, the
> > > > exceptions are then enabled as indicated by fegetexcept (or by getting
> > > > SIGFPE, as test-fenv.c tests various cases, but a test using fegetexcept
> > > > is simpler to write).  (It's best for this to be a new test rather than
> > > > adding to the things in test-fenv.c.)
> >
> > This patch adds a new test to ensure the return values of fesetenv,
> > feupdateenv and feenbleexcept are correct when FE_NOMASK_ENV is not
> > supported. This ensures BZ16918/BZ17009 remain fixed.
> >
> > OK?
> 
> This test doesn't look correct.  You have
> 
> > +  if (!EXCEPTION_ENABLE_SUPPORTED (FE_ALL_EXCEPT))
> > +    {
> > +      if (res == -1)
> > +	{
> > +	  puts ("feenableexcept (FE_ALL_EXCEPT) not supported, cannot test.");
> > +	  return;
> > +	}
> > +      else
> > +	{
> > +	  printf ("feenableexcept incorrect return value %d\n", res);
> > +	  count_errors++;
> > +	}
> 
> but !EXCEPTION_ENABLE_SUPPORTED simply means enabling traps *might or
> might not* be supported at runtime.  Thus, if feenableexcept returns
> success (some value other than -1), this should not make the test fail; it
> should simply continue on to the test with fegetexcept of whether the
> exceptions did in fact get enabled.  The same applies to the tests of the
> other two functions.  With the existing patch, I'd expect the test to fail
> when run on ARM implementations that support exception traps.

Yes, the test was too strict for trapping implementations, I've updated it.

Wilco
---
 math/Makefile           |    2 +-
 math/test-fenv-return.c |  123 +++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 124 insertions(+), 1 deletion(-)
 create mode 100644 math/test-fenv-return.c

Comments

Joseph Myers June 3, 2014, 4:37 p.m. UTC | #1 
On Tue, 3 Jun 2014, Wilco wrote:

> Yes, the test was too strict for trapping implementations, I've updated it.

This version is OK with the comment at the top of the test fixed to say 
what it actually tests, rather than being copied from a test for a 
different bug.
diff mbox

Patch

diff --git a/math/Makefile b/math/Makefile
index f337a0e..cd59ce5 100644
--- a/math/Makefile
+++ b/math/Makefile
@@ -90,7 +90,7 @@  tests = test-matherr test-fenv atest-exp atest-sincos atest-exp2 basic-test \
 	test-misc test-fpucw test-fpucw-ieee tst-definitions test-tgmath \
 	test-tgmath-ret bug-nextafter bug-nexttoward bug-tgmath1 \
 	test-tgmath-int test-tgmath2 test-powl tst-CMPLX tst-CMPLX2 test-snan \
-	test-fenv-tls test-fenv-preserve $(tests-static)
+	test-fenv-tls test-fenv-preserve test-fenv-return $(tests-static)
 tests-static = test-fpucw-static test-fpucw-ieee-static
 # We do the `long double' tests only if this data type is available and
 # distinct from `double'.
diff --git a/math/test-fenv-return.c b/math/test-fenv-return.c
new file mode 100644
index 0000000..60f85be
--- /dev/null
+++ b/math/test-fenv-return.c
@@ -0,0 +1,123 @@ 
+/* Test fegetenv preserves exception mask (bug 16198).
+   Copyright (C) 2014 Free Software Foundation, Inc.
+   This file is part of the GNU C Library.
+
+   The GNU C Library is free software; you can redistribute it and/or
+   modify it under the terms of the GNU Lesser General Public
+   License as published by the Free Software Foundation; either
+   version 2.1 of the License, or (at your option) any later version.
+
+   The GNU C Library is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+   Lesser General Public License for more details.
+
+   You should have received a copy of the GNU Lesser General Public
+   License along with the GNU C Library; if not, see
+   <http://www.gnu.org/licenses/>.  */
+
+#include <fenv.h>
+#include <stdio.h>
+#include <math-tests.h>
+
+static int count_errors;
+
+static void
+test_feenableexcept (void)
+{
+#if defined FE_ALL_EXCEPT
+  int res;
+
+  fedisableexcept (FE_ALL_EXCEPT);
+
+  res = feenableexcept (FE_ALL_EXCEPT);
+
+  if (!EXCEPTION_ENABLE_SUPPORTED (FE_ALL_EXCEPT) && (res == -1))
+    {
+      puts ("feenableexcept (FE_ALL_EXCEPT) not supported, cannot test.");
+      return;
+    }
+  else if (res != 0)
+    {
+      puts ("feenableexcept (FE_ALL_EXCEPT) failed");
+      count_errors++;
+    }
+
+  if (fegetexcept () != FE_ALL_EXCEPT)
+    {
+      puts ("feenableexcept did not set all exceptions");
+      count_errors++;
+    }
+#endif
+}
+
+static void
+test_fesetenv (void)
+{
+#if defined FE_NOMASK_ENV && defined FE_ALL_EXCEPT
+  int res;
+
+  fedisableexcept (FE_ALL_EXCEPT);
+
+  res = fesetenv (FE_NOMASK_ENV);
+
+  if (!EXCEPTION_ENABLE_SUPPORTED (FE_ALL_EXCEPT) && (res != 0))
+    {
+      puts ("fesetenv (FE_NOMASK_ENV) not supported, cannot test.");
+      return;
+    }
+  else if (res != 0)
+    {
+      puts ("fesetenv (FE_NOMASK_ENV) failed");
+      count_errors++;
+    }
+
+  if (fegetexcept () != FE_ALL_EXCEPT)
+    {
+      puts ("fesetenv did not set all exceptions");
+      count_errors++;
+    }
+#endif
+}
+
+static void
+test_feupdateenv (void)
+{
+#if defined FE_NOMASK_ENV && defined FE_ALL_EXCEPT
+  int res;
+
+  fedisableexcept (FE_ALL_EXCEPT);
+
+  res = feupdateenv (FE_NOMASK_ENV);
+
+  if (!EXCEPTION_ENABLE_SUPPORTED (FE_ALL_EXCEPT) && (res != 0))
+    {
+      puts ("feupdateenv (FE_NOMASK_ENV)) not supported, cannot test.");
+      return;
+    }
+  else if (res != 0)
+    {
+      puts ("feupdateenv (FE_NOMASK_ENV) failed");
+      count_errors++;
+    }
+
+  if (fegetexcept () != FE_ALL_EXCEPT)
+    {
+      puts ("feupdateenv did not set all exceptions");
+      count_errors++;
+    }
+#endif
+}
+
+static int
+do_test (void)
+{
+  test_feenableexcept ();
+  test_fesetenv ();
+  test_feupdateenv ();
+
+  return count_errors != 0 ? 1 : 0;
+}
+
+#define TEST_FUNCTION do_test ()
+#include "../test-skeleton.c"