Opened 4 years ago

Closed 4 years ago

Last modified 4 years ago

#22860 closed defect (fixed)

ticket #22840 causes breakage with some linkers when the order is important.

Reported by: fbissey Owned by:
Priority: blocker Milestone: sage-8.0
Component: packages: standard Keywords:
Cc: dimpase, jmantysalo, tscrim, strogdon Merged in:
Authors: François Bissey Reviewers: Travis Scrimshaw, Dima Pasechnik, Steven Trogdon
Report Upstream: N/A Work issues:
Branch: 5f52680 (Commits, GitHub, GitLab) Commit:
Dependencies: Stopgaps:

Status badges

Description

After #22840 was merged and released as part of 8.0.beta3 users of ubuntu 16.04.4 started to complain that lcalc couldn't be built

g++ -O3   -ffast-math -fPIC   -I../include -L/home/travis/sage-build/local/lib -Wl,-rpath,/home/travis/sage-build/local/lib  -L/home/travis/sage-build/local/lib -lpari -lgmp -L/home/travis/sage-build/local/lib -lpari -lgmp Lglobals.o Lgamma.o Lriemannsiegel.o Lriemannsiegel_blfi.o Ldokchitser.o Lcommandline_globals.o Lcommandline_misc.o Lcommandline_numbertheory.o Lcommandline_values_zeros.o Lcommandline_elliptic.o Lcommandline_twist.o Lcommandline.o cmdline.o -o lcalc 
Lcommandline_elliptic.o: In function `cxcompotor.constprop.52':
Lcommandline_elliptic.cc:(.text+0x44): undefined reference to `pari_err'
Lcommandline_elliptic.cc:(.text+0x63): undefined reference to `avma'
Lcommandline_elliptic.cc:(.text+0x6a): undefined reference to `pari_mainstack'
Lcommandline_elliptic.cc:(.text+0xa3): undefined reference to `affir'
Lcommandline_elliptic.cc:(.text+0xc3): undefined reference to `avma'
...
...
Lcommandline_elliptic.cc:(.text+0x1289): undefined reference to `new_chunk_resize'
Lcommandline.o: In function `main':
Lcommandline.cc:(.text.startup+0x8d5): undefined reference to `pari_init'
Lcommandline.cc:(.text.startup+0x14ba): undefined reference to `paristack_setsize'
collect2: error: ld returned 1 exit status

This is a perfect example were the libraries need to be after the objects needed them in some linkers.

Change History (32)

comment:1 Changed 4 years ago by fbissey

  • Cc dimpase added

Dima, I will move LDFLAGS after the objects file in those makefiles. If this isn't working for you we'll have to do some split between flags proper and LIBS.

Last edited 4 years ago by fbissey (previous) (diff)

comment:2 Changed 4 years ago by fbissey

I think only lcalc has an issue but looking closer I think LDFLAGS should be separated from "PARI_LIBS". Can you tell me what's in your LDFLAGS on freeBSD?

comment:3 Changed 4 years ago by jmantysalo

  • Cc jmantysalo added

Cc'ing myself. Got same error on two different Ubuntu 16.04 LTS machines.

comment:4 Changed 4 years ago by tscrim

  • Cc tscrim added

comment:5 Changed 4 years ago by fbissey

  • Authors set to François Bissey
  • Branch set to u/fbissey/lcalc_linking
  • Commit set to cf3deccd71c5150b8d618cd1cbd244d5dd62c82e
  • Status changed from new to needs_info

OK here is a quick branch to test and give feedback on. But I am waiting for comments from Dima on freeBSD before putting to review #22840 was motivated by linking issues on freeBSD and I don't want to go backwards on that.


New commits:

cf3deccNew makefile.patch. Separate pari libraries from LDFLAGS and few cosmetics.

comment:6 Changed 4 years ago by dimpase

On FreeBSD I have LDFLAGS=-L/usr/home/dima/Sage/sage/local/lib -Wl,--add-needed -lm

And this is the (working) command in question:

clang++-devel -fPIC -pipe -O3   -ffast-math -fPIC   -I../include -L/usr/home/dima/Sage/sage/local/lib -Wl,-rpath,/usr/home/dima/Sage/
sage/local/lib -L/usr/home/dima/Sage/sage/local/lib -Wl,--add-needed -lm -L/usr/home/dima/Sage/sage/local/lib -lpari -lgmp -L/usr/hom
e/dima/Sage/sage/local/lib -lpari -lgmp Lglobals.o Lgamma.o Lriemannsiegel.o Lriemannsiegel_blfi.o Ldokchitser.o Lcommandline_globals
.o Lcommandline_misc.o Lcommandline_numbertheory.o Lcommandline_values_zeros.o Lcommandline_elliptic.o Lcommandline_twist.o Lcommandl
ine.o cmdline.o -o lcalc 

