Remove preliminary_late_includes_cy28 directive by cxzhong · Pull Request #244 · sagemath/cysignals

cxzhong · 2026-04-04T18:24:05Z

Move all static inline functions from macros.h to implementation.c and replace them with #define macros. Since macros are text templates that expand at the call site (where Cython's capsule declarations are available), this eliminates the need for late includes entirely.

The capsule mechanism shares the implementation.c functions across all importing modules without requiring any linking — this works on all platforms (Linux, macOS, Windows).

Changes:

macros.h: Remove all static inline functions; keep only #define macros (sig_on, sig_str, sig_off, sig_check, sig_block, sig_unblock, sig_retry, sig_error)
implementation.c: Add former inline functions as static functions (_sig_on_prejmp, _sig_on_postjmp, sig_off, sig_unblock, sig_retry, sig_error, _set_debug_level)
signals.pxd: Remove directive; add new functions to capsule block
signals.pyx: Remove directive; add new function declarations
tests.pyx: Remove directive
tests_helper.c: Remove direct cysigs reference (not needed in fork child process)

cxzhong · 2026-04-04T18:32:36Z

Now we do not need to use cython0.28 compatible mode for cysignals

Copilot

Pull request overview

This PR removes the preliminary_late_includes_cy28 Cython directive by restructuring cysignals’ C helpers: static inline helper functions are moved out of macros.h into implementation.c, while macros.h is kept as macro-only so expansions occur at the call site where Cython’s imported C-API/capsule symbols are available.

Changes:

Converted macros.h to macro-only helpers and moved former inline implementations into implementation.c.
Extended the Cython-exported C-API surface in signals.pxd/signals.pyx to include the moved helper functions.
Removed preliminary_late_includes_cy28 directives and simplified test helper logic to avoid direct cysigs references.

Reviewed changes

Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.

Show a summary per file

File	Description
src/cysignals/tests.pyx	Removes the `preliminary_late_includes_cy28` directive from test module config.
src/cysignals/tests_helper.c	Removes direct manipulation of `cysigs.block_sigint` in the forked child helper.
src/cysignals/signals.pyx	Adds extern declarations for helper functions now implemented in `implementation.c`; removes the directive.
src/cysignals/signals.pxd	Removes the directive; adds moved helper functions to the module C-API/capsule declarations.
src/cysignals/macros.h	Replaces inline helper functions with macro wrappers and relies on capsule-imported helper symbols.
src/cysignals/implementation.c	Moves helper logic from `macros.h` into this file and adjusts supporting definitions/includes.

Comments suppressed due to low confidence (1)

src/cysignals/macros.h:70

proc_raise is redefined: it is conditionally defined in implementation.c (as a fallback before macros.h is included) and then defined again unconditionally in macros.h. This can trigger macro redefinition warnings (and may fail builds that treat warnings as errors). Consider guarding the definition in macros.h with #ifndef proc_raise (or #undef/centralize the definition) so it is defined exactly once.

/* Send a signal to the calling process. The POSIX raise() function
 * sends a signal to the calling thread, while kill() typically sends
 * a signal to the main thread (although this is not guaranteed by the
 * POSIX standard) */
#if HAVE_KILL
#define proc_raise(sig)  kill(getpid(), sig)
#else
/* On Windows, raise() actually signals the process */
#define proc_raise(sig)  raise(sig)
#endif

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

tornaria · 2026-04-04T20:47:00Z

 static inline void ulong_to_str(unsigned long val, char *str, int base)
 {
-    const char xdigits[16] = "0123456789abcdef";
+    const char xdigits[17] = "0123456789abcdef";


Since it ends with /0. the null hidden char

why?

If you want to write 16bit char, you should write as 'a' 'b' 'c' like this.

No, the null character is only included if there is room in the array. We don't need it here.

but the compiler thinks the char is 17 not 16, so it raises a warning.

See §6.7.8 paragraph 14 in p.126 of https://www.open-std.org/jtc1/sc22/wg14/www/docs/n1256.pdf

An array of character type may be initialized by a character string literal, optionally enclosed in braces. Successive characters of the character string literal (including the terminating null character if there is room or if the array is of unknown size) initialize the elements of the array.

But I see a warning in gcc 15.2. I will put the log later.

[8/15] Compiling C object src/cys...ated_src_cysignals_signals.pyx.c.o In file included from src/cysignals/signals.cpython-314-x86_64-linux-gnu.so.p/src/cysignals/signals.pyx.c:1133: ../../src/cysignals/implementation.c: In function ‘ulong_to_str’: ../../src/cysignals/implementation.c:163:30: warning: initializer-string for array of ‘char’ truncates NUL terminator but destination lacks ‘nonstring’ attribute (17 chars into 16 available) [-Wunterminated-string-initialization] 163 | const char xdigits[16] = "0123456789abcdef"; | ^~~~~~~~~~~~~~~~~~

This warning, seems gcc15.2 thinks it is 17 chars

tornaria · 2026-04-04T20:51:51Z

Can you explain the issue this solves. Is it possible to solve this without repeating code and without macros?

How is it that cdef extern from "macros.h": ... doesn't include macros.h before using the functions defined in there?

tornaria · 2026-04-04T22:00:46Z



-#if __USE_FORTIFY_LEVEL
-#error "cysignals must be compiled without _FORTIFY_SOURCE"


Is it now safe to build with _FORTIFY_SOURCE?

meson directly send the parameter do not use Fortify level. So I think this part is unreachable.

I will test this.

Please leave the test in place. This is there to catch any situation where the file is incorrectly built with fortify as shown in #226. There's no downside.

In addition, I'd suggest not doing unrelated changes in a PR, so it's easier to review. If your goal is to remove preliminary_late_includes_cy28, just stick to that (but also please explain why we would need / want to do that).

OK, I will remove unrelated part.

Please leave the test in place. This is there to catch any situation where the file is incorrectly built with fortify as shown in #226. There's no downside.

In addition, I'd suggest not doing unrelated changes in a PR, so it's easier to review. If your goal is to remove preliminary_late_includes_cy28, just stick to that (but also please explain why we would need / want to do that).

It is in cython/cython#2079, it is a backward compatibility setting, and the author said it will be removed in some version

the author said it is temporary and deprecated set. So I think we should not rely on this

AFAICT the feature is not going away:
cython/cython#2079 (comment)

Until they decide a different way to indicate the cython compiler which files are to be late included, they'll keep the directive.

I don't see a point on making our code uglier, repeat cython code, etc.

See #49 on how this was done before the late include feature (which was added to cython by @jdemeyer for this).

Move all static inline functions from macros.h to implementation.c and replace them with #define macros. Since macros are text templates that expand at the call site (where Cython's capsule declarations are available), this eliminates the need for late includes entirely. The capsule mechanism shares the implementation.c functions across all importing modules without requiring any linking — this works on all platforms (Linux, macOS, Windows). Changes: - macros.h: Remove all static inline functions; keep only #define macros (sig_on, sig_str, sig_off, sig_check, sig_block, sig_unblock, sig_retry, sig_error) - implementation.c: Add former inline functions as static functions (_sig_on_prejmp, _sig_on_postjmp, _sig_off_, _sig_unblock_, _sig_retry_, _sig_error_, _set_debug_level) - signals.pxd: Remove directive; add new functions to capsule block - signals.pyx: Remove directive; add new function declarations - tests.pyx: Remove directive - tests_helper.c: Remove direct cysigs reference (not needed in fork child process)

_sig_on_prejmp, _sig_on_postjmp, _sig_off_, _sig_unblock_, and _set_debug_level are called on every sig_on/sig_off. Since implementation.c is always #included (same translation unit), static inline is valid and gives the compiler a stronger hint to inline them, matching the original behaviour in macros.h.

cxzhong requested a review from tornaria April 4, 2026 18:26

cxzhong requested a review from Copilot April 4, 2026 19:33

Copilot started reviewing on behalf of cxzhong April 4, 2026 19:33 View session

Copilot AI reviewed Apr 4, 2026

View reviewed changes

Comment thread src/cysignals/signals.pxd Outdated

tornaria reviewed Apr 4, 2026

View reviewed changes

cxzhong force-pushed the remove-preliminary_late_includes_cy28 branch 2 times, most recently from 9c7546d to d8b8bda Compare April 5, 2026 08:38

cxzhong added 2 commits April 5, 2026 17:06

cxzhong force-pushed the remove-preliminary_late_includes_cy28 branch from d8b8bda to c1e8888 Compare April 5, 2026 09:06

Fix the document which is incorrect

aab2284

cxzhong closed this Apr 6, 2026



		#if __USE_FORTIFY_LEVEL
		#error "cysignals must be compiled without _FORTIFY_SOURCE"

Uh oh!

Uh oh!

Conversation

cxzhong commented Apr 4, 2026

Uh oh!

cxzhong commented Apr 4, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cxzhong Apr 4, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tornaria Apr 4, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tornaria commented Apr 4, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

cxzhong commented Apr 4, 2026 •

edited

Loading

cxzhong Apr 4, 2026 •

edited

Loading

tornaria Apr 4, 2026 •

edited

Loading