Hi Volker,

Thanks for your clear message ;-)


Volker Schöch wrote On 14-11-2014 15:28:

Hi Barend,

Thank you for your recent reply. I meant to answer all your questions in one mail, which took a while, so please bear with me while I try to sort it all out.

 

1) I think I have to write: it is volunteer work - the ticket is opened one week ago. We appreciate your usage of Boost.Geometry and the feedback too, but take care not to transfer the pressure you feel to us volunteers... At least not too soon.

Sure, absolutely. I can’t tell you how much I appreciate your work and your availability! Honestly. By “we” I was referring to think-cell: We have to deal with these problems one way or another. Of course, before spending time on working around problems, it’s tremendously helpful to understand if and when there might be a real fix available. I apologize if my way of putting this into words was ambiguous. No harm meant!


Thanks - we appreciate all your messages and researches. There is no harm of course, I just wanted to subtle indicate something about pressure but maybe it was not subtle enough... Anyway, no problem at all.


2) Menelaos sent a sample program which works for him, and works for me. Did you try that program? He also asked
Do you think you can reproduce the problem in a standalone program?

but you did not answer that question. Actually the whole mail was unanswered.

 

The fact that the algorithm accesses uninitialized memory means two things:

-       The problem may be hard to reproduce.

-       Just because it happens to work under certain circumstances, doesn’t mean it’s correct.

For this reason, instead of insisting on the fact that it keeps crashing on my machine when compiled with my compiler, my colleague Valentin made an effort and stepped into the algorithm and found some code where you were (in 1.56.0) accessing uninitialized memory. Whether or not that produces a crash later, depends on the actual values found in that uninitialized memory. For example, most compilers in fact do initialize “uninitialized” memory with a certain value pattern, when compiling with debug build settings, and so when testing you may actually reliably read the same value that doesn’t lead to a crash further down. But that doesn’t mean it’s correct, and it may still crash, e.g., with my release build compiler settings.

 

For all these reasons, I deemed Valentins email an appropriate answer to Menelaos’ question.


The answer was helpful indeed.


3) you already mention that your input is invalid - so you should be able to workaround the problem probably easily.

Yes, you’re right. Running the new is_valid(…) predicate before invoking the actual algorithm helps to suppress the problem in 1.56.0. I’m still struggling with your ideas of valid/invalid, pre conditions and post conditions. Let’s postpone that topic to a separate discussion.


4) you include a stack-trace (thanks for that) but things have been changed, get_situation_map is gone from enrich_intersection_points, and so might be the whole problem. This is by chance released this week
So my new question is: does it work for you in 1.57?

We moved our entire project to 1.57.0 (which is part of the reason why I did not reply earlier). The apparently good news is: I cannot reproduce the crash any more. I even made sure, that copy_segment_point does not return false, and in my quick testing so far, it never did, which (to my understanding) means that point_out actually gets initialized.

 

I still find it suspicious that consider_relative_order calls copy_segment_points without checking for the return value. If copy_segment_point and consequently copy_segment_points would return false, then pi, pj, ri, rj, si, sj would be uninitialized and all kinds of things could happen further down. I would feel much better if at that point there were an assertion, or an exception thrown, because then the problem – if there is any – would become obvious independent of the actual values that happen to be found in uninitialized memory.


Yes you are right. Actually I already did, because of your last mail. After inspecting the code I made a fix. For open polygons, copying the very last segment, it did not work correctly and therefore failed. This was because the size of the range was inspected before the view. Most probably this was the problem. That was after 1.57 was released, so you don't have it. It's this commit:

https://github.com/boostorg/geometry/commit/16fb68921763ca188397d5e1bebf6ff005ecdaa8

In fact I implemented your suggestion too, because it asserts now and always return true. We can remove its return value now (I think I will do).

 

Thus here is my suggestion: Since this piece of code is vulnerable to problems that lead to errors much later, and those errors are inherently hard to reproduce, you should assert or throw whenever copy_segment_points does not initialize its out-parameters. With this kind of safeguard in place, the issue could be closed for the time being. If there is still a problem in 1.57.0, or if new problems are introduced in later versions, we will then learn soon enough.

https://svn.boost.org/trac/boost/ticket/10719

 


So reading this, I could indeed close the ticket now..

Thanks again for your message.

Regards, Barend