[issue25658] PyThread assumes pthread_key_t is an integer, which is against POSIX

[Stefan Behnel]

Stefan Behnel  added the comment:

Seems like this isn't trivial, so I created a new ticket for this. See issue 
31828.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue25658] PyThread assumes pthread_key_t is an integer, which is against POSIX

[Stefan Behnel]

Stefan Behnel  added the comment:

It seems that there's a simpler way that uses a cast on the literal. I added a 
pull request. Looks simple enough to not merit its own ticket, I think.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue25658] PyThread assumes pthread_key_t is an integer, which is against POSIX

[Stefan Behnel]

Change by Stefan Behnel :


--
pull_requests: +4029

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue25658] PyThread assumes pthread_key_t is an integer, which is against POSIX

[Nick Coghlan]

Nick Coghlan  added the comment:

Stefan: I'd expect so, but that's best covered by a new RFE and an associated 
PR (I'm not sure what you mean from the text description, but I assume it will 
be obvious as C code)

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue25658] PyThread assumes pthread_key_t is an integer, which is against POSIX

[Stefan Behnel]

Stefan Behnel  added the comment:

Would it be possible to define Py_tss_NEEDS_INIT as a constant variable instead 
of a static initialiser? That would enable its use also for non-static 
initialisations.

--
nosy: +scoder

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue25658] PyThread assumes pthread_key_t is an integer, which is against POSIX

[Nick Coghlan]

Nick Coghlan  added the comment:

This has been merged now: 
https://github.com/python/cpython/commit/731e18901484c75b60167a06a0ba0719a6d4827d

Thank you for the PEP & implementation! :)

--
resolution:  -> fixed
stage: patch review -> resolved
status: open -> closed

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue25658] PyThread assumes pthread_key_t is an integer, which is against POSIX

[Masayuki Yamamoto]

Masayuki Yamamoto added the comment:

FYI, PEP 539 was accepted (see python-dev threads [1] [2]).
The only thing missing is a reference implementation, next I complete it.

[1] https://mail.python.org/pipermail/python-dev/2017-August/149091.html
[2] https://mail.python.org/pipermail/python-dev/2017-September/149354.html

--
versions:  -Python 3.6

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue25658] PyThread assumes pthread_key_t is an integer, which is against POSIX

[Masayuki Yamamoto]

Masayuki Yamamoto added the comment:

Since previous comment, I've studied the switch for show/hide implementation 
detail.  As the result, I have understood the Py_BUILD_CORE macro hasn't been 
generally used for hiding implementation detail (and Py_LIMITED_API does the 
part).
Therefore, I withdraw previous proposal for implementation detail, and I'm 
going to fix PEP a few.  After that, I'd like to post to python-dev to know the 
developer's view of the PEP draft.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue25658] PyThread assumes pthread_key_t is an integer, which is against POSIX

[Masayuki Yamamoto]

Masayuki Yamamoto added the comment:

oh, I found a mistake.
- replace Py_LIMITED_API with Py_BUILD_CORE on Include/pythread.h Py_tss_t 
definition

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue25658] PyThread assumes pthread_key_t is an integer, which is against POSIX

[Masayuki Yamamoto]

Masayuki Yamamoto added the comment:

Nick and Erik, thank you for the comments! I merged proposal into the PR.

Well, I'm interested in the hide implementation detail for TSS API (lately, 
I've read the python-ideas thread "PEP: Hide implementation details in the C 
API" which Victor opened [1]).

Present, the draft of new API has given two methods for thread key 
initialization for the non-limited API (i.e. Py_tss_NEEDS_INIT for statically, 
PyThread_tss_alloc for dynamic). The static initialization needs implementation 
detail for thread key, but Py_tss_t is designed as an opaque data type based on 
the stable ABI and we don't feel like to open the implementation detail to the 
API client. On the other hand, we'd provide newly thread key (de-)allocation 
functions for the limited API, the API client is able to get an initialized 
value without knowing thread key detail. And also the functions can be used on 
the non-limited API.

Therefore, I think it makes more sense that all API clients use 
PyThread_tss_(alloc|free) regardless of the limited API. The reason is which 
are (1) Py_tss_t behaves as an opaque data type as expected for any API client 
(cannot read and write directly the fields in any case), (2) the API gets more 
consistency (just one method for key initialization on the API client).

