Build failure on powerpc64

classic Classic list List threaded Threaded
6 messages Options
Reply | Threaded
Open this post in threaded view
|

Build failure on powerpc64

Tom "spot" Callaway
Hi R folks,

Went to build R 3.6.2 for Fedora/EPEL and got failures across the board.

Disabling the test suite for all non-intel architectures resolves most of
the failures, but powerpc64 dies in the compiler, specifically here:

gcc -m64  -I../../src/extra/xdr -I. -I../../src/include -I../../src/include
 -I/usr/local/include -I../../src/nmath -DHAVE_CONFIG_H   -fopenmp -fPIC
 -O2 -g -pipe -Wall -Werror=format-security -Wp,-D_FORTIFY_SOURCE=2
-Wp,-D_GLIBCXX_ASSERTIONS -fexceptions -fstack-protector-strong
-grecord-gcc-switches -specs=/usr/lib/rpm/redhat/redhat-hardened-cc1
-specs=/usr/lib/rpm/redhat/redhat-annobin-cc1 -m64 -mcpu=power8
-mtune=power8 -fasynchronous-unwind-tables -fstack-clash-protection  -c
arithmetic.c -o arithmetic.o
arithmetic.c:180:26: error: initializer element is not constant
  180 | static LDOUBLE q_1_eps = (1 / LDBL_EPSILON);
      |                          ^
make[3]: *** [../../Makeconf:124: arithmetic.o] Error 1

Took me a bit to figure out why, but this is happening because on
powerpc64, gcc is compiled with -mlong-double-128, and the long double
format used on PPC is IBM's 128bit long double which is two doubles added
together. As a result, gcc can't safely do const assignments to long
doubles on ppc64, so it dies there.

The fix is easy enough, do not try to assign a value to a static long
double on ppc64.
--- ./src/main/arithmetic.c.orig        2019-12-12 18:30:12.416334062 +0000
+++ ./src/main/arithmetic.c     2019-12-12 18:30:44.966334062 +0000
@@ -179,7 +179,10 @@ void attribute_hidden InitArithmetic()
 #endif
 }

-#if HAVE_LONG_DOUBLE && (SIZEOF_LONG_DOUBLE > SIZEOF_DOUBLE)
+/* PowerPC 64 (when gcc has -mlong-double-128) breaks here because
+ * of issues constant folding 128bit IBM long doubles.
+ */
+#if HAVE_LONG_DOUBLE && (SIZEOF_LONG_DOUBLE > SIZEOF_DOUBLE) && !__PPC64__
 static LDOUBLE q_1_eps = 1 / LDBL_EPSILON;
 #else
 static double  q_1_eps = 1 / DBL_EPSILON;

Hope that helps someone else.

Tom

        [[alternative HTML version deleted]]

______________________________________________
[hidden email] mailing list
https://stat.ethz.ch/mailman/listinfo/r-devel
Reply | Threaded
Open this post in threaded view
|

Re: Build failure on powerpc64

