Don't end up in global locale upon thread destruction #20178

khwilliamson · 2022-08-28T18:37:10Z

The POSIX 2008 locale API introduces per-thread locales. But the
previous global locale system is retained, probably for backward
compatibility.

Prior to this commit, there was a bug in which, when a thread
terminates, the master thread was switched into the global locale. That
meant that that thread was no longer thread-safe with regards to
locales.

This bug stems from the fact that perl assumes that all you need to do
to switch between threads (or embedded interpreters) is to change out
aTHX. Indeed much effort was expended in crafting perl to make this the
case. But it breaks down in the case of some alien library that keeps
per-thread information. That library needs to be informed of the
switch. In this case it is libc keeping per-thread locale information.
We change the thread context, but the library still retains the old
thread's locale.

One cannot be using a given locale object and successfully free it.
Therefore the code switches to the global locale (which isn't
deletable) before freeing. There was no apparent need to do more
switching, as the thread is in the process of dying. What I was unaware
of is that it is the parent thread pretending to be the dying one for the
purposes of destruction. So switching to the global locale affected the
parent, leaving it there.

The parent thread called the locale.c thread locale termination
function, and then called the perl.c perl_destruct() on the thread. This
commit moves all the code for thread destruction from perl.c into the
locale.c code, and calls it. Thus the thread initiation and termination
is moved into locale.c

The thread termination is also called from thread.c. This cleans up a
dying thread. The perl.c call is needed for thread0 and
non-multiplicity builds. A check is done to prevent duplicate work.

This commit adds a new per-interpreter variable which maps aTHX to its
locale. This is used to get the terminating thread's locale instead of
the master. And the master locale is switched back to at the end.

This commit is incomplete. Something similar needs to be done for
Windows where the libc knows the per-thread locale.

I'm unsure of if this is the full correct approach. It only works for
thread termination. Perhaps a better solution would be to change the
locale every time aTHX is changed. PERL_SET_INTERP, PERL_SET_CONTEXT,
and PERL_SET_THX all seem to do the aTHX change, and I can't figure out
when you would prefer one over the other. But maybe one of them should
then arrange also to change the locale when aTHX is changed.

Perhaps you can think of other libraries and functions that have a
similar problem that also would need something like this.

This commit causes #20155 to go away. The triggering failure is merely
a symptom of the deeper problem. A proper test will need to be done in
XS.

jkeenan · 2022-08-28T18:59:03Z

This commit causes #20155 to go away. The triggering failure is merely a symptom of the deeper problem. A proper test will need to be done in XS.

This gets a quick build-time error on OpenBSD.

$ uname -mrs
OpenBSD 6.9 amd64

$ cc --version | head -1
OpenBSD clang version 10.0.1 

$ sh ./Configure -des -Dusedevel && make test_prep
...
cc -c -DPERL_CORE -fno-strict-aliasing -pipe -fstack-protector-strong -I/usr/local/include -std=c99 -O2 -Wall -Werror=pointer-arith -Werror=vla -Wextra -Wno-long-long -Wno-declaration-after-statement -Wc++-compat -Wwrite-strings locale.c
locale.c:6960:24: error: 'void' must be the first and only parameter if specified
Perl_thread_locale_term(pTHX, const bool caller_is_thread_termination)
                       ^
locale.c:6960:1: error: conflicting types for 'Perl_thread_locale_term'
Perl_thread_locale_term(pTHX, const bool caller_is_thread_termination)
^
./proto.h:4353:20: note: previous declaration is here
PERL_CALLCONV void      Perl_thread_locale_term(pTHX_ const bool caller_is_thread_termination);
                        ^
locale.c:6960:29: error: parameter name omitted
Perl_thread_locale_term(pTHX, const bool caller_is_thread_termination)
                            ^
3 errors generated.
*** Error 1 in target 'locale.o'
*** Error 1 in /home/jkeenan/gitwork/perl (makefile:246 'locale.o': @`sh  cflags "optimize='-O2'" locale.o`  locale.c)

(It did, however, pass all tests on FreeBSD-12.)

jkeenan · 2022-08-28T19:06:51Z

(It did, however, pass all tests on FreeBSD-12.)

That was a threaded build on FreeBSD. But it looks like this is failing more generally on unthreaded builds. Unthreaded build on FreeBSD:

