[m-dev.] Almost ready to merge the update_boehm branch

[Peter Wang]

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.
>  #
> -BOEHM_CFLAGS_FOR_THREADS = @ENABLE_BOEHM_THREAD_LOCAL_ALLOC@ \
> -	@ENABLE_BOEHM_PARALLEL_MARK@ \
> -	@BOEHM_MISC_CFLAGS_FOR_THREADS@
> +BOEHM_CFLAGS_FOR_THREADS = @BOEHM_CFLAGS_FOR_THREADS@ \
> +    @ENABLE_BOEHM_THREAD_LOCAL_ALLOC@ \
> +	@ENABLE_BOEHM_PARALLEL_MARK@

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