TL;DR: I'd suggest to make key type strict, what do you think about below 
changes?

- replace Py_LIMITED_API with Py_BUILD_CORE on Python/pythread.h Py_tss_t 
definition
- use PyThread_tss_(alloc|free) in Modules/_tracemalloc.c
- also use in Modules/_testcapimodule.c

[1] https://mail.python.org/pipermail/python-ideas/2017-July/046399.html

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue25658] PyThread assumes pthread_key_t is an integer, which is against POSIX

[Nick Coghlan]

Nick Coghlan added the comment:

Nice :)

With the legacy code cleanups merged, I'd say the next step would be to update 
the PEP with the simplified API and the explanation for why the removed 
functions are no longer needed (i.e. we're making native TSS support a hard 
dependency for 3.7+).

I'm also thinking we may want to update PEP 11, akin to the entry for the 
legacy ASCII-only C locale: 
https://www.python.org/dev/peps/pep-0011/#legacy-c-locale

I'll handle the PEP 11 update after PEP 539 is approved, the PEP itself would 
just need a new Platform Support Changes section like the one in PEP 538: 
https://www.python.org/dev/peps/pep-0538/#platform-support-changes

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue25658] PyThread assumes pthread_key_t is an integer, which is against POSIX

[Masayuki Yamamoto]

Masayuki Yamamoto added the comment:

Hi, I attempted Nick's proposal and removed unused codes from TLS 
implementation (bpo-30279, bpo-30832). This change looks good to me (PR 1362).
As the result, I think ready to be more slim API because the own implementation 
for TLS was removed. Therefore, additional proposal to two API functions:

* Omit PyThread_ReInitTSS function
* Omit PyThread_tss_delete_value function

These functions have handled memory deallocation to key-value storage on the 
own implementation, but above said, currently the own implementation is gone. 
It means:

1. PyThread_ReInitTSS makes to be no-op for all supported platforms (Windows 
and pthreads)
2. likewise PyThread_tss_delete_value just does to empty the storage (i.e. 
"PyThread_tss_delete_value(key)"  makes to be equal to "PyThread_tss_set(key, 
NULL)").

We (or at least I) won't get back to the own implementation, therefore, I 
suggest to omit these functions for new TSS API.

See proposed updates https://github.com/ma8ma/cpython/pull/1

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue25658] PyThread assumes pthread_key_t is an integer, which is against POSIX

[Nick Coghlan]

Nick Coghlan added the comment:

Noting a design consideration that I only picked up in the latest PR review: 
when exposed as part of a non-opaque struct, the type used for TSS keys becomes 
part of Python's ABI, which means the API design in the PEP is going to have to 
handle making the struct fully opaque when Py_LIMITED_API is defined.

My proposal for handling that:

- update all of the API functions to accept Py_tss_t by pointer rather than by 
value
- update the Py_tss_t definition to be an opaque struct when Py_LIMITED_API is 
defined
- add PyThread_tss_alloc and PyThread_tss_free functions for dynamic allocation 
of key storage (since static allocation won't be possible in the Py_LIMITED_API 
case)

My main rationale for going to that level of effort is that the current thread 
local storage API is available when Py_LIMITED_API is defined, so its 
replacement should be available as well. However, exposing NATIVE_TSS_KEY_T as 
part of the stable ABI would prevent us from switching from `int` to a platform 
specific implementation for platforms that start out using the generic 
emulation.

If issue #29881 ever became a public API, it would then be useful for managing 
the lifecycle of dynamically allocated TSS keys in extension modules that 
restrict themselves to the stable ABI.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue25658] PyThread assumes pthread_key_t is an integer, which is against POSIX

[Masayuki Yamamoto]

Masayuki Yamamoto added the comment:

Victor,
The tracemalloc module is not available on platforms that provides own TLS 
implementation that uses PyMem_* functions.  In the own implementation, it 
occurs deadlock by recursion call in thread key search.  PyOnceVar API handles 
extendable array by using PyMem_Reallloc, therefore, I think TSS key 
initialization using the API will raise deadlock likewise.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue25658] PyThread assumes pthread_key_t is an integer, which is against POSIX

[Masayuki Yamamoto]

Changes by Masayuki Yamamoto :


