Discussion:
[m-dev.] Almost ready to merge the update_boehm branch
Paul Bone
2015-09-23 01:36:53 UTC
Permalink
I'm almost ready to merge the upgrade_boehm branch.

Once it's merged you will need to use the prepare.sh script to prepare your
checkout before running configure (see below). I will now run some
performance tests (for curiosity's sake) and then I'm ready to merge this
into master. Now is your chance to check my work for anything I may have
missed, compiler/notes/upgrade_boehm_gc.html contains a summary of how I've
set this up and is a good place to start. Note that the general plan of how
to set this up has already been reviewed, I'd rather not go back and forth
on that.

If anyone would like to test it you can do the following. This is also a
test that this works from a fresh checkout.

$ git clone https://github.com/Mercury-Language/mercury.git
Cloning into 'mercury'...
remote: Counting objects: 157849, done.
remote: Compressing objects: 100% (371/371), done.
remote: Total 157849 (delta 129), reused 0 (delta 0), pack-reused 157478
Receiving objects: 100% (157849/157849), 79.24 MiB | 1.05 MiB/s, done.
Resolving deltas: 100% (128159/128159), done.
Checking connectivity... done.

Switch branches:

$ cd mercury
$ git branch upgrade_boehm origin/upgrade_boehm
Branch upgrade_boehm set up to track remote branch upgrade_boehm from origin.
$ git checkout upgrade_boehm
Switched to branch 'upgrade_boehm'
Your branch is up-to-date with 'origin/upgrade_boehm'.

The new prepare.sh script will fetch/update the boehm_gc and libatomic_ops
repositories. It also runs aclocal -I m4 and autoconf.

$ ./prepare.sh
Cloning into 'boehm_gc'...
remote: Counting objects: 18928, done.
remote: Compressing objects: 100% (44/44), done.
remote: Total 18928 (delta 22), reused 0 (delta 0), pack-reused 18884
Receiving objects: 100% (18928/18928), 10.70 MiB | 995.00 KiB/s, done.
Resolving deltas: 100% (14986/14986), done.
Checking connectivity... done.
Submodule path 'boehm_gc': checked out
'a16bf06bdc61e07ae65b431bcce9227f4e2b1f63'
Cloning into 'libatomic_ops'...
remote: Counting objects: 4638, done.
remote: Compressing objects: 100% (2/2), done.
remote: Total 4638 (delta 0), reused 0 (delta 0), pack-reused 4636
Receiving objects: 100% (4638/4638), 1.91 MiB | 508.00 KiB/s, done.
Resolving deltas: 100% (3039/3039), done.
Checking connectivity... done.
Submodule path 'libatomic_ops': checked out
'f7549252397e1b315773a5755f0d8fd1c9a284d4'

Now you're good to go with ./configure
--
Paul Bone
Julien Fischer
2015-09-23 01:54:31 UTC
Permalink
Hi Paul,
Post by Paul Bone
I'm almost ready to merge the upgrade_boehm branch.
Once it's merged you will need to use the prepare.sh script to prepare your
checkout before running configure (see below).
Neither of the issues I raised here
<http://www.mercurylang.org/list-archives/developers/2015-January/016237.html>
appear to have been addressed. prepare.sh will still attempt to create
symbolic links on systems that don't support them.

Related to the above, is the new verison of the collector arranged in
such a way that the source distribution will build on all systems?

When I last looked at this branch there seemed to be a bunch of new tool
dependencies, for example libtool. Is this still the case? If so, what
are the new dependencies?

Julien.
Paul Bone
2015-09-23 02:08:31 UTC
Permalink
Post by Julien Fischer
Hi Paul,
Post by Paul Bone
I'm almost ready to merge the upgrade_boehm branch.
Once it's merged you will need to use the prepare.sh script to prepare your
checkout before running configure (see below).
Neither of the issues I raised here
<http://www.mercurylang.org/list-archives/developers/2015-January/016237.html>
appear to have been addressed. prepare.sh will still attempt to create
symbolic links on systems that don't support them.
Okay, I can fix that.
Post by Julien Fischer
Related to the above, is the new verison of the collector arranged in
such a way that the source distribution will build on all systems?
Who knows? I don't ahve all systems here.

Can someone test on OS X, I can try to test on a windows virtual machine but
I really don't know what the "correct" way to setup a development
environment on windows is. So far I've been testing on Linux, I can also
test on FreeBSD.
Post by Julien Fischer
When I last looked at this branch there seemed to be a bunch of new tool
dependencies, for example libtool. Is this still the case? If so, what
are the new dependencies?
AFAIK libtool is the only dependency. Keeping in mind that any extra
dependencies are only for building from git. The source distribution orght
to be unaffected (which I will test this afternoon).
--
Paul Bone
Julien Fischer
2015-09-23 03:06:51 UTC
Permalink
Hi Paul,
Post by Paul Bone
Post by Julien Fischer
Hi Paul,
Post by Paul Bone
I'm almost ready to merge the upgrade_boehm branch.
Once it's merged you will need to use the prepare.sh script to prepare your
checkout before running configure (see below).
Neither of the issues I raised here
<http://www.mercurylang.org/list-archives/developers/2015-January/016237.html>
appear to have been addressed. prepare.sh will still attempt to create
symbolic links on systems that don't support them.
Okay, I can fix that.
Post by Julien Fischer
Related to the above, is the new verison of the collector arranged in
such a way that the source distribution will build on all systems?
Who knows? I don't ahve all systems here.
Can someone test on OS X,
I can test it on OS X. (I tried the updated collector back last time
this was looked at and it was fine on OS X -- my main concern is
ensuring that the Windows version works.)
Post by Paul Bone
I can try to test on a windows virtual machine but
I really don't know what the "correct" way to setup a development
environment on windows is.
There are (too) many "correct" ways, for example:

Cygwin/gcc
Cgwin/mingw-gcc
Cygwin/MSVC
MSYS/mingw-gcc
MSYS/MSVC
MSYS2/gcc
MSYS2/MSVC

(And the above completely ignores the existence of clang!)

The Mercury specific stuff is (or at least ought to be ) documented
starting with README.MS-Windows.
Post by Paul Bone
So far I've been testing on Linux, I can also
test on FreeBSD.
Post by Julien Fischer
When I last looked at this branch there seemed to be a bunch of new tool
dependencies, for example libtool. Is this still the case? If so, what
are the new dependencies?
AFAIK libtool is the only dependency. Keeping in mind that any extra
dependencies are only for building from git. The source distribution orght
to be unaffected (which I will test this afternoon).
Could you prepare a source distribution from the branch and make it
available for people to test? I would like to confirm that it works on
at least Linux, OS X and Windows before the merge.

Julien.
Paul Bone
2015-09-24 07:45:18 UTC
Permalink
Post by Julien Fischer
Hi Paul,
Post by Paul Bone
I'm almost ready to merge the upgrade_boehm branch.
Once it's merged you will need to use the prepare.sh script to prepare your
checkout before running configure (see below).
When I last looked at this branch there seemed to be a bunch of new tool
dependencies, for example libtool. Is this still the case? If so, what
are the new dependencies?
I've fixed the other issues except libtool. I have two options, neither is
great.

+ Commit generated files to git such as the configure script and Makefiles,
note that that's the generated script, not configure.ac.

+ Attempt to generate these files, which ideally should only happen for
people checking out from git, not the source distribution. This adds the
dependency of libtool.

So why this situation? what's different now that I'm updating boehm_gc?
Nothing, libatomic_ops has previously also depended on libtool. We just
used to commit generated files to git/CVS in the libatomic_ops directory.
Maybe we made a decision to do this or maybe it was by accident. I suspect
an accident because I try to avoid committing generated files.

So is it bad to depend on libtool for developers only? This does not affect
anyone building from the source distribution. Is it okay if we just call it
with glibtoolize or whatever you said it was called on, was it OS X?

Cheers.
--
Paul Bone
Julien Fischer
2015-09-24 08:02:18 UTC
Permalink
Hi Paul,
Post by Paul Bone
Post by Julien Fischer
Hi Paul,
Post by Paul Bone
I'm almost ready to merge the upgrade_boehm branch.
Once it's merged you will need to use the prepare.sh script to prepare your
checkout before running configure (see below).
When I last looked at this branch there seemed to be a bunch of new tool
dependencies, for example libtool. Is this still the case? If so, what
are the new dependencies?
I've fixed the other issues except libtool. I have two options, neither is
great.
+ Commit generated files to git such as the configure script and Makefiles,
note that that's the generated script, not configure.ac.
+ Attempt to generate these files, which ideally should only happen for
people checking out from git, not the source distribution. This adds the
dependency of libtool.
So why this situation? what's different now that I'm updating boehm_gc?
Nothing, libatomic_ops has previously also depended on libtool. We just
used to commit generated files to git/CVS in the libatomic_ops directory.
Maybe we made a decision to do this or maybe it was by accident. I suspect
an accident because I try to avoid committing generated files.
It was probably a result of the fact that we used to import the Boehm GC
stable tarballs which already had their configuration files generated.
(The fact that this avoided mucking about with libtool was just a happy
accident!)
Post by Paul Bone
So is it bad to depend on libtool for developers only? This does not affect
anyone building from the source distribution. Is it okay if we just call it
with glibtoolize or whatever you said it was called on, was it OS X?
libtoolize is a separate thing from libtool: is the new dependency only
on the latter or are both programs required?

The issue on OS X is that there is already a separate utility name
libtool. macports renames both the executables in the libtool package
in order to avoid clashing with this.

Julien.
Paul Bone
2015-09-24 09:52:30 UTC
Permalink
Post by Julien Fischer
Hi Paul,
Post by Paul Bone
Post by Julien Fischer
Hi Paul,
Post by Paul Bone
I'm almost ready to merge the upgrade_boehm branch.
Once it's merged you will need to use the prepare.sh script to prepare your
checkout before running configure (see below).
When I last looked at this branch there seemed to be a bunch of new tool
dependencies, for example libtool. Is this still the case? If so, what
are the new dependencies?
I've fixed the other issues except libtool. I have two options, neither is
great.
+ Commit generated files to git such as the configure script and Makefiles,
note that that's the generated script, not configure.ac.
+ Attempt to generate these files, which ideally should only happen for
people checking out from git, not the source distribution. This adds the
dependency of libtool.
So why this situation? what's different now that I'm updating boehm_gc?
Nothing, libatomic_ops has previously also depended on libtool. We just
used to commit generated files to git/CVS in the libatomic_ops directory.
Maybe we made a decision to do this or maybe it was by accident. I suspect
an accident because I try to avoid committing generated files.
It was probably a result of the fact that we used to import the Boehm GC
stable tarballs which already had their configuration files generated.
(The fact that this avoided mucking about with libtool was just a happy
accident!)
Post by Paul Bone
So is it bad to depend on libtool for developers only? This does not affect
anyone building from the source distribution. Is it okay if we just call it
with glibtoolize or whatever you said it was called on, was it OS X?
libtoolize is a separate thing from libtool: is the new dependency only
on the latter or are both programs required?
I don't know. I will try to understand / find out. Urgh, autotools.
Post by Julien Fischer
The issue on OS X is that there is already a separate utility name
libtool. macports renames both the executables in the libtool package
in order to avoid clashing with this.
Ah okay.
--
Paul Bone
Paul Bone
2015-09-28 07:44:25 UTC
Permalink
Post by Paul Bone
Post by Julien Fischer
Hi Paul,
Post by Paul Bone
I'm almost ready to merge the upgrade_boehm branch.
Once it's merged you will need to use the prepare.sh script to prepare your
checkout before running configure (see below).
When I last looked at this branch there seemed to be a bunch of new tool
dependencies, for example libtool. Is this still the case? If so, what
are the new dependencies?
I've fixed the other issues except libtool. I have two options, neither is
great.
+ Commit generated files to git such as the configure script and Makefiles,
note that that's the generated script, not configure.ac.
+ Attempt to generate these files, which ideally should only happen for
people checking out from git, not the source distribution. This adds the
dependency of libtool.
Okay, I've committed the libtool/autotools files for libatomic_ops. This is
working.

I've built a source package for testing and put it online
http://dl.mercurylang.org/tests/mercury-srcdist-test-2015-09-28-newgc.tar.gz

Feel free to test it out. Next I'll make some performance comparisons for
Mercury and Prince.

Cheers.
--
Paul Bone
Paul Bone
2015-09-29 05:21:03 UTC
Permalink
Post by Paul Bone
Okay, I've committed the libtool/autotools files for libatomic_ops. This is
working.
I've built a source package for testing and put it online
http://dl.mercurylang.org/tests/mercury-srcdist-test-2015-09-28-newgc.tar.gz
Feel free to test it out. Next I'll make some performance comparisons for
Mercury and Prince.
Some performance results using speedtest -l, average of 5 runs with ignore=0

Boehm version
7.2 7.4.2
asm_fast.gc 14.32s 14.04s
hlc.gc 13.34s 13.30s
hlc.par.gc 18.86s 19.04s

The variation is a little too high and the results too similar to draw any
concusions. I'll test them again later with n=20 when I'm not using the
computer for anything else. However it's safe to say that there's no
sagnificant performance regression.
--
Paul Bone
Peter Wang
2015-09-23 02:35:06 UTC
Permalink
Post by Paul Bone
I'm almost ready to merge the upgrade_boehm branch.
Hi Paul,

Can I ask that, prior to a merge, you rebase the upgrade_boehm branch to
eliminate the merges from master into upgrade_boehm? Otherwise the
history will be unnecessarily convoluted on master.

prepare.sh is trying to link from the PARENT of the 'mercury' directory:

ln -s ../libatomic_ops boehm_gc/libatomic_ops

prepare.sh could be more careful in case it is run with the current
directory not being the 'mercury' directory.

The two submodule repositories seem to use different naming conventions
for branches. How about naming them ${UPSTREAM_BRANCH}-mercury
for our forks based on ${UPSTREAM_BRANCH}?

Peter
Paul Bone
2015-09-23 06:28:28 UTC
Permalink
Post by Julien Fischer
Post by Paul Bone
I'm almost ready to merge the upgrade_boehm branch.
Hi Paul,
Can I ask that, prior to a merge, you rebase the upgrade_boehm branch to
eliminate the merges from master into upgrade_boehm? Otherwise the
history will be unnecessarily convoluted on master.
Wow, that was surprisingly painless. I thought it would be more difficult
since I had pulled from master. I was all ready to edit out those commits
but I didn't have to. git just did the right thing.
Post by Julien Fischer
ln -s ../libatomic_ops boehm_gc/libatomic_ops
prepare.sh could be more careful in case it is run with the current
directory not being the 'mercury' directory.
The two submodule repositories seem to use different naming conventions
for branches. How about naming them ${UPSTREAM_BRANCH}-mercury
for our forks based on ${UPSTREAM_BRANCH}?
I'll make the other changes suggested here and in the other thread and then
rebase once more, to catch up to master and to collapse some changes.
--
Paul Bone
Peter Wang
2015-09-24 07:54:00 UTC
Permalink
Hi Paul,

Here's a review of the diff.
diff --git a/Mmake.common.in b/Mmake.common.in
index 760fe5d..6a197ce 100644
--- a/Mmake.common.in
+++ b/Mmake.common.in
@@ -172,9 +172,9 @@ BOEHM_CFLAGS = @ENABLE_BOEHM_LARGE_CONFIG@ \
# Additional options to pass to the C compiler when building Boehm-GC for
# threads.
#
Whitespace change.
diff --git a/compiler/notes/upgrade_boehm_gc.html b/compiler/notes/upgrade_boehm_gc.html
index dd2ea08..901672c 100644
--- a/compiler/notes/upgrade_boehm_gc.html
+++ b/compiler/notes/upgrade_boehm_gc.html
...
+<p>
+Over time we've made some changes to the collector, some of which have not
+been pushed upstream.
+The changes that have not been pushed upstream must be managed by us.
+I've we've forked the bdwgc and libatomic_opts
+repositories.
I've we'eve libatomic_opts

The branch names should be updated in this document.
diff --git a/configure.ac b/configure.ac
index 0a5fca6..47141e5 100644
--- a/configure.ac
+++ b/configure.ac
@@ -3067,7 +3067,7 @@ ENABLE_BOEHM_PARALLEL_MARK=
# This following variable is used for passing any other C compiler
# flags to the version of the Boehm GC built in parallel grades.
#
-BOEHM_MISC_CFLAGS_FOR_THREADS=
+BOEHM_CFLAGS_FOR_THREADS="-DGC_THREADS"
Hmm, it's strange we now need to pass -DGC_THREADS in both
CFLAGS_FOR_THREADS and BOEHM_CFLAGS_FOR_THREADS. (I saw the comment in
the log message.)
@@ -3075,7 +3075,7 @@ avoid_sbrk=
case "$host" in
*solaris*)
- CFLAGS_FOR_THREADS="-DGC_SOLARIS_PTHREADS -D_REENTRANT"
+ CFLAGS_FOR_THREADS="-DGC_THREADS -D_REENTRANT"
THREAD_LIBS="-lpthread -lrt -ldl"
ENABLE_BOEHM_THREAD_LOCAL_ALLOC="-DTHREAD_LOCAL_ALLOC"
ENABLE_BOEHM_PARALLEL_MARK="-DPARALLEL_MARK"
@@ -3085,7 +3085,7 @@ case "$host" in
# Note that for old versions of Linux / glibc,
# you may also need to make sure that you don't
# pass -ansi to gcc.
- CFLAGS_FOR_THREADS="-DGC_LINUX_THREADS -D_THREAD_SAFE -D_REENTRANT"
+ CFLAGS_FOR_THREADS="-DGC_THREADS -D_THREAD_SAFE -D_REENTRANT"
THREAD_LIBS="-lpthread -ldl"
ENABLE_BOEHM_THREAD_LOCAL_ALLOC="-DTHREAD_LOCAL_ALLOC"
ENABLE_BOEHM_PARALLEL_MARK="-DPARALLEL_MARK"
@@ -3096,7 +3096,8 @@ case "$host" in
# (With the current collector these settings only appear to work
# correctly on Linux.)
# XXX disabled as it aborts on GC_free when --enable-gc-munmap is used
- # BOEHM_MISC_CFLAGS_FOR_THREADS="-DHBLKSIZE=32768 -DCPP_LOG_HBLKSIZE=15"
+ # BOEHM_CFLAGS_FOR_THREADS="-DGC_THREADS -DHBLKSIZE=32768 \
+ # -DCPP_LOG_HBLKSIZE=15"
avoid_sbrk=yes
;;
@@ -3104,16 +3105,15 @@ case "$host" in
THREAD_LIBS=""
case "$mercury_cv_cc_type" in
msvc)
- CFLAGS_FOR_THREADS="-DGC_WIN32_THREADS -MD"
+ CFLAGS_FOR_THREADS="-DGC_THREADS -MD"
LDFLAGS_FOR_THREADS="-MD"
LD_LIBFLAGS_FOR_THREADS="-MD"
;;
*)
CFLAGS_FOR_THREADS="$WIN32_GC_THREADLIB"
Add -DGC_THREADS, I guess.
ENABLE_BOEHM_THREAD_LOCAL_ALLOC="-DTHREAD_LOCAL_ALLOC"
- # Disable parallel marking on Windows until next upgrade.
- # It is broken on Windows 7+ (see bdwgc commit 57cc049).
- # ENABLE_BOEHM_PARALLEL_MARK="-DPARALLEL_MARK"
+ ENABLE_BOEHM_PARALLEL_MARK="-DPARALLEL_MARK"
+ BOEHM_CFLAGS_FOR_THREADS="-DGC_THREADS $WIN32_GC_THREADLIB"
;;
esac
# avoid_sbrk?
@@ -3123,7 +3123,7 @@ case "$host" in
THREAD_LIBS=""
case "$mercury_cv_cc_type" in
msvc)
- CFLAGS_FOR_THREADS="-DGC_WIN32_THREADS -MD"
+ CFLAGS_FOR_THREADS="-DGC_THREADS -MD"
LDFLAGS_FOR_THREADS="-MD"
LD_LIBFLAGS_FOR_THREADS="-MD"
;;
@@ -3132,25 +3132,28 @@ case "$host" in
then
# By default the MinGW port of GCC targets the i386
# architecture. We need to tell it to target a later
- # architecture for the GCC built-in atomic ops to be available.
+ # architecture for the GCC built-in atomic ops to be
+ # available.
CFLAGS_FOR_THREADS="$WIN32_GC_THREADLIB -march=i686"
Ditto.
+ BOEHM_CFLAGS_FOR_THREADS="-DGC_THREADS $WIN32_GC_THREADLIB"
THREAD_LIBS="-lpthread"
else
# The compiler is unconditionally linked against the thread
# libraries so if pthreads is not present then we need to
# set THREAD_LIBS to empty in order to avoid linker errors.
CFLAGS_FOR_THREADS="$WIN32_GC_THREADLIB"
+ BOEHM_CFLAGS_FOR_THREADS="-DGC_THREADS $WIN32_GC_THREADLIB"
THREAD_LIBS=
fi
ENABLE_BOEHM_THREAD_LOCAL_ALLOC="-DTHREAD_LOCAL_ALLOC"
- # Disable parallel marking on Windows until next upgrade.
- # ENABLE_BOEHM_PARALLEL_MARK="-DPARALLEL_MARK"
+ ENABLE_BOEHM_PARALLEL_MARK="-DPARALLEL_MARK"
;;
esac
;;
*-w64-mingw*)
CFLAGS_FOR_THREADS="$WIN32_GC_THREADLIB"
+ BOEHM_CFLAGS_FOR_THREADS="-DGC_THREADS $WIN32_GC_THREADLIB"
THREAD_LIBS="-lpthread"
ENABLE_BOEHM_THREAD_LOCAL_ALLOC="-DTHREAD_LOCAL_ALLOC"
# Disable parallel marking on Windows until next upgrade.
@@ -3158,7 +3161,7 @@ case "$host" in
;;
*apple*darwin*)
- CFLAGS_FOR_THREADS="-DGC_DARWIN_THREADS"
+ CFLAGS_FOR_THREADS="-DGC_THREADS"
THREAD_LIBS=""
ENABLE_BOEHM_THREAD_LOCAL_ALLOC="-DTHREAD_LOCAL_ALLOC"
ENABLE_BOEHM_PARALLEL_MARK="-DPARALLEL_MARK"
The freebsd branch is using -DGC_FREEBSD_THREADS, in case you wanted to
update that as well
@@ -3184,6 +3187,7 @@ case "$host" in
# (2) a port of the Boehm gc for that architecture
# that works with threads.
CFLAGS_FOR_THREADS=""
+ BOEHM_CFLAGS_FOR_THREADS=""
;;
esac
There's a code block relating to sbrk that still uses
BOEHM_MISC_CFLAGS_FOR_THREADS here.
@@ -3204,7 +3208,7 @@ AC_SUBST(LDFLAGS_FOR_THREADS)
AC_SUBST(LD_LIBFLAGS_FOR_THREADS)
AC_SUBST(ENABLE_BOEHM_THREAD_LOCAL_ALLOC)
AC_SUBST(ENABLE_BOEHM_PARALLEL_MARK)
-AC_SUBST(BOEHM_MISC_CFLAGS_FOR_THREADS)
+AC_SUBST(BOEHM_CFLAGS_FOR_THREADS)
save_LIBS="$LIBS"
LIBS="$THREAD_LIBS"
diff --git a/library/thread.m b/library/thread.m
index 1e1748d..0177681 100644
--- a/library/thread.m
+++ b/library/thread.m
@@ -263,8 +263,9 @@ spawn_context_2(_, Res, "", !IO) :-
/*
** Store Goal and ThreadId on the top of the new context's stack.
*/
- ctxt->MR_ctxt_sp[0] = Goal;
- ctxt->MR_ctxt_sp[-1] = (MR_Word) ThreadId;
+ ctxt->MR_ctxt_sp += 2;
+ ctxt->MR_ctxt_sp[0] = Goal; /* MR_stackvar(1) */
+ ctxt->MR_ctxt_sp[-1] = (MR_Word) ThreadId; /* MR_stackvar(2) */
MR_schedule_context(ctxt);
@@ -439,6 +440,7 @@ INIT mercury_sys_init_thread_modules
/* Call the closure placed the top of the stack. */
MR_r1 = MR_stackvar(1); /* Goal */
MR_r2 = MR_stackvar(2); /* ThreadId */
+ MR_decr_sp(2);
MR_noprof_call(MR_ENTRY(mercury__do_call_closure_1),
MR_LABEL(mercury__thread__spawn_end_thread));
}
Should this change be committed to master independent of the GC upgrade
change?
diff --git a/prepare.sh b/prepare.sh
new file mode 100755
index 0000000..c9e1e17
--- /dev/null
+++ b/prepare.sh
@@ -0,0 +1,19 @@
+#!/bin/sh
Add a descriptive comment, say:

# This script should be run in a fresh git checkout in order to
# initialise the necessary git submodules and produce the configure
# script.
+
+set -e
+
Can write #!/bin/sh -e
+if [ ! -e boehm_gc/libatomic_ops ]; then
Add a progress message:

echo "Setting up submodules..."
+ if git submodule --quiet init; then
+ git submodule update --remote --checkout
+ ln -s ../libatomic_ops boehm_gc/libatomic_ops || \
+ cp -R libatomic_ops boehm_gc/libatomic_ops
+ else
+ echo There was a problem configuring the submodules. If the
+ echo repositories could not be found then edit .git/config and run
+ echo get submodule update
Quote the strings.
+ exit 1
+ fi
+fi
+
+aclocal -I m4
+autoconf
diff --git a/util/mkinit.c b/util/mkinit.c
index 900426e..ae83bd0 100644
--- a/util/mkinit.c
+++ b/util/mkinit.c
@@ -390,10 +390,15 @@ static const char mercury_funcs1[] =
" ** For the Boehm GC, if the stackbottom argument is NULL then\n"
" ** do not explicitly register the bottom of the stack, but\n"
" ** let the collector determine an appropriate value itself.\n"
+ " **\n"
+ " ** Boehm GC 7.4.2 deprecates the use of GC_stackbottom and\n"
+ " ** prohibits the use of the alterantive GC_register_my_thread()\n"
+ " ** for the primordial thread. Therefore we no-longer attempt to\n"
+ " ** register the bottom of the C stack except on AIX.\n"
" */\n"
" #if defined(MR_HGC)\n"
" MR_hgc_set_stack_bot(stackbottom);\n"
- " #elif defined(MR_BOEHM_GC)\n"
+ " #elif defined(MR_BOEHM_GC) && defined(_AIX)\n"
" if (stackbottom != NULL) {\n"
" GC_stackbottom = stackbottom;\n"
" }\n"
alternative

