[PATCH] libc: Replace i386/sys/fenv.h symlink with an #include shim
David Macek
david.macek.0@gmail.com
Wed Apr 7 07:28:13 GMT 2021
> > > 3835c015d09d Add build mechanism to share common header files between machines
> >
> > I assume I only need to check Makefile.am (and acinclude.m4) here.
> > It's good as far as I can tell, the order among other sources of
> > header files seems reasonable.
> >
> > 1. I'm unsure about the indentation, the file already had been
> > inconsistent, so I just note that the first hunk doesn't match its
> > immediate surroundings.
>
> Do you mean the indentation between the first if and the following for
> loops by any chance? There's a mix of 4 and 2 space indentations and I
> opted for a uniform upper level indentation of 4 spaces. Only the inner
> if has 2 spaces, as in the surrounding code. Given there's no other
> outer if in the surrounding code, there was no precedent at this point
> in the file. I don't care, actually. If anybody wants a 2 space
> indentation from the outer if to the for loops, ok with me.
I don't really care either way, just wanted to point out in case
anyone else does.
> > 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=
> > > 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?
> > > 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?
> > > 7803e212adcd fenv: drop Cygwin-specific implementation in favor of newlib code
> >
> > 4. A noticeable difference between cygwin's and newlib's fenv.c is
> > use_sse being a run-time-initialized constant vs a (inline) function.
> > Are we keeping the latter intentionally, or it doesn't matter?
>
> I checked this when I created the patch and AFAICS it doesn't matter.
> use_sse as a global var requires initialization, while the inline
> function does not. It's more a question of speed vs. simplicity
> I guess, but these functions are typically not called in time-critical
> loops, so the speed factor is likely neglectable.
OK.
> > 5. Is it okay for autoload.cc and dcrt0.cc to still #include "fenv.h"
> > instead of <fenv.h>?
>
> 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.
> > > 3b79d7e1c31f Cygwin: don't call _feinitialise from _dll_crt0
> >
> > 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,
- the intentional usage in initialization code injected into programs
has been removed for a long time now,
- the intentional usage in cygwin1.dll is now being replaced (as noted below).
> > 7. Why can we assume the FP environment is initialized? Is it because
> > fesetenv() is called in autoload.cc, or is it just a guarantee from
> > the OS?
>
> Good point! The FP environment is initialized by Windows, but it's
> initialized differently from what the x86 finit/fninit op does. The
> default precision after finit/fninit is 64 bit, while the Windows
> default precision is 53 bit.
>
> That's the only difference, but it still requires to initialize the FP
> env prior to calling the application's main function. However, rather
> than keeping the non-standard _feinitialise call, I change dcrt0.cc
> to call fesetenv(FE_DFL_ENV) instead, which is more standard.
OK.
> > 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.
*/
/* deliberately empty */
(
> 797a9278bb29 Cygwin: don't export _feinitialise from newlib
OK, see above.
> de6ffe470c44 Cygwin: fix fenv.h includes
OK, see above.
--
David Macek
More information about the Newlib
mailing list