--
pull_requests: +1472

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue25658] PyThread assumes pthread_key_t is an integer, which is against POSIX

[STINNER Victor]

STINNER Victor added the comment:

Hi people working on the new TLS API: I would like your opinion on a related 
API, issue #29881: Add a new private API for "static C variables" 
(_PyStaticVar) to clear them at exit !

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue25658] PyThread assumes pthread_key_t is an integer, which is against POSIX

[Masayuki Yamamoto]

Masayuki Yamamoto added the comment:

Above said, I updated minor changes to the version 2 patch.  Several codes have 
kept the words "thread local" and "TLS" because they have pointed programming 
method or other meanings, not CPython TLS API itself. (e.g. _decimal module)

--
Added file: http://bugs.python.org/file46379/pythread-tss-3.patch

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue25658] PyThread assumes pthread_key_t is an integer, which is against POSIX

[Masayuki Yamamoto]

Masayuki Yamamoto added the comment:

I commented at the Rietveld, since I think that patch needs a bit more 
modification.

* rename PyThread_ReInitTLS
* update comments and messages that have explained CPython TLS API

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue25658] PyThread assumes pthread_key_t is an integer, which is against POSIX

[Erik Bray]

Erik Bray added the comment:

Thanks Masayuki for the updated patch, and especially for the additional test 
cases.

Looking at the patch, it occurs to me that this solution seems to be the 
minimal needed to fix the issue that we were originally trying to fix, without 
adding too much additional complexity or new semantics to how TLS keys are used 
in the interpreter.  In other words, this preserves the existing usage with 
minimal changes except to better support opaque key types.  So I think it's a 
point in favor of this change that's managed to remain focused.

I'll work on updating the PEP draft to reflect the additions.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue25658] PyThread assumes pthread_key_t is an integer, which is against POSIX

[Masayuki Yamamoto]

Masayuki Yamamoto added the comment:

After Erik posted PEP 539 draft, we've discussed features of new API on the 
Python-ideas [*]. As the result of discussion, features of new API have been 
changed below points. Thus, I updated patch based on the result.

1. API uses opaque type because to cover the key details.
2. The key has the state that is whether initialized, and a function is added 
to check the state.
3. When key creation and deletion, API silently skips function in unnecessary 
case (i.e. valid key never be overwritten).
4. The test is added to check the key state that after calling API.

[*] https://mail.python.org/pipermail/python-ideas/2016-December/043983.html

--
Added file: http://bugs.python.org/file46227/pythread-tss-2.patch

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue25658] PyThread assumes pthread_key_t is an integer, which is against POSIX

[Nick Coghlan]

Changes by Nick Coghlan :


--
nosy: +ncoghlan

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue25658] PyThread assumes pthread_key_t is an integer, which is against POSIX

[Erik Bray]

Erik Bray added the comment:

I'm still pretty happy with the previous patch, personally, since I don't need 
the tracemalloc module.  But it seems like that should be fixed (or if nothing 
else that code in _tracemalloc.c should check Py_HAVE_NATIVE_TLS too).

I like the idea of the new PyThread_tss_ API.  At first I wasn't sure because I 
thought you implied that it would use tss_t and related APIs from C11 which was 
going to be a non-starter (as CPython has only just barely started using *some* 
features from C99, per the recent update to PEP 7).  But I see in your patch 
that the naming is only inspired by C11 (and could be consistent with it if 
CPython ever moves toward C11 support).

I imagine this will likely require a PEP though?  I would happy to help draft 
one.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue25658] PyThread assumes pthread_key_t is an integer, which is against POSIX

[Masayuki Yamamoto]

Masayuki Yamamoto added the comment:

I wrote a patch based on msg281227's idea.  I confirmed to pass related tests 
(test_capi, test_threading and test_tracemalloc) on Cygwin x86 and Ubuntu 16.04 
x86.

This patch adds to change CPython about:

1. Avoid compile error on Currently TLS API.
   With POSIX threads, configure script checks pthread_key_t type. And if the 
type is not integer, TLS API is implemented empty (do nothing or return fail).
2. Implement New TSS API based on C11 threads.
   Thread key type is changed wrapped native TLS key type by typedef instead of 
