<html>
<head>
<meta content="text/html; charset=utf-8" http-equiv="Content-Type">
</head>
<body bgcolor="#FFFFFF" text="#000000">
<div class="moz-cite-prefix">On 07.03.2015 8:35, Anna Zaks wrote:<br>
</div>
<blockquote
cite="mid:D89304E4-8661-42AF-AA0E-7F89A45DF7D0@apple.com"
type="cite">
<meta http-equiv="Content-Type" content="text/html; charset=utf-8">
<br class="">
<div>
<blockquote type="cite" class="">
<div class="">On Mar 6, 2015, at 6:09 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="">
<div class="moz-cite-prefix" style="font-family: Helvetica;
font-size: 12px; font-style: normal; font-variant: normal;
font-weight: normal; letter-spacing: normal; line-height:
normal; orphans: auto; text-align: start; text-indent:
0px; text-transform: none; white-space: normal; widows:
auto; word-spacing: 0px; -webkit-text-stroke-width: 0px;
background-color: rgb(255, 255, 255);">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 class="">
Reverted an enhancement at r229593, additional cleanup at
r231548<br class="">
<br class="">
</div>
</div>
</blockquote>
Great! Thanks.</div>
<div>
<div class=""><br class="">
</div>
<blockquote type="cite" class="">
<div class="">
<div class="moz-cite-prefix" style="font-family: Helvetica;
font-size: 12px; font-style: normal; font-variant: normal;
font-weight: normal; letter-spacing: normal; line-height:
normal; orphans: auto; text-align: start; text-indent:
0px; text-transform: none; white-space: normal; widows:
auto; word-spacing: 0px; -webkit-text-stroke-width: 0px;
background-color: rgb(255, 255, 255);">Attach is the patch
that adds an additional parameter to getCheckIfTracked(),
please review!<br class="">
<br class="">
</div>
</div>
</blockquote>
<div>+
Optional<bool> IsALeakCheck) const;</div>
<div class="">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.)</div>
</div>
</blockquote>
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?<br>
<br>
<blockquote
cite="mid:D89304E4-8661-42AF-AA0E-7F89A45DF7D0@apple.com"
type="cite">
<div>
<div class=""><br class="">
</div>
<div class="">Anna.</div>
<blockquote type="cite" class="">
<div class="">
<blockquote
cite="mid:E481D348-0D78-44EA-A548-E99E997A5DBC@apple.com"
type="cite" style="font-family: Helvetica; font-size:
12px; font-style: normal; font-variant: normal;
font-weight: normal; letter-spacing: normal; line-height:
normal; orphans: auto; text-align: start; text-indent:
0px; text-transform: none; white-space: normal; widows:
auto; word-spacing: 0px; -webkit-text-stroke-width: 0px;
background-color: rgb(255, 255, 255);" class="">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 class="">
<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="">
<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=""><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="">
<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=""><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:<span
class="Apple-converted-space"> </span><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 class="" style="margin:
0px; font-size: 11px;
font-family: Menlo; color:
rgb(49, 89, 93);"><span
class=""><br class="">
</span></div>
<div class="" style="margin:
0px; font-size: 11px;
font-family: Menlo; color:
rgb(49, 89, 93);">
<div class="" style="margin:
0px;"><span class=""> </span><span
class="" style="color:
rgb(187, 44, 162);">auto</span><span
class=""><span
class="Apple-converted-space"> </span>CheckKind
=<span
class="Apple-converted-space"> </span></span>getCheckIfTracked<span
class="">(</span>C,
DeallocExpr);</div>
<div class="" style="margin:
0px;">vs</div>
</div>
<div class="" style="margin:
0px; font-size: 11px;
font-family: Menlo; color:
rgb(49, 89, 93);">
<div class="" style="margin:
0px;"><span class=""> </span><span
class="" style="color:
rgb(187, 44, 162);">auto</span><span
class=""><span
class="Apple-converted-space"> </span>CheckKind
=<span
class="Apple-converted-space"> </span></span>getCheckIfTracked<span
class="">(</span>MakeVecFromCK<span
class="">(</span>CK_MallocOptimistic<span
class="">,</span></div>
<div class="" style="margin:
0px;">
<span
class="Apple-converted-space"> </span>CK_MallocPessimistic,</div>
<div class="" style="margin:
0px;">
<span
class="Apple-converted-space"> </span>CK_NewDeleteChecker),</div>
<div class="" style="margin:
0px;">
<span
class="Apple-converted-space"> </span>C,
DeallocExpr);</div>
<div class="" style="margin:
0px;"><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.<span
class="Apple-converted-space"> </span><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 class=""><br class="">
</div>
<div class="">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" style="font-family: Helvetica; font-size:
12px; font-style: normal; font-variant: normal;
font-weight: normal; letter-spacing: normal; line-height:
normal; orphans: auto; text-align: start; text-indent:
0px; text-transform: none; white-space: normal; widows:
auto; word-spacing: 0px; -webkit-text-stroke-width: 0px;
background-color: rgb(255, 255, 255);" class="">
<div class="">
<div class="">
<div class=""><br class="">
</div>
<div class="">
<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'.<span class="Apple-converted-space"> </span></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?<span
class="Apple-converted-space"> </span></div>
</div>
</blockquote>
<div class=""><br class="">
</div>
<div class="">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 class="">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 class=""> - 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 class=""> - 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 class=""><br class="">
</div>
<div class=""><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.<span class="Apple-converted-space"> </span><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 class=""><br class="">
</div>
<div class="">Most calls will need to be modified when
this is extended to handle more API families.</div>
<div class="">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 class=""><br class="">
</div>
<div class=""><br class="">
</div>
</div>
<div 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=""><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:<span
class="Apple-converted-space"> </span><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<span
class="Apple-converted-space"> </span><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?<span
class="Apple-converted-space"> </span><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.<span
class="Apple-converted-space"> </span></div>
</div>
</blockquote>
<div class=""><br class="">
</div>
<div class="">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 class=""><br class="">
</div>
<div class="">This is the view we should have:</div>
<div class=""><br class="">
</div>
<div class="">Family
| Regular Checker |
Leaks checker</div>
<div class=""><br class="">
</div>
<div class="">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 class=""><br class="">
</div>
<div class="">CK_MismatchedDeallocatorChecker does not
belong to a family. It's point is to find family
mismatches.</div>
<div class=""><br class="">
</div>
<div class=""><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=""> <span
class="Apple-converted-space"> </span>if
(ChecksEnabled[CK_MallocOptimistic])
{<br class="">
<span
class="Apple-converted-space"> </span>return
CK_MallocOptimistic;<br class="">
<span
class="Apple-converted-space"> </span>}
else if
(ChecksEnabled[CK_MallocPessimistic])
{<br class="">
<span
class="Apple-converted-space"> </span>return
CK_MallocPessimistic;<br class="">
<span
class="Apple-converted-space"> </span>}<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,<span
class="Apple-converted-space"> </span><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,<span
class="Apple-converted-space"> </span><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,<span
class="Apple-converted-space"> </span><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 class=""><br class="">
</div>
<div class="">Not sure why we cannot reuse
ReportDoubleFree here...</div>
</div>
</div>
</blockquote>
<span style="font-family: Helvetica; font-size: 12px;
font-style: normal; font-variant: normal; font-weight:
normal; letter-spacing: normal; line-height: normal;
orphans: auto; text-align: start; text-indent: 0px;
text-transform: none; white-space: normal; widows: auto;
word-spacing: 0px; -webkit-text-stroke-width: 0px;
background-color: rgb(255, 255, 255); float: none;
display: inline !important;" class="">I'll meditate on
this.</span><br style="font-family: Helvetica; font-size:
12px; font-style: normal; font-variant: normal;
font-weight: normal; letter-spacing: normal; line-height:
normal; orphans: auto; text-align: start; text-indent:
0px; text-transform: none; white-space: normal; widows:
auto; word-spacing: 0px; -webkit-text-stroke-width: 0px;
background-color: rgb(255, 255, 255);" class="">
<br style="font-family: Helvetica; font-size: 12px;
font-style: normal; font-variant: normal; font-weight:
normal; letter-spacing: normal; line-height: normal;
orphans: auto; text-align: start; text-indent: 0px;
text-transform: none; white-space: normal; widows: auto;
word-spacing: 0px; -webkit-text-stroke-width: 0px;
background-color: rgb(255, 255, 255);" class="">
<blockquote
cite="mid:E481D348-0D78-44EA-A548-E99E997A5DBC@apple.com"
type="cite" style="font-family: Helvetica; font-size:
12px; font-style: normal; font-variant: normal;
font-weight: normal; letter-spacing: normal; line-height:
normal; orphans: auto; text-align: start; text-indent:
0px; text-transform: none; white-space: normal; widows:
auto; word-spacing: 0px; -webkit-text-stroke-width: 0px;
background-color: rgb(255, 255, 255);" class="">
<div class="">
<div class=""><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 class=""><br class="">
</div>
<div class="">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 class=""><br class="">
</div>
This is wrong. We've disabled printing for several
checks.</div>
<div class=""><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 style="font-family: Helvetica; font-size: 12px;
font-style: normal; font-variant: normal; font-weight:
normal; letter-spacing: normal; line-height: normal;
orphans: auto; text-align: start; text-indent: 0px;
text-transform: none; white-space: normal; widows: auto;
word-spacing: 0px; -webkit-text-stroke-width: 0px;
background-color: rgb(255, 255, 255);" class="">
<br style="font-family: Helvetica; font-size: 12px;
font-style: normal; font-variant: normal; font-weight:
normal; letter-spacing: normal; line-height: normal;
orphans: auto; text-align: start; text-indent: 0px;
text-transform: none; white-space: normal; widows: auto;
word-spacing: 0px; -webkit-text-stroke-width: 0px;
background-color: rgb(255, 255, 255);" class="">
<pre class="moz-signature" cols="72" style="font-size: 12px; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; line-height: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px; background-color: rgb(255, 255, 255);">--
Anton</pre>
<span id="cid:9114DCAE-60DE-4666-B021-238BBC0C7221"><Bool_param_for_getCheckIfTracked.patch></span></div>
</blockquote>
</div>
<br class="">
</blockquote>
<br>
<br>
<pre class="moz-signature" cols="72">--
Anton</pre>
</body>
</html>