r229593 - [analyzer] Refactoring: clarified the way the proper check kind is chosen.
Anton Yartsev
anton.yartsev at gmail.com
Fri Mar 6 07:03:19 PST 2015
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.
>
> 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? 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.
>
>> 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 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 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);
>>>> 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),
>>>> + 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.
>>
>>>
>>>> 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
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20150306/356c5647/attachment.html>
More information about the cfe-commits
mailing list