[PATCH 0/6] WIP aarch64 support

[PATCH 0/6] WIP aarch64 support

[Efraim Flashner]

Here's my current aarch64 patch set. As attached I don't know if the patches
will apply, I stripped bootstrap bash/mkdir/tar/xz out of the set so the email
won't be too big.

grep without custom phase:
The main purpose of this patch is so that it can be reapplied in the next
patch. Without the patch, fgrep is:

#! /gnu/store/eee...-bootstrap-binaries/bin/sh
exec /gnu/store/eee...-grep-2.28/bin/grep -F ...

and there's no simple way to fix that
This patch should work against master.

patch egrep/fgrep:
I don't believe this actually patches egrep/fgrep, just the version in /tmp
so that it passes the test. Regardless, it passes the test at unpack time,
and it works later on.
This patch breaks all the current architectures, since egrep/fgrep are
binaries pre grep-2.25.

daemon patch:
The if statement should be easy to write, but I'm having a really hard time
with it. Interestingly, when I used the armhf binary install tarball the
daemon there worked.

add bootstrap-binaries:
This one (still) isn't final, the ones currently hosted at the time of this
email were built without the first patch, so building failed.
Even with the patch to the test, not all the tests passed at last check.

gcc patch:
I don't know if this is necessary, but we try not to use lib64. There was
supposed to be another aarch64 gcc patch, to add aarch64 specific flags,
but all of the flags caused the bootstrapping GCCs to fail, so I had to take
them out. We'll have to figure something out, at the very least to make sure
we're targeting ARMv8-a and not 8.1 or 8.2 as more devices come out.

aarch64-linux-gnu target:
This one I tossed in.


Efraim Flashner (6):
  gnu: %static-inputs: Use 'grep' without custom phase.
  gnu: %bootstrap-coreutils&co: Patch egrep/fgrep to work regardless of
    $PATH.
  daemon: On aarch64, use increments of 16 on the stack.
  gnu: Add bootstrap-binaries for 'aarch64-linux'.
  gnu: gcc: Force Aarch64 to use /lib.
  hydra: Add "aarch64-linux-gnu" as a cross-compilation target.

 build-aux/download.scm                     |  20 ++++++++++++-------
 build-aux/hydra/gnu-system.scm             |   1 +
 doc/guix.texi                              |   3 +++
 gnu/local.mk                               |  15 +++++++++++++++
 gnu/packages/bootstrap.scm                 |  30 ++++++++++++++++++++++++++++-
 gnu/packages/bootstrap/aarch64-linux/bash  | Bin 0 -> 1162056 bytes
 gnu/packages/bootstrap/aarch64-linux/mkdir | Bin 0 -> 558216 bytes
 gnu/packages/bootstrap/aarch64-linux/tar   | Bin 0 -> 1085128 bytes
 gnu/packages/bootstrap/aarch64-linux/xz    | Bin 0 -> 738576 bytes
 gnu/packages/gcc.scm                       |   4 ++++
 gnu/packages/make-bootstrap.scm            |  12 +++++++++++-
 m4/guix.m4                                 |   3 ++-
 nix/libstore/build.cc                      |   7 ++++++-
 tests/packages.scm                         |   2 ++
 14 files changed, 86 insertions(+), 11 deletions(-)
 create mode 100755 gnu/packages/bootstrap/aarch64-linux/bash
 create mode 100755 gnu/packages/bootstrap/aarch64-linux/mkdir
 create mode 100755 gnu/packages/bootstrap/aarch64-linux/tar
 create mode 100755 gnu/packages/bootstrap/aarch64-linux/xz

-- 
2.11.1

[PATCH 1/6] gnu: %static-inputs: Use 'grep' without custom phase.

[Efraim Flashner]

This reverts commit 1063d325ea76aa2b00dfcd3d436b16e412103df1 for during
creation of the bootstrap-binaries.

* gnu/packages/make-bootstrap.scm (%static-inputs): Use a custom 'grep'
without the absolute path name in fgrep/egrep.
---
 gnu/packages/make-bootstrap.scm | 12 +++++++++++-
 1 file changed, 11 insertions(+), 1 deletion(-)

diff --git a/gnu/packages/make-bootstrap.scm b/gnu/packages/make-bootstrap.scm
index d2a559c08..c529e03c9 100644
--- a/gnu/packages/make-bootstrap.scm
+++ b/gnu/packages/make-bootstrap.scm
@@ -1,5 +1,6 @@
 ;;; GNU Guix --- Functional package management for GNU
 ;;; Copyright © 2012, 2013, 2014, 2015, 2016 Ludovic Courtès <ludo@gnu.org>
+;;; Copyright © 2017 Efraim Flashner <efraim@flashner.co.il>
 ;;;
 ;;; This file is part of GNU Guix.
 ;;;
@@ -206,7 +207,16 @@ for `sh' in $PATH, and without nscd, and with static NSS modules."
                ("patch" ,patch)
                ("coreutils" ,coreutils)
                ("sed" ,sed)
-               ("grep" ,grep)
+               ;; We don't want to retain a reference to /gnu/store in the
+               ;; bootstrap versions of egrep/fgrep, so we remove the custom
+               ;; phase added since grep@2.25.
+               ("grep" ,(package
+                          (inherit grep)
+                          (arguments
+                            (substitute-keyword-arguments (package-arguments grep)
+                              ((#:phases phases)
+                               `(delete 'fix-egrep-and-fgrep
+                                ,phases))))))
                ("gawk" ,gawk)))
       ("bash" ,static-bash))))
 
-- 
2.11.1

[PATCH 2/6] gnu: %bootstrap-coreutils&co: Patch egrep/fgrep to work regardless of $PATH.

[Efraim Flashner]

This is the bootstrap version of 1063d325ea76aa2b00dfcd3d436b16e412103df1

* gnu/packages/bootstrap.scm (%bootstrap-coreutils&co)[source]: Patch
the absolute location of 'grep' when called from 'egrep' or 'fgrep'.
---
 gnu/packages/bootstrap.scm | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/gnu/packages/bootstrap.scm b/gnu/packages/bootstrap.scm
