Opened 7 years ago

Closed 7 years ago

Last modified 6 years ago

#19233 closed defect (fixed)

ecm doesn't build with Xcode 7.0

Reported by: jhpalmieri Owned by:
Priority: critical Milestone: sage-6.9
Component: packages: standard Keywords:
Cc: fbissey, zimmerma Merged in:
Authors: John Palmieri, Volker Braun Reviewers: John Palmieri, Volker Braun
Report Upstream: N/A Work issues:
Branch: 1f3e8dc (Commits, GitHub, GitLab) Commit:
Dependencies: Stopgaps:

Status badges

Description

As the summary says. Log file.

Change History (22)

comment:1 Changed 7 years ago by fbissey

  • Cc fbissey added

comment:2 Changed 7 years ago by fbissey

  • Cc zimmerma added

Hi Paul,

this is different from the build failure that is currently on the gmp-ecm tracker. By the look of things it is something that has been going on for a while but xcode 7.0 made it break. With the previous version of xcode we had this kind of message

/bin/sh ../libtool   --mode=compile gcc  -march=corei7 -mtune=corei7-avx -g -O3  -fPIC -c -o mulredc1.lo mulredc1.s
libtool: compile:  gcc -march=corei7 -mtune=corei7-avx -g -O3 -fPIC -c mulredc1.s -o mulredc1.o
/bin/sh ../libtool   --mode=compile gcc  -march=corei7 -mtune=corei7-avx -g -O3  -fPIC -c -o mulredc2.lo mulredc2.s
libtool: compile:  gcc -march=corei7 -mtune=corei7-avx -g -O3 -fPIC -c mulredc2.s -o mulredc2.o
mulredc2.s:40:Alignment too large: 15. assumed.
mulredc2.s:150:Alignment too large: 15. assumed.
/bin/sh ../libtool   --mode=compile gcc  -march=corei7 -mtune=corei7-avx -g -O3  -fPIC -c -o mulredc3.lo mulredc3.s
libtool: compile:  gcc -march=corei7 -mtune=corei7-avx -g -O3 -fPIC -c mulredc3.s -o mulredc3.o
mulredc3.s:40:Alignment too large: 15. assumed.
mulredc3.s:26:Alignment too large: 15. assumed.

As can be seen in the log attached to the ticket "it stopped" assuming and decided it was invalid.

comment:3 follow-up: Changed 7 years ago by zimmerma

I see two ways to solve this issue:

1) the easy way with --disable-asm-redc. There should not be too much speed regression (if any).

2) the hard way. Maybe the alignment values should be replaced by something different, or simply removed. If GMP compiles successfully on the same machine, you could copy what GMP does. It seems other people hit the same issue, but I say no definite solution with google.

Paul

comment:4 in reply to: ↑ 3 Changed 7 years ago by jhpalmieri

Replying to zimmerma:

I see two ways to solve this issue:

1) the easy way with --disable-asm-redc. There should not be too much speed regression (if any).

This works for me, or at least the package installs and passes its test suite.

comment:5 follow-up: Changed 7 years ago by vbraun

Afaik:

  • gcc/gas uses byte count on most platforms: .align 64
  • llvm/clang use log2: .align 6

Looks like xcode 7 now errors out instead of limiting it to 2^15. Using .p2align would be unambiguous (and the latter behavior).

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

Replying to vbraun:

Afaik:

  • gcc/gas uses byte count on most platforms: .align 64
  • llvm/clang use log2: .align 6

Looks like xcode 7 now errors out instead of limiting it to 2^15. Using .p2align would be unambiguous (and the latter behavior).

So you suggest to replace .align 64 by .p2align 6? I will have a go at that a little bit later.

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

Replying to fbissey:

Replying to vbraun:

Afaik:

  • gcc/gas uses byte count on most platforms: .align 64
  • llvm/clang use log2: .align 6

Looks like xcode 7 now errors out instead of limiting it to 2^15. Using .p2align would be unambiguous (and the latter behavior).

So you suggest to replace .align 64 by .p2align 6? I will have a go at that a little bit later.

I also had to replace .align 32,,16 with .p2align 5,,4. It builds with those changes. Should the patch only be applied on OS X? On OS X with a new version of Xcode? Always?

comment:8 Changed 7 years ago by fbissey

It should always work - on all x86. Remember that before xcode 7.0 there was already a warning that it wasn't correct so that should take care of the warning in the pre-xcode 7 environment. Another option would be to replace .align with .balign (and the values wouldn't have to be expressed in power of 2 then).

comment:9 Changed 7 years ago by zimmerma

please submit a patch: I will apply it upstream if it passes all tests. Paul

comment:10 Changed 7 years ago by vbraun

