Opened 6 years ago

Closed 6 years ago

Last modified 4 years ago

#13948 closed defect (fixed)

Let MPIR build with Clang

Reported by: jpflori Owned by: leif
Priority: blocker Milestone: sage-5.12
Component: packages: standard Keywords: spkg mpir clang
Cc: leif, jhpalmieri Merged in: sage-5.12.rc1
Authors: Jeroen Demeyer Reviewers: John Palmieri
Report Upstream: Reported upstream. Developers acknowledge bug. Work issues:
Branch: Commit:
Dependencies: Stopgaps:

Description (last modified by jdemeyer)

There are two problems with the MPIR configure script for Clang:

1) It uses __builtin_malloc() which Clang doesn't have, which is an MPIR upstream bug (up to and including MPIR 2.6.0). The spkg simply removes this test.

2) The "long long reliability" test fails with a linker error. This looks like a Clang bug. The spkg changes the test slightly for Clang.

spkg: http://boxen.math.washington.edu/home/jdemeyer/spkg/mpir-2.6.0.p3.spkg (spkg diff)

apply 13948_clang_doc.patch (documentation only)

Attachments (2)

mpir-2.6.0.p3.diff (8.3 KB) - added by jdemeyer 6 years ago.
13948_clang_doc.patch (1.9 KB) - added by jdemeyer 6 years ago.

Download all attachments as: .zip

Change History (46)

comment:1 Changed 6 years ago by leif

  • Dependencies set to #13137
  • Owner changed from tbd to leif

comment:2 follow-up: Changed 6 years ago by leif

Pinging myself... ;-)