index 3be6e1246..1dd853260 100644
--- a/gnu/packages/bootstrap.scm
+++ b/gnu/packages/bootstrap.scm
@@ -325,6 +325,10 @@ $out/bin/guile --version~%"
                            (chmod "bin" #o755)
                            (patch-shebang "bin/egrep" path)
                            (patch-shebang "bin/fgrep" path)
+                           ;; Patch 'egrep' and 'fgrep' to execute 'grep' via its
+                           ;; absolute file name instead of searching for it in $PATH.
+                           (substitute* '("bin/egrep" "bin/fgrep")
+                             (("grep") (string-append (getcwd) "/bin/grep")))
                            (chmod "bin" #o555)
                            #t)))
 
-- 
2.11.1

[PATCH 3/6] daemon: On aarch64, use increments of 16 on the stack.

[Efraim Flashner]

man2 clone: EINVAL: ... on aarch64, child_stack must be a multiple of 16.

* nix/libstore/build.cc (DerivationGoal::startBuilder): When on aarch64,
when calling clone(), increment the stack by 16.
---
 nix/libstore/build.cc | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/nix/libstore/build.cc b/nix/libstore/build.cc
index cebc404d1..362b2d91d 100644
--- a/nix/libstore/build.cc
+++ b/nix/libstore/build.cc
@@ -2008,7 +2008,12 @@ void DerivationGoal::startBuilder()
 	char stack[32 * 1024];
 	int flags = CLONE_NEWPID | CLONE_NEWNS | CLONE_NEWIPC | CLONE_NEWUTS | SIGCHLD;
 	if (!fixedOutput) flags |= CLONE_NEWNET;
-	pid = clone(childEntry, stack + sizeof(stack) - 8, flags, this);
+// if statements are hard, fix this
+//#if __AARCH64__
+	pid = clone(childEntry, stack + sizeof(stack) - 16, flags, this);
+//#else
+//	pid = clone(childEntry, stack + sizeof(stack) - 8, flags, this);
+//#endif
 	if (pid == -1)
 	    throw SysError("cloning builder process");
     } else
-- 
2.11.1

[PATCH 4/6] gnu: Add bootstrap-binaries for 'aarch64-linux'.

[Efraim Flashner]

* gnu/packages/bootstrap/aarch64-linux/bash,
gnu/packages/bootstrap/aarch64-linux/mkdir,
gnu/packages/bootstrap/aarch64-linux/xz,
gnu/packages/bootstrap/aarch64-linux/tar: New files.

* gnu/local.mk (bootstrap_aarch64_linuxdir)
(dist_bootstrap_aarch64_linux_DATA)
(nodist_bootstrap_aarch64_linux_DATA): New variables.
(DISTCLEANFILES): Add $(nodist_bootstrap_aarch64_linux_DATA).
(gnu/packages/bootstrap/aarch64-linux/guile-2.0.13.tar.xz): New target.
* build-aux/download.scm (filename->uri): Add aarch64-linux entry.
* gnu/packages/bootstrap.scm (raw-build): Use guile-2.0.13.tar.xz on
aarch64-linux.
(glibc-dynamic-linker, %bootstrap-coreutils&co, %boostrap-binutils)
(%bootstrap-glibc, %bootstrap-gcc): Add aarch64-linux cases.
* m4/guix.m4 (GUIX_SYSTEM_TYPE): Add aarch64 case.
(GUIX_ASSERT_SUPPORTED_SYSTEM): Add aarch64-linux to supported list.
* doc/guix.texi (GNU Distribution): Add aarch64-linux to the list of
supported systems.
* tests/packages.scm (package-search-derivation, snippet): Add aarch64
case.
---
 build-aux/download.scm                     |  20 +++++++++++++-------
 doc/guix.texi                              |   3 +++
 gnu/local.mk                               |  15 +++++++++++++++
 gnu/packages/bootstrap.scm                 |  26 +++++++++++++++++++++++++-
 gnu/packages/bootstrap/aarch64-linux/bash  | Bin 0 -> 1162056 bytes
 gnu/packages/bootstrap/aarch64-linux/mkdir | Bin 0 -> 558216 bytes
 gnu/packages/bootstrap/aarch64-linux/tar   | Bin 0 -> 1085128 bytes
 gnu/packages/bootstrap/aarch64-linux/xz    | Bin 0 -> 738576 bytes
 m4/guix.m4                                 |   3 ++-
 tests/packages.scm                         |   2 ++
 10 files changed, 60 insertions(+), 9 deletions(-)
 create mode 100755 gnu/packages/bootstrap/aarch64-linux/bash
 create mode 100755 gnu/packages/bootstrap/aarch64-linux/mkdir
 create mode 100755 gnu/packages/bootstrap/aarch64-linux/tar
 create mode 100755 gnu/packages/bootstrap/aarch64-linux/xz

diff --git a/build-aux/download.scm b/build-aux/download.scm
index 1e91e4b87..383cdf880 100644
--- a/build-aux/download.scm
+++ b/build-aux/download.scm
@@ -1,6 +1,7 @@
 ;;; GNU Guix --- Functional package management for GNU
 ;;; Copyright © 2012, 2013 Ludovic Courtès <ludo@gnu.org>
 ;;; Copyright © 2014, 2015 Mark H Weaver <mhw@netris.org>
+;;; Copyright © 2017 Efraim Flashner <efraim@flashner.co.il>
 ;;;
 ;;; This file is part of GNU Guix.
 ;;;
@@ -45,13 +46,18 @@
   "Return the URI for FILE."
   (match (string-tokenize file (char-set-complement (char-set #\/)))
     ((_ ... system basename)
-     (string->uri (string-append %url-base "/" system
-                                 (match system
-                                   ("armhf-linux"
-                                    "/20150101/")
-                                   (_
-                                    "/20131110/"))
-                                 basename)))))
+     (string->uri
+       (match system
+        ("aarch64-linux"
+         (string-append "http://flashner.co.il/guix/bootstrap/aarch64-linux"
+                        "/20170209/" basename))
+        (_ (string-append %url-base "/" system
+                          (match system
+                                 ("armhf-linux"
+                                  "/20150101/")
+                                 (_
+                                  "/20131110/"))
+                          basename)))))))
 
 (match (command-line)
   ((_ file expected-hash)
diff --git a/doc/guix.texi b/doc/guix.texi
index 6acde6621..5242b1fc4 100644
--- a/doc/guix.texi
+++ b/doc/guix.texi
@@ -6669,6 +6669,9 @@ ARMv7-A architecture with hard float, Thumb-2 and NEON,
 using the EABI hard-float application binary interface (ABI),
 and Linux-Libre kernel.
 
+@item aarch64-linux
+little-endian 64-bit ARMv8 processors.
+
 @item mips64el-linux
 little-endian 64-bit MIPS processors, specifically the Loongson series,
 n32 ABI, and Linux-Libre kernel.
diff --git a/gnu/local.mk b/gnu/local.mk
index ad3be4b13..5a6fbb08e 100644
--- a/gnu/local.mk
+++ b/gnu/local.mk
@@ -9,6 +9,7 @@
 # Copyright © 2016 Adonay "adfeno" Felipe Nogueira <https://libreplanet.org/wiki/User:Adfeno> <adfeno@openmailbox.org>
 # Copyright © 2016, 2017 Ricardo Wurmus <rekado@elephly.net>
 # Copyright © 2016 Ben Woodcroft <donttrustben@gmail.com>
+# Copyright © 2017 Efraim Flashner <efraim@flashner.co.il>
 #
 # This file is part of GNU Guix.
 #
@@ -970,6 +971,7 @@ bootstrapdir = $(guilemoduledir)/%D%/packages/bootstrap
 bootstrap_x86_64_linuxdir = $(bootstrapdir)/x86_64-linux
 bootstrap_i686_linuxdir = $(bootstrapdir)/i686-linux
 bootstrap_armhf_linuxdir = $(bootstrapdir)/armhf-linux
+bootstrap_aarch64_linuxdir = $(bootstrapdir)/aarch64-linux
 bootstrap_mips64el_linuxdir = $(bootstrapdir)/mips64el-linux
 
 dist_bootstrap_x86_64_linux_DATA =		\
@@ -990,6 +992,12 @@ dist_bootstrap_armhf_linux_DATA =		\
   %D%/packages/bootstrap/armhf-linux/tar	\
   %D%/packages/bootstrap/armhf-linux/xz
 
+dist_bootstrap_aarch64_linux_DATA =		\
+  %D%/packages/bootstrap/aarch64-linux/bash	\
+  %D%/packages/bootstrap/aarch64-linux/mkdir	\
+  %D%/packages/bootstrap/aarch64-linux/tar	\
+  %D%/packages/bootstrap/aarch64-linux/xz
+
 dist_bootstrap_mips64el_linux_DATA =		\
   %D%/packages/bootstrap/mips64el-linux/bash	\
   %D%/packages/bootstrap/mips64el-linux/mkdir	\
@@ -1004,6 +1012,8 @@ nodist_bootstrap_i686_linux_DATA =					\
   %D%/packages/bootstrap/i686-linux/guile-2.0.9.tar.xz
 nodist_bootstrap_armhf_linux_DATA =					\
   %D%/packages/bootstrap/armhf-linux/guile-2.0.11.tar.xz
+nodist_bootstrap_aarch64_linux_DATA =					\
+  %D%/packages/bootstrap/aarch64-linux/guile-2.0.13.tar.xz
 nodist_bootstrap_mips64el_linux_DATA =					\
   %D%/packages/bootstrap/mips64el-linux/guile-2.0.9.tar.xz
 
@@ -1016,6 +1026,7 @@ DISTCLEANFILES =				\
   $(nodist_bootstrap_x86_64_linux_DATA)		\
   $(nodist_bootstrap_i686_linux_DATA)		\
   $(nodist_bootstrap_armhf_linux_DATA)		\
+  $(nodist_bootstrap_aarch64_linux_DATA)		\
   $(nodist_bootstrap_mips64el_linux_DATA)
 
 # Method to download a file from an external source.
@@ -1036,6 +1047,10 @@ DOWNLOAD_FILE =								\
 	$(AM_V_DL)$(MKDIR_P) `dirname "$@"`;	\
 	$(DOWNLOAD_FILE) "$@"			\
 	  "e551d05d4d385d6706ab8d574856a087758294dc90ab4c06e70a157a685e23d6"
+%D%/packages/bootstrap/aarch64-linux/guile-2.0.13.tar.xz:
+	$(AM_V_DL)$(MKDIR_P) `dirname "$@"`;	\
+	$(DOWNLOAD_FILE) "$@"			\
+	  "7cbc2580b862b1dd02c365b4a48c804040f2f7d57689d822a3d6c3f13b9868d2"
 %D%/packages/bootstrap/mips64el-linux/guile-2.0.9.tar.xz:
 	$(AM_V_DL)$(MKDIR_P) `dirname "$@"`;	\
 	$(DOWNLOAD_FILE) "$@" 			\
diff --git a/gnu/packages/bootstrap.scm b/gnu/packages/bootstrap.scm
index 1dd853260..21d743a98 100644
--- a/gnu/packages/bootstrap.scm
+++ b/gnu/packages/bootstrap.scm
@@ -1,6 +1,7 @@
 ;;; GNU Guix --- Functional package management for GNU
 ;;; Copyright © 2012, 2013, 2014, 2015, 2016, 2017 Ludovic Courtès <ludo@gnu.org>
 ;;; Copyright © 2014, 2015 Mark H Weaver <mhw@netris.org>
+;;; Copyright © 2017 Efraim Flashner <efraim@flashner.co.il>
 ;;;
 ;;; This file is part of GNU Guix.
 ;;;
@@ -202,6 +203,8 @@ successful, or false to signal an error."
          (guile (->store (match system
                            ("armhf-linux"
                             "guile-2.0.11.tar.xz")
+                           ("aarch64-linux"
+                            "guile-2.0.13.tar.xz")
                            (_
                             "guile-2.0.9.tar.xz"))))
          ;; The following code, run by the bootstrap guile after it is
@@ -290,7 +293,8 @@ $out/bin/guile --version~%"
   ;; This is where the initial binaries come from.
   '("ftp://alpha.gnu.org/gnu/guix/bootstrap"
     "http://alpha.gnu.org/gnu/guix/bootstrap"
-    "http://www.fdn.fr/~lcourtes/software/guix/packages"))
+    "http://www.fdn.fr/~lcourtes/software/guix/packages"
+    "http://flashner.co.il/guix/bootstrap/"))
 
 (define %bootstrap-coreutils&co
   (package-from-tarball "bootstrap-binaries"
@@ -301,6 +305,8 @@ $out/bin/guile --version~%"
                                           (match system
                                             ("armhf-linux"
                                              "/20150101/static-binaries.tar.xz")
+                                            ("aarch64-linux"
+                                             "/20170209/static-binaries.tar.xz")
                                             (_
                                              "/20131110/static-binaries.tar.xz")))
                                      %bootstrap-base-urls))
