From mboxrd@z Thu Jan 1 00:00:00 1970 Path: news.gmane.org!not-for-mail From: Philipp Stephani Newsgroups: gmane.emacs.devel Subject: Re: Dynamic loading progress Date: Fri, 20 Nov 2015 23:22:49 +0000 Message-ID: References: <83k2ptq5t3.fsf@gnu.org> <87h9kxx60e.fsf@lifelogs.com> <877flswse5.fsf@lifelogs.com> <8737wgw7kf.fsf@lifelogs.com> <87io5bv1it.fsf@lifelogs.com> <87egfzuwca.fsf@lifelogs.com> <876118u6f2.fsf@lifelogs.com> <8737w3qero.fsf@lifelogs.com> <831tbn9g9j.fsf@gnu.org> <878u5upw7o.fsf@lifelogs.com> <83ziya8xph.fsf@gnu.org> <83y4du80xo.fsf@gnu.org> <837fld6lps.fsf@gnu.org> <83610w5o97.fsf@gnu.org> NNTP-Posting-Host: plane.gmane.org Mime-Version: 1.0 Content-Type: multipart/alternative; boundary=001a1145aa24f5ce540525012734 X-Trace: ger.gmane.org 1448061798 881 80.91.229.3 (20 Nov 2015 23:23:18 GMT) X-Complaints-To: usenet@ger.gmane.org NNTP-Posting-Date: Fri, 20 Nov 2015 23:23:18 +0000 (UTC) Cc: aurelien.aptel+emacs@gmail.com, tzz@lifelogs.com, emacs-devel@gnu.org To: Eli Zaretskii Original-X-From: emacs-devel-bounces+ged-emacs-devel=m.gmane.org@gnu.org Sat Nov 21 00:23:13 2015 Return-path: Envelope-to: ged-emacs-devel@m.gmane.org Original-Received: from lists.gnu.org ([208.118.235.17]) by plane.gmane.org with esmtp (Exim 4.69) (envelope-from ) id 1Zzv1L-0005jF-QR for ged-emacs-devel@m.gmane.org; Sat, 21 Nov 2015 00:23:12 +0100 Original-Received: from localhost ([::1]:50357 helo=lists.gnu.org) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Zzv1L-00056f-1Z for ged-emacs-devel@m.gmane.org; Fri, 20 Nov 2015 18:23:11 -0500 Original-Received: from eggs.gnu.org ([2001:4830:134:3::10]:44257) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Zzv1G-00056Q-1S for emacs-devel@gnu.org; Fri, 20 Nov 2015 18:23:08 -0500 Original-Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Zzv1D-0007Ov-Q4 for emacs-devel@gnu.org; Fri, 20 Nov 2015 18:23:06 -0500 Original-Received: from mail-wm0-x22f.google.com ([2a00:1450:400c:c09::22f]:38230) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Zzv19-0007Og-QF; Fri, 20 Nov 2015 18:23:00 -0500 Original-Received: by wmec201 with SMTP id c201so39760197wme.1; Fri, 20 Nov 2015 15:22:59 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20120113; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc:content-type; bh=SO+TydQWDC0DzClbQ5KWchHVdjmaV3iaPTYlyzUniRc=; b=peQfv6XfufkOl2h5L2fnlnCjSNrOWwHhHnuv2NQhr7NL83BC7ljTO808ibS5C9m65h aEkZtkH/OQTwptS2NwyAM7QAP2fdDDrLMuguVJfeO8FHfc7veVHwnc7OLoWbKOqK7TqK FGwCSCHLM/K7GIe7830+GN7cCGNjkVTUsjuQofuHrZ4AFvzRNh5gYrcrUao9v5CBs2S2 FvA53LTQd41/4lVtL5XzHWinRFrQeoGJ11rk2o0nOFIy7D8PWLfxbbaxDBt8l7WpE+JI E1q9O+cA9cgjpGPpwdeoR0r6HVxjDJ2svARMjE6dpxJ+fNrDU0zNlfC6Yyrutabt5t3i M9kw== X-Received: by 10.28.17.7 with SMTP id 7mr2326458wmr.45.1448061779167; Fri, 20 Nov 2015 15:22:59 -0800 (PST) In-Reply-To: <83610w5o97.fsf@gnu.org> X-detected-operating-system: by eggs.gnu.org: Error: Malformed IPv6 address (bad octet value). X-Received-From: 2a00:1450:400c:c09::22f X-BeenThere: emacs-devel@gnu.org X-Mailman-Version: 2.1.14 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-bounces+ged-emacs-devel=m.gmane.org@gnu.org Xref: news.gmane.org gmane.emacs.devel:194911 Archived-At: --001a1145aa24f5ce540525012734 Content-Type: text/plain; charset=UTF-8 Eli Zaretskii schrieb am Fr., 20. Nov. 2015 um 22:56 Uhr: > > From: Philipp Stephani > > Date: Fri, 20 Nov 2015 19:58:25 +0000 > > Cc: tzz@lifelogs.com, aurelien.aptel+emacs@gmail.com, > emacs-devel@gnu.org > > > > So you are saying that module_init runs in the main thread, but > > dynamic initializers might run in another thread? How's that > > possible? > > > > According to http://stackoverflow.com/a/29776416 it is not guaranteed > that > > dynamic initializers run in the main thread. > > Ah, you are talking about C++ dynamic initializers! So the model is > that someone writes a module in C++, starts a thread there, and then > inside some dynamic initializer calls emacs-module interface > functions, is that it? If that's the situation, I'd suggest a > prominent commentary describing this in the source. > The SO post talks about C++ but the issue is the same in C. AFAIK with C11 and C++11 the execution models are harmonized. It seems the comment is overly confusing. It is supposed to warn about the following. Naively, if you wanted to test whether you are in the main thread, you would do (module types and naming): static thread_id main_thread = get_current_thread(); bool in_main_thread() { return get_current_thread() == main_thread; } The dynamic initializer here is the first "get_current_thread()"; it is not guaranteed to run in the main thread, so "main_thread" is not guaranteed to contain the main thread ID. Therefore you have to do: static thread_id main_thread; // initialized later int main() { // guaranteed to be in the main thread main_thread = get_current_thread(); } That's all. I'm not aware of any runtime that would run dynamic initializers outside of the main thread, but it's not impossible and easy to protect against. > > > On what OS can this happen? It cannot happen on Windows, AFAIK, > > > > http://blogs.msdn.com/b/oldnewthing/archive/2010/08/27/10054832.aspx > seems to > > indicate that it is possible that other threads continue running after > the main > > thread has exited. > > Not in Emacs: we use the C runtime, and never call ExitThread, let > alone ExitProcess. Doing that would be a silly bug. There's no need > to make a single component of Emacs, and not the most important one, > protected to such extreme degree against calamities that are only > possible if Emacs developers will lose their senses. > Fair enough. We can replace this with a comment ("assume no more threads are running after main has exited") if you prefer that. > > > My initial code didn't use a thread identifier at all because it could > have > > been reused after the main thread ended, introducing a subtle bug. > > No, it cannot be reused, because we hold a handle on the main thread, > see w32term.c around line 6985. As long as a single handle is open on > a thread, it still exists from the OS POV, and its ID cannot be > reused. > > > > This is undefined behavior, but we included an explicit check if > > > checking is enabled because that case is somewhat subtle. > > > > I'm surprised this could happen on some OS. Can you point to any > > details about this? > > > > > > It's not ruled out by the C standard or the APIs of operating systems. > If Emacs > > doesn't call ExitThread or manipulates the C runtime, then indeed it > can't > > happen (at least on Windows), but I think it's brittle to rely on such > > subtleties without explicitly checking for them (somebody might > introduce a > > call to ExitThread in the future without looking at module code). > > I think these precautions are unnecessary. > > Anyway, thanks for explaining this, I now know how to change the code > to DTRT on MS-Windows wrt to the thread checks. > This is unfortunately all surprisingly subtle and vaguely defined. See e.g. http://stackoverflow.com/q/19744250/178761 (apparently the standards are vague about what happens to detached threads after main has exited). > > > OK, but then either there should have been a comment explaining this, > > or, better, the test should have been after the addition. (Which, by > > some strange luck -- or maybe something else -- is just what Paul > > already did ;-) > > > > If the test gets moved after the addition, then we should have a verify > > (MAX_EMACS_UINT - 1 > MOST_POSITIVE_FIXNUM) or use > __builtin_add_overflow to > > make sure the addition doesn't cause UB. > > I don't think so. Please take a look at the code Paul wrote there, I > don't see a problem with it. > There is no problem with the code, but reading it requires knowledge that there are tag bits, which prevent a possible signed overflow. A verify would be a compiler-checked comment for that. > > > > No validity checks on 'fin'? > > > > > > How should it be validated? In C an arbitrary (invalid) pointer > could be > > > passed. I think we just have to accept that this is UB. > > > > Given the extensive checks elsewhere in the file, this surprised me, > > that's all. How to test it? calling valid_pointer_p would be one > > thing I'd suggest. Maybe there are other (perhaps platform-dependent) > > checks possible here, I would have to look around. For example, it > > would be good to make sure this points into executable code. > > > > You can't validate pointers in C. For example, is the following pointer > valid? > > > > long a; > > double *p = (double *)(&a); > > > > The answer is no; this is an aliasing violation, and dereferencing p is > UB, but > > you won't detect this by trying to read the process's address space. > > The function valid_pointer_p validates a pointer in the sense that it > points into the program's address space. That's not a full > validation, and won't catch unaligned pointers or pointers into > non-executable portions of memory, but it's better than nothing. I'd > certainly consider it under "#ifdef ENABLE_CHECKING" at least. > We can add that for consistency, though I'm not sure whether such checks don't do more harm than good. > > > See also > > http://blogs.msdn.com/b/oldnewthing/archive/2006/09/27/773741.aspx. > > We don't use IsBadWritePtr on Windows to check this, see > w32_valid_pointer_p for how this is actually implemented. > > Much of this applies generally. > "But what should I do, then, if somebody passes me a bad pointer?" > You should crash. > Anyway, I'm surprised by this extreme POV: even if we cannot validate > a pointer 100%, surely it doesn't mean we cannot or shouldn't do some > partial job? Why this "all or nothing" approach? > We can check whether it's NULL. Apart from that, everything else is outside of the C standard. > > > > if (len > INT_MAX || len < envptr->min_arity || (envptr->max_arity > >= 0 > > && > > > len > envptr->max_arity)) > > > xsignal2 (Qwrong_number_of_arguments, module_format_fun_env > (envptr), > > > make_number (len)); > > > > > > Why the test against INT_MAX? EMACS_INT can legitimately be a > 64-bit > > > data type with values far exceeding INT_MAX. Why impose this > > > limitation here? > > > > > > Because the nargs argument in the module interface is an int. > > > > Shouldn't it be EMACS_INT instead? > > > > EMACS_INT is an internal type that isn't exposed to module authors. > > OK, but the type of nargs is ptrdiff_t, so the test should be against > the maximum of that type. On 64-bit hosts, ptrdiff_t is a 64-bit > type, while an int is still 32-bit wide. > Yes, the type is *now* ptrdiff_t. It was int when I wrote the code :-) > > > Could xmalloc grow a variant that is guaranteed not to call longjmp? > > We need to devise a way for it to detect that it was called from > emacs-module.c, the rest is simple, I think. > > Hmm, why does it need to detect anything? Can't it just be a different function that doesn't signal, similar to push_handler and push_handler_nosignal? --001a1145aa24f5ce540525012734 Content-Type: text/html; charset=UTF-8 Content-Transfer-Encoding: quoted-printable


