> On Mar 6, 2015, at 6:09 PM, Anton Yartsev <[email protected]> wrote: > > Ok, I see, I wrongly used to think of MallockChecker as about more general > checker then it really is. If to consider CK_MismatchedDeallocatorChecker an > exception, then all other checks appear to be similar for a family and there > are always two types of checkers responsible for a family: a leaks checker > and a checker responsible for everything else. An additional bool parameter > to getCheckIfTracked() is sufficient in this case. > Reverted an enhancement at r229593, additional cleanup at r231548 > Great! Thanks.
> Attach is the patch that adds an additional parameter to getCheckIfTracked(), > please review! > + Optional<bool> IsALeakCheck) const; Let’s replace this with a bool parameter with false as the default value. This should simplify the patch. (We don’t need to differentiate between the parameter not having a value and having a false value here.) Anna. >> Anton, >> >> I am not convinced. Please, revert the patch until we agree on what is the >> right thing to do here. >> >> More comments below. >> >>> On Mar 6, 2015, at 7:03 AM, Anton Yartsev <[email protected] >>> <mailto:[email protected]>> wrote: >>> >>> On 06.03.2015 8:55, Anna Zaks wrote: >>>> >>>>> On Mar 5, 2015, at 5:37 PM, Anton Yartsev <[email protected] >>>>> <mailto:[email protected]>> wrote: >>>>> >>>>> On 05.03.2015 21:39, Anna Zaks wrote: >>>>>> >>>>>>> On Feb 17, 2015, at 4:39 PM, Anton Yartsev <[email protected] >>>>>>> <mailto:[email protected]>> wrote: >>>>>>> >>>>>>> Author: ayartsev >>>>>>> Date: Tue Feb 17 18:39:06 2015 >>>>>>> New Revision: 229593 >>>>>>> >>>>>>> URL: http://llvm.org/viewvc/llvm-project?rev=229593&view=rev >>>>>>> <http://llvm.org/viewvc/llvm-project?rev=229593&view=rev> >>>>>>> Log: >>>>>>> [analyzer] Refactoring: clarified the way the proper check kind is >>>>>>> chosen. >>>>>>> >>>>>> >>>>>> Anton, this doesn’t look like a simple refactoring. Also, the new API >>>>>> looks more confusing and difficult to use. >>>>>> >>>>>> auto CheckKind = getCheckIfTracked(C, DeallocExpr); >>>>>> vs >>>>>> auto CheckKind = getCheckIfTracked(MakeVecFromCK(CK_MallocOptimistic, >>>>>> CK_MallocPessimistic, >>>>>> CK_NewDeleteChecker), >>>>>> C, DeallocExpr); >>>>>> >>>>>> Instead of checking if any of our checkers handle a specific family and >>>>>> returning the one that does, we now have to pass in the list of checkers >>>>>> we are interested in. Can you explain why this is needed? >>>>>> >>>>>> I think this is a step in the wrong direction. My understanding is that >>>>>> some of the methods only work for specific checkers (regardless of the >>>>>> family being processed). Therefore, they returned early in case they >>>>>> were called on checkers, where they are useless. Looks like you are >>>>>> trying to fold that check into the API family check, which is unrelated. >>>>>> Though, I might be missing something.. >>>>> Hi Anna!) >>>> >>>> Here is my very high level description on how this works: >>>> When reporting a bug, we call getCheckIfTracked(..) to find out which >>>> check should be used to report it. (We might ocasionaly use the method in >>>> some other context as well.) In most cases, we expect only one of the >>>> checkers to track the symbol. >>>> >>>>> The old getCheckIfTracked() has two drawbacks: first, it does not >>>>> considered CK_MismatchedDeallocatorChecker and CK_NewDeleteLeaksChecker >>>>> checkers. >>>> >>>> I don’t think it should work with CK_MismatchedDeallocatorChecker as it >>>> covers the case of mixed families. How is this API useful in that case? In >>>> your implementation, you always return it back. >>> The checker is returned back if the family of the given symbol fits the >>> checker, otherwise no checker is returned. >> >> I am talking about CK_MismatchedDeallocatorChecker here. This method does >> not provide us with useful information when processing mismatched >> deallocators. Don't try to overgeneralize and alter the API to fit in this >> check. It does not fit by design. >> >>>>>>> + if (CK == CK_MismatchedDeallocatorChecker) >>>>>>> + return CK; >> >>> >>>> >>>> We can discuss the specifics of CK_NewDeleteLeaksChecker in more detail, >>>> but my understanding is that the reason why it does not work is that we >>>> want to be able to turn the DeleteLeaks off separately because it could >>>> lead to false positives. Hopefully, that is a transitional limitation, so >>>> we should not design the malloc checker around that. >>> As you correctly described 'we call getCheckIfTracked(..) to find out which >>> check should be used to report the bug'. Old implementation just returned >>> CK_MallockChecker for AF_Malloc family and CK_NewDeleteChecker for >>> AF_CXXNew/AF_CXXNewArray families which is correct only in the case, when >>> CK_MallockChecker and CK_NewDeleteChecker are 'on' and all other are 'off'. >> >>> I agree, most reports belong to CK_MallockChecker and CK_NewDeleteChecker >>> checkers, but why not to make getCheckIfTracked(..) return the proper check >>> in all cases? >> >> What is the "proper" check? I believe this method should return a single >> matched check and not depend on the order of checks in the input array, >> which is confusing and error prone. >> For that we need to decide what to do in cases where there is no 1-to-1 >> correspondence between families and checkers. There are 2 cases: >> - CK_MismatchedDeallocatorChecker // It is not useful to have the method >> cover this. I think mismatched deallocator checking should be special cased. >> (We don't have to call this method every time a bug is reported.) >> - Leaks // We may want to have leaks be reported by separate checks. For >> that, we can pass a boolean to the getCheckIfTracked to specify if we want >> leak check or a regular check. It would return MallocChecker for malloc >> family since the leaks check is not separate there. >> >> >>> Consider the use of the new API, for example, in ReportFreeAlloca(). >>> However much new checks/checkers/families we add the new API will remain >>> usable. >>> Concerning the CK_NewDeleteLeaksChecker checker, currently >>> CK_NewDeleteLeaksChecker is considered a part of CK_NewDelete checker. >>> Technically it is implemented as follows: if CK_NewDeleteLeaksChecker is >>> 'on' then CK_NewDelete is being automatically turned 'on'. If this link is >>> broken some day returning CK_NewDelete by an old API will be incorrect. >>> >>>> >>>> On the other hand, we should design this to be easily extendable to handle >>>> more families, and this patch hampers that. You’d need to grow the list of >>>> checkers you send to each call to this function for every new family. Ex: >>>> KeychainAPI checker should be folded into this. >>> You always send the list of checks responsible for the particular given >>> error and getCheckIfTracked(..) returns (if any) one that is responsible >>> for the family of the given slmbol/region. If your report is just for >>> KeychainAPI checker then you send only this checker and you'll get it back >>> if the family of the given symbol is tracked by the checker, otherwise no >>> checker is returned. All other calls will remain unmodified. >> >> Most calls will need to be modified when this is extended to handle more API >> families. >> In this patch, you call the method 7 times. In 5 out of 7 calls you pass the >> same list of 3 regular checkers: CK_MallocOptimistic, CK_MallocPessimistic, >> CK_NewDeleteChecker. In two cases, you special case: once for leaks and once >> for reporting double delete. Every time a new family is added, we would need >> to add it's check to all of the 5 call sites. >> >> >>> >>>> >>>>> The second is that there is, in fact, unable to customize the set of >>>>> checkers getCheckIfTracked() chooses from. For each family there are >>>>> several checkers responsible for it. Without providing the set of >>>>> checkers of interest getCheckIfTracked() is ugly in use. >>>> >>>>> Consider changes in MallocChecker::reportLeak() below - the removed block >>>>> of code (marked start and end of the code with "---------" for you). This >>>>> piece was just added for situations (hard to guess looking at the code), >>>>> when, for example, CK_MallocPessimistic and CK_NewDelete are 'on' and >>>>> CK_NewDeleteLeaksChecker is 'off' and in this case getCheckIfTracked() >>>>> returns CK_NewDelete checker as the checker, responsible for the >>>>> AF_CXXNew/AF_CXXNewArray families. The code looks confusing in >>>>> consideration of the fact that we rejected all the checkers responsible >>>>> for AF_CXXNew/AF_CXXNewArray families, except CK_NewDeleteLeaksChecker, >>>>> by writing 'if (... && !ChecksEnabled[CK_NewDeleteLeaksChecker]) return;' >>>>> at the beginning of the method. In the current implementation >>>>> getCheckIfTracked() returns only the checkers it was restricted for. >>>> >>>> I think it’s better to have one ugly spot that handles a corner case such >>>> as DeleteLeaks. (If we want all leak checks to be separate, we can design >>>> a solution for that as well. Maybe a boolean argument is passed in >>>> whenever we are processing a leak?) >>>> >>>>> >>>>> The second bonus of the current implementation is that it gets us rid of >>>>> the check for specific checkers at the beginning. >>>>>> >>>>>>> Modified: >>>>>>> cfe/trunk/lib/StaticAnalyzer/Checkers/MallocChecker.cpp >>>>>>> >>>>>>> Modified: cfe/trunk/lib/StaticAnalyzer/Checkers/MallocChecker.cpp >>>>>>> URL: >>>>>>> http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/StaticAnalyzer/Checkers/MallocChecker.cpp?rev=229593&r1=229592&r2=229593&view=diff >>>>>>> >>>>>>> <http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/StaticAnalyzer/Checkers/MallocChecker.cpp?rev=229593&r1=229592&r2=229593&view=diff> >>>>>>> ============================================================================== >>>>>>> --- cfe/trunk/lib/StaticAnalyzer/Checkers/MallocChecker.cpp (original) >>>>>>> +++ cfe/trunk/lib/StaticAnalyzer/Checkers/MallocChecker.cpp Tue Feb 17 >>>>>>> 18:39:06 2015 >>>>>>> @@ -184,6 +184,7 @@ public: >>>>>>> >>>>>>> DefaultBool ChecksEnabled[CK_NumCheckKinds]; >>>>>>> CheckName CheckNames[CK_NumCheckKinds]; >>>>>>> + typedef llvm::SmallVector<CheckKind, CK_NumCheckKinds> CKVecTy; >>>>>>> >>>>>>> void checkPreCall(const CallEvent &Call, CheckerContext &C) const; >>>>>>> void checkPostStmt(const CallExpr *CE, CheckerContext &C) const; >>>>>>> @@ -327,12 +328,16 @@ private: >>>>>>> >>>>>>> ///@{ >>>>>>> /// Tells if a given family/call/symbol is tracked by the current >>>>>>> checker. >>>>>>> - /// Sets CheckKind to the kind of the checker responsible for this >>>>>>> - /// family/call/symbol. >>>>>>> - Optional<CheckKind> getCheckIfTracked(AllocationFamily Family) const; >>>>>>> - Optional<CheckKind> getCheckIfTracked(CheckerContext &C, >>>>>>> + /// Looks through incoming CheckKind(s) and returns the kind of the >>>>>>> checker >>>>>>> + /// responsible for this family/call/symbol. >>>>>> >>>>>> Is it possible for more than one checker to be responsible for the same >>>>>> family? >>>>> Yes, it is possible, e.g. NewDelete, NewDeleteLeaks and >>>>> MismatchedDeallocator are responsible for AF_CXXNew/AF_CXXNewArray >>>>> families. >>>>> >>>> >>>> NewDeleteLeaks and MismatchedDeallocator are the only non-conformant >>>> checks, correct? >>> My understanding is that the family just tells, which API was used to >>> allocate the memory (Unix, c++, etc), while the checkers are separated from >>> each other not only by the family they process, but also by functionality. >> >> The idea is to generalize this as much as possible, so that you could add >> more families and share the functionality. >> >>> The family don't necessarily have to be handled by the particular sole >>> checker. Currently we have: >>> AF_Malloc, AF_Alloca, AF_IfNameIndex: CK_MallocChecker, >>> CK_MismatchedDeallocatorChecker >>> AF_CXXNew, AF_CXXNewArray: CK_NewDeleteChecker, CK_NewDeleteLeaksChecker, >>> CK_MismatchedDeallocatorChecker >>> >>>> >> >> This is the view we should have: >> >> Family | Regular >> Checker | Leaks checker >> >> AF_Malloc, AF_Alloca, AF_IfNameIndex: CK_MallocChecker, >> CK_MallocChecker >> AF_CXXNew, AF_CXXNewArray: CK_NewDeleteChecker, >> CK_NewDeleteLeaksChecker >> New family CK_NewFamily >> , CK_NewFamilyLeaks >> >> CK_MismatchedDeallocatorChecker does not belong to a family. It's point is >> to find family mismatches. >> >> >>>>>> This returns the first checker that handles the family from the given >>>>>> list. >>>>> Yes, that is how getCheckIfTracked() was designed before, but the order >>>>> of the checkers was hardcoded: >>>>> if (ChecksEnabled[CK_MallocOptimistic]) { >>>>> return CK_MallocOptimistic; >>>>> } else if (ChecksEnabled[CK_MallocPessimistic]) { >>>>> return CK_MallocPessimistic; >>>>> } >>>>> >>>>> Now it is possible to customize the order in which the checkers are >>>>> checked and returned. >>>>> >>>>>> >>>>>>> + Optional<CheckKind> getCheckIfTracked(CheckKind CK, >>>>>>> + AllocationFamily Family) const; >>>>>> >>>>>> This always returns either the input checker or an empty one. Looks like >>>>>> it should just return a bool... >>>>> I left this to be consistent with other overloads, and also the name of >>>>> the method implies that the checker is returned. Do you think the return >>>>> value should be changed to bool? And, if yes, do you think the method >>>>> should be renamed? >>>>> >>>>>> >>>>>>> + Optional<CheckKind> getCheckIfTracked(CKVecTy CKVec, >>>>>> >>>>>> Hard to tell what this argument is from documentation/name. >>>>> I'll address this! >>>>> >>>>>> >>>>>>> + AllocationFamily Family) const; >>>>>>> + Optional<CheckKind> getCheckIfTracked(CKVecTy CKVec, CheckerContext >>>>>>> &C, >>>>>>> const Stmt *AllocDeallocStmt) >>>>>>> const; >>>>>>> - Optional<CheckKind> getCheckIfTracked(CheckerContext &C, SymbolRef >>>>>>> Sym) const; >>>>>>> + Optional<CheckKind> getCheckIfTracked(CKVecTy CKVec, CheckerContext >>>>>>> &C, >>>>>>> + SymbolRef Sym) const; >>>>>>> ///@} >>>>>>> static bool SummarizeValue(raw_ostream &os, SVal V); >>>>>>> static bool SummarizeRegion(raw_ostream &os, const MemRegion *MR); >>>>>>> @@ -1310,21 +1315,32 @@ ProgramStateRef MallocChecker::FreeMemAu >>>>>>> } >>>>>>> >>>>>>> Optional<MallocChecker::CheckKind> >>>>>>> -MallocChecker::getCheckIfTracked(AllocationFamily Family) const { >>>>>>> +MallocChecker::getCheckIfTracked(MallocChecker::CheckKind CK, >>>>>>> + AllocationFamily Family) const { >>>>>>> + >>>>>>> + if (CK == CK_NumCheckKinds || !ChecksEnabled[CK]) >>>>>>> + return Optional<MallocChecker::CheckKind>(); >>>>>>> + >>>>>>> + // C/C++ checkers. >>>>>>> + if (CK == CK_MismatchedDeallocatorChecker) >>>>>>> + return CK; >>>>>>> + >>>>>>> switch (Family) { >>>>>>> case AF_Malloc: >>>>>>> case AF_IfNameIndex: { >>>>>>> - if (ChecksEnabled[CK_MallocOptimistic]) { >>>>>>> - return CK_MallocOptimistic; >>>>>>> - } else if (ChecksEnabled[CK_MallocPessimistic]) { >>>>>>> - return CK_MallocPessimistic; >>>>>>> + // C checkers. >>>>>>> + if (CK == CK_MallocOptimistic || >>>>>>> + CK == CK_MallocPessimistic) { >>>>>>> + return CK; >>>>>>> } >>>>>>> return Optional<MallocChecker::CheckKind>(); >>>>>>> } >>>>>>> case AF_CXXNew: >>>>>>> case AF_CXXNewArray: { >>>>>>> - if (ChecksEnabled[CK_NewDeleteChecker]) { >>>>>>> - return CK_NewDeleteChecker; >>>>>>> + // C++ checkers. >>>>>>> + if (CK == CK_NewDeleteChecker || >>>>>>> + CK == CK_NewDeleteLeaksChecker) { >>>>>>> + return CK; >>>>>>> } >>>>>>> return Optional<MallocChecker::CheckKind>(); >>>>>>> } >>>>>>> @@ -1335,18 +1351,45 @@ MallocChecker::getCheckIfTracked(Allocat >>>>>>> llvm_unreachable("unhandled family"); >>>>>>> } >>>>>>> >>>>>>> +static MallocChecker::CKVecTy MakeVecFromCK(MallocChecker::CheckKind >>>>>>> CK1, >>>>>>> + MallocChecker::CheckKind CK2 = >>>>>>> MallocChecker::CK_NumCheckKinds, >>>>>>> + MallocChecker::CheckKind CK3 = >>>>>>> MallocChecker::CK_NumCheckKinds, >>>>>>> + MallocChecker::CheckKind CK4 = >>>>>>> MallocChecker::CK_NumCheckKinds) { >>>>>>> + MallocChecker::CKVecTy CKVec; >>>>>>> + CKVec.push_back(CK1); >>>>>>> + if (CK2 != MallocChecker::CK_NumCheckKinds) { >>>>>>> + CKVec.push_back(CK2); >>>>>>> + if (CK3 != MallocChecker::CK_NumCheckKinds) { >>>>>>> + CKVec.push_back(CK3); >>>>>>> + if (CK4 != MallocChecker::CK_NumCheckKinds) >>>>>>> + CKVec.push_back(CK4); >>>>>>> + } >>>>>>> + } >>>>>>> + return CKVec; >>>>>>> +} >>>>>>> + >>>>>>> Optional<MallocChecker::CheckKind> >>>>>>> -MallocChecker::getCheckIfTracked(CheckerContext &C, >>>>>>> - const Stmt *AllocDeallocStmt) const { >>>>>>> - return getCheckIfTracked(getAllocationFamily(C, AllocDeallocStmt)); >>>>>>> +MallocChecker::getCheckIfTracked(CKVecTy CKVec, AllocationFamily >>>>>>> Family) const { >>>>>>> + for (auto CK: CKVec) { >>>>>>> + auto RetCK = getCheckIfTracked(CK, Family); >>>>>>> + if (RetCK.hasValue()) >>>>>>> + return RetCK; >>>>>>> + } >>>>>>> + return Optional<MallocChecker::CheckKind>(); >>>>>>> } >>>>>>> >>>>>>> Optional<MallocChecker::CheckKind> >>>>>>> -MallocChecker::getCheckIfTracked(CheckerContext &C, SymbolRef Sym) >>>>>>> const { >>>>>>> +MallocChecker::getCheckIfTracked(CKVecTy CKVec, CheckerContext &C, >>>>>>> + const Stmt *AllocDeallocStmt) const { >>>>>>> + return getCheckIfTracked(CKVec, getAllocationFamily(C, >>>>>>> AllocDeallocStmt)); >>>>>>> +} >>>>>>> >>>>>>> +Optional<MallocChecker::CheckKind> >>>>>>> +MallocChecker::getCheckIfTracked(CKVecTy CKVec, CheckerContext &C, >>>>>>> + SymbolRef Sym) const { >>>>>>> const RefState *RS = C.getState()->get<RegionState>(Sym); >>>>>>> assert(RS); >>>>>>> - return getCheckIfTracked(RS->getAllocationFamily()); >>>>>>> + return getCheckIfTracked(CKVec, RS->getAllocationFamily()); >>>>>>> } >>>>>>> >>>>>>> bool MallocChecker::SummarizeValue(raw_ostream &os, SVal V) { >>>>>>> @@ -1440,13 +1483,10 @@ void MallocChecker::ReportBadFree(Checke >>>>>>> SourceRange Range, >>>>>>> const Expr *DeallocExpr) const { >>>>>>> >>>>>>> - if (!ChecksEnabled[CK_MallocOptimistic] && >>>>>>> - !ChecksEnabled[CK_MallocPessimistic] && >>>>>>> - !ChecksEnabled[CK_NewDeleteChecker]) >>>>>>> - return; >>>>>>> - >>>>>>> - Optional<MallocChecker::CheckKind> CheckKind = >>>>>>> - getCheckIfTracked(C, DeallocExpr); >>>>>>> + auto CheckKind = getCheckIfTracked(MakeVecFromCK(CK_MallocOptimistic, >>>>>>> + >>>>>>> CK_MallocPessimistic, >>>>>>> + >>>>>>> CK_NewDeleteChecker), >>>>>>> + C, DeallocExpr); >>>>>>> if (!CheckKind.hasValue()) >>>>>>> return; >>>>>>> >>>>>>> @@ -1546,13 +1586,11 @@ void MallocChecker::ReportOffsetFree(Che >>>>>>> SourceRange Range, const Expr >>>>>>> *DeallocExpr, >>>>>>> const Expr *AllocExpr) const { >>>>>>> >>>>>>> - if (!ChecksEnabled[CK_MallocOptimistic] && >>>>>>> - !ChecksEnabled[CK_MallocPessimistic] && >>>>>>> - !ChecksEnabled[CK_NewDeleteChecker]) >>>>>>> - return; >>>>>>> >>>>>>> - Optional<MallocChecker::CheckKind> CheckKind = >>>>>>> - getCheckIfTracked(C, AllocExpr); >>>>>>> + auto CheckKind = getCheckIfTracked(MakeVecFromCK(CK_MallocOptimistic, >>>>>>> + >>>>>>> CK_MallocPessimistic, >>>>>>> + >>>>>>> CK_NewDeleteChecker), >>>>>>> + C, AllocExpr); >>>>>>> if (!CheckKind.hasValue()) >>>>>>> return; >>>>>>> >>>>>>> @@ -1602,12 +1640,10 @@ void MallocChecker::ReportOffsetFree(Che >>>>>>> void MallocChecker::ReportUseAfterFree(CheckerContext &C, SourceRange >>>>>>> Range, >>>>>>> SymbolRef Sym) const { >>>>>>> >>>>>>> - if (!ChecksEnabled[CK_MallocOptimistic] && >>>>>>> - !ChecksEnabled[CK_MallocPessimistic] && >>>>>>> - !ChecksEnabled[CK_NewDeleteChecker]) >>>>>>> - return; >>>>>>> - >>>>>>> - Optional<MallocChecker::CheckKind> CheckKind = getCheckIfTracked(C, >>>>>>> Sym); >>>>>>> + auto CheckKind = getCheckIfTracked(MakeVecFromCK(CK_MallocOptimistic, >>>>>>> + >>>>>>> CK_MallocPessimistic, >>>>>>> + >>>>>>> CK_NewDeleteChecker), >>>>>>> + C, Sym); >>>>>>> if (!CheckKind.hasValue()) >>>>>>> return; >>>>>>> >>>>>>> @@ -1630,12 +1666,10 @@ void MallocChecker::ReportDoubleFree(Che >>>>>>> bool Released, SymbolRef Sym, >>>>>>> SymbolRef PrevSym) const { >>>>>>> >>>>>>> - if (!ChecksEnabled[CK_MallocOptimistic] && >>>>>>> - !ChecksEnabled[CK_MallocPessimistic] && >>>>>>> - !ChecksEnabled[CK_NewDeleteChecker]) >>>>>>> - return; >>>>>>> - >>>>>>> - Optional<MallocChecker::CheckKind> CheckKind = getCheckIfTracked(C, >>>>>>> Sym); >>>>>>> + auto CheckKind = getCheckIfTracked(MakeVecFromCK(CK_MallocOptimistic, >>>>>>> + >>>>>>> CK_MallocPessimistic, >>>>>>> + >>>>>>> CK_NewDeleteChecker), >>>>>>> + C, Sym); >>>>>>> if (!CheckKind.hasValue()) >>>>>>> return; >>>>>>> >>>>>>> @@ -1660,13 +1694,10 @@ void MallocChecker::ReportDoubleFree(Che >>>>>>> >>>>>>> void MallocChecker::ReportDoubleDelete(CheckerContext &C, SymbolRef >>>>>>> Sym) const { >>>>>>> >>>>>>> - if (!ChecksEnabled[CK_NewDeleteChecker]) >>>>>>> - return; >>>>>>> - >>>>>>> - Optional<MallocChecker::CheckKind> CheckKind = getCheckIfTracked(C, >>>>>>> Sym); >>>>>>> + auto CheckKind = >>>>>>> getCheckIfTracked(MakeVecFromCK(CK_NewDeleteChecker), >>>>>>> + C, Sym); >> >> Not sure why we cannot reuse ReportDoubleFree here... > I'll meditate on this. > >> >>>>>>> if (!CheckKind.hasValue()) >>>>>>> return; >>>>>>> - assert(*CheckKind == CK_NewDeleteChecker && "invalid check kind"); >>>>>>> >>>>>>> if (ExplodedNode *N = C.generateSink()) { >>>>>>> if (!BT_DoubleDelete) >>>>>>> @@ -1851,24 +1882,13 @@ MallocChecker::getAllocationSite(const E >>>>>>> void MallocChecker::reportLeak(SymbolRef Sym, ExplodedNode *N, >>>>>>> CheckerContext &C) const { >>>>>>> >>>>>>> - if (!ChecksEnabled[CK_MallocOptimistic] && >>>>>>> - !ChecksEnabled[CK_MallocPessimistic] && >>>>>>> - !ChecksEnabled[CK_NewDeleteLeaksChecker]) >>>>>>> - return; >>>>>>> - >>>>>>> - const RefState *RS = C.getState()->get<RegionState>(Sym); >>>>>>> - assert(RS && "cannot leak an untracked symbol"); >>>>>>> - AllocationFamily Family = RS->getAllocationFamily(); >>>>>>> - Optional<MallocChecker::CheckKind> CheckKind = >>>>>>> getCheckIfTracked(Family); >>>>>>> + auto CheckKind = getCheckIfTracked(MakeVecFromCK(CK_MallocOptimistic, >>>>>>> + >>>>>>> CK_MallocPessimistic, >>>>>>> + >>>>>>> CK_NewDeleteLeaksChecker), >> >> This should ask getCheckIfTracked() return a "leak" check. This would also >> make it easy to allow malloc leaks to be turned on/off separately. >> >>>>>>> + C, Sym); >>>>>>> if (!CheckKind.hasValue()) >>>>>>> return; >>>>>>> >>>>> ----------------------------------- >>>>>>> - // Special case for new and new[]; these are controlled by a >>>>>>> separate checker >>>>>>> - // flag so that they can be selectively disabled. >>>>>>> - if (Family == AF_CXXNew || Family == AF_CXXNewArray) >>>>>>> - if (!ChecksEnabled[CK_NewDeleteLeaksChecker]) >>>>>>> - return; >>>>>>> - >>>>> ----------------------------------- >>>>>> >>>>>>> assert(N); >>>>>>> if (!BT_Leak[*CheckKind]) { >>>>>>> BT_Leak[*CheckKind].reset( >>>>>>> @@ -2479,8 +2499,10 @@ void MallocChecker::printState(raw_ostre >>>>>>> for (RegionStateTy::iterator I = RS.begin(), E = RS.end(); I != E; >>>>>>> ++I) { >>>>>>> const RefState *RefS = State->get<RegionState>(I.getKey()); >>>>>>> AllocationFamily Family = RefS->getAllocationFamily(); >>>>>>> - Optional<MallocChecker::CheckKind> CheckKind = >>>>>>> getCheckIfTracked(Family); >>>>>>> - >>>>>>> + auto CheckKind = >>>>>>> getCheckIfTracked(MakeVecFromCK(CK_MallocOptimistic, >>>>>>> + >>>>>>> CK_MallocPessimistic, >>>>>>> + >>>>>>> CK_NewDeleteChecker), >>>>>>> + Family); >>>>>> >>>>>> This is a generic printing routine, which is used for debugging. Why is >>>>>> this restricted to the specific checkers? >>>>> This particular branch handles leak detecting checkers which are >>>>> CK_MallocOptimistic, CK_MallocPessimistic, and CK_NewDeleteChecker. >> >> This is wrong. We've disabled printing for several checks. >> >>>>>> >>>>>>> I.getKey()->dumpToStream(Out); >>>>>>> Out << " : "; >>>>>>> I.getData().dump(Out); >>>>>>> >>>>>>> >>>>>>> _______________________________________________ >>>>>>> cfe-commits mailing list >>>>>>> [email protected] <mailto:[email protected]> >>>>>>> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits >>>>>>> <http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits> >>>>>> >>>>> -- >>>>> Anton >>>> >>> >>> >>> -- >>> Anton >> > > > -- > Anton > <Bool_param_for_getCheckIfTracked.patch>
_______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