Martin Maechler
>>>>> Tom Callaway
>>>>>     on Thu, 12 Dec 2019 14:21:10 -0500 writes:

    > Hi R folks,

    > Went to build R 3.6.2 for Fedora/EPEL and got failures across the board.

    > Disabling the test suite for all non-intel architectures resolves most of
    > the failures, but powerpc64 dies in the compiler, specifically here:

    > gcc -m64  -I../../src/extra/xdr -I. -I../../src/include -I../../src/include
    > -I/usr/local/include -I../../src/nmath -DHAVE_CONFIG_H   -fopenmp -fPIC
    > -O2 -g -pipe -Wall -Werror=format-security -Wp,-D_FORTIFY_SOURCE=2
    > -Wp,-D_GLIBCXX_ASSERTIONS -fexceptions -fstack-protector-strong
    > -grecord-gcc-switches -specs=/usr/lib/rpm/redhat/redhat-hardened-cc1
    > -specs=/usr/lib/rpm/redhat/redhat-annobin-cc1 -m64 -mcpu=power8
    > -mtune=power8 -fasynchronous-unwind-tables -fstack-clash-protection  -c
    > arithmetic.c -o arithmetic.o
    > arithmetic.c:180:26: error: initializer element is not constant
    > 180 | static LDOUBLE q_1_eps = (1 / LDBL_EPSILON);
    > |                          ^
    > make[3]: *** [../../Makeconf:124: arithmetic.o] Error 1

    > Took me a bit to figure out why, but this is happening because on
    > powerpc64, gcc is compiled with -mlong-double-128, and the long double
    > format used on PPC is IBM's 128bit long double which is two doubles added
    > together. As a result, gcc can't safely do const assignments to long
    > doubles on ppc64, so it dies there.

    > The fix is easy enough, do not try to assign a value to a static long
    > double on ppc64.
    > --- ./src/main/arithmetic.c.orig        2019-12-12 18:30:12.416334062 +0000
    > +++ ./src/main/arithmetic.c     2019-12-12 18:30:44.966334062 +0000
    > @@ -179,7 +179,10 @@ void attribute_hidden InitArithmetic()
    > #endif
    > }

    > -#if HAVE_LONG_DOUBLE && (SIZEOF_LONG_DOUBLE > SIZEOF_DOUBLE)
    > +/* PowerPC 64 (when gcc has -mlong-double-128) breaks here because
    > + * of issues constant folding 128bit IBM long doubles.
    > + */
    > +#if HAVE_LONG_DOUBLE && (SIZEOF_LONG_DOUBLE > SIZEOF_DOUBLE) && !__PPC64__
    > static LDOUBLE q_1_eps = 1 / LDBL_EPSILON;
    > #else
    > static double  q_1_eps = 1 / DBL_EPSILON;


    > Hope that helps someone else.
    > Tom

Thank you, Tom.  That is "interesting"  ...

The piece in question had been added by me,
------------------------------------------------------------------------
r77193 | maechler | 2019-09-18 13:21:49 +0200 (Wed, 18 Sep 2019) | 1 line

 x %% +/- Inf  now typically return non-NaN; fix the "FIXME" in C
------------------------------------------------------------------------
in order to use double precision in order to deal with finite cases
close to Inf, etc.

IIUC, your proposed patch is really a workaround a bug on PPC64 ?