@@ -315,6 +321,9 @@ $out/bin/guile --version~%"
                               ("armhf-linux"
                                (base32
                                 "0gf0fn2kbpxkjixkmx5f4z6hv6qpmgixl69zgg74dbsfdfj8jdv5"))
+                              ("aarch64-linux"
+                               (base32
+                                "0kaj9xbxjglzvcs7g8gvsf5sysy1hjdc6hxn7dv51sgpv32qqgvr"))
                               ("mips64el-linux"
                                (base32
                                 "072y4wyfsj1bs80r6vbybbafy8ya4vfy7qj25dklwk97m6g71753"))))))
@@ -341,6 +350,8 @@ $out/bin/guile --version~%"
                                           (match system
                                             ("armhf-linux"
                                              "/20150101/binutils-2.25.tar.xz")
+                                            ("aarch64-linux"
+                                             "/20170209/binutils-2.27.tar.xz")
                                             (_
                                              "/20131110/binutils-2.23.2.tar.xz")))
                                      %bootstrap-base-urls))
@@ -355,6 +366,9 @@ $out/bin/guile --version~%"
                               ("armhf-linux"
                                (base32
                                 "1v7dj6bzn6m36f20gw31l99xaabq4xrhrx3gwqkhhig0mdlmr69q"))
+                              ("aarch64-linux"
+                               (base32
+                                "111s7ilfiby033rczc71797xrmaa3qlv179wdvsaq132pd51xv3n"))
                               ("mips64el-linux"
                                (base32
                                 "1x8kkhcxmfyzg1ddpz2pxs6fbdl6412r7x0nzbmi5n7mj8zw2gy7"))))))
@@ -402,6 +416,8 @@ $out/bin/guile --version~%"
                                     (match (%current-system)
                                       ("armhf-linux"
                                        "/20150101/glibc-2.20.tar.xz")
+                                      ("aarch64-linux"
+                                       "/20170209/glibc-2.25.tar.xz")
                                       (_
                                        "/20131110/glibc-2.18.tar.xz")))
                                %bootstrap-base-urls))
@@ -416,6 +432,9 @@ $out/bin/guile --version~%"
                         ("armhf-linux"
                          (base32
                           "18cmgvpllqfpn6khsmivqib7ys8ymnq0hdzi3qp24prik0ykz8gn"))
+                        ("aarch64-linux"
+                         (base32
+                          "07n3xn4znp5z8h3cm0lqifl5d6yrdygl8zlnckwyvpmk3aarrm2j"))
                         ("mips64el-linux"
                          (base32
                           "0k97a3whzx3apsi9n2cbsrr79ad6lh00klxph9hw4fqyp1abkdsg")))))))))
@@ -480,6 +499,8 @@ exec ~a/bin/.gcc-wrapped -B~a/lib \
                                     (match (%current-system)
                                       ("armhf-linux"
                                        "/20150101/gcc-4.8.4.tar.xz")
+                                      ("aarch64-linux"
+                                       "/20170209/gcc-5.4.0.tar.xz")
                                       (_
                                        "/20131110/gcc-4.8.2.tar.xz")))
                                %bootstrap-base-urls))
@@ -494,6 +515,9 @@ exec ~a/bin/.gcc-wrapped -B~a/lib \
                         ("armhf-linux"
                          (base32
                           "0ghz825yzp43fxw53kd6afm8nkz16f7dxi9xi40bfwc8x3nbbr8v"))
+                        ("aarch64-linux"
+                         (base32
+                          "06pdsmff7r3hw1x7c6ipbmxxd9j2d215hf422i2drjigacc25pq3"))
                         ("mips64el-linux"
                          (base32
                           "1m5miqkyng45l745n0sfafdpjkqv9225xf44jqkygwsipj2cv9ks")))))))))
diff --git a/m4/guix.m4 b/m4/guix.m4
index 663059841..e546b8f4d 100644
--- a/m4/guix.m4
+++ b/m4/guix.m4
@@ -1,6 +1,7 @@
 dnl GNU Guix --- Functional package management for GNU
 dnl Copyright © 2012, 2013, 2014, 2015, 2016 Ludovic Courtès <ludo@gnu.org>
 dnl Copyright © 2014 Mark H Weaver <mhw@netris.org>
+dnl Copyright © 2017 Efraim Flashner <efraim@flashner.co.il>
 dnl
 dnl This file is part of GNU Guix.
 dnl
@@ -105,7 +106,7 @@ courageous and port the GNU System distribution to it (see
   # Currently only Linux-based systems are supported, and only on some
   # platforms.
   case "$guix_system" in
-    x86_64-linux|i686-linux|armhf-linux|mips64el-linux)
+    x86_64-linux|i686-linux|armhf-linux|aarch64-linux|mips64el-linux)
       ;;
     *)
       if test "x$guix_courageous" = "xyes"; then
diff --git a/tests/packages.scm b/tests/packages.scm
index 962f120ea..d509a1e51 100644
--- a/tests/packages.scm
+++ b/tests/packages.scm
@@ -380,6 +380,8 @@
   (let* ((file   (search-bootstrap-binary (match (%current-system)
                                             ("armhf-linux"
                                              "guile-2.0.11.tar.xz")
+                                            ("aarch64-linux"
+                                             "guile-2.0.13.tar.xz")
                                             (_
                                              "guile-2.0.9.tar.xz"))
                                           (%current-system)))
-- 
2.11.1

[PATCH 5/6] gnu: gcc: Force Aarch64 to use /lib.

[Efraim Flashner]

* gnu/packages/gcc.scm (gcc)[arguments]: On aarch64 replace force libdir
to be lib and not lib64.
---
 gnu/packages/gcc.scm | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/gnu/packages/gcc.scm b/gnu/packages/gcc.scm
index 075642ebd..2bd630d5d 100644
--- a/gnu/packages/gcc.scm
+++ b/gnu/packages/gcc.scm
@@ -263,6 +263,10 @@ where the OS part is overloaded to denote a specific ABI---into GCC
                 (("static char const sed_cmd_z\\[\\] =.*;")
                  "static char const sed_cmd_z[] = \"sed\";"))
 
+              ;; Force Aarch64 libdir to be /lib and not /lib64
+              (substitute* "gcc/config/aarch64/t-aarch64-linux"
+                (("lib64") "lib"))
+
               (when (file-exists? "libbacktrace")
                 ;; GCC 4.8+ comes with libbacktrace.  By default it builds
                 ;; with -Werror, which fails with a -Wcast-qual error in glibc
-- 
2.11.1

[PATCH 6/6] hydra: Add "aarch64-linux-gnu" as a cross-compilation target.

[Efraim Flashner]

* build-aux/hydra/gnu-system.scm (%cross-targets): Add
"aarch64-linux-gnu".
---
 build-aux/hydra/gnu-system.scm | 1 +
 1 file changed, 1 insertion(+)

diff --git a/build-aux/hydra/gnu-system.scm b/build-aux/hydra/gnu-system.scm
index 17c224ea4..53fa637f1 100644
--- a/build-aux/hydra/gnu-system.scm
+++ b/build-aux/hydra/gnu-system.scm
@@ -123,6 +123,7 @@ SYSTEM."
   '("mips64el-linux-gnu"
     "mips64el-linux-gnuabi64"
     "arm-linux-gnueabihf"
+    "aarch64-linux-gnu"
     "powerpc-linux-gnu"
     "i586-pc-gnu"                                 ;aka. GNU/Hurd
     "i686-w64-mingw32"))
-- 
2.11.1

Re: [PATCH 5/6] gnu: gcc: Force Aarch64 to use /lib.

[Danny Milosavljevic]

> +              ;; Force Aarch64 libdir to be /lib and not /lib64
> +              (substitute* "gcc/config/aarch64/t-aarch64-linux"
> +                (("lib64") "lib"))
> +

I'd amend the comment to say why.

Re: [PATCH 1/6] gnu: %static-inputs: Use 'grep' without custom phase.

[Ludovic Courtès]

Efraim Flashner <efraim@flashner.co.il> skribis:

> This reverts commit 1063d325ea76aa2b00dfcd3d436b16e412103df1 for during
> creation of the bootstrap-binaries.
>
> * gnu/packages/make-bootstrap.scm (%static-inputs): Use a custom 'grep'
> without the absolute path name in fgrep/egrep.

[...]

> +               ;; We don't want to retain a reference to /gnu/store in the
> +               ;; bootstrap versions of egrep/fgrep, so we remove the custom
> +               ;; phase added since grep@2.25.

Maybe add something like: “The effect is that 'egrep' and 'fgrep' look
for 'grep' in $PATH.”

OK for master!

Thanks,
Ludo’.

Re: [PATCH 2/6] gnu: %bootstrap-coreutils&co: Patch egrep/fgrep to work regardless of $PATH.

[Ludovic Courtès]

Efraim Flashner <efraim@flashner.co.il> skribis:

