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

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

[David Macek]

As in fbaa096772f77be664864d80508906ad018cc23b:

Having symlinks leads to issue unpacking the sources on platforms without proper
symlink support.  These platforms mostly extract symlinks from the archive file
as copies of the files the symlinks point to.  If the links appear in the tar
file before the source exists, it cannot copy the file.

The solution in this patch is to convert the files that are symbolic
links into simple files which include the file they were linked to.
This should be more portable and avoids the symbolic link problem.

Signed-off-by: David Macek <david.macek.0@gmail.com>
---

Excuse my ignorance, but is this acceptable?  I'm not sure
what actually happens with these files, but it'd be nice to
get rid of the last symlink in the repo.

 newlib/libc/machine/i386/sys/fenv.h | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)
 mode change 120000 => 100644 newlib/libc/machine/i386/sys/fenv.h

diff --git a/newlib/libc/machine/i386/sys/fenv.h
b/newlib/libc/machine/i386/sys/fenv.h
deleted file mode 120000
index 218057825e..0000000000
--- a/newlib/libc/machine/i386/sys/fenv.h
+++ /dev/null
@@ -1 +0,0 @@
-../../x86_64/sys/fenv.h
\ No newline at end of file
diff --git a/newlib/libc/machine/i386/sys/fenv.h
b/newlib/libc/machine/i386/sys/fenv.h
new file mode 100644
index 0000000000..d2c41a6d5a
--- /dev/null
+++ b/newlib/libc/machine/i386/sys/fenv.h
@@ -0,0 +1,5 @@
+/*
+ * SPDX-License-Identifier: BSD-2-Clause
+ */
+
+#include "../../x86_64/sys/fenv.h"

-- 
David Macek

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

[Corinna Vinschen]

On Mar  7 16:03, David Macek via Newlib wrote:
> As in fbaa096772f77be664864d80508906ad018cc23b:
> 
> Having symlinks leads to issue unpacking the sources on platforms without proper
> symlink support.  These platforms mostly extract symlinks from the archive file
> as copies of the files the symlinks point to.  If the links appear in the tar
> file before the source exists, it cannot copy the file.
> 
> The solution in this patch is to convert the files that are symbolic
> links into simple files which include the file they were linked to.
> This should be more portable and avoids the symbolic link problem.
> 
> Signed-off-by: David Macek <david.macek.0@gmail.com>
> ---
> 
> Excuse my ignorance, but is this acceptable?  I'm not sure
> what actually happens with these files, but it'd be nice to
> get rid of the last symlink in the repo.

This doesn't work.  The machine-specific header will get installed to
$DESTDIR/usr/include/sys/fenv.h, and it will now include a non-existing
file.  We have to keep the symlink or to duplicate the file.

However, I'm not aware that we still support a development platform not
supporting symlinks.  Even WIndows supports them for ages now.


Corinna

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

[David Macek]

On Mon, Mar 8, 2021 at 11:00 AM Corinna Vinschen <vinschen@redhat.com> wrote:
> This doesn't work.  The machine-specific header will get installed to
> $DESTDIR/usr/include/sys/fenv.h, and it will now include a non-existing
> file.  We have to keep the symlink or to duplicate the file.

How come the newlib/libm/machine/i386/fenv.c non-symlink works?  Is it
installed differently?

Coincidentally, is newlib/libc/machine/ft32/stpcmp.S file correct?

> However, I'm not aware that we still support a development platform not
> supporting symlinks.  Even WIndows supports them for ages now.

In the commit, I only mentioned "proper" symlink support for brevity,
but there's a lot in that word.  The differences are summarized for
example in https://github.com/git-for-windows/git/wiki/Symbolic-Links#background.
Due to these incompatibilities, if we want to handle symlinks on
Windows we only have bad options.

MSYS2 is keeping its traditional approach of making copies instead of
symlinks (which could probably be re-evaluated) and that leads to the
extraction issue described.  Git for Windows seems to just create
regular text files containing the link path by default.  Multiple
programs I use just skip the symlink when extracting.  Microsoft's
build of bsdtar (and probably Cygwin with winsymlinks:native) extracts
a symlink successfully, but then it's fixed as a file symlink.  None
of the three other Cygwin's methods (default, .lnk, WSL symlink) are
compatible with regular Win32 programs IIRC.  I think by now you can
see why I'm leaning towards not using symlinks unless unavoidable or
portability is out of question.

-- 
David Macek

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

[Corinna Vinschen]

On Mar  9 20:11, David Macek via Newlib wrote:
> On Mon, Mar 8, 2021 at 11:00 AM Corinna Vinschen <vinschen@redhat.com> wrote:
> > This doesn't work.  The machine-specific header will get installed to
> > $DESTDIR/usr/include/sys/fenv.h, and it will now include a non-existing
> > file.  We have to keep the symlink or to duplicate the file.
> 
> How come the newlib/libm/machine/i386/fenv.c non-symlink works?  Is it
> installed differently?

It's not installed at all.  It's a source file so the include only has
to work at build time.  The header file on the other hand will be
installed on the target.  As such, the result has to work on the target.

> Coincidentally, is newlib/libc/machine/ft32/stpcmp.S file correct?

I have no idea, you have to ask the guys knowing that chip type.


Corinna

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

[David Macek]

> > > This doesn't work.  The machine-specific header will get installed to
> > > $DESTDIR/usr/include/sys/fenv.h, and it will now include a non-existing
> > > file.  We have to keep the symlink or to duplicate the file.
> >
> > How come the newlib/libm/machine/i386/fenv.c non-symlink works?  Is it
> > installed differently?
>
> It's not installed at all.  It's a source file so the include only has
> to work at build time.  The header file on the other hand will be
> installed on the target.  As such, the result has to work on the target.

Ah, right.  But then I'm confused how a symlink can work in the first
place.  I guess some part of the build process copies the contents
instead of the file.

More to the point, how big of an issue would be to duplicate the file?

> > Coincidentally, is newlib/libc/machine/ft32/stpcmp.S file correct?
>
> I have no idea, you have to ask the guys knowing that chip type.

I didn't read the instructions, it's just the label looks wrong. :)
Should I start another thread?

-- 
David Macek

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

[Corinna Vinschen]

On Mar 11 08:13, David Macek via Newlib wrote:
> > > > This doesn't work.  The machine-specific header will get installed to
> > > > $DESTDIR/usr/include/sys/fenv.h, and it will now include a non-existing
> > > > file.  We have to keep the symlink or to duplicate the file.
> > >
> > > How come the newlib/libm/machine/i386/fenv.c non-symlink works?  Is it
> > > installed differently?
> >
> > It's not installed at all.  It's a source file so the include only has
> > to work at build time.  The header file on the other hand will be
> > installed on the target.  As such, the result has to work on the target.
> 
> Ah, right.  But then I'm confused how a symlink can work in the first
> place.  I guess some part of the build process copies the contents
> instead of the file.
> 
> More to the point, how big of an issue would be to duplicate the file?

I'm not hot on duplicating an identical file, it just raises maintenance
cost.  I'm currently looking into a solution to share the file without
duplicating it.

> > > Coincidentally, is newlib/libc/machine/ft32/stpcmp.S file correct?
> >
> > I have no idea, you have to ask the guys knowing that chip type.
> 
> I didn't read the instructions, it's just the label looks wrong. :)

Oh, right, I see what you mean.

> Should I start another thread?

Yes, ideally addressing the FT32 CPU in the subject.


Thanks,
Corinna

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

[David Macek]

> > More to the point, how big of an issue would be to duplicate the file?
>
> I'm not hot on duplicating an identical file, it just raises maintenance
> cost.  I'm currently looking into a solution to share the file without
> duplicating it.

Something like a Makefile rule?

--
David Macek

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

[Joel Sherrill]

On Sat, Mar 13, 2021, 12:11 PM David Macek via Newlib <newlib@sourceware.org>
wrote:

> > > More to the point, how big of an issue would be to duplicate the file?
> >
> > I'm not hot on duplicating an identical file, it just raises maintenance
> > cost.  I'm currently looking into a solution to share the file without
> > duplicating it.
>
> Something like a Makefile rule?
>

The include file set for newlib already merges files from multiple
directories in a specific order and I recall (maybe incorrectly) that they
aren't merged during the build but the include path is carefully managed.

Is there a place put a shared file and have a one liner in both targets?
The problem I see here is that this is an odd case where two architectures
share the same code and you need a common include directory. For RTEMS,
this isn't unusual and our build system accounts for this well. But this is
an outlier for newlib.

Maybe a mix of a unique shared filename that is included and a secondary
machine directory being added to the include processing.

Just thinking out loud. There is an overarching pattern to how the shared,
machine, and system include directories are processed and I think any
solution to avoid symbolic links across architectures is going to have to
account for that.

--joel

>
> --
> David Macek
>

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

[Corinna Vinschen]

On Mar 13 19:11, David Macek via Newlib wrote:
> > > More to the point, how big of an issue would be to duplicate the file?
> >
> > I'm not hot on duplicating an identical file, it just raises maintenance
> > cost.  I'm currently looking into a solution to share the file without
> > duplicating it.
> 
> Something like a Makefile rule?

A bit more complicated in fact.  I added a new topic branch called
topic/shared_arch_headers to the upstream repo, containing a single
patch

  a8cd2849ed0c Add build mechanism to share common header files between
               machines

This patch introduces a new way to share header files between
architectures, not share by other architectures.

I tried to change this in a generic way.  The fenv stuff in this patch
is basically just the first user of this new mechanism.

As a side-effect, this also allows to remove Cygwin's own fenv code
finally, which was already pretty much the same as newlib's x86 code
anyway.  The major exception was the call to _feinitialise in Cygwin DLL
startup.  Given that the newlib fenv code is auto-initializing, this can
go away.  Cygwin just has to keep the _feinitialise entry point for
backward compatibility for apps built in a certain time frame.

David, Joel, anybody else interested in this stuff, please inspect
this patch on the topic/shared_arch_headers branch thorougly.  I will
not push this quickly, since we're in the process of releasing Cygwin
3.2.0.  I don't want to destabilise the release without need :)