integer, and the key parameter on key create function is changed to calling by 
pointer.  Key create function doesn't support destructor because users have 
need to manage memory.  And this patch changes Currently TLS API to call New 
TSS API. Or does it need to keep implementation?
3. Move to TSS API.
   Py_DEPRECATED(3.7) is added to TLS API declaration, and 
Modules/_tracemalloc.c and Python/pystate.c are modified to use TSS API instead 
of TLS API.

--
Added file: http://bugs.python.org/file45763/pythread-tss.patch

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue25658] PyThread assumes pthread_key_t is an integer, which is against POSIX

[Masayuki Yamamoto]

Masayuki Yamamoto added the comment:

Elik, Ed,

I have overlooked tracemalloc module raises deadlock if apply the patch.
I found out a source comment on Modules/_tracemalloc.c:161

/* If your OS does not provide native thread local storage, you can implement
   it manually using a lock. Functions of thread.c cannot be used because
   they use PyMem_RawMalloc() which leads to a reentrant call. */
#if !(defined(_POSIX_THREADS) || defined(NT_THREADS))
#  error "need native thread local storage (TLS)"
#endif

Py_HAVE_NATIVE_TLS is used only on thread source code. Therefore C Compiler 
couldn't report error about tracemalloc. I'm sorry that I didn't check test.
Currently I'm trying to implement new API based on msg281227.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue25658] PyThread assumes pthread_key_t is an integer, which is against POSIX

[Ed Schouten]

Ed Schouten added the comment:

Looks good to me!

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue25658] PyThread assumes pthread_key_t is an integer, which is against POSIX

[Erik Bray]

Erik Bray added the comment:

To me, Masayuki's patch is an acceptable work-around for the time being.  I 
don't *like* it because the fact remains that native TLS can work on these 
platforms and falling back on Python's slower implementation isn't necessary.  
That said, the previous patch attempts also add additional overhead that 
shouldn't be necessary.

I agree the best thing to do would be to change this API, but that is a bigger 
issue I guess...

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue25658] PyThread assumes pthread_key_t is an integer, which is against POSIX

[Masayuki Yamamoto]

Masayuki Yamamoto added the comment:

I wrote a patch to avoid compile error for platforms that pthread_key_t is not 
integer.
This patch changes to turn off Py_HAVE_NATIVE_TLS if pthread_key_t is not 
integer. Hence the platforms use TLS functions implemented by CPython self.

And, I would propose new thread local storage API based on C11 thread and 
current TLS functions move to deprecated. C11 tss_t doesn't require defined as 
integer [1]. Therefore I think new API should use tss_t, not hide into integer.

[1] http://www.open-std.org/jtc1/sc22/wg14/www/docs/n1570.pdf (page 394)

I'm thinking of new interfaces. For example, declaration in Include/pythread.h

/* Specialise to each platforms using #if directive */
typedef /* TLS key types or C11 tss_t */ Py_tss_t;

/* Based on C11 threads.h, but destructor doesn't support.
   the delete value function is maintained for the implementation by CPython 
self.
*/
PyAPI_FUNC(int) PyThread_tss_create(Py_tss_t *);
PyAPI_FUNC(void) PyThread_tss_delete(Py_tss_t);
PyAPI_FUNC(void *) PyThread_tss_get(Py_tss_t);
PyAPI_FUNC(int) PyThread_tss_set(Py_tss_t, void *);
PyAPI_FUNC(void) PyThread_tss_delete_value(Py_tss_t);

--
Added file: http://bugs.python.org/file45548/configure-pthread_key_t.patch

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue25658] PyThread assumes pthread_key_t is an integer, which is against POSIX

[Masayuki Yamamoto]

Masayuki Yamamoto added the comment:

On, I got complex type in cloudABI. I see my patch doesn't solve it.

https://github.com/NuxiNL/cloudlibc/blob/master/src/include/sys/types.h#L94
https://github.com/NuxiNL/cloudlibc/blob/master/src/include/_/types.h#L209

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue25658] PyThread assumes pthread_key_t is an integer, which is against POSIX

[Ed Schouten]

Ed Schouten added the comment:

CloudABI uses a structure type, which is exactly why I filed this bug report.

Instead of speculating about what kind of type existing implementations use, 
please just focus on the specification to which we're trying to stick: POSIX.

http://pubs.opengroup.org/onlinepubs/9699919799/basedefs/sys_types.h.html

