<html><head><meta http-equiv="Content-Type" content="text/html charset=utf-8"></head><body style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space;" 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><blockquote type="cite" class=""><div class="">On Mar 6, 2015, at 7:03 AM, Anton Yartsev <<a 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><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><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></body></html>