Eli Za= retskii <eliz@gnu.org> schrieb am= Fr., 20. Nov. 2015 um 22:56=C2=A0Uhr:
> From: Philipp Stephani <p.stephani2@gmail.com>
> Date: Fri, 20 Nov 2015 19:58:25 +0000
> Cc: tzz@lifelogs= .com, aurelien.aptel+emacs@gmail.com, emacs-devel@gnu.org
>
>=C2=A0 =C2=A0 =C2=A0So you are saying that module_init runs in the main= thread, but
>=C2=A0 =C2=A0 =C2=A0dynamic initializers might run in another thread? H= ow's that
>=C2=A0 =C2=A0 =C2=A0possible?
>
> According to http://stackoverflow.com/a/29776416 it is n= ot guaranteed that
> dynamic initializers run in the main thread.

Ah, you are talking about C++ dynamic initializers!=C2=A0 So the model is that someone writes a module in C++, starts a thread there, and then
inside some dynamic initializer calls emacs-module interface
functions, is that it?=C2=A0 If that's the situation, I'd suggest a=
prominent commentary describing this in the source.
The SO post talks about C++ but the issue is the same in C. AF= AIK with C11 and C++11 the execution models are harmonized.

<= /div>
It seems the comment is overly confusing. It is supposed to warn = about the following. Naively, if you wanted to test whether you are in the = main thread, you would do (module types and naming):

