<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 10.03.2015 1:02, Anna Zaks wrote:<br>
</div>
<blockquote
cite="mid:21EB5EAE-3606-4940-AECB-BDC424A9A695@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 7, 2015, at 12:43 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 07.03.2015 8:35, Anna Zaks
wrote:<br class="">
</div>
<blockquote
cite="mid:D89304E4-8661-42AF-AA0E-7F89A45DF7D0@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 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 class="">
<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 class="">+
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
class="">
<br class="">
</div>
</div>
</blockquote>
<div><br class="">
</div>
I think that in printState, we want to check both: if the symbol
is tracked by a leak check and if the symbol is tracked by a
non-leak check, so calling the method twice makes sense.</div>
</blockquote>
Done! Committed as r231863.<br>
<br>
<blockquote
cite="mid:21EB5EAE-3606-4940-AECB-BDC424A9A695@apple.com"
type="cite">
<div><br class="">
<blockquote type="cite" class="">
<div class="">
<div bgcolor="#FFFFFF" text="#000000" class="">
<blockquote
cite="mid:D89304E4-8661-42AF-AA0E-7F89A45DF7D0@apple.com"
type="cite" class="">
<div class="">
<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"
class=""><Bool_param_for_getCheckIfTracked.patch></span></div>
</blockquote>
</div>
<br class="">
</blockquote>
<br class="">
<br class="">
<pre class="moz-signature" cols="72">--
Anton</pre>
</div>
</div>
</blockquote>
</div>
<br class="">
</blockquote>
<br>
<br>
<pre class="moz-signature" cols="72">--
Anton</pre>
</body>
</html>