<div dir="ltr">evalCall-ing a function based on annotations worked for the most part except for the following scenario.<div><br></div><div>If the definition of the annotated function is <i style="font-weight:bold">after </i>it is called somewhere in the code, the RetainCountChecker is unable to "see" the annotation on this function (I checked this by printing debug messages in evalCall to see if the RetainCountChecker is able to find the annotation).</div><div><br></div><div>Consider the following examples.</div><div><b><font color="#000000"><br></font></b></div><div>
<h2></h2>
<div class="gmail-highlight" style="background:rgb(248,248,248)"><pre><b><font color="#000000">// Definition of annotated function after it is called.</font></b></pre><pre><span style="color:rgb(188,122,0)">#define NULL 0</span>
<span style="color:rgb(0,128,0);font-weight:bold">typedef</span> <span style="color:rgb(0,128,0);font-weight:bold">struct</span>
{
<span style="color:rgb(176,0,64)">int</span> ref;
} isl_basic_map;
<span style="color:rgb(176,0,64)">void</span> <span style="color:rgb(0,0,255)">free</span>(<span style="color:rgb(176,0,64)">void</span> <span style="color:rgb(102,102,102)">*</span>);
__isl_give isl_basic_map <span style="color:rgb(102,102,102)">*</span><span style="color:rgb(0,0,255)">foo</span>(__isl_take isl_basic_map <span style="color:rgb(102,102,102)">*</span>bmap);
isl_basic_map <span style="color:rgb(102,102,102)">*</span><span style="color:rgb(0,0,255)">isl_basic_map_free</span>(__isl_take isl_basic_map <span style="color:rgb(102,102,102)">*</span>bmap);
__isl_give isl_basic_map <span style="color:rgb(102,102,102)">*</span><span style="color:rgb(0,0,255)">bar</span>(__isl_take isl_basic_map <span style="color:rgb(102,102,102)">*</span>bmap) {
bmap <span style="color:rgb(102,102,102)">=</span> foo(bmap);
<span style="color:rgb(0,128,0);font-weight:bold">return</span> isl_basic_map_free(bmap); <b>// Leak warning for 'bmap' raised here.</b>
}
__attribute__((annotate(<span style="color:rgb(186,33,33)">"rc_ownership_trusted_implementation"</span>))) isl_basic_map <span style="color:rgb(102,102,102)">*</span>isl_basic_map_free(__isl_take isl_basic_map <span style="color:rgb(102,102,102)">*</span>bmap)
{
<span style="color:rgb(0,128,0);font-weight:bold">if</span> (<span style="color:rgb(102,102,102)">!</span>bmap)
<span style="color:rgb(0,128,0);font-weight:bold">return</span> <span style="color:rgb(0,128,0)">NULL</span>;
<span style="color:rgb(0,128,0);font-weight:bold">if</span> (<span style="color:rgb(102,102,102)">--</span>bmap<span style="color:rgb(102,102,102)">-></span>ref <span style="color:rgb(102,102,102)">></span> <span style="color:rgb(102,102,102)">0</span>)
<span style="color:rgb(0,128,0);font-weight:bold">return</span> <span style="color:rgb(0,128,0)">NULL</span>;
free(bmap);
<span style="color:rgb(0,128,0);font-weight:bold">return</span> <span style="color:rgb(0,128,0)">NULL</span>;
}</pre></div></div><div class="gmail_extra"><b><font color="#000000"><br></font></b></div><div class="gmail_extra">
<h2></h2>
<div class="gmail-highlight" style="background:rgb(248,248,248)"><pre><b><font color="#000000">// Definition of annotated function before it is called.</font></b></pre><pre><span style="color:rgb(188,122,0)">#define NULL 0</span>
<span style="color:rgb(0,128,0);font-weight:bold">typedef</span> <span style="color:rgb(0,128,0);font-weight:bold">struct</span>
{
<span style="color:rgb(176,0,64)">int</span> ref;
} isl_basic_map;
<span style="color:rgb(176,0,64)">void</span> <span style="color:rgb(0,0,255)">free</span>(<span style="color:rgb(176,0,64)">void</span> <span style="color:rgb(102,102,102)">*</span>);
__isl_give isl_basic_map <span style="color:rgb(102,102,102)">*</span><span style="color:rgb(0,0,255)">foo</span>(__isl_take isl_basic_map <span style="color:rgb(102,102,102)">*</span>bmap);
isl_basic_map <span style="color:rgb(102,102,102)">*</span><span style="color:rgb(0,0,255)">isl_basic_map_free</span>(__isl_take isl_basic_map <span style="color:rgb(102,102,102)">*</span>bmap);
__attribute__((annotate(<span style="color:rgb(186,33,33)">"rc_ownership_trusted_implementation"</span>))) isl_basic_map <span style="color:rgb(102,102,102)">*</span>isl_basic_map_free(__isl_take isl_basic_map <span style="color:rgb(102,102,102)">*</span>bmap)
{
<span style="color:rgb(0,128,0);font-weight:bold">if</span> (<span style="color:rgb(102,102,102)">!</span>bmap)
<span style="color:rgb(0,128,0);font-weight:bold">return</span> <span style="color:rgb(0,128,0)">NULL</span>;
<span style="color:rgb(0,128,0);font-weight:bold">if</span> (<span style="color:rgb(102,102,102)">--</span>bmap<span style="color:rgb(102,102,102)">-></span>ref <span style="color:rgb(102,102,102)">></span> <span style="color:rgb(102,102,102)">0</span>)
<span style="color:rgb(0,128,0);font-weight:bold">return</span> <span style="color:rgb(0,128,0)">NULL</span>;
free(bmap);
<span style="color:rgb(0,128,0);font-weight:bold">return</span> <span style="color:rgb(0,128,0)">NULL</span>;
}
__isl_give isl_basic_map <span style="color:rgb(102,102,102)">*</span>bar(__isl_take isl_basic_map <span style="color:rgb(102,102,102)">*</span>bmap) {
bmap <span style="color:rgb(102,102,102)">=</span> foo(bmap);
<span style="color:rgb(0,128,0);font-weight:bold">return</span> <span style="color:rgb(0,0,255)">isl_basic_map_free</span>(bmap); <b>// No leak warning for 'bmap' raised here.</b>
}
</pre></div></div><div class="gmail_extra">It would really help me if someone could point out why the RetainCountChecker behaves this way.</div><div class="gmail_extra"><br></div><div class="gmail_extra"><br></div><div class="gmail_extra"><br></div><div class="gmail_extra">Also, I have a few queries directed to Dr. Alexandre and Dr. Sven.</div><div class="gmail_extra">I applied these trusted annotations to obj_free(), obj_cow() and obj_copy() as they have a pattern (__isl.*free, __isl.*cow and __isl.*copy). </div><div class="gmail_extra"><br></div><div class="gmail_extra">Do, functions of the type obj_alloc_* have the same pattern if at all they do? Also, are there any other functions which require such annotations? </div><div class="gmail_extra"><br></div><div class="gmail_extra">I feel that I'll have to add annotations to obj_dup() as well because although adding an annotation to obj_cow() guarantees that the RetainCountChecker will not enter obj_cow's body when it is called, the analyzer might begin its analysis from obj_cow() which may lead it to call obj_dup() and result in leak warnings.</div><div class="gmail_extra"><br></div><div class="gmail_extra">Thank you.</div><div class="gmail_extra"><br></div><div class="gmail_extra"><br></div><div class="gmail_extra">Regards,</div><div class="gmail_extra">Malhar</div><div class="gmail_extra"><br><div class="gmail_quote">On Wed, Jul 12, 2017 at 7:16 PM, Alexandre Isoard <span dir="ltr"><<a href="mailto:alexandre.isoard@gmail.com" target="_blank">alexandre.isoard@gmail.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir="ltr"><div>I would include obj_alloc_* too.</div><div><br></div>More exactly, any function interacting with reference counters:<div><br></div><div>$ rc -o -R isl_map::ref</div><div>isl_map_copy<br></div><div>isl_map_cow<br></div><div>isl_map_alloc_space<br></div><div>isl_map_free</div></div><div class="gmail_extra"><div><div class="gmail-h5"><br><div class="gmail_quote">On Wed, Jul 12, 2017 at 1:42 PM, Malhar Thakkar <span dir="ltr"><<a href="mailto:cs13b1031@iith.ac.in" target="_blank">cs13b1031@iith.ac.in</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir="auto"><div><div class="gmail-m_-5927325299740534381h5"><div><br><div class="gmail_extra"><br><div class="gmail_quote">On Jul 12, 2017 10:35, "Devin Coughlin" <<a href="mailto:dcoughlin@apple.com" target="_blank">dcoughlin@apple.com</a>> wrote:<br type="attribution"><blockquote class="gmail-m_-5927325299740534381m_-7454467079152165493quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div style="word-wrap:break-word"><br><div><div class="gmail-m_-5927325299740534381m_-7454467079152165493elided-text"><blockquote type="cite"><div>On Jul 11, 2017, at 1:36 PM, Devin Coughlin via cfe-dev <<a href="mailto:cfe-dev@lists.llvm.org" target="_blank">cfe-dev@lists.llvm.org</a>> wrote:</div><br class="gmail-m_-5927325299740534381m_-7454467079152165493m_-5810994498571277456Apple-interchange-newline"><div><div style="font-family:Helvetica;font-size:12px;font-style:normal;font-variant-caps:normal;font-weight:normal;letter-spacing:normal;text-align:start;text-indent:0px;text-transform:none;white-space:normal;word-spacing:0px"><blockquote type="cite"><div><br class="gmail-m_-5927325299740534381m_-7454467079152165493m_-5810994498571277456Apple-interchange-newline">On Jul 11, 2017, at 1:12 PM, Malhar Thakkar via cfe-dev <<a href="mailto:cfe-dev@lists.llvm.org" target="_blank">cfe-dev@lists.llvm.org</a>> wrote:</div><br class="gmail-m_-5927325299740534381m_-7454467079152165493m_-5810994498571277456Apple-interchange-newline"><div><div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote">On Tue, Jul 11, 2017 at 10:49 PM, Devin Coughlin<span class="gmail-m_-5927325299740534381m_-7454467079152165493m_-5810994498571277456Apple-converted-space"> </span><span dir="ltr"><<a href="mailto:devin.coughlin@gmail.com" target="_blank">devin.coughlin@gmail<wbr>.com</a>></span><span class="gmail-m_-5927325299740534381m_-7454467079152165493m_-5810994498571277456Apple-converted-space"> </span>wrote:<br><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div style="word-wrap:break-word"><br><div><span><blockquote type="cite"><div>On Jul 11, 2017, at 5:02 AM, Malhar Thakkar via cfe-dev <<a href="mailto:cfe-dev@lists.llvm.org" target="_blank">cfe-dev@lists.llvm.org</a>> wrote:</div><br class="gmail-m_-5927325299740534381m_-7454467079152165493m_-5810994498571277456m_4204122371872823108Apple-interchange-newline"><div><div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote">On Tue, Jul 11, 2017 at 5:03 PM, Sven Verdoolaege<span class="gmail-m_-5927325299740534381m_-7454467079152165493m_-5810994498571277456Apple-converted-space"> </span><span dir="ltr"><<a href="mailto:skimo-cfe@kotnet.org" target="_blank">skimo-cfe@kotnet.<wbr>org</a>></span><span class="gmail-m_-5927325299740534381m_-7454467079152165493m_-5810994498571277456Apple-converted-space"> </span>wrote:<br><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><span class="gmail-m_-5927325299740534381m_-7454467079152165493m_-5810994498571277456m_4204122371872823108gmail-">On Tue, Jul 11, 2017 at 10:33:06AM +0530, Malhar Thakkar wrote:<br>> Hence, although evalCall() works perfectly for ISL, we may not be able to<br>> generalize it for other C codebases.<br><br></span>I think it's reasonable to assume that frameworks that shield off<br>free for reference counting, would also shield off malloc<br>in order to initialize the reference counting.<br>Of course, this may just be a lack of imagination on my part.<br>Do you have any examples of frameworks that could use your<br>annotations where this is not the case?<br></blockquote><div>Well, I haven't come across/thought of any such codebase which doesn't shield off malloc which is why I created a hypothetical test case. Now that you mention it, it does seem reasonable to assume that frameworks would shield off malloc as well.<span class="gmail-m_-5927325299740534381m_-7454467079152165493m_-5810994498571277456Apple-converted-space"> </span></div></div></div></div></div></blockquote><div><br></div></span><div>There are some frameworks/idioms that allow a transfer of ownership from a raw malloc’d pointer to a referenced-counted opaque pointer container (this is common in C++ where you use new to allocate and initialize an object). However, I think the ‘trusted implementation’ annotation could be extended to handle this as well. It would require the function that transfers ownership to the reference-counted implementation to be annotated, but I don’t think that is a high burden. That said, I wouldn’t worry about raw malloc’d pointer case for now.</div><span><div><br></div><br><blockquote type="cite"><div><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><div>Also, keeping MallocChecker aside, there are a lot of other checkers which may create some issues if there are additional side-effects (as Dr. Artem mentioned) in such annotated functions. However, I guess it may be safe to assume that there are no such additional side-effects in such "trusted" functions.</div></div></div></div></div></blockquote><div><br></div></span><div>I suspect that many of these side effects would have high-level invariants that the analyzer on its own wouldn’t be able to discover (for example, that a set created with a single item in it is not empty) — so custom modeling would be needed in any event, even if the calls were inlined.</div><div><br></div><div>Devin</div><span><div><br></div><br><blockquote type="cite"><div><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><br>skimo<br></blockquote><div><br></div><div>Now, I was experimenting a bit more with evalCall-ing based on annotations and although this works like a charm for functions of the type obj_free() and obj_cow(), it is unable to avoid the problems created by obj_copy(). This is probably because of the lack of a core-foundation annotation which is analogous to isl_keep.<br></div><div>Consider the following example.</div><div><br></div><div><div></div><br><div><div class="gmail-m_-5927325299740534381m_-7454467079152165493m_-5810994498571277456m_4204122371872823108gmail-hljs gmail-m_-5927325299740534381m_-7454467079152165493m_-5810994498571277456m_4204122371872823108gmail-cpp" style="display:block;overflow-x:auto;padding:0.5em;background-color:rgb(248,248,248)"><div><font color="#1f7199" face="monospace">#define __isl_give __attribute__((cf_returns_reta<wbr>ined))</font></div><div><font color="#1f7199" face="monospace">#define __isl_take __attribute__((cf_consumed))</font></div><div><font color="#1f7199" face="monospace">#define __isl_null</font></div><div><font color="#1f7199" face="monospace">#define __isl_keep</font></div><div><font color="#1f7199" face="monospace">#define NULL 0</font></div><div><font color="#1f7199" face="monospace"><br></font></div><div><font color="#1f7199" face="monospace">typedef struct</font></div><div><font color="#1f7199" face="monospace">{</font></div><div><font color="#1f7199" face="monospace"> int ref;</font></div><div><font color="#1f7199" face="monospace">} isl_basic_map;</font></div><div><font color="#1f7199" face="monospace"><br></font></div><div><font color="#1f7199" face="monospace">__isl_give isl_basic_map *isl_basic_map_cow(__isl_take isl_basic_map *bmap);</font></div><div><font color="#1f7199" face="monospace">__isl_null isl_basic_map *isl_basic_map_free(__isl_take isl_basic_map *bmap);</font></div><div><font color="#1f7199" face="monospace"><br></font></div><div><font color="#1f7199" face="monospace">__attribute__((annotate("rc_ow<wbr>nership_trusted_implementation<wbr>"))) __isl_give isl_basic_map *isl_basic_map_copy(__isl_keep isl_basic_map *bmap)</font></div><div><font color="#1f7199" face="monospace">{</font></div><div><font color="#1f7199" face="monospace"> if (!bmap)</font></div><div><font color="#1f7199" face="monospace"> return NULL;</font></div><div><font color="#1f7199" face="monospace"><br></font></div><div><font color="#1f7199" face="monospace"> bmap->ref++;</font></div><div><font color="#1f7199" face="monospace"> return bmap;</font></div><div><font color="#1f7199" face="monospace">}</font></div><div><font color="#1f7199" face="monospace"><br></font></div><div><font color="#1f7199" face="monospace">void test_use_after_release_with_tr<wbr>usted_implementation_annotate_<wbr>attribute(__isl_take isl_basic_map *bmap) {</font></div><div><font color="#1f7199" face="monospace"> bmap = isl_basic_map_cow(bmap);</font></div><div><font color="#1f7199" face="monospace"> isl_basic_map *temp = isl_basic_map_cow(isl_basic_ma<wbr>p_copy(bmap));<span class="gmail-m_-5927325299740534381m_-7454467079152165493m_-5810994498571277456Apple-converted-space"> </span><b>// Here, the analyzer states "Object released".</b></font></div><div><font color="#1f7199" face="monospace"> isl_basic_map *temp2 = isl_basic_map_cow(bmap);<span class="gmail-m_-5927325299740534381m_-7454467079152165493m_-5810994498571277456Apple-converted-space"> </span><b>// Use-after-release for 'bmap' raised here.</b></font></div><div><font color="#1f7199" face="monospace"> isl_basic_map_free(temp2);</font></div><div><font color="#1f7199" face="monospace"> isl_basic_map_free(temp);</font></div><div><font color="#1f7199" face="monospace">}</font></div></div></div></div></div></div></div></div></blockquote><div><br></div></span><div>I don’t understand what is going on in the above. Can you please simplify the example and explain what is going on and what you expected the analyzer to do? What call is the analyzer complaining about? Is it inlining the call? Please present the simplest example that illustrates the problem.</div></div></div></blockquote><div><br></div><div></div><br><div><div class="gmail-m_-5927325299740534381m_-7454467079152165493m_-5810994498571277456cpp gmail-m_-5927325299740534381m_-7454467079152165493m_-5810994498571277456hljs" style="display:block;overflow-x:auto;padding:0.5em;color:rgb(51,51,51);background-color:rgb(248,248,248);font-family:monospace"><div><span class="gmail-m_-5927325299740534381m_-7454467079152165493m_-5810994498571277456hljs-meta" style="color:rgb(31,113,153)">#<span class="gmail-m_-5927325299740534381m_-7454467079152165493m_-5810994498571277456hljs-meta-keyword" style="font-weight:bold">define</span><span class="gmail-m_-5927325299740534381m_-7454467079152165493m_-5810994498571277456Apple-converted-space"> </span>__isl_keep</span></div><div><span class="gmail-m_-5927325299740534381m_-7454467079152165493m_-5810994498571277456hljs-meta" style="color:rgb(31,113,153)">isl_basic_map *foo(__isl_keep isl_basic_map *bmap);</span></div></div></div><div class="gmail_quote"><br></div>Consider the above code snippet. Now, I feel that when a parameter has no annotation associated to it in some function (foo, in this case), the analyzer assumes that the object returned by that function (foo) points to the same memory location as that pointed to by the argument passed to it (bmap). Please correct me if I'm wrong.</div></div></div></div></blockquote><div><br></div><div>The analyzer shouldn’t be doing this when the foo is not inlined. If foo is inlined it should only do this if foo() returns its single parameter. Is foo() being inlined, or not? If so, what is its body? Is this when evalCall is used, or not?</div></div></div></blockquote><div><br></div></div><div>I think I understand what you’ve been describing. If you are using the evalCall currently in the retain count checker, it is modeling CFRetain(), which returns its argument. For your purposes you will not want to return the argument but instead conjure a symbol when the called function has the trusted annotation. This essentially tells the analyzer that it can’t assume anything about the return value. There is code in RetainCountChecker::evalCal<wbr>l to do this:</div><div><br></div><div> RetVal = SVB.conjureSymbolVal(nullptr, CE, LCtx, ResultTy, C.blockCount()); </div><div><br></div><div>but it only kicks in when the first argument is unknown. You’ll want to extend it to conjure a symbol whenever the evaluated call has the trusted annotation and has a return type that isn’t void.</div></div></div></blockquote></div></div></div></div></div><div dir="auto">Yes, this is exactly what I needed. I added that functionality and it seems to be working now. So, just to make sure everything's working as expected, I am currently running the static analyzer again on the ISL codebase after adding trusted implementation annotations to obj_free(), obj_cow() and obj_copy().</div><div dir="auto"><div class="gmail_extra"><div class="gmail_quote"><blockquote class="gmail-m_-5927325299740534381m_-7454467079152165493quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div style="word-wrap:break-word"><div><div>Hope this helps,</div><div><br></div><div>Devin</div></div></div></blockquote></div><br></div></div></div>
</blockquote></div><br><br clear="all"><div><br></div></div></div><span class="gmail-HOEnZb"><font color="#888888">-- <br><div class="gmail-m_-5927325299740534381gmail_signature"><div dir="ltr"><b>Alexandre Isoard</b><br></div></div>
</font></span></div>
</blockquote></div><br></div></div><div hspace="streak-pt-mark" style="max-height:1px"><img alt="" style="width:0px;max-height:0px;overflow:hidden" src="https://mailfoogae.appspot.com/t?sender=aY3MxM2IxMDMxQGlpdGguYWMuaW4%3D&type=zerocontent&guid=b6809d81-853c-4f0d-b37f-f3b5e666c7b1"><font color="#ffffff" size="1">ᐧ</font></div>