Anna,<br><br>Thanks for committing these!<br><br>Who should I contact to have the <span class="name">LeakPtrValChanged section of the following page updated/removed, as the checker is now in?</span><br><a href="http://clang-analyzer.llvm.org/potential_checkers.html">http://clang-analyzer.llvm.org/potential_checkers.html</a><br>
<br>- Branden<br><br><div class="gmail_quote">On Thu, Feb 7, 2013 at 6:09 PM, Anna Zaks <span dir="ltr"><<a href="mailto:ganna@apple.com" target="_blank">ganna@apple.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0pt 0pt 0pt 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<div style="word-wrap:break-word">Branden,<div><br></div><div>Thanks for working on this and polishing it up! I've committed the changes in r174677 and r 174678.</div><div><br></div><div>I had to change the tests from the first patch slightly since we've made analyzer more pessimistic as a quick fix to a regression in r174468. This resulted in the expected leaks not being reported.</div>
<div><br></div><div>Cheers,</div><div>Anna.<br><div><div><div class="h5"><div>On Feb 3, 2013, at 8:10 PM, Branden Archer <<a href="mailto:b.m.archer4@gmail.com" target="_blank">b.m.archer4@gmail.com</a>> wrote:</div>
<br></div></div><blockquote type="cite"><div><div class="h5">Here is the commit message for the first patch:<br><span style="font-family:courier new,monospace">[analyzer] Pass pointer escape type to checkPointerEscape checker<br>
<br>The checkPointerEscape checker previously did not specify how a<br>
pointer escaped. This change includes an enum which describes the<br>different ways a pointer may escape. This enum is passed to the<br>checkPointerEscape checker when a pointer escapes. If the escape<br>is due to a function call, the call is passed. This changes<br>
previous behavior where the call is passed as NULL if the escape<br>was due to indirectly invalidating the region the pointer referenced.</span><br><br>Here is the commit message for the second patch:<br><span style="font-family:courier new,monospace">[analyzer] malloc checker reports bugs when freeing memory with offset pointer<br>
<br>The malloc checker will now catch the case when a previously malloc'ed<br>region is freed, but the pointer passed to free does not point to the<br>start of the allocated memory. For example:<br> <br>int *p1 = malloc(sizeof(int));<br>
p1++;<br>free(p1); // warn<br> <br>From the "memory.LeakPtrValChanged enhancement to unix.Malloc" entry<br>in the list of potential checkers.</span><br><br><br><blockquote style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex" class="gmail_quote">
What I meant is that the tests should not pass before the patch is
applied. Then, after you apply the patch, the tests should start
passing.<br></blockquote><br>Thanks for the reminder. The other tests for the second patch checked out, but what I was trying for the first patch was not. I decided to take a more direct approach to testing the first patch. Instead of trying to figure out a system header function that would take a struct containing a pointer (or for the simple stream checker, a FILE pointer), I decided to 'create my own' by adding a fake system header function in the header included in the test files. In doing so, I can easily see that previous to the change there was a false negative, but after the change a bug was found.<br>
<br>- Branden<br><br><div class="gmail_quote">
On Fri, Feb 1, 2013 at 1:20 PM, Anna Zaks <span dir="ltr"><<a href="mailto:ganna@apple.com" target="_blank">ganna@apple.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0pt 0pt 0pt 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<div style="word-wrap:break-word"><br><div><div><div>On Jan 31, 2013, at 7:20 PM, Branden Archer <<a href="mailto:b.m.archer4@gmail.com" target="_blank">b.m.archer4@gmail.com</a>> wrote:</div><br><blockquote type="cite">
Oh, one other thing. I do not know who does the committing when the patch is approved. Should I prove a patch that has my commit message in it and username? Would either of you be pushing the change, or would I be pushing it?<br>
<br></blockquote><div><br></div></div>One of us will push this and mention that the patch is by you in the commit message. Please do send the commit message!</div><div><div><br><blockquote type="cite">Also, thanks for your comments and discussion on this patch, and for keeping with it for so long! I appreciate your feedback and the opportunity to learn some about the static analyzer of clang.<br>
<br>- Branden<br><br>
<div class="gmail_quote">On Thu, Jan 31, 2013 at 10:10 PM, Branden Archer <span dir="ltr"><<a href="mailto:b.m.archer4@gmail.com" target="_blank">b.m.archer4@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><blockquote style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex" class="gmail_quote">Have you double checked that the tests did not generate the warnings before the patch?<br>
</blockquote>
<br></div>When you mentioned this, I had a moment of doubt. I checked for compiler warnings and the 'make test' before I emailed my patches, and all came back clean. Just to be sure I updated my source and compiled again, all still clean. Did you see something in particular, or just wanted to make sure?<div>
<br></div></blockquote></div></blockquote><div><br></div></div>What I meant is that the tests should not pass before the patch is applied. Then, after you apply the patch, the tests should start passing.</div><div>
<div><br><blockquote type="cite"><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">
<div>
<br><blockquote style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex" class="gmail_quote"><div><div>+void testPassConstPointerIndirectly() {</div><div>+ struct HasPtr hp;</div><div>+ hp.p = fopen("myfile.txt", "w");</div>
<div>+ fputc(0, (FILE *)&hp);</div><div>+ return; // expected-warning {{Opened file is never closed; potential resource leak}}</div><div>+}</div></div><div><br></div><div>Heh. Did you really want this test case? It's not actually valid (&hp is a FILE**, not a FILE*):</div>
</blockquote></div><div><br>I knew the test was not proper code, but it was the only way I could think of to pass a structure to a known library function that was known to not close a file. I replaced it in the attached patches with passing the HasPtr structure to a function that accepts it as a const parameter. I am not sure this still tests the same thing (as I do not fully appreciate how the analyzer knows that a function will not close the stream. I am hoping that if the parameter is passed as a const that it will assume this).<span><font color="#888888"><br>
</font></span></div><span><font color="#888888"><br>- Branden</font></span><div><br><br><div class="gmail_quote">On Thu, Jan 31, 2013 at 8:26 PM, Jordan Rose <span dir="ltr"><<a href="mailto:jordan_rose@apple.com" target="_blank">jordan_rose@apple.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0pt 0pt 0pt 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<div style="word-wrap:break-word"><div><br><div><div>On Jan 31, 2013, at 5:21 , Branden Archer <<a href="mailto:b.m.archer4@gmail.com" target="_blank">b.m.archer4@gmail.com</a>> wrote:</div><br><blockquote type="cite">
<blockquote style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex" class="gmail_quote"><div>Basically, you need to pass a pointer which we are tracking to a
function call indirectly (ex: as a field in a struct..). You should pass
it to a function which is known not to free memory or close stream.
Finally, you leak that resource/pointer.</div><div><br></div><div>Previously, we would have a false negative - no leak would be reported. Now, we should be catching the leak.</div></blockquote><br>Ah, got you. See the first attached patch for these added cases.<br>
<br>- Branden<br></blockquote></div><div><br></div></div><div><div>+void testPassConstPointerIndirectly() {</div><div>+ struct HasPtr hp;</div><div>+ hp.p = fopen("myfile.txt", "w");</div><div>+ fputc(0, (FILE *)&hp);</div>
<div>+ return; // expected-warning {{Opened file is never closed; potential resource leak}}</div><div>+}</div></div><div><br></div><div>Heh. Did you really want this test case? It's not actually valid (&hp is a FILE**, not a FILE*):</div>
<div><br></div><div><br></div><div>A few remaining comments for the MallocChecker patch:</div><div><br></div><div>+ if (ExplodedNode *N = C.generateSink()) {</div><div><br></div><div>Please use an early return here.</div>
<div><br></div><div><br></div><div>+ int offsetBytes = Offset.getOffset()/C.getASTContext().getCharWidth();</div><div><br></div><div><i>Very</i> nitpicky, but can you put spaces around the /?</div><div><br>
</div>
<div><br></div><div>+ << ((abs(offsetBytes) > 1) ? "bytes" : "byte")</div><div><br></div><div>Perfect!</div><span><font color="#888888"><div><br></div><div>Jordan</div>
</font></span></div></blockquote></div><br>
</div></blockquote></div><br>
</blockquote></div><br></div></div>
</blockquote></div><br>
</div></div><span><01-modify-checkPointerEscape.patch></span><span><02-update-malloc-checker.patch></span></blockquote></div><br></div></div></blockquote></div><br>