static thread_id main_thread =3D get_current_thread();
bool in= _main_thread() { return get_current_thread() =3D=3D main_thread; }

The dynamic initializer here is the first "get_curren= t_thread()"; it is not guaranteed to run in the main thread, so "= main_thread" is not guaranteed to contain the main thread ID. Therefor= e you have to do:

static thread_id main_thread; = =C2=A0// initialized later
int main() {
=C2=A0 // guara= nteed to be in the main thread
=C2=A0 main_thread =3D get_current= _thread();
}

That's all. I'm not= aware of any runtime that would run dynamic initializers outside of the ma= in thread, but it's not impossible and easy to protect against.

=C2=A0

>=C2=A0 =C2=A0 =C2=A0On what OS can this happen? It cannot happen on Win= dows, AFAIK,
>
> http://blogs.msdn.com/b/old= newthing/archive/2010/08/27/10054832.aspx seems to
> indicate that it is possible that other threads continue running after= the main
> thread has exited.

Not in Emacs: we use the C runtime, and never call ExitThread, let
alone ExitProcess.=C2=A0 Doing that would be a silly bug.=C2=A0 There's= no need
to make a single component of Emacs, and not the most important one,
protected to such extreme degree against calamities that are only
possible if Emacs developers will lose their senses.
<= br>
Fair enough. We can replace this with a comment ("assume= no more threads are running after main has exited") if you prefer tha= t.
=C2=A0

> My initial code didn't use a thread identifier at all because it c= ould have
> been reused after the main thread ended, introducing a subtle bug.

No, it cannot be reused, because we hold a handle on the main thread,
see w32term.c around line 6985.=C2=A0 As long as a single handle is open on=
a thread, it still exists from the OS POV, and its ID cannot be
reused.

>=C2=A0 =C2=A0 =C2=A0> This is undefined behavior, but we included an= explicit check if
>=C2=A0 =C2=A0 =C2=A0> checking is enabled because that case is somew= hat subtle.
>
>=C2=A0 =C2=A0 =C2=A0I'm surprised this could happen on some OS. Can= you point to any
>=C2=A0 =C2=A0 =C2=A0details about this?
>
>
> It's not ruled out by the C standard or the APIs of operating syst= ems. If Emacs
> doesn't call ExitThread or manipulates the C runtime, then indeed = it can't
> happen (at least on Windows), but I think it's brittle to rely on = such
> subtleties without explicitly checking for them (somebody might introd= uce a
> call to ExitThread in the future without looking at module code).

I think these precautions are unnecessary.

Anyway, thanks for explaining this, I now know how to change the code
to DTRT on MS-Windows wrt to the thread checks.

This is unfortunately all surprisingly subtle and vaguely defined.= See e.g.=C2=A0http:= //stackoverflow.com/q/19744250/178761=C2=A0(apparently the standards ar= e vague about what happens to detached threads after main has exited).=C2= =A0
=C2=A0

>=C2=A0 =C2=A0 =C2=A0OK, but then either there should have been a commen= t explaining this,
>=C2=A0 =C2=A0 =C2=A0or, better, the test should have been after the add= ition. (Which, by
>=C2=A0 =C2=A0 =C2=A0some strange luck -- or maybe something else -- is = just what Paul
>=C2=A0 =C2=A0 =C2=A0already did ;-)
>
> If the test gets moved after the addition, then we should have a verif= y
> (MAX_EMACS_UINT - 1 > MOST_POSITIVE_FIXNUM) or use __builtin_add_ov= erflow to
> make sure the addition doesn't cause UB.

I don't think so.=C2=A0 Please take a look at the code Paul wrote there= , I
don't see a problem with it.

There = is no problem with the code, but reading it requires knowledge that there a= re tag bits, which prevent a possible signed overflow. A verify would be a = compiler-checked comment for that.
=C2=A0