> This is the bootstrap version of 1063d325ea76aa2b00dfcd3d436b16e412103df1
>
> * gnu/packages/bootstrap.scm (%bootstrap-coreutils&co)[source]: Patch
> the absolute location of 'grep' when called from 'egrep' or 'fgrep'.
> ---
>  gnu/packages/bootstrap.scm | 4 ++++
>  1 file changed, 4 insertions(+)
>
> diff --git a/gnu/packages/bootstrap.scm b/gnu/packages/bootstrap.scm
> index 3be6e1246..1dd853260 100644
> --- a/gnu/packages/bootstrap.scm
> +++ b/gnu/packages/bootstrap.scm
> @@ -325,6 +325,10 @@ $out/bin/guile --version~%"
>                             (chmod "bin" #o755)
>                             (patch-shebang "bin/egrep" path)
>                             (patch-shebang "bin/fgrep" path)
> +                           ;; Patch 'egrep' and 'fgrep' to execute 'grep' via its
> +                           ;; absolute file name instead of searching for it in $PATH.
> +                           (substitute* '("bin/egrep" "bin/fgrep")
> +                             (("grep") (string-append (getcwd) "/bin/grep")))

Am I right that this is not needed if we produce bootstrap binaries with
patch #1 applied?  (That is, where ‘egrep’ and ‘fgrep’ look for ‘grep’
in $PATH.)

OTOH it cannot hurt, so it’s best to apply it.

Have you check whether it works on the other arches, which use an old
grep where ‘egrep’ and ‘fgrep’ are not scripts?  (‘substitute*’ might
throw an exception if it’s passed a binary file, though I think that’s
not the case here.)

If it doesn’t break the other arches, OK for ‘core-updates’, thanks!

Ludo’.

Re: [PATCH 3/6] daemon: On aarch64, use increments of 16 on the stack.

[Ludovic Courtès]

[-- Attachment #1: Type: text/plain, Size: 1170 bytes --]

Efraim Flashner <efraim@flashner.co.il> skribis:

> man2 clone: EINVAL: ... on aarch64, child_stack must be a multiple of 16.
>
> * nix/libstore/build.cc (DerivationGoal::startBuilder): When on aarch64,
> when calling clone(), increment the stack by 16.
> ---
>  nix/libstore/build.cc | 7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
>
> diff --git a/nix/libstore/build.cc b/nix/libstore/build.cc
> index cebc404d1..362b2d91d 100644
> --- a/nix/libstore/build.cc
> +++ b/nix/libstore/build.cc
> @@ -2008,7 +2008,12 @@ void DerivationGoal::startBuilder()
>  	char stack[32 * 1024];
>  	int flags = CLONE_NEWPID | CLONE_NEWNS | CLONE_NEWIPC | CLONE_NEWUTS | SIGCHLD;
>  	if (!fixedOutput) flags |= CLONE_NEWNET;
> -	pid = clone(childEntry, stack + sizeof(stack) - 8, flags, this);
> +// if statements are hard, fix this
> +//#if __AARCH64__
> +	pid = clone(childEntry, stack + sizeof(stack) - 16, flags, this);
> +//#else
> +//	pid = clone(childEntry, stack + sizeof(stack) - 8, flags, this);
> +//#endif

I think we can make it unconditional.  Could you test whether the
attached patch works for aarch64?

Thanks!

Ludo’.


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: Type: text/x-patch, Size: 819 bytes --]

diff --git a/nix/libstore/build.cc b/nix/libstore/build.cc
index cebc404d1..9b7bb5391 100644
--- a/nix/libstore/build.cc
+++ b/nix/libstore/build.cc
@@ -26,6 +26,7 @@
 #include <errno.h>
 #include <stdio.h>
 #include <cstring>
+#include <stdint.h>
 
 #include <pwd.h>
 #include <grp.h>
@@ -2008,7 +2009,11 @@ void DerivationGoal::startBuilder()
 	char stack[32 * 1024];
 	int flags = CLONE_NEWPID | CLONE_NEWNS | CLONE_NEWIPC | CLONE_NEWUTS | SIGCHLD;
 	if (!fixedOutput) flags |= CLONE_NEWNET;
-	pid = clone(childEntry, stack + sizeof(stack) - 8, flags, this);
+
+	/* Ensure proper alignment on the stack.  On aarch64, it has to be 16
+	   bytes.  */
+	pid = clone(childEntry, (char *)(((uintptr_t)stack + 16) & ~0xf),
+		    flags, this);
 	if (pid == -1)
 	    throw SysError("cloning builder process");
     } else

Re: [PATCH 4/6] gnu: Add bootstrap-binaries for 'aarch64-linux'.

[Ludovic Courtès]

Efraim Flashner <efraim@flashner.co.il> skribis:

> * gnu/packages/bootstrap/aarch64-linux/bash,
> gnu/packages/bootstrap/aarch64-linux/mkdir,
> gnu/packages/bootstrap/aarch64-linux/xz,
> gnu/packages/bootstrap/aarch64-linux/tar: New files.
>
> * gnu/local.mk (bootstrap_aarch64_linuxdir)
> (dist_bootstrap_aarch64_linux_DATA)
> (nodist_bootstrap_aarch64_linux_DATA): New variables.
> (DISTCLEANFILES): Add $(nodist_bootstrap_aarch64_linux_DATA).
> (gnu/packages/bootstrap/aarch64-linux/guile-2.0.13.tar.xz): New target.
> * build-aux/download.scm (filename->uri): Add aarch64-linux entry.
> * gnu/packages/bootstrap.scm (raw-build): Use guile-2.0.13.tar.xz on
> aarch64-linux.
> (glibc-dynamic-linker, %bootstrap-coreutils&co, %boostrap-binutils)
> (%bootstrap-glibc, %bootstrap-gcc): Add aarch64-linux cases.
> * m4/guix.m4 (GUIX_SYSTEM_TYPE): Add aarch64 case.
> (GUIX_ASSERT_SUPPORTED_SYSTEM): Add aarch64-linux to supported list.
> * doc/guix.texi (GNU Distribution): Add aarch64-linux to the list of
> supported systems.
> * tests/packages.scm (package-search-derivation, snippet): Add aarch64
> case.

This one looks good.  It would be nice to specify which commit was used
to produce the binaries though, and I would encourage using Guile 2.0.14
(I’m afraid you’ll be mad at me for asking you that ;-)) because it’s
bit-reproducible.

How does that sound?

> +@item aarch64-linux
> +little-endian 64-bit ARMv8 processors.

I think they call it ARMv9 no?  I would also add “This is currently in
an experimental stage, with limited support.  @xref{Contributing}, for
how to help!”.

Thanks,
Ludo’.

Re: [PATCH 5/6] gnu: gcc: Force Aarch64 to use /lib.

[Ludovic Courtès]

Danny Milosavljevic <dannym@scratchpost.org> skribis:

>> +              ;; Force Aarch64 libdir to be /lib and not /lib64
>> +              (substitute* "gcc/config/aarch64/t-aarch64-linux"
>> +                (("lib64") "lib"))
>> +
>
> I'd amend the comment to say why.

I think we should just skip this patch.  There’s no reason one
architecture should be treated different from the others in that
respect.

WDYT, Efraim?

Ludo’.

Re: [PATCH 6/6] hydra: Add "aarch64-linux-gnu" as a cross-compilation target.

[Ludovic Courtès]

Efraim Flashner <efraim@flashner.co.il> skribis:

> * build-aux/hydra/gnu-system.scm (%cross-targets): Add
> "aarch64-linux-gnu".

OK for core-updates or master, whichever is able to cross-build to that
platform.  :-)

Thank you!

Ludo'.

Re: [PATCH 2/6] gnu: %bootstrap-coreutils&co: Patch egrep/fgrep to work regardless of $PATH.

[Efraim Flashner]

[-- Attachment #1: Type: text/plain, Size: 2248 bytes --]

On Tue, Feb 14, 2017 at 09:35:58AM +0100, Ludovic Courtès wrote:
> Efraim Flashner <efraim@flashner.co.il> skribis:
> 
> > This is the bootstrap version of 1063d325ea76aa2b00dfcd3d436b16e412103df1
> >
> > * gnu/packages/bootstrap.scm (%bootstrap-coreutils&co)[source]: Patch
> > the absolute location of 'grep' when called from 'egrep' or 'fgrep'.
> > ---
> >  gnu/packages/bootstrap.scm | 4 ++++
> >  1 file changed, 4 insertions(+)
> >
> > diff --git a/gnu/packages/bootstrap.scm b/gnu/packages/bootstrap.scm
> > index 3be6e1246..1dd853260 100644
> > --- a/gnu/packages/bootstrap.scm
> > +++ b/gnu/packages/bootstrap.scm
> > @@ -325,6 +325,10 @@ $out/bin/guile --version~%"
> >                             (chmod "bin" #o755)
> >                             (patch-shebang "bin/egrep" path)
> >                             (patch-shebang "bin/fgrep" path)
> > +                           ;; Patch 'egrep' and 'fgrep' to execute 'grep' via its
> > +                           ;; absolute file name instead of searching for it in $PATH.
> > +                           (substitute* '("bin/egrep" "bin/fgrep")
> > +                             (("grep") (string-append (getcwd) "/bin/grep")))
> 
> Am I right that this is not needed if we produce bootstrap binaries with
> patch #1 applied?  (That is, where ‘egrep’ and ‘fgrep’ look for ‘grep’
> in $PATH.)
> 
> OTOH it cannot hurt, so it’s best to apply it.
> 
> Have you check whether it works on the other arches, which use an old
> grep where ‘egrep’ and ‘fgrep’ are not scripts?  (‘substitute*’ might
> throw an exception if it’s passed a binary file, though I think that’s
> not the case here.)
> 
> If it doesn’t break the other arches, OK for ‘core-updates’, thanks!
> 
> Ludo’.

before grep-2.25 egrep and fgrep are binary files. The one time I tried
it on x86_64 it failed spectacularly. In the end it definately needs
some sort of test to make sure its aarch64 (or another newly added
target)

-- 
Efraim Flashner   <efraim@flashner.co.il>   אפרים פלשנר
GPG key = A28B F40C 3E55 1372 662D  14F7 41AA E7DC CA3D 8351
Confidentiality cannot be guaranteed on emails sent or received unencrypted

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 5/6] gnu: gcc: Force Aarch64 to use /lib.

