On Feb 8, 2013, at 6:13 AM, Branden Archer <[email protected]> wrote:
> Anna, > > Thanks for committing these! > > Who should I contact to have the LeakPtrValChanged section of the following > page updated/removed, as the checker is now in? > http://clang-analyzer.llvm.org/potential_checkers.html > Just send in a patch. The html page is in the clang repository. Anna. > - Branden > > On Thu, Feb 7, 2013 at 6:09 PM, Anna Zaks <[email protected]> wrote: > Branden, > > Thanks for working on this and polishing it up! I've committed the changes in > r174677 and r 174678. > > 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. > > Cheers, > Anna. > On Feb 3, 2013, at 8:10 PM, Branden Archer <[email protected]> wrote: > >> Here is the commit message for the first patch: >> [analyzer] Pass pointer escape type to checkPointerEscape checker >> >> The checkPointerEscape checker previously did not specify how a >> pointer escaped. This change includes an enum which describes the >> different ways a pointer may escape. This enum is passed to the >> checkPointerEscape checker when a pointer escapes. If the escape >> is due to a function call, the call is passed. This changes >> previous behavior where the call is passed as NULL if the escape >> was due to indirectly invalidating the region the pointer referenced. >> >> Here is the commit message for the second patch: >> [analyzer] malloc checker reports bugs when freeing memory with offset >> pointer >> >> The malloc checker will now catch the case when a previously malloc'ed >> region is freed, but the pointer passed to free does not point to the >> start of the allocated memory. For example: >> >> int *p1 = malloc(sizeof(int)); >> p1++; >> free(p1); // warn >> >> From the "memory.LeakPtrValChanged enhancement to unix.Malloc" entry >> in the list of potential checkers. >> >> >> 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. >> >> 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. >> >> - Branden >> >> On Fri, Feb 1, 2013 at 1:20 PM, Anna Zaks <[email protected]> wrote: >> >> On Jan 31, 2013, at 7:20 PM, Branden Archer <[email protected]> wrote: >> >>> 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? >>> >> >> One of us will push this and mention that the patch is by you in the commit >> message. Please do send the commit message! >> >>> 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. >>> >>> - Branden >>> >>> On Thu, Jan 31, 2013 at 10:10 PM, Branden Archer <[email protected]> >>> wrote: >>> Have you double checked that the tests did not generate the warnings before >>> the patch? >>> >>> 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? >>> >> >> 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. >> >>> >>> +void testPassConstPointerIndirectly() { >>> + struct HasPtr hp; >>> + hp.p = fopen("myfile.txt", "w"); >>> + fputc(0, (FILE *)&hp); >>> + return; // expected-warning {{Opened file is never closed; potential >>> resource leak}} >>> +} >>> >>> Heh. Did you really want this test case? It's not actually valid (&hp is a >>> FILE**, not a FILE*): >>> >>> 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). >>> >>> - Branden >>> >>> >>> On Thu, Jan 31, 2013 at 8:26 PM, Jordan Rose <[email protected]> wrote: >>> >>> On Jan 31, 2013, at 5:21 , Branden Archer <[email protected]> wrote: >>> >>>> 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. >>>> >>>> Previously, we would have a false negative - no leak would be reported. >>>> Now, we should be catching the leak. >>>> >>>> Ah, got you. See the first attached patch for these added cases. >>>> >>>> - Branden >>> >>> >>> +void testPassConstPointerIndirectly() { >>> + struct HasPtr hp; >>> + hp.p = fopen("myfile.txt", "w"); >>> + fputc(0, (FILE *)&hp); >>> + return; // expected-warning {{Opened file is never closed; potential >>> resource leak}} >>> +} >>> >>> Heh. Did you really want this test case? It's not actually valid (&hp is a >>> FILE**, not a FILE*): >>> >>> >>> A few remaining comments for the MallocChecker patch: >>> >>> + if (ExplodedNode *N = C.generateSink()) { >>> >>> Please use an early return here. >>> >>> >>> + int offsetBytes = Offset.getOffset()/C.getASTContext().getCharWidth(); >>> >>> Very nitpicky, but can you put spaces around the /? >>> >>> >>> + << ((abs(offsetBytes) > 1) ? "bytes" : "byte") >>> >>> Perfect! >>> >>> Jordan >>> >>> >> >> >> <01-modify-checkPointerEscape.patch><02-update-malloc-checker.patch> > >
_______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
