From mboxrd@z Thu Jan 1 00:00:00 1970 From: Mark H Weaver Subject: bug#28211: Stack marking issue in multi-threaded code Date: Fri, 29 Jun 2018 19:18:07 -0400 Message-ID: <87efgpw5ao.fsf@netris.org> References: <877exuj58y.fsf@gnu.org> <87d0yo1tie.fsf@gnu.org> <87fu3124nt.fsf@gnu.org> <87d0y5k6sl.fsf@netris.org> <871sel6vnq.fsf@igalia.com> <87fu30dmx3.fsf@netris.org> <87tvrg3q1d.fsf@igalia.com> <87a7rdvdm9.fsf_-_@gnu.org> <87in61y1m0.fsf@netris.org> <87muvdthp4.fsf@gnu.org> 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]:53798) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1fZ2gP-0002qa-M0 for bug-guix@gnu.org; Fri, 29 Jun 2018 19:20:06 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1fZ2gM-0006Ta-EG for bug-guix@gnu.org; Fri, 29 Jun 2018 19:20:05 -0400 Received: from debbugs.gnu.org ([208.118.235.43]:60526) by eggs.gnu.org with esmtps (TLS1.0:RSA_AES_128_CBC_SHA1:16) (Exim 4.71) (envelope-from ) id 1fZ2gM-0006T3-Aq for bug-guix@gnu.org; Fri, 29 Jun 2018 19:20:02 -0400 Sender: "Debbugs-submit" Resent-Message-ID: In-Reply-To: <87muvdthp4.fsf@gnu.org> ("Ludovic \=\?utf-8\?Q\?Court\=C3\=A8s\=22'\?\= \=\?utf-8\?Q\?s\?\= message of "Fri, 29 Jun 2018 23:18:31 +0200") List-Id: Bug reports for GNU Guix List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: bug-guix-bounces+gcggb-bug-guix=m.gmane.org@gnu.org Sender: "bug-Guix" To: Ludovic =?UTF-8?Q?Court=C3=A8s?= Cc: Andy Wingo , 28211@debbugs.gnu.org Hi Ludovic, ludo@gnu.org (Ludovic Court=C3=A8s) writes: > Mark H Weaver skribis: > >> ludo@gnu.org (Ludovic Court=C3=A8s) writes: > > [...] > >>> I=E2=80=99m thinking we could perhaps add a compiler barrier before =E2= =80=98vp->fp =3D new_fp=E2=80=99 >>> statements, but in practice it=E2=80=99s not necessary here (x86_64, gc= c 7). >>> >>> Thoughts? > > I just pushed the patch as 23af45e248e8e2bec99c712842bf24d6661abbe2. > >> Indeed, we need something to ensure that the compiler won't reorder >> these writes. My recommendation would be to use the 'volatile' >> qualifier in the declarations of both the 'fp' field of 'struct scm_vm' >> and the stack element modified by SCM_FRAME_SET_DYNAMIC_LINK. >> >> Maybe something like this: >> >> diff --git a/libguile/frames.h b/libguile/frames.h >> index ef2db3df5..8554f886b 100644 >> --- a/libguile/frames.h >> +++ b/libguile/frames.h >> @@ -89,6 +89,7 @@ >> union scm_vm_stack_element >> { >> scm_t_uintptr as_uint; >> + volatile scm_t_uintptr as_volatile_uint; > > [...] > >> +#define SCM_FRAME_SET_DYNAMIC_LINK(fp, dl) ((fp)[1].as_volatile_uint = =3D ((dl) - (fp))) > > I=E2=80=99m not sure this is exactly what we need. > > What I had in mind, to make sure the vp->fp assignment really happens > after the SCM_FRAME_SET_DYNAMIC_LINK, was to do something like this: > > SCM_FRAME_SET_RETURN_ADDRESS (new_fp, =E2=80=A6); > SCM_FRAME_SET_DYNAMIC_LINK (new_fp, =E2=80=A6); > > asm volatile ("" : : : "memory"); > > vp->fp =3D new_fp; > > I think that more accurately reflects what we want, WDYT? > > It=E2=80=99s GCC-specific though (I don=E2=80=99t think Clang implements = =E2=80=98asm=E2=80=99 in a > compatible way, does it?) and I suppose we=E2=80=99d have to ignore the n= on-GCC > case. Right, the problem is that the "asm volatile" solution is not portable. To my mind, it's fine to optionally use GCC extensions for improved performance, but I think we should ensure that Guile works reliably when compiled with any conforming C compiler. What problem do you see with my proposed portable solution? If I understand C99 section 5.1.2.3 paragraph 2 correctly, compilers are not permitted to reorder accesses to volatile objects as long as there is a sequence point between them. I should say that I'm not confident that _either_ of these proposed solutions will adequately address all of the possible problems that could occur when GC is performed on VM threads stopped at arbitrary points in their execution. Mark