Packaged Python doesn't compile extensions with thread-local "errno"

[jcea]

pkgsrc 2019Q1.

I have spend quite a few hours triaging this.

Packaged python, when installing new C-based python packages using "pip install", doesn't use "thread local" "errno". Result: erratic behavior in multithread applications. I have solved it manually adding "-D_REENTRANT" when compiling new extensions to be used in multithread python code.

The relevant code lives in "/usr/include/errno.h":

#if defined(_REENTRANT) || defined(_TS_ERRNO) || _POSIX_C_SOURCE - 0 >= 199506L
extern int *___errno();
#define errno (*(___errno()))
#else
extern int errno;
/* ANSI C++ requires that errno be a macro */
#if __cplusplus >= 199711L
#define errno errno
#endif
#endif  /* defined(_REENTRANT) || defined(_TS_ERRNO) */

Of course, the right way to do this would be via "_POSIX_C_SOURCE", most probably.

STEPS to reproduce:

1. Create a new directory. "cd" into it.

2. Create a file named "test.c" with this content, extracted from https://stackoverflow.com/a/52344223/322220:

#include <stdio.h>                                                                                                                                             
#include <pthread.h>                                                                                                                                           
#include <errno.h>                                                                                                                                             
#define NTHREADS 5                                                                                                                                             
void *thread_function(void *);                                                                                                                                 

int                                                                                                                                                            
main()                                                                                                                                                         
{                                                                                                                                                              
   pthread_t thread_id[NTHREADS];                                                                                                                              
   int i, j;                                                                                                                                                   

   for(i=0; i < NTHREADS; i++)                                                                                                                                 
   {
      pthread_create( &thread_id[i], NULL, thread_function, NULL );                                                                                            
   }                                                                                                                                                           

   for(j=0; j < NTHREADS; j++)                                                                                                                                 
   {                                                                                                                                                           
      pthread_join( thread_id[j], NULL);                                                                                                                       
   }                                                                                                                                                           
   return 0;                                                                                                                                                   
}                                                                                                                                                              

void *thread_function(void *dummyPtr)                                                                                                                          
{                                                                                                                                                              
   printf("Thread number %ld addr(errno):%p\n", pthread_self(), &errno);                                                                                       
}

3. Create a file "setup.py" with this content:

from setuptools import Extension
from setuptools import setup

# Compile the module with defaults.
ext_modules = [Extension(
    name='test_default',
    sources=['test.c'],
)]

setup(
    name = 'test_default',
    ext_modules = ext_modules,
)

# Compile the same module with an explicit '-D_REENTRANT'.
ext_modules = [Extension(
    name='test_reentrant',
    sources=['test.c'],
    extra_compile_args=['-D_REENTRANT'],
)]

setup(
    name = 'test_reentrant',
    ext_modules = ext_modules,
)

4. Create a "Makefile" with the following content. Remember that each indented line MUST start with a tab, even the empty lines:

all:
        @python3.7 setup.py build
        
        @echo
        
        @echo "We show 'errno' addresses by thread with default compilation"
        @gcc build/lib.solaris-2.11-i86pc.64bit-3.7/test_default.so
        @LD_LIBRARY_PATH=./build/lib.solaris-2.11-i86pc.64bit-3.7/ ./a.out
        
        @echo
        
        @echo "We show 'errno' addresses by thread compiling with an explicit '-D_REENTRANT'"
        @gcc build/lib.solaris-2.11-i86pc.64bit-3.7/test_reentrant.so
        @LD_LIBRARY_PATH=./build/lib.solaris-2.11-i86pc.64bit-3.7/ ./a.out

5. Now, do "make".

6. You will see something similar to this:

running build
running build_ext
running build
running build_ext

We show 'errno' addresses by thread with default compilation
Thread number 4 addr(errno):fffffc7fef312e98
Thread number 6 addr(errno):fffffc7fef312e98
Thread number 5 addr(errno):fffffc7fef312e98
Thread number 3 addr(errno):fffffc7fef312e98
Thread number 2 addr(errno):fffffc7fef312e98

We show 'errno' addresses by thread compiling with an explicit '-D_REENTRANT'
Thread number 2 addr(errno):fffffc7fee7b034c
Thread number 6 addr(errno):fffffc7fee7b234c
Thread number 5 addr(errno):fffffc7fee7b1b4c
Thread number 3 addr(errno):fffffc7fee7b0b4c
Thread number 4 addr(errno):fffffc7fee7b134c

Here we are showing the address of "errno" with multiple threads. You can see that using -D_REENTRANT, the addresses are different per thread, good. Nevertheless, compiling with the default python environment, "errno" address is the same for all threads.

So, my conclusion is that compilation parameter in the packaged Python are wrong and they will break extension modules with weird symptoms when running in a multithreaded python process.

This is a serious bug, probably affecting plenty of old pkgsrc releases, including LTS.

[jperkin]

While unfortunate, I'm not convinced this is a bug. The illumos compilation environment is set up so that users are expected to add -D_REENTRANT when targetting a multithreaded environment. To quote the standard section of text that appears in various manual pages on this:

       When compiling multithreaded applications, the _REENTRANT flag must be
       defined on the compile line.  This flag should only be used in
       multithreaded applications.

Ordinarily this is hidden behind the -pthread compiler flag that is used whenever multithreaded applications are built, for example:

$ /opt/local/gcc7/bin/gcc -dM -E - </dev/null | grep _REENTRANT
$

$ /opt/local/gcc7/bin/gcc -dM -pthread -E - </dev/null | grep _REENTRANT
#define _REENTRANT 1
$

There is nothing in your example that I can see that would allow python to determine that the program it is compiling will be multithreaded and thus needs to add -pthread to the build. Whether adding -pthread globally is still considered to be a drawback I do not know.

