Re: [Inkscape-devel] [Inkscape-user] svn-19741 node tool crash
On Aug 20, 2008, at 10:35 AM, Docsonic wrote:
I just updated svn to 19741 and it has an odd crash situation -
If I open a document or create new and add objects, I can manipulate the objects with either the select tool or the node tool with no problems. However, if I convert any object to a path and select the path with the node tool, Inkscape crashes as soon as I click on the path. I also tried drawing a rectangle, converting to a path and selecting with the select tool. Then I chose the node tool so that the path nodes are displayed and I tried rubberbanding 2 of the nodes. The moment I let go of the mouse button, Inkscape crashes.
This is in WinXP SP2
I was chasing this down the other night with a few others. We narrowed it down to only happening when -O2 was used with the build, but not -O0. In other words it was only showing up with optimizations on.
Maybe that will give someone a hint for reproducing and/or fixing.
Just wanted to confirm the crash, unfortunately I don't have time right now to help track it down. Maybe tomorrow if it's not fixed by then :)
Max: this is apparently the same crash as I was getting with your Geometry tool, so it's not your tool to blame but something else.
On Wed, Aug 20, 2008 at 2:42 PM, Jon A. Cruz <jon@...18...> wrote:
I was chasing this down the other night with a few others. We narrowed it down to only happening when -O2 was used with the build, but not -O0. In other words it was only showing up with optimizations on.
Maybe that will give someone a hint for reproducing and/or fixing.
Jon A. Cruz schrieb:
I was chasing this down the other night with a few others. We narrowed it down to only happening when -O2 was used with the build, but not -O0. In other words it was only showing up with optimizations on.
I didn't experience that crash because I usually build without optimization. But since I did a whole number of commits around the date when the crash was introduced, I now did a fresh checkout and rebuilt with -O2. With this build, I was able to reproduce the crash and thus to bisect the history to track it down.
It turned out that the culprit commit was indeed one of mine, namely rev. 19690. But I totally don't understand what's causing it. I committed a "fix" (of which I don't understand why it works) in rev. 19744. Everyone is invited to take a look at it because I'm at a loss with this. Basically, what fixed the issue was that I moved the following line
mc = SP_NODE_CONTEXT(ec)->_node_message_context;
from the function get_message_context() at the beginning of nodepath.cpp to its original (pre-19690) locations in line 3930 and line 4762. But i didn't change the line itself, and I did check that it really was executed in the case distinction in get_message_context(). So there shouldn't be any difference in functionality whatsoever.
Can anyone please confirm that this really fixes the issue? And perhaps even provide an explanation? Is this a compiler bug which should be reported upstream? I will be away for one and a half weeks so if there are any further issues feel free to mess with the code yourself.
Regards, Max
On Fri, Aug 22, 2008 at 12:36 PM, Maximilian Albert <Anhalter42@...173...> wrote:
Can anyone please confirm that this really fixes the issue? And perhaps even provide an explanation?
Fix confirmed, and looking at http://inkscape.svn.sourceforge.net/viewvc/inkscape/inkscape/trunk/src/nodep..., it appears pretty obvious to me at least for the Geometry tool crashes: since you now use this function not only in node tool, you need to check if it's a node tool context before taking its message context. But why it was also crashing in Node tool I have no idea.
Anyway, I'm playing with the Geometry tool and will be able to give you my feedbacl soon.
bulia byak schrieb:
On Fri, Aug 22, 2008 at 12:36 PM, Maximilian Albert <Anhalter42@...173...> wrote:
Can anyone please confirm that this really fixes the issue? And perhaps even provide an explanation?
Fix confirmed, and looking at http://inkscape.svn.sourceforge.net/viewvc/inkscape/inkscape/trunk/src/nodep..., it appears pretty obvious to me at least for the Geometry tool crashes: since you now use this function not only in node tool, you need to check if it's a node tool context before taking its message context.
But that's exactly what the function get_message_context() does (see nodepath.cpp, line 844), which was introduced precisely for this purpose. :-/
Anyway, I'm playing with the Geometry tool and will be able to give you my feedbacl soon.
Great, although please bear in mind the limitations I wrote about (e.g., selecting objects by click is broken). I have a whole number of things on my todo list, too, but it's of course helpful to know what other people regard as the most pressing limitations/lacking features.
Max
Maximilian Albert schrieb:
bulia byak schrieb:
On Fri, Aug 22, 2008 at 12:36 PM, Maximilian Albert <Anhalter42@...173...> wrote:
Can anyone please confirm that this really fixes the issue? And perhaps even provide an explanation?
Fix confirmed, and looking at http://inkscape.svn.sourceforge.net/viewvc/inkscape/inkscape/trunk/src/nodep..., it appears pretty obvious to me at least for the Geometry tool crashes: since you now use this function not only in node tool, you need to check if it's a node tool context before taking its message context.
But that's exactly what the function get_message_context() does (see nodepath.cpp, line 844), which was introduced precisely for this purpose. :-/
Also, this doesn't explain why it didn't crash for me at all (and apparently neither for other people) when compiling without -O2.
max
On Fri, 2008-08-22 at 19:08 +0200, Maximilian Albert wrote:
Maximilian Albert schrieb:
bulia byak schrieb:
On Fri, Aug 22, 2008 at 12:36 PM, Maximilian Albert
<Anhalter42@...173...> wrote:
Can anyone please confirm that this really fixes the issue? And
perhaps
even provide an explanation?
Fix confirmed, and looking at
http://inkscape.svn.sourceforge.net/viewvc/inkscape/inkscape/trunk/src/nodep...,
it appears pretty obvious to me at least for the Geometry tool crashes: since you now use this function not only in node tool, you need to check if it's a node tool context before taking its message context.
But that's exactly what the function get_message_context() does
(see
nodepath.cpp, line 844), which was introduced precisely for this purpose. :-/
Also, this doesn't explain why it didn't crash for me at all (and apparently neither for other people) when compiling without -O2.
I think I finally tracked it down.
With tonight's compile I saw a warning that led me to look into get_message_context(). It turns out that the function had an early return statement, but was missing a line to actually return a value. That happens to be one of the problems with code with multiple returns: hard to note a case where the wrong thing is returned or not returned.
I cleaned it up and removed the work-around and it seems happy with a base compile and with running under valgrind.
However, it would be good if people checked to see that all of the cleanup did not introduce any other problems. I did some extra code review and some simple tests here, but it's best to be gone over.
Thanks.
I can confirm that this fixed the problem for me :-)
Tony
Maximilian Albert wrote:
Can anyone please confirm that this really fixes the issue? And perhaps even provide an explanation? Is this a compiler bug which should be reported upstream? I will be away for one and a half weeks so if there are any further issues feel free to mess with the code yourself.
Regards, Max
participants (4)
-
bulia byak -
Docsonic -
Jon A. Cruz -
Maximilian Albert