Opened 4 years ago

Closed 3 years ago

#23700 closed enhancement (fixed)

update gc to version 7.6

Reported by: dimpase Owned by:
Priority: major Milestone: sage-8.2
Component: packages: standard Keywords:
Cc: fbissey, embray Merged in:
Authors: Dima Pasechnik, Erik Bray Reviewers: Erik Bray, Dima Pasechnik, Jeroen Demeyer
Report Upstream: N/A Work issues:
Branch: cd63b82 (Commits, GitHub, GitLab) Commit: cd63b82e0d58ecd4961a22e084c6d43af667c7bc
Dependencies: Stopgaps:

Status badges

Description (last modified by dimpase)

An update to gc is needed to support arm64/aarch64, see #23687, as well as for FreeBSD support, see #22679. It also appears to be good to be in sync with Linux boxes that ship make with guile extension (the latter depends on gc, and causes interesting errors, see #24575).

The source is here: https://github.com/ivmai/bdwgc/releases/download/v7.6.4/gc-7.6.4.tar.gz

On some platforms this new version also needs libatomic_ops, see #23701

Change History (58)

comment:1 Changed 4 years ago by dimpase

  • Dependencies set to #23701
  • Description modified (diff)

comment:2 Changed 4 years ago by git

  • Commit changed from 37cbca100a8be09b868a9280550de5a039600cac to 277ddd77e391269febfa10a66c62f34f475b4181

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

7d09a89libatomic_ops added as standard pkg
7c8c3e3Merge branch 'libatomic_ops' into gcupdate
277ddd7libatomic_ops is a dependency

comment:3 Changed 4 years ago by dimpase

  • Authors set to Dima Pasechnik
  • Cc fbissey embray added
  • Status changed from new to needs_review

Erik, could you please check whether this works on Cygwin without your patch?

