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

On Wed, Aug 28, 2019 at 10:41 AM Corinna Vinschen <[email protected]> wrote:
>
> On Aug 28 10:06, [email protected] wrote:
> > From: Joel Sherrill <[email protected]>
> >
> > ---
> >  newlib/libc/machine/i386/sys/fenv.h    |   1 +
> >  newlib/libc/machine/x86_64/sys/fenv.h  | 150 ++++++++++
> >  newlib/libm/machine/i386/Makefile.am   |   2 +-
> >  newlib/libm/machine/i386/fenv.c        |   1 +
> >  newlib/libm/machine/x86_64/Makefile.am |  18 ++
> >  newlib/libm/machine/x86_64/fenv.c      | 485 +++++++++++++++++++++++++++++++++
> >  6 files changed, 656 insertions(+), 1 deletion(-)
> >  create mode 120000 newlib/libc/machine/i386/sys/fenv.h
> >  create mode 100644 newlib/libc/machine/x86_64/sys/fenv.h
> >  create mode 120000 newlib/libm/machine/i386/fenv.c
> >  create mode 100644 newlib/libm/machine/x86_64/Makefile.am
> >  create mode 100644 newlib/libm/machine/x86_64/fenv.c
> >
> > diff --git a/newlib/libc/machine/i386/sys/fenv.h b/newlib/libc/machine/i386/sys/fenv.h
> > new file mode 120000
> > index 0000000..2180578
> > --- /dev/null
> > +++ b/newlib/libc/machine/i386/sys/fenv.h
> > @@ -0,0 +1 @@
> > +../../x86_64/sys/fenv.h
> > \ No newline at end of file
> > diff --git a/newlib/libc/machine/x86_64/sys/fenv.h b/newlib/libc/machine/x86_64/sys/fenv.h
> > new file mode 100644
> > index 0000000..69f7bef
> > --- /dev/null
> > +++ b/newlib/libc/machine/x86_64/sys/fenv.h
> > @@ -0,0 +1,150 @@
> > +/* fenv.h
> > +
> > +This file is part of Cygwin.
> > +
> > +This software is a copyrighted work licensed under the terms of the
> > +Cygwin license.  Please consult the file "CYGWIN_LICENSE" for
> > +details. */
>
>   SPDX-License-Identifier: BSD-2-Clause
>
> not BSD-3-Clause as I wrote in other mail.

I added that using some other random file as an example.

>
> > +/*  The <fenv.h> header shall define the following constant, which
> > +   represents the default floating-point environment (that is, the one
> > +   installed at program startup) and has type pointer to const-qualified
> > +   fenv_t. It can be used as an argument to the functions within the
> > +   <fenv.h> header that manage the floating-point environment.  */
> > +
> > +extern const fenv_t *_fe_dfl_env;
> > +#define FE_DFL_ENV (_fe_dfl_env)
>
> These can go away, right?  They are already defined in
> newlib/libc/include/sys/fenv.h.

Each architecture overrides sys/fenv.h. There is no sharing of
libc/include/sys/fenv.h
with a functional implementation.

>
> > +#if __GNU_VISIBLE
> > +/*  If possible, the GNU C Library defines a macro FE_NOMASK_ENV which
> > +   represents an environment where every exception raised causes a trap
> > +   to occur. You can test for this macro using #ifdef. It is only defined
> > +   if _GNU_SOURCE is defined.  */
> > +extern const fenv_t *_fe_nomask_env;
> > +#define FE_NOMASK_ENV (_fe_nomask_env)
> > +#endif /* __GNU_VISIBLE */
>
> And those you just added to newlib/libc/include/sys/fenv.h in patch 2 of
> this set.

Ditto on architecture speciific sys/fenv.h

> > +/* 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;
>
> Given these are now declared in a shared header, shouldn't these be
> added to their own file, newlib/libm/fenv/fe_nomask_env.c parallel
> to newlib/libm/fenv/fe_dfl_env.c?

Sure. I need to add a stub for this.

>
> > +/*  Although Cygwin assumes i686 or above (hence SSE available) these
>
> Please drop Cygwin-specific comments.  They just don't make sense in
> common newlib code, except in rare cases to explain a difference to
> other targets.
>
> > [...]
> > +#if defined(__CYGWIN__)
>
> Great.
>
> > +/*  Returns the currently selected precision, represented by one of the
> > +   values of the defined precision macros.  */
> > +int
> > +fegetprec (void)
> > +{
> > [...]
> > +int
> > +fesetprec (int prec)
> > +{
> > [...]
> > +#endif
>
> Uh oh.  What about _feinitialise()?  Cygwin calls this function
> right from the initial code, but is it really the right thing
> to enforce this for all i386/x86_64 targets?
>
> Any idea how we can generate the default environment on the fly
> while maintaining backward compat on Cygwin?

Cygwin, libgloss, and RTEMS could call this I suppose. But each OS would have
to do their own thing.

Should be really be called from the beginning of each thread? Otherwise,
things are inconsistent.

--joel

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