[PATCH] D18073: Add memory allocating functions

dcoughlin added a comment. Thanks for iterating on the patch! Some comments in-line. Comment at: llvm/tools/clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp:569 + // allocating functions initialized to nullptr, which will never equal a + //non-null IdentifierInfo*, and

[PATCH] D18073: Add memory allocating functions

ariccio added a comment. Nevermind, the order is correct! https://reviews.llvm.org/D18073 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D18073: Add memory allocating functions

On Mon, Aug 8, 2016 at 8:13 PM, Alexander Riccio wrote: > ariccio added a comment. > > In https://reviews.llvm.org/D18073#504216, @dcoughlin wrote: > >> No. The identifier info will be null for C++ operators. > > > I assume you mean `operator new/new[]/delete/delete[]

Re: [PATCH] D18073: Add memory allocating functions

ariccio added a comment. In https://reviews.llvm.org/D18073#504216, @dcoughlin wrote: > No. The identifier info will be null for C++ operators. I assume you mean `operator new/new[]/delete/delete[] > > Thus, when`! isWindowsMSVCEnvironment`, I leave the Windows-only memory > > allocating

Re: [PATCH] D18073: Add memory allocating functions

dcoughlin added a comment. In https://reviews.llvm.org/D18073#437171, @ariccio wrote: > I should elaborate. The principle of operation of this latest patch is that > the `FunctionDecl` in `IsCMemFunction` should never return a `nullptr` > `IdentifierInfo*` from `getIdentifier` (is that a valid

Re: [PATCH] D18073: Add memory allocating functions

ariccio added a comment. Bump? https://reviews.llvm.org/D18073 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D18073: Add memory allocating functions

ariccio added a comment. Bump? http://reviews.llvm.org/D18073 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D18073: Add memory allocating functions

ariccio added a comment. I should elaborate. The principle of operation of this latest patch is that the `FunctionDecl` in `IsCMemFunction` should never return a `nullptr` `IdentifierInfo*` from `getIdentifier` (is that a valid assumption?)... Thus, when`! isWindowsMSVCEnvironment`, I leave

Re: [PATCH] D18073: Add memory allocating functions

ariccio updated this revision to Diff 55774. ariccio added a comment. Only check for MSVC-specific functions when actually building FOR MSVC (i.e. `isWindowsMSVCEnvironment`). Sidenote: is there any reason why none of the `ASTContext&`s are `const`ified in this file?

Re: [PATCH] D18073: Add memory allocating functions

zaks.anna added a comment. You conditionally defined when you build ON Windows machine, not when you build FOR Windows. You should be able to query the compiler to check which targets it's building for. http://reviews.llvm.org/D18073 ___

Re: [PATCH] D18073: Add memory allocating functions

ariccio marked an inline comment as done. ariccio added a comment. Wrongly `#if defined` out test is now fixed. http://reviews.llvm.org/D18073 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

Re: [PATCH] D18073: Add memory allocating functions

ariccio updated this revision to Diff 55366. ariccio added a comment. Conditionally check Windows methods on Windows. Is this what you're thinking of? It's kinda ugly - I was hoping to avoid the `#if`s - but it does only check the Windows functions on Windows builds only.

Re: [PATCH] D18073: Add memory allocating functions

zaks.anna added a comment. "Since we are adding support for so many new APIs that are only available on Windows, could you please condition checking them only when we build for Windows. You probably can look and Language Options to figure that out." By this, I was suggesting that we should be

Re: [PATCH] D18073: Add memory allocating functions

aaron.ballman added a comment. This looks reasonable to me, but you should wait for @zaks.anna to sign off. http://reviews.llvm.org/D18073 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

Re: [PATCH] D18073: Add memory allocating functions

ariccio updated this revision to Diff 53995. ariccio added a comment. So, duh, it turns out I //can// use `_WIN32` to conditionally test. http://reviews.llvm.org/D18073 Files: llvm/tools/clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp

Re: [PATCH] D18073: Add memory allocating functions

aaron.ballman added a comment. In http://reviews.llvm.org/D18073#399270, @ariccio wrote: > In http://reviews.llvm.org/D18073#398882, @zaks.anna wrote: > > > "Since we are adding support for so many new APIs that are only available > > on Windows, could you please condition checking them only

Re: [PATCH] D18073: Add memory allocating functions

ariccio added a comment. In http://reviews.llvm.org/D18073#398882, @zaks.anna wrote: > "Since we are adding support for so many new APIs that are only available on > Windows, could you please condition checking them only when we build for > Windows. You probably can look and Language Options