"All of the types shall be defined as arithmetic types of an appropriate 
length, with the following exceptions:

[...]
pthread_key_t
[...]"

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue25658] PyThread assumes pthread_key_t is an integer, which is against POSIX

[Masayuki Yamamoto]

Masayuki Yamamoto added the comment:

Umm, API seems a design that is passing into function by integer or pointer 
because the users don't need type detail. I think the implementation of complex 
type is not realistic. Actually, CouldABI and Cygwin are used pointer, and type 
detail is hided.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue25658] PyThread assumes pthread_key_t is an integer, which is against POSIX

[Ed Schouten]

Ed Schouten added the comment:

It can also be a structure or a union.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue25658] PyThread assumes pthread_key_t is an integer, which is against POSIX

[Masayuki Yamamoto]

Masayuki Yamamoto added the comment:

Hi, I came from #28656.
I have read past discussions, And I've understood the pthread_key_t has 
possible of either integer or pointer.  So I think there is a simple solution 
that replaces key type to intptr_t.
Thus I wrote a patch, but it maybe need configure check for intptr_t if 
CPytyhon still be supporting for before C99 compilers.
I confirmed patch on Cygwin x86. Would you be able to confirm for other 
platforms?

--
nosy: +masamoto
type:  -> compile error
versions: +Python 3.7
Added file: http://bugs.python.org/file45427/pythread-key-intptr_t.patch

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue25658] PyThread assumes pthread_key_t is an integer, which is against POSIX

[Erik Bray]

Erik Bray added the comment:

Ah, I wasn't thinking clearly toward the bottom of my last message.  I see now 
that after a fork, _PyGILState_Reinit calls PyThread_delete_key followed by a 
new PyThread_create_key--in the current version of my patch that would result 
in putting the autoTLSkey at the end of the linked list, which is no good.  
That could be worked around, but...

Ed's version looks good to me.  I had the same idea as an alternative, though 
was a little concerned with the possibility that the array could grow too 
large.  But as I wrote in my last message that would be an extreme case.  And 
regardless his version will at least maintain constant time, so +1.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue25658] PyThread assumes pthread_key_t is an integer, which is against POSIX

[Ed Schouten]

Ed Schouten added the comment:

I think that PEP 11 also doesn't rule out changes in this area. For example, 
consider the paragraph starting with the sentence "This policy does not 
disqualify supporting other platforms indirectly. [...]"

Attached is a patch that accomplishes the same with a constant running time for 
operations on keys by using an array. Allocation of new keys runs in expected 
constant time by using a randomised allocation pattern.

If we're not big fans of using randomised access, changing this to a linear 
scan would already be an improvement: keys are only allocated infrequently, but 
hopefully accessed quite often.

--
Added file: http://bugs.python.org/file44303/key-constant-time.diff

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue25658] PyThread assumes pthread_key_t is an integer, which is against POSIX

[Erik Bray]

Erik Bray added the comment:

Hi,

First of all, I should be clear this is not just about CloudABI.  In fact I 
don't even know what CloudABI is.  I'm personally working toward getting a 
working/stable buildbot for Cygwin, so that I can move Python toward supporting 
it again.  Cygwin has not been officially unsupported, but support for it is 
broken due to lack of a buildbot and lack of a maintainer, both of which I'm 
willing to offer.  I can't get a stable buildbot though until some issues with 
the build are resolved, and this is a major blocker.  If you prefer I can also 
work on a more formal plan to clarify Python's support for Cygwin which is 
currently in limbo.

Not saying a solution to this needs to be merged ASAP as I can work on a 
branch, but in the meantime it's nice to have an initial fix for the issue so 
that it can be worked around--I'm sure other platforms that have this issue, 
such as the CloudABI folks, will appreciate that as well.

Second of all, the point must be made that I'm not suggesting this be changed 
just in order to support some specific platform.  The fact remains that the 
existing code is misusing the pthread API and is fragile to begin with.  "It 
works on Linux" is not an excuse--not only does it suggest a bias, but the fact 
that it even works on Linux is an accident of implementation. The 
implementation of the pthread_key_t type could change tomorrow, and it would be 
Python's fault if that breaks things (not that I think that's at all likely to 
happen, though I'd be less surprised if OSX suddenly changed :)