[Efraim Flashner]

[-- Attachment #1: Type: text/plain, Size: 918 bytes --]

On Tue, Feb 14, 2017 at 09:51:47AM +0100, Ludovic Courtès wrote:
> Danny Milosavljevic <dannym@scratchpost.org> skribis:
> 
> >> +              ;; Force Aarch64 libdir to be /lib and not /lib64
> >> +              (substitute* "gcc/config/aarch64/t-aarch64-linux"
> >> +                (("lib64") "lib"))
> >> +
> >
> > I'd amend the comment to say why.
> 
> I think we should just skip this patch.  There’s no reason one
> architecture should be treated different from the others in that
> respect.
> 
> WDYT, Efraim?
> 
> Ludo’.

I don't think it should cause a problem either way. As far as I can tell
it doesn't make a difference to the software built further down the
line.

-- 
Efraim Flashner   <efraim@flashner.co.il>   אפרים פלשנר
GPG key = A28B F40C 3E55 1372 662D  14F7 41AA E7DC CA3D 8351
Confidentiality cannot be guaranteed on emails sent or received unencrypted

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 1/6] gnu: %static-inputs: Use 'grep' without custom phase.

[Efraim Flashner]

[-- Attachment #1: Type: text/plain, Size: 1104 bytes --]

On Tue, Feb 14, 2017 at 09:30:01AM +0100, Ludovic Courtès wrote:
> Efraim Flashner <efraim@flashner.co.il> skribis:
> 
> > This reverts commit 1063d325ea76aa2b00dfcd3d436b16e412103df1 for during
> > creation of the bootstrap-binaries.
> >
> > * gnu/packages/make-bootstrap.scm (%static-inputs): Use a custom 'grep'
> > without the absolute path name in fgrep/egrep.
> 
> [...]
> 
> > +               ;; We don't want to retain a reference to /gnu/store in the
> > +               ;; bootstrap versions of egrep/fgrep, so we remove the custom
> > +               ;; phase added since grep@2.25.
> 
> Maybe add something like: “The effect is that 'egrep' and 'fgrep' look
> for 'grep' in $PATH.”
> 
> OK for master!
> 
> Thanks,
> Ludo’.

Sounds good. I'll rebuild the bootstrap-binaries with guile-2.0.14 and
make sure that everything is working as it should.

-- 
Efraim Flashner   <efraim@flashner.co.il>   אפרים פלשנר
GPG key = A28B F40C 3E55 1372 662D  14F7 41AA E7DC CA3D 8351
Confidentiality cannot be guaranteed on emails sent or received unencrypted

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 4/6] gnu: Add bootstrap-binaries for 'aarch64-linux'.

[Efraim Flashner]

[-- Attachment #1: Type: text/plain, Size: 2503 bytes --]

On Tue, Feb 14, 2017 at 09:51:01AM +0100, Ludovic Courtès wrote:
> Efraim Flashner <efraim@flashner.co.il> skribis:
> 
> > * gnu/packages/bootstrap/aarch64-linux/bash,
> > gnu/packages/bootstrap/aarch64-linux/mkdir,
> > gnu/packages/bootstrap/aarch64-linux/xz,
> > gnu/packages/bootstrap/aarch64-linux/tar: New files.
> >
> > * gnu/local.mk (bootstrap_aarch64_linuxdir)
> > (dist_bootstrap_aarch64_linux_DATA)
> > (nodist_bootstrap_aarch64_linux_DATA): New variables.
> > (DISTCLEANFILES): Add $(nodist_bootstrap_aarch64_linux_DATA).
> > (gnu/packages/bootstrap/aarch64-linux/guile-2.0.13.tar.xz): New target.
> > * build-aux/download.scm (filename->uri): Add aarch64-linux entry.
> > * gnu/packages/bootstrap.scm (raw-build): Use guile-2.0.13.tar.xz on
> > aarch64-linux.
> > (glibc-dynamic-linker, %bootstrap-coreutils&co, %boostrap-binutils)
> > (%bootstrap-glibc, %bootstrap-gcc): Add aarch64-linux cases.
> > * m4/guix.m4 (GUIX_SYSTEM_TYPE): Add aarch64 case.
> > (GUIX_ASSERT_SUPPORTED_SYSTEM): Add aarch64-linux to supported list.
> > * doc/guix.texi (GNU Distribution): Add aarch64-linux to the list of
> > supported systems.
> > * tests/packages.scm (package-search-derivation, snippet): Add aarch64
> > case.
> 
> This one looks good.  It would be nice to specify which commit was used
> to produce the binaries though, and I would encourage using Guile 2.0.14
> (I’m afraid you’ll be mad at me for asking you that ;-)) because it’s
> bit-reproducible.
> 
> How does that sound?

That actually sounds like a great idea. I started by copying Mark's
armhf patch, and I was wondering which commit he used. As far as
rebuilding the bootstrap-binaries, its much faster on my current
machine. (And by much faster I mean around 10 hours, not 15.)

As far as reproducability, of the 5 targets, in the past I found guile
and gcc to be unreproducable, so its great to see it down to just gcc.

> 
> > +@item aarch64-linux
> > +little-endian 64-bit ARMv8 processors.
> 
> I think they call it ARMv9 no?  I would also add “This is currently in
> an experimental stage, with limited support.  @xref{Contributing}, for
> how to help!”.

I need to check if its ARMv8 or ARMv8-a, I don't actually remember.

> 
> Thanks,
> Ludo’.

-- 
Efraim Flashner   <efraim@flashner.co.il>   אפרים פלשנר
GPG key = A28B F40C 3E55 1372 662D  14F7 41AA E7DC CA3D 8351
Confidentiality cannot be guaranteed on emails sent or received unencrypted

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 3/6] daemon: On aarch64, use increments of 16 on the stack.

[Efraim Flashner]

[-- Attachment #1: Type: text/plain, Size: 2516 bytes --]

On Tue, Feb 14, 2017 at 09:47:30AM +0100, Ludovic Courtès wrote:
> Efraim Flashner <efraim@flashner.co.il> skribis:
> 
> > man2 clone: EINVAL: ... on aarch64, child_stack must be a multiple of 16.
> >
> > * nix/libstore/build.cc (DerivationGoal::startBuilder): When on aarch64,
> > when calling clone(), increment the stack by 16.
> > ---
> >  nix/libstore/build.cc | 7 ++++++-
> >  1 file changed, 6 insertions(+), 1 deletion(-)
> >
> > diff --git a/nix/libstore/build.cc b/nix/libstore/build.cc
> > index cebc404d1..362b2d91d 100644
> > --- a/nix/libstore/build.cc
> > +++ b/nix/libstore/build.cc
> > @@ -2008,7 +2008,12 @@ void DerivationGoal::startBuilder()
> >  	char stack[32 * 1024];
> >  	int flags = CLONE_NEWPID | CLONE_NEWNS | CLONE_NEWIPC | CLONE_NEWUTS | SIGCHLD;
> >  	if (!fixedOutput) flags |= CLONE_NEWNET;
> > -	pid = clone(childEntry, stack + sizeof(stack) - 8, flags, this);
> > +// if statements are hard, fix this
> > +//#if __AARCH64__
> > +	pid = clone(childEntry, stack + sizeof(stack) - 16, flags, this);
> > +//#else
> > +//	pid = clone(childEntry, stack + sizeof(stack) - 8, flags, this);
> > +//#endif
> 
> I think we can make it unconditional.  Could you test whether the
> attached patch works for aarch64?
> 
> Thanks!
> 
> Ludo’.
> 

I don't get 'failed to clone' or similar error like I did without
either patch, so it looks good.

> diff --git a/nix/libstore/build.cc b/nix/libstore/build.cc
> index cebc404d1..9b7bb5391 100644
> --- a/nix/libstore/build.cc
> +++ b/nix/libstore/build.cc
> @@ -26,6 +26,7 @@
>  #include <errno.h>
>  #include <stdio.h>
>  #include <cstring>
> +#include <stdint.h>
>  
>  #include <pwd.h>
>  #include <grp.h>
> @@ -2008,7 +2009,11 @@ void DerivationGoal::startBuilder()
>  	char stack[32 * 1024];
>  	int flags = CLONE_NEWPID | CLONE_NEWNS | CLONE_NEWIPC | CLONE_NEWUTS | SIGCHLD;
>  	if (!fixedOutput) flags |= CLONE_NEWNET;
> -	pid = clone(childEntry, stack + sizeof(stack) - 8, flags, this);
> +
> +	/* Ensure proper alignment on the stack.  On aarch64, it has to be 16
> +	   bytes.  */
> +	pid = clone(childEntry, (char *)(((uintptr_t)stack + 16) & ~0xf),
> +		    flags, this);
>  	if (pid == -1)
>  	    throw SysError("cloning builder process");
>      } else


