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

[David Macek <david.macek.0@gmail.com>]

> > > 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