(one need #23701 first, and re-running ./configure to register the new package)

comment:4 Changed 4 years ago by embray

Ah, yes. I remember this. I'll give it a go.

comment:5 Changed 4 years ago by embray

I think this branch needs to be updated on the latest version of #23701 since it's using 7.4.6 and this branch is using 7.4.4.

comment:6 Changed 4 years ago by git

  • Commit changed from 277ddd77e391269febfa10a66c62f34f475b4181 to e53f78480379ce726091927c870c0869cb115b40

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

4d5bbd0libatomic_ops 7.4.6 added as standard pkg
e53f784Merge branch 'u/dimpase/libatomic_ops' of trac.sagemath.org:sage into gcupdate

comment:7 Changed 4 years ago by dimpase

You're right - done now.

comment:8 Changed 4 years ago by embray

Thanks--now it's working. Or at least, it's building. Can't say anything about tests right now.

comment:9 Changed 4 years ago by dimpase

on FreeBSD this needs the patch fixing https://github.com/ivmai/bdwgc/issues/132

comment:10 Changed 4 years ago by git

  • Commit changed from e53f78480379ce726091927c870c0869cb115b40 to 931652aecee2bd1d775ccbd8fcc18aa5d1afb0ee

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

8166e21libatomic_ops 7.6.4 added as standard pkg
64da17fMerge branch 'libatomic_ops' into gcupdate
82eabaepatch for FreeBSD
931652aMerge branch 'u/dimpase/gcupdate' of trac.sagemath.org:sage into gcupdate

comment:11 Changed 4 years ago by dimpase

  • Status changed from needs_review to needs_work

On Linux tests pass, but I can trigger a nasty crash interactively:

sage: from sage.libs.ecl import *
sage: from sage.interfaces.maxima_lib import <TAB>

Debian experimental uses gc 7.4.4, so this would be one to test for this regression, too.

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

comment:12 Changed 4 years ago by dimpase

With GC included in ECL (tagged version 7.5.0 in ECL source, although I haven't checked how canonical this is) I do get the crash in comment 11.

comment:13 Changed 4 years ago by git

  • Commit changed from 931652aecee2bd1d775ccbd8fcc18aa5d1afb0ee to e365c32caf4e82cc9f6a43f066d8314313756248

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

f9ca589Merge branch 'u/dimpase/gcupdate' of trac.sagemath.org:sage into gc76
e365c32backported patch for FreeBSD support

comment:14 Changed 4 years ago by dimpase

  • Status changed from needs_work to needs_review

comment:15 Changed 4 years ago by embray

  • Status changed from needs_review to needs_work

Turns out this is still not building properly on Cygwin. It fails to produce a shared lib at link time:

/bin/sh ./libtool  --tag=CC   --mode=link gcc   -fexceptions -Wall -Wextra -O2 -g  -fno-strict-aliasing  -version-info 1:3:0 -no-undefined -L/home/embray/src/sagemath/sage/local/lib -Wl,-rpath,/home/embray/src/sagemath/sage/local/lib  -o libgc.la -rpath /home/embray/src/sagemath/sage/local/lib allchblk.lo alloc.lo blacklst.lo checksums.lo dbg_mlc.lo dyn_load.lo finalize.lo gc_dlopen.lo gcj_mlc.lo headers.lo mach_dep.lo malloc.lo mallocx.lo mark.lo mark_rts.lo misc.lo new_hblk.lo obj_map.lo os_dep.lo pcr_interface.lo ptr_chck.lo real_malloc.lo reclaim.lo specific.lo stubborn.lo thread_local_alloc.lo typd_mlc.lo win32_threads.lo     fnlz_mlc.lo    -L/home/embray/src/sagemath/sage/local/lib -latomic_ops
mv -f cord/.deps/libcord_la-cordxtra.Tpo cord/.deps/libcord_la-cordxtra.Plo

*** Warning: linker path does not have real file for library -latomic_ops.
*** I have the capability to make that library automatically link in when
*** you link to this library.  But I can only do this if you have a
*** shared version of the library, which you do not appear to have
*** because I did check the linker path looking for a file starting
*** with libatomic_ops and none of the candidates passed a file format test
*** using a file magic. Last file checked: /home/embray/src/sagemath/sage/local/lib/libatomic_ops.a
*** The inter-library dependencies that have been dropped here will be
*** automatically added whenever a program is linked with this library
*** or is declared to -dlopen it.

*** Since this library must not contain undefined symbols,
*** because either the platform does not support them or
*** it was explicitly requested with -no-undefined,
*** libtool will only create a static version of it.
libtool: link: ar cru .libs/libgc.a  allchblk.o alloc.o blacklst.o checksums.o dbg_mlc.o dyn_load.o finalize.o gc_dlopen.o gcj_mlc.o headers.o mach_dep.o malloc.o mallocx.o mark.o mark_rts.o misc.o new_hblk.o obj_map.o os_dep.o pcr_interface.o ptr_chck.o real_malloc.o reclaim.o specific.o stubborn.o thread_local_alloc.o typd_mlc.o win32_threads.o fnlz_mlc.o
libtool: link: ranlib .libs/libgc.a
libtool: link: ( cd ".libs" && rm -f "libgc.la" && ln -s "../libgc.la" "libgc.la" )
/bin/sh ./libtool  --tag=CC   --mode=link gcc   -fexceptions -Wall -Wextra -O2 -g  -fno-strict-aliasing -version-info 1:3:0 -no-undefined -L/home/embray/src/sagemath/sage/local/lib -Wl,-rpath,/home/embray/src/sagemath/sage/local/lib  -o libcord.la -rpath /home/embray/src/sagemath/sage/local/lib cord/libcord_la-cordbscs.lo cord/libcord_la-cordprnt.lo cord/libcord_la-cordxtra.lo ./libgc.la

*** Warning: This system cannot link to static lib archive ./libgc.la.
*** I have the capability to make that library automatically link in when
*** you link to this library.  But I can only do this if you have a
*** shared version of the library, which you do not appear to have.

*** Warning: linker path does not have real file for library -latomic_ops.
*** I have the capability to make that library automatically link in when
*** you link to this library.  But I can only do this if you have a
*** shared version of the library, which you do not appear to have
*** because I did check the linker path looking for a file starting
*** with libatomic_ops and none of the candidates passed a file format test
*** using a file magic. Last file checked: /home/embray/src/sagemath/sage/local/lib/libatomic_ops.a
*** The inter-library dependencies that have been dropped here will be
*** automatically added whenever a program is linked with this library
*** or is declared to -dlopen it.

*** Since this library must not contain undefined symbols,
*** because either the platform does not support them or
*** it was explicitly requested with -no-undefined,
*** libtool will only create a static version of it.
libtool: link: ar cru .libs/libcord.a  cord/libcord_la-cordbscs.o cord/libcord_la-cordprnt.o cord/libcord_la-cordxtra.o
libtool: link: ranlib .libs/libcord.a
libtool: link: ( cd ".libs" && rm -f "libcord.la" && ln -s "../libcord.la" "libcord.la" )

It's probably a minor issue with link flag ordering. I will investigate and provide a solution.

comment:16 Changed 4 years ago by git

  • Commit changed from e365c32caf4e82cc9f6a43f066d8314313756248 to 32b8eeba01a780943c24e58e8108582babeec8b7

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

7b0f626Merge branch 'u/dimpase/libatomic_ops' of trac.sagemath.org:sage into laup
5c5d8f6remove meaningless flags
32b8eebMerge branch 'u/dimpase/gcupdate' of trac.sagemath.org:sage into laup

comment:17 Changed 4 years ago by dimpase

updated the branch to take into account update on #23701

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

comment:18 Changed 4 years ago by dimpase

  • Description modified (diff)

comment:19 Changed 4 years ago by embray

Ah, I should have warned you, I'm about to propose another needed change for #23701.

comment:20 Changed 3 years ago by dimpase

We should go for 7.6.2: https://github.com/ivmai/bdwgc/releases/tag/v7.6.2 In particular, this includes the patches needed by the FreeBSD port. I will update this accordingly.

comment:21 Changed 3 years ago by git

  • Commit changed from 32b8eeba01a780943c24e58e8108582babeec8b7 to 7db1b1444d1dbb2829f06a78585c8bb40bca5855

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

54e1da7Merge branch 'develop' into libatomic_ops
a024f8d7.6.2 is the current version
7db1b14Merge 'libatomic_ops' -> gcupdate, update to 7.6.2

comment:22 Changed 3 years ago by dimpase

  • Description modified (diff)
  • Milestone changed from sage-8.1 to sage-8.2

comment:23 Changed 3 years ago by git

  • Commit changed from 7db1b1444d1dbb2829f06a78585c8bb40bca5855 to b97bf993652a49c9dbc14dd40e2bed2090e70465

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

b97bf99removing already included patches

comment:24 Changed 3 years ago by git

  • Commit changed from b97bf993652a49c9dbc14dd40e2bed2090e70465 to e790e65e44e0ff74a9b58ee8366348f3f2ca9a5d

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

e790e65removing already included patches

comment:25 Changed 3 years ago by dimpase

  • Status changed from needs_work to needs_review

OK, another advantage is that all our patches are now upstream!

comment:26 Changed 3 years ago by embray

I'm trying to get this built on Cygwin--will report back with any findings.

comment:27 Changed 3 years ago by embray

IIRC I had some trouble with this on Cygwin (specifically with libatomic_ops), but that was a while ago and I got distracted. Going to give it another try.

comment:28 Changed 3 years ago by embray

Likewise with ticket:23701#comment:15 would you accept a small update to this for Cygwin?

comment:29 Changed 3 years ago by dimpase

sure

comment:30 Changed 3 years ago by embray

  • Branch changed from u/dimpase/gcupdate to public/ticket-23700
  • Commit changed from e790e65e44e0ff74a9b58ee8366348f3f2ca9a5d to c2eb88c41a904432f8f6e81a6f462ed01f909ee0
  • Reviewers set to Erik Bray

Rebased on top of my updates to #23701 and incorporated a minor change for Cygwin.


New commits:

7135b2fOn Cygwin install libatomic_ops as a shared lib only; this is inline with what the official Cygwin package does
18813a2Convert libatomic_ops to use sage-dist-helpers
e57cfc4update to 7.6.0
897c091patch for FreeBSD
2d923f2backported patch for FreeBSD support
822cc06removing already included patches
f6fd533update gc to 7.6.2
c2eb88cFor Cygwin only builds shared libraries

comment:31 Changed 3 years ago by dimpase

  • Authors changed from Dima Pasechnik to Dima Pasechnik, Erik Bray
  • Reviewers changed from Erik Bray to Erik Bray, Dima Pasechnik
  • Status changed from needs_review to positive_review

comment:32 Changed 3 years ago by vbraun

  • Status changed from positive_review to needs_work
Found local metadata for gc-7.6.2
Using cached file /mnt/disk/home/release/Sage/upstream/gc-7.6.2.tar.gz
gc-7.6.2
====================================================
Setting up build directory for gc-7.6.2
Finished extraction
Applying patches from ../patches...
Applying ../patches/0001-On-Cygwin-use-mprotect-instead-of-mmap-to-set-PROT_N.patch
patching file os_dep.c
Hunk #1 FAILED at 698.
Hunk #2 FAILED at 2404.
Hunk #3 FAILED at 2515.
3 out of 3 hunks FAILED -- saving rejects to file os_dep.c.rej
Error applying '../patches/0001-On-Cygwin-use-mprotect-instead-of-mmap-to-set-PROT_N.patch'
************************************************************************
Error applying patches
************************************************************************
Please email sage-devel (http://groups.google.com/group/sage-devel)
explaining the problem and including the log file
  /mnt/disk/home/release/Sage/logs/pkgs/gc-7.6.2.log
Describe your computer, operating system, etc.
************************************************************************

comment:33 Changed 3 years ago by dimpase

facepalm... we should bump it up to 7.6.4 anyway.

comment:34 Changed 3 years ago by dimpase

  • Description modified (diff)

I believe that the failing patch is already in 7.6.4, see https://github.com/ivmai/bdwgc/commit/be148f4141ab5d6812928a7bceeb2617384e7fe7

comment:35 Changed 3 years ago by dimpase

  • Branch changed from public/ticket-23700 to u/dimpase/gc764
  • Commit changed from c2eb88c41a904432f8f6e81a6f462ed01f909ee0 to 0263addf139baf3f62fda637bb220a7491af2f18
  • Status changed from needs_work to needs_review

Last 10 new commits:

871bc6a7.6.2 is the current version
71e46a4On Cygwin install libatomic_ops as a shared lib only; this is inline with what the official Cygwin package does
0308045Convert libatomic_ops to use sage-dist-helpers
8c47a8aupdate to 7.6.0
6148efdpatch for FreeBSD
7496a68backported patch for FreeBSD support
674a6d6removing already included patches
48766ddupdate gc to 7.6.2
cdb00d4For Cygwin only builds shared libraries
0263addupdated to 7.6.4 and removed patch

comment:36 Changed 3 years ago by dimpase

  • Description modified (diff)

comment:37 Changed 3 years ago by jdemeyer

  • Status changed from needs_review to needs_work
  1. build/pkgs/libatomic_ops/spkg-install: you are wrongly using "BoehmGC".
  1. build/pkgs/libatomic_ops/spkg-install: instead of a generic variable like CONFIGURE_FLAGS, please use LIBATOMIC_OPS_CONFIGURE to be more consistent with other packages.
  1. build/pkgs/libatomic_ops/spkg-check: there is no longer need for the if [ -z "$SAGE_LOCAL" ]; then boilerplate.

comment:38 follow-up: Changed 3 years ago by dimpase

  • Status changed from needs_work to needs_review

Jeroen, libatomic_ops has nothing to do with this ticket. It was #23701. If you think #23701 needs an improvement please open another ticket.

comment:39 Changed 3 years ago by jdemeyer

They were just minor comments, nothing serious.

comment:40 in reply to: ↑ 38 Changed 3 years ago by jdemeyer

  • Status changed from needs_review to needs_work

Replying to dimpase:

Jeroen, libatomic_ops has nothing to do with this ticket. It was #23701. If you think #23701 needs an improvement please open another ticket.

To be very precise, the commits which are supposedly from #23701 do not actually appear on the branch at #23701. So this ticket needs to be rebased.

comment:41 follow-up: Changed 3 years ago by jdemeyer

There is also the obvious missing dependency

[gc-7.6.4] checking for atomic_ops... no
[gc-7.6.4] checking atomic_ops.h usability... no
[gc-7.6.4] checking atomic_ops.h presence... no
[gc-7.6.4] checking for atomic_ops.h... no
[gc-7.6.4] configure: error: libatomic_ops is required.  You can either install it on
[gc-7.6.4]                   your system, or fetch and unpack a recent version into the
[gc-7.6.4]                   source directory and link or rename it to libatomic_ops.
[gc-7.6.4] Error configuring BoehmGC.

comment:42 in reply to: ↑ 41 ; follow-up: Changed 3 years ago by dimpase

Replying to jdemeyer:

There is also the obvious missing dependency

[gc-7.6.4] checking for atomic_ops... no
[gc-7.6.4] checking atomic_ops.h usability... no
[gc-7.6.4] checking atomic_ops.h presence... no
[gc-7.6.4] checking for atomic_ops.h... no
[gc-7.6.4] configure: error: libatomic_ops is required.  You can either install it on
[gc-7.6.4]                   your system, or fetch and unpack a recent version into the
[gc-7.6.4]                   source directory and link or rename it to libatomic_ops.
[gc-7.6.4] Error configuring BoehmGC.

good catch, but how do you get this? OS/compiler?

comment:43 in reply to: ↑ 42 Changed 3 years ago by jdemeyer

Replying to dimpase:

OS/compiler?

Doesn't matter. Any OS, any compiler.

comment:44 Changed 3 years ago by git

  • Commit changed from 0263addf139baf3f62fda637bb220a7491af2f18 to 4d6eee33939ae419776f36b4cd08406e8212d663

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

4d6eee3added deps; addressed reviewer comments: #23700:37

comment:45 Changed 3 years ago by dimpase

  • Status changed from needs_work to needs_review

all fixed, hopefully.

comment:46 Changed 3 years ago by jdemeyer

  • Status changed from needs_review to needs_work
  1. This branch still needs to be rebased to the branch at #23701.
  1. You should replace CONFIGURE_FLAGS -> LIBATOMIC_OPS_CONFIGURE everywhere in build/pkgs/libatomic_ops/spkg-install

comment:47 Changed 3 years ago by git

  • Commit changed from 4d6eee33939ae419776f36b4cd08406e8212d663 to cd63b82e0d58ecd4961a22e084c6d43af667c7bc

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

4d5bbd0libatomic_ops 7.4.6 added as standard pkg
7b0f626Merge branch 'u/dimpase/libatomic_ops' of trac.sagemath.org:sage into laup
5c5d8f6remove meaningless flags
54e1da7Merge branch 'develop' into libatomic_ops
a024f8d7.6.2 is the current version
7135b2fOn Cygwin install libatomic_ops as a shared lib only; this is inline with what the official Cygwin package does
18813a2Convert libatomic_ops to use sage-dist-helpers
8d05b70Merge branch 'atomic762' into gc764
cd63b82LIBATOMIC_OPS_CONFIGURE throughout

comment:48 Changed 3 years ago by dimpase

  • Status changed from needs_work to needs_review

I've addressed the comment 46.

comment:49 Changed 3 years ago by jdemeyer

The git history is a total mess now... at least messy enough that I'd like to see it merged with the next beta before considering to review it.

comment:50 Changed 3 years ago by dimpase

well, yes, I tried to rebase, and it was really doing weird stuff, such as creating LICENSE file and changing .gitignore, so I merged instead.

comment:51 Changed 3 years ago by embray

Perhaps I could try to clean it up?

comment:52 follow-up: Changed 3 years ago by dimpase

  • Dependencies #23701 deleted

Hopefully it is clean enough now, as the merge of #23701 has happened.

comment:53 in reply to: ↑ 52 Changed 3 years ago by jdemeyer

Replying to dimpase:

Hopefully it is clean enough now

It's not cleaner than before since the branch didn't change. The fact that #23701 was merged does mean that the diff is easier to read on Trac.

comment:54 follow-up: Changed 3 years ago by dimpase

The diff is pretty small now: there are largish patches removed, and the rest it really "admin".

Erik, does the branch work on Cygwin for you now? (You never answered comment 34 above). The commit in the upstream repo seems to do the same job and is authored by you, but it looks differently---hopefully it does the same thing.

comment:55 in reply to: ↑ 54 Changed 3 years ago by jdemeyer

  • Status changed from needs_review to positive_review

Replying to dimpase:

The diff is pretty small now

Yes, the diff is small but the history remains ugly: there are several commits on this branch which are really duplicates of commits from #23701.

Now, all that doesn't really matter.

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

comment:56 Changed 3 years ago by jdemeyer

  • Reviewers changed from Erik Bray, Dima Pasechnik to Erik Bray, Dima Pasechnik, Jeroen Demeyer

comment:57 Changed 3 years ago by embray

LGTM. My offer still stands to clean up the commit history if desired, but otherwise I'll let it be.

comment:58 Changed 3 years ago by vbraun

  • Branch changed from u/dimpase/gc764 to cd63b82e0d58ecd4961a22e084c6d43af667c7bc
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.