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

Corinna Vinschen <[email protected]> Wed, 4 Sep 2019 10:19:03 +0200
Newsgroups	gmane.comp.lib.newlib
Message-ID	<[email protected]>
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...

> + * 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?

> +/*  Although Cygwin assumes i686 or above (hence SSE available) these

Still, please drop Cygwin-specific comments.

> +/*  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?


Thanks,
Corinna

-- 
Corinna Vinschen
Cygwin Maintainer
Red Hat
signature.asc (application/pgp-signature, 833 B)