s/no-longer/do not/

Where does the exception for AIX come from? It should be documented if
possible.

Peter
Paul Bone
2015-09-24 09:58:40 UTC
Permalink
Post by Julien Fischer
Hi Paul,
Here's a review of the diff.
I'll take a look at this in more detail tomorrow, most of it is
Post by Julien Fischer
diff --git a/library/thread.m b/library/thread.m
index 1e1748d..0177681 100644
--- a/library/thread.m
+++ b/library/thread.m
@@ -263,8 +263,9 @@ spawn_context_2(_, Res, "", !IO) :-
/*
** Store Goal and ThreadId on the top of the new context's stack.
*/
- ctxt->MR_ctxt_sp[0] = Goal;
- ctxt->MR_ctxt_sp[-1] = (MR_Word) ThreadId;
+ ctxt->MR_ctxt_sp += 2;
+ ctxt->MR_ctxt_sp[0] = Goal; /* MR_stackvar(1) */
+ ctxt->MR_ctxt_sp[-1] = (MR_Word) ThreadId; /* MR_stackvar(2) */
MR_schedule_context(ctxt);
@@ -439,6 +440,7 @@ INIT mercury_sys_init_thread_modules
/* Call the closure placed the top of the stack. */
MR_r1 = MR_stackvar(1); /* Goal */
MR_r2 = MR_stackvar(2); /* ThreadId */
+ MR_decr_sp(2);
MR_noprof_call(MR_ENTRY(mercury__do_call_closure_1),
MR_LABEL(mercury__thread__spawn_end_thread));
}
Should this change be committed to master independent of the GC upgrade
change?
I don't really see a reason why not to as it fixes what seems to be a
genuine memory error. I think that this is your code, so if you don't know
or remember a reason for it being the way it is I can put it on master.
--
Paul Bone
Peter Wang
2015-09-25 06:18:19 UTC
Permalink
Post by Paul Bone
Post by Peter Wang
Should this change be committed to master independent of the GC upgrade
change?
I don't really see a reason why not to as it fixes what seems to be a
genuine memory error. I think that this is your code, so if you don't know
or remember a reason for it being the way it is I can put it on master.
Please do, thanks.

