On Tue, Mar 1, 2016 at 1:42 PM, <Alexander G. Riccio> <alexan...@riccio.com> 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 just don't understand why some functions made it in this patch and not others (notably, why the lack of _mbsdup, which is documented on the same page as others you are adding). > 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 _expand) which are especially > important to be able to analyze (because they have really tricky APIs), but > because of their kooky APIs, they're harder to implement checkers for. By "here", do you mean in this review thread, or as part of this section of code? ~Aaron > > Sincerely, > Alexander Riccio > -- > "Change the world or go home." > about.me/ariccio > > If left to my own devices, I will build more. > ⁂ > > On Tue, Mar 1, 2016 at 8:38 AM, Aaron Ballman <aaron.ball...@gmail.com> > wrote: >> >> On Tue, Mar 1, 2016 at 2:16 AM, Alexander Riccio <alexan...@riccio.com> >> 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: llvm/tools/clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp >> > =================================================================== >> > --- llvm/tools/clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp >> > +++ llvm/tools/clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp >> > @@ -169,11 +169,12 @@ >> > { >> > public: >> > MallocChecker() >> > - : II_alloca(nullptr), II_malloc(nullptr), II_free(nullptr), >> > - II_realloc(nullptr), II_calloc(nullptr), II_valloc(nullptr), >> > - II_reallocf(nullptr), II_strndup(nullptr), II_strdup(nullptr), >> > - II_kmalloc(nullptr), II_if_nameindex(nullptr), >> > - II_if_freenameindex(nullptr) {} >> > + : II_alloca(nullptr), II_win_alloca(nullptr), II_malloc(nullptr), >> > + II_free(nullptr), II_realloc(nullptr), II_calloc(nullptr), >> > + II_valloc(nullptr), II_reallocf(nullptr), II_strndup(nullptr), >> > + II_strdup(nullptr), II_win_strdup(nullptr), >> > II_kmalloc(nullptr), >> > + II_if_nameindex(nullptr), II_if_freenameindex(nullptr), >> > + II_wcsdup(nullptr), II_win_wcsdup(nullptr) {} >> > >> > /// In pessimistic mode, the checker assumes that it does not know >> > which >> > /// functions might free the memory. >> > @@ -231,10 +232,11 @@ >> > mutable std::unique_ptr<BugType> BT_MismatchedDealloc; >> > mutable std::unique_ptr<BugType> BT_OffsetFree[CK_NumCheckKinds]; >> > mutable std::unique_ptr<BugType> >> > BT_UseZerroAllocated[CK_NumCheckKinds]; >> > - mutable IdentifierInfo *II_alloca, *II_malloc, *II_free, *II_realloc, >> > - *II_calloc, *II_valloc, *II_reallocf, >> > *II_strndup, >> > - *II_strdup, *II_kmalloc, *II_if_nameindex, >> > - *II_if_freenameindex; >> > + mutable IdentifierInfo *II_alloca, *II_win_alloca, *II_malloc, >> > *II_free, >> > + *II_realloc, *II_calloc, *II_valloc, >> > *II_reallocf, >> > + *II_strndup, *II_strdup, *II_win_strdup, >> > *II_kmalloc, >> > + *II_if_nameindex, *II_if_freenameindex, >> > *II_wcsdup, >> > + *II_win_wcsdup; >> > mutable Optional<uint64_t> KernelZeroFlagVal; >> > >> > void initIdentifierInfo(ASTContext &C) const; >> > @@ -540,9 +542,15 @@ >> > II_valloc = &Ctx.Idents.get("valloc"); >> > II_strdup = &Ctx.Idents.get("strdup"); >> > II_strndup = &Ctx.Idents.get("strndup"); >> > + II_wcsdup = &Ctx.Idents.get("wcsdup"); >> > II_kmalloc = &Ctx.Idents.get("kmalloc"); >> > II_if_nameindex = &Ctx.Idents.get("if_nameindex"); >> > II_if_freenameindex = &Ctx.Idents.get("if_freenameindex"); >> > + >> > + //MSVC uses `_`-prefixed instead, so we check for them too. >> > + II_win_strdup = &Ctx.Idents.get("_strdup"); >> > + II_win_wcsdup = &Ctx.Idents.get("_wcsdup"); >> > + II_win_alloca = &Ctx.Idents.get("_alloca"); >> >> What about: _mbsdup, _strdup_dbg, _wcsdup_dbg, _aligned_realloc, and >> the rest? If we're going to add these (which I really support), it >> would be good to make a comprehensive sweep for the Win32 additions >> and add them all. >> >> ~Aaron >> >> > } >> > >> > bool MallocChecker::isMemFunction(const FunctionDecl *FD, ASTContext >> > &C) const { >> > @@ -585,7 +593,8 @@ >> > if (Family == AF_Malloc && CheckAlloc) { >> > if (FunI == II_malloc || FunI == II_realloc || FunI == >> > II_reallocf || >> > FunI == II_calloc || FunI == II_valloc || FunI == II_strdup >> > || >> > - FunI == II_strndup || FunI == II_kmalloc) >> > + FunI == II_win_strdup || FunI == II_strndup || FunI == >> > II_wcsdup || >> > + FunI == II_win_wcsdup || FunI == II_kmalloc) >> > return true; >> > } >> > >> > @@ -600,7 +609,7 @@ >> > } >> > >> > if (Family == AF_Alloca && CheckAlloc) { >> > - if (FunI == II_alloca) >> > + if (FunI == II_alloca || FunI == II_win_alloca) >> > return true; >> > } >> > } >> > @@ -789,11 +798,12 @@ >> > State = ProcessZeroAllocation(C, CE, 1, State); >> > } else if (FunI == II_free) { >> > State = FreeMemAux(C, CE, State, 0, false, >> > ReleasedAllocatedMemory); >> > - } else if (FunI == II_strdup) { >> > + } else if (FunI == II_strdup || FunI == II_win_strdup || >> > + FunI == II_wcsdup || FunI == II_win_wcsdup) { >> > State = MallocUpdateRefState(C, CE, State); >> > } else if (FunI == II_strndup) { >> > State = MallocUpdateRefState(C, CE, State); >> > - } else if (FunI == II_alloca) { >> > + } else if (FunI == II_alloca || FunI == II_win_alloca) { >> > State = MallocMemAux(C, CE, CE->getArg(0), UndefinedVal(), State, >> > AF_Alloca); >> > State = ProcessZeroAllocation(C, CE, 0, State); >> > >> > > > _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
