Revert of GC plugin: improve error reporting when tracing illegal fields.

Revert of GC plugin: improve error reporting when tracing illegal fields. (patchset #2 id:20001 of https://codereview.chromium.org/2060553002/ )

Reason for revert:
New test fails: https://build.chromium.org/p/chromium.fyi/builders/ClangToTMac%20%28dbg%29/builds/5481/steps/gclient%20runhooks/logs/stdio

Original issue's description:
> GC plugin: improve error reporting when tracing illegal fields.
>
> Add detection of trace() calls over smart pointer types that either do not
> wrap up references to heap objects, or are otherwise not meant to be traced
> over. In particular, CrossThread(Weak)Persistent<T> fields are now detected
> as being illegal to trace over. Also consider OwnPtr<T>, RefPtr<T> and
> std::unique_ptr<T> as illegal to trace over & emit a more concise error
> messages for these.
>
> R=
> BUG=619149
>
> Committed: https://crrev.com/3ba6089cd6a901b62ff5a0d8f08a2bd818edcbe8
> Cr-Commit-Position: refs/heads/master@{#399861}

TBR=oilpan-reviews@chromium.org,sigbjornf@opera.com
# Skipping CQ checks because original CL landed less than 1 days ago.
NOPRESUBMIT=true
NOTREECHECKS=true
NOTRY=true
BUG=619149
Committed: https://crrev.com/fd0eaf988b6b216d51e3eb9dde44b4df4f42c3a3
Cr-Commit-Position: refs/heads/master@{#399868}

[Nico, 2016-06-15 09:38:02]

The CQ bit was checked by thakis@chromium.org

[Nico, 2016-06-15 09:38:03]

Created Revert of GC plugin: improve error reporting when tracing illegal
fields.

[commit-bot: I haz the power, 2016-06-15 09:38:16]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2068983003/1

[commit-bot: I haz the power, 2016-06-15 09:39:12]

Committed patchset #1 (id:1)

[haraken, 2016-06-15 09:39:12]

LGTM

[commit-bot: I haz the power, 2016-06-15 09:40:23]

Description was changed from

==========
Revert of GC plugin: improve error reporting when tracing illegal fields.
(patchset #2 id:20001 of https://codereview.chromium.org/2060553002/ )

Reason for revert:
New test fails:
https://build.chromium.org/p/chromium.fyi/builders/ClangToTMac%20%28dbg%29/bu...

Original issue's description:
> GC plugin: improve error reporting when tracing illegal fields.
> 
> Add detection of trace() calls over smart pointer types that either do not
> wrap up references to heap objects, or are otherwise not meant to be traced
> over. In particular, CrossThread(Weak)Persistent<T> fields are now detected
> as being illegal to trace over. Also consider OwnPtr<T>, RefPtr<T> and
> std::unique_ptr<T> as illegal to trace over & emit a more concise error
> messages for these.
> 
> R=
> BUG=619149
> 
> Committed: https://crrev.com/3ba6089cd6a901b62ff5a0d8f08a2bd818edcbe8
> Cr-Commit-Position: refs/heads/master@{#399861}

TBR=oilpan-reviews@chromium.org,sigbjornf@opera.com
# Skipping CQ checks because original CL landed less than 1 days ago.
NOPRESUBMIT=true
NOTREECHECKS=true
NOTRY=true
BUG=619149
==========

to

==========
Revert of GC plugin: improve error reporting when tracing illegal fields.
(patchset #2 id:20001 of https://codereview.chromium.org/2060553002/ )

Reason for revert:
New test fails:
https://build.chromium.org/p/chromium.fyi/builders/ClangToTMac%20%28dbg%29/bu...

Original issue's description:
> GC plugin: improve error reporting when tracing illegal fields.
>
> Add detection of trace() calls over smart pointer types that either do not
> wrap up references to heap objects, or are otherwise not meant to be traced
> over. In particular, CrossThread(Weak)Persistent<T> fields are now detected
> as being illegal to trace over. Also consider OwnPtr<T>, RefPtr<T> and
> std::unique_ptr<T> as illegal to trace over & emit a more concise error
> messages for these.
>
> R=
> BUG=619149
>
> Committed: https://crrev.com/3ba6089cd6a901b62ff5a0d8f08a2bd818edcbe8
> Cr-Commit-Position: refs/heads/master@{#399861}

TBR=oilpan-reviews@chromium.org,sigbjornf@opera.com
# Skipping CQ checks because original CL landed less than 1 days ago.
NOPRESUBMIT=true
NOTREECHECKS=true
NOTRY=true
BUG=619149

Committed: https://crrev.com/fd0eaf988b6b216d51e3eb9dde44b4df4f42c3a3
Cr-Commit-Position: refs/heads/master@{#399868}
==========

[commit-bot: I haz the power, 2016-06-15 09:40:25]

Patchset 1 (id:??) landed as
https://crrev.com/fd0eaf988b6b216d51e3eb9dde44b4df4f42c3a3
Cr-Commit-Position: refs/heads/master@{#399868}

[sof, 2016-06-15 14:45:06]

lgtm, sorry about the breakage - taking a look.

[sof, 2016-06-15 19:00:49]

On 2016/06/15 14:45:06, sof wrote:
> lgtm, sorry about the breakage - taking a look.

Hmm, I can't reproduce this on a couple of boxes when building clang from ToT.
i.e., the discrepancy here is that the bot is reporting no errors when using
std::unique_ptr<>s over GC types, but local builds are. As they should.

If I do the reasonable thing of removing the dummy definition of
std::unique_ptr<> from heap/stubs.h and include <memory> straight, local builds
behave like the bot and do not detect the std::unique_ptr<> cases. It looks like
the assumed singleton instance of a NamespaceDecl for "std" doesn't hold -- if I
add the assert

 assert(ns->getName() == "std" || ns == sema.getStdNamespace());

it fails. Hence I think we need to drop back to performing name comparisons when
resolving std::unique_ptr<> usages.

[Nico, 2016-06-15 21:09:23]

On 2016/06/15 19:00:49, sof wrote:
> On 2016/06/15 14:45:06, sof wrote:
> > lgtm, sorry about the breakage - taking a look.
> 
> Hmm, I can't reproduce this on a couple of boxes when building clang from ToT.
> i.e., the discrepancy here is that the bot is reporting no errors when using
> std::unique_ptr<>s over GC types, but local builds are. As they should.
> 
> If I do the reasonable thing of removing the dummy definition of
> std::unique_ptr<> from heap/stubs.h and include <memory> straight, local
builds
> behave like the bot and do not detect the std::unique_ptr<> cases. It looks
like
> the assumed singleton instance of a NamespaceDecl for "std" doesn't hold -- if
I
> add the assert
> 
>  assert(ns->getName() == "std" || ns == sema.getStdNamespace());
> 
> it fails. Hence I think we need to drop back to performing name comparisons
when
> resolving std::unique_ptr<> usages.

Ah right, I think you need something like

      bool IsInStd = false;
      for (NamespaceDecl *ND = dyn_cast<NamespaceDecl>(R->getDeclContext());
           ND && !IsInStd; ND = dyn_cast<NamespaceDecl>(ND->getParent())) {
        if (SemaRef.getStdNamespace()->InEnclosingNamespaceSetOf(ND))
          IsInStd = true;
      }

(behind the scenes this calls DeclContext::Equals() which compares the
getPrimaryContext() of both contexts instead of the contexts themselves). To
repro in a test, `namespace std {} namespace std { /* unique_ptr in second
namespace */}` will probably do the trick

[Nico, 2016-06-15 21:10:18]

ps: great tool for going through clang code:
http://llvm-cs.pcc.me.uk/?q=InEnclosingNamespaceSetOf

[sof, 2016-06-16 06:19:10]

On 2016/06/15 21:09:23, Nico (traveling...slow) wrote:
> On 2016/06/15 19:00:49, sof wrote:
> > On 2016/06/15 14:45:06, sof wrote:
> > > lgtm, sorry about the breakage - taking a look.
> > 
> > Hmm, I can't reproduce this on a couple of boxes when building clang from
ToT.
> > i.e., the discrepancy here is that the bot is reporting no errors when using
> > std::unique_ptr<>s over GC types, but local builds are. As they should.
> > 
> > If I do the reasonable thing of removing the dummy definition of
> > std::unique_ptr<> from heap/stubs.h and include <memory> straight, local
> builds
> > behave like the bot and do not detect the std::unique_ptr<> cases. It looks
> like
> > the assumed singleton instance of a NamespaceDecl for "std" doesn't hold --
if
> I
> > add the assert
> > 
> >  assert(ns->getName() == "std" || ns == sema.getStdNamespace());
> > 
> > it fails. Hence I think we need to drop back to performing name comparisons
> when
> > resolving std::unique_ptr<> usages.
> 
> Ah right, I think you need something like
> 
>       bool IsInStd = false;
>       for (NamespaceDecl *ND = dyn_cast<NamespaceDecl>(R->getDeclContext());
>            ND && !IsInStd; ND = dyn_cast<NamespaceDecl>(ND->getParent())) {
>         if (SemaRef.getStdNamespace()->InEnclosingNamespaceSetOf(ND))
>           IsInStd = true;
>       }
> 
> (behind the scenes this calls DeclContext::Equals() which compares the
> getPrimaryContext() of both contexts instead of the contexts themselves). To
> repro in a test, `namespace std {} namespace std { /* unique_ptr in second
> namespace */}` will probably do the trick

Thanks, I couldn't find any form of 'deeper' equality checking over
NamespaceDecls when (randomly) looking. Adopted the above in
https://codereview.chromium.org/2060553002/