>=C2=A0 =C2=A0 =C2=A0> No validity checks on 'fin'?
>=C2=A0 =C2=A0 =C2=A0>
>=C2=A0 =C2=A0 =C2=A0> How should it be validated? In C an arbitrary = (invalid) pointer could be
>=C2=A0 =C2=A0 =C2=A0> passed. I think we just have to accept that th= is is UB.
>
>=C2=A0 =C2=A0 =C2=A0Given the extensive checks elsewhere in the file, t= his surprised me,
>=C2=A0 =C2=A0 =C2=A0that's all. How to test it? calling valid_point= er_p would be one
>=C2=A0 =C2=A0 =C2=A0thing I'd suggest. Maybe there are other (perha= ps platform-dependent)
>=C2=A0 =C2=A0 =C2=A0checks possible here, I would have to look around. = For example, it
>=C2=A0 =C2=A0 =C2=A0would be good to make sure this points into executa= ble code.
>
> You can't validate pointers in C. For example, is the following po= inter valid?
>
> long a;
> double *p =3D (double *)(&a);
>
> The answer is no; this is an aliasing violation, and dereferencing p i= s UB, but
> you won't detect this by trying to read the process's address = space.

The function valid_pointer_p validates a pointer in the sense that it
points into the program's address space.=C2=A0 That's not a full validation, and won't catch unaligned pointers or pointers into
non-executable portions of memory, but it's better than nothing.=C2=A0 = I'd
certainly consider it under "#ifdef ENABLE_CHECKING" at least.

We can add that for consistency, though I= 'm not sure whether such checks don't do more harm than good.
=
=C2=A0

> See also
> http://blogs.msdn.com/b/oldne= wthing/archive/2006/09/27/773741.aspx.

We don't use IsBadWritePtr on Windows to check this, see
w32_valid_pointer_p for how this is actually implemented.


Much of this applies generally.
<= div>
> "But what should I do, then, if somebody passes me a bad= pointer?"
> You should crash.
=C2=A0
Anyway, I'm surprised by this extreme POV: even if we cannot validate a pointer 100%, surely it doesn't mean we cannot or shouldn't do so= me
partial job?=C2=A0 Why this "all or nothing" approach?

We can check whether it's NULL. Apart from th= at, everything else is outside of the C standard.
=C2=A0

>=C2=A0 =C2=A0 =C2=A0> if (len > INT_MAX || len < envptr->mi= n_arity || (envptr->max_arity >=3D 0
>=C2=A0 =C2=A0 =C2=A0&&
>=C2=A0 =C2=A0 =C2=A0> len > envptr->max_arity))
>=C2=A0 =C2=A0 =C2=A0> xsignal2 (Qwrong_number_of_arguments, module_f= ormat_fun_env (envptr),
>=C2=A0 =C2=A0 =C2=A0> make_number (len));
>=C2=A0 =C2=A0 =C2=A0>
>=C2=A0 =C2=A0 =C2=A0> Why the test against INT_MAX? EMACS_INT can le= gitimately be a 64-bit
>=C2=A0 =C2=A0 =C2=A0> data type with values far exceeding INT_MAX. W= hy impose this
>=C2=A0 =C2=A0 =C2=A0> limitation here?
>=C2=A0 =C2=A0 =C2=A0>
>=C2=A0 =C2=A0 =C2=A0> Because the nargs argument in the module inter= face is an int.
>
>=C2=A0 =C2=A0 =C2=A0Shouldn't it be EMACS_INT instead?
>
> EMACS_INT is an internal type that isn't exposed to module authors= .

OK, but the type of nargs is ptrdiff_t, so the test should be against
the maximum of that type.=C2=A0 On 64-bit hosts, ptrdiff_t is a 64-bit
type, while an int is still 32-bit wide.

Yes, the type is *now* ptrdiff_t. It was int when I wrote the code :-)
=C2=A0

> Could xmalloc grow a variant that is guaranteed not to call longjmp?
We need to devise a way for it to detect that it was called from
emacs-module.c, the rest is simple, I think.


Hmm, why does it need to detect anythi= ng? Can't it just be a different function that doesn't signal, simi= lar to push_handler and push_handler_nosignal?=C2=A0
--001a1145aa24f5ce540525012734--