Re: [PATCH] D17688: Fix missed leak from MSVC specific allocation functions

This revision was automatically updated to reflect the committed changes. Closed by commit rL262894: [analyzer] Fix missed leak from MSVC specific allocation functions (authored by zaks). Changed prior to commit: http://reviews.llvm.org/D17688?vs=49845=50013#toc Repository: rL LLVM

Re: [PATCH] D17688: Fix missed leak from MSVC specific allocation functions

ariccio added a comment. In http://reviews.llvm.org/D17688#366780, @zaks.anna wrote: > LGTM. Thanks! > > I can commit this in your behalf. Oh, and yeah, I don't have privs. http://reviews.llvm.org/D17688 ___ cfe-commits mailing list

Re: [PATCH] D17688: Fix missed leak from MSVC specific allocation functions

ariccio added a comment. In http://reviews.llvm.org/D17688#368330, @zaks.anna wrote: > ? http://reviews.llvm.org/D17688 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D17688: Fix missed leak from MSVC specific allocation functions

zaks.anna added a comment. http://reviews.llvm.org/D17688 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D17688: Fix missed leak from MSVC specific allocation functions

ariccio updated this revision to Diff 49845. ariccio added a comment. Alrighty. This final version of the patch causes no test failures (vs the unmodified build*). *An unrelated test normally fails on my machine. http://reviews.llvm.org/D17688 Files:

Re: [PATCH] D17688: Fix missed leak from MSVC specific allocation functions

ariccio added a comment. Hmm. This seems to be because `wchar_t` is enabled by `-fms-compatibility`. @aaron.ballman do you have any idea how to define `wchar_t` without `-fms-compatibility`? http://reviews.llvm.org/D17688 ___ cfe-commits mailing

Re: [PATCH] D17688: Fix missed leak from MSVC specific allocation functions

zaks.anna added a comment. The second: you should not update the diff. When a patch is uploaded, the assumption is that all tests pass:) http://reviews.llvm.org/D17688 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

Re: [PATCH] D17688: Fix missed leak from MSVC specific allocation functions

ariccio added a comment. In http://reviews.llvm.org/D17688#366883, @zaks.anna wrote: > Please, do not submit patches for review before they pass all tests. Whoops, sorry. Should I been a bit clearer that check-all hadn't finished when I updated the diff, or should I not update the diff at all

Re: [PATCH] D17688: Fix missed leak from MSVC specific allocation functions

zaks.anna added a comment. Please, do not submit patches for review before they pass all tests. http://reviews.llvm.org/D17688 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D17688: Fix missed leak from MSVC specific allocation functions

zaks.anna added a comment. Generate a preprocessed file and keep copying the definitions from there until you reach the types compiler knows about. http://reviews.llvm.org/D17688 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

Re: [PATCH] D17688: Fix missed leak from MSVC specific allocation functions

ariccio added a comment. Hmm, check-all produced a number of issues, which I think stem from the following warning: File C:\LLVM\llvm\tools\clang\test\Analysis\malloc.c Line 16: unknown type name 'wchar_t' (with a bunch of instances of that) ...so how do I declare/define `wchar_t`

Re: [PATCH] D17688: Fix missed leak from MSVC specific allocation functions

zaks.anna accepted this revision. zaks.anna added a comment. This revision is now accepted and ready to land. LGTM. Thanks! I can commit this in your behalf. http://reviews.llvm.org/D17688 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

Re: [PATCH] D17688: Fix missed leak from MSVC specific allocation functions

ariccio updated this revision to Diff 49674. ariccio added a comment. Added newly checked variants to the malloc checks. (Running the check-all suite now, will update with results) http://reviews.llvm.org/D17688 Files: llvm/tools/clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp

Re: [PATCH] D17688: Fix missed leak from MSVC specific allocation functions

ariccio added a comment. In http://reviews.llvm.org/D17688#366011, @ariccio wrote: > In http://reviews.llvm.org/D17688#365951, @zaks.anna wrote: > > > ls ./clang/test/Analysis/malloc* > > > Ah, ok. Would it be ok if (for _strdup & _alloca) I just do this at the > beginning: > > #if

Re: [PATCH] D17688: Fix missed leak from MSVC specific allocation functions