-- 
Efraim Flashner   <efraim@flashner.co.il>   אפרים פלשנר
GPG key = A28B F40C 3E55 1372 662D  14F7 41AA E7DC CA3D 8351
Confidentiality cannot be guaranteed on emails sent or received unencrypted

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 5/6] gnu: gcc: Force Aarch64 to use /lib.

[Efraim Flashner]

[-- Attachment #1: Type: text/plain, Size: 1529 bytes --]

On Tue, Feb 14, 2017 at 09:51:20PM +0200, Efraim Flashner wrote:
> On Tue, Feb 14, 2017 at 09:51:47AM +0100, Ludovic Courtès wrote:
> > Danny Milosavljevic <dannym@scratchpost.org> skribis:
> > 
> > >> +              ;; Force Aarch64 libdir to be /lib and not /lib64
> > >> +              (substitute* "gcc/config/aarch64/t-aarch64-linux"
> > >> +                (("lib64") "lib"))
> > >> +
> > >
> > > I'd amend the comment to say why.
> > 
> > I think we should just skip this patch.  There’s no reason one
> > architecture should be treated different from the others in that
> > respect.
> > 
> > WDYT, Efraim?
> > 
> > Ludo’.
> 
> I don't think it should cause a problem either way. As far as I can tell
> it doesn't make a difference to the software built further down the
> line.
> 

Looks like I spoke too soon. I tried to build gccgo which failed at the
linking stage, since it turned out libgcc_s was in gccgo/lib64 and not
gccgo/lib. I then tried gcc@4.9 and had a similar failure, the lib files
were split between lib and lib64. Other than this patch (with a when
file-exists), the other idea is to change libdir in gcc.scm:86 to be
lib64 on aarch64.

Unfortunately it looks like it'd cause a full rebuild on core-updates.
I'll test it overnight and see how it goes.

-- 
Efraim Flashner   <efraim@flashner.co.il>   אפרים פלשנר
GPG key = A28B F40C 3E55 1372 662D  14F7 41AA E7DC CA3D 8351
Confidentiality cannot be guaranteed on emails sent or received unencrypted

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 5/6] gnu: gcc: Force Aarch64 to use /lib.

[Efraim Flashner]

[-- Attachment #1: Type: text/plain, Size: 2131 bytes --]



On February 22, 2017 9:42:58 PM GMT+02:00, Efraim Flashner <efraim@flashner.co.il> wrote:
>On Tue, Feb 14, 2017 at 09:51:20PM +0200, Efraim Flashner wrote:
>> On Tue, Feb 14, 2017 at 09:51:47AM +0100, Ludovic Courtès wrote:
>> > Danny Milosavljevic <dannym@scratchpost.org> skribis:
>> > 
>> > >> +              ;; Force Aarch64 libdir to be /lib and not /lib64
>> > >> +              (substitute* "gcc/config/aarch64/t-aarch64-linux"
>> > >> +                (("lib64") "lib"))
>> > >> +
>> > >
>> > > I'd amend the comment to say why.
>> > 
>> > I think we should just skip this patch.  There’s no reason one
>> > architecture should be treated different from the others in that
>> > respect.
>> > 
>> > WDYT, Efraim?
>> > 
>> > Ludo’.
>> 
>> I don't think it should cause a problem either way. As far as I can
>tell
>> it doesn't make a difference to the software built further down the
>> line.
>> 
>
>Looks like I spoke too soon. I tried to build gccgo which failed at the
>linking stage, since it turned out libgcc_s was in gccgo/lib64 and not
>gccgo/lib. I then tried gcc@4.9 and had a similar failure, the lib
>files
>were split between lib and lib64. Other than this patch (with a when
>file-exists), the other idea is to change libdir in gcc.scm:86 to be
>lib64 on aarch64.
>
>Unfortunately it looks like it'd cause a full rebuild on core-updates.
>I'll test it overnight and see how it goes.
>
>-- 
>Efraim Flashner   <efraim@flashner.co.il>   אפרים פלשנר
>GPG key = A28B F40C 3E55 1372 662D  14F7 41AA E7DC CA3D 8351
>Confidentiality cannot be guaranteed on emails sent or received
>unencrypted

As is, all of our GCC versions FTBFS on aarch64, except the versions used during bootstrapping. This includes gccgo, but I haven't checked the other 'special GCCs' to see if also affects them.

With the above patch I was able to build GCC@4.9 and gccgo, and gccgo@5 failed as expected.

Unfortunately pushing this patch would result in a full rebuild on core-updates. Suggestions?

-- 
Sent from my Android device with K-9 Mail. Please excuse my brevity.

[-- Attachment #2: Type: text/html, Size: 2802 bytes --]

Re: [PATCH 5/6] gnu: gcc: Force Aarch64 to use /lib.

[Ludovic Courtès]

Efraim Flashner <efraim@flashner.co.il> skribis:

> On February 22, 2017 9:42:58 PM GMT+02:00, Efraim Flashner <efraim@flashner.co.il> wrote:
>>On Tue, Feb 14, 2017 at 09:51:20PM +0200, Efraim Flashner wrote:
>>> On Tue, Feb 14, 2017 at 09:51:47AM +0100, Ludovic Courtès wrote:
>>> > Danny Milosavljevic <dannym@scratchpost.org> skribis:
>>> > 
>>> > >> +              ;; Force Aarch64 libdir to be /lib and not /lib64
>>> > >> +              (substitute* "gcc/config/aarch64/t-aarch64-linux"
>>> > >> +                (("lib64") "lib"))
>>> > >> +
>>> > >
>>> > > I'd amend the comment to say why.
>>> > 
>>> > I think we should just skip this patch.  There’s no reason one
>>> > architecture should be treated different from the others in that
>>> > respect.
>>> > 
>>> > WDYT, Efraim?
>>> > 
>>> > Ludo’.
>>> 
>>> I don't think it should cause a problem either way. As far as I can
>>tell
>>> it doesn't make a difference to the software built further down the
>>> line.
>>> 
>>
>>Looks like I spoke too soon. I tried to build gccgo which failed at the
>>linking stage, since it turned out libgcc_s was in gccgo/lib64 and not
>>gccgo/lib. I then tried gcc@4.9 and had a similar failure, the lib
>>files
>>were split between lib and lib64. Other than this patch (with a when
>>file-exists), the other idea is to change libdir in gcc.scm:86 to be
>>lib64 on aarch64.
>>
>>Unfortunately it looks like it'd cause a full rebuild on core-updates.
>>I'll test it overnight and see how it goes.
>>
>>-- 
>>Efraim Flashner   <efraim@flashner.co.il>   אפרים פלשנר
>>GPG key = A28B F40C 3E55 1372 662D  14F7 41AA E7DC CA3D 8351
>>Confidentiality cannot be guaranteed on emails sent or received
>>unencrypted
>
> As is, all of our GCC versions FTBFS on aarch64, except the versions used during bootstrapping. This includes gccgo, but I haven't checked the other 'special GCCs' to see if also affects them.
>
> With the above patch I was able to build GCC@4.9 and gccgo, and gccgo@5 failed as expected.
>
> Unfortunately pushing this patch would result in a full rebuild on core-updates. Suggestions?

Given that ‘core-updates’ is still in the stage where we haven’t build
everything, you could push this ‘substitute*’ statement now IMO.

It’s pretty bad that software insists on using /lib64 down the road,
though.

Thanks,
Ludo’.

Re: [PATCH 3/6] daemon: On aarch64, use increments of 16 on the stack.

[Mark H Weaver]

Reviving a very old thread...

ludo@gnu.org (Ludovic Courtès) writes:

> Efraim Flashner <efraim@flashner.co.il> skribis:
>
>> man2 clone: EINVAL: ... on aarch64, child_stack must be a multiple of 16.
>>
>> * nix/libstore/build.cc (DerivationGoal::startBuilder): When on aarch64,
>> when calling clone(), increment the stack by 16.
>> ---
>>  nix/libstore/build.cc | 7 ++++++-
>>  1 file changed, 6 insertions(+), 1 deletion(-)
>>
>> diff --git a/nix/libstore/build.cc b/nix/libstore/build.cc
>> index cebc404d1..362b2d91d 100644
>> --- a/nix/libstore/build.cc
>> +++ b/nix/libstore/build.cc
>> @@ -2008,7 +2008,12 @@ void DerivationGoal::startBuilder()
>>  	char stack[32 * 1024];
>>  	int flags = CLONE_NEWPID | CLONE_NEWNS | CLONE_NEWIPC | CLONE_NEWUTS | SIGCHLD;
>>  	if (!fixedOutput) flags |= CLONE_NEWNET;
>> -	pid = clone(childEntry, stack + sizeof(stack) - 8, flags, this);
>> +// if statements are hard, fix this
>> +//#if __AARCH64__
>> +	pid = clone(childEntry, stack + sizeof(stack) - 16, flags, this);
>> +//#else
>> +//	pid = clone(childEntry, stack + sizeof(stack) - 8, flags, this);
>> +//#endif
>
> I think we can make it unconditional.  Could you test whether the
> attached patch works for aarch64?
>
> Thanks!
>
> Ludo’.
>
> diff --git a/nix/libstore/build.cc b/nix/libstore/build.cc
> index cebc404d1..9b7bb5391 100644
> --- a/nix/libstore/build.cc
> +++ b/nix/libstore/build.cc
> @@ -26,6 +26,7 @@
>  #include <errno.h>
>  #include <stdio.h>
>  #include <cstring>
> +#include <stdint.h>
>  
>  #include <pwd.h>
>  #include <grp.h>
> @@ -2008,7 +2009,11 @@ void DerivationGoal::startBuilder()
>  	char stack[32 * 1024];
>  	int flags = CLONE_NEWPID | CLONE_NEWNS | CLONE_NEWIPC | CLONE_NEWUTS | SIGCHLD;
>  	if (!fixedOutput) flags |= CLONE_NEWNET;
> -	pid = clone(childEntry, stack + sizeof(stack) - 8, flags, this);
> +
> +	/* Ensure proper alignment on the stack.  On aarch64, it has to be 16
> +	   bytes.  */
> +	pid = clone(childEntry, (char *)(((uintptr_t)stack + 16) & ~0xf),
> +		    flags, this);
>  	if (pid == -1)
>  	    throw SysError("cloning builder process");
>      } else

This patch, applied in February, contains a serious error.  The stack
address passed to 'clone' is supposed to be near the end of the memory
block allocated for the stack, and that's how it was before this patch
was applied.  Since this patch was applied, it now passes an address
very close to the *start* of the memory block.

This broke the daemon on mips64el in a subtle way that was rather
difficult to debug.  After about six months of being too busy with other
things to investigate properly, I finally tracked it down to this
change.

I reverted this commit.  Let's try again to find a proper fix for this
issue on aarch64.

     Thanks,
       Mark

Re: [PATCH 3/6] daemon: On aarch64, use increments of 16 on the stack.

[Efraim Flashner]

[-- Attachment #1: Type: text/plain, Size: 3511 bytes --]

On Sat, Aug 05, 2017 at 02:21:55AM -0400, Mark H Weaver wrote:
> Reviving a very old thread...
> 
> ludo@gnu.org (Ludovic Courtès) writes:
> 
> > Efraim Flashner <efraim@flashner.co.il> skribis:
> >
> >> man2 clone: EINVAL: ... on aarch64, child_stack must be a multiple of 16.
> >>
> >> * nix/libstore/build.cc (DerivationGoal::startBuilder): When on aarch64,
> >> when calling clone(), increment the stack by 16.
> >> ---
> >>  nix/libstore/build.cc | 7 ++++++-
> >>  1 file changed, 6 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/nix/libstore/build.cc b/nix/libstore/build.cc
> >> index cebc404d1..362b2d91d 100644
> >> --- a/nix/libstore/build.cc
> >> +++ b/nix/libstore/build.cc
> >> @@ -2008,7 +2008,12 @@ void DerivationGoal::startBuilder()
> >>  	char stack[32 * 1024];
> >>  	int flags = CLONE_NEWPID | CLONE_NEWNS | CLONE_NEWIPC | CLONE_NEWUTS | SIGCHLD;
> >>  	if (!fixedOutput) flags |= CLONE_NEWNET;
> >> -	pid = clone(childEntry, stack + sizeof(stack) - 8, flags, this);
> >> +// if statements are hard, fix this
> >> +//#if __AARCH64__
> >> +	pid = clone(childEntry, stack + sizeof(stack) - 16, flags, this);
> >> +//#else
> >> +//	pid = clone(childEntry, stack + sizeof(stack) - 8, flags, this);
> >> +//#endif
> >
> > I think we can make it unconditional.  Could you test whether the
> > attached patch works for aarch64?
> >
> > Thanks!
> >
> > Ludo’.
> >
> > diff --git a/nix/libstore/build.cc b/nix/libstore/build.cc
> > index cebc404d1..9b7bb5391 100644
> > --- a/nix/libstore/build.cc
> > +++ b/nix/libstore/build.cc
> > @@ -26,6 +26,7 @@
> >  #include <errno.h>
> >  #include <stdio.h>
> >  #include <cstring>
> > +#include <stdint.h>
> >  
> >  #include <pwd.h>
> >  #include <grp.h>
> > @@ -2008,7 +2009,11 @@ void DerivationGoal::startBuilder()
> >  	char stack[32 * 1024];
> >  	int flags = CLONE_NEWPID | CLONE_NEWNS | CLONE_NEWIPC | CLONE_NEWUTS | SIGCHLD;
> >  	if (!fixedOutput) flags |= CLONE_NEWNET;
> > -	pid = clone(childEntry, stack + sizeof(stack) - 8, flags, this);
> > +
> > +	/* Ensure proper alignment on the stack.  On aarch64, it has to be 16
> > +	   bytes.  */
> > +	pid = clone(childEntry, (char *)(((uintptr_t)stack + 16) & ~0xf),
> > +		    flags, this);
> >  	if (pid == -1)
> >  	    throw SysError("cloning builder process");
> >      } else
> 
> This patch, applied in February, contains a serious error.  The stack
> address passed to 'clone' is supposed to be near the end of the memory
> block allocated for the stack, and that's how it was before this patch
> was applied.  Since this patch was applied, it now passes an address
> very close to the *start* of the memory block.
> 
> This broke the daemon on mips64el in a subtle way that was rather
> difficult to debug.  After about six months of being too busy with other
> things to investigate properly, I finally tracked it down to this
> change.
> 
> I reverted this commit.  Let's try again to find a proper fix for this
> issue on aarch64.
> 
>      Thanks,
>        Mark

How about doubling the size of the stack to [32 * 1024  * 2] and
changing the clone location to 'stack + sizeof(stack) - 16', does that
work for mips64el? With this revert the daemon is completely broken on
aarch64.

-- 
Efraim Flashner   <efraim@flashner.co.il>   אפרים פלשנר
GPG key = A28B F40C 3E55 1372 662D  14F7 41AA E7DC CA3D 8351
Confidentiality cannot be guaranteed on emails sent or received unencrypted

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 3/6] daemon: On aarch64, use increments of 16 on the stack.