As for the performance I agree that PyThread_(get|set)_key_value should be 
fast.  This may be O(n) but how big, typically, is n?  In a normal run of the 
Python interpreter n=1, and the autoTLSkey is always the first in the list.  It 
is only larger if some third-party code is using the PyThread_ API for TLS (and 
in most typical uses this will only be a few keys at the most, though I 
couldn't even find any examples in the wild where third-party code is using 
these functions).

n could also grow on forks, or manual Py_Finialize/Py_Initialize.  This patch 
didn't implement PyThread_ReInitTLS but it could, in which case it would reset 
next_key_id to 0 for each child process.  Likewise, PyThread_ReInitTLS could 
also be called from Py_Initialize, which should be harmless.

So TL;DR I'm not really convinced by the performance argument.  But even if 
that were an issue other implementations are possible; this was just among the 
simplest and least wasteful.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue25658] PyThread assumes pthread_key_t is an integer, which is against POSIX

[STINNER Victor]

STINNER Victor added the comment:

-1 for your change issue25658-1.patch.

It adds a O(n) slowdown to PyThread_set_key_value() whereas the performance of 
this function matters. Moreover, the code currently works fine on Linux, I fail 
to see why we should make Python slower to support a new platform.

The PEP 11 explains the criteria to support a new platform in Python. Before we 
start to officially support CloudABI, you should start to work on a fork. When 
you will get enough users and have a better view of all requires changes, you 
can come back with a full plan to support officially this platform.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue25658] PyThread assumes pthread_key_t is an integer, which is against POSIX

[Erik Bray]

Changes by Erik Bray :


--
stage:  -> patch review

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue25658] PyThread assumes pthread_key_t is an integer, which is against POSIX

[Erik Bray]

Changes by Erik Bray :


--
keywords: +patch
Added file: https://bugs.python.org/file44269/issue25658-1.patch

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue25658] PyThread assumes pthread_key_t is an integer, which is against POSIX

[Erik Bray]

Erik Bray added the comment:

Here's a first stab at a patch for this.  A linked list is maintained which 
maps pthread_key_t instances to an int "key_id" that is used for the PyThread 
API.  Each function needs to map a given key_id to the actual pthread_key_t.  
This adds a little bit of overhead of course, but in typical usage I don't 
think there are many entries in this list to loop over.

This also reverts the change from #22206 which is no longer relevant.  No 
synchronization is provided for PyThread_create_key and PyThread_delete_key, 
but in typical usage that would be the user's responsibility anyways.  
Otherwise every PyThread_*_key function would have to have a mutex around it.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue25658] PyThread assumes pthread_key_t is an integer, which is against POSIX

[R. David Murray]

Changes by R. David Murray :


--
hgrepos:  -352

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue25658] PyThread assumes pthread_key_t is an integer, which is against POSIX

[Erik Bray]

Erik Bray added the comment:

FWIW I've created an initial patch for this.  Seems to work fine, but it's a 
bit of a mess and I have a few small implementation concerns.  I'll try to get 
it cleaned up sometime in the next few days and I'll post it.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue25658] PyThread assumes pthread_key_t is an integer, which is against POSIX

[Erik Bray]

Erik Bray added the comment:

The good news about this (in the pthread case) is that it does not need to be 
seen as some workaround for unusual platforms, but rather making the existing 
code more POSIX-compliant (and hence correct).

The Win32 side I'm a little less worried about because the TLS key is 
documented to be a DWORD, and so the existing casting to int should still work 
fine most of the time.  However, the docs for TlsAlloc() 
(https://msdn.microsoft.com/en-us/library/windows/desktop/ms686801(v=vs.85).aspx)
 do state:

> The value of the TLS index should be treated as an opaque value; do not 
> assume that it is an index into a zero-based array.

which again makes #22206 suspect :(

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue25658] PyThread assumes pthread_key_t is an integer, which is against POSIX

[R. David Murray]

R. David Murray added the comment:

I'd say that sounds reasonable, but most likely it will only be someone working 
with one of the impacted platforms who will have the motivation to come up with 
a patch.  Especially since neither of the impacted platforms is current 
formally supported (meaning, we don't yet have anyone on the core team working 
with those platforms).

We have, it seems, arrived at the time that MvL foresaw :)