But note the check on LONG_DOUBLE is not the only one in R's
sources: Via 'grep' in src/main/*.?  I also see

connections.c4285:#if HAVE_LONG_DOUBLE && (SIZEOF_LONG_DOUBLE > SIZEOF_DOUBLE)
connections.c4329:#if HAVE_LONG_DOUBLE
connections.c4379:#if HAVE_LONG_DOUBLE && (SIZEOF_LONG_DOUBLE > SIZEOF_DOUBLE)
connections.c4514:#if HAVE_LONG_DOUBLE && (SIZEOF_LONG_DOUBLE > SIZEOF_DOUBLE)
connections.c4592:#if HAVE_LONG_DOUBLE && (SIZEOF_LONG_DOUBLE > SIZEOF_DOUBLE)
format.c250:#if defined(HAVE_LONG_DOUBLE) && (SIZEOF_LONG_DOUBLE > SIZEOF_DOUBLE)
format.c285:#if defined(HAVE_LONG_DOUBLE) && (SIZEOF_LONG_DOUBLE > SIZEOF_DOUBLE)
format.c339:#if defined(HAVE_LONG_DOUBLE) && (SIZEOF_LONG_DOUBLE > SIZEOF_DOUBLE)

Do they also need protection against this PPC64 bug ?

Best,

Martin Maechler
ETH Zurich and R Core Team
______________________________________________
[hidden email] mailing list
https://stat.ethz.ch/mailman/listinfo/r-devel
Reply | Threaded
Open this post in threaded view
|

Re: Build failure on powerpc64

Tom "spot" Callaway
An excellent question. It is important to remember two key facts:

1. With gcc on ppc64, long doubles exist, they can be used, just not safely
as constants (and maybe they still can be used safely under specific
conditions?).
2. I am not an expert in either PowerPC64 or gcc. :)

Looking at connections.c, we have (in order):
  * handling long double as a valid case in a switch statement checking size
  * adding long double as a field in the u union define
  * handling long double as a valid case in a switch statement checking size
  * handling long double as a valid case in a switch statement checking size
  * memcpy from the address of a long double

In format.c, we have (in order):
  * conditionally creating private_nearbyintl for R_nearbyintl
  * defining a static const long double tbl[]
  * use exact scaling factor in long double precision

For most of these, it seems safe to leave them as is for ppc64. I would
have thought that the gcc compiler might have had issue with:

connections.c:
              static long double ld1;
                for (i = 0, j = 0; i < len; i++, j += size) {
                    ld1 = (long double) REAL(object)[i];

format.c:
   static const long double tbl[] =

... but it doesn't. Perhaps the original code at issue:

arithmetic.c:
   static LDOUBLE q_1_eps = 1 / LDBL_EPSILON;

only makes gcc unhappy because of the very large value trying to be stored
in the static long double, which would make it span the "folded double" on
that architecture.

*****

It seems that the options are:

A) Patch the one place where the compiler determines it is not safe to use
a static long double on ppc64.
B) Treat PPC64 as a platform where it is never safe to use a static long
double

FWIW, I did run the test suite after applying my patch and all of the tests
pass on ppc64.

Tom


On Fri, Dec 13, 2019 at 5:44 AM Martin Maechler <[hidden email]>
wrote:

> >>>>> Tom Callaway
> >>>>>     on Thu, 12 Dec 2019 14:21:10 -0500 writes:
>
>     > Hi R folks,
>
>     > Went to build R 3.6.2 for Fedora/EPEL and got failures across the
> board.
>
>     > Disabling the test suite for all non-intel architectures resolves
> most of
>     > the failures, but powerpc64 dies in the compiler, specifically here:
>
>     > gcc -m64  -I../../src/extra/xdr -I. -I../../src/include
> -I../../src/include
>     > -I/usr/local/include -I../../src/nmath -DHAVE_CONFIG_H   -fopenmp
> -fPIC
>     > -O2 -g -pipe -Wall -Werror=format-security -Wp,-D_FORTIFY_SOURCE=2
>     > -Wp,-D_GLIBCXX_ASSERTIONS -fexceptions -fstack-protector-strong
>     > -grecord-gcc-switches -specs=/usr/lib/rpm/redhat/redhat-hardened-cc1
>     > -specs=/usr/lib/rpm/redhat/redhat-annobin-cc1 -m64 -mcpu=power8
>     > -mtune=power8 -fasynchronous-unwind-tables -fstack-clash-protection
> -c
>     > arithmetic.c -o arithmetic.o
>     > arithmetic.c:180:26: error: initializer element is not constant
>     > 180 | static LDOUBLE q_1_eps = (1 / LDBL_EPSILON);
>     > |                          ^
>     > make[3]: *** [../../Makeconf:124: arithmetic.o] Error 1
>
>     > Took me a bit to figure out why, but this is happening because on
>     > powerpc64, gcc is compiled with -mlong-double-128, and the long
> double
>     > format used on PPC is IBM's 128bit long double which is two doubles
> added
>     > together. As a result, gcc can't safely do const assignments to long
>     > doubles on ppc64, so it dies there.
>
>     > The fix is easy enough, do not try to assign a value to a static long
>     > double on ppc64.
>     > --- ./src/main/arithmetic.c.orig        2019-12-12
> 18:30:12.416334062 +0000
>     > +++ ./src/main/arithmetic.c     2019-12-12 18:30:44.966334062 +0000
>     > @@ -179,7 +179,10 @@ void attribute_hidden InitArithmetic()
>     > #endif
>     > }
>
>     > -#if HAVE_LONG_DOUBLE && (SIZEOF_LONG_DOUBLE > SIZEOF_DOUBLE)
>     > +/* PowerPC 64 (when gcc has -mlong-double-128) breaks here because
>     > + * of issues constant folding 128bit IBM long doubles.
>     > + */
>     > +#if HAVE_LONG_DOUBLE && (SIZEOF_LONG_DOUBLE > SIZEOF_DOUBLE) &&
> !__PPC64__
>     > static LDOUBLE q_1_eps = 1 / LDBL_EPSILON;
>     > #else
>     > static double  q_1_eps = 1 / DBL_EPSILON;
>
>
>     > Hope that helps someone else.
>     > Tom
>
> Thank you, Tom.  That is "interesting"  ...
>
> The piece in question had been added by me,
> ------------------------------------------------------------------------
> r77193 | maechler | 2019-09-18 13:21:49 +0200 (Wed, 18 Sep 2019) | 1 line
>
>  x %% +/- Inf  now typically return non-NaN; fix the "FIXME" in C
> ------------------------------------------------------------------------
> in order to use double precision in order to deal with finite cases
> close to Inf, etc.
>
> IIUC, your proposed patch is really a workaround a bug on PPC64 ?
>
> But note the check on LONG_DOUBLE is not the only one in R's
> sources: Via 'grep' in src/main/*.?  I also see
>
> connections.c 4285:#if HAVE_LONG_DOUBLE && (SIZEOF_LONG_DOUBLE >
> SIZEOF_DOUBLE)
> connections.c 4329:#if HAVE_LONG_DOUBLE
> connections.c 4379:#if HAVE_LONG_DOUBLE && (SIZEOF_LONG_DOUBLE >
> SIZEOF_DOUBLE)
> connections.c 4514:#if HAVE_LONG_DOUBLE && (SIZEOF_LONG_DOUBLE >
> SIZEOF_DOUBLE)
> connections.c 4592:#if HAVE_LONG_DOUBLE && (SIZEOF_LONG_DOUBLE >
> SIZEOF_DOUBLE)
> format.c 250:#if defined(HAVE_LONG_DOUBLE) && (SIZEOF_LONG_DOUBLE >
> SIZEOF_DOUBLE)
> format.c 285:#if defined(HAVE_LONG_DOUBLE) && (SIZEOF_LONG_DOUBLE >
> SIZEOF_DOUBLE)
> format.c 339:#if defined(HAVE_LONG_DOUBLE) && (SIZEOF_LONG_DOUBLE >
> SIZEOF_DOUBLE)
>
> Do they also need protection against this PPC64 bug ?
>
> Best,
>
> Martin Maechler
> ETH Zurich and R Core Team
>

        [[alternative HTML version deleted]]

______________________________________________
[hidden email] mailing list
https://stat.ethz.ch/mailman/listinfo/r-devel
Reply | Threaded
Open this post in threaded view
|

Re: Build failure on powerpc64

Serguei Sokol
Le 13/12/2019 à 17:06, Tom Callaway a écrit :
> arithmetic.c:
>     static LDOUBLE q_1_eps = 1 / LDBL_EPSILON;
Just a thought: can it be that it's "1" which is at the origin of
compiler complaint?
In this case, would the syntax "1.L" be sufficient to keep it calm?

Serguei.

______________________________________________
[hidden email] mailing list
https://stat.ethz.ch/mailman/listinfo/r-devel
Reply | Threaded
Open this post in threaded view
|

Re: Build failure on powerpc64

Tom "spot" Callaway
No, that does not change the issue:

arithmetic.c:180:26: error: initializer element is not constant
  180 | static LDOUBLE q_1_eps = 1.L / LDBL_EPSILON;

Tom

On Fri, Dec 13, 2019 at 11:56 AM Serguei Sokol <[hidden email]>
wrote:

> Le 13/12/2019 à 17:06, Tom Callaway a écrit :
> > arithmetic.c:
> >     static LDOUBLE q_1_eps = 1 / LDBL_EPSILON;
> Just a thought: can it be that it's "1" which is at the origin of
> compiler complaint?
> In this case, would the syntax "1.L" be sufficient to keep it calm?
>
> Serguei.
>
>

        [[alternative HTML version deleted]]

______________________________________________
[hidden email] mailing list
https://stat.ethz.ch/mailman/listinfo/r-devel
Reply | Threaded
Open this post in threaded view
|

Re: Build failure on powerpc64

Martin Maechler
In reply to this post by Tom "spot" Callaway
>>>>> Tom Callaway
>>>>>     on Fri, 13 Dec 2019 11:06:25 -0500 writes:

    > An excellent question. It is important to remember two key
    > facts:

    > 1. With gcc on ppc64, long doubles exist, they can
    > be used, just not safely as constants (and maybe they
    > still can be used safely under specific conditions?).

    > 2. I am not an expert in either PowerPC64 or gcc. :)

    > Looking at connections.c, we have (in order):
    > * handling long double as a valid case in a switch statement checking size
    > * adding long double as a field in the u union define
    > * handling long double as a valid case in a switch statement checking size
    > * handling long double as a valid case in a switch statement checking size
    > * memcpy from the address of a long double

    > In format.c, we have (in order):
    > * conditionally creating private_nearbyintl for R_nearbyintl
    > * defining a static const long double tbl[]
    > * use exact scaling factor in long double precision

    > For most of these, it seems safe to leave them as is for ppc64. I would
    > have thought that the gcc compiler might have had issue with:

    > connections.c:
    > static long double ld1;
    > for (i = 0, j = 0; i < len; i++, j += size) {
    > ld1 = (long double) REAL(object)[i];

    > format.c:
    > static const long double tbl[] =

    > ... but it doesn't. Perhaps the original code at issue:

    > arithmetic.c:
    > static LDOUBLE q_1_eps = 1 / LDBL_EPSILON;

    > only makes gcc unhappy because of the very large value trying to be stored
    > in the static long double, which would make it span the "folded double" on
    > that architecture.

    > *****

    > It seems that the options are:

    > A) Patch the one place where the compiler determines it is not safe to use
    > a static long double on ppc64.
    > B) Treat PPC64 as a platform where it is never safe to use a static long
    > double

    > FWIW, I did run the test suite after applying my patch and all of the tests
    > pass on ppc64.

    > Tom

Thank you, Tom.
You were right... and only  A)  is needed.

In the mean time I've also been CC'ed in a corresponding debian
bug report on the exact same architecture.

In the end, after explanation and recommendation by Tomas
Kalibera, I've committed a slightly better change to R's
sources, both in the R-devel (trunk) and the "R-3.6.x patched"
branch:  Via a macro, it continues to use long double also for
the PPC 64 in this case:

$ svn diff -c77587
Index: src/main/arithmetic.c
===================================================================
--- src/main/arithmetic.c (Revision 77586)
+++ src/main/arithmetic.c (Revision 77587)
@@ -176,8 +176,14 @@
 #endif
 }
 
+
 #if HAVE_LONG_DOUBLE && (SIZEOF_LONG_DOUBLE > SIZEOF_DOUBLE)
+# ifdef __PPC64__
+ // PowerPC 64 (when gcc has -mlong-double-128) fails constant folding with LDOUBLE
+#  define q_1_eps (1 / LDBL_EPSILON)
+# else
 static LDOUBLE q_1_eps = 1 / LDBL_EPSILON;
+# endif
 #else
 static double  q_1_eps = 1 / DBL_EPSILON;
 #endif

------------- ------------- -------------

Thank you (and everybody else involved) once more,
Martin

______________________________________________
[hidden email] mailing list
https://stat.ethz.ch/mailman/listinfo/r-devel