Peter
Paul Bone
2015-09-29 08:27:31 UTC
Permalink
Post by Julien Fischer
Hi Paul,
Here's a review of the diff.
Cheers.
Post by Julien Fischer
index 0a5fca6..47141e5 100644
--- a/configure.ac
+++ b/configure.ac
@@ -3104,16 +3105,15 @@ case "$host" in
THREAD_LIBS=""
case "$mercury_cv_cc_type" in
msvc)
- CFLAGS_FOR_THREADS="-DGC_WIN32_THREADS -MD"
+ CFLAGS_FOR_THREADS="-DGC_THREADS -MD"
LDFLAGS_FOR_THREADS="-MD"
LD_LIBFLAGS_FOR_THREADS="-MD"
;;
*)
CFLAGS_FOR_THREADS="$WIN32_GC_THREADLIB"
Add -DGC_THREADS, I guess.
My interpretation of boehm_gc/docs/README.macros is that it will be implied
by the the value in $WIN32_GC_THREADLIB, which should be either
-DGC_WIN32_THREADS or -DGC_WIN32_PTHREADS. But this orght to be tested.

I don't know if I've got a suitable windows environment to test this. I've
got a VM with some things on it but i never know if I've set it up
correctly.
Post by Julien Fischer
@@ -3158,7 +3161,7 @@ case "$host" in
;;
*apple*darwin*)
- CFLAGS_FOR_THREADS="-DGC_DARWIN_THREADS"
+ CFLAGS_FOR_THREADS="-DGC_THREADS"
THREAD_LIBS=""
ENABLE_BOEHM_THREAD_LOCAL_ALLOC="-DTHREAD_LOCAL_ALLOC"
ENABLE_BOEHM_PARALLEL_MARK="-DPARALLEL_MARK"
The freebsd branch is using -DGC_FREEBSD_THREADS, in case you wanted to
update that as well
Yeah, done.
Post by Julien Fischer
diff --git a/prepare.sh b/prepare.sh
new file mode 100755
index 0000000..c9e1e17
--- /dev/null
+++ b/prepare.sh
@@ -0,0 +1,19 @@
+#!/bin/sh
...
Post by Julien Fischer
+if [ ! -e boehm_gc/libatomic_ops ]; then
echo "Setting up submodules..."
I've added one at the end too.
Post by Julien Fischer
diff --git a/util/mkinit.c b/util/mkinit.c
index 900426e..ae83bd0 100644
--- a/util/mkinit.c
+++ b/util/mkinit.c
@@ -390,10 +390,15 @@ static const char mercury_funcs1[] =
" ** For the Boehm GC, if the stackbottom argument is NULL then\n"
" ** do not explicitly register the bottom of the stack, but\n"
" ** let the collector determine an appropriate value itself.\n"
+ " **\n"
+ " ** Boehm GC 7.4.2 deprecates the use of GC_stackbottom and\n"
+ " ** prohibits the use of the alterantive GC_register_my_thread()\n"
+ " ** for the primordial thread. Therefore we no-longer attempt to\n"
+ " ** register the bottom of the C stack except on AIX.\n"
" */\n"
" #if defined(MR_HGC)\n"
" MR_hgc_set_stack_bot(stackbottom);\n"
- " #elif defined(MR_BOEHM_GC)\n"
+ " #elif defined(MR_BOEHM_GC) && defined(_AIX)\n"
" if (stackbottom != NULL) {\n"
" GC_stackbottom = stackbottom;\n"
" }\n"
alternative
s/no-longer/do not/
Where does the exception for AIX come from? It should be documented if
possible.
It's explained earlier in that block comment, diff hasn't shown enough
context lines for it to be included in my patch.
--
Paul Bone
Loading...