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.

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

Segfault for boolean evaluation of expression with assumptions #28538

Closed
sagetrac-tmonteil mannequin opened this issue Sep 25, 2019 · 26 comments
Closed

Segfault for boolean evaluation of expression with assumptions #28538

sagetrac-tmonteil mannequin opened this issue Sep 25, 2019 · 26 comments

Comments

@sagetrac-tmonteil
Copy link
Mannequin

sagetrac-tmonteil mannequin commented Sep 25, 2019

As reported on this ask question:

sage: x, y = var('x, y')
sage: assume(x>0)
sage: assume(y>0)
sage: bool(y*(x-y)==0)

leads to (on my computer 8.9.rc1) a sequence of:

;;;
;;; Detected access to protected memory, also kwown as 'bus or segmentation fault'.
;;; Jumping to the outermost toplevel prompt
;;;

followed by a Segmentation fault crash of Sage.

Or (as reported, on 8.8):

RuntimeError: ECL says: C-STACK overflow at size 1048576. Stack can probably be resized. Proceed with caution.

Exchanging x and y works correctly:

sage: x, y = var('x, y')
sage: assume(x>0)
sage: assume(y>0)
sage: bool(x*(y-x)==0)
False

Upstream ticket: https://sourceforge.net/p/maxima/bugs/3583/

Depends on #30063

Upstream: Fixed upstream, in a later stable release.

CC: @rwst @kcrisman

Component: symbolics

Author: Thierry Monteil

Branch/Commit: cb70171

Reviewer: Matthias Koeppe

Issue created by migration from https://trac.sagemath.org/ticket/28538

@sagetrac-tmonteil sagetrac-tmonteil mannequin added this to the sage-9.0 milestone Sep 25, 2019
@EmmanuelCharpentier
Copy link
Mannequin

EmmanuelCharpentier mannequin commented Sep 26, 2019

comment:1

On 8.9.rc1+#28534 (Python 3-based), I get a lot of

;;;
;;; Detected access to protected memory, also kwown as 'bus or segmentation fault'.
;;; Jumping to the outermost toplevel prompt
;;;

and a Sage crash:

Process Sage erreur de segmentation

Nice one...

@sagetrac-tmonteil

This comment has been minimized.

@kcrisman
Copy link
Member

comment:4

I'm so sorry I just don't have time any more to track down as many of these (though once in a while I somehow make the time). But I think the best thing to do is to do whatever bool does in Maxima-in-sage sage -maxima and then load exactly the packages preloaded by Sage - the complex domain is usually the most suspicious one on these fronts, though I have to say this is really puzzling. I imagine bool calls a comparison with zero at some point in Maxima, though I don't recall any more because I wasn't involved with the comparison-with-zero code much.

@nbruin
Copy link
Contributor

nbruin commented Sep 26, 2019

comment:5

In maxima:

domain: complex;
assume(x>0,y>0);
is(equal(y*(x-y),0));

replicates the crash. That's sufficient to report upstream. Perhaps they can fix it.

@sagetrac-tmonteil
Copy link
Mannequin Author

sagetrac-tmonteil mannequin commented Sep 26, 2019

Upstream: Reported upstream. No feedback yet.

@sagetrac-tmonteil
Copy link
Mannequin Author

sagetrac-tmonteil mannequin commented Sep 26, 2019

comment:6

Thanks for tracking, this is now tracked upstream as https://sourceforge.net/p/maxima/bugs/3583/

@sagetrac-tmonteil

This comment has been minimized.

@sagetrac-tmonteil
Copy link
Mannequin Author

sagetrac-tmonteil mannequin commented Nov 21, 2019

Changed upstream from Reported upstream. No feedback yet. to Fixed upstream, in a later stable release.

@embray
Copy link
Contributor

embray commented Jan 6, 2020

comment:8

Ticket retargeted after milestone closed

@embray embray modified the milestones: sage-9.0, sage-9.1 Jan 6, 2020
@mkoeppe mkoeppe modified the milestones: sage-9.1, sage-9.2 May 7, 2020
@sagetrac-tmonteil
Copy link
Mannequin Author