comment:7 Changed 4 years ago by dimpase

Strangely, patchbots (one of then Ubuntu 16.04) had no problem with #22840.

comment:8 Changed 4 years ago by fbissey

  • Status changed from needs_info to needs_review

OK, -Wl,--add-needed probably shield you from a lot of stuff. In any case it should work with this branch. You guys can start trashing it, it was a quicky so there may be a mistake or two lurking.

comment:9 Changed 4 years ago by dimpase

--add-needed is necessary for the extra libm we install on FreeBSD, and which is linked to the "real" libm.

By the way,

-+	$(MAKE) libLfunction$(LIBEXT)
-+	$(MAKE) lcalc$(EXEEXT)
++	$(MAKE) libLfunction.so
++	$(MAKE) lcalc

looks too optimistic for OSX, don't we get .dynlib rather than .so, and too optimistic for Cygwin, where we get .exe for executables.

comment:10 Changed 4 years ago by fbissey

Thanks, I missed these. I should have had a look at a diff of the patch. Correcting shortly.

comment:11 Changed 4 years ago by git

  • Commit changed from cf3deccd71c5150b8d618cd1cbd244d5dd62c82e to 958610e2f5bbe29e18b3621d679ab6ded30400bf

Branch pushed to git repo; I updated commit sha1. New commits:

958610eRe-add proper extensions in makefile patch

comment:12 follow-up: Changed 4 years ago by fbissey

  • Cc strogdon added

OK, so for the review I am expecting testing from Dima on freeBSD, someone on ubuntu 16.04.4 and Steve Trogdon on Gentoo with --as-needed in LDFLAGS.

comment:13 follow-up: Changed 4 years ago by dimpase

-+	rm -f *.o lcalc libLfunction$(LIBEXT) example_programs/example
++	rm -f *.o lcalc$(EXEEXT) libLfunction$(LIBEXT) example_programs/example

should be example_programs/example$(EXEEXT) at the 2nd line

comment:14 in reply to: ↑ 13 ; follow-up: Changed 4 years ago by fbissey

Replying to dimpase:

-+	rm -f *.o lcalc libLfunction$(LIBEXT) example_programs/example
++	rm -f *.o lcalc$(EXEEXT) libLfunction$(LIBEXT) example_programs/example

should be example_programs/example$(EXEEXT) at the 2nd line

If you look at the original Makefile.patch you will note that $(EXEEXT) has never been appended to example neither here or the earlier rule to build it. Neither did you in your patch revision. Nothing in spkg-install or spkg-build will ever call that target, it has only been amended for some measure of consistency but someone (probably working on cygwin) didn't see the point in adding that bit.

comment:15 in reply to: ↑ 14 Changed 4 years ago by dimpase

Replying to fbissey:

Replying to dimpase:

-+	rm -f *.o lcalc libLfunction$(LIBEXT) example_programs/example
++	rm -f *.o lcalc$(EXEEXT) libLfunction$(LIBEXT) example_programs/example

should be example_programs/example$(EXEEXT) at the 2nd line

If you look at the original Makefile.patch you will note that $(EXEEXT) has never been appended to example neither here or the earlier rule to build it. Neither did you in your patch revision. Nothing in spkg-install or spkg-build will ever call that target, it has only been amended for some measure of consistency but someone (probably working on cygwin) didn't see the point in adding that bit.

I think on Windows you'd get .exe appended to an executable even if -o ... does not mention it. And then you need to remove the file with the .exe extension.

comment:16 Changed 4 years ago by git

  • Commit changed from 958610e2f5bbe29e18b3621d679ab6ded30400bf to 70a427e9a92320cb83dbe769f776e34c27be0686

Branch pushed to git repo; I updated commit sha1. New commits:

70a427emake example target consistent

comment:17 Changed 4 years ago by fbissey

OK things are more consistent now.

comment:18 in reply to: ↑ 12 Changed 4 years ago by egourgoulhon

Replying to fbissey:

OK, so for the review I am expecting testing from Dima on freeBSD, someone on ubuntu 16.04.4

I confirm that pulling the ticket branch in Sage 8.0.beta3 sources (from a fresh git clone) yields a successful built on Ubuntu 16.04 (it failed otherwise).

comment:19 Changed 4 years ago by dimpase

OK, this works on FreeBSD.

comment:20 Changed 4 years ago by tscrim

  • Reviewers set to Travis Scrimshaw, Dima Pasechnik

Worked for me on linux. I'm going to try on Cygwin in a minute.

comment:21 follow-up: Changed 4 years ago by strogdon

Builds here on Gentoo with LDFLAGS=-Wl,-O1 -Wl,--as-needed with a slightly different ordering of linked libs

g++ -O3   -ffast-math -fPIC   -I../include -L/64bitdev/storage/sage-git_develop/sage/local/lib -Wl,-rpath,/64bitdev/storage/sage-git_develop/sage/local/lib -Wl,-O1 -Wl,--as-needed Lglobals.o Lgamma.o Lriemannsiegel.o Lriemannsiegel_blfi.o Ldokchitser.o Lcommandline_globals.o Lcommandline_misc.o Lcommandline_numbertheory.o Lcommandline_values_zeros.o Lcommandline_elliptic.o Lcommandline_twist.o Lcommandline.o cmdline.o -o lcalc -L/64bitdev/storage/sage-git_develop/sage/local/lib -lpari -lgmp -lm

comment:22 in reply to: ↑ 21 Changed 4 years ago by strogdon

Replying to strogdon:

Builds here on Gentoo with LDFLAGS=-Wl,-O1 -Wl,--as-needed with a slightly different ordering of linked libs

g++ -O3   -ffast-math -fPIC   -I../include -L/64bitdev/storage/sage-git_develop/sage/local/lib -Wl,-rpath,/64bitdev/storage/sage-git_develop/sage/local/lib -Wl,-O1 -Wl,--as-needed Lglobals.o Lgamma.o Lriemannsiegel.o Lriemannsiegel_blfi.o Ldokchitser.o Lcommandline_globals.o Lcommandline_misc.o Lcommandline_numbertheory.o Lcommandline_values_zeros.o Lcommandline_elliptic.o Lcommandline_twist.o Lcommandline.o cmdline.o -o lcalc -L/64bitdev/storage/sage-git_develop/sage/local/lib -lpari -lgmp -lm

I'm curious about the -L/64bitdev/storage/sage-git_develop/sage/local/lib -lpari -lgmp -lm that appears above. In particular the -lm that wasn't present when it failed. I have no libm.so under 64bitdev/storage/sage-git_develop/sage/local/lib. I don't think it's needed. But why does it appear?

comment:23 follow-up: Changed 4 years ago by strogdon

I see, the -lm is now in the Makefile. Is it needed?

comment:24 Changed 4 years ago by tscrim

This built for me without problems on Cygwin.

comment:25 in reply to: ↑ 23 Changed 4 years ago by fbissey

Replying to strogdon:

I see, the -lm is now in the Makefile. Is it needed?

Possibly not, I am just not taking chances in case someone has a system with somewhat broken dynamic libraries. Both pari and gmp use libm.

comment:26 Changed 4 years ago by fbissey

  • Reviewers changed from Travis Scrimshaw, Dima Pasechnik to Travis Scrimshaw, Dima Pasechnik, Steve Trogdon
  • Status changed from needs_review to positive_review

I think that's enough reviews :)

comment:27 Changed 4 years ago by jdemeyer

  • Status changed from positive_review to needs_work

Don't forget to bump the patchlevel (you can set this back to positive_review after you did that).

comment:28 Changed 4 years ago by git

  • Commit changed from 70a427e9a92320cb83dbe769f776e34c27be0686 to 5f52680da6b7a705bccc40dd83cffeac1789abcd

Branch pushed to git repo; I updated commit sha1. New commits:

5f52680version bumping to make Jeroen happy

comment:29 Changed 4 years ago by fbissey

  • Status changed from needs_work to positive_review

comment:30 Changed 4 years ago by jpflori

Is there still an upstream for lcalc where to forward our patches?

comment:31 Changed 4 years ago by vbraun

  • Branch changed from u/fbissey/lcalc_linking to 5f52680da6b7a705bccc40dd83cffeac1789abcd
  • Resolution set to fixed
  • Status changed from positive_review to closed

comment:32 Changed 4 years ago by jdemeyer

  • Commit 5f52680da6b7a705bccc40dd83cffeac1789abcd deleted
  • Reviewers changed from Travis Scrimshaw, Dima Pasechnik, Steve Trogdon to Travis Scrimshaw, Dima Pasechnik, Steven Trogdon
Note: See TracTickets for help on using tickets.