From mboxrd@z Thu Jan 1 00:00:00 1970 From: Mark H Weaver Subject: Re: 01/01: gnu: glibc/linux: Fix runtime crashes on i686 systems. Date: Sun, 30 Apr 2017 04:16:42 -0400 Message-ID: <87bmrewbrp.fsf@netris.org> References: <20170429213253.6080.61219@vcs0.savannah.gnu.org> <20170429213255.095472109D@vcs0.savannah.gnu.org> Mime-Version: 1.0 Content-Type: text/plain Return-path: Received: from eggs.gnu.org ([2001:4830:134:3::10]:59205) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1d4k2Z-0006Qf-99 for guix-devel@gnu.org; Sun, 30 Apr 2017 04:17:12 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1d4k2V-0002W6-9W for guix-devel@gnu.org; Sun, 30 Apr 2017 04:17:11 -0400 Received: from world.peace.net ([50.252.239.5]:40327) by eggs.gnu.org with esmtps (TLS1.0:RSA_AES_128_CBC_SHA1:16) (Exim 4.71) (envelope-from ) id 1d4k2V-0002UG-5h for guix-devel@gnu.org; Sun, 30 Apr 2017 04:17:07 -0400 In-Reply-To: <20170429213255.095472109D@vcs0.savannah.gnu.org> (Ricardo Wurmus's message of "Sat, 29 Apr 2017 17:32:54 -0400 (EDT)") List-Id: "Development of GNU Guix and the GNU System distribution." List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: guix-devel-bounces+gcggd-guix-devel=m.gmane.org@gnu.org Sender: "Guix-devel" To: Ricardo Wurmus Cc: guix-devel@gnu.org Hi Ricardo, rekado@elephly.net (Ricardo Wurmus) writes: > rekado pushed a commit to branch master > in repository guix. > > commit b2fd8f63679aa4f244c36fdca62f23c00b8eded9 > Author: Ricardo Wurmus > Date: Wed Apr 26 13:03:48 2017 +0200 > > gnu: glibc/linux: Fix runtime crashes on i686 systems. > > * gnu/packages/patches/glibc-memchr-overflow-i686.patch: New file. > * gnu/local.mk (dist_patch_DATA): Add it. > * gnu/packages/commencement.scm (glibc-final-with-bootstrap-bash)[native-inputs]: > Add the patch conditionally for i686 systems. > * gnu/packages/base.scm (glibc/linux)[native-inputs]: Add the patch > conditionally for i686 systems. > [arguments]: Apply the patch conditionally on i686 systems. Thanks very much for this. I noticed a minor issue with this patch: > diff --git a/gnu/packages/base.scm b/gnu/packages/base.scm > index 9fcca45..6dc9e97 100644 > --- a/gnu/packages/base.scm > +++ b/gnu/packages/base.scm > @@ -666,6 +666,16 @@ store.") > ;; 4.7.1. > ((" -lgcc_s") "")) > > + ;; Apply patch only on i686. > + ;; TODO: Move the patch to 'patches' in the next update cycle. > + ,@(if (string-prefix? "i686" (or (%current-target-system) > + (%current-system))) > + `(zero? (system* "patch" "-p1" "--force" > + "--input" > + (assoc-ref native-inputs > + "glibc-memchr-overflow-i686.patch"))) The `(zero? ...) subexpression is incorrect for two reasons: The first issue is that you need another layer of parentheses, i.e. `((zero? ...)), to generate the code as you presumably intended. As it is, the generated code looks like this: --8<---------------cut here---------------start------------->8--- (substitute* "Makeconfig" ;; According to ;; , ;; linking against libgcc_s is not needed with GCC ;; 4.7.1. ((" -lgcc_s") "")) zero? (system* "patch" "-p1" "--force" "--input" (assoc-ref native-inputs "glibc-memchr-overflow-i686.patch")) --8<---------------cut here---------------end--------------->8--- Since the isolated variable reference 'zero?' has no side effects and the result is discarded, it has no effect and is presumably optimized out. 'system*' returns an integer. All integers are interpreted as "true" in Scheme, including zero. The other problem is that (zero? (system* ...)) does not have the desired effect, because it is not the final expression in the outer body, and therefore its result is discarded and errors are not flagged. What's needed here is to explicitly raise an exception if 'system*' returns a non-zero result, e.g. something like this (untested): > + ;; Apply patch only on i686. > + ;; TODO: Move the patch to 'patches' in the next update cycle. > + ,@(if (string-prefix? "i686" (or (%current-target-system) > + (%current-system))) > + `((unless (zero? (system* "patch" "-p1" "--force" > + "--input" > + (assoc-ref native-inputs > + "glibc-memchr-overflow-i686.patch"))) > + (error "patch failed for glibc-memchr-overflow-i686.patch"))) This reminds me of something I've felt for a while: that we should stop using 'system*' in most places, and instead use a variant of 'system*' that automatically raises an exception in case of a non-zero result code, but that's a subject for another thread. Thoughts? Mark