--
hgrepos: +352
nosy: +r.david.murray
versions: +Python 3.6 -Python 3.5

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue25658] PyThread assumes pthread_key_t is an integer, which is against POSIX

[Erik Bray]

Erik Bray added the comment:

(Of course, maintaining such a list might take some care, but only when 
creating and deleting keys--it wouldn't add any overhead to using them to 
get/set values.)

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue25658] PyThread assumes pthread_key_t is an integer, which is against POSIX

[Erik Bray]

Erik Bray added the comment:

I'm not really sure what "long" has to do with it...

The problem is that the PyThread API uses ints to represent TLS keys, and has 
for about as long as it's existed (maybe what you meant by "long").  But when 
support for native TLS was added (#9786 for pthread, sometime earlier for 
Windows) , the faulty assumption as made in several places that this API (i.e. 
the type of key is "int") should always map perfectly onto native APIs, and it 
doesn't.

There are several places for example where an int key is passed to 
pthread_getspecific and pthread_setspecific 
(http://pubs.opengroup.org/onlinepubs/009695399/functions/pthread_getspecific.html).
  On Linux the compiler happens to allow this because pthread_key_t is defined 
as "unsigned int"  So yeah there's an implicit cast, but since it counts up 
from zero it usually works.  Likewise TlsAlloc on Windows expects the key to be 
a DWORD but the compiler will accept an int.

This is really an unsafe assumption though, especially when PyThread_create_key 
casts the key to an int, and later reuses the (possibly not safely cast) key 
with PyThread_delete_key/get_key_value/set_key_value).  This was brought up at 
the time, where MvL wrote:

> In principle, this is really difficult to get right. AFAICT,
pthread_key_t doesn't even have to be an integral type, and even
if it is, it may well become -1. However, we can probably worry
about this when a system comes along where this implementation
breaks.

One possible workaround without changing the existing API would be this:  Each 
native support wrapper should also provide a *safe* mapping between its native 
key types and ints, to support the PyThread API.

For example, the pthread interface could maintain a linked list or an even an 
array of pthread_key_t pointers, and use the int "key" as the index into that 
list.  If I understand correctly this should be basically harmless since the 
same key (and hence key -> native-key mapping) can be shared across threads.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue25658] PyThread assumes pthread_key_t is an integer, which is against POSIX

[STINNER Victor]

STINNER Victor added the comment:

What do you suggest? Python C code bases uses "long" everywhere.

--
nosy: +haypo

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue25658] PyThread assumes pthread_key_t is an integer, which is against POSIX

[Erik Bray]

Erik Bray added the comment:

I agree--this has the same problem on Cygwin, where pthread_key_t is not just a 
typedef'd integer (in fact it's a pointer to an instance of a class).

Anyways as Ed wrote above POSIX says this is supposed to be an opaque type and 
there's no reason to assume it's an integer. Linux could change its definition 
tomorrow and we'd have no one to blame but ourselves if it breaks Python.

If it's too hard to change the Python API, at the very least the change in 
#22206 should be reverted or reworked somehow, because there's no reason to 
assume that pthread_key_t can even be compared safely to an integer, much less 
that it would be less than INT_MAX.

--
nosy: +erik.bray

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue25658] PyThread assumes pthread_key_t is an integer, which is against POSIX

[Ed Schouten]

New submission from Ed Schouten:

While trying to port Python over to a new platform (CloudABI), I noticed a 
couple of compiler errors in PyThread_create_key(), PyThread_delete_key(), 
PyThread_delete_key_value() and PyThread_set_key_value() caused by fact that 
pthread_key_t is converted to an integer (and vice versa)

POSIX doesn't seem to require that pthread_key_t is an integer or any other 
arithmetic type:

http://pubs.opengroup.org/onlinepubs/9699919799/basedefs/sys_types.h.html

Old revisions of the standard did require pthread_t to be an arithmetic type, 
but this requirement was dropped later on.

In my opinion we should strongly consider changing the API, so that we can 
treat the key created by pthread_key_create() or returned by TlsAlloc() as an 
opaque type.

--
components: Interpreter Core
messages: 254846
nosy: EdSchouten
priority: normal
severity: normal
status: open
title: PyThread assumes pthread_key_t is an integer, which is against POSIX
versions: Python 3.5

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com