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

On Wed, Sep 4, 2019 at 3:19 AM Corinna Vinschen <[email protected]> wrote:
>
> Hi Joel,
>
> 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 don't see changing the pattern for this file and I doubt we will be deleting
copyright/license text from at least FreeBSD sourced code since that will
make it harder to track against the upstream.

>
> > + * 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.
> > + */
>
> Ditto.
>
> > +/* These are writable so we can initialise them at startup.  */
> > +static fenv_t fe_nomask_env;
> > +
> > +/* These pointers provide the outside world with read-only access to them.  */
> > +const fenv_t *_fe_nomask_env = &fe_nomask_env;
> > +
>
> 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.

By keeping them in the machine sys/fenv.h and defining them
in the machine specific fenv.c, I thought that addressed that.

> > +/*  Although Cygwin assumes i686 or above (hence SSE available) these
>
> Still, please drop Cygwin-specific comments.

I forgot to do that. First sentence rewritten.

> > +/*  Set the floating-point environment to that described by envp.  The
> > +   function returns zero in case the operation was successful, a non-zero
> > +   value otherwise.  */
> > +int
> > +fesetenv (const fenv_t *envp)
> > +{
> > +  __asm__ volatile ("fldenv %0" :: "m" (envp->_fpu) );
> > +  if (use_sse)
> > +    __asm__ volatile ("ldmxcsr %0" :: "m" (envp->_sse_mxcsr));
> > +  return 0;
> > +}
>
> 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?

--joel

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