ieeefp.c: Auto-detect _LDBL_EQ_DBL

Message ID 20180906041829.15935-1-keithp@keithp.com
State New
Headers show
Series
  • ieeefp.c: Auto-detect _LDBL_EQ_DBL
Related show

Commit Message

Keith Packard Sept. 6, 2018, 4:18 a.m.
Make configuring the library a bit simpler

Signed-off-by: Keith Packard <keithp@keithp.com>

---
This seems simpler than computing this value during configure, and
will make using meson easier in the future.

 newlib/libc/include/ieeefp.h | 5 +++++
 1 file changed, 5 insertions(+)

-- 
2.19.0.rc2

Comments

Corinna Vinschen Sept. 6, 2018, 12:22 p.m. | #1
On Sep  5 21:18, Keith Packard wrote:
> Make configuring the library a bit simpler

> 

> Signed-off-by: Keith Packard <keithp@keithp.com>

> ---

> This seems simpler than computing this value during configure, and

> will make using meson easier in the future.

> 

>  newlib/libc/include/ieeefp.h | 5 +++++

>  1 file changed, 5 insertions(+)

> 

> diff --git a/newlib/libc/include/ieeefp.h b/newlib/libc/include/ieeefp.h

> index 2d6421a4c..71749ccc0 100644

> --- a/newlib/libc/include/ieeefp.h

> +++ b/newlib/libc/include/ieeefp.h

> @@ -143,6 +143,11 @@ typedef union

>  

>  #endif /* __IEEE_LITTLE_ENDIAN */

>  

> +#if LDBL_MANT_DIG == DBL_MANT_DIG && LDBL_MIN_EXP == DBL_MIN_EXP && \

> +    LDBL_MAX_EXP == DBL_MAX_EXP

> +#define _LDBL_EQ_DBL

> +#endif

> +

>  #ifndef _LDBL_EQ_DBL

>  

>  #ifndef LDBL_MANT_DIG

> -- 

> 2.19.0.rc2


I leave that one to Jeff.


Thanks,
Corinna

-- 
Corinna Vinschen
Cygwin Maintainer
Red Hat
Craig Howland Sept. 6, 2018, 10:13 p.m. | #2
On 09/06/2018 12:18 AM, Keith Packard wrote:
> Make configuring the library a bit simpler

>

> Signed-off-by: Keith Packard <keithp@keithp.com>

> ---

> This seems simpler than computing this value during configure, and

> will make using meson easier in the future.

>

>   newlib/libc/include/ieeefp.h | 5 +++++

>   1 file changed, 5 insertions(+)

>

> diff --git a/newlib/libc/include/ieeefp.h b/newlib/libc/include/ieeefp.h

> index 2d6421a4c..71749ccc0 100644

> --- a/newlib/libc/include/ieeefp.h

> +++ b/newlib/libc/include/ieeefp.h

> @@ -143,6 +143,11 @@ typedef union

>   

>   #endif /* __IEEE_LITTLE_ENDIAN */

>   

> +#if LDBL_MANT_DIG == DBL_MANT_DIG && LDBL_MIN_EXP == DBL_MIN_EXP && \

> +    LDBL_MAX_EXP == DBL_MAX_EXP

> +#define _LDBL_EQ_DBL

> +#endif

> +

>   #ifndef _LDBL_EQ_DBL

>   

>   #ifndef LDBL_MANT_DIG

     Back when _LDBL_EQ_DBL was added, the reason for a configure-time test was 
that there were compiler options that choose long double size.  (See 
https://sourceware.org/ml/newlib/2009/msg00497.html)  GCC does have 
-mlong-double options which allow the size of long double to be changed, so the 
original reason for the decision still applies.  So unless some kind of multilib 
magic can be added to take care of this, the proposal is a non-starter.
      However, even assuming that the configure-time problem can be solved, this 
patch by itself seems to be a bad idea because it is leaving the 
config-generated define in newlib.h.  I don't think the math routines all 
include include/ieeefp.h (they probably all get include/machine/ieeefp.h), so 
that the definitions could end up coming from different places.  (Realistically 
they should never give a different result, but it is not good practice have a 
decision like this in 2 places.)  So to do something like this, it 1) needs a 
place where all files using it can get it (or all files using it would need to 
be sure they included ieeefp.h), and 2)  get rid of the configured part of it 
and the definition in newlib.h.  (That is, this patch is the start of a 
configuration simplification, but is incomplete.)
      In addition, this could/would produce warnings on platforms without long 
