BUG: threads.h existence test requires GLIBC > 2.12. #18180

pearu · 2021-01-17T19:00:47Z

Closes #18179

pearu · 2021-01-17T19:03:42Z

CC: @mattip @melissawm

mattip · 2021-01-17T19:18:13Z

CentOS7 has glibc 2.17, Ubuntu 18.04 has 2.27. Is one of our builds on Ubuntu 18.04 so we can test this?

pearu · 2021-01-17T19:31:51Z

I am on Ubuntu 18.04.

mattip · 2021-01-17T20:02:48Z

I thought one or more of our CI builds use ubuntu 18.04, so how do tests pass?

pearu · 2021-01-17T20:19:22Z

I thought one or more of our CI builds use ubuntu 18.04, so how do tests pass?

Can you point to a CI build using ubuntu 18.04 that runs f2py tests?

mattip · 2021-01-18T07:56:56Z

I thought the f2py tests are part of the test suite. We should be running them on at least one of the builds. Since the tests pass without this PR, I guess they are not running. We go through the trouble of installing a gfortran compiler and libraries, so we should run the tests. Maybe as part of this PR you could add whatever is needed to make them run?

pearu · 2021-01-18T08:40:10Z

The f2py tests are running. There are other differences between local and CI ubuntu boxes that may affect the build/test results:

local: 18.04.4, CI: 18.04.5
I using conda environment to build and test numpy.f2py, CI is installing numpy to system
gcc version - local: 9.3.0, CI: 7.5.0

pearu · 2021-01-18T09:13:57Z

@mattip I found the explanation of why the callback tests pass in CI but not locally: when using ubuntu gcc 7.5.0 compiler (CI), __STDC_NO_THREADS__ is defined but when using conda gcc 9.3.0 compiler (local), __STDC_NO_THREADS__ is not defined. Hence, gcc 7.5.0 does not try to include threads.h while gcc 9.3.0 does.

mattip · 2021-01-18T09:33:25Z

Is the glibc version the standard way to detect threads.h ?

This discussion suggests to explicitly #include <features.h> to get the __STDC_NO_THREADS__ definition.

pearu · 2021-01-18T10:16:49Z

Is the glibc version the standard way to detect threads.h ?

It is known that threads.h was introduced in glibc 2.28.

This discussion suggests to explicitly #include <features.h> to get the __STDC_NO_THREADS__ definition.

__STDC_NO_THREADS__ is specific to C11 only. The conda gcc 9.3 uses -std=c++17 by default and hence __STDC_NO_THREADS__ is not defined. The ubuntu gcc 7.5 uses -std=c++11 by default.
So, we should not include features.h just to make C11 specific __STDC_NO_THREADS__ available when for non-C11 cases it should not be defined anyway.
Notice that the thread is about ICC compiler and from pre glibc 2.28 era and hence not directly applicable here, IMHO.

Another approach would be to use __STDC_VERSION__ == 201112L (notice the change from >= to ==) but that may be too restrictive and does not protect us for glibc < 2.28 cases.

pearu · 2021-01-18T10:49:18Z

On the second thought, since both __STDC_NO_THREADS__ and threads.h are C11-only specific, we should use __STDC_VERSION__ == 201112L anyway when trying to include threads.h.

The glibc >= 2.28 constraint might be redundant though: when using gcc -std=c11 ..., __STDC_VERSION__ is 201710 when using gcc 9 and 201112 when using gcc7.

What do you think, @mattip ?

mattip · 2021-01-18T10:53:20Z

we should use __STDC_VERSION__ == 201112L anyway when trying to include threads.h.

This sounds like the most reasonable choice, without the glibc version check

pearu · 2021-01-18T12:34:46Z

@mattip please review.

numpy/f2py/cfuncs.py

mattip

Would be nice to add a comment, it took quite a bit of detective work to figure this one out and my future self will forget why this is here. Otherwise LGTM

pearu · 2021-01-18T14:11:59Z

Don't merge until #18180 (comment) is resolved.

eric-wieser · 2021-01-18T14:16:30Z

It's not clear from the pr title - but am I right in thinking this is actually just working around a bug in the Intel compiler where the appropriate macro doesn't end up defined? Or are we seeing this problem with gcc alone?

pearu · 2021-01-18T14:30:16Z

It's not clear from the pr title - but am I right in thinking this is actually just working around a bug in the Intel compiler where the appropriate macro doesn't end up defined? Or are we seeing this problem with gcc alone?

The problem is with gcc (I don't have icc installed to test this atm).

IIUC, gcc 9 is C17 (__STDC_VERSION__ == 201710) and it does not define __STDC_NO_THREADS__.

eric-wieser · 2021-01-18T14:33:49Z

My reading of the Intel thread is that glibc itself is responsible for providing __STDC_NO_THREADS__, and that it uses a special gcc hook to inject a header into all source files that does so. The Intel issue is that the hook doesn't exist in icc - but if you're using gcc, that suggests that something else is going wrong with the hook.

Apparently more work is needed.

pearu · 2021-01-19T10:33:12Z

Consider the following test program glibc-test.c

#include <stdio.h>
// #include <features.h>

int main() {
  printf("__STDC_VERSION__ = %ld\n", __STDC_VERSION__);
#if defined(__STDC_NO_THREADS__)
  printf("__STDC_NO_THREADS__ = %d\n", __STDC_NO_THREADS__);
#else
  printf("__STDC_NO_THREADS__ not defined\n");
#endif
#if defined(__GLIBC__)
  printf("GLIBC version = %d.%d\n", __GLIBC__, __GLIBC_MINOR__);
#else
  printf("__GLIBC__ not defined\n");
#endif
#if defined(__GNUC__)
  printf("GNUC version %d.%d\n", __GNUC__, __GNUC_MINOR__);
#else
  printf("__GNUC__ not defined\n");
#endif
  return 0;
}

that has the following outputs (including features.h does not change the output here) in ubuntu 18.04.4 box:

$ which gcc
/usr/bin/gcc
$ gcc --version
gcc (Ubuntu 7.5.0-3ubuntu1~18.04) 7.5.0
$ gcc glibc-test.c
$ ./a.out 
__STDC_VERSION__ = 201112
__STDC_NO_THREADS__ = 1
GLIBC version = 2.27
GNUC version 7.5

$ conda activate numpy-dev
$ which gcc
/home/pearu/miniconda3/envs/numpy-dev/bin/gcc
$ gcc --version
gcc (crosstool-NG 1.24.0.133_b0863d8_dirty) 9.3.0
$ gcc glibc-test.c
$ ./a.out 
__STDC_VERSION__ = 201710
__STDC_NO_THREADS__ not defined
GLIBC version = 2.12
GNUC version 9.3

$ gcc glibc-test.c -std=c11
$ ./a.out 
__STDC_VERSION__ = 201112
__STDC_NO_THREADS__ not defined
GLIBC version = 2.12
GNUC version 9.3

Notice that the conda gcc 9.3 does not define __STDC_NO_THREADS__ neither in C17 nor in C11 mode.

A similar issue has been reported in https://stackoverflow.com/questions/61887795/gcc-is-non-conforming for GCC 9.2.0 (Windows 10 x64).

Then according to https://gcc.gnu.org/legacy-ml/gcc-help/2019-06/msg00063.html, __STDC_NO_THREADS__ is defined in stdc-predef.h that gcc should implicitly pre-include.

Digging further... it turns out that stdc-predef.h is included in conda-forge package sysroot_linux-64 version 2.17 but not in sysroot_linux-64 version 2.12 (that is installed to my numpy-dev environment).

So, the problem boils down to using glibc 2.12 (or older) when __STDC_NO_THREADS__ is not defined, and it seems to me that checking for GLIBC version is inevitable.

My suggestion is to use:

#elif defined(__STDC_VERSION__) \\
      && (__STDC_VERSION__ >= 201112L) \\
      && !defined(__STDC_NO_THREADS__) \\
      && (!defined(__GLIBC__) || __GLIBC_MINOR__>12)
// see https://github.com/numpy/numpy/pull/18180 discussion for details
#include <threads.h>
#define F2PY_THREAD_LOCAL_DECL thread_local

@mattip @eric-wieser what do you think?

eric-wieser · 2021-01-19T10:43:41Z

Are you certain that 2.12 is the last version before __STDC_NO_THREADS__ was added, or is that simply the last version you tested without it?

Either way, that looks reasonable - though you should be checking __GLIBC__ > 2 || (__GLIBC__ == 2 && __GLIBC_MINOR__ > 12) for forwards-compatibility.

I suppose the other option is to just discover if threads.h is available as part of config_test.

pearu · 2021-01-19T10:56:36Z

Are you certain that 2.12 is the last version before __STDC_NO_THREADS__ was added, or is that simply the last version you tested without it?

2.12 is the version I have tested. However, according to https://lists.gnu.org/archive/html/commit-hurd/2012-07/msg00180.html, __STDC_NO_THREADS__ was defined in a maintenance release of glibc 2.12 (see commit 6d74dd09d29f77ac2b22410f45687def349ba3da)

Either way, that looks reasonable - though you should be checking __GLIBC__ > 2 || (__GLIBC__ == 2 && __GLIBC_MINOR__ > 12) for forwards-compatibility.

Makes sense.

I suppose the other option is to just discover if threads.h is available as part of config_test.

For f2py, I am uncertain that this would be worth it. In fact, the sources of f2py generated extension modules must be self-contained (modulo numpy and Python) so that the result of discovering threads.h may be invalid when compiling the (possibly relocated) extension modules.

eric-wieser · 2021-01-19T11:12:23Z

Huh, that commit is authored by someone (@jsm28) I've seen doing something completely different in another repository - open source is a small world! Referencing the mailing list post in a comment might be a good idea

numpy/f2py/cfuncs.py

eric-wieser

Thanks for the thorough investigation!

mattip · 2021-01-19T18:53:05Z

Thanks @pearu

* defined(__STDC_NO_THREADS__) can be trusted only when using glibc > 2.12 * Add __GLIBC__ defined check.

dongkeun-oh

Please update a little more against the same trouble in MINGW32.
The 548th line has to be replaced with
#if defined(_MSC_VER)||defined(__MINGW32__).

mattip · 2021-05-05T03:29:46Z

@dongkeun-oh this was merged a few months ago. Please open a new PR with the requested change.

Begasus · 2021-05-24T16:03:45Z

numpy/f2py/cfuncs.py

@@ -549,7 +549,12 @@
 #define F2PY_THREAD_LOCAL_DECL __declspec(thread)
 #elif defined(__STDC_VERSION__) \\
      && (__STDC_VERSION__ >= 201112L) \\
-      && !defined(__STDC_NO_THREADS__)
+      && !defined(__STDC_NO_THREADS__) \\
+      && (!defined(__GLIBC__) || __GLIBC__ > 2 || (__GLIBC__ == 2 && __GLIBC_MINOR__ > 12))


This would fix the #include <threads.h> on Haiku too (ticket has been filed for this upstream: https://dev.haiku-os.org/ticket/16965 )

- && (!defined(__GLIBC__) || __GLIBC__ > 2 || (__GLIBC__ == 2 && __GLIBC_MINOR__ > 12)) + && (!defined(__GLIBC__) || __GLIBC__ > 2 || (__GLIBC__ == 2 && __GLIBC_MINOR__ > 12)) \\ + && !defined(__HAIKU__)

BUG: Require glibc 2.28+ for threads.h. Closes numpy#18179

ce91dcb

github-actions bot added the 00 - Bug label Jan 17, 2021

pearu changed the title ~~BUG: Require glibc 2.28+ for threads.h.~~ BUG: threads.h inclusion requires glibc 2.28+ Jan 18, 2021

BUG: Require C11 for threads.h. Closes numpy#18179

e9eecc4

pearu changed the title ~~BUG: threads.h inclusion requires glibc 2.28+~~ BUG: threads.h inclusion requires C11 Jan 18, 2021

mattip reviewed Jan 18, 2021

View reviewed changes

numpy/f2py/cfuncs.py Outdated Show resolved Hide resolved

mattip previously approved these changes Jan 18, 2021

View reviewed changes

Check for glibc > 2.12 when __STDC_NO_THREADS__ is not defined.

072ed01

pearu changed the title ~~BUG: threads.h inclusion requires C11~~ BUG: threads.h existence test requires GLIBC > 2.12. Jan 19, 2021

defined(__STDC_NO_THREADS__) can be trusted only when using glibc > 2.12

dae55c3

pearu requested review from eric-wieser and mattip January 19, 2021 11:34

eric-wieser reviewed Jan 19, 2021

View reviewed changes

numpy/f2py/cfuncs.py Outdated Show resolved Hide resolved

Add __GLIBC__ defined check.

5fe6159

pearu added 58 - Ready for review component: numpy.f2py labels Jan 19, 2021

pearu self-assigned this Jan 19, 2021

pearu requested a review from eric-wieser January 19, 2021 12:45

eric-wieser approved these changes Jan 19, 2021

View reviewed changes

mattip merged commit cafda24 into numpy:master Jan 19, 2021

pearu deleted the pearu/18179 branch January 19, 2021 19:44

charris pushed a commit to charris/numpy that referenced this pull request Feb 7, 2021

BUG: threads.h existence test requires GLIBC > 2.12. (numpy#18180)

45a6ec0

* defined(__STDC_NO_THREADS__) can be trusted only when using glibc > 2.12 * Add __GLIBC__ defined check.

charris mentioned this pull request Feb 7, 2021

BUG: threads.h existence test requires GLIBC > 2.12. #18357

Merged

dongkeun-oh reviewed May 4, 2021

View reviewed changes

dongkeun-oh mentioned this pull request May 5, 2021

BUG : for MINGW, threads.h existence test requires GLIBC > 2.12 #18910

Merged

charris removed the 58 - Ready for review label May 5, 2021

charris mentioned this pull request May 5, 2021

BUG: for MINGW, threads.h existence test requires GLIBC > 2.12 #18925

Merged

Begasus reviewed May 24, 2021

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

BUG: threads.h existence test requires GLIBC > 2.12. #18180

BUG: threads.h existence test requires GLIBC > 2.12. #18180

pearu commented Jan 17, 2021

pearu commented Jan 17, 2021

mattip commented Jan 17, 2021

pearu commented Jan 17, 2021 •

edited

mattip commented Jan 17, 2021

pearu commented Jan 17, 2021

mattip commented Jan 18, 2021

pearu commented Jan 18, 2021

pearu commented Jan 18, 2021

mattip commented Jan 18, 2021

pearu commented Jan 18, 2021

pearu commented Jan 18, 2021

mattip commented Jan 18, 2021

pearu commented Jan 18, 2021

mattip left a comment

pearu commented Jan 18, 2021

eric-wieser commented Jan 18, 2021

pearu commented Jan 18, 2021

eric-wieser commented Jan 18, 2021 •

edited

pearu commented Jan 19, 2021 •

edited by eric-wieser

eric-wieser commented Jan 19, 2021 •

edited

pearu commented Jan 19, 2021

eric-wieser commented Jan 19, 2021

eric-wieser left a comment

mattip commented Jan 19, 2021

dongkeun-oh left a comment

mattip commented May 5, 2021

Begasus May 24, 2021

BUG: threads.h existence test requires GLIBC > 2.12. #18180

BUG: threads.h existence test requires GLIBC > 2.12. #18180

Conversation

pearu commented Jan 17, 2021

pearu commented Jan 17, 2021

mattip commented Jan 17, 2021

pearu commented Jan 17, 2021 • edited

mattip commented Jan 17, 2021

pearu commented Jan 17, 2021

mattip commented Jan 18, 2021

pearu commented Jan 18, 2021

pearu commented Jan 18, 2021

mattip commented Jan 18, 2021

pearu commented Jan 18, 2021

pearu commented Jan 18, 2021

mattip commented Jan 18, 2021

pearu commented Jan 18, 2021

mattip left a comment

Choose a reason for hiding this comment

pearu commented Jan 18, 2021

eric-wieser commented Jan 18, 2021

pearu commented Jan 18, 2021

eric-wieser commented Jan 18, 2021 • edited

pearu commented Jan 19, 2021 • edited by eric-wieser

eric-wieser commented Jan 19, 2021 • edited

pearu commented Jan 19, 2021

eric-wieser commented Jan 19, 2021

eric-wieser left a comment

Choose a reason for hiding this comment

mattip commented Jan 19, 2021

dongkeun-oh left a comment

Choose a reason for hiding this comment

mattip commented May 5, 2021

Begasus May 24, 2021

Choose a reason for hiding this comment

pearu commented Jan 17, 2021 •

edited

eric-wieser commented Jan 18, 2021 •

edited

pearu commented Jan 19, 2021 •

edited by eric-wieser

eric-wieser commented Jan 19, 2021 •

edited