+1 to applying it unconditionally. There may be platforms where the asm doesn't understand .p2align but those are probably rare by now.

comment:11 Changed 7 years ago by jhpalmieri

  • Branch set to u/jhpalmieri/build-ecm-with-new-xcode

comment:12 Changed 7 years ago by jhpalmieri

  • Commit set to 1f3e8dc1a1f72f99c8c758c80463138bbef90534
  • Status changed from new to needs_review

Here is a patch. Paul, for upstream, note that it patches the .asm source files to be built, not the .py and .m4 files that autogenerate the source.


New commits:

1f3e8dctrac 19233: change ".align 2^n" to ".p2align n"

comment:13 Changed 7 years ago by jhpalmieri

By the way, although I produced the patch/branch, I was basically just following the suggestions of others. So probably someone else should be the author, and I can be one of the reviewers. From the point of view of reviewing, I can verify that Sage builds with these changes, but that doesn't mean I understand the changes.

comment:14 Changed 7 years ago by vbraun

  • Authors set to John Palmieri, Volker Braun
  • Reviewers set to John Palmieri, Volker Braun
  • Status changed from needs_review to positive_review

comment:15 Changed 7 years ago by vbraun

  • Branch changed from u/jhpalmieri/build-ecm-with-new-xcode to 1f3e8dc1a1f72f99c8c758c80463138bbef90534
  • Resolution set to fixed
  • Status changed from positive_review to closed

comment:16 follow-up: Changed 7 years ago by zimmerma

  • Commit 1f3e8dc1a1f72f99c8c758c80463138bbef90534 deleted

I wonder why the patch is so long. The asm files are generated automatically. Here is the patch I applied upstream:

--- x86_64/mulredc.m4   (revision 2730)
+++ x86_64/mulredc.m4   (working copy)
@@ -30,7 +30,7 @@
 `include(`config.m4')'
 
        TEXT
-.align 64 # Opteron L1 code cache line is 64 bytes long
+.p2align 6 # Opteron L1 code cache line is 64 bytes long
        GLOBL GSYM_PREFIX``''mulredc`'LENGTH
        TYPE(GSYM_PREFIX``''mulredc``''LENGTH,``function'')
 
@@ -248,7 +248,7 @@
 # i > 0 passes
 #########################################################################
 
-.align 32,,16
+.p2align 5,,4
 LABEL_SUFFIX(1)
 
 # register values at loop entry: %TP = tmp, %I = i, %YP = y, %MP = m

Paul

comment:17 Changed 7 years ago by fbissey

Stupid question! If the assembly files are generated automatically, why are they distributed? Does that mean that m4 is a requirement for gmp-ecm?

comment:18 Changed 7 years ago by zimmerma

good point. Only mulredc1.asm needs to be distributed. I've fixed that upstream in revision 2732. Paul

comment:19 Changed 7 years ago by jhpalmieri

I didn't say it correctly: those files can be autogenerated, but they're not by default. The file x86_64/README explains why:

mulredc[1..20].asm are size-specific asm functions for mulredc.
These are generated by the Python autogen.py script (old version, still
used for sizes 1 and 2) and the m4 script mulredc.m4 (all other sizes). 
In order to avoid dependency on the Python and m4 packages, this generation 
is not done automatically with the autoconf/automake stuff.

comment:20 Changed 7 years ago by zimmerma

sorry I missed that. Thank you John.

comment:21 in reply to: ↑ 16 Changed 6 years ago by leif

Replying to zimmerma:

Here is the patch I applied upstream:

--- x86_64/mulredc.m4   (revision 2730)
+++ x86_64/mulredc.m4   (working copy)
@@ -30,7 +30,7 @@
 `include(`config.m4')'
 
        TEXT
-.align 64 # Opteron L1 code cache line is 64 bytes long
+.p2align 6 # Opteron L1 code cache line is 64 bytes long
        GLOBL GSYM_PREFIX``''mulredc`'LENGTH
        TYPE(GSYM_PREFIX``''mulredc``''LENGTH,``function'')
 
@@ -248,7 +248,7 @@
 # i > 0 passes
 #########################################################################
 
-.align 32,,16
+.p2align 5,,4
 LABEL_SUFFIX(1)
 
 # register values at loop entry: %TP = tmp, %I = i, %YP = y, %MP = m

You could have changed the comment as well, as the L1 cache line size is 64 bytes on any (recent) Intel x86_64, too. ;-)

comment:22 Changed 6 years ago by zimmerma

You could have changed the comment as well, as the L1 cache line size is 64 bytes on any (recent) Intel x86_64, too. ;-)

indeed, I've changed Opteron to x86_64 upstream (revision 2974).

Paul

Note: See TracTickets for help on using tickets.