Re: [PATCH] newlib: switch to autoconf long double wider macro

[C Howland <cc1964t@gmail.com>]

> ------------------------------
> *From:* Newlib <newlib-bounces+craig.howland=caci.com@sourceware.org> on
> behalf of Corinna Vinschen <vinschen@redhat.com>
> *Sent:* Friday, January 21, 2022 8:27 AM
> *To:* newlib@sourceware.org <newlib@sourceware.org>
> *Subject:* Re: [PATCH] newlib: switch to autoconf long double wider macro
>
>
>
> On Jan 21 07:39, Mike Frysinger wrote:
> > On 21 Jan 2022 12:27, Corinna Vinschen wrote:
> > > On Jan 21 00:04, Mike Frysinger wrote:
> > > > Now that we require a recent version of autoconf, we can rely on this
> > > > macro existing.  It has inverted semantics from the existing test (it
> > > > looks for "is wider" instead of "is equal"), so we have to invert the
> > > > check when creating our _LDBL_EQ_DBL.
> > > > ---
> > > >  newlib/configure    | 72
> +++++++++++++++++++++++++++++----------------
> > > >  newlib/configure.ac | 22 ++------------
> > > >  newlib/newlib.hin   |  4 +++
> > > >  3 files changed, 53 insertions(+), 45 deletions(-)
> > >
> > > Looks right to me, please push.
> >
> > for posterity, i'll note that autoconf uses a different (more
> comprehensive)
> > testing method that ultimately arrives at a diff answer than what newlib
> is
> > atm.  for aarch64, _LDBL_EQ_DBL is now defined when it wasn't before.
> >
> > newlib today is doing:
> > #if DBL_MANT_DIG == LDBL_MANT_DIG && \
> >     LDBL_MIN_EXP == DBL_MIN_EXP   && \
> >     LDBL_MAX_EXP == DBL_MAX_EXP
> >   #define _LDBL_EQ_DBL
> >  #else
> >   #error "LDBL != DBL"
> > #endif
> >
> > while autoconf is doing:
> >        (0 < ((DBL_MAX_EXP < LDBL_MAX_EXP)
> >           + (DBL_MANT_DIG < LDBL_MANT_DIG)
> >           - (LDBL_MAX_EXP < DBL_MAX_EXP)
> >           - (LDBL_MANT_DIG < DBL_MANT_DIG)))
> >        && (int) LDBL_EPSILON == 0
> > -mike
>
> Erm... that's kind of weird.  The newlib expression doesn't look wrong.
>
> So, out of curiosity of somebody not being familiar with aarch64,
> why is that?
>
>
> Corinna
>
>
>
Major problem, as that result is wrong, as they are definitely not equal:
$ aarch64-none-elf-cpp -dM null.c | grep DBL | sort
#define __DBL_DECIMAL_DIG__ 17
#define __DBL_DENORM_MIN__
((double)4.94065645841246544176568792868221372e-324L)
#define __DBL_DIG__ 15
#define __DBL_EPSILON__ ((double)2.22044604925031308084726333618164062e-16L)
#define __DBL_HAS_DENORM__ 1
#define __DBL_HAS_INFINITY__ 1
#define __DBL_HAS_QUIET_NAN__ 1
#define __DBL_MANT_DIG__ 53
#define __DBL_MAX_10_EXP__ 308
#define __DBL_MAX__ ((double)1.79769313486231570814527423731704357e+308L)
#define __DBL_MAX_EXP__ 1024
#define __DBL_MIN_10_EXP__ (-307)
#define __DBL_MIN__ ((double)2.22507385850720138309023271733240406e-308L)
#define __DBL_MIN_EXP__ (-1021)
#define __DBL_NORM_MAX__
((double)1.79769313486231570814527423731704357e+308L)
#define __LDBL_DECIMAL_DIG__ 36
#define __LDBL_DENORM_MIN__ 6.47517511943802511092443895822764655e-4966L
#define __LDBL_DIG__ 33
#define __LDBL_EPSILON__ 1.92592994438723585305597794258492732e-34L
#define __LDBL_HAS_DENORM__ 1
#define __LDBL_HAS_INFINITY__ 1
#define __LDBL_HAS_QUIET_NAN__ 1
#define __LDBL_MANT_DIG__ 113
#define __LDBL_MAX_10_EXP__ 4932
#define __LDBL_MAX__ 1.18973149535723176508575932662800702e+4932L
#define __LDBL_MAX_EXP__ 16384
#define __LDBL_MIN_10_EXP__ (-4931)
#define __LDBL_MIN__ 3.36210314311209350626267781732175260e-4932L
#define __LDBL_MIN_EXP__ (-16381)
#define __LDBL_NORM_MAX__ 1.18973149535723176508575932662800702e+4932L

     I am confused by the patch.  The opening statement says "Now that we
require a recent version of autoconf, we can rely on this macro existing."
But then it proceeds to define a test for it.  If it already exists, why is
a test being defined?  (Or is it just meaning to say that there is now, all
this time later, a macro name that has become a de-facto standard to use?)
     And a question about the test.  There are two main segments in it:

+           long double const a[] =
+             {
+                0.0L, DBL_MIN, DBL_MAX, DBL_EPSILON,
+                LDBL_MIN, LDBL_MAX, LDBL_EPSILON
+             };
+           long double
+           f (long double x)
+           {
+              return ((x + (unsigned long int) 10) * (-1 / x) + a[0]
+                       + (x ? f (x) : 'c'));
+           }
+
+int
+main ()
+{
+static int test_array [1 - 2 * !((0 < ((DBL_MAX_EXP < LDBL_MAX_EXP)
+                  + (DBL_MANT_DIG < LDBL_MANT_DIG)
+                  - (LDBL_MAX_EXP < DBL_MAX_EXP)
+                  - (LDBL_MANT_DIG < DBL_MANT_DIG)))
+           && (int) LDBL_EPSILON == 0
+         )];
+test_array [0] = 0;
+return test_array [0];
+
+  ;
+  return 0;
+}

What's the first part doing?  Only the second part (main) is performing the
size comparison.
     Lastly, I tried this check with my GCC 10.2.0 aarch64 and it does
compile.  (I hand-made a file to test.  I did not make a configure file to
run.)  I then changed the test_array definition to make it compare LDBL
against LDBL (to force them to be equal) and the compilation does fail, as
expected.  So then what's going on that for Mike aarch64 _LDBL_EQ_DBL is
now defined when it wasn't before?  The answer cannot be both different and
also still correct.  It is critical that _LDBL_EQ_DBL only be defined when
they are indeed the same size, as that gates using the double routines as
being long double.
     Now a secondary item:  the (int) LDBL_EPSILON == 0 term in the check
is degenerate and does nothing.  (By rule (from the standard) the cast to
int discards the fraction so it will always be 0 and it becomes ... && 1.)
   (In a minor quibble I have to disagree with the statement that the new
check is more comprehensive.  In a rough way of looking at it they are both
checking 3 values.  But as noted the one term in the new check does
nothing, so it really is only checking 2 values.  I actually made the
original check and had a positive reason for checking both min and max
exponent--although at the moment I'm not remembering what that reason was.)
                                          Craig