Re: [PATCH v2] Add i386 and x86_64 fenv support from Cygwin.

On Wed, Sep 4, 2019 at 10:12 AM Corinna Vinschen <[email protected]> wrote:
>
> On Sep  4 07:43, Joel Sherrill wrote:
> > On Wed, Sep 4, 2019 at 3:19 AM Corinna Vinschen <[email protected]> wrote:
> > > On Sep  3 11:02, [email protected] wrote:
> > > > index 0000000..83f5995
> > > > --- /dev/null
> > > > +++ b/newlib/libc/machine/x86_64/sys/fenv.h
> > > > @@ -0,0 +1,170 @@
> > > > +/*
> > > > + * SPDX-License-Identifier: BSD-2-Clause
> > > > + *
> > > > + * Copyright (c) 2010-2019 Red Hat, Inc.
> > > > + * All rights reserved.
> > > > + *
> > > > + * Redistribution and use in source and binary forms, with or without
> > > > + * modification, are permitted provided that the following conditions
> > > > + * are met:
> > > > + * 1. Redistributions of source code must retain the above copyright
> > > > + *    notice, this list of conditions and the following disclaimer.
> > > > + * 2. Redistributions in binary form must reproduce the above copyright
> > > > + *    notice, this list of conditions and the following disclaimer in the
> > > > + *    documentation and/or other materials provided with the distribution.
> > > > + *
> > > > + * THIS SOFTWARE IS PROVIDED BY THE AUTHOR AND CONTRIBUTORS ``AS IS'' AND
> > > > + * ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE
> > > > + * IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE
> > > > + * ARE DISCLAIMED.  IN NO EVENT SHALL THE AUTHOR OR CONTRIBUTORS BE LIABLE
> > > > + * FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL
> > > > + * DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS
> > > > + * OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION)
> > > > + * HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT
> > > > + * LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY
> > > > + * OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF
> > > > + * SUCH DAMAGE.
> > > > + */
> > >
> > > The SPDX-License-Identifier usually replaces the license text...
> >
> > Maybe in principle but I couldn't find a single file in newlib which had only
> > an SPDX indicator. All 63 had SPDX and license text.
>
> I didn't actually check newlib files, but SPDX + written out license
> only makes marginal sense.  So, let's start with *these* files to do it
> right, please.

SPDX line plus an organization copyright? Like this:

/*
 * SPDX-License-Identifier: BSD-2-Clause
 *
 * Copyright (c) 2010-2019 Red Hat, Inc.
 * All rights reserved.
 */

>
> > > Didn't we talk about defining those in their own file?
> >
> > I had a patch which added them to the stub support but they aren't
> > generic C/POSIX methods and there was a comment that they
> > should be target specific.
>
> Hmm, yeah, right.  I missed that, sorry.
>
> > > I'm still not happy that all targets have to call _feinitialise() at
> > > some arbitrary point in their init code to set up FE_DFL_ENV and
> > > FE_NOMASK_ENV.  That may work for Cygwin with the Cygwin DLL being an
> > > integral part of all processes, but for other targets that only makes
> > > marginal sense, especially if no fenv functions are used.
> > >
> > > So, here's an idea:
> > >
> > > int
> > > fesetenv (const fenv_t *envp)
> > > {
> > >   if ((envp == FE_DFL_ENV || envp == FE_NOMASK_ENV)
> > >       && envp->_fpu._fpu_cw == 0)
> > >     _feinitialise ();
> > >   [...]
> > > }
> > >
> > > In theory, this single test should cover all bases.  Even Cygwin
> > > can drop the _feinitialise() call from its init code then.
> > >
> > > Am I missing something?
> >
> > I think _feinitialize can now be static. I moved it to the top of the file
> >
> > Doing that lets the prototype and a CYGWIN specific comment in the sys/fenv.h
> > disappear.
> >
> > Does that sound ok?
>
> That sounds right to me.  Let's do that for now.
>
> With the above changes I can push the patch, but I won't probably be
> able to test this on Cygwin until after my vacation.  It might be
> better to go ahead with the current code in Cygwin for the Cygwin 3.1
> release and only afterr that I'll start switching, ok?

I think that's prudent for Cygwin. It had a working implementation. I don't
like to touch production code right on a Friday afternoon or before a
vacation. :)

I am testing a new version of the patch now. I can make the SPDX changes
if you confirm.

>
>
> Thanks,
> Corinna
>
> --
> Corinna Vinschen
> Cygwin Maintainer
> Red Hat