r229593 - [analyzer] Refactoring: clarified the way the proper check kind is chosen.
Anton Yartsev
anton.yartsev at gmail.com
Sat Mar 7 00:43:08 PST 2015
On 07.03.2015 8:35, Anna Zaks wrote:
>
>> On Mar 6, 2015, at 6:09 PM, Anton Yartsev <anton.yartsev at gmail.com
>> <mailto:anton.yartsev at gmail.com>> 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?
>
> 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 <anton.yartsev at gmail.com
>>>> <mailto:anton.yartsev at gmail.com>> wrote:
>>>>
>>>> On 06.03.2015 8:55, Anna Zaks wrote:
>>>>>
>>>>>> On Mar 5, 2015, at 5:37 PM, Anton Yartsev
>>>>>> <anton.yartsev at gmail.com <mailto:anton.yartsev at gmail.com>> wrote:
>>>>>>
>>>>>> On 05.03.2015 21:39, Anna Zaks wrote:
>>>>>>>
>>>>>>>> On Feb 17, 2015, at 4:39 PM, Anton Yartsev
>>>>>>>> <anton.yartsev at gmail.com <mailto:anton.yartsev at gmail.com>> 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
>>>>>>>> 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.
>>>>>>>
>>>>>>> autoCheckKind =getCheckIfTracked(C, DeallocExpr);
>>>>>>> vs
>>>>>>> autoCheckKind =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
>>>>>>>> ==============================================================================
>>>>>>>> --- 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
>>>>>>>> cfe-commits at cs.uiuc.edu <mailto:cfe-commits at cs.uiuc.edu>
>>>>>>>> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
>>>>>>>
>>>>>> --
>>>>>> Anton
>>>>>
>>>>
>>>>
>>>> --
>>>> Anton
>>>
>>
>>
>> --
>> Anton
>> <Bool_param_for_getCheckIfTracked.patch>
>
--
Anton
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20150307/4e21582d/attachment.html>
More information about the cfe-commits
mailing list