cc -c -DPERL_CORE -DHAS_FPSETMASK -DHAS_FLOATINGPOINT_H -fno-strict-aliasing -pipe -fstack-protector-strong -I/usr/local/include -std=c99 -O2 -Wall -Werror=pointer-arith -Werror=vla -Wextra -Wno-long-long -Wno-declaration-after-statement -Wc++-compat -Wwrite-strings locale.c
locale.c:6960:24: error: 'void' must be the first and only parameter if specified
Perl_thread_locale_term(pTHX, const bool caller_is_thread_termination)
                       ^
locale.c:6960:1: error: conflicting types for 'Perl_thread_locale_term'
Perl_thread_locale_term(pTHX, const bool caller_is_thread_termination)
^
./proto.h:4353:20: note: previous declaration is here
PERL_CALLCONV void      Perl_thread_locale_term(pTHX_ const bool caller_is_thread_termination);
                        ^
locale.c:6960:29: error: parameter name omitted
Perl_thread_locale_term(pTHX, const bool caller_is_thread_termination)
                            ^
3 errors generated.
*** Error code 1

Stop.

Threaded build on OpenBSD in progress -- but it's gotten past the locale.c failure point in the unthreaded build.

Leont · 2022-08-29T21:02:22Z

My opinion on "should this be API or not" greatly depends on "do I need this to correctly support locales with Thread::Csp". I have the impression the answer is "yes"

Leont · 2022-09-04T18:57:05Z

dist/threads/threads.xs

@@ -667,8 +668,6 @@ S_ithread_run(void * arg)
    MUTEX_UNLOCK(&thread->mutex);
    MUTEX_UNLOCK(&MY_POOL.create_destruct_mutex);

-    thread_locale_term();


I think this is wrong, or if it's right it needs more explanation of why it's right.

S_ithread_clear will be called from other (OS) threads, whereas S_ithread_run will be run from the interpreter's own thread. This is an essential distinction.

we agree its wrong

Leont · 2022-09-07T10:37:44Z

Currently the code will destroy the current locale on the thread that calls perl_destroy. Because of threads.pm architecture this will often not be the thread matching the interpreter (ithread objects are refcounted, and perl_destroy is called in the last thread referencing it).

I can think of two possible solution:

perl_destroy never destroys thread-local data like the locale.
This does beg the question how we will destroy it.
threads.pm will always call perl_destroy from the matching thread.
This will require a rearchitecture of thread destruction in threads.pm

To be honest, doing both may be the best idea.

A future commit will need it to be defined earlier in the file than currently.

This moves the code for cloning the locale variables to a single place, consolidating some that had the same cpp directives. It showed that one variable was cloned twice; the redundant one is now removed.

A thread is supposed to start with the locale set to the C locale. We were duping the parent values, and later overriding. Better to set to C from the beginning.

Some configurations require us to store the current locale for each category. Prior to this commit, this was done in the array PL_curlocales, with the entry for LC_ALL being in the highest element. Future commits will need just the value for LC_ALL in some other configurations, without needing the rest of the array. This commit splits off the LC_ALL element into its own per-interpreter variable to accommodate those. It always has had to be handled specially anyway, not like the rest.

A future commit will want the context for more than just DEBUGGING builds.

This is in preparation for it to be used in more instances in future commits.

This prevents some unnecessary steps, that the next commit would turn into memory leaks.

So far this hadn't been an issue, but it will be for a future commit.

This is a step in solving Perl#20155 The POSIX 2008 locale API introduces per-thread locales. But the previous global locale system is retained, probably for backward compatibility. The POSIX 2008 interface causes memory to be malloc'd that needs to be freed. In order to do this, the caller must first stop using that memory, by switching to another locale. perl accomplishes this during termination by switching to the global locale, which is always available and doesn't need to be freed. Perl has long assumed that all that was needed to switch threads was to change out tTHX. That's because that structure was intended to hold all the information for a given thread. But it turns out that this doesn't work when some library independently holds information about the thread's state. And there are now some libraries that do that. What was happening in this case was that perl thought that it was sufficient to switch tTHX to change to a different thread in order to do the freeing of memory, and then used the POSIX 2008 function to change to the global locale so that the memory could be safely freed. But the POSIX 2008 function doesn't care about tTHX, and actually was typically operating on a different thread, and so changed that thread to the global locale instead of the intended thread. Often that was the top-level thread, thread 0. That caused whatever thread it was to no longer be in the expected locale, and to no longer be thread-safe with regards to localess, This commit causes locale_term(), which has always been called from the actual terminating thread that POSIX 2008 knows about, to change to the global thread and free the memory. It also creates a new per-interpreter variable that effectively maps the tTHX thread to the associated POSIX 2008 memory. During perl_destruct(), it frees the memory this variable points to. This catches anything missed by locale_term(). This fixes the symptoms associtated with Perl#20155, but doesn't solve the whole problem. In general, a library that has independent thread status needs to be updated to the new thread when Perl changes threads using tTHX. Future commits will do this.

