From mboxrd@z Thu Jan 1 00:00:00 1970 From: Mark H Weaver Subject: Re: [PATCH 3/6] daemon: On aarch64, use increments of 16 on the stack. Date: Sat, 05 Aug 2017 17:32:03 -0400 Message-ID: <87r2wp4s6k.fsf@netris.org> References: <20170209184510.24200-1-efraim@flashner.co.il> <20170209184510.24200-4-efraim@flashner.co.il> <87r331xiot.fsf@gnu.org> <874ltm5ybg.fsf@netris.org> <20170805182401.GA2458@macbook42.flashner.co.il> Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable Return-path: Received: from eggs.gnu.org ([2001:4830:134:3::10]:44855) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1de6gM-0005fB-4Y for guix-devel@gnu.org; Sat, 05 Aug 2017 17:32:27 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1de6gI-000823-VN for guix-devel@gnu.org; Sat, 05 Aug 2017 17:32:26 -0400 In-Reply-To: <20170805182401.GA2458@macbook42.flashner.co.il> (Efraim Flashner's message of "Sat, 5 Aug 2017 21:24:02 +0300") 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: Efraim Flashner Cc: guix-devel@gnu.org Hi Efraim, Efraim Flashner writes: > On Sat, Aug 05, 2017 at 02:21:55AM -0400, Mark H Weaver wrote: >> Reviving a very old thread... >>=20 >> ludo@gnu.org (Ludovic Court=C3=A8s) writes: >>=20 >> > 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 >> > #include >> > #include >> > +#include >> >=20=20 >> > #include >> > #include >> > @@ -2008,7 +2009,11 @@ void DerivationGoal::startBuilder() >> > char stack[32 * 1024]; >> > int flags =3D CLONE_NEWPID | CLONE_NEWNS | CLONE_NEWIPC | CLONE_NEWU= TS | SIGCHLD; >> > if (!fixedOutput) flags |=3D CLONE_NEWNET; >> > - pid =3D clone(childEntry, stack + sizeof(stack) - 8, flags, this); >> > + >> > + /* Ensure proper alignment on the stack. On aarch64, it has to be 16 >> > + bytes. */ >> > + pid =3D clone(childEntry, (char *)(((uintptr_t)stack + 16) & ~0xf), >> > + flags, this); >> > if (pid =3D=3D -1) >> > throw SysError("cloning builder process"); >> > } else >>=20 >> 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. >>=20 >> 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. >>=20 >> I reverted this commit. Let's try again to find a proper fix for this >> issue on aarch64. >>=20 >> 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 #include #include +#include =20 #include #include @@ -2008,11 +2009,11 @@ void DerivationGoal::startBuilder() char stack[32 * 1024]; int flags =3D CLONE_NEWPID | CLONE_NEWNS | CLONE_NEWIPC | CLONE_NEWUTS | = SIGCHLD; if (!fixedOutput) flags |=3D CLONE_NEWNET; -#ifdef __aarch64__ - pid =3D clone(childEntry, stack + sizeof(stack) - 16, flags, this); -#else - pid =3D clone(childEntry, stack + sizeof(stack) - 8, flags, this); -#endif + /* Ensure proper alignment on the stack. On aarch64, it has to be 16 + bytes. */ + pid =3D clone(childEntry, + (char *)(((uintptr_t)stack + sizeof(stack) - 8) & ~0xf), + flags, this); if (pid =3D=3D -1) throw SysError("cloning builder process"); } else --8<---------------cut here---------------end--------------->8---