Opened 11 years ago
Closed 11 years ago
#11942 closed defect (fixed)
segfault on Graph().connected_component_containing_vertex('')
Reported by: | kini | Owned by: | jason, ncohen, rlm |
---|---|---|---|
Priority: | major | Milestone: | sage-5.0 |
Component: | graph theory | Keywords: | segfault cython |
Cc: | Merged in: | sage-5.0.beta0 | |
Authors: | Nathann Cohen, Keshav Kini, Jeroen Demeyer | Reviewers: | Keshav Kini, David Coudert, Nathann Cohen |
Report Upstream: | N/A | Work issues: | |
Branch: | Commit: | ||
Dependencies: | Stopgaps: |
Description (last modified by )
As title.
fs@zhenghe /tmp $ sage -c "Graph().connected_component_containing_vertex('')" /opt/sage/local/lib/libcsage.so(print_backtrace+0x31)[0x7f14e1a96df2] /opt/sage/local/lib/libcsage.so(sigdie+0x14)[0x7f14e1a96e24] /opt/sage/local/lib/libcsage.so(sage_signal_handler+0x20c)[0x7f14e1a96a72] /lib/x86_64-linux-gnu/libpthread.so.0(+0xf020)[0x7f14e31fa020] /opt/sage/local/lib/python2.6/site-packages/sage/graphs/base/c_graph.so(+0x11aa1)[0x7f14be4d8aa1] /opt/sage/local/lib/libpython2.6.so.1.0(+0x75876)[0x7f14e347c876] /opt/sage/local/lib/libpython2.6.so.1.0(+0x75b92)[0x7f14e347cb92] /opt/sage/local/lib/libpython2.6.so.1.0(+0xa0f08)[0x7f14e34a7f08] /opt/sage/local/lib/libpython2.6.so.1.0(PyObject_Call+0x53)[0x7f14e3451cd3] /opt/sage/local/lib/libpython2.6.so.1.0(PyEval_EvalFrameEx+0x395d)[0x7f14e34e835d] /opt/sage/local/lib/libpython2.6.so.1.0(PyEval_EvalFrameEx+0x602d)[0x7f14e34eaa2d] /opt/sage/local/lib/libpython2.6.so.1.0(PyEval_EvalCodeEx+0x879)[0x7f14e34eb549] /opt/sage/local/lib/libpython2.6.so.1.0(PyEval_EvalCode+0x32)[0x7f14e34eb642] /opt/sage/local/lib/libpython2.6.so.1.0(+0xdb07e)[0x7f14e34e207e] /opt/sage/local/lib/libpython2.6.so.1.0(PyEval_EvalFrameEx+0x4f8d)[0x7f14e34e998d] /opt/sage/local/lib/libpython2.6.so.1.0(PyEval_EvalCodeEx+0x879)[0x7f14e34eb549] /opt/sage/local/lib/libpython2.6.so.1.0(PyEval_EvalCode+0x32)[0x7f14e34eb642] /opt/sage/local/lib/libpython2.6.so.1.0(PyRun_FileExFlags+0xb0)[0x7f14e350d9b0] /opt/sage/local/lib/libpython2.6.so.1.0(PyRun_SimpleFileExFlags+0xdf)[0x7f14e350e3ef] /opt/sage/local/lib/libpython2.6.so.1.0(Py_Main+0xb23)[0x7f14e351b693] /lib/x86_64-linux-gnu/libc.so.6(__libc_start_main+0xfd)[0x7f14e27fcead] python[0x4006a1] ------------------------------------------------------------------------ Unhandled SIGSEGV: A segmentation fault occurred in Sage. This probably occurred because a *compiled* component of Sage has a bug in it and is not properly wrapped with sig_on(), sig_off(). You might want to run Sage under gdb with 'sage -gdb' to debug this. Sage will now terminate. ------------------------------------------------------------------------ /opt/sage/local/bin/sage-sage: line 772: 19065 Segmentation fault sage-eval "$@" fs@zhenghe /tmp $
Apply:
Attachments (3)
Change History (34)
comment:1 Changed 11 years ago by
- Description modified (diff)
- Status changed from new to needs_review
comment:2 follow-up: ↓ 3 Changed 11 years ago by
- Reviewers set to Keshav Kini
Wow, that was fast! :D I have a couple of questions, though. Graph().connected_component_containing_vertex(0)
already produces an exception, which includes the number 0 in the message. This is still true with your patch. Currently investigating why...
Also, your exception is a ValueError
- shouldn't it be a KeyError
? Shouldn't the current error for vertex 0
be a KeyError
too instead of a (!?) RuntimeError
? The Python docs say:
This exception is mostly a relic from a previous version of the interpreter; it is not used very much any more.
comment:3 in reply to: ↑ 2 Changed 11 years ago by
Hellooooooo !
This exception is mostly a relic from a previous version of the interpreter; it is not used very much any more.
Oh ? That's a pity, I really use it a lot :-)
I immediately update the patch to return a KeyError? :-)
Nathann
comment:4 follow-up: ↓ 5 Changed 11 years ago by
Oh, ValueError
is still used a lot but it is RuntimeError
which is not used much anymore.
comment:5 in reply to: ↑ 4 Changed 11 years ago by
Oh,
ValueError
is still used a lot but it isRuntimeError
which is not used much anymore.
Argggggg !!!
Patch reverted to the former one :-D
Nathann
Changed 11 years ago by
comment:6 follow-up: ↓ 7 Changed 11 years ago by
... but... even though ValueError
is used a lot, the correct one to use here is KeyError
... D:
Anyway don't worry about it, I'll do that in another patch later :) Sorry, didn't get much time to look at this earlier this week.
comment:7 in reply to: ↑ 6 Changed 11 years ago by
Replying to kini:
... but... even though
ValueError
is used a lot, the correct one to use here isKeyError
...D:
Arggggggggggg !!!
Anyway don't worry about it, I'll do that in another patch later :) Sorry, didn't get much time to look at this earlier this week.
Good !! THaaaaanks ! :-)
Nathann
comment:9 Changed 11 years ago by
- Milestone set to sage-4.8
- Reviewers changed from Keshav Kini to Keshav Kini, David Coudert
- Status changed from needs_review to positive_review
I don't understand why you have both stopped the discussion without concluding on the status of this patch.
I have no problem when installing the patch and building the documentation. The function tests correctly that the parameter is or is not a vertex of the graph.
For me the patch is correct, so I give a positive review.
Best, David.
comment:10 Changed 11 years ago by
- Status changed from positive_review to needs_work
Please read the comments. The ValueError? needs to be changed to KeyError?, and also I wanted to understand the following before giving it a positive review, as possibly the fix could be improved (multiple code paths for a single purpose is bad design):
Graph().connected_component_containing_vertex(0)
already produces an exception, which includes the number 0 in the message. This is still true with your patch.
Sorry for neglecting this ticket for so long.
comment:11 Changed 11 years ago by
- Description modified (diff)
- Keywords segfault cython added
- Status changed from needs_work to needs_review
Well, I'm still not sure about the multiple code paths. But here is a patch to use better exceptions throughout the file in question, though that's sort of tangential to the original purpose of the ticket. This ticket has been sitting around too long for me to delay it any further just because of my ignorance of the Cython graphs code, especially since it fixes a segfault, so I will give a positive review to Nathann's part. Maybe someone can review my exceptions patch? It passes make ptestlong
on my machine fwiw.
comment:12 Changed 11 years ago by
- Status changed from needs_review to needs_work
Well, what about setting the message to what it was before, and just change the exception's name if yo feels it should be ? The former message was way clearer, especially when the exception is thrown by a submethod of the method run by the user.
Nathann
comment:13 Changed 11 years ago by
ValueError
's constructor accepts a string representing a message, but KeyError
's constructor accepts an object representing the key which was not found in some collection. So this is the "correct" way to raise the exception here. The traceback call stack should theoretically give you all the info that was in the error message before. I noticed this because raise ValueError('foo')
gives you a message "ValueError: foo" whereas raise KeyError('foo')
gives you a message "KeyError: 'foo'" (note the quotation marks). I asked about it on the Python IRC channel and the above is the answer I got.
comment:14 Changed 11 years ago by
Well, for me this modification just makes the error message harder to understand, and I do not see the point of losing that if the only gain is to respect some standard we did not know even existed two weeks ago.
comment:15 Changed 11 years ago by
Hmm. OK, I'll just put back the messages and still use KeyError?. The quotation marks are a bit weird, but not such a big deal, I guess - we are not actually trying to catch these exceptions and do anything with them anyway.
comment:16 Changed 11 years ago by
Hello, It would be nice if the KeyError? method could take an extra argument to describe the error. For instance KeyError?(key, 'You should add some tests to your algorithm!'). Unfortunately, I'm not able to find the documentation of the KeyError? function.
However, according to the documentation of Python, the raise method can handle multiple arguments:
raise_stmt ::= "raise" [expression ["," expression ["," expression]]]
See: http://docs.python.org/reference/simple_stmts.html#raise
It may help satisfying both of you ;-)
Best, David.
comment:17 Changed 11 years ago by
Thank you, David! I agree with you, that would be great. Unfortunately it is not supported by Python. The documentation you linked about the raise
statement is unfortunately not so helpful - there must be between 1 and 3 arguments, and they have special meanings. Actually, the multiple-argument syntax for raise
is there in order to provide backwards compatibility with old versions of Python in which Exceptions were not objects with constructors. Now we would do something like
raise HALError("I can't let you do that, Dave")
but before you would write
raise HALError, "I can't let you do that, Dave"
because the HALError(foo)
syntax didn't exist. So the multiple arguments basically allows you to still use the old syntax. Unfortunately this doesn't mean that we can just raise multiple exceptions...
comment:18 Changed 11 years ago by
- Status changed from needs_work to needs_review
New patch up. Sorry about all this noise, I should really have just made a different ticket for this exception stuff.
comment:19 Changed 11 years ago by
- Reviewers changed from Keshav Kini, David Coudert to Keshav Kini, David Coudert, Nathann Cohen
- Status changed from needs_review to positive_review
Hellooooo !!!
Well, thank you for this new version :-)
It passes all tests, so this ticket can go !
Nathann
comment:20 follow-up: ↓ 21 Changed 11 years ago by
Hurrah! Thanks for the review (and the original patch)!
BTW, I botched that 2001 quote above... it should be "I'm sorry, Dave, I'm afraid I can't do that", of course :P
comment:21 in reply to: ↑ 20 Changed 11 years ago by
BTW, I botched that 2001 quote above... it should be "I'm sorry, Dave, I'm afraid I can't do that", of course
:P
No harm done, we both watched the french version of it :-D
comment:22 Changed 11 years ago by
- Milestone changed from sage-4.8 to sage-5.0
comment:23 Changed 11 years ago by
- Description modified (diff)
- Status changed from positive_review to needs_work
Since KeyError
is specifically about dicts, I think the more general LookupError
is better. Needs review.
comment:24 Changed 11 years ago by
- Status changed from needs_work to needs_review
comment:25 Changed 11 years ago by
- Milestone changed from sage-5.0 to sage-4.8
Oh, I was under the impression that those base classes shouldn't be instantiated, but that seems to be unfounded after reading the docs. Looks good to me! Lines 855-856 of $SAGE_ROOT/devel/sage/sage/graphs/base/graph_backends.py
contains another exception which seems like it should be a LookupError
but I'm not sure I understand why the current exception is imported from !NetworkX. Maybe there is a good reason for it?
comment:26 Changed 11 years ago by
- Milestone changed from sage-4.8 to sage-5.0
Whoops, didn't mean to change the milestone.
comment:27 Changed 11 years ago by
Well, these lines are contained in the NetworkX backend, so it may mean that they define their own Exceptions in the NetworkX project. I do not think this class is still used in Sage anyway.
Nathann
comment:28 Changed 11 years ago by
- Status changed from needs_review to positive_review
But didn't Sage developers write that NetworkX backend wrapper? Well, never mind, if it's not used in Sage anymore.
comment:29 follow-up: ↓ 30 Changed 11 years ago by
Well yes, and that's most probably this class. But if you run some code made for NetworkX on a Sage graph with our own exception it may break the NetworkX code. I guess this was the point of importing exceptions. Thank you for the patch Jeroen !
Nathann
comment:30 in reply to: ↑ 29 Changed 11 years ago by
Replying to ncohen:
if you run some code made for NetworkX on a Sage graph
Aha, now I get it. And yes, thanks Jeroen :)
comment:31 Changed 11 years ago by
- Merged in set to sage-5.0.beta0
- Resolution set to fixed
- Status changed from positive_review to closed
Right !
:-p
This comes from an unchecked operation at the beginning of BFS/DFS. Fixed in the following patch
:-)
Nathann