(I do have a trivial patch to acinclude.m4 to make MPIR configure [and of course build and pass its test suite] with clang. Haven't yet submitted it upstream either IIRC.)

comment:3 in reply to: ↑ 2 Changed 6 years ago by leif

Replying to leif:

(I do have a trivial patch to acinclude.m4 to make MPIR configure [and of course build and pass its test suite] with clang. Haven't yet submitted it upstream either IIRC.)

I thinkTM this is the resulting patch to configure (resulting from patching acinclude.m4 in two places):

  • mpir-2.6.0/configure

     
    51325132  rm -f conftest* a.out b.out a.exe a_out.exe
    51335133  cat >conftest.c <<EOF
    51345134/* The following aborts with gcc-4.3.2 on a 64bit system which is an unusable compiler */
    5135 #if defined(__GNUC__) && !defined(__INTEL_COMPILER)
     5135#if defined(__GNUC__) && !defined(__INTEL_COMPILER) && !defined(__clang__)
    51365136int __attribute__((noinline))
    51375137foo(int i)
    51385138{
     
    55885588   Extracted from tests/mpn/t-iord_u.c.  Causes Apple's gcc 3.3 build 1640 and
    55895589   1666 to segfault with e.g., -O2 -mpowerpc64.  */
    55905590
    5591 #ifdef __GNUC__
     5591#if     defined(__GNUC__) && !defined(__clang__)
    55925592typedef unsigned long long t1;typedef t1*t2;
    55935593__inline__ t1 e(t2 rp,t2 up,int n,t1 v0)
    55945594{t1 c,x,r;int i;if(v0){c=1;for(i=1;i<n;i++){x=up[i];r=x+1;rp[i]=r;}}return c;}
     
    63936393  rm -f conftest* a.out b.out a.exe a_out.exe
    63946394  cat >conftest.c <<EOF
    63956395/* The following aborts with gcc-4.3.2 on a 64bit system which is an unusable compiler */
    6396 #if defined(__GNUC__) && !defined(__INTEL_COMPILER)
     6396#if defined(__GNUC__) && !defined(__INTEL_COMPILER) && !defined(__clang__)
    63976397int __attribute__((noinline))
    63986398foo(int i)
    63996399{
     
    68496849   Extracted from tests/mpn/t-iord_u.c.  Causes Apple's gcc 3.3 build 1640 and
    68506850   1666 to segfault with e.g., -O2 -mpowerpc64.  */
    68516851
    6852 #ifdef __GNUC__
     6852#if     defined(__GNUC__) && !defined(__clang__)
    68536853typedef unsigned long long t1;typedef t1*t2;
    68546854__inline__ t1 e(t2 rp,t2 up,int n,t1 v0)
    68556855{t1 c,x,r;int i;if(v0){c=1;for(i=1;i<n;i++){x=up[i];r=x+1;rp[i]=r;}}return c;}

comment:4 Changed 6 years ago by leif

And that's the corresponding one against upstream's acinclude.m4:

  • mpir-2.6.0/acinclude.m4

     
    480480# first see a simple "main()" works, then go on to other checks
    481481GMP_PROG_CC_WORKS_PART([$1], [])
    482482
    483 GMP_PROG_CC_WORKS_PART_MAIN([$1], [gcc-4.3.2 on 64bit is bad , try -O1 or -fno-strict-aliasing for the flags],
    484 [/* The following aborts with gcc-4.3.2 on a 64bit system which is an unusable compiler */
    485 #if defined(__GNUC__) && !defined(__INTEL_COMPILER)
     483GMP_PROG_CC_WORKS_PART_MAIN([$1], [gcc-4.3.2 on 64-bit is bad, try -O1 or -fno-strict-aliasing for the flags],
     484[/* The following aborts with gcc-4.3.2 on a 64-bit system which is an unusable compiler */
     485#if defined(__GNUC__) && !defined(__INTEL_COMPILER) && !defined(__clang__)
    486486int __attribute__((noinline))
    487487foo(int i)
    488488{
     
    566566GMP_PROG_CC_WORKS_PART([$1], [long long reliability test 1],
    567567[/* The following provokes a segfault in the compiler on powerpc-apple-darwin.
    568568   Extracted from tests/mpn/t-iord_u.c.  Causes Apple's gcc 3.3 build 1640 and
    569    1666 to segfault with e.g., -O2 -mpowerpc64. */
     569   1666 to segfault with, e.g., -O2 -mpowerpc64. */
    570570
    571 #ifdef __GNUC__
     571#if     defined(__GNUC__) && !defined(__clang__)
    572572typedef unsigned long long t1;typedef t1*t2;
    573573__inline__ t1 e(t2 rp,t2 up,int n,t1 v0)
    574574{t1 c,x,r;int i;if(v0){c=1;for(i=1;i<n;i++){x=up[i];r=x+1;rp[i]=r;}}return c;}

comment:5 Changed 6 years ago by leif

  • Description modified (diff)

comment:6 Changed 6 years ago by leif

  • Dependencies #13137 deleted

comment:7 follow-up: Changed 6 years ago by jdemeyer

Leif: do you plan to make a proper spkg? Have you reported it upstream? (If both answers are "NO", that's fine, I can do it).

comment:8 Changed 6 years ago by jdemeyer

  • Description modified (diff)

comment:9 in reply to: ↑ 7 ; follow-up: Changed 6 years ago by leif

Replying to jdemeyer:

Leif: do you plan to make a proper spkg?

Plan to: Yes.

Right now: No. (So feel free to do so.)


Have you reported it upstream?

Yes, quite a while ago (on mpir-devel).

I also reported how to fix it, but haven't (yet) submitted the "final" acinclude.m4 patch above IIRC.

(I was actually going to ask Bill about the MPIR release schedule, also because of this.)

comment:10 in reply to: ↑ 9 ; follow-up: Changed 6 years ago by leif

  • Report Upstream changed from N/A to Reported upstream. Developers acknowledge bug.

Replying to leif:

Replying to jdemeyer:

Have you reported it upstream?

Yes, quite a while ago (on mpir-devel).

http://groups.google.com/group/mpir-devel/browse_thread/thread/7888d9a026432871#

(See also http://groups.google.com/group/mpir-devel/browse_thread/thread/c9d70a195f846258#.)


I also reported how to fix it, but haven't (yet) submitted the "final" acinclude.m4 patch above IIRC.

Yep.

comment:11 in reply to: ↑ 10 Changed 6 years ago by leif

  • Report Upstream changed from Reported upstream. Developers acknowledge bug. to Completely fixed; Fix reported upstream

Replying to leif:

I also reported how to fix it, but haven't (yet) submitted the "final" acinclude.m4 patch above IIRC.

Patch now submitted upstream.

comment:12 Changed 6 years ago by jpflori

Any chance to get an spkg to review? Or should I begin working on Cygwin64 support without fearing a future rebase?

comment:13 Changed 6 years ago by jdemeyer

  • Milestone changed from sage-5.11 to sage-5.12

comment:14 Changed 6 years ago by jdemeyer

I am going to move forward on this and simply make a spkg which completely removes the offending test.

comment:15 Changed 6 years ago by jdemeyer

  • Authors set to Jeroen Demeyer
  • Description modified (diff)
  • Report Upstream changed from Completely fixed; Fix reported upstream to Reported upstream. Developers acknowledge bug.

comment:16 Changed 6 years ago by jdemeyer

  • Description modified (diff)

comment:17 Changed 6 years ago by jdemeyer

  • Status changed from new to needs_review

comment:18 Changed 6 years ago by jhpalmieri

I'm using OS X 10.8.5 with Xcode 5.0, which means that gcc --version returns

$ gcc --version
Configured with: --prefix=/Applications/Xcode.app/Contents/Developer/usr --with-gxx-include-dir=/usr/include/c++/4.2.1
Apple LLVM version 5.0 (clang-500.2.76) (based on LLVM 3.3svn)
Target: x86_64-apple-darwin12.5.0
Thread model: posix

The old mpir spkg fails to build for known reasons. The new mpir spkg fails also, though. Log file.

(I also tried unsetting MAKE and it didn't help.)

Last edited 6 years ago by jhpalmieri (previous) (diff)

comment:19 follow-up: Changed 6 years ago by jdemeyer

Please try the following: extract the spkg and try to "manually" configure it by doing:

$ cd mpir-2.6.0.p3/src
$ ./configure

I'm interested to see the output of that, as well as the resulting config.log file.

comment:20 in reply to: ↑ 19 Changed 6 years ago by jhpalmieri

Replying to jdemeyer:

Please try the following: extract the spkg and try to "manually" configure it by doing:

$ cd mpir-2.6.0.p3/src
$ ./configure

I'm interested to see the output of that, as well as the resulting config.log file.

This fails almost immediately because it thinks that clang is a broken compiler: config.log.

comment:21 Changed 6 years ago by jdemeyer

Aah sorry, I meant extract the spkg, apply the patches and then do what you did.

comment:23 Changed 6 years ago by jdemeyer

Hmm, at first sight looks like a genuine compiler bug, but that's hard to say from here...

comment:24 Changed 6 years ago by jdemeyer

Could you try the same (i.e. manual ./configure) with export CC=clang?

comment:25 follow-up: Changed 6 years ago by jhpalmieri

With export CC=clang: config.log and configure output.

By the way, running make seems to succeed after all of these manual runs of ./configure. Once I set CFLAGS as Sage does -- that is,

export CFLAGS="-m32 -O2 -fomit-frame-pointer -mtune=corei7-avx -march=corei7-avx  -g"

then do ./configure and make, it fails. Remove the -m32 flag and it works again. Can we just remove -m32 in the spkg?

comment:26 Changed 6 years ago by jdemeyer

It seems that gcc is identical to clang, so that didn't make a difference.

Can we just remove -m32 in the spkg?

There is no -m32 in the spkg. The problem is that -m64 simply doesn't work, so MPIR's configure falls back to -m32.

comment:27 in reply to: ↑ 25 Changed 6 years ago by jdemeyer

Replying to jhpalmieri:

Remove the -m32 flag and it works again.

Exactly what does "it" refer to here? Did you manage to ./configure MPIR as 64-bit?

Last edited 6 years ago by jdemeyer (previous) (diff)

comment:28 Changed 6 years ago by jhpalmieri

I mean that if I do

export CFLAGS="-O2 -fomit-frame-pointer -mtune=corei7-avx -march=corei7-avx  -g"
./configure
make

it finishes without error. (I haven't tried a full Sage build on top of this, of course.) If CFLAGS also includes -m32, then make fails.

In the spkg, we could have some code like

  • spkg-install

    diff --git a/spkg-install b/spkg-install
    a b  
    275275echo "  ABI:     $user_abi"
    276276echo "  (CPP, CPPFLAGS, CXX and CXXFLAGS are listed below; these don't get modified.)"
    277277
     278# Fix bug with Xcode 5.0 on Darwin: mpir doesn't build using the -m32
     279# flag with the gcc from this version of Xcode.
     280if [ blah ]; then        # using Xcode 5.0 on Darwin
     281    mpir_cflags=...      # remove -m32 from $mpir_cflags
     282fi
     283
    278284# Finally: use MPIR's flags, plus those required by Sage for the
    279285# package to build properly, plus those specified by the user.
    280286CFLAGS="$mpir_cflags $required_cflags $user_cflags"

I have no idea if this is actually a good idea, though.

comment:29 Changed 6 years ago by jhpalmieri

Now I have tried a full Sage build on top of this, and the build finishes. I guess that isn't too surprising, because mpir gets rebuilt with Sage's GCC anyway.

I'll try again with a more carefully constructed spkg, and I'll also run the full test suite. Here's my actual proposed patch to spkg-install:

  • spkg-install

    diff --git a/spkg-install b/spkg-install
    a b  
    275275echo "  ABI:     $user_abi"
    276276echo "  (CPP, CPPFLAGS, CXX and CXXFLAGS are listed below; these don't get modified.)"
    277277
     278# Fix bug with Xcode 5.0 or later on Darwin: mpir doesn't build using
     279# the -m32 flag with the gcc from this version of Xcode, so remove
     280# -m32 from CFLAGS.
     281if [ "$UNAME" = "Darwin" ]; then
     282    # Xcode version detection, etc., taken from spkg/base/prereq
     283    XCODE_VERS=`xcodebuild -version 2> /dev/null | grep Xcode | sed -e 's/[A-Za-z ]//g'`
     284    if [ -z $XCODE_VERS ]; then
     285        XCODE_VERS="2"
     286    fi
     287    XCODE_VERS_MAJOR=`echo $XCODE_VERS | cut '-d.' -f1`
     288    if [ $XCODE_VERS_MAJOR -gt 4 ]; then
     289        mpir_cflags=`echo $mpir_cflags | sed 's/-m32//'` # remove -m32
     290    fi
     291fi
     292
    278293# Finally: use MPIR's flags, plus those required by Sage for the
    279294# package to build properly, plus those specified by the user.
    280295CFLAGS="$mpir_cflags $required_cflags $user_cflags"

comment:30 Changed 6 years ago by jhpalmieri

I used an spkg with this patch, took an otherwise clean tarball for 5.12.rc0, did export SAGE_CHECK=yes. Then make ptestlong was completely successful.

comment:31 follow-up: Changed 6 years ago by jdemeyer

What does

$ gcc -E -dM -x c /dev/null |grep -i clang

say on that machine?

Instead of fiddling with CFLAGS, I propose to try to "fix" the test causing the 64-bit build to fail.

Last edited 6 years ago by jdemeyer (previous) (diff)

comment:32 Changed 6 years ago by jdemeyer

I prepared a new version of the spkg with a simple fix, can you test it? Simply try to install the spkg normally through Sage. If that fails, send me the file configure-empty.log

Last edited 6 years ago by jdemeyer (previous) (diff)

Changed 6 years ago by jdemeyer

comment:33 Changed 6 years ago by jdemeyer

  • Description modified (diff)

comment:34 Changed 6 years ago by jdemeyer

  • Description modified (diff)

comment:35 in reply to: ↑ 31 ; follow-up: Changed 6 years ago by jhpalmieri

Replying to jdemeyer:

What does

$ gcc -E -dM -x c /dev/null |grep -i clang

say on that machine?

gcc -E -dM -x c /dev/null |grep -i clang
#define __VERSION__ "4.2.1 Compatible Apple LLVM 5.0 (clang-500.2.76)"
#define __clang__ 1
#define __clang_major__ 5
#define __clang_minor__ 0
#define __clang_patchlevel__ 0
#define __clang_version__ "5.0 (clang-500.2.76)"

The new spkg seems to work. I'm trying a full Sage build to make sure.

comment:36 in reply to: ↑ 35 Changed 6 years ago by jdemeyer

Replying to jhpalmieri:

gcc -E -dM -x c /dev/null |grep -i clang
#define __VERSION__ "4.2.1 Compatible Apple LLVM 5.0 (clang-500.2.76)"
#define __clang__ 1
#define __clang_major__ 5
#define __clang_minor__ 0
#define __clang_patchlevel__ 0
#define __clang_version__ "5.0 (clang-500.2.76)"

This confirms my suspicion that gcc really is Clang.

comment:37 Changed 6 years ago by jdemeyer

  • Description modified (diff)

comment:38 follow-up: Changed 6 years ago by jhpalmieri

  • Priority changed from major to blocker
  • Reviewers set to John Palmieri
  • Status changed from needs_review to positive_review

The doc patch is certainly fine. As far as the spkg goes, first, is it worth it to rebase the patch files so we don't get the "offset -128 lines" messages? Second, it seems to work, and I understand in principle what the changes are, but I'm not an expert in acinclude.m4 files. I think I'll give it a positive review anyway: this ticket is very important (I think a blocker) since anyone with OS X and the most recent Xcode won't be able to build Sage without it.

comment:39 in reply to: ↑ 38 Changed 6 years ago by jdemeyer

  • Milestone changed from sage-5.13 to sage-5.12

Replying to jhpalmieri:

As far as the spkg goes, first, is it worth it to rebase the patch files so we don't get the "offset -128 lines" messages?

In my opinion, no. Offsets are harmless.

this ticket is very important (I think a blocker)

I agree. In fact, it justifies an extra rc for sage-5.12.

Changed 6 years ago by jdemeyer

comment:40 Changed 6 years ago by jhpalmieri

By the way, thank you for taking care of this so quickly after my message to sage-devel. It wasn't so urgent until Apple released Xcode 5.0.

comment:41 Changed 6 years ago by jdemeyer

  • Merged in set to sage-5.12.rc1
  • Resolution set to fixed
  • Status changed from positive_review to closed

comment:42 in reply to: ↑ description ; follow-up: Changed 4 years ago by leif

Replying to jpflori:

There are two problems with the MPIR configure script for Clang:

1) It uses __builtin_malloc() which Clang doesn't have, which is an MPIR upstream bug (up to and including MPIR 2.6.0). The spkg simply removes this test.

2) The "long long reliability" test fails with a linker error. This looks like a Clang bug.

The latter one apparently now hits GCC (5.0.1 RC1) as well... B)

(From config.log it also seems that the __builtin_alloca() availability test fails for a different reason, but that's harmless.)

comment:43 in reply to: ↑ 42 ; follow-up: Changed 4 years ago by leif

Replying to leif:

2) The "long long reliability" test fails with a linker error. This looks like a Clang bug.

The latter one apparently now hits GCC (5.0.1 RC1) as well... B)

The difference appears to be

#define __GNUC_STDC_INLINE__ 1

in GCC 5.x vs.

#define __GNUC_GNU_INLINE__ 1

in older versions. (In each case the other macro isn't defined, corresponding to 0.)

comment:44 in reply to: ↑ 43 Changed 4 years ago by leif

Replying to leif:

Replying to leif:

2) The "long long reliability" test fails with a linker error. This looks like a Clang bug.

The latter one apparently now hits GCC (5.0.1 RC1) as well... B)

The difference appears to be

#define __GNUC_STDC_INLINE__ 1

in GCC 5.x vs.

#define __GNUC_GNU_INLINE__ 1

in older versions. (In each case the other macro isn't defined, corresponding to 0.)

Follow-up: #18247

Note: See TracTickets for help on using tickets.