define ATOMIC macros as 0 per default

[kliem]

I made a mistake in #128.

The state of CYSIGNALS_C_ATOMIC and friends is 1 or undefined.

This has the unfortunate consequence that

define CYSIGNALS_C_ATOMIC CYSIGNALS_C_ATOMIC_WITH_OPENMP

will not define CYSIGNALS_C_ATOMIC just as CYSIGNALS_C_ATOMIC_WITH_OPENMP, but if the later is undefined this will just define CYSIGNALS_C_ATOMIC and then

#if CYSIGNALS_C_ATOMIC

will unexpectedly evaluate to true.

Sorry about the mess.

(What is a greater mess yet is that in principle defining macros in configuration and then undefining them in a configured file seems to not work well. For me src/cysignals/cysignals_config.h.in looks way different then src/cysignals/cysignals_config.h.)

[kliem]

The problem is really that the configured cysignals_config.h.in file looks so much different:

cysignals_config.h.in:

/* An implementation of atomic variables might not be allowed with OpenMP */
#if _OPENMP
#ifndef CYSIGNALS_C_ATOMIC_WITH_OPENMP
#undef CYSIGNALS_C_ATOMIC
#endif
#ifndef CYSIGNALS_CXX_ATOMIC_WITH_OPENMP
#undef CYSIGNALS_CXX_ATOMIC
#endif
#ifndef CYSIGNALS_STD_ATOMIC_WITH_OPENMP
#undef CYSIGNALS_STD_ATOMIC
#endif
#endif

/* Which implementation of atomic variables (if any) to use? */
#ifndef __cplusplus
#undef CYSIGNALS_C_ATOMIC
#else
#undef CYSIGNALS_CXX_ATOMIC
#undef CYSIGNALS_STD_ATOMIC
#endif

cysignals_config.h:

/* An implementation of atomic variables might not be allowed with OpenMP */
#if _OPENMP
#ifndef CYSIGNALS_C_ATOMIC_WITH_OPENMP
#define CYSIGNALS_C_ATOMIC 1
#endif
#ifndef CYSIGNALS_CXX_ATOMIC_WITH_OPENMP
/* #undef CYSIGNALS_CXX_ATOMIC */
#endif
#ifndef CYSIGNALS_STD_ATOMIC_WITH_OPENMP
#define CYSIGNALS_STD_ATOMIC 1
#endif
#endif

/* Which implementation of atomic variables (if any) to use? */
#ifndef __cplusplus
#define CYSIGNALS_C_ATOMIC 1
#else
/* #undef CYSIGNALS_CXX_ATOMIC */
#define CYSIGNALS_STD_ATOMIC 1
#endif

What I didn't know is that AC_DEFINE treats #define the same as #undef.

[kliem]

I'll reopen a correct pull request, once done.

[embray]

I see, thanks for the catch.

[embray]

I'll reopen this to serve as the issue report. You can also do a git push -f to this branch if you have a working fix, rather than open a new PR.

[kliem]

Ok. Whatever works for you. I've been using git push -f a lot already. I'll remark, once I think I'm done.

[embray]

You can also just add commits as you go if you want and they can be squashed when merging :)

[kliem]

I was able to reproduce the original bug of #122 here: https://github.com/kliem/sage/runs/2064520070?check_suite_focus=true

The current patch is now being tested here: https://github.com/kliem/sage/actions/runs/637369408 [Edit: Updated the run 3x]

At least locally I'm able to compile sage with the patched cysignals now.

[embray]

I was able to reproduce the original bug of #122 here: https://github.com/kliem/sage/runs/2064520070?check_suite_focus=true

The current patch is now being tested here: https://github.com/kliem/sage/actions/runs/636120269 [Edit: Updated the run.]

At least locally I'm able to compile sage with the patched cysignals now.

Thanks! Should I wait until this testing is done to merge? FWIW I tested this out a bit just in terms of what configure outputs in different cases, and it looks good now.

Beyond that, this is hard to test in a sensible manner (short of creating compiler shims or something), so it's probably not worth the effort of making a test for as long as it's been manually confirmed to work.

[kliem]

We can and should use https://trac.sagemath.org/ticket/31455 before a release. I already encountered one problem that I don't think is due to myself.

I don't know, if you need to wait on the tests. They mostly test #128 now (at least how I intended it) and not this here anymore. I'm rather confident that the configuration works as expected now.