> On Mar 7, 2015, at 12:43 AM, Anton Yartsev <[email protected]> wrote: > > On 07.03.2015 8:35, Anna Zaks wrote: >> >>> On Mar 6, 2015, at 6:09 PM, Anton Yartsev <[email protected] >>> <mailto:[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.) > Parameter not having a value means we do not now/care, if we want a leak > check, or not, like in MallocChecker::printState(). What to do in this case? >
I think that in printState, we want to check both: if the symbol is tracked by a leak check and if the symbol is tracked by a non-leak check, so calling the method twice makes sense. >> >> 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> >> > > > -- > Anton
_______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
