<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 27.03.2013 20:58, Anna Zaks wrote:<br>
    </div>
    <blockquote
      cite="mid:DFCEC3A6-7D53-43D0-9FFE-FCAC94006ADF@apple.com"
      type="cite">
      <meta http-equiv="Content-Type" content="text/html;
        charset=ISO-8859-1">
      <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. <br>
        </div>
      </div>
    </blockquote>
    I am OK with this, thanx!<br>
    <br>
    <blockquote
      cite="mid:DFCEC3A6-7D53-43D0-9FFE-FCAC94006ADF@apple.com"
      type="cite">
      <div>
        <div><br>
        </div>
        Cheers,</div>
      <div>Anna.</div>
      <div><br>
        <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;">
            <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;"><br>
                <br>
                Evolved another issue, that I first thought to be
                related to case 1), here is the minimal test:<br>
                struct S {<br>
                  int **p;<br>
                };<br>
                void testOk(S &s) {<br>
                  new(s.p) (int*)(new int); // false-positive leak<br>
                }<br>
                void testLeak() {<br>
                  S s;<br>
                  new(s.p) (int*)(new int); // real leak<br>
                }<br>
                <br>
                Haven't addressed these yet. The leak is reported for
                cases of the form 'small_vector.push_back(new
                Something)', where push_back() uses placement new to
                store data.<br>
                <br>
                <pre class="moz-signature" cols="72">-- 
Anton</pre>
                <span><FixForCase1.patch></span></div>
            </blockquote>
          </div>
          <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-size-adjust: auto;
            -webkit-text-stroke-width: 0px;">
          <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-size-adjust: auto;
            -webkit-text-stroke-width: 0px; float: none; display: inline
            !important;">_______________________________________________</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-size-adjust: auto; -webkit-text-stroke-width:
            0px;">
          <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-size-adjust: auto;
            -webkit-text-stroke-width: 0px; float: none; display: inline
            !important;">cfe-commits mailing list</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-size-adjust: auto; -webkit-text-stroke-width:
            0px;">
          <a moz-do-not-send="true"
            href="mailto:cfe-commits@cs.uiuc.edu" 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;">cfe-commits@cs.uiuc.edu</a><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-size-adjust: auto; -webkit-text-stroke-width:
            0px;">
          <a moz-do-not-send="true"
            href="http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits"
            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;">http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits</a></blockquote>
      </div>
      <br>
    </blockquote>
    <br>
    <pre class="moz-signature" cols="72">-- 
Anton</pre>
  </body>
</html>