From mboxrd@z Thu Jan 1 00:00:00 1970 Path: news.gmane.org!.POSTED!not-for-mail From: Valentin Gatien-Baron Newsgroups: gmane.emacs.bugs Subject: bug#29066: 26.0.90; crash in gc involving buffer local symbols Date: Tue, 31 Oct 2017 15:58:45 -0400 Message-ID: References: <83a808tlqp.fsf@gnu.org> <838tfst27n.fsf@gnu.org> <87efpjzv2p.fsf@linux-m68k.org> <83375zt9ow.fsf@gnu.org> NNTP-Posting-Host: blaine.gmane.org Mime-Version: 1.0 Content-Type: multipart/alternative; boundary="94eb2c1a614acdba33055cdd2e20" X-Trace: blaine.gmane.org 1509479956 11023 195.159.176.226 (31 Oct 2017 19:59:16 GMT) X-Complaints-To: usenet@blaine.gmane.org NNTP-Posting-Date: Tue, 31 Oct 2017 19:59:16 +0000 (UTC) Cc: 29066@debbugs.gnu.org, Andreas Schwab , Mark Shinwell To: Eli Zaretskii Original-X-From: bug-gnu-emacs-bounces+geb-bug-gnu-emacs=m.gmane.org@gnu.org Tue Oct 31 20:59:10 2017 Return-path: Envelope-to: geb-bug-gnu-emacs@m.gmane.org Original-Received: from lists.gnu.org ([208.118.235.17]) by blaine.gmane.org with esmtp (Exim 4.84_2) (envelope-from ) id 1e9cgm-0001xF-Af for geb-bug-gnu-emacs@m.gmane.org; Tue, 31 Oct 2017 20:59:08 +0100 Original-Received: from localhost ([::1]:47160 helo=lists.gnu.org) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1e9cgr-0000k3-Vv for geb-bug-gnu-emacs@m.gmane.org; Tue, 31 Oct 2017 15:59:14 -0400 Original-Received: from eggs.gnu.org ([2001:4830:134:3::10]:47772) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1e9cgl-0000jv-Ob for bug-gnu-emacs@gnu.org; Tue, 31 Oct 2017 15:59:09 -0400 Original-Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1e9cgg-0000tD-Oo for bug-gnu-emacs@gnu.org; Tue, 31 Oct 2017 15:59:07 -0400 Original-Received: from debbugs.gnu.org ([208.118.235.43]:35906) by eggs.gnu.org with esmtps (TLS1.0:RSA_AES_128_CBC_SHA1:16) (Exim 4.71) (envelope-from ) id 1e9cgg-0000t3-Kr for bug-gnu-emacs@gnu.org; Tue, 31 Oct 2017 15:59:02 -0400 Original-Received: from Debian-debbugs by debbugs.gnu.org with local (Exim 4.84_2) (envelope-from ) id 1e9cgg-0005GL-AO for bug-gnu-emacs@gnu.org; Tue, 31 Oct 2017 15:59:02 -0400 X-Loop: help-debbugs@gnu.org Resent-From: Valentin Gatien-Baron Original-Sender: "Debbugs-submit" Resent-CC: bug-gnu-emacs@gnu.org Resent-Date: Tue, 31 Oct 2017 19:59:02 +0000 Resent-Message-ID: Resent-Sender: help-debbugs@gnu.org X-GNU-PR-Message: followup 29066 X-GNU-PR-Package: emacs X-GNU-PR-Keywords: Original-Received: via spool by 29066-submit@debbugs.gnu.org id=B29066.150947993520216 (code B ref 29066); Tue, 31 Oct 2017 19:59:02 +0000 Original-Received: (at 29066) by debbugs.gnu.org; 31 Oct 2017 19:58:55 +0000 Original-Received: from localhost ([127.0.0.1]:44587 helo=debbugs.gnu.org) by debbugs.gnu.org with esmtp (Exim 4.84_2) (envelope-from ) id 1e9cgZ-0005Fz-DB for submit@debbugs.gnu.org; Tue, 31 Oct 2017 15:58:55 -0400 Original-Received: from mxout1.mail.janestreet.com ([38.105.200.78]:57692) by debbugs.gnu.org with esmtp (Exim 4.84_2) (envelope-from ) id 1e9cgX-0005Fj-UF for 29066@debbugs.gnu.org; Tue, 31 Oct 2017 15:58:54 -0400 Original-Received: from [172.27.56.106] (helo=tot-qpr-mailcore2) by mxout1.mail.janestreet.com with esmtps (TLSv1.2:ECDHE-RSA-AES256-GCM-SHA384:256) (Exim 4.89) (envelope-from ) id 1e9cgR-00060k-Lc for 29066@debbugs.gnu.org; Tue, 31 Oct 2017 15:58:47 -0400 X-JS-Flow: external X-JS-Scanner-attachment: (ok) No attachments Original-Received: by tot-qpr-mailcore2 with ocaml/mailcore/mailcore 1.0+137 (04e1cd915edc) (envelope-from ) id BZ-NX3-CYJJoA-Uf; 2017-10-31 15:58:47.661107-04:00 Original-Received: from mail-lf0-f69.google.com ([209.85.215.69]) by mxgoog1.mail.janestreet.com with esmtps (TLSv1.2:ECDHE-RSA-AES128-GCM-SHA256:128) (Exim 4.89) (envelope-from ) id 1e9cgR-0003nI-GX for 29066@debbugs.gnu.org; Tue, 31 Oct 2017 15:58:47 -0400 Original-Received: by mail-lf0-f69.google.com with SMTP id s16so35632lfs.22 for <29066@debbugs.gnu.org>; Tue, 31 Oct 2017 12:58:47 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=janestreet.com; s=google; h=mime-version:in-reply-to:references:from:date:message-id:subject:to :cc; bh=XhTxqeEL2f1ZJC6ULXKHpbtlVVYWXxBqlI8lQUboze4=; b=LXnlFUMcZnJdraRFMcOUhYNLZZMUhz6F5H6umCS7nGwdBZNSwLdfqUfhEQ1j48QdCz Uv0ySOovW0O87YxOVMjll3p0sb1I6rfleshmO8LWHBDgA3b7Y/vyhT7PrbCNnnRLNrDm dQI0Z8c4lKJ+TH9Gc6gNR6FvJ6p2e3pZ+5voU= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:in-reply-to:references:from:date :message-id:subject:to:cc; bh=XhTxqeEL2f1ZJC6ULXKHpbtlVVYWXxBqlI8lQUboze4=; b=t0BXX9Ry6HFPHVmRq8oJxLvCvxzmWjj/+wQq+AZ2eEvAe/XZ8lRK5cEgt/Txhcv3wI 0EiVu0/NXjPO66tsHXQCukgZ71TlbyBEpd/1IVEo06JjJ+VOnNRHjRlMamwpxqcua0pS 30NwBDqgWmeMZk88c7rwcijTBcPr4UM8mYPwm/231eEXMt/tXlGs6sTy3/mLJceE8OBN cOma04AJZx21W7CaeYj59TG/Tyhxl8b6Q8l9yoZAmKKUa7ADPZ5Xi41dTgeBhT2PmSXr PcmuDAuWvRHckbetz9ctg08FCUkLyhb6uqPbGc2aFdCI+g6iYfJKE9wV/Ye9CRA7kznm EqoQ== X-Gm-Message-State: AMCzsaXfun5/CyV/3ubXJgXVEMckvKQAWrQoNW3Dl7HwXrqqNOSrwkGP IFk/SGaqWiwG0/lBrjSM1XJs0qK0oBtwty5qBEhVt0uQuR9jtubNc6qKu4ZSsFC6AYTYtmS3ADb brqcBc6hdN01X0lHz+ldWoaUu+NJW+A== X-Received: by 10.46.2.78 with SMTP id 75mr1328933ljc.75.1509479926505; Tue, 31 Oct 2017 12:58:46 -0700 (PDT) X-Google-Smtp-Source: ABhQp+QiSw+ZMxLS/JIVtnSBSS2qY503JDy8UfvutczhbTahuGXRWuHtvcdvRIw7TH0PT0lzeZfAicX30kxntuwTjlI= X-Received: by 10.46.2.78 with SMTP id 75mr1328926ljc.75.1509479926306; Tue, 31 Oct 2017 12:58:46 -0700 (PDT) Original-Received: by 10.25.234.11 with HTTP; Tue, 31 Oct 2017 12:58:45 -0700 (PDT) In-Reply-To: <83375zt9ow.fsf@gnu.org> X-JS-Exim-Data-Received: 2017-10-31 15:58:47-0400 X-JS-Processed-by: mailcore X-BeenThere: debbugs-submit@debbugs.gnu.org X-Mailman-Version: 2.1.18 Precedence: list X-detected-operating-system: by eggs.gnu.org: GNU/Linux 2.2.x-3.x [generic] X-Received-From: 208.118.235.43 X-BeenThere: bug-gnu-emacs@gnu.org List-Id: "Bug reports for GNU Emacs, the Swiss army knife of text editors" List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: bug-gnu-emacs-bounces+geb-bug-gnu-emacs=m.gmane.org@gnu.org Original-Sender: "bug-gnu-emacs" Xref: news.gmane.org gmane.emacs.bugs:139275 Archived-At: --94eb2c1a614acdba33055cdd2e20 Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable On Tue, Oct 31, 2017 at 3:10 PM, Eli Zaretskii wrote: > > From: Valentin Gatien-Baron > > Date: Tue, 31 Oct 2017 10:52:44 -0400 > > Cc: Eli Zaretskii , > > 29066@debbugs.gnu.org, > > Mark Shinwell > > > > > That was my first attempt, but various macros like SYMBOL_BLV and > > > SET_SYMBOL_BLV insist on val.blv being non-NULL. > > > > SET_SYMBOL_BLV doesn't. And calling SYMBOL_BLV with a freed symbol is= a > > bug anyway. > > > > =E2=80=8BSET_SYMBOL_BLV insists that the new value is not NULL, even if= it > asserts nothing about the current value. > > > > We do call SYMBOL_BLV after freeing, when we re-sweep the symbol, which > is fine because free does > > nothing when given NULL, but triggers the assertion=E2=80=8B. > > > > I would do this, to avoid the assertion failure: > > > > diff --git a/src/alloc.c b/src/alloc.c > > index da0c3ad4b3..72550e812b 100644 > > --- a/src/alloc.c > > +++ b/src/alloc.c > > @@ -7030,8 +7030,10 @@ sweep_symbols (void) > > { > > if (!sym->s.gcmarkbit) > > { > > - > =E2=80=8B=E2=80=8B > if (sym->s.redirect =3D=3D SYMBOL_LOCALIZED) > > + if (sym->s.redirect =3D=3D SYMBOL_LOCALIZED && > sym->s.val.blv) { > > xfree (SYMBOL_BLV ( > =E2=80=8B=E2=80=8B > &sym->s)); > > + sym->s.val.blv =3D NULL; > > + } > > sym->s.next =3D symbol_free_list; > > symbol_free_list =3D &sym->s; > > symbol_free_list->function =3D Vdead; > > Thanks, but it makes little sense to me to work around our own > assertions this way. Why do we have these macros if we sometimes > don't use them? And why do we have the assertions if they sometimes > get in the way? > > Or changing the redirect type: > > > > diff --git a/src/alloc.c b/src/alloc.c > > index da0c3ad4b3..6966d96c6d 100644 > > --- a/src/alloc.c > > +++ b/src/alloc.c > > @@ -7030,8 +7030,11 @@ sweep_symbols (void) > > { > > if (!sym->s.gcmarkbit) > > { > > - if (sym->s.redirect =3D=3D SYMBOL_LOCALIZED) > > + if (sym->s.redirect =3D=3D SYMBOL_LOCALIZED) { > > xfree (SYMBOL_BLV (&sym->s)); > > + sym->s.redirect =3D SYMBOL_PLAINVAL; > > + SET_SYMBOL_VAL (&sym->s, Qunbound); > > + } > > We could do several things, but I still didn't hear even a single > reason why my suggestion isn't OK. IMO, it's simpler, and the test > for Vdead is already in at least one other place. > > So I went ahead and pushed my change. > > Is there anything else we need to do before closing this bug? > =E2=80=8BThe slight downside of your fix is that there are dangling pointer= s that point to valid-looking things in the debugger. It's runtime behavior=E2=80= =8B looks fine otherwise. =E2=80=8BBut I am not going to insist. There's nothing else to do, you can = close the bug. Thanks! > > Thanks. > --94eb2c1a614acdba33055cdd2e20 Content-Type: text/html; charset="UTF-8" Content-Transfer-Encoding: quoted-printable


On Tue, Oct 31, 2017 at 3:10 PM, Eli Zaretskii <eliz@gnu.org&g= t; wrote:
> = From: Valentin Gatien-Baron <vgatien-baron@janestreet.com>
> Date: Tue, 31 Oct 2017 10:52:44 -0400
> Cc: Eli Zaretskii <eliz@gnu.org= >,
>=C2=A0 =C2=A0 =C2=A0 =C2=A0290= 66@debbugs.gnu.org,
>=C2=A0 =C2=A0 =C2=A0 =C2=A0Mark Shinwell <mshinwell@janestreet.com>
>
>=C2=A0 > That was my first attempt, but various macros like SYMBOL_B= LV and
>=C2=A0 > SET_SYMBOL_BLV insist on val.blv being non-NULL.
>
>=C2=A0 SET_SYMBOL_BLV doesn't.=C2=A0 And calling SYMBOL_BLV with a = freed symbol is a
>=C2=A0 bug anyway.
>
> =E2=80=8BSET_SYMBOL_BLV insists that the new value is not NULL, even i= f it asserts nothing about the current value.
>
> We do call SYMBOL_BLV after freeing, when we re-sweep the symbol, whic= h is fine because free does
> nothing when given NULL, but triggers the assertion=E2=80=8B.
>
> I would do this, to avoid the assertion failure:
>
> diff --git a/src/alloc.c b/src/alloc.c
> index da0c3ad4b3..72550e812b 100644
> --- a/src/alloc.c
> +++ b/src/alloc.c
> @@ -7030,8 +7030,10 @@ sweep_symbols (void)
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 {
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 if (!sym->s.gcmarkbit)
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 {
> -=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0
= =E2=80=8B=E2=80=8B
if (sym->s.redirect =3D=3D SYMBOL_LOCALIZED)
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 if (sym->s.redire= ct =3D=3D SYMBOL_LOCALIZED && sym->s.val.blv) {
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 xfree (S= YMBOL_BLV (
=E2=80=8B=E2=80=8B
&sym->s));
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 sym->s.val= .blv =3D NULL;
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 }
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 sym->s.next = =3D symbol_free_list;
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 symbol_free_lis= t =3D &sym->s;
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 symbol_free_lis= t->function =3D Vdead;

Thanks, but it makes little sense to me to work around our own
assertions this way.=C2=A0 Why do we have these macros if we sometimes
don't use them?=C2=A0 And why do we have the assertions if they sometim= es
get in the way?=C2=A0

> Or changing the redirect type:
>
> diff --git a/src/alloc.c b/src/alloc.c
> index da0c3ad4b3..6966d96c6d 100644
> --- a/src/alloc.c
> +++ b/src/alloc.c
> @@ -7030,8 +7030,11 @@ sweep_symbols (void)
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 {
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 if (!sym->s.gcmarkbit)
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 {
> -=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 if (sym->s.redire= ct =3D=3D SYMBOL_LOCALIZED)
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 if (sym->s.redire= ct =3D=3D SYMBOL_LOCALIZED) {
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 xfree (S= YMBOL_BLV (&sym->s));
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 sym->s.red= irect =3D SYMBOL_PLAINVAL;
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 SET_SYMBOL_VA= L (&sym->s, Qunbound);
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 }

We could do several things, but I still didn't hear even a singl= e
reason why my suggestion isn't OK.=C2=A0 IMO, it's simpler, and the= test
for Vdead is already in at least one other place.

So I went ahead and pushed my change.

Is there anything else we need to do before closing this bug?

=E2=80=8BThe slight downside of your fix is that = there are dangling pointers that point to valid-looking things in the debug= ger. It's runtime behavior=E2=80=8B looks fine otherwise.
=E2=80=8BBut I am not going to insist. There's nothing else to do= , you can close the bug. Thanks!

=C2=A0<= /div>

Thanks.

--94eb2c1a614acdba33055cdd2e20--