sagetrac-tmonteil mannequin commented Aug 14, 2020

Dependencies: #30063

@sagetrac-tmonteil
Copy link
Mannequin Author

sagetrac-tmonteil mannequin commented Aug 14, 2020

comment:10

Once #30063 will be merged, i will make a patch to doctest that ticket.

@EmmanuelCharpentier
Copy link
Mannequin

EmmanuelCharpentier mannequin commented Aug 14, 2020

comment:11

Replying to @sagetrac-tmonteil:

Once #30063 will be merged, i will make a patch to doctest that ticket.

The original problem seems fixed by #30063 :

sage: x, y = var('x, y') 
....: assume(x>0) 
....: assume(y>0) 
....: bool(y*(x-y)==0)                                                          
False

Testing it properly might be a bit tricky, tough...

@sagetrac-tmonteil
Copy link
Mannequin Author

sagetrac-tmonteil mannequin commented Aug 14, 2020

comment:12

Replying to @EmmanuelCharpentier:

Replying to @sagetrac-tmonteil:

Once #30063 will be merged, i will make a patch to doctest that ticket.

The original problem seems fixed by #30063 :

This is why i am waiting #30063 to be merged to add a doctest.

sage: x, y = var('x, y') 
....: assume(x>0) 
....: assume(y>0) 
....: bool(y*(x-y)==0)                                                          
False

Testing it properly might be a bit tricky, tough...

What is wrong with using the raw example ?

@EmmanuelCharpentier
Copy link
Mannequin

EmmanuelCharpentier mannequin commented Aug 14, 2020

comment:13

Replying to @sagetrac-tmonteil:

[ Snip... ]

Testing it properly might be a bit tricky, tough...

What is wrong with using the raw example ?

Too special case... I am not sure what the original problem was.

@sagetrac-tmonteil
Copy link
Mannequin Author

sagetrac-tmonteil mannequin commented Aug 24, 2020

@sagetrac-tmonteil
Copy link
Mannequin Author

sagetrac-tmonteil mannequin commented Aug 24, 2020

Commit: fe71c17

@sagetrac-tmonteil
Copy link
Mannequin Author

sagetrac-tmonteil mannequin commented Aug 24, 2020

New commits:

fe71c17#28538 : add doctest for #28538

@sagetrac-tmonteil
Copy link
Mannequin Author

sagetrac-tmonteil mannequin commented Aug 24, 2020

Author: Thierry Monteil

@sagetrac-tmonteil
Copy link
Mannequin Author

sagetrac-tmonteil mannequin commented Aug 24, 2020

comment:16

Replying to @EmmanuelCharpentier:

Replying to @sagetrac-tmonteil:

[ Snip... ]

Testing it properly might be a bit tricky, tough...

What is wrong with using the raw example ?

Too special case... I am not sure what the original problem was.

This is a Maxima bug, it was reported and fixed upstream. I do not have the skill to inspect further within Maxima source code, so this doctest is the best i can provide, and it corresponds to the reported bug. If someone could provide more doctests to surround the original problem more securely, i am all for it.

@kcrisman
Copy link
Member

comment:17

I think that testing the original problem no longer leads to a segfault is fine. I agree that this is all we can do if Maxima upstream fixed it and we don't really know what the issue was. (Though it looks like it was, again, our use of domain:complex that triggered it.)

@seblabbe
Copy link
Contributor

comment:18

Typo: Chack -> Check

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Aug 27, 2020

Branch pushed to git repo; I updated commit sha1. New commits:

cb70171#28538 : typo

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Aug 27, 2020

Changed commit from fe71c17 to cb70171

@sagetrac-tmonteil
Copy link
Mannequin Author

sagetrac-tmonteil mannequin commented Aug 27, 2020

comment:20

Fixed, thanks for pointing this !

@mkoeppe
Copy link
Member

mkoeppe commented Aug 28, 2020

Reviewer: Matthias Koeppe

@vbraun
Copy link
Member

vbraun commented Sep 1, 2020

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

No branches or pull requests

6 participants