Thanks,
Corinna

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

[Corinna Vinschen]

On Mar 23 15:38, Corinna Vinschen via Newlib wrote:
> On Mar 13 19:11, David Macek via Newlib wrote:
> > > > More to the point, how big of an issue would be to duplicate the file?
> > >
> > > I'm not hot on duplicating an identical file, it just raises maintenance
> > > cost.  I'm currently looking into a solution to share the file without
> > > duplicating it.
> > 
> > Something like a Makefile rule?
> 
> A bit more complicated in fact.  I added a new topic branch called
> topic/shared_arch_headers to the upstream repo, containing a single
> patch
> 
>   a8cd2849ed0c Add build mechanism to share common header files between
>                machines

Update: I split this patch into seven patches and force pushed them
onto topic/shared_arch_headers:

3835c015d09d Add build mechanism to share common header files between machines
e8e95bf6436a configure.host: define shared ix86 and x86_64 directory
718f3955449f fenv: add missing declarations to x86 fenv.h
a0d06f6c50ed fenv: Move shared x86 sys/fenv.h from x86_64 to shared_x86
c06e30d3d961 fenv: move shared x86 fenv.c to libm/machine/shared_x86
7803e212adcd fenv: drop Cygwin-specific implementation in favor of newlib code
3b79d7e1c31f Cygwin: don't call _feinitialise from _dll_crt0

Please fetch this change and reset your branch to this new state.
This should very much simplify reviewing the changes.

> This patch introduces a new way to share header files between
> architectures, not share by other architectures.
> 
> I tried to change this in a generic way.  The fenv stuff in this patch
> is basically just the first user of this new mechanism.
> 
> As a side-effect, this also allows to remove Cygwin's own fenv code
> finally, which was already pretty much the same as newlib's x86 code
> anyway.  The major exception was the call to _feinitialise in Cygwin DLL
> startup.  Given that the newlib fenv code is auto-initializing, this can
> go away.  Cygwin just has to keep the _feinitialise entry point for
> backward compatibility for apps built in a certain time frame.
> 
> David, Joel, anybody else interested in this stuff, please inspect
> this patch on the topic/shared_arch_headers branch thorougly.  I will
> not push this quickly, since we're in the process of releasing Cygwin
> 3.2.0.  I don't want to destabilise the release without need :)


Thanks,
Corinna

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

[David Macek]

Thank you.  Below are my notes.  I believe some are real issues, but
most are novice questions, so be kind. :)

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

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.

> e8e95bf6436a configure.host: define shared ix86 and x86_64 directory

3. The globs in Makefile.am fail gracefully when they don't match, right?

> 718f3955449f fenv: add missing declarations to x86 fenv.h

Can't judge.

> a0d06f6c50ed fenv: Move shared x86 sys/fenv.h from x86_64 to shared_x86

OK.

> c06e30d3d961 fenv: move shared x86 fenv.c to libm/machine/shared_x86

OK.

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

5. Is it okay for autoload.cc and dcrt0.cc to still #include "fenv.h"
instead of <fenv.h>?

> 3b79d7e1c31f Cygwin: don't call _feinitialise from _dll_crt0

6. The date in the new fenv.c seems awfully in the past.

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?

> Please fetch this change and reset your branch to this new state.
> This should very much simplify reviewing the changes.

8. I noticed newlib/libm/fenv/fenv_stub.c still mentions symlinks.

-- 
David Macek

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

[Corinna Vinschen]

On Apr  5 09:37, David Macek via Newlib wrote:
> Thank you.  Below are my notes.  I believe some are real issues, but
> most are novice questions, so be kind. :)
> 
> > 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.

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

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

  if [ -n "" ]; then \
      for i in $(srcdir)/libc/machine//machine/*.h; do
      [...]

There's nothing *failing* as such.

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

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

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

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

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

> > Please fetch this change and reset your branch to this new state.
> > This should very much simplify reviewing the changes.
> 
> 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 force-pushed the changes in terms of the includes and the call to
fesetenv from dcrt0.

  3b79d7e1c31f Cygwin: don't call _feinitialise from _dll_crt0

has been replaced with

  797a9278bb29 Cygwin: don't export _feinitialise from newlib
  de6ffe470c44 Cygwin: fix fenv.h includes

Please have another look.


Thanks,
Corinna

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

[David Macek]

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

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

[Corinna Vinschen]

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

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

[David Macek]

> Changes force-pushed to topic/shared_arch_headers.

LGTM overall now.  Would be nice if someone tested the actual build
now (could be me if given some instructions).

-- 
David Macek

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

[Corinna Vinschen]

On Apr  7 18:16, David Macek wrote:
> > Changes force-pushed to topic/shared_arch_headers.
> 
> LGTM overall now.  Would be nice if someone tested the actual build
> now (could be me if given some instructions).

I pushed this change to the master branch.

Testing is basically building and installing into some DESTDIR
and see if fenv.h and sys/fenv.h are correctly copied into DESTDIR.


Thanks,
Corinna