Skip to content
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.

Sign up for GitHub

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

Jump to bottom

declare the last arg to GAP_CallFunc3Args volatile #37951

Merged
merged 1 commit into from
May 25, 2024
Merged

declare the last arg to GAP_CallFunc3Args volatile #37951

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion src/sage/libs/gap/element.pyx
View file
Open in desktop
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since sig_GAP_Enter() calls sig_error(), shouldn't it be called after the call to sig_on()?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree that this ought to be fixed, but it's a different bug. Flipping these orders don't cure the issue at hand, that volatile declaration is still needed. I'm not sure it's a Sage or a gcc bug.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, I was just wondering. Since I don't have gcc 14 I can't reproduce myself. Since we don't know what is the cause of the problem, we don't know whether the volatile fixes the issue or we just push around the optimizer to avoid it. The nesting of setjmp() (one for gap, one for sig_on) seems to complicate things.

Looking at the assembler, it seems that gcc is using %ebx for both t in #define sig_GAP_Enter() {int t = GAP_Enter(); if (!t) sig_error();} and for __pyx_t_6 (which contains the third parameter). Making the third parameter volatile avoids the conflict t, but maybe it's better to mark t volatile instead (we don't know if another gcc will place the second parameter in %ebx, etc). Does that work?

I.e.,

--- a/src/sage/libs/gap/gap_includes.pxd
+++ b/src/sage/libs/gap/gap_includes.pxd
@@ -28,7 +28,7 @@ cdef extern from "gap/calls.h" nogil:
 
 cdef extern from "gap/libgap-api.h" nogil:
     """
-    #define sig_GAP_Enter()  {int t = GAP_Enter(); if (!t) sig_error();}
+    #define sig_GAP_Enter()  {volatile int t = GAP_Enter(); if (!t) sig_error();}
     """
     ctypedef void (*GAP_CallbackFunc)()
     void GAP_Initialize(int argc, char ** argv,

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it's actually gcc 13.2.1 which gives this trouble on Gentoo, not gcc 14.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah.... I'm on 13.2.0

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking at the assembler, it seems that gcc is using %ebx for both t in #define sig_GAP_Enter() {int t = GAP_Enter(); if (!t) sig_error();} and for __pyx_t_6 (which contains the third parameter). Making the third parameter volatile avoids the conflict t, but maybe it's better to mark t volatile instead (we don't know if another gcc will place the second parameter in %ebx, etc). Does that work?

no, it doesn't. And, after all, t in int t = GAP_Enter(); gets out of the way almost immediately, before the list for arguments for GAP_CallFunc3Args is even created.

Original file line number Diff line number Diff line change
Expand Up @@ -2504,6 +2504,7 @@ cdef class GapElement_Function(GapElement):
cdef Obj result = NULL
cdef Obj arg_list
cdef int n = len(args)
cdef volatile Obj v2

if n > 0 and n <= 3:
libgap = self.parent()
Expand All @@ -2522,10 +2523,11 @@ cdef class GapElement_Function(GapElement):
(<GapElement>a[0]).value,
(<GapElement>a[1]).value)
elif n == 3:
v2 = (<GapElement>a[2]).value
result = GAP_CallFunc3Args(self.value,
(<GapElement>a[0]).value,
(<GapElement>a[1]).value,
(<GapElement>a[2]).value)
v2)
else:
arg_list = make_gap_list(args)
result = GAP_CallFuncList(self.value, arg_list)
Expand Down
Loading