r229593 - [analyzer] Refactoring: clarified the way the proper check kind is chosen.
Anton Yartsev
anton.yartsev at gmail.com
Tue Mar 10 15:29:12 PDT 2015
On 10.03.2015 1:02, Anna Zaks wrote:
>
>> On Mar 7, 2015, at 12:43 AM, Anton Yartsev <anton.yartsev at gmail.com
>> <mailto:anton.yartsev at gmail.com>> wrote:
>>
>> 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?
>>
>
> 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.
Done! Committed as r231863.
>
>>>
>>> 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
>
--
Anton
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20150311/9c204527/attachment.html>
More information about the cfe-commits
mailing list