<div dir="ltr"><div><div>Hi Filip,<br><br></div>Sorry for the delay. I made the fixes you pointed out and pushed them to trunk and stable. The fixes for CoinUtils have already been released along with some other fixes I made. I will have a new release of SYMPHONY incorporating everything in a day or two. Thanks for the feedback!<br><br></div>Cheers,<br><br>Ted<br></div><div class="gmail_extra"><br><div class="gmail_quote">On Thu, Sep 25, 2014 at 12:52 PM, Ted Ralphs <span dir="ltr"><<a href="mailto:ted@lehigh.edu" target="_blank">ted@lehigh.edu</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div>Thanks for the report! I haven't had a chance to fix these issues yet, but I will do so soon.<br><br></div>Cheers,<br><br>Ted<br></div><div class="gmail_extra"><br><div class="gmail_quote"><div><div class="h5">On Tue, Sep 16, 2014 at 7:03 AM, Konvicka Filip <span dir="ltr"><<a href="mailto:Filip.Konvicka@logis.cz" target="_blank">Filip.Konvicka@logis.cz</a>></span> wrote:<br></div></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div><div class="h5">
<div link="blue" vlink="purple" lang="CS">
<div>
<p class="MsoNormal"><span lang="EN-US">Hi,<u></u><u></u></span></p>
<p class="MsoNormal"><span lang="EN-US"><u></u> <u></u></span></p>
<p class="MsoNormal"><span lang="EN-US">While cleaning some warnings I stumbled upon several very suspicious lines of code in Symphony 5.5.7.<u></u><u></u></span></p>
<p class="MsoNormal"><span lang="EN-US"><u></u> <u></u></span></p>
<p class="MsoNormal"><span lang="EN-US">Note that item 1 is a change in the headers, and unless this is fixed this causes warnings in programs that link libsymphony.<u></u><u></u></span></p>
<p class="MsoNormal"><span lang="EN-US">Items 2 - 4 are possible bugs in Symphony.<u></u><u></u></span></p>
<p class="MsoNormal"><span lang="EN-US"><u></u> <u></u></span></p>
<p class="MsoNormal"><span lang="EN-US">1. In OsiSolverInterface.hpp and OsiClpSolverInterface.hpp, there are functions defined with the default parameter values of std::string::npos. The type of this is size_t, but the argument for which the default is defined
is unsigned. In 64bit builds, this leads to a truncation of the value.<u></u><u></u></span></p>
<p class="MsoNormal"><span lang="EN-US"><u></u> <u></u></span></p>
<p class="MsoNormal" style="text-autospace:none"><span style="font-size:9.5pt;font-family:Consolas;color:black;background:white">
</span><span style="font-size:9.5pt;font-family:Consolas;color:blue;background:white">virtual</span><span style="font-size:9.5pt;font-family:Consolas;color:black;background:white"> std::string getRowName(</span><span style="font-size:9.5pt;font-family:Consolas;color:blue;background:white">int</span><span style="font-size:9.5pt;font-family:Consolas;color:black;background:white">
rowIndex,<u></u><u></u></span></p>
<p class="MsoNormal" style="text-autospace:none"><span style="font-size:9.5pt;font-family:Consolas;color:black;background:white">
</span><span style="font-size:9.5pt;font-family:Consolas;color:blue;background:white">unsigned</span><span style="font-size:9.5pt;font-family:Consolas;color:black;background:white"> maxLen =
</span><span style="font-size:9.5pt;font-family:Consolas;color:blue;background:white">static_cast</span><span style="font-size:9.5pt;font-family:Consolas;color:black;background:white"><</span><span style="font-size:9.5pt;font-family:Consolas;color:blue;background:white">unsigned</span><span style="font-size:9.5pt;font-family:Consolas;color:black;background:white">>(std::string::npos))
</span><span style="font-size:9.5pt;font-family:Consolas;color:blue;background:white">const</span><span style="font-size:9.5pt;font-family:Consolas;color:black;background:white">;<u></u><u></u></span></p>
<p class="MsoNormal"><u></u> <u></u></p>
<p class="MsoNormal">I think that it is better to use<u></u><u></u></p>
<p class="MsoNormal"><u></u> <u></u></p>
<p class="MsoNormal" style="text-autospace:none"><span style="font-size:9.5pt;font-family:Consolas;color:black;background:white">
</span><span style="font-size:9.5pt;font-family:Consolas;color:blue;background:white">virtual</span><span style="font-size:9.5pt;font-family:Consolas;color:black;background:white"> std::string getRowName(</span><span style="font-size:9.5pt;font-family:Consolas;color:blue;background:white">int</span><span style="font-size:9.5pt;font-family:Consolas;color:black;background:white">
rowIndex,<u></u><u></u></span></p>
<p class="MsoNormal" style="text-autospace:none"><span style="font-size:9.5pt;font-family:Consolas;color:black;background:white">
</span><span style="font-size:9.5pt;font-family:Consolas;color:blue;background:white">unsigned</span><span style="font-size:9.5pt;font-family:Consolas;color:black;background:white"> maxLen = std::numeric_limits<</span><span style="font-size:9.5pt;font-family:Consolas;color:blue;background:white">unsigned</span><span style="font-size:9.5pt;font-family:Consolas;color:black;background:white">>::max())
</span><span style="font-size:9.5pt;font-family:Consolas;color:blue;background:white">const</span><span style="font-size:9.5pt;font-family:Consolas;color:black;background:white">;<u></u><u></u></span></p>
<p class="MsoNormal"><u></u> <u></u></p>
<p class="MsoNormal">If you look at how COIN_INT_MAX is defined in CoinFinite.hpp, this is seems to be an acceptable COIN coding practice. There are 5 occurrences in total of this (in
<span lang="EN-US">OsiSolverInterface.hpp and OsiClpSolverInterface.hpp). The only use of this that I could find is OsiNames.cpp which uses this as a limit to std::string::substr. Also this change should not break the ABI since this does not affect the symbols.</span><u></u><u></u></p>
<p class="MsoNormal"><u></u> <u></u></p>
<p class="MsoNormal"><span lang="EN-US">2. In lp_heuristics.c, line 1410:<u></u><u></u></span></p>
<p class="MsoNormal"><span lang="EN-US"><u></u> <u></u></span></p>
<p class="MsoNormal" style="text-autospace:none"><span style="font-size:9.5pt;font-family:Consolas;color:black;background:white">
</span><span style="font-size:9.5pt;font-family:Consolas;color:blue;background:white">if</span><span style="font-size:9.5pt;font-family:Consolas;color:black;background:white">(dive_depth > dive_depth_limit/2){
<u></u><u></u></span></p>
<p class="MsoNormal" style="text-autospace:none"><span style="font-size:9.5pt;font-family:Consolas;color:black;background:white"> fix_incr_cnt =(</span><span style="font-size:9.5pt;font-family:Consolas;color:blue;background:white">int</span><span style="font-size:9.5pt;font-family:Consolas;color:black;background:white">)(1.0*(frac_ip_cnt
- d_fixed_cnt)/<u></u><u></u></span></p>
<p class="MsoNormal" style="text-autospace:none"><span style="font-size:9.5pt;font-family:Consolas;color:black;background:white"> (dive_depth_limit > (dive_depth ? dive_depth_limit - dive_depth : 1)));<u></u><u></u></span></p>
<p class="MsoNormal" style="text-autospace:none"><span style="font-size:9.5pt;font-family:Consolas;color:black;background:white">
</span><span style="font-size:9.5pt;font-family:Consolas;color:blue;background:white">if</span><span style="font-size:9.5pt;font-family:Consolas;color:black;background:white">(fix_incr_cnt < 1) fix_incr_cnt = 1;<u></u><u></u></span></p>
<p class="MsoNormal" style="text-autospace:none"><span style="font-size:9.5pt;font-family:Consolas;color:black;background:white"> }<u></u><u></u></span></p>
<p class="MsoNormal"><span lang="EN-US"><u></u> <u></u></span></p>
<p class="MsoNormal"><span lang="EN-US">Look at the division. The divider is a bool value!
</span><span style="font-size:9.5pt;font-family:Consolas;color:black;background:white">(dive_depth_limit > (dive_depth ? dive_depth_limit - dive_depth : 1))</span><span style="font-size:9.5pt;font-family:Consolas;color:black"><u></u><u></u></span></p>
<p class="MsoNormal"><span lang="EN-US">I think that most likely this should have been (dive_depth_limit > dive_depth) ? dive_depth_limit-dive_depth : 1, which by the way is equivalent to std::max(dive_depth_limit - dive_depth, 1).</span><span style="font-size:9.5pt;font-family:Consolas;color:black"><u></u><u></u></span></p>
<p class="MsoNormal"><span lang="EN-US"><u></u> <u></u></span></p>
<p class="MsoNormal" style="text-autospace:none"><span style="font-size:9.5pt;font-family:Consolas;color:black;background:white">
</span><span style="font-size:9.5pt;font-family:Consolas;color:blue;background:white">if</span><span style="font-size:9.5pt;font-family:Consolas;color:black;background:white">(dive_depth > dive_depth_limit/2){<u></u><u></u></span></p>
<p class="MsoNormal" style="text-autospace:none"><span style="font-size:9.5pt;font-family:Consolas;color:black;background:white"> fix_incr_cnt =(</span><span style="font-size:9.5pt;font-family:Consolas;color:blue;background:white">int</span><span style="font-size:9.5pt;font-family:Consolas;color:black;background:white">)(1.0*(frac_ip_cnt
- d_fixed_cnt)/<u></u><u></u></span></p>
<p class="MsoNormal" style="text-autospace:none"><span style="font-size:9.5pt;font-family:Consolas;color:black;background:white"> ((dive_depth_limit > dive_depth) ? dive_depth_limit - dive_depth : 1));<u></u><u></u></span></p>
<p class="MsoNormal" style="text-autospace:none"><span style="font-size:9.5pt;font-family:Consolas;color:black;background:white">
</span><span style="font-size:9.5pt;font-family:Consolas;color:blue;background:white">if</span><span style="font-size:9.5pt;font-family:Consolas;color:black;background:white">(fix_incr_cnt < 1) fix_incr_cnt = 1;<u></u><u></u></span></p>
<p class="MsoNormal" style="text-autospace:none"><span style="font-size:9.5pt;font-family:Consolas;color:black;background:white"> }<u></u><u></u></span></p>
<p class="MsoNormal"><span lang="EN-US"><u></u> <u></u></span></p>
<p class="MsoNormal"><span lang="EN-US">By the way, the warning that caught the compiler's attention was - lp_heuristics.c(1412): warning C4804: '/' : unsafe use of type 'bool' in operation<u></u><u></u></span></p>
<p class="MsoNormal"><span lang="EN-US"><u></u> <u></u></span></p>
<p class="MsoNormal"><span lang="EN-US">3. In master_prep_base.c:504, I can also see something very suspicious:<u></u><u></u></span></p>
<p class="MsoNormal"><span lang="EN-US"><u></u> <u></u></span></p>
<p class="MsoNormal" style="text-autospace:none"><span style="font-size:9.5pt;font-family:Consolas;color:black;background:white">
</span><span style="font-size:9.5pt;font-family:Consolas;color:blue;background:white">if</span><span style="font-size:9.5pt;font-family:Consolas;color:black;background:white">(</span><span style="font-size:9.5pt;font-family:Consolas;color:gray;background:white">P</span><span style="font-size:9.5pt;font-family:Consolas;color:black;background:white">->ub
< (</span><span style="font-size:9.5pt;font-family:Consolas;color:gray;background:white">P</span><span style="font-size:9.5pt;font-family:Consolas;color:black;background:white">->mip->obj_sense ==
</span><span style="font-size:9.5pt;font-family:Consolas;color:#6f008a;background:white">SYM_MAXIMIZE</span><span style="font-size:9.5pt;font-family:Consolas;color:black;background:white">) ? -(</span><span style="font-size:9.5pt;font-family:Consolas;color:gray;background:white">P</span><span style="font-size:9.5pt;font-family:Consolas;color:black;background:white">->mip->obj_offset)
: <u></u><u></u></span></p>
<p class="MsoNormal" style="text-autospace:none"><span style="font-size:9.5pt;font-family:Consolas;color:black;background:white"> </span><span style="font-size:9.5pt;font-family:Consolas;color:gray;background:white">P</span><span style="font-size:9.5pt;font-family:Consolas;color:black;background:white">->mip->obj_offset){<u></u><u></u></span></p>
<p class="MsoNormal"><span lang="EN-US"><u></u> <u></u></span></p>
<p class="MsoNormal"><span lang="EN-US">This is actually evaluated as (P->ub < (P->mip->obj_sense == SYM_MAXIMIZE)) ? -(P->mip->obj_offset) : P->mip->obj_offset, but obviously the intention was<u></u><u></u></span></p>
<p class="MsoNormal"><span lang="EN-US"><u></u> <u></u></span></p>
<p class="MsoNormal" style="text-autospace:none"><span style="font-size:9.5pt;font-family:Consolas;color:black;background:white">
</span><span style="font-size:9.5pt;font-family:Consolas;color:blue;background:white">if</span><span style="font-size:9.5pt;font-family:Consolas;color:black;background:white">(</span><span style="font-size:9.5pt;font-family:Consolas;color:gray;background:white">P</span><span style="font-size:9.5pt;font-family:Consolas;color:black;background:white">->ub
< ((</span><span style="font-size:9.5pt;font-family:Consolas;color:gray;background:white">P</span><span style="font-size:9.5pt;font-family:Consolas;color:black;background:white">->mip->obj_sense ==
</span><span style="font-size:9.5pt;font-family:Consolas;color:#6f008a;background:white">SYM_MAXIMIZE</span><span style="font-size:9.5pt;font-family:Consolas;color:black;background:white">) ? -(</span><span style="font-size:9.5pt;font-family:Consolas;color:gray;background:white">P</span><span style="font-size:9.5pt;font-family:Consolas;color:black;background:white">->mip->obj_offset)
: <u></u><u></u></span></p>
<p class="MsoNormal" style="text-autospace:none"><span style="font-size:9.5pt;font-family:Consolas;color:black;background:white"> </span><span style="font-size:9.5pt;font-family:Consolas;color:gray;background:white">P</span><span style="font-size:9.5pt;font-family:Consolas;color:black;background:white">->mip->obj_offset)){<u></u><u></u></span></p>
<p class="MsoNormal"><span lang="EN-US"><u></u> <u></u></span></p>
<p class="MsoNormal"><span lang="EN-US">Again the warning was master_prep_base.c(504): warning C4804: '<' : unsafe use of type 'bool' in operation<u></u><u></u></span></p>
<p class="MsoNormal"><span lang="EN-US"><u></u> <u></u></span></p>
<p class="MsoNormal"><span lang="EN-US">4. In master.c:231 and 233, there are assignments like this:<u></u><u></u></span></p>
<p class="MsoNormal"><span lang="EN-US"><u></u> <u></u></span></p>
<p class="MsoNormal" style="text-autospace:none"><span style="font-size:9.5pt;font-family:Consolas;color:black;background:white"> tm_par->warm_start_node_limit = (</span><span style="font-size:9.5pt;font-family:Consolas;color:blue;background:white">int</span><span style="font-size:9.5pt;font-family:Consolas;color:black;background:white">)</span><span style="font-size:9.5pt;font-family:Consolas;color:#6f008a;background:white">SYM_INFINITY</span><span style="font-size:9.5pt;font-family:Consolas;color:black;background:white">;
<u></u><u></u></span></p>
<p class="MsoNormal"><u></u> <u></u></p>
<p class="MsoNormal"><span lang="EN-US">warm_start_node_limit is an int, while SYM_INFINITY is defined as 1e20 (i.e. a double). This does not make much sense (the conversion results in some strange value), so I think that it's much safer to write<u></u><u></u></span></p>
<p class="MsoNormal"><span lang="EN-US"><u></u> <u></u></span></p>
<p class="MsoNormal" style="text-autospace:none"><span style="font-size:9.5pt;font-family:Consolas;color:black;background:white"> tm_par->warm_start_node_limit =
</span><span style="font-size:9.5pt;font-family:Consolas;color:#6f008a;background:white">INT_MAX</span><span style="font-size:9.5pt;font-family:Consolas;color:black;background:white">;
<u></u><u></u></span></p>
<p class="MsoNormal" style="text-autospace:none"><span style="font-size:9.5pt;font-family:Consolas;color:black;background:white"><u></u> <u></u></span></p>
<p class="MsoNormal" style="text-autospace:none"><span lang="EN-US">Note that 1e20 mod (UINT_MAX+1) is the number that gets assigned, which is 1661992960 for 4byte ints but zero for 2byte ints. Lucky we don't have small ints anymore :-)<u></u><u></u></span></p>
<p class="MsoNormal" style="text-autospace:none"><span lang="EN-US"><u></u> <u></u></span></p>
<p class="MsoNormal" style="text-autospace:none"><span lang="EN-US">Cheers,<u></u><u></u></span></p>
<p class="MsoNormal" style="text-autospace:none"><span lang="EN-US">Filip<u></u><u></u></span></p>
<p class="MsoNormal" style="text-autospace:none"><span style="font-size:9.5pt;font-family:Consolas;color:black;background:white"><u></u> <u></u></span></p>
</div>
</div>
<br></div></div>_______________________________________________<br>
Symphony mailing list<br>
<a href="mailto:Symphony@list.coin-or.org" target="_blank">Symphony@list.coin-or.org</a><br>
<a href="http://list.coin-or.org/mailman/listinfo/symphony" target="_blank">http://list.coin-or.org/mailman/listinfo/symphony</a><br>
<br></blockquote></div><span class="HOEnZb"><font color="#888888"><br><br clear="all"><br>-- <br>Dr. Ted Ralphs<br>Associate Professor, Lehigh University<br><a href="tel:%28610%29%20628-1280" value="+16106281280" target="_blank">(610) 628-1280</a><br>ted 'at' lehigh 'dot' edu<br><a href="http://coral.ie.lehigh.edu/~ted" target="_blank">coral.ie.lehigh.edu/~ted</a><br>
</font></span></div>
</blockquote></div><br><br clear="all"><br>-- <br><div dir="ltr">Dr. Ted Ralphs<br>Professor, Lehigh University<br>(610) 628-1280<br>ted 'at' lehigh 'dot' edu<br><a href="http://coral.ie.lehigh.edu/~ted" target="_blank">coral.ie.lehigh.edu/~ted</a><br></div>
</div>