[Ludovic Courtès]

Mark H Weaver <mhw@netris.org> skribis:

> Reviving a very old thread...
>
> ludo@gnu.org (Ludovic Courtès) writes:
>
>> Efraim Flashner <efraim@flashner.co.il> skribis:
>>
>>> man2 clone: EINVAL: ... on aarch64, child_stack must be a multiple of 16.
>>>
>>> * nix/libstore/build.cc (DerivationGoal::startBuilder): When on aarch64,
>>> when calling clone(), increment the stack by 16.
>>> ---
>>>  nix/libstore/build.cc | 7 ++++++-
>>>  1 file changed, 6 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/nix/libstore/build.cc b/nix/libstore/build.cc
>>> index cebc404d1..362b2d91d 100644
>>> --- a/nix/libstore/build.cc
>>> +++ b/nix/libstore/build.cc
>>> @@ -2008,7 +2008,12 @@ void DerivationGoal::startBuilder()
>>>  	char stack[32 * 1024];
>>>  	int flags = CLONE_NEWPID | CLONE_NEWNS | CLONE_NEWIPC | CLONE_NEWUTS | SIGCHLD;
>>>  	if (!fixedOutput) flags |= CLONE_NEWNET;
>>> -	pid = clone(childEntry, stack + sizeof(stack) - 8, flags, this);
>>> +// if statements are hard, fix this
>>> +//#if __AARCH64__
>>> +	pid = clone(childEntry, stack + sizeof(stack) - 16, flags, this);
>>> +//#else
>>> +//	pid = clone(childEntry, stack + sizeof(stack) - 8, flags, this);
>>> +//#endif
>>
>> I think we can make it unconditional.  Could you test whether the
>> attached patch works for aarch64?
>>
>> Thanks!
>>
>> Ludo’.
>>
>> diff --git a/nix/libstore/build.cc b/nix/libstore/build.cc
>> index cebc404d1..9b7bb5391 100644
>> --- a/nix/libstore/build.cc
>> +++ b/nix/libstore/build.cc
>> @@ -26,6 +26,7 @@
>>  #include <errno.h>
>>  #include <stdio.h>
>>  #include <cstring>
>> +#include <stdint.h>
>>  
>>  #include <pwd.h>
>>  #include <grp.h>
>> @@ -2008,7 +2009,11 @@ void DerivationGoal::startBuilder()
>>  	char stack[32 * 1024];
>>  	int flags = CLONE_NEWPID | CLONE_NEWNS | CLONE_NEWIPC | CLONE_NEWUTS | SIGCHLD;
>>  	if (!fixedOutput) flags |= CLONE_NEWNET;
>> -	pid = clone(childEntry, stack + sizeof(stack) - 8, flags, this);
>> +
>> +	/* Ensure proper alignment on the stack.  On aarch64, it has to be 16
>> +	   bytes.  */
>> +	pid = clone(childEntry, (char *)(((uintptr_t)stack + 16) & ~0xf),
>> +		    flags, this);
>>  	if (pid == -1)
>>  	    throw SysError("cloning builder process");
>>      } else
>
> This patch, applied in February, contains a serious error.  The stack
> address passed to 'clone' is supposed to be near the end of the memory
> block allocated for the stack, and that's how it was before this patch
> was applied.  Since this patch was applied, it now passes an address
> very close to the *start* of the memory block.

