Re: [PATCH] Support threads in modules

[Philipp Stephani <p.stephani2@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 3284 bytes --]

Eli Zaretskii <eliz@gnu.org> schrieb am So., 11. Juni 2017 um 17:01 Uhr:

> > From: Philipp Stephani <p.stephani2@gmail.com>
> > 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.

[-- Attachment #2: Type: text/html, Size: 4520 bytes --]