Boost C++ Libraries: Ticket #5143: segfaults in ordered_index https://svn.boost.org/trac10/ticket/5143 <p> Hi, </p> <p> we are suffering segfaults in ordered_index_node_impl::increment line 254 </p> <p> static void increment(pointer&amp; x) { </p> <blockquote> <p> if(x-&gt;right()!=pointer(0)){ </p> <blockquote> <p> x=x-&gt;right(); while(x-&gt;left()!=pointer(0))x=x-&gt;left(); <em> XXX </em></p> </blockquote> <p> } else{ </p> <blockquote> <p> pointer y=x-&gt;parent(); while(x==y-&gt;right()){ </p> <blockquote> <p> x=y; y=y-&gt;parent(); </p> </blockquote> <p> } if(x-&gt;right()!=y)x=y; </p> </blockquote> <p> } </p> </blockquote> <p> } </p> <p> What, if x-&gt;left() becomes 0? Well, it does, over here! Shouldn't this read </p> <blockquote> <p> while(x!=pointer(0)&amp;&amp;x-&gt;left()!=pointer(0))x=x-&gt;left(); </p> </blockquote> <p> ? What is this line of code supposed to do anyway, other than uselessly consuming CPU time? Why not write </p> <blockquote> <p> x = 0; </p> </blockquote> <p> instead, if this was meant? ordered_index_node_impl_base::left and ordered_index_node_impl_base::right do not have any side-effects. </p> <p> By the way, for the classes ordered_index_node_std_base and ordered_index_node_compressed_base, I don't see any of the POD data members getting initialized. A user-provided constructor is not defined. From the C++ Standard, IIRC, POD data members are not initialized by the implicitly-generated constructor and therefore have indeterminate values. </p> <p> This also applies to recent versions of boost, the trunk included. The code doesn't appear to have changed here. </p> <p> To me this looks like this is working only by chance. Has anyone ever bothered to review this code? </p> <p> Cheers, </p> <blockquote> <p> Christoph </p> </blockquote> en-us Boost C++ Libraries /htdocs/site/boost.png https://svn.boost.org/trac10/ticket/5143 Trac 1.4.3 Joaquín M López Muñoz Mon, 31 Jan 2011 11:25:04 GMT status changed; resolution set https://svn.boost.org/trac10/ticket/5143#comment:1 https://svn.boost.org/trac10/ticket/5143#comment:1 <ul> <li><strong>status</strong> <span class="trac-field-old">new</span> → <span class="trac-field-new">closed</span> </li> <li><strong>resolution</strong> → <span class="trac-field-new">invalid</span> </li> </ul> <p> <em>What, if x-&gt;left() becomes 0?</em> </p> <p> If x-&gt;left() is 0, then x=x-&gt;left() is not executed and x retains a non-null value (specifically, x points to a node whose left child is 0). </p> <p> <em>Well, it does, over here! </em> </p> <p> Can you provide a complete test program showing the effect? Without any other info, my bet is the internal rb-tree has been corrupted by some bug in your program. </p> <p> <em>Shouldn't this read </em></p> <blockquote> <p> while(x!=pointer(0)&amp;&amp;x-&gt;left()!=pointer(0))x=x-&gt;left();<em> </em></p> </blockquote> <p> No; note that just before the while loop we have </p> <blockquote> <p> if(x-&gt;right()!=pointer(0)){ </p> <blockquote> <p> x=x-&gt;right(); ... </p> </blockquote> </blockquote> <p> So x can't be null and the extra check inside the loop is redundant. </p> <p> <em>What is this line of code supposed to do anyway, other than uselessly consuming CPU time? Why not write</em> </p> <blockquote> <p> x = 0; </p> </blockquote> <p> As explained above, x won't be null after executing the while loop. </p> <p> <em>ordered_index_node_impl_base::left and ordered_index_node_impl_base::right do not have any side-effects.</em> </p> <p> This is supposedly not related to your problem, but: what's the problem with those member functions not having side effects? </p> <p> <em>By the way, for the classes ordered_index_node_std_base and ordered_index_node_compressed_base, I don't see any of the POD data members getting initialized. A user-provided constructor is not defined. From the C++ Standard, IIRC, POD data members are not initialized by the implicitly-generated constructor and therefore have indeterminate values.</em> </p> <p> This is correct. Initialization takes place at points in time other than construction, for instance in ordered_index::empty_initialize() and ordered_index_node_impl::link(). It is not an error to let POD values be uninitialized as long as they are actually initialized before being read. </p> <p> <em>To me this looks like this is working only by chance. Has anyone ever bothered to review this code?</em> </p> <p> This is slightly offensive but, as it does not contain either info on your problem or any other useful suggestion, I'll just ignore it. </p> <p> I'm closing this as "invalid" (i.e. no bug detected). If you can come up with a test program where the crash happens please reopen the ticket and attach the relevant info. Thank you! </p> Ticket