Re: [PATCH] libc: Replace i386/sys/fenv.h symlink with an #include shim

On Apr  7 09:28, David Macek wrote:
> > > 2. I would expect shared_machine_dir to be introduced to
> > > configure.host in this patch as well, in the documentation and in the
> > > initialization block.
> >
> > The first patch actually introduces shared_machine_dir.  The second
> > patch introduces its first usage for the first targets.
> 
> First patch says what to do when shared_machine_dir is defined, second
> patch defines shared_machine_dir for some architectures but it's not
> declared anywhere.  In other words, I'm missing this:
> 
> diff --git a/newlib/configure.host b/newlib/configure.host
> index ca0176e778..90416e3fa7 100644
> --- a/newlib/configure.host
> +++ b/newlib/configure.host
> @@ -33,6 +33,7 @@
> 
>  # It sets the following shell variables:
>  #   newlib_cflags      Special CFLAGS to use when building
> +#   shared_machine_dir Subdirectory of libc/machine to use as base, optional
>  #   machine_dir                Subdirectory of libc/machine to configure
>  #   sys_dir            Subdirectory of libc/sys to configure
>  #   have_sys_mach_dir  Is there a machine subdirectory in sys subdirectory
> @@ -54,6 +55,7 @@
> 
>  newlib_cflags=
>  libm_machine_dir=
> +shared_machine_dir=
>  machine_dir=
>  sys_dir=
>  posix_dir=

Ah, right.  I added that to the first patch.

> > > > e8e95bf6436a configure.host: define shared ix86 and x86_64 directory
> > >
> > > 3. The globs in Makefile.am fail gracefully when they don't match, right?
> >
> > Not sure what you're asking.  If shared_machine_dir is empty, the
> > shell code is eqivalent to
> 
> I mean if shared_machine_dir is nonempty, three globs are executed:
> 
> - $(srcdir)/libc/machine/$(shared_machine_dir)/machine/*.h
> - $(srcdir)/libc/machine/$(shared_machine_dir)/sys/*.h
> - $(srcdir)/libc/machine/$(shared_machine_dir)/include/*.h
> 
> ... while only the .../sys/... one actually matches anything.  I have
> learned to be wary of such things.  But having read the code again, I
> guess the following -f test saves the day there, right?

Right.  The same mechanism is used in the already existing code and
not all machine dirs have all these subdirs.

> > > > 718f3955449f fenv: add missing declarations to x86 fenv.h
> > >
> > > Can't judge.
> >
> > These functions are defined in x86 fenv.c, but they were not declared
> > in the fenv.h header.  This patch is adding them to the header, that's
> > all.
> 
> OK, sounds good.  Are new declarations supposed to be documented somewhere?

They aren't new, check other arches.  They were just not declared for
x86/x86_64, accidentally.

> > It shouldn't matter, but it was never actually 100% correct to include
> > "fenv.h" with fenv.h being a system header.  I'll push a patch changing
> > this in autoload.cc and dropping it entirely from dcrt0.cc.
> 
> It's not dropped from dcrt0.cc in
> de6ffe470c44bc6fcbd60986507a9ec8fdc77a4a, which I think is actually
> correct.

D'oh!  I wrote that prior to checking the fenv initialization and
forgot to fix my mail, sorry.

> > > 6. The date in the new fenv.c seems awfully in the past.
> >
> > Do you mean the dates in the comment?  Yes, it's pretty long ago.  But
> > Cygwin's striving for backward compatibility.  If we don't provide this
> > symbol, and *iff* there's still an application running built in this
> > timeframe, it will stop working.
> 
> Ah, okay, so calling _feinitialise():
> 
> - has never been supported in user code,

ACK

> - the intentional usage in initialization code injected into programs
> has been removed for a long time now,

Well, not injected as such, it was just originally part of the crt0.o
startup code and later moved into the DLL startup code.

> - the intentional usage in cygwin1.dll is now being replaced (as noted below).

ACK

> > > 8. I noticed newlib/libm/fenv/fenv_stub.c still mentions symlinks.
> >
> > Not sure how to change the wording here.  In retrospect the text is
> > actually puzzeling me more than it helps.  Joel, can you please take
> > a look?
> 
> I propose this:
> 
> diff --git a/newlib/libm/fenv/fenv_stub.c b/newlib/libm/fenv/fenv_stub.c
> index a4eb652f3e..5a560bf7c5 100644
> --- a/newlib/libm/fenv/fenv_stub.c
> +++ b/newlib/libm/fenv/fenv_stub.c
> @@ -7,16 +7,16 @@
>  /*
>   * This file is intentionally empty.
>   *
> - * Newlib's build infrastructure needs a machine specific fiel to override
> + * Newlib's build infrastructure needs a machine specific file to override
>   * the generic implementation in the library.  When a target
>   * implementation of the fenv.h methods puts all methods in a single file
>   * (e.g. fenv.c) or some as inline methods in its <sys/fenv.h>, it will need
>   * to override the default implementation found in a file in this directory.
>   *
>   * For each file that the target's machine directory needs to override,
> - * this file should be symbolically linked to that specific file name
> - * in the target directory. For example, the target may use fe_dfl_env.c
> - * from the default implementation but need to override all others.
> + * there should be a corresponding stub file in the target directory.
> + * To avoid copying this explanation far and wide, #including this fenv_stub.c
> + * from the stub files in encouraged.
>   */

Thanks, I applied this and just changed the overall formatting a bit.

Changes force-pushed to topic/shared_arch_headers.


Corinna