From mboxrd@z Thu Jan 1 00:00:00 1970 Path: news.gmane.org!.POSTED!not-for-mail From: Philipp Stephani Newsgroups: gmane.emacs.devel Subject: Re: [PATCH] Support threads in modules Date: Mon, 12 Jun 2017 15:04:23 +0000 Message-ID: References: <20170422152444.48735-1-phst@google.com> <83zif8p8d3.fsf@gnu.org> <83tw5gp6hy.fsf@gnu.org> <83r30kp5d1.fsf@gnu.org> <83inlrpwds.fsf@gnu.org> <8337b86mrl.fsf@gnu.org> <83mv9e6026.fsf@gnu.org> NNTP-Posting-Host: blaine.gmane.org Mime-Version: 1.0 Content-Type: multipart/alternative; boundary="001a113e1e7e0783540551c4a317" X-Trace: blaine.gmane.org 1497279947 25306 195.159.176.226 (12 Jun 2017 15:05:47 GMT) X-Complaints-To: usenet@blaine.gmane.org NNTP-Posting-Date: Mon, 12 Jun 2017 15:05:47 +0000 (UTC) Cc: phst@google.com, emacs-devel@gnu.org To: Eli Zaretskii Original-X-From: emacs-devel-bounces+ged-emacs-devel=m.gmane.org@gnu.org Mon Jun 12 17:05:40 2017 Return-path: Envelope-to: ged-emacs-devel@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 1dKQuR-00068t-DF for ged-emacs-devel@m.gmane.org; Mon, 12 Jun 2017 17:05:39 +0200 Original-Received: from localhost ([::1]:38581 helo=lists.gnu.org) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1dKQuW-00005d-Kx for ged-emacs-devel@m.gmane.org; Mon, 12 Jun 2017 11:05:44 -0400 Original-Received: from eggs.gnu.org ([2001:4830:134:3::10]:60795) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1dKQtX-0008Vs-Pz for emacs-devel@gnu.org; Mon, 12 Jun 2017 11:04:45 -0400 Original-Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1dKQtR-0004nP-I2 for emacs-devel@gnu.org; Mon, 12 Jun 2017 11:04:43 -0400 Original-Received: from mail-ot0-x22c.google.com ([2607:f8b0:4003:c0f::22c]:36471) by eggs.gnu.org with esmtps (TLS1.0:RSA_AES_128_CBC_SHA1:16) (Exim 4.71) (envelope-from ) id 1dKQtP-0004ml-BY; Mon, 12 Jun 2017 11:04:35 -0400 Original-Received: by mail-ot0-x22c.google.com with SMTP id i31so67768970ota.3; Mon, 12 Jun 2017 08:04:35 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=Lw5n5a+AX3OFQiPPCRFx2uQI9QfJEJIzfFUhK0uxF8M=; b=o/NESiij+RccsOO7VNmg8Lald1GlA0WCbuQEdleShc5vEtCqw+V7yh60ls8GnmYW5v euSqjEuqUzrWGEsP8log32G89nlBaa0zvaZfbRRPyipkHsE2ExmmjaK3qevAkBRCyxfw 9GXJv/JaLueb+7WsTABM/r5yDhjBe8TAM1M8itnGgqGgXtTz1CU9G7vXQvhG3bES848+ tNPt2cal63M8DFt4t1eJ+FJDUOV4hVxUTD0UbzdOtkyoQSjxpIQXTXUYWNNgsJt/k8xP lg61xX7OoeiamwjrO6rWXf9PrWguVxgpUIiqhkzaSK4nST1fmaVL8av9c/YwB6tJn8Sq fmUw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=Lw5n5a+AX3OFQiPPCRFx2uQI9QfJEJIzfFUhK0uxF8M=; b=CUO9oYHpbj2r1iSiR1DT4TaUuFR1Ku88Pp8vmMhQaWwnZYfVlthSCTEbgnPFvIyRh6 U+AmqOWS7ylxc88292SGTG3DP0OeD0HtRufXVqDPdRZXBuGwLZu5mBdYgrPXYyrKrCY0 BS5384mNFbMQuXXcF5gfeXejRfCqPv55L2mcUn1ghAf1EameLn3CHt21FZtSI/3c1uYX WGyfA70s0c1fWE9jTXmVCV9sjoS7X3kww73GUvSdHZZjdORjhRtYvOGMiM6pdbz14QvC Rm6tvulbS61Rwlb7Krwkhi8Rd0338c9EEH+Ei1yE1J2aP7wqti0wJapRPIr57AwcCvsw oQIA== X-Gm-Message-State: AODbwcBGzx5YjK7EG1R7LTMzfIEfS2qUjz0CbR0fAXCDG0I4Pn574JsH dX9jLi14wyLaUyG8alEa7jaZQys2aWcA X-Received: by 10.157.24.51 with SMTP id b48mr23352523ote.143.1497279874150; Mon, 12 Jun 2017 08:04:34 -0700 (PDT) In-Reply-To: <83mv9e6026.fsf@gnu.org> X-detected-operating-system: by eggs.gnu.org: Genre and OS details not recognized. X-Received-From: 2607:f8b0:4003:c0f::22c X-BeenThere: emacs-devel@gnu.org X-Mailman-Version: 2.1.21 Precedence: list List-Id: "Emacs development discussions." List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: emacs-devel-bounces+ged-emacs-devel=m.gmane.org@gnu.org Original-Sender: "Emacs-devel" Xref: news.gmane.org gmane.emacs.devel:215591 Archived-At: --001a113e1e7e0783540551c4a317 Content-Type: text/plain; charset="UTF-8" Eli Zaretskii schrieb am So., 11. Juni 2017 um 17:01 Uhr: > > From: Philipp Stephani > > Date: Sat, 10 Jun 2017 19:58:40 +0000 > > Cc: emacs-devel@gnu.org, phst@google.com > > There's no point in arguing further, as you've already submitted a > patch that I agree with. But just for the record, a couple more > comments to clarify the issue. > Thanks. Please note there's no real argument, just a matter of explicitly making some decisions and documenting them. > > > > You were AFAIU talking about accesses to Lisp objects, and these are > > > only possible via Lisp, which can only be run in a single thread at a > > > time. So if the non-Lisp threads of any kind can enter this picture, > > > in a way that module functions will be run by those threads and access > > > Lisp objects, please describe such a use case, so we all are on the > > > same page regarding the situations we are considering. > > > > Such a use case is what these assertions should guard against. Calling > > environment functions from non-Lisp threads is undefined behavior, and > > these assertions are meant to protect module developers against them. > > Right. But these assertions should IMO not by themselves invoke > undefined behavior, which dereferencing a NULL pointer does. > Yes, as discussed in the other thread these shouldn't be easserts. > > > > Once again: there _are_ legitimate situations in Emacs when for a > > > short time current_thread is NULL. If you assume that these > > > situations don't happen, your code will sooner or later crash and > > > burn. I'm saying this because I once thought current_thread should be > > > non-NULL at all times, and my code which assumed that did crash. > > > > > > > I've reviewed the threads code, and I'm quite sure that current_thread > can > > never be NULL during check_thread. current_thread is only NULL between > > exiting a Lisp thread and resuming execution in another thread after > > reacquiring the GIL. The evaluator doesn't run during that phase, and > > therefore no module functions can run. > > current_thread could only be NULL during check_thread if called from a > > thread that is not a Lisp thread (i.e. an OS thread created by the > module). > > That's exactly one of the undefined behavior situations that the > assertions > > are meant to prevent. > > Right. And that's why dereferencing a potentially NULL pointer is IMO > something we should avoid doing. > Yes, but in general we can't avoid that. It's a module bug, but we can't protect against all module bugs. The assertions are there for module developers to catch these bugs before releasing a module. > > I also think that we should try replacing eassert in this case with a > function that doesn't crash Emacs, only errors out of the offending > module, since a faulty module ideally shouldn't crash the entire Emacs > session. But that might be hard to accomplish. In any case, note > that eassert compiles to nothing in the production build. > Yes, that's why in the other thread I've implemented the module assertions, which are enabled on the command line and reliably crash Emacs. Other ways of error reporting without crashing are unfortunately not possible without major changes to the module API. --001a113e1e7e0783540551c4a317 Content-Type: text/html; charset="UTF-8" Content-Transfer-Encoding: quoted-printable


