<html>
  <head>
    <meta content="text/html; charset=ISO-8859-1"
      http-equiv="Content-Type">
  </head>
  <body text="#000000" bgcolor="#FFFFFF">
    <div class="moz-cite-prefix">On 28.03.2013 1:18, Anna Zaks wrote:<br>
    </div>
    <blockquote
      cite="mid:FC08E2B0-E6FA-4C7A-930F-AF2F3F9C4CBA@apple.com"
      type="cite">
      <meta http-equiv="Content-Type" content="text/html;
        charset=ISO-8859-1">
      <br>
      <div>
        <div>On Mar 27, 2013, at 10:13 AM, Anton Yartsev <<a
            moz-do-not-send="true" href="mailto:anton.yartsev@gmail.com">anton.yartsev@gmail.com</a>>
          wrote:</div>
        <br class="Apple-interchange-newline">
        <blockquote type="cite">
          <div text="#000000" bgcolor="#FFFFFF" style="letter-spacing:
            normal; orphans: auto; text-align: start; text-indent: 0px;
            text-transform: none; white-space: normal; widows: auto;
            word-spacing: 0px; -webkit-text-size-adjust: auto;
            -webkit-text-stroke-width: 0px;">
            <div class="moz-cite-prefix">On 27.03.2013 20:58, Anna Zaks
              wrote:<br>
            </div>
            <blockquote
              cite="mid:DFCEC3A6-7D53-43D0-9FFE-FCAC94006ADF@apple.com"
              type="cite"><br>
              <div>
                <div>On Mar 26, 2013, at 6:10 PM, Anna Zaks <<a
                    moz-do-not-send="true" href="mailto:ganna@apple.com">ganna@apple.com</a>>
                  wrote:</div>
                <br class="Apple-interchange-newline">
                <blockquote type="cite">
                  <div 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-size-adjust: auto;
                    -webkit-text-stroke-width: 0px;">
                    <div><br class="Apple-interchange-newline">
                      On Mar 26, 2013, at 5:10 PM, Anton Yartsev <<a
                        moz-do-not-send="true"
                        href="mailto:anton.yartsev@gmail.com">anton.yartsev@gmail.com</a>>
                      wrote:</div>
                    <br class="Apple-interchange-newline">
                    <blockquote type="cite">
                      <div text="#000000" bgcolor="#FFFFFF"
                        style="letter-spacing: normal; orphans: auto;
                        text-align: start; text-indent: 0px;
                        text-transform: none; white-space: normal;
                        widows: auto; word-spacing: 0px;
                        -webkit-text-size-adjust: auto;
                        -webkit-text-stroke-width: 0px;">
                        <div class="moz-cite-prefix">On 25.03.2013
                          21:39, Anna Zaks wrote:<br>
                        </div>
                        <blockquote
                          cite="mid:9084B92B-44BB-497B-9F7D-2A1AAA599822@apple.com"
                          type="cite">
                          <div>On Mar 25, 2013, at 9:30 AM, Jordan Rose
                            <<a moz-do-not-send="true"
                              href="mailto:jordan_rose@apple.com">jordan_rose@apple.com</a>>
                            wrote:</div>
                          <br class="Apple-interchange-newline">
                          <blockquote type="cite">
                            <div 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-size-adjust: auto;
                              -webkit-text-stroke-width: 0px;">
                              <div><br class="Apple-interchange-newline">
                                On Mar 25, 2013, at 8:01 , Anton Yartsev
                                <<a moz-do-not-send="true"
                                  href="mailto:anton.yartsev@gmail.com">anton.yartsev@gmail.com</a>>
                                wrote:</div>
                              <br class="Apple-interchange-newline">
                              <blockquote type="cite">
                                <div text="#000000" bgcolor="#FFFFFF">
                                  <div class="moz-cite-prefix">Committed
                                    as r177849</div>
                                  <br>
                                  Manage to find several random real
                                  bugs (report-843813.html,
                                  report-230257.html, recursive case in
                                  report-727931.html), but for now it is
                                  hard to detect real bugs among tons of
                                  false-positives.<br>
                                  <br>
                                  There are two types of false-positives
                                  that form the majority of reports:<br>
                                  1) Illustrated by the following test
                                  (added similar test to
                                  NewDelete-checker-test.mm):<br>
                                  int *global;<br>
                                  void testMemIsOnHeap() {<br>
                                    int *p = new int; // FIXME:
                                  currently not heap allocated!<span
                                    class="Apple-converted-space"> </span><br>
                                    if (global != p) // evaluates to
                                  UnknownVal() rather then 'true'<br>
                                      global = p;<br>
                                  } // report false-positive leak<br>
                                  <br>
                                  As I understand the problem is that
                                  currently a memory region for 'new' is
                                  not a heap region and this lead to
                                  false-positives like
                                  report-863595.html and others. (e.g.
                                  that causes 'global != p' evaluate to
                                  UnknownVal() rather then 'true' (logic
                                  of SimpleSValBuilder::evalBinOpLL))<br>
                                  <br>
                                  Attached is the proposed patch that
                                  fixes these issues.     </div>
                              </blockquote>
                              <div><br>
                              </div>
                              <div>There are two reasons I didn't use
                                getConjuredHeapSymbol when I originally
                                put in this code:</div>
                              <div>(1) It handles all CXXNewExprs, even
                                if the allocator is not one of the
                                global ones.</div>
                              <div>(2) Heap symbols weren't used yet
                                (Anna added them later for
                                MallocChecker).</div>
                              <div><br>
                              </div>
                              <div>Obviously #2 is bogus now. #1 worries
                                me a bit because it requires duplicating
                                some of the checks you just added to
                                MallocChecker.</div>
                              <div><br>
                              </div>
                              <div>In the long run, the design would be
                                to get the appropriate memory from the
                                allocator call, and we have PR12014's
                                restructuring of the CFG blocking that.
                                I'm not sure if we'd then move the heap
                                symbol logic into a checker, or if it
                                would still stay in Core.</div>
                              <div><br>
                              </div>
                              <div>In the short term, I guess the best
                                idea is to duplicate some of the checks
                                (or refactor them to a helper function
                                somewhere...though probably<span
                                  class="Apple-converted-space"> </span><i>not</i> into
                                AST) and then conjure a heap symbol if
                                we know we can.</div>
                            </div>
                          </blockquote>
                        </blockquote>
                        Failed to find any suitable place other then AST
                        :) Eventually noticed, that actually only a
                        single check should be duplicated. Decided to
                        leave the wide comment added when I tried to
                        find the proper place for isStandardNewDelete().<br>
                        New fix attached.<br>
                        <br>
                        <blockquote
                          cite="mid:9084B92B-44BB-497B-9F7D-2A1AAA599822@apple.com"
                          type="cite">
                          <blockquote type="cite">
                            <div 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-size-adjust: auto;
                              -webkit-text-stroke-width: 0px;">
                              <div><br>
                              </div>
                              <br>
                              <blockquote type="cite">
                                <div text="#000000" bgcolor="#FFFFFF">2)
                                  The second type of false-positives is
                                  illustrated by the following minimal
                                  test:<br>
                                  void f(const int & x);<br>
                                  <br>
                                  void test() {<br>
                                    int *p = (int *)malloc(sizeof(int));<br>
                                    f(*p);<br>
                                  } // report false-positive leak<br>
                                  <br>
                                  report-218274.html shows how it looks
                                  like in reality.<br>
                                  Haven't addressed this yet. Removing
                                  'const' from the declaration
                                  eliminates the leak report.     </div>
                              </blockquote>
                              <div><br>
                              </div>
                              <div>Interesting. You can't change a const
                                region (and pedantically you can't
                                free() it either), but you certainly can
                                'delete' it. (C++11 [expr.delete]p2)</div>
                              <div><br>
                              </div>
                              <div>Anna, any thoughts on this? Should
                                these also count as "pointer escaped"
                                even though their contents have not been
                                invalidated?</div>
                              <div><br>
                              </div>
                            </div>
                          </blockquote>
                          <div><br>
                          </div>
                          <div>I think handling this similarly to
                            pointer escape is the best. However, I am
                            concerned that if we simply extend pointer
                            escape to trigger on another "Kind" of
                            escape, all the other users of pointerEscape
                            will need to know about it (and do nothing
                            for this kind of escape). How about a new
                            checker callback, which will rely on the
                            same core code as _checkPointerEscape<span
                              style="font-family: Menlo;">? </span>This
                            way the checker developers would not need to
                            know about this special case, and we can
                            reuse all the code that determines when the
                            escape should be triggered.</div>
                        </blockquote>
                        As for me it would be better to leave the single
                        callback as this is just another type of pointer
                        escape, if I understand correctly. Have no other
                        arguments for this :)</div>
                    </blockquote>
                    <blockquote type="cite">
                      <div text="#000000" bgcolor="#FFFFFF"
                        style="letter-spacing: normal; orphans: auto;
                        text-align: start; text-indent: 0px;
                        text-transform: none; white-space: normal;
                        widows: auto; word-spacing: 0px;
                        -webkit-text-size-adjust: auto;
                        -webkit-text-stroke-width: 0px;">In addition,
                        the "Kind" approach is relatively new, so
                        hopefully a few users of pointerEscape be
                        affected.<span class="Apple-converted-space"> </span><br>
                      </div>
                    </blockquote>
                    The main concern is that every user of the callback
                    will now have to check if the kind is ConstPointer
                    (or whatever we call it, maybe multiple kinds..) and
                    do nothing in that case. So every user will need to
                    know about this special case and handle it
                    specially.<br>
                  </div>
                </blockquote>
                <div>Anton,</div>
                <div><br>
                </div>
                <div>If you don't mind, I am going to work on this one.
                  I have some spare time and we'd like to get the
                  new/delete checker out in the next open source build.<span
                    class="Apple-converted-space"> </span><br>
                </div>
              </div>
            </blockquote>
            I am OK with this, thanx!<br>
            <br>
          </div>
        </blockquote>
        Anton,</div>
      <div><br>
      </div>
      <div>I've realized that I need the part of your new/delete work
        that tracks families for fixing this. Specifically, I want to
        assume that a const pointer cannot be freed but could be
        deleted. Can you commit the remaining patches? Specifically, I
        need the part that performs family tracking, but you can commit
        the mismatched deallocators work as well.</div>
    </blockquote>
    Committed at r178250<br>
    <pre class="moz-signature" cols="72">-- 
Anton</pre>
  </body>
</html>