As for adding _POSIX_C_SOURCE by default, that comes with its own issues. The illumos headers are strict about how the POSIX standards and C standards interact, and builds fail if they do not match correctly. For example, if you attempt to use _POSIX_C_SOURCE=199506L in the default compilation environment that is now C99 the build will fail. Conversely, if you attempt to use _POSIX_C_SOURCE=200112L or newer but some part of the build specifies pre-C99 flags, the build will fail. These also interact badly with _XOPEN_SOURCE that users from other platforms sprinkle liberally all over their code to get it to compile without knowing it will cause problems on illumos.

So while it is undoubtedly frustrating, I think the only path forwards that avoids such issues and perhaps harder to diagnose problems where threads are enabled when unused is to continue adding -D_REENTRANT or compiling with -pthread when you know that the target application is going to use threads.

Note that Solaris appears to have resolved this issue in their 11.4 release, it may be that we can do something similar: https://blogs.oracle.com/solaris/mtreentrant-v2

I'll leave this issue open for a bit in case anyone from the OS side wants to provide some context or perhaps some better suggestions for a way forward.

[jlevon]

@jperkin FYI the _POSIX_C_SOURCE pedantry was fixed last year via:

9812 headers should be free of SUS compiler tyranny

It's been a while since I was in Python land, but IIRC there is a Pyconfig.h header that gets included when compiling Python extensions. That is supposed to reflect the default compilation environment to use. Is it possible that we can define _REENTRANT (or POSIX_C_SOURCE+EXTENSIONS?) there?

(It'd be awesome to get rid of the need for it altogether, but that's an unknown amount of possibly tricky work.)

[jcea]

While unfortunate, I'm not convinced this is a bug. The illumos compilation environment is set up so that users are expected to add -D_REENTRANT when targetting a multithreaded environment. To quote the standard section of text that appears in various manual pages on this:

I know about "-mt", I have been fighting this kind of issues since 1996 :-).

But Python is special. I elaborate.

When you compile Python, it keep around compilation details about how to compile future external modules to be "consistent" and "compatible" with the installed python interpreter.

What we have here is a Python interpreter compiled with thread support (good!), but it doesn't know how to compile external modules correctly to be compatible with threads. It is not providing "-D_REENTRANT" or whatever is needed to enable that support.

I am not requesting to add "-pthread" or whatever by default, system wide. I am saying that provided python supports threads, but it can not install extensions supporting threads correctly. That is why I am filing this bug as "pkgsrc" and not as "illumos".

There is something missing in the way we compile python that generates a thread-compatible interpreter UNABLE to compile extensions thread-compatible.

There is nothing in your example that I can see that would allow python to determine that the program it is compiling will be multithreaded and thus needs to add -pthread to the build. Whether adding -pthread globally is still considered to be a drawback I do not know.

Our Python is thread compatible and it MUST BE ABLE to compile extensions as thread-compatible. That is the point of this bug report.

Basically, if python is compiled with thread support, it must compile&install modules as thread-safe, even if a particular python program uses no threads at all.

As for adding _POSIX_C_SOURCE by default, that comes with its own issues.

Yes, I suffer this pain frequently :).

So while it is undoubtedly frustrating, I think the only path forwards that avoids such issues and perhaps harder to diagnose problems where threads are enabled when unused is to continue adding -D_REENTRANT or compiling with -pthread when you know that the target application is going to use threads.

My point is that python takes care of this, when done correctly.

According to "/opt/local/lib/python3.7/config-3.7/Makefile", python is compiled with "--with threads" but I don't see any thread-related compilation switch in "CONFIGURE_CFLAGS" or related variables. I don't know what kind of magic is involved in pkgsrc compilation of python to provide a thread-compatible interpreter, but it is not reaching the "and now python can compile extension modules compatible with main interpreter configuration".

What kind of trick is done in pkgsrc to generate a thread-compatible interpreter. Maybe it should be replaced by a simple "a --with-threads in Solaris/derivatives must add -pthreads to the compiler flags". Such a python interpreter would be able to compile extensions with the right flags.

Note that Solaris appears to have resolved this issue in their 11.4 release, it may be that we can do something similar: https://blogs.oracle.com/solaris/mtreentrant-v2

I will check it out, thanks for the point. Nevertheless this bug is not about system wide thread safe compilation, but specific to the way we compile python.

In any case, solving this system wide would be very nice. It is a frequent compatibility bumper and if "real" Solaris have done it, we should do too :).

I'll leave this issue open for a bit in case anyone from the OS side wants to provide some context or perhaps some better suggestions for a way forward.

Please, reconsider. This is a real bug not requiring system-wide action. it "only" requires compile python in the right way :).

[jperkin]

To be clear I was never advocating this be done system-wide, only python-wide. If you are certain that adding -D_REENTRANT to the default python build flags will not impact any python software then I'm certainly happy to do that and it's easy enough for me to do, I just do not have the confidence that you have that it won't cause issues in ways that might be incredibly difficult to diagnose.

@jlevon we still build on older platforms so the feature tests still apply unfortunately.

[jcea]

Sorry for leaving this to stale. I have been quite busy.

@jperkin , there are some news about this. "lmdb" and Python will be released with fixes, but in the meantime (and for old but supported python releases), you "should" (asked respectfully) add "-D_REENTRANT" when compiling Python interpreter.

Some details:

jnwatson/py-lmdb#213
jnwatson/py-lmdb#214

python/cpython#16446
https://bugs.python.org/issue38301

[jcea]

I don't think -D_REENTRANT has a negative impact in other python modules, because any module doing #include "python.h" will get the symbol defined anyway. The problem is with modules not including the header in every single file or when including it "too late".