double, so an added term would be needed if this bit of it is kept, something like:

#if defined(LDBL_MANT_DIG) && LDBL_MANT_DIG == DBL_MANT_DIG && ...



                 Craig
Joel Sherrill Sept. 6, 2018, 11:08 p.m. | #3
On Thu, Sep 6, 2018 at 5:14 PM Craig Howland <howland@lgsinnovations.com>
wrote:

> On 09/06/2018 12:18 AM, Keith Packard wrote:

> > Make configuring the library a bit simpler

> >

> > Signed-off-by: Keith Packard <keithp@keithp.com>

> > ---

> > This seems simpler than computing this value during configure, and

> > will make using meson easier in the future.

> >

> >   newlib/libc/include/ieeefp.h | 5 +++++

> >   1 file changed, 5 insertions(+)

> >

> > diff --git a/newlib/libc/include/ieeefp.h b/newlib/libc/include/ieeefp.h

> > index 2d6421a4c..71749ccc0 100644

> > --- a/newlib/libc/include/ieeefp.h

> > +++ b/newlib/libc/include/ieeefp.h

> > @@ -143,6 +143,11 @@ typedef union

> >

> >   #endif /* __IEEE_LITTLE_ENDIAN */

> >

> > +#if LDBL_MANT_DIG == DBL_MANT_DIG && LDBL_MIN_EXP == DBL_MIN_EXP && \

> > +    LDBL_MAX_EXP == DBL_MAX_EXP

> > +#define _LDBL_EQ_DBL

> > +#endif

> > +

> >   #ifndef _LDBL_EQ_DBL

> >

> >   #ifndef LDBL_MANT_DIG

>      Back when _LDBL_EQ_DBL was added, the reason for a configure-time

> test was

> that there were compiler options that choose long double size.  (See

> https://sourceware.org/ml/newlib/2009/msg00497.html)  GCC does have

> -mlong-double options which allow the size of long double to be changed,

> so the

> original reason for the decision still applies.  So unless some kind of

> multilib

> magic can be added to take care of this, the proposal is a non-starter.

>       However, even assuming that the configure-time problem can be

> solved, this

> patch by itself seems to be a bad idea because it is leaving the

> config-generated define in newlib.h.  I don't think the math routines all

> include include/ieeefp.h (they probably all get include/machine/ieeefp.h),

> so

> that the definitions could end up coming from different places.

> (Realistically

> they should never give a different result, but it is not good practice

> have a

> decision like this in 2 places.)  So to do something like this, it 1)

> needs a

> place where all files using it can get it (or all files using it would

> need to

> be sure they included ieeefp.h), and 2)  get rid of the configured part of

> it

> and the definition in newlib.h.  (That is, this patch is the start of a

> configuration simplification, but is incomplete.)

>       In addition, this could/would produce warnings on platforms without

> long

> double, so an added term would be needed if this bit of it is kept,

> something like:

>

> #if defined(LDBL_MANT_DIG) && LDBL_MANT_DIG == DBL_MANT_DIG && ...

>


There is some old thread on newlib and maybe gcc where I took a
swing at the same issue. I got stuck on the multilib variant issue and
decided I couldn't spend more time. I recall problems on architectures
like the sh where some variant has only single precision floating point.
I think a Coldfire variant also tripped it up.

It needs to vary on a per-multilib basis. I am not sure where the logic
goes for a probe like that would end up in a .h file so it could vary.

--joel

>

>

>

>                  Craig

>
Keith Packard Sept. 7, 2018, 4:34 a.m. | #4
Craig Howland <howland@LGSInnovations.com> writes:

