<html>
<head>
<meta content="text/html; charset=utf-8" http-equiv="Content-Type">
</head>
<body bgcolor="#FFFFFF" text="#000000">
<div class="moz-cite-prefix">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.<br>
Reverted an enhancement at r229593, additional cleanup at r231548<br>
<br>
Attach is the patch that adds an additional parameter to
getCheckIfTracked(), please review!<br>
<br>
</div>
<blockquote
cite="mid:E481D348-0D78-44EA-A548-E99E997A5DBC@apple.com"
type="cite">
<meta http-equiv="Content-Type" content="text/html; charset=utf-8">
Anton,
<div class=""><br class="">
</div>
<div class="">I am not convinced. Please, revert the patch until
we agree on what is the right thing to do here.</div>
<div class=""><br class="">
</div>
<div class="">More comments below.</div>
<div class=""><br class="">
<div>
<blockquote type="cite" class="">
<div class="">On Mar 6, 2015, at 7:03 AM, Anton Yartsev <<a
moz-do-not-send="true"
href="mailto:anton.yartsev@gmail.com" class="">anton.yartsev@gmail.com</a>>
wrote:</div>
<br class="Apple-interchange-newline">
<div class="">
<meta content="text/html; charset=utf-8"
http-equiv="Content-Type" class="">
<div bgcolor="#FFFFFF" text="#000000" class="">
<div class="moz-cite-prefix">On 06.03.2015 8:55, Anna
Zaks wrote:<br class="">
</div>
<blockquote
cite="mid:5EEED25F-4278-4399-97AE-6CB660AEACA7@apple.com"
type="cite" class="">
<meta http-equiv="Content-Type" content="text/html;
charset=utf-8" class="">
<br class="">
<div class="">
<blockquote type="cite" class="">
<div class="">On Mar 5, 2015, at 5:37 PM, Anton
Yartsev <<a moz-do-not-send="true"
href="mailto:anton.yartsev@gmail.com" class="">anton.yartsev@gmail.com</a>>
wrote:</div>
<br class="Apple-interchange-newline">
<div class="">
<meta content="text/html; charset=utf-8"
http-equiv="Content-Type" class="">
<div bgcolor="#FFFFFF" text="#000000" class="">
<div class="moz-cite-prefix">On 05.03.2015
21:39, Anna Zaks wrote:<br class="">
</div>
<blockquote
cite="mid:1F718B4B-258E-41D6-9DBE-58E7B5F330EC@apple.com"
type="cite" class="">
<meta http-equiv="Content-Type"
content="text/html; charset=utf-8"
class="">
<meta http-equiv="Content-Type"
content="text/html; charset=utf-8"
class="">
<br class="">
<div class="">
<blockquote type="cite" class="">
<div class="">On Feb 17, 2015, at 4:39
PM, Anton Yartsev <<a
moz-do-not-send="true"
href="mailto:anton.yartsev@gmail.com"
class="">anton.yartsev@gmail.com</a>>
wrote:</div>
<br class="Apple-interchange-newline">
<div class="">Author: ayartsev<br
class="">
Date: Tue Feb 17 18:39:06 2015<br
class="">
New Revision: 229593<br class="">
<br class="">
URL: <a moz-do-not-send="true"
href="http://llvm.org/viewvc/llvm-project?rev=229593&view=rev"
class="">http://llvm.org/viewvc/llvm-project?rev=229593&view=rev</a><br
class="">
Log:<br class="">
[analyzer] Refactoring: clarified the
way the proper check kind is chosen.<br
class="">
<br class="">
</div>
</blockquote>
<div class=""><br class="">
</div>
<div class="">Anton, this doesn’t look
like a simple refactoring. Also, the new
API looks more confusing and difficult
to use. </div>
<div class="">
<div style="margin: 0px; font-size:
11px; font-family: Menlo; color:
rgb(49, 89, 93);" class=""><span
style="" class=""><br class="">
</span></div>
<div style="margin: 0px; font-size:
11px; font-family: Menlo; color:
rgb(49, 89, 93);" class="">
<div style="margin: 0px;" class=""><span
style="" class=""> </span><span
style="font-variant-ligatures:
no-common-ligatures; color:
#bb2ca2" class="">auto</span><span
style="" class=""> CheckKind = </span>getCheckIfTracked<span
style="" class="">(</span>C,
DeallocExpr);</div>
<div style="margin: 0px;" class="">vs</div>
</div>
<div style="margin: 0px; font-size:
11px; font-family: Menlo; color:
rgb(49, 89, 93);" class="">
<div style="margin: 0px;" class=""><span
style="" class=""> </span><span
style="font-variant-ligatures:
no-common-ligatures; color:
#bb2ca2" class="">auto</span><span
style="" class=""> CheckKind = </span>getCheckIfTracked<span
style="" class="">(</span>MakeVecFromCK<span
style="" class="">(</span>CK_MallocOptimistic<span
style="" class="">,</span></div>
<div style="margin: 0px;" class="">
CK_MallocPessimistic,</div>
<div style="margin: 0px;" class="">
CK_NewDeleteChecker),</div>
<div style="margin: 0px;" class="">
C,
DeallocExpr);</div>
<div style="margin: 0px;" class=""><br
class="">
</div>
</div>
</div>
<div class="">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? </div>
<div class=""><br class="">
</div>
<div class="">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..</div>
</div>
</blockquote>
Hi Anna!)</div>
</div>
</blockquote>
<div class=""><br class="">
</div>
<div class="">Here is my very high level description
on how this works:</div>
<div class="">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.</div>
<br class="">
<blockquote type="cite" class="">
<div class="">
<div bgcolor="#FFFFFF" text="#000000" class="">
The old getCheckIfTracked() has two drawbacks:
first, it does not considered
CK_MismatchedDeallocatorChecker and
CK_NewDeleteLeaksChecker checkers. <br
class="">
</div>
</div>
</blockquote>
<div class=""><br class="">
</div>
<div class="">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.</div>
</div>
</blockquote>
The checker is returned back if the family of the given
symbol fits the checker, otherwise no checker is
returned.<br class="">
</div>
</div>
</blockquote>
<div><br class="">
</div>
<div>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.</div>
</div>
</div>
</blockquote>
<blockquote
cite="mid:E481D348-0D78-44EA-A548-E99E997A5DBC@apple.com"
type="cite">
<div class="">
<div>
<div><br class="">
</div>
<div>
<blockquote type="cite" class="">
<div bgcolor="#FFFFFF" text="#000000" class="">
<blockquote
cite="mid:5EEED25F-4278-4399-97AE-6CB660AEACA7@apple.com"
type="cite" class="">
<div class="">
<blockquote type="cite" class="">
<div class="">
<div bgcolor="#FFFFFF" text="#000000" class="">
<blockquote
cite="mid:1F718B4B-258E-41D6-9DBE-58E7B5F330EC@apple.com"
type="cite" class="">
<div class="">
<blockquote type="cite" class="">
<div class="">+ if (CK ==
CK_MismatchedDeallocatorChecker)<br
class="">
+ return CK;</div>
</blockquote>
</div>
</blockquote>
</div>
</div>
</blockquote>
</div>
</blockquote>
</div>
</blockquote>
</div>
<br class="">
<blockquote type="cite" class="">
<div class="">
<div bgcolor="#FFFFFF" text="#000000" class=""> <br
class="">
<blockquote
cite="mid:5EEED25F-4278-4399-97AE-6CB660AEACA7@apple.com"
type="cite" class="">
<div class="">
<div class=""><br class="">
</div>
</div>
<div class="">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.</div>
</blockquote>
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'. </div>
</div>
</blockquote>
<br class="">
<blockquote type="cite" class="">
<div class="">
<div bgcolor="#FFFFFF" text="#000000" class="">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? </div>
</div>
</blockquote>
<div><br class="">
</div>
<div>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. </div>
<div>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:</div>
<div> - 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.)</div>
<div> - 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.</div>
<div><br class="">
</div>
<div><br class="">
</div>
<blockquote type="cite" class="">
<div class="">
<div bgcolor="#FFFFFF" text="#000000" class="">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.<br class="">
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.<br class="">
<br class="">
</div>
</div>
</blockquote>
<blockquote type="cite" class="">
<div class="">
<div bgcolor="#FFFFFF" text="#000000" class="">
<blockquote
cite="mid:5EEED25F-4278-4399-97AE-6CB660AEACA7@apple.com"
type="cite" class="">
<div class=""><br class="">
</div>
<div class="">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. <br class="">
</div>
</blockquote>
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.<br
class="">
</div>
</div>
</blockquote>
<div><br class="">
</div>
<div>Most calls will need to be modified when this is extended
to handle more API families.</div>
<div>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. </div>
<div><br class="">
</div>
<div><br class="">
</div>
</div>
<div>
<blockquote type="cite" class="">
<div class="">
<div bgcolor="#FFFFFF" text="#000000" class=""> <br
class="">
<blockquote
cite="mid:5EEED25F-4278-4399-97AE-6CB660AEACA7@apple.com"
type="cite" class="">
<div class=""><br class="">
<blockquote type="cite" class="">
<div class="">
<div bgcolor="#FFFFFF" text="#000000" class="">
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.</div>
</div>
</blockquote>
</div>
<div class="">
<blockquote type="cite" class="">
<div class="">
<div bgcolor="#FFFFFF" text="#000000" class="">
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 '<small
class=""><small class="">if (... &&
!ChecksEnabled[CK_NewDeleteLeaksChecker])
return;</small></small>' at the beginning
of the method. In the current implementation
getCheckIfTracked() returns only the checkers
it was restricted for.<br class="">
</div>
</div>
</blockquote>
<div class=""><br class="">
</div>
<div class="">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?)</div>
<div class=""><br class="">
</div>
<blockquote type="cite" class="">
<div class="">
<div bgcolor="#FFFFFF" text="#000000" class="">
<br class="">
The second bonus of the current implementation
is that it gets us rid of the check for
specific checkers at the beginning. <br
class="">
</div>
</div>
</blockquote>
<blockquote type="cite" class="">
<div class="">
<div bgcolor="#FFFFFF" text="#000000" class="">
<blockquote
cite="mid:1F718B4B-258E-41D6-9DBE-58E7B5F330EC@apple.com"
type="cite" class="">
<div class="">
<div class=""><br class="">
</div>
<blockquote type="cite" class="">
<div class="">Modified:<br class="">
cfe/trunk/lib/StaticAnalyzer/Checkers/MallocChecker.cpp<br class="">
<br class="">
Modified:
cfe/trunk/lib/StaticAnalyzer/Checkers/MallocChecker.cpp<br
class="">
URL: <a moz-do-not-send="true"
href="http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/StaticAnalyzer/Checkers/MallocChecker.cpp?rev=229593&r1=229592&r2=229593&view=diff"
class="">http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/StaticAnalyzer/Checkers/MallocChecker.cpp?rev=229593&r1=229592&r2=229593&view=diff</a><br
class="">
==============================================================================<br
class="">
---
cfe/trunk/lib/StaticAnalyzer/Checkers/MallocChecker.cpp
(original)<br class="">
+++
cfe/trunk/lib/StaticAnalyzer/Checkers/MallocChecker.cpp
Tue Feb 17 18:39:06 2015<br class="">
@@ -184,6 +184,7 @@ public:<br
class="">
<br class="">
DefaultBool
ChecksEnabled[CK_NumCheckKinds];<br
class="">
CheckName
CheckNames[CK_NumCheckKinds];<br
class="">
+ typedef
llvm::SmallVector<CheckKind,
CK_NumCheckKinds> CKVecTy;<br
class="">
<br class="">
void checkPreCall(const CallEvent
&Call, CheckerContext &C)
const;<br class="">
void checkPostStmt(const CallExpr
*CE, CheckerContext &C) const;<br
class="">
@@ -327,12 +328,16 @@ private:<br
class="">
<br class="">
///@{<br class="">
/// Tells if a given
family/call/symbol is tracked by the
current checker.<br class="">
- /// Sets CheckKind to the kind of
the checker responsible for this<br
class="">
- /// family/call/symbol.<br class="">
- Optional<CheckKind>
getCheckIfTracked(AllocationFamily
Family) const;<br class="">
- Optional<CheckKind>
getCheckIfTracked(CheckerContext
&C,<br class="">
+ /// Looks through incoming
CheckKind(s) and returns the kind of
the checker <br class="">
+ /// responsible for this
family/call/symbol.<br class="">
</div>
</blockquote>
<div class=""><br class="">
</div>
Is it possible for more than one checker
to be responsible for the same family? <br
class="">
</div>
</blockquote>
Yes, it is possible, e.g. NewDelete,
NewDeleteLeaks and MismatchedDeallocator are
responsible for AF_CXXNew/AF_CXXNewArray
families.<br class="">
<br class="">
</div>
</div>
</blockquote>
<div class=""><br class="">
</div>
<div class="">NewDeleteLeaks and
MismatchedDeallocator are the only non-conformant
checks, correct?</div>
</div>
</blockquote>
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. </div>
</div>
</blockquote>
<div><br class="">
</div>
<div>The idea is to generalize this as much as possible, so
that you could add more families and share the
functionality. </div>
<br class="">
<blockquote type="cite" class="">
<div class="">
<div bgcolor="#FFFFFF" text="#000000" class="">The family
don't necessarily have to be handled by the particular
sole checker. Currently we have:<br class="">
AF_Malloc, AF_Alloca, AF_IfNameIndex: CK_MallocChecker,
CK_MismatchedDeallocatorChecker<br class="">
AF_CXXNew, AF_CXXNewArray: CK_NewDeleteChecker,
CK_NewDeleteLeaksChecker,
CK_MismatchedDeallocatorChecker<br class="">
<br class="">
<blockquote
cite="mid:5EEED25F-4278-4399-97AE-6CB660AEACA7@apple.com"
type="cite" class="">
<div class=""><br class="">
</div>
</blockquote>
</div>
</div>
</blockquote>
<div><br class="">
</div>
<div>This is the view we should have:</div>
<div><br class="">
</div>
<div>Family
| Regular Checker | Leaks checker</div>
<div><br class="">
</div>
<div>AF_Malloc, AF_Alloca, AF_IfNameIndex: CK_MallocChecker,
CK_MallocChecker<br class="">
AF_CXXNew, AF_CXXNewArray: CK_NewDeleteChecker,
CK_NewDeleteLeaksChecker<br class="">
New family
CK_NewFamily , CK_NewFamilyLeaks</div>
<div><br class="">
</div>
<div>CK_MismatchedDeallocatorChecker does not belong to a
family. It's point is to find family mismatches.</div>
<div><br class="">
</div>
<div><br class="">
</div>
<blockquote type="cite" class="">
<div class="">
<div bgcolor="#FFFFFF" text="#000000" class="">
<blockquote
cite="mid:5EEED25F-4278-4399-97AE-6CB660AEACA7@apple.com"
type="cite" class="">
<div class="">
<blockquote type="cite" class="">
<div class="">
<div bgcolor="#FFFFFF" text="#000000" class="">
<blockquote
cite="mid:1F718B4B-258E-41D6-9DBE-58E7B5F330EC@apple.com"
type="cite" class="">
<div class="">This returns the first checker
that handles the family from the given
list.</div>
</blockquote>
Yes, that is how getCheckIfTracked() was
designed before, but the order of the checkers
was hardcoded:<br class="">
<small class=""><small class=""> if
(ChecksEnabled[CK_MallocOptimistic]) {<br
class="">
return CK_MallocOptimistic;<br
class="">
} else if
(ChecksEnabled[CK_MallocPessimistic]) {<br
class="">
return CK_MallocPessimistic;<br
class="">
}<br class="">
<br class="">
</small></small>Now it is possible to
customize the order in which the checkers are
checked and returned.</div>
</div>
</blockquote>
<blockquote type="cite" class="">
<div class="">
<div bgcolor="#FFFFFF" text="#000000" class=""><br
class="">
<blockquote
cite="mid:1F718B4B-258E-41D6-9DBE-58E7B5F330EC@apple.com"
type="cite" class="">
<div class=""><br class="">
</div>
<div class="">
<blockquote type="cite" class="">
<div class="">+
Optional<CheckKind>
getCheckIfTracked(CheckKind CK,<br
class="">
+
AllocationFamily
Family) const;<br class="">
</div>
</blockquote>
<div class=""><br class="">
</div>
<div class="">This always returns either
the input checker or an empty one. Looks
like it should just return a bool...</div>
</div>
</blockquote>
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?<br class="">
<br class="">
<blockquote
cite="mid:1F718B4B-258E-41D6-9DBE-58E7B5F330EC@apple.com"
type="cite" class="">
<div class=""><br class="">
<blockquote type="cite" class="">
<div class="">+
Optional<CheckKind>
getCheckIfTracked(CKVecTy CKVec, <br
class="">
</div>
</blockquote>
<br class="">
Hard to tell what this argument is from
documentation/name.</div>
</blockquote>
I'll address this!<br class="">
<br class="">
<blockquote
cite="mid:1F718B4B-258E-41D6-9DBE-58E7B5F330EC@apple.com"
type="cite" class="">
<div class=""><br class="">
<blockquote type="cite" class="">
<div class="">+
AllocationFamily
Family) const;<br class="">
+ Optional<CheckKind>
getCheckIfTracked(CKVecTy CKVec,
CheckerContext &C,<br class="">
const
Stmt *AllocDeallocStmt) const;<br
class="">
- Optional<CheckKind>
getCheckIfTracked(CheckerContext
&C, SymbolRef Sym) const;<br
class="">
+ Optional<CheckKind>
getCheckIfTracked(CKVecTy CKVec,
CheckerContext &C,<br class="">
+
SymbolRef
Sym) const;<br class="">
///@}<br class="">
static bool
SummarizeValue(raw_ostream &os,
SVal V);<br class="">
static bool
SummarizeRegion(raw_ostream &os,
const MemRegion *MR);<br class="">
@@ -1310,21 +1315,32 @@
ProgramStateRef
MallocChecker::FreeMemAu<br class="">
}<br class="">
<br class="">
Optional<MallocChecker::CheckKind><br class="">
-MallocChecker::getCheckIfTracked(AllocationFamily
Family) const {<br class="">
+MallocChecker::getCheckIfTracked(MallocChecker::CheckKind
CK,<br class="">
+
AllocationFamily
Family) const {<br class="">
+<br class="">
+ if (CK == CK_NumCheckKinds ||
!ChecksEnabled[CK])<br class="">
+ return
Optional<MallocChecker::CheckKind>();<br
class="">
+<br class="">
+ // C/C++ checkers.<br class="">
+ if (CK ==
CK_MismatchedDeallocatorChecker)<br
class="">
+ return CK;<br class="">
+<br class="">
switch (Family) {<br class="">
case AF_Malloc:<br class="">
case AF_IfNameIndex: {<br class="">
- if
(ChecksEnabled[CK_MallocOptimistic]) {<br
class="">
- return CK_MallocOptimistic;<br
class="">
- } else if
(ChecksEnabled[CK_MallocPessimistic])
{<br class="">
- return CK_MallocPessimistic;<br
class="">
+ // C checkers.<br class="">
+ if (CK == CK_MallocOptimistic ||<br
class="">
+ CK == CK_MallocPessimistic) {<br
class="">
+ return CK;<br class="">
}<br class="">
return
Optional<MallocChecker::CheckKind>();<br
class="">
}<br class="">
case AF_CXXNew:<br class="">
case AF_CXXNewArray: {<br class="">
- if
(ChecksEnabled[CK_NewDeleteChecker]) {<br
class="">
- return CK_NewDeleteChecker;<br
class="">
+ // C++ checkers.<br class="">
+ if (CK == CK_NewDeleteChecker ||<br
class="">
+ CK ==
CK_NewDeleteLeaksChecker) {<br
class="">
+ return CK;<br class="">
}<br class="">
return
Optional<MallocChecker::CheckKind>();<br
class="">
}<br class="">
@@ -1335,18 +1351,45 @@
MallocChecker::getCheckIfTracked(Allocat<br
class="">
llvm_unreachable("unhandled
family");<br class="">
}<br class="">
<br class="">
+static MallocChecker::CKVecTy
MakeVecFromCK(MallocChecker::CheckKind
CK1,<br class="">
+
MallocChecker::CheckKind
CK2 = MallocChecker::CK_NumCheckKinds,<br
class="">
+
MallocChecker::CheckKind
CK3 = MallocChecker::CK_NumCheckKinds,<br
class="">
+
MallocChecker::CheckKind
CK4 = MallocChecker::CK_NumCheckKinds)
{<br class="">
+ MallocChecker::CKVecTy CKVec;<br
class="">
+ CKVec.push_back(CK1);<br class="">
+ if (CK2 !=
MallocChecker::CK_NumCheckKinds) {<br
class="">
+ CKVec.push_back(CK2);<br class="">
+ if (CK3 !=
MallocChecker::CK_NumCheckKinds) {<br
class="">
+ CKVec.push_back(CK3);<br
class="">
+ if (CK4 !=
MallocChecker::CK_NumCheckKinds)<br
class="">
+ CKVec.push_back(CK4);<br
class="">
+ }<br class="">
+ }<br class="">
+ return CKVec;<br class="">
+}<br class="">
+<br class="">
Optional<MallocChecker::CheckKind><br class="">
-MallocChecker::getCheckIfTracked(CheckerContext
&C,<br class="">
-
const
Stmt *AllocDeallocStmt) const {<br
class="">
- return
getCheckIfTracked(getAllocationFamily(C,
AllocDeallocStmt));<br class="">
+MallocChecker::getCheckIfTracked(CKVecTy
CKVec, AllocationFamily Family) const
{<br class="">
+ for (auto CK: CKVec) {<br class="">
+ auto RetCK =
getCheckIfTracked(CK, Family);<br
class="">
+ if (RetCK.hasValue())<br class="">
+ return RetCK;<br class="">
+ }<br class="">
+ return
Optional<MallocChecker::CheckKind>();<br
class="">
}<br class="">
<br class="">
Optional<MallocChecker::CheckKind><br class="">
-MallocChecker::getCheckIfTracked(CheckerContext
&C, SymbolRef Sym) const {<br
class="">
+MallocChecker::getCheckIfTracked(CKVecTy
CKVec, CheckerContext &C,<br
class="">
+
const
Stmt *AllocDeallocStmt) const {<br
class="">
+ return getCheckIfTracked(CKVec,
getAllocationFamily(C,
AllocDeallocStmt));<br class="">
+}<br class="">
<br class="">
+Optional<MallocChecker::CheckKind><br class="">
+MallocChecker::getCheckIfTracked(CKVecTy
CKVec, CheckerContext &C,<br
class="">
+
SymbolRef
Sym) const {<br class="">
const RefState *RS =
C.getState()->get<RegionState>(Sym);<br
class="">
assert(RS);<br class="">
- return
getCheckIfTracked(RS->getAllocationFamily());<br
class="">
+ return getCheckIfTracked(CKVec,
RS->getAllocationFamily());<br
class="">
}<br class="">
<br class="">
bool
MallocChecker::SummarizeValue(raw_ostream
&os, SVal V) {<br class="">
@@ -1440,13 +1483,10 @@ void
MallocChecker::ReportBadFree(Checke<br
class="">
SourceRange
Range, <br class="">
const
Expr *DeallocExpr) const {<br class="">
<br class="">
- if
(!ChecksEnabled[CK_MallocOptimistic]
&&<br class="">
-
!ChecksEnabled[CK_MallocPessimistic]
&&<br class="">
-
!ChecksEnabled[CK_NewDeleteChecker])<br
class="">
- return;<br class="">
-<br class="">
-
Optional<MallocChecker::CheckKind>
CheckKind =<br class="">
- getCheckIfTracked(C,
DeallocExpr);<br class="">
+ auto CheckKind =
getCheckIfTracked(MakeVecFromCK(CK_MallocOptimistic,<br
class="">
+
CK_MallocPessimistic,<br
class="">
+
CK_NewDeleteChecker),<br
class="">
+
C,
DeallocExpr);<br class="">
if (!CheckKind.hasValue())<br
class="">
return;<br class="">
<br class="">
@@ -1546,13 +1586,11 @@ void
MallocChecker::ReportOffsetFree(Che<br
class="">
SourceRange
Range, const Expr *DeallocExpr,<br
class="">
const
Expr *AllocExpr) const {<br class="">
<br class="">
- if
(!ChecksEnabled[CK_MallocOptimistic]
&&<br class="">
-
!ChecksEnabled[CK_MallocPessimistic]
&&<br class="">
-
!ChecksEnabled[CK_NewDeleteChecker])<br
class="">
- return;<br class="">
<br class="">
-
Optional<MallocChecker::CheckKind>
CheckKind =<br class="">
- getCheckIfTracked(C,
AllocExpr);<br class="">
+ auto CheckKind =
getCheckIfTracked(MakeVecFromCK(CK_MallocOptimistic,<br
class="">
+
CK_MallocPessimistic,<br
class="">
+
CK_NewDeleteChecker),<br
class="">
+
C,
AllocExpr);<br class="">
if (!CheckKind.hasValue())<br
class="">
return;<br class="">
<br class="">
@@ -1602,12 +1640,10 @@ void
MallocChecker::ReportOffsetFree(Che<br
class="">
void
MallocChecker::ReportUseAfterFree(CheckerContext
&C, SourceRange Range,<br class="">
SymbolRef
Sym) const {<br class="">
<br class="">
- if
(!ChecksEnabled[CK_MallocOptimistic]
&&<br class="">
-
!ChecksEnabled[CK_MallocPessimistic]
&&<br class="">
-
!ChecksEnabled[CK_NewDeleteChecker])<br
class="">
- return;<br class="">
-<br class="">
-
Optional<MallocChecker::CheckKind>
CheckKind = getCheckIfTracked(C, Sym);<br
class="">
+ auto CheckKind =
getCheckIfTracked(MakeVecFromCK(CK_MallocOptimistic,<br
class="">
+
CK_MallocPessimistic,<br
class="">
+
CK_NewDeleteChecker),<br
class="">
+
C,
Sym);<br class="">
if (!CheckKind.hasValue())<br
class="">
return;<br class="">
<br class="">
@@ -1630,12 +1666,10 @@ void
MallocChecker::ReportDoubleFree(Che<br
class="">
bool
Released, SymbolRef Sym, <br class="">
SymbolRef
PrevSym) const {<br class="">
<br class="">
- if
(!ChecksEnabled[CK_MallocOptimistic]
&&<br class="">
-
!ChecksEnabled[CK_MallocPessimistic]
&&<br class="">
-
!ChecksEnabled[CK_NewDeleteChecker])<br
class="">
- return;<br class="">
-<br class="">
-
Optional<MallocChecker::CheckKind>
CheckKind = getCheckIfTracked(C, Sym);<br
class="">
+ auto CheckKind =
getCheckIfTracked(MakeVecFromCK(CK_MallocOptimistic,<br
class="">
+
CK_MallocPessimistic,<br
class="">
+
CK_NewDeleteChecker),<br
class="">
+
C,
Sym);<br class="">
if (!CheckKind.hasValue())<br
class="">
return;<br class="">
<br class="">
@@ -1660,13 +1694,10 @@ void
MallocChecker::ReportDoubleFree(Che<br
class="">
<br class="">
void
MallocChecker::ReportDoubleDelete(CheckerContext
&C, SymbolRef Sym) const {<br
class="">
<br class="">
- if
(!ChecksEnabled[CK_NewDeleteChecker])<br
class="">
- return;<br class="">
-<br class="">
-
Optional<MallocChecker::CheckKind>
CheckKind = getCheckIfTracked(C, Sym);<br
class="">
+ auto CheckKind =
getCheckIfTracked(MakeVecFromCK(CK_NewDeleteChecker),<br
class="">
+
C,
Sym);<br class="">
</div>
</blockquote>
</div>
</blockquote>
</div>
</div>
</blockquote>
</div>
</blockquote>
</div>
</div>
</blockquote>
<div><br class="">
</div>
<div>Not sure why we cannot reuse ReportDoubleFree here...</div>
</div>
</div>
</blockquote>
I'll meditate on this.<br>
<br>
<blockquote
cite="mid:E481D348-0D78-44EA-A548-E99E997A5DBC@apple.com"
type="cite">
<div class="">
<div><br class="">
<blockquote type="cite" class="">
<div class="">
<div bgcolor="#FFFFFF" text="#000000" class="">
<blockquote
cite="mid:5EEED25F-4278-4399-97AE-6CB660AEACA7@apple.com"
type="cite" class="">
<div class="">
<blockquote type="cite" class="">
<div class="">
<div bgcolor="#FFFFFF" text="#000000" class="">
<blockquote
cite="mid:1F718B4B-258E-41D6-9DBE-58E7B5F330EC@apple.com"
type="cite" class="">
<div class="">
<blockquote type="cite" class="">
<div class=""> if
(!CheckKind.hasValue())<br class="">
return;<br class="">
- assert(*CheckKind ==
CK_NewDeleteChecker &&
"invalid check kind");<br class="">
<br class="">
if (ExplodedNode *N =
C.generateSink()) {<br class="">
if (!BT_DoubleDelete)<br class="">
@@ -1851,24 +1882,13 @@
MallocChecker::getAllocationSite(const
E<br class="">
void
MallocChecker::reportLeak(SymbolRef
Sym, ExplodedNode *N,<br class="">
CheckerContext
&C) const {<br class="">
<br class="">
- if
(!ChecksEnabled[CK_MallocOptimistic]
&&<br class="">
-
!ChecksEnabled[CK_MallocPessimistic]
&&<br class="">
-
!ChecksEnabled[CK_NewDeleteLeaksChecker])<br
class="">
- return;<br class="">
-<br class="">
- const RefState *RS =
C.getState()->get<RegionState>(Sym);<br
class="">
- assert(RS && "cannot leak
an untracked symbol");<br class="">
- AllocationFamily Family =
RS->getAllocationFamily();<br
class="">
-
Optional<MallocChecker::CheckKind>
CheckKind = getCheckIfTracked(Family);<br
class="">
+ auto CheckKind =
getCheckIfTracked(MakeVecFromCK(CK_MallocOptimistic,<br
class="">
+
CK_MallocPessimistic,<br
class="">
+
CK_NewDeleteLeaksChecker),<br
class="">
</div>
</blockquote>
</div>
</blockquote>
</div>
</div>
</blockquote>
</div>
</blockquote>
</div>
</div>
</blockquote>
<div><br class="">
</div>
<div>This should ask getCheckIfTracked() return a "leak"
check. This would also make it easy to allow malloc leaks to
be turned on/off separately.</div>
<br class="">
<blockquote type="cite" class="">
<div class="">
<div bgcolor="#FFFFFF" text="#000000" class="">
<blockquote
cite="mid:5EEED25F-4278-4399-97AE-6CB660AEACA7@apple.com"
type="cite" class="">
<div class="">
<blockquote type="cite" class="">
<div class="">
<div bgcolor="#FFFFFF" text="#000000" class="">
<blockquote
cite="mid:1F718B4B-258E-41D6-9DBE-58E7B5F330EC@apple.com"
type="cite" class="">
<div class="">
<blockquote type="cite" class="">
<div class=""> +
C,
Sym);<br class="">
if (!CheckKind.hasValue())<br
class="">
return;<br class="">
<br class="">
</div>
</blockquote>
</div>
</blockquote>
-----------------------------------<br
class="">
<blockquote
cite="mid:1F718B4B-258E-41D6-9DBE-58E7B5F330EC@apple.com"
type="cite" class="">
<div class="">
<blockquote type="cite" class="">
<div class="">- // Special case for new
and new[]; these are controlled by a
separate checker<br class="">
- // flag so that they can be
selectively disabled.<br class="">
- if (Family == AF_CXXNew || Family
== AF_CXXNewArray)<br class="">
- if
(!ChecksEnabled[CK_NewDeleteLeaksChecker])<br
class="">
- return;<br class="">
-<br class="">
</div>
</blockquote>
</div>
</blockquote>
-----------------------------------
<blockquote
cite="mid:1F718B4B-258E-41D6-9DBE-58E7B5F330EC@apple.com"
type="cite" class="">
<div class="">
<blockquote type="cite" class="">
<div class=""> assert(N);<br class="">
if (!BT_Leak[*CheckKind]) {<br
class="">
BT_Leak[*CheckKind].reset(<br
class="">
@@ -2479,8 +2499,10 @@ void
MallocChecker::printState(raw_ostre<br
class="">
for (RegionStateTy::iterator I =
RS.begin(), E = RS.end(); I != E; ++I)
{<br class="">
const RefState *RefS =
State->get<RegionState>(I.getKey());<br
class="">
AllocationFamily Family =
RefS->getAllocationFamily();<br
class="">
-
Optional<MallocChecker::CheckKind>
CheckKind = getCheckIfTracked(Family);<br
class="">
-<br class="">
+ auto CheckKind =
getCheckIfTracked(MakeVecFromCK(CK_MallocOptimistic,<br
class="">
+
CK_MallocPessimistic,<br
class="">
+
CK_NewDeleteChecker),<br
class="">
+
Family);<br
class="">
</div>
</blockquote>
<div class=""><br class="">
</div>
This is a generic printing routine, which
is used for debugging. Why is this
restricted to the specific checkers?</div>
</blockquote>
This particular branch handles leak detecting
checkers which are CK_MallocOptimistic,
CK_MallocPessimistic, and CK_NewDeleteChecker.<br
class="">
</div>
</div>
</blockquote>
</div>
</blockquote>
</div>
</div>
</blockquote>
<div><br class="">
</div>
This is wrong. We've disabled printing for several checks.</div>
<div><br class="">
<blockquote type="cite" class="">
<div class="">
<div bgcolor="#FFFFFF" text="#000000" class="">
<blockquote
cite="mid:5EEED25F-4278-4399-97AE-6CB660AEACA7@apple.com"
type="cite" class="">
<div class="">
<blockquote type="cite" class="">
<div class="">
<div bgcolor="#FFFFFF" text="#000000" class="">
<blockquote
cite="mid:1F718B4B-258E-41D6-9DBE-58E7B5F330EC@apple.com"
type="cite" class="">
<div class=""><br class="">
<blockquote type="cite" class="">
<div class="">
I.getKey()->dumpToStream(Out);<br
class="">
Out << " : ";<br class="">
I.getData().dump(Out);<br
class="">
<br class="">
<br class="">
_______________________________________________<br class="">
cfe-commits mailing list<br class="">
<a moz-do-not-send="true"
href="mailto:cfe-commits@cs.uiuc.edu"
class="">cfe-commits@cs.uiuc.edu</a><br
class="">
<a moz-do-not-send="true"
href="http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits"
class="">http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits</a><br
class="">
</div>
</blockquote>
</div>
<br class="">
</blockquote>
<pre class="moz-signature" cols="72">--
Anton</pre>
</div>
</div>
</blockquote>
</div>
<br class="">
</blockquote>
<br class="">
<br class="">
<pre class="moz-signature" cols="72">--
Anton</pre>
</div>
</div>
</blockquote>
</div>
<br class="">
</div>
</blockquote>
<br>
<br>
<pre class="moz-signature" cols="72">--
Anton</pre>
</body>
</html>