Re: [PATCH] D18073: Add memory allocating functions

ariccio updated this revision to Diff 53495. ariccio added a comment. Changed the new file name. http://reviews.llvm.org/D18073 Files: llvm/tools/clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp llvm/tools/clang/test/Analysis/alternative-malloc-api.c

Re: [PATCH] D18073: Add memory allocating functions

zaks.anna added a comment. "Since we are adding support for so many new APIs that are only available on Windows, could you please condition checking them only when we build for Windows. You probably can look and Language Options to figure that out." malloc-uses.c -> "alternative-malloc-api.c"

Re: [PATCH] D18073: Add memory allocating functions

ariccio updated this revision to Diff 53193. ariccio added a comment. I'm not actually sure why I didn't want to split these off into a separate file, but now I finally have. Is this what you were looking for? http://reviews.llvm.org/D18073 Files:

Re: [PATCH] D18073: Add memory allocating functions

ariccio added a comment. In http://reviews.llvm.org/D18073#393613, @zaks.anna wrote: > You will have to add one test function to smoke test that the newly added API > is modeled correctly. Isn't that what I've already done? > We also have a lot of existing tests that verify that each of the

Re: [PATCH] D18073: Add memory allocating functions

ariccio updated this revision to Diff 52876. ariccio added a comment. Fewer added test functions. http://reviews.llvm.org/D18073 Files: llvm/tools/clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp llvm/tools/clang/test/Analysis/malloc.c Index: llvm/tools/clang/test/Analysis/malloc.c

Re: [PATCH] D18073: Add memory allocating functions

zaks.anna added a comment. You will have to add one test function to smoke test that the newly added API is modeled correctly. We also have a lot of existing tests that verify that each of the original APIs (malloc. free, new) function correctly in all possible settings. What is the

Re: [PATCH] D18073: Add memory allocating functions

ariccio added a comment. In http://reviews.llvm.org/D18073#392972, @zaks.anna wrote: > > So for _wcsdup_dbg, I should leave only testWinWcsdupDbg? > > > Yes. Ok, that I can do. Will upload patch later this afternoon/tonight. In http://reviews.llvm.org/D18073#392972, @zaks.anna wrote: > Also,

Re: [PATCH] D18073: Add memory allocating functions

ariccio added a comment. Maybe I'm being thick headed and I can't see it - sorry if I am - but I'm still a bit confused. Can you tell me what I should do in the `_wcsdup_dbg` example? http://reviews.llvm.org/D18073 ___ cfe-commits mailing list

Re: [PATCH] D18073: Add memory allocating functions

ariccio added a comment. So for `_wcsdup_dbg`, I should leave only `testWinWcsdupDbg`? http://reviews.llvm.org/D18073 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D18073: Add memory allocating functions

zaks.anna added a comment. Can you add one test per new function, which tests that the function if modeled correctly. Please, do not duplicate all tests that contain a function with it's windows variants. Hope this is more clear. http://reviews.llvm.org/D18073

Re: [PATCH] D18073: Add memory allocating functions

ariccio added a comment. I'm sorry, I'm confused. As an example, for `_wcsdup_dbg`, I've added `testWinWcsdupDbg`, `testWinWcsdupDbgContentIsDefined`, and `testWinWideDbgLeakWithinReturn`. Which ones should I drop for that API? http://reviews.llvm.org/D18073

Re: [PATCH] D18073: Add memory allocating functions

zaks.anna added a comment. > > Also, we should not duplicate all of our tests. Instead, we should add a > > single test per added API checking that it is modeling the operation we > > think it > > > should model. > > > So should I dump the _dbg versions from the tests? No but do not

Re: [PATCH] D18073: Add memory allocating functions

ariccio added a comment. In http://reviews.llvm.org/D18073#372697, @zaks.anna wrote: > Since we are adding support for so many new APIs that are only available on > Windows, could you please condition checking them only when we build for > Windows. You probably can look and Language Options to

Re: [PATCH] D18073: Add memory allocating functions

zaks.anna added a comment. Since we are adding support for so many new APIs that are only available on Windows, could you please condition checking them only when we build for Windows. You probably can look and Language Options to figure that out. Also, we should not duplicate all of our

[PATCH] D18073: Add memory allocating functions

ariccio created this revision. ariccio added reviewers: zaks.anna, dcoughlin, aaron.ballman. ariccio added a subscriber: cfe-commits. As discussed... http://reviews.llvm.org/D18073 Files: llvm/tools/clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp llvm/tools/clang/test/Analysis/malloc.c