>      Back when _LDBL_EQ_DBL was added, the reason for a configure-time test was 

> that there were compiler options that choose long double size.  (See 

> https://sourceware.org/ml/newlib/2009/msg00497.html)  GCC does have 

> -mlong-double options which allow the size of long double to be changed, so the 

> original reason for the decision still applies.  So unless some kind of multilib 

> magic can be added to take care of this, the proposal is a

> non-starter.


Thanks for the history; I figured there was a good reason this was done
at configure time and not build time, but I couldn't think of what it
could be.

multilib is precisely the reason I want to have this detected at compile
time and not configure time. multilib installations do not have
per-variant include files, so if there is any difference in this value
across variants, then the application will not be able to know how its
particular library was compiled, only how the version which produced the
installed newlib.h file was compiled.

In a correctly configured multilib environment, the compiler options in
force for the application will direct the linking process to locate the
matching libraries, so having the application use the compile-time
definition of this value should cause it to match the selected library.

I think the current mechanism can only work in a non-multilib
environment where the application can rely on the newlib.h file
precisely matching the library it will use.

>       However, even assuming that the configure-time problem can be

> solved, this patch by itself seems to be a bad idea because it is

> leaving the config-generated define in newlib.h.  I don't think the

> math routines all include include/ieeefp.h (they probably all get

> include/machine/ieeefp.h), so that the definitions could end up coming

> from different places.


Yes, the autotools stuff would also need changes; right now I'm looking
for feedback on whether this change in the code makes sense. My
meson-based configuration bits do not define this value in newlib.h, so
it ends up only defined in ieeefp.h.

If there's a version of this patch which could make sense to upstream, I
could work on hacking the autotools files, but frankly those are really
hard to work with given the requirement for a specific version of
automake and autoconf which are no longer packaged for my distribution.

>   (Realistically they should never give a different result, but it is

> not good practice have a decision like this in 2 places.)


Yup, one version of the truth is always easier to keep synced than two :-)

>   So to do something like this, it 1) needs a place where all files

> using it can get it (or all files using it would need to be sure they

> included ieeefp.h)


To ensure that applications continue to be able to rely on the symbol
being set correctly, I think we would need to define this in a file
equivalent to newlib.h; I think sys/config.h could work?

> , and 2)  get rid of the configured part of it and the definition in

> newlib.h.  (That is, this patch is the start of a configuration

> simplification, but is incomplete.)


Yup, I don't have the ability to hack the autotools bits though.

>       In addition, this could/would produce warnings on platforms without long 

> double, so an added term would be needed if this bit of it is kept, something like:

>

> #if defined(LDBL_MANT_DIG) && LDBL_MANT_DIG == DBL_MANT_DIG && ...


Fortunately, cpp substitutes 0 for undefined names in these expressions
so this shouldn't be necessary.

-- 
-keith
Keith Packard Sept. 7, 2018, 4:36 a.m. | #5
Joel Sherrill <joel@rtems.org> writes:

> It needs to vary on a per-multilib basis. I am not sure where the logic

> goes for a probe like that would end up in a .h file so it could vary.


Because multilib installs only a single set of header files, I can't see
how this can work without having those header files define this based on
the compilation environment.

-- 
-keith

Patch

diff --git a/newlib/libc/include/ieeefp.h b/newlib/libc/include/ieeefp.h
index 2d6421a4c..71749ccc0 100644
--- a/newlib/libc/include/ieeefp.h
+++ b/newlib/libc/include/ieeefp.h
@@ -143,6 +143,11 @@  typedef union
 
 #endif /* __IEEE_LITTLE_ENDIAN */
 
+#if LDBL_MANT_DIG == DBL_MANT_DIG && LDBL_MIN_EXP == DBL_MIN_EXP && \
+    LDBL_MAX_EXP == DBL_MAX_EXP
+#define _LDBL_EQ_DBL
+#endif
+
 #ifndef _LDBL_EQ_DBL
 
 #ifndef LDBL_MANT_DIG