ariccio added a comment. In http://reviews.llvm.org/D17688#365951, @zaks.anna wrote: > ls ./clang/test/Analysis/malloc* Ah, ok. Would it be ok if (for _strdup & _alloca) I just do this at the beginning: #if defined(_WIN32) #define strdup _strdup #define alloca _alloca #endif

Re: [PATCH] D17688: Fix missed leak from MSVC specific allocation functions

zaks.anna added a comment. Of cause, we have regression tests for (almost) everything: ls ./clang/test/Analysis/malloc* http://reviews.llvm.org/D17688 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

Re: [PATCH] D17688: Fix missed leak from MSVC specific allocation functions

zaks.anna added a comment. I suggest to update the malloc regression test with these. http://reviews.llvm.org/D17688 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D17688: Fix missed leak from MSVC specific allocation functions

On Tue, Mar 1, 2016 at 1:42 PM, wrote: > I'd quite happily add them... but can I do it in another patch? I think I > could be more thorough that way. I'm not certain I understand the reasoning, but I also don't have strong feelings on whether it's this patch or another. I

Re: [PATCH] D17688: Fix missed leak from MSVC specific allocation functions

ariccio added a comment. Is this patch all clear to go? I hope I don't sound too pushy - I just don't want to lose momentum here. http://reviews.llvm.org/D17688 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

Re: [PATCH] D17688: Fix missed leak from MSVC specific allocation functions

I'd quite happily add them... but can I do it in another patch? I think I could be more thorough that way. For the same reason, can we list all the microsoft memory allocating routines here? There are a thousand routines we might want to add, and then a few others (like _dupenv_s, _malloca, and

Re: [PATCH] D17688: Fix missed leak from MSVC specific allocation functions

On Tue, Mar 1, 2016 at 2:16 AM, Alexander Riccio wrote: > ariccio updated this revision to Diff 49456. > ariccio added a comment. > > Nit addressed. > > > http://reviews.llvm.org/D17688 > > Files: > llvm/tools/clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp > > Index:

Re: [PATCH] D17688: Fix missed leak from MSVC specific allocation functions

zaks.anna added a comment. Nit: Could you change the prefix from "win" to "win_"? http://reviews.llvm.org/D17688 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D17688: Fix missed leak from MSVC specific allocation functions

ariccio added a comment. //(This is mostly for my own reference)// BTW, there are a few other non-MS-only functions in the C standard library that allocate memory that needs to be free()d: 1. getcwd (and _getcwd/_wgetcwd) And some MS-specific functions that I'm surprised are actually MS-only:

Re: [PATCH] D17688: Fix missed leak from MSVC specific allocation functions

ariccio updated this revision to Diff 49330. ariccio added a comment. Changed underscore prefixed variable names to `win` prefixed variable names. http://reviews.llvm.org/D17688 Files: llvm/tools/clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp Index:

Re: [PATCH] D17688: Fix missed leak from MSVC specific allocation functions

ariccio added a comment. In http://reviews.llvm.org/D17688#363835, @zaks.anna wrote: > The two underscores in the names are hard to read. Maybe we should just > prefix them with 'win'? I like that better, will change. http://reviews.llvm.org/D17688

Re: [PATCH] D17688: Fix missed leak from MSVC specific allocation functions

On Sun, Feb 28, 2016 at 1:21 PM, Anna Zaks wrote: > zaks.anna added a comment. > > The two underscores in the names are hard to read. Maybe we should just > prefix them with 'win'? Two underscores in the name is actually undefined behavior, so I would second this

Re: [PATCH] D17688: Fix missed leak from MSVC specific allocation functions

zaks.anna added a comment. The two underscores in the names are hard to read. Maybe we should just prefix them with 'win'? http://reviews.llvm.org/D17688 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

[PATCH] D17688: Fix missed leak from MSVC specific allocation functions

ariccio created this revision. ariccio added a subscriber: cfe-commits. I've found & fixed a leak that Clang misses when compiling on Windows. The leak was found by [[ https://samate.nist.gov/SARD/view_testcase.php?tID=149071 | SARD #149071 ]], mem1-bad.c. Clang misses it because MSVC uses