Arrgh, good catch; my bad!  I wonder why this did not cause more
problems on other architectures.

Ludo’.

Re: [PATCH 3/6] daemon: On aarch64, use increments of 16 on the stack.

[Mark H Weaver]

Hi Efraim,

Efraim Flashner <efraim@flashner.co.il> writes:

> On Sat, Aug 05, 2017 at 02:21:55AM -0400, Mark H Weaver wrote:
>> Reviving a very old thread...
>> 
>> ludo@gnu.org (Ludovic Courtès) writes:
>> 
>> > diff --git a/nix/libstore/build.cc b/nix/libstore/build.cc
>> > index cebc404d1..9b7bb5391 100644
>> > --- a/nix/libstore/build.cc
>> > +++ b/nix/libstore/build.cc
>> > @@ -26,6 +26,7 @@
>> >  #include <errno.h>
>> >  #include <stdio.h>
>> >  #include <cstring>
>> > +#include <stdint.h>
>> >  
>> >  #include <pwd.h>
>> >  #include <grp.h>
>> > @@ -2008,7 +2009,11 @@ void DerivationGoal::startBuilder()
>> >  	char stack[32 * 1024];
>> >  	int flags = CLONE_NEWPID | CLONE_NEWNS | CLONE_NEWIPC | CLONE_NEWUTS | SIGCHLD;
>> >  	if (!fixedOutput) flags |= CLONE_NEWNET;
>> > -	pid = clone(childEntry, stack + sizeof(stack) - 8, flags, this);
>> > +
>> > +	/* Ensure proper alignment on the stack.  On aarch64, it has to be 16
>> > +	   bytes.  */
>> > +	pid = clone(childEntry, (char *)(((uintptr_t)stack + 16) & ~0xf),
>> > +		    flags, this);
>> >  	if (pid == -1)
>> >  	    throw SysError("cloning builder process");
>> >      } else
>> 
>> This patch, applied in February, contains a serious error.  The stack
>> address passed to 'clone' is supposed to be near the end of the memory
>> block allocated for the stack, and that's how it was before this patch
>> was applied.  Since this patch was applied, it now passes an address
>> very close to the *start* of the memory block.
>> 
>> This broke the daemon on mips64el in a subtle way that was rather
>> difficult to debug.  After about six months of being too busy with other
>> things to investigate properly, I finally tracked it down to this
>> change.
>> 
>> I reverted this commit.  Let's try again to find a proper fix for this
>> issue on aarch64.
>> 
>>      Thanks,
>>        Mark
>
> How about doubling the size of the stack to [32 * 1024  * 2] and

Is there a need to double the size of the stack?  If we have no reason
to think so, I'd rather leave it alone.

> changing the clone location to 'stack + sizeof(stack) - 16', does that
> work for mips64el?

The problem with (stack + sizeof(stack) - 16) is that there's no
guarantee that 'stack' will be aligned on a 16-byte boundary.  It might
be that if we add another local variable somewhere else in this
function, or if the compiler changes, we'll need to change the 16 to a
different number to make it work.

Can you try the following patch on aarch64 and report back?

     Thanks,
       Mark

--8<---------------cut here---------------start------------->8---
diff --git a/nix/libstore/build.cc b/nix/libstore/build.cc
index 693fa70c8..c5cd4bdb2 100644
--- a/nix/libstore/build.cc
+++ b/nix/libstore/build.cc
@@ -26,6 +26,7 @@
 #include <errno.h>
 #include <stdio.h>
 #include <cstring>
+#include <stdint.h>
 
 #include <pwd.h>
 #include <grp.h>
@@ -2008,11 +2009,11 @@ void DerivationGoal::startBuilder()
 	char stack[32 * 1024];
 	int flags = CLONE_NEWPID | CLONE_NEWNS | CLONE_NEWIPC | CLONE_NEWUTS | SIGCHLD;
 	if (!fixedOutput) flags |= CLONE_NEWNET;
-#ifdef __aarch64__
-	    pid = clone(childEntry, stack + sizeof(stack) - 16, flags, this);
-#else
-	    pid = clone(childEntry, stack + sizeof(stack) - 8, flags, this);
-#endif
+	/* Ensure proper alignment on the stack.  On aarch64, it has to be 16
+	   bytes.  */
+	pid = clone(childEntry,
+		    (char *)(((uintptr_t)stack + sizeof(stack) - 8) & ~0xf),
+		    flags, this);
 	if (pid == -1)
 	    throw SysError("cloning builder process");
     } else
--8<---------------cut here---------------end--------------->8---

Re: [PATCH 3/6] daemon: On aarch64, use increments of 16 on the stack.

[Mark H Weaver]

I wrote:
> Can you try the following patch on aarch64 and report back?

Actually, the last patch was not quite right.  C/C++ makes it rather
difficult to avoid edge cases in arithmetic.  Can you try this one
instead?

    Thanks,
      Mark

--8<---------------cut here---------------start------------->8---
diff --git a/nix/libstore/build.cc b/nix/libstore/build.cc
index 693fa70c8..63540ddfc 100644
--- a/nix/libstore/build.cc
+++ b/nix/libstore/build.cc
@@ -26,6 +26,7 @@
 #include <errno.h>
 #include <stdio.h>
 #include <cstring>
+#include <stdint.h>
 
 #include <pwd.h>
 #include <grp.h>
@@ -2008,11 +2009,11 @@ void DerivationGoal::startBuilder()
 	char stack[32 * 1024];
 	int flags = CLONE_NEWPID | CLONE_NEWNS | CLONE_NEWIPC | CLONE_NEWUTS | SIGCHLD;
 	if (!fixedOutput) flags |= CLONE_NEWNET;
-#ifdef __aarch64__
-	    pid = clone(childEntry, stack + sizeof(stack) - 16, flags, this);
-#else
-	    pid = clone(childEntry, stack + sizeof(stack) - 8, flags, this);
-#endif
+	/* Ensure proper alignment on the stack.  On aarch64, it has to be 16
+	   bytes.  */
+	pid = clone(childEntry,
+		    (char *)(((uintptr_t)stack + sizeof(stack) - 8) & ~(uintptr_t)0xf),
+		    flags, this);
 	if (pid == -1)
 	    throw SysError("cloning builder process");
     } else
--8<---------------cut here---------------end--------------->8---

Re: [PATCH 3/6] daemon: On aarch64, use increments of 16 on the stack.

[Efraim Flashner]

[-- Attachment #1: Type: text/plain, Size: 1837 bytes --]

On Sat, Aug 05, 2017 at 05:41:50PM -0400, Mark H Weaver wrote:
> I wrote:
> > Can you try the following patch on aarch64 and report back?
> 
> Actually, the last patch was not quite right.  C/C++ makes it rather
> difficult to avoid edge cases in arithmetic.  Can you try this one
> instead?
> 
>     Thanks,
>       Mark
> 
> --8<---------------cut here---------------start------------->8---
> diff --git a/nix/libstore/build.cc b/nix/libstore/build.cc
> index 693fa70c8..63540ddfc 100644
> --- a/nix/libstore/build.cc
> +++ b/nix/libstore/build.cc
> @@ -26,6 +26,7 @@
>  #include <errno.h>
>  #include <stdio.h>
>  #include <cstring>
> +#include <stdint.h>
>  
>  #include <pwd.h>
>  #include <grp.h>
> @@ -2008,11 +2009,11 @@ void DerivationGoal::startBuilder()
>  	char stack[32 * 1024];
>  	int flags = CLONE_NEWPID | CLONE_NEWNS | CLONE_NEWIPC | CLONE_NEWUTS | SIGCHLD;
>  	if (!fixedOutput) flags |= CLONE_NEWNET;
> -#ifdef __aarch64__
> -	    pid = clone(childEntry, stack + sizeof(stack) - 16, flags, this);
> -#else
> -	    pid = clone(childEntry, stack + sizeof(stack) - 8, flags, this);
> -#endif
> +	/* Ensure proper alignment on the stack.  On aarch64, it has to be 16
> +	   bytes.  */
> +	pid = clone(childEntry,
> +		    (char *)(((uintptr_t)stack + sizeof(stack) - 8) & ~(uintptr_t)0xf),
> +		    flags, this);
>  	if (pid == -1)
>  	    throw SysError("cloning builder process");
>      } else
> --8<---------------cut here---------------end--------------->8---

The aarch64 machine that I tested this on built packages without any
problems. Looks good to me.

-- 
Efraim Flashner   <efraim@flashner.co.il>   אפרים פלשנר
GPG key = A28B F40C 3E55 1372 662D  14F7 41AA E7DC CA3D 8351
Confidentiality cannot be guaranteed on emails sent or received unencrypted

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 3/6] daemon: On aarch64, use increments of 16 on the stack.

[Mark H Weaver]

Efraim Flashner <efraim@flashner.co.il> writes:
> The aarch64 machine that I tested this on built packages without any
> problems. Looks good to me.

Pushed in commit a1aa5dabaa5d570710da7190a3c3dca5442b9daa.

    Thanks,
      Mark