Re: [PATCH] Add FE_NOMASK_ENV return value test

["Joseph S. Myers" <joseph at codesourcery dot com>]

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.

-- 
Joseph S. Myers
joseph@codesourcery.com