search

sagemath / cysignals

cysignals: interrupt and signal handling for Cython. Source repository for https://pypi.org/project/cysignals/
GNU Lesser General Public License v3.0
44 stars 23 forks source link

Compilation with fopenmp #128

Closed kliem closed 3 years ago

kliem commented 3 years ago

This is supposed to fix #122. I'm not entirely sure how to test it though.

embray commented 3 years ago

How do you reproduce the problem in the first place? Do you just add CFLAGS=-fopenmp when compiling?

kliem commented 3 years ago

How do you reproduce the problem in the first place? Do you just add CFLAGS=-fopenmp when compiling?

I don't know exactly. It happens with sage's kucalc builtbot:

https://trac.sagemath.org/ticket/31245#comment:11

Probably I should try reproducing this with tox/github actions.

embray commented 3 years ago

I see, so this doesn't really impact cysignals itself, but rather using cysignals macros in third-party code that is being compiled with OpenMP support. It also seems to be compiler-specific as the relevant issue appears to be fixed in GCC (though I admit I don't fully understand what's being fixed here).

One concern about this is that by having configure-time checks it will disable use of _Atomic for all code using cysignals, with or without OpenMP. I'm not sure there's a good workaround to that...

embray commented 3 years ago

Or is that what the #if _OPENMP is supposed to address?

Answer: Yes, that seems to take care of it.

Okay, looks good to me then.

kliem commented 3 years ago

Thanks for reviewing.

I saw that link regarding GCC earlier and forgot about it again. Yes it seems to be fixed in GCC 5.0. However, sage allows GCC starting with 4.8. That might have been the problem with the kucalc buildbot.