This is the mail archive of the libc-alpha@sourceware.org mailing list for the glibc project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

RE: [PATCH] Add FE_NOMASK_ENV return value test


> 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

Attachment: Add-fenv-return-test.txt
Description: Text document


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]