[PATCH] libc: Replace i386/sys/fenv.h symlink with an #include shim
Corinna Vinschen
vinschen@redhat.com
Wed Apr 7 09:28:19 GMT 2021
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
More information about the Newlib
mailing list