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