This partially reverts ebb1d9c. The real fix for this problem has now been committed, so the workaround can be reverted, leaving the tests.

khwilliamson · 2022-09-29T20:09:06Z

#20361 supersedes this

khwilliamson mentioned this pull request Aug 28, 2022

Locale patch breaks dist/threads/t/kill.t by triggering attribute.pm XS version mismatch errors. #20155

Closed

jkeenan added type-locale type-threads labels Aug 28, 2022

khwilliamson force-pushed the attributes branch from 475b64b to 3c10fd8 Compare August 29, 2022 16:46

khwilliamson changed the title ~~Mark locale_thread_(init|term) as internal~~ Don't end up in global locale upon thread destruction Aug 29, 2022

github-actions bot added the hasConflicts label Aug 31, 2022

Leont reviewed Sep 4, 2022

View reviewed changes

khwilliamson added 14 commits September 29, 2022 13:12

perl.h: Move PERL_CALLCONV definition to earlier

6de29e3

A future commit will need it to be defined earlier in the file than currently.

sv.c: Move some code; consolidate

16b4f99

This moves the code for cloning the locale variables to a single place, consolidating some that had the same cpp directives. It showed that one variable was cloned twice; the redundant one is now removed.

sv.c: Clone interpreter with locale set to C

96f8404

A thread is supposed to start with the locale set to the C locale. We were duping the parent values, and later overriding. Better to set to C from the beginning.

Add pTHX to thread_locale_(init|term)

b6f605b

A future commit will want the context for more than just DEBUGGING builds.

XXX NOT ACCURATE: locale.c: Compile display fcn under more circumstances

0bc92fb

This is in preparation for it to be used in more instances in future commits.

locale.c: Do uselocale() earlier in init process

81cfc06

This prevents some unnecessary steps, that the next commit would turn into memory leaks.

sv.c: Set phase to CONSTRUCT on interpreter being cloned

fe07765

So far this hadn't been an issue, but it will be for a future commit.

thread.h: White space only

9d3c6e3

l

9ff7822

win

ade885e

Revert code changes for Workaround for attributes.pm breakage

b9e71aa

This partially reverts ebb1d9c. The real fix for this problem has now been committed, so the workaround can be reverted, leaving the tests.

version

5ea0082

khwilliamson force-pushed the attributes branch from 3c10fd8 to 5ea0082 Compare September 29, 2022 19:28

khwilliamson closed this Sep 29, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Don't end up in global locale upon thread destruction #20178

Don't end up in global locale upon thread destruction #20178

khwilliamson commented Aug 28, 2022 •

edited

Loading

jkeenan commented Aug 28, 2022

jkeenan commented Aug 28, 2022

Leont commented Aug 29, 2022 •

edited

Loading

Leont Sep 4, 2022

khwilliamson Sep 6, 2022

Leont commented Sep 7, 2022 •

edited

Loading

khwilliamson commented Sep 29, 2022

Don't end up in global locale upon thread destruction #20178

Don't end up in global locale upon thread destruction #20178

Conversation

khwilliamson commented Aug 28, 2022 • edited Loading

jkeenan commented Aug 28, 2022

jkeenan commented Aug 28, 2022

Leont commented Aug 29, 2022 • edited Loading

Leont Sep 4, 2022

Choose a reason for hiding this comment

khwilliamson Sep 6, 2022

Choose a reason for hiding this comment

Leont commented Sep 7, 2022 • edited Loading

khwilliamson commented Sep 29, 2022

khwilliamson commented Aug 28, 2022 •

edited

Loading

Leont commented Aug 29, 2022 •

edited

Loading

Leont commented Sep 7, 2022 •

edited

Loading