Eli Za= retskii <eliz@gnu.org> schrieb am= So., 11. Juni 2017 um 17:01=C2=A0Uhr:
> From: Philipp Stephani <p.stephani2@gmail.com>
> Date: Sat, 10 Jun 2017 19:58:40 +0000
> Cc: emacs-dev= el@gnu.org, phst@g= oogle.com

There's no point in arguing further, as you've already submitted a<= br> patch that I agree with.=C2=A0 But just for the record, a couple more
comments to clarify the issue.

Thanks. = Please note there's no real argument, just a matter of explicitly makin= g some decisions and documenting them.
=C2=A0

> > You were AFAIU talking about accesses to Lisp objects, and these = are
> > only possible via Lisp, which can only be run in a single thread = at a
> > time.=C2=A0 So if the non-Lisp threads of any kind can enter this= picture,
> > in a way that module functions will be run by those threads and a= ccess
> > Lisp objects, please describe such a use case, so we all are on t= he
> > same page regarding the situations we are considering.
>
> Such a use case is what these assertions should guard against. Calling=
> environment functions from non-Lisp threads is undefined behavior, and=
> these assertions are meant to protect module developers against them.<= br>
Right.=C2=A0 But these assertions should IMO not by themselves invoke
undefined behavior, which dereferencing a NULL pointer does.

Yes, as discussed in the other thread these shouldn&#= 39;t be easserts.
=C2=A0

> > Once again: there _are_ legitimate situations in Emacs when for a=
> > short time current_thread is NULL.=C2=A0 If you assume that these=
> > situations don't happen, your code will sooner or later crash= and
> > burn.=C2=A0 I'm saying this because I once thought current_th= read should be
> > non-NULL at all times, and my code which assumed that did crash.<= br> > >
>
> I've reviewed the threads code, and I'm quite sure that curren= t_thread can
> never be NULL during check_thread. current_thread is only NULL between=
> exiting a Lisp thread and resuming execution in another thread after > reacquiring the GIL. The evaluator doesn't run during that phase, = and
> therefore no module functions can run.
> current_thread could only be NULL during check_thread if called from a=
> thread that is not a Lisp thread (i.e. an OS thread created by the mod= ule).
> That's exactly one of the undefined behavior situations that the a= ssertions
> are meant to prevent.

Right.=C2=A0 And that's why dereferencing a potentially NULL pointer is= IMO
something we should avoid doing.

Yes, b= ut in general we can't avoid that. It's a module bug, but we can= 9;t protect against all module bugs. The assertions are there for module de= velopers to catch these bugs before releasing a module.
=C2=A0

I also think that we should try replacing eassert in this case with a
function that doesn't crash Emacs, only errors out of the offending
module, since a faulty module ideally shouldn't crash the entire Emacs<= br> session.=C2=A0 But that might be hard to accomplish.=C2=A0 In any case, not= e
that eassert compiles to nothing in the production build.
<= div>
Yes, that's why in the other thread I've impleme= nted the module assertions, which are enabled on the command line and relia= bly crash Emacs.
Other ways of error reporting without crashing a= re unfortunately not possible without major changes to the module API.=C2= =A0
--001a113e1e7e0783540551c4a317--