Adds the kfree( ) function to MallocChecker.cpp
Details
Diff Detail
Event Timeline
Change the test file kmalloc-linux.c to use kfree( ). Sorry if there was a way to put both patch files together; I wasn't able to find one.
Thanks for the patches! I've commandeered this revision to be able to update the diff to merge the two patch files into one. You should commandeer it back by using the 'Commandeer Revision' action.
By the way, there are instructions for how to create and upload diffs using Phabricator at http://llvm.org/docs/Phabricator.html#requesting-a-review-via-the-web-interface
lib/StaticAnalyzer/Checkers/MallocChecker.cpp | ||
---|---|---|
593 | It looks like you are using tabs here. The LLVM style guide calls for spaces. http://llvm.org/docs/CodingStandards.html#use-spaces-instead-of-tabs If you haven't seen it, the clang-format tool can help automatically format code to conform to LLVM's expected style. | |
950 | It looks like this method has recently been renamed "parameters()" and so your patch doesn't compile. Are you using top of trunk from the llvm.org repo? | |
1296 | I think it would good to add a test that covers this code. There are analogous tests in test/Analysis/free.c -- that would be a good place for this added test. | |
1309 | Should there be a test for this case, as well? This would be exercised if code is allocated with kmalloc() and then freed in some way other than kfree (such as free() or delete). There are analogous tests in the MismatchedDeallocator test files. | |
2206 | There are tabs on these two lines, as well. |
I agree that more tests would be good. Where should I put them? Should it be in a separate file or should I just have kmalloc/kfree tests next to the malloc/free ones?
I would recommend putting the tests in existing test files next to tests that for similar diagnostics for other allocation/free schemes. So the test for printExpectedAllocName() should go in test/Analysis/free.c and the test for printExpectedDeallocName() should probably go in test/Analysis/MismatchedDeallocator-checker-test.mm.
One more thing, obviously it should mimic malloc/free's behavior in complaining about delete/new being used. Should it also complain about free/malloc being used? I can't imagine that would be something people would usually intend to do, but I don't think it's really "incorrect." Thought?
I'm not familiar with linux kernel programming. Is free() available in the kernel? If someone did define their own free() and used it with kmalloc'd memory, would they consider this a false positive?
It looks like you are using tabs here. The LLVM style guide calls for spaces. http://llvm.org/docs/CodingStandards.html#use-spaces-instead-of-tabs
If you haven't seen it, the clang-format tool can help automatically format code to conform to LLVM's expected style.