Skip to content

Fix the atomicity of cy_atomic_int #230

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
Jul 22, 2025
Merged

Fix the atomicity of cy_atomic_int #230

merged 2 commits into from
Jul 22, 2025

Conversation

ptrrsn
Copy link
Contributor

@ptrrsn ptrrsn commented Jul 20, 2025

When building sage, meson.build incorrectly sets all CYSIGNALS_* macros to 0 because of lacks of main function, which leads to a non-atomic declaration of cy_atomic_int.

Moreover, the cy_atomic_int typedefs in struct_signals.h was structured in a way that when it is compiled by a C++ compiler in an environment with a C compiler that supports _Atomic, it would never use std::atomic, which is incorrect and might cause compilation problem when the C++ compiler does not support _Atomic. This problem was hidden by the incorrect configuration of meson.build above but got surfaced after the meson.build fix.

As a context: LLewark/khoca#3

@orlitzky
Copy link

int main(void) please, for consistency across the various C standards

@ptrrsn
Copy link
Contributor Author

ptrrsn commented Jul 21, 2025

Thank you for the review. I changed int main() to int main(void) for the C atomicity checks.

@dimpase dimpase merged commit 8f6de54 into sagemath:main Jul 22, 2025
23 checks passed
@dimpase
Copy link
Member

dimpase commented Jul 22, 2025

I'll draft a new release later today

@ptrrsn
Copy link
Contributor Author

ptrrsn commented Jul 22, 2025

Thank you for the reviews and merge.

@dimpase
Copy link
Member

dimpase commented Jul 23, 2025

@ptrrsn I've put 1.12.4 with this fix on PyPI (so pip install cysignals should get it)

@ptrrsn
Copy link
Contributor Author

ptrrsn commented Jul 23, 2025

Thank you, I can see it now. cc @soehms

@soehms
Copy link
Member

soehms commented Jul 25, 2025

Thank you, I can see it now. cc @soehms

Thank you! For the record: Before applying 1.12.4 I got at least 2 failures with ./sage -t --global-iterations 20 src/sage/knots/link.py on each run. After upgrading cysignals, reinstalling khoca and cypari2 an rebuilding Sage I don't get failures any more on several runs.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants