remove a stray leftover in in sa2si_ZZmod() #37792
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
dimpase
changed the title
5 tasks
|
Documentation preview for this PR (built with commit 4b35649; changes) is ready! 🎉 |
|
When I tried out the example in the report on sage-devel, I was getting crashes in the same routine. Furthermore, it really does seem that the value assigned in the removed statement isn't used in the routine. Do we somehow need a side effect of requesting the characteristic of the parent? I doubt it, so I expect this code to be a simple clean-up. I'll leave this comment here for a short while in case someone has something to contribute. Otherwise, it's a clean-up that doesn't need a documentation added. EDIT: I see the PR already has a positive review. Never mind! |
|
A Source: build with |
|
this must be a side-effect of mass-slapping Cython functions with |
|
One of them is not Sage, but numpy. |
Not really. In cython 2, all functions were implicitly In cython 3, this default was changed, but for the migration to cython 3 it was decided to use Note:
Conveniently, cython 3 gives a warning each time the "implicit noexcept" is triggered. Thus #36507 made all those Plot twist: the cython warning was incorrect (see cython/cython#6087 for the fix) so a lot of the explicitly added Since I've now actually reviewed cython code for cython/cython#6087, I'm reasonably confident now that we've got all the As for the 572 problems I raised in my previous comment. There are three possible ways to fix each one:
The first one could be made very quickly of course, at the potential expense of performance. |
I think I already fixed it in numpy/numpy#26129 but I'll double check. |
|
... but sure, this means there are only 570 left. |
Indeed it's there. Should be fixed in numpy 1.26.5 (and in numpy 2.0). |
this causes a segfault with Ctrl-C with probability about 1/3.
dimpase
force-pushed
the
remove_stray_statement
branch
from
c3e8e8a to
4b35649
Compare
|
there are places where Less sure about |
Yes, unless the Rather than just excluding https://cython.readthedocs.io/en/latest/src/userguide/language_basics.html#error-return-values
Please give the example you found. I think a If there is such a function, it might mean there's something wrong with the warnings. With an explicit example I could have a look into the code and see what is going on. |
|
@tornaria To be able to reproduce the warnings 1-1, I need to exactly know what Cython I should use, and whether anything else in Sage needs to be modified. Currently I use Cython 3.0.10 from Gentoo. |
|
regarding $ git grep cpdef | grep noexcept | wc -l
341so there are quite a few of these in the repo. |
|
It looks like this ticket has identified the problem and is progressing nicely. I don't think I have much to add in terms of a review here. (the main point of removing my "reviewer invitation" status on the PR here was to get rid of the blue dots on the github mailbox that arise. But I'm afraid I'll now be getting blue dots due to having commented on the PR ... github does give strange incentives) |
|
@nbruin you can click the "unsubscribe" button on the right pane:
|
3 tasks
vbraun
pushed a commit
to vbraun/sage
that referenced
this pull request
Apr 28, 2024
As reported on [sage-devel](https://groups.google.com/g/sage- devel/c/r7IpxjZTz5Y/m/d0I95VSnAAAJ) and reproduced, the following crashes Sage with probability about 1/3. ```python x,y=var('x,y');n=10**6 while True: so=solve_mod(x*y-1,n) #press CTL-C, crashes with probability about 1/3 ``` The backtrace points to is a meaningless assignment in `sa2si_ZZmod()` ```python nr2mModul = d.parent().characteristic() ``` where this happens. `nr2mModul` is not used anywhere in the code. Removing it appears to make the crash to go away. ### 📝 Checklist <!-- Put an `x` in all the boxes that apply. --> - [x] The title is concise and informative. - [x] The description explains in detail what this PR is about. - [x] I have linked a relevant issue or discussion. - [ ] I have created tests covering the changes. - [ ] I have updated the documentation accordingly. URL: sagemath#37792 Reported by: Dima Pasechnik Reviewer(s):
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
As reported on sage-devel and reproduced, the following crashes Sage with probability about 1/3.
The backtrace points to is a meaningless assignment in
sa2si_ZZmod()where this happens.
nr2mModulis not used anywhere in the code